diff --git a/CHANGELOG.md b/CHANGELOG.md index 0bf42ace..6b53bef9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,7 +11,110 @@ pre-1.0 crates). These changes require at least a 0.1.0 -> 0.2.0 bump before the next release. -### Changed (API consistency pass) +### Added + +- `NegativeResponse::new_with_sid(request_service_sid, nrc)`, the construction-side counterpart + to `Request::Other { sid }`. `new()` routes through `to_request_sid()`, which collapses every + unmodeled service to `0x7F`, so a server that decoded `Request::Other { sid: 0x40 }` could not + answer `serviceNotSupported` echoing `0x40` — despite this type already preserving such bytes + losslessly on decode. + +- `Request::decode` and `Response::decode` now document that the returned remainder is **always + empty**. A UDS frame is not self-delimiting, so one buffer is one frame and every payload is + decoded with `decode_exact`; feeding concatenated frames (or `DecodeIter`) will treat the whole + buffer as a single frame. + +- `RequestUpload` (0x35 / 0x75) is now modeled, via `RequestUploadRequest` and + `RequestUploadResponse` plus `Request::RequestUpload` / `Response::RequestUpload`. It was the + conspicuous gap in the transfer story: `RequestDownload`, `TransferData` and + `RequestTransferExit` were all modeled, so the download flow was complete while the + structurally identical upload flow decoded only to `Request::Other`. ISO 14229-1 gives the + two services the same message layout, so both pairs are now generated from one macro in + `services/upload_download.rs` — a fix to the address/size width derivation cannot land on one + service and miss the other. The two NRC tables are kept separate so they can diverge later. + +- `Error::negative_response_code()` maps any decode error to the `NegativeResponseCode` a server + should answer with, following ISO 14229-1: `0x13` for a malformed frame, `0x12` for an + unsupported sub-function byte, `0x31` for an out-of-range parameter, and `0x10` only for + `IoError`. Three `Error` variants documented their NRC in prose and eighteen said nothing, so + every server had to re-derive the mapping against a `#[non_exhaustive]` enum. + +- `Request::allowed_nack_codes()` dispatches to the per-service tables from a decoded + `Request`, alongside the existing `service()` and `is_positive_response_suppressed()`. All 15 + request types already exposed the associated function, but reaching it from a `Request` + required matching every variant. Returns an empty slice for `Request::Other`, meaning "NRC set + unknown" rather than "no codes apply". + +- `RequestDownloadRequest::data_format_identifier()`, plus + `DataFormatIdentifier::compression_method()` and `DataFormatIdentifier::encryption_method()`. + The DFI was previously write-only: a server could decode a download request but had no way to + read the compression or encryption method it had been asked to use. + +- `TesterPresentRequest::sub_function()` and `TesterPresentResponse::sub_function()`. + +- **Breaking:** New `Error::TrailingBytes` variant, produced when a decode leaves unconsumed + bytes in the input. Both `Error::InsufficientData` and `Error::TrailingBytes` map to + NRC `0x13` (`IncorrectMessageLengthOrInvalidFormat`). + +- `DtcRecord::high_byte()`, `middle_byte()` and `low_byte()`. The fields are private and the type + had no accessors at all, so a decoded DTC could only be inspected by round-tripping through + `u32` — awkward given ISO 14229-1 Annex D.1 assigns the high byte its own meaning (system + group). All three are `const fn`. + +### Changed + +- **Breaking:** `ClearDiagnosticInfoRequest::memory_selection` is now `Option`, and the + constructors are split accordingly: `new(group_of_dtc)` / `clear_all()` for the ordinary case, + `new_with_memory_selection(group_of_dtc, selection)` / `clear_all_in_memory(selection)` when + addressing user-defined DTC memory. ISO 14229-1:2020 Table 296 marks `MemorySelection` `U` + (user option), so it is absent from the wire unless the client is targeting user-defined + memory — the crate previously required it, which meant the plain 3-byte request (the only + form in the 2013 edition, and the one in the standard's own Table 300 flow example) failed to + decode with `InsufficientData`, while every encode emitted a spurious 4th byte. + +- **Breaking:** `SecurityAccessLevel::value` now takes `&self` instead of `self`, matching the + other twenty accessors in the crate. No call-site change is needed: the type is `Copy`. + +- `DtcRecord::new`, `DtcSnapshotRecordNumber::new`, `DtcExtDataRecordNumber::new`, + `DtcStoredDataRecordNumber::new`, `DataFormatIdentifier::new`, `NegativeResponse::new`, + `NegativeResponse::request_service`, `RequestDownloadRequest::new`, `RequestUploadRequest::new` + and all four `UdsServiceType` SID conversions (`from_request_sid`, `to_request_sid`, + `from_response_sid`, `to_response_sid`) are now `const fn`. The crate was otherwise uniformly + `const fn new`, and the gaps fell exactly on the primitives a caller wants in a `const` table: + DTC constants, record numbers, the format identifier, and the SID map a server dispatch table + is built from. `NegativeResponse::new` was blocked only because `to_request_sid` was not const. + +- The DTC iterators now implement `size_hint` (exact) and + [`FusedIterator`](core::iter::FusedIterator). They deliberately do **not** implement + `ExactSizeIterator`: its `len()` would have to count items yielded, which exceeds the + complete-record count when a partial tail is present, contradicting the inherent `len()`. + Documented on each type. + +- **Breaking:** `DtcSeverityAndStatusIter` -> `WwhObdDtcSeverityIter`, and + `ReadDtcInfoResponse::severity_and_status_iter` -> `wwh_obd_dtc_severity_iter`. The old name pointed at + the wrong variant: it reads as "the severity iterator" but only handles the 5-byte records of + `WwhObdDtcByMaskRecord` (0x42), while the 0x08/0x09 `DtcSeverityList` records are 6 bytes with + an extra functional-unit byte. The `DtcSeverityList` doc previously had to carry a warning that + the iterator did not apply to it. + +- **Breaking:** `DataFormatIdentifier::new` now takes its arguments in **wire order** — + `new(compression_method, encryption_method)`, compression being the high nibble. It previously + took encryption first, contradicting both the wire layout and the type's own doc comment, and + since both parameters are `u8` the compiler could not catch a transposition. **Review any + call site passing two different non-zero values.** `From` is unaffected and remains the + usual path. Added `DataFormatIdentifier::NONE` for the common no-compression/no-encryption case. + +- **Breaking:** `CommunicationControlRequest::suppress_positive_response()` is now a public + field, matching the other six suppressable requests. The type stays encapsulated, but because + of the `control_type`/`node_id` invariant — `node_id` must be present exactly when + `control_type` is an enhanced-address variant — not because of SPRMIB, which is independent and + fused onto the sub-function byte only at the wire boundary. `control_type()` remains a getter. + +- **Breaking:** `ReadDataByIdentifierResponse::records()` is now the public field `records`, + matching every other opaque response slice. It carries no invariant. + +- `CommunicationControlResponse::control_type` (public) and `NegativeResponse`'s private fields + are both deliberate and now documented, so the remaining asymmetry is not read as an oversight. - **Breaking:** Acronyms in type and variant names now follow the Rust API guideline ([C-CASE](https://rust-lang.github.io/api-guidelines/naming.html)): `Dtc`, `Uds`, `Ecu`, @@ -51,8 +154,8 @@ These changes require at least a 0.1.0 -> 0.2.0 bump before the next release. `Iso26021_2Values`. (`non_camel_case_types` permits `_` between digits, so no `allow` is needed.) Trailing sequence numbers do lose theirs: `SAE_J2012_DA_DTCFormat_00` -> `SaeJ2012DaDtcFormat00`. - - `FunctionalGroupIdentifier::VODBSystem` -> `VobdSystem`. The old name was a - transposition typo; ISO 14229-1 Table D.1 names 0xFE `VOBDSystem`. + - `FunctionalGroupIdentifier::VODBSystem` -> `VobdSystem`, which also corrects a typo — + see *Fixed* below. All three `#[allow(non_camel_case_types)]` attributes in the crate are now gone. @@ -71,57 +174,129 @@ These changes require at least a 0.1.0 -> 0.2.0 bump before the next release. `NegativeResponse`/`UnsupportedDiagnosticService`, and point at `Request::Other` / `Response::Other` for lossless pass-through of unmodeled services. -- **Breaking:** `#[non_exhaustive]` added to eleven public types that ISO will grow into, so - that later additions are not breaking changes: `ReadDtcInfoSubFunction`, `FileOperationMode`, - `DtcExtDataRecordNumber`, `DtcSnapshotRecordNumber`, `DtcFaultDetectionCounterRecord`, - `SizePayload`, `NamePayload`, `SentDataPayload`, `FileSizePayload`, `DirSizePayload`, - `PositionPayload`. Downstream `match` statements over these need a wildcard arm, and the - seven structs must be built through `new()` rather than a struct literal. - `ReadDtcInfoSubFunction` is the important one: it has sub-functions the crate does not model - yet, so adding one post-tag would otherwise have been breaking. +- **Breaking:** `Error::InsufficientData` now carries an `automotive_wire_codec::Incomplete` + (with `needed` and `available` byte counts) instead of a bare `usize`. + +- `automotive-wire-codec` is now a public dependency: its `Incomplete` and `TrailingBytes` + types are re-exported at the crate root (`uds_protocol::{Incomplete, TrailingBytes}`) and + are considered part of `uds_protocol`'s public API. A semver-major release of + `automotive-wire-codec` is therefore a breaking change for `uds_protocol`. + +- **Breaking:** `uds_protocol::{Decode, DecodeIter, Encode}` are now re-exports of the + `automotive-wire-codec` 0.3 traits (previously crate-local traits). This is the underlying + cause of most of the other breaking changes in this release. + +- **Breaking:** `Encode::encoded_size` is now `Result` (previously infallible + `usize`), via the codec trait's correct-by-construction counting-sink default. Crate-local + `encoded_size` overrides have been removed; callers must handle/unwrap the `Result`. + +- **Breaking:** Added `Error::InvalidWidth`, produced when a wire-declared variable-width field + requests a byte width the target type cannot hold. The underlying `automotive_wire_codec::InvalidWidth` + fragment is also re-exported at the crate root (alongside `Incomplete` and `TrailingBytes`). + +- **Breaking:** `decode_exact` trailing-bytes now surface as `Error::TrailingBytes` instead of + `Error::IncorrectMessageLengthOrInvalidFormat` (both still map to NRC 0x13). + +- This release remains a semver-major bump. ### Fixed +- Documentation corrections across the shared format identifiers and DTC record numbers: + `DataFormatIdentifier` named only `RequestDownloadRequest` (it is also used by + `RequestUploadRequest` and four `RequestFileTransferRequest` variants) and pointed at a + `data_format_identifier` *field* that is now private behind an accessor; the same staleness + affected `MemoryFormatIdentifier` and `LengthFormatIdentifier`. `DtcStoredDataRecordNumber` + described itself as a `DTCSnapshot` record, and its `new()` had an empty summary line and a + malformed `Error::ReservedForLegislativeUse` link. `DtcSettingType` was the only type whose doc + comment sat after its derives. Two redundant intra-doc link targets in + `communication_control.rs` are gone, so `cargo doc --document-private-items` is now clean. + +- **Breaking:** `ReadDtcInfoResponse::decode` now rejects a record list whose length is not a + whole number of records, with `Error::IncorrectMessageLengthOrInvalidFormat` (NRC `0x13`). + It previously passed the tail through verbatim, so a malformed frame decoded successfully and + only failed later, during iteration. This is the same strictness the crate already applies to + trailing bytes everywhere else. All four record-carrying variants are checked at their own + width: `DtcList` and `DtcFaultDetectionCounterList` at 4 bytes, `DtcSeverityList` at 6, + `WwhObdDtcByMaskRecord` at 5. Empty record lists remain valid — a server with no matching DTCs + answers with the header and no records. Iterators reached from a decoded response therefore + never see a partial tail; a hand-constructed variant still can, so they keep their `Result` + item type. + +- **All three DTC iterators looped forever on a partial trailing record.** `next()` returned + `Some(Err(..))` without advancing, so the error was yielded indefinitely: `for` loops and + `count()` hung, and `collect::>>()` allocated without bound. Reachable from + untrusted wire input, because `ReadDtcInfoResponse::decode` passes the record tail through + verbatim without checking that it divides evenly: + + ```rust + let (resp, _) = Response::decode(&[0x59, 0x02, 0xFF, 0x01, 0x02])?; // 2 leftover bytes + for r in resp.dtc_and_status_iter().unwrap() { /* never returns */ } + ``` + + `collect_all()` was the only safe path, and only incidentally — + `collect::>()` short-circuits on the first error. The fuzz targets missed it + because they call `decode` and never drive the iterators. Each iterator now consumes the + partial tail, reporting the error exactly once and terminating. + +- **Breaking:** `TesterPresentRequest` no longer rewrites reserved sub-function bytes. + `[0x3E, 0x01]` decoded and re-encoded as `[0x3E, 0x00]`: the reserved value was parsed and + then discarded. The normalization was deliberate, but it left the service inconsistent with + its own response type, which retains the same values, and with every other service + (`ResetType::IsoSaeReserved`, `DiagnosticSessionType::IsoSaeReserved`, ...). The value is now + retained in a private field — callers still cannot mint a reserved value, `new(suppress)` + keeps its signature and its `0x00` encoding — so a server can report + `subFunctionNotSupported` naming the byte it actually received. + +- `TesterPresentResponse::new()` is now `const`, the last `new()` in the crate that was not. + +- The README service table was missing rows for `DynamicallyDefinedDataIdentifier` (0x2C) and + `AccessTimingParameter` (0x83), both enumerated in `UdsServiceType`, and named two services + differently from the code (`ECUReset`, `ControlDTCSetting`). The table's 27 rows now match the + 27 request SIDs in `UdsServiceType` exactly. + +- **Breaking:** The `utoipa` and `clap` features now imply `std`. Neither compiled without it: + their derive macros expand to `std::`, `String` and `Vec` paths inside this crate, so + `cargo build --no-default-features --features utoipa` failed with 318 resolution errors, as + did the `clap` equivalent. Only the `--all-features` / `--no-default-features` / + `--no-default-features --features alloc` combinations were ever built, so the optional + integrations were never exercised in isolation. + +- The `serde` feature now works on a bare-metal target. The dependency was declared with + serde's default features on, which pulls `serde/std`, so + `cargo build --no-default-features --features serde --target thumbv6m-none-eabi` failed even + though every host-side build passed — a host build proves nothing here, because the host has + `std` available for serde to compile against regardless of this crate being `#![no_std]`. + serde is now wired `default-features = false`, picking up its `alloc` and `std` layers + through weak `serde?/alloc` and `serde?/std` features only when this crate's own `alloc`/`std` + features are enabled. + +- `FunctionalGroupIdentifier::VODBSystem` is now `VobdSystem`. Beyond the casing change, the + old name transposed the letters: ISO 14229-1 Table D.1 names functional group `0xFE` + `VOBDSystem` (vehicle OBD system). + - `DtcFaultDetectionCounterRecord` is now exported from the crate root. It is the `Item` of the public `DtcFaultDetectionIter`, but had no public path, so callers could iterate it and read its fields yet could not name the type — no `Vec`, no struct field, no function signature. Its two `pub` fields are also documented now; `missing_docs` had never fired on an unreachable type. + - Four private type aliases no longer appear in public signatures, where rustdoc rendered them as unlinkable names: `DTCFaultDetectionCounter`, `MemorySelection` and `DTCReadinessGroupIdentifier` (all `= u8`) are spelled `u8` with the meaning moved into the field and variant docs, and `DTCStatusAvailabilityMask` (`= DtcStatusMask`) is spelled `DtcStatusMask` with its "bits on = supported by server" semantics moved onto the four `status_availability_mask` fields. This closes the `TODO` above sub-function `0x18`. -- `RequestTransferExitRequest` and `RequestTransferExitResponse` now derive `serde` and - `utoipa` support like every other public request/response type. Enabling the `serde` feature - previously left these two types unserializable. ### Removed - The `serde_bytes` optional dependency. The `serde` feature activated it, but the crate never referenced it. -### Changed - -- **Breaking:** `Error::InsufficientData` now carries an `automotive_wire_codec::Incomplete` - (with `needed` and `available` byte counts) instead of a bare `usize`. -- `automotive-wire-codec` is now a public dependency: its `Incomplete` and `TrailingBytes` - types are re-exported at the crate root (`uds_protocol::{Incomplete, TrailingBytes}`) and - are considered part of `uds_protocol`'s public API. A semver-major release of - `automotive-wire-codec` is therefore a breaking change for `uds_protocol`. - -### Added - -- **Breaking:** New `Error::TrailingBytes` variant, produced when a decode leaves unconsumed - bytes in the input. Both `Error::InsufficientData` and `Error::TrailingBytes` map to - NRC `0x13` (`IncorrectMessageLengthOrInvalidFormat`). - -### Removed - - The `byteorder-embedded-io` dependency, superseded by `automotive-wire-codec`. + - **Breaking:** `param_length_u16`/`param_length_u32`/`param_length_u64`/`param_length_u128` have been removed. Use `automotive_wire_codec::minimal_be_len` instead. + - **Breaking:** `uds_protocol`'s `Encode`/`Decode` implementations for the primitive numeric types (`u8, u16, u32, u64, u128, i8, i16, i32, i64, i128, f32, f64`) have been removed. Now that `Encode`/`Decode` are re-exports of `automotive-wire-codec`'s traits, implementing them on @@ -131,17 +306,13 @@ These changes require at least a 0.1.0 -> 0.2.0 bump before the next release. `read_be_uint`/`write_be_uint`/`read_be_uint_into` for variable-width fields) re-exported from `automotive_wire_codec`. -### Changed (migration to `automotive-wire-codec` 0.3) +### CI -- **Breaking:** `uds_protocol::{Decode, DecodeIter, Encode}` are now re-exports of the - `automotive-wire-codec` 0.3 traits (previously crate-local traits). This is the underlying - cause of most of the other breaking changes in this release. -- **Breaking:** `Encode::encoded_size` is now `Result` (previously infallible - `usize`), via the codec trait's correct-by-construction counting-sink default. Crate-local - `encoded_size` overrides have been removed; callers must handle/unwrap the `Result`. -- **Breaking:** Added `Error::InvalidWidth`, produced when a wire-declared variable-width field - requests a byte width the target type cannot hold. The underlying `automotive_wire_codec::InvalidWidth` - fragment is also re-exported at the crate root (alongside `Incomplete` and `TrailingBytes`). -- **Breaking:** `decode_exact` trailing-bytes now surface as `Error::TrailingBytes` instead of - `Error::IncorrectMessageLengthOrInvalidFormat` (both still map to NRC 0x13). -- This release remains a semver-major bump. +- New `features` job running `cargo hack check --feature-powerset --no-dev-deps` (20 + combinations). The previous matrix only ever built `--all-features`, + `--no-default-features`, and `--no-default-features --features alloc`, which is why the + `utoipa`/`clap` breakage went unnoticed. +- The bare-metal job now also builds `serde` and `alloc,serde` for `thumbv6m-none-eabi`, the only + place the serde `default-features` defect was observable. +- Publication is now gated on `no-std` and `features` in addition to the existing jobs. A tag + could previously publish a crate whose `no_std` build or feature graph was broken. diff --git a/Cargo.lock b/Cargo.lock index 75d46930..2269d6f4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -425,16 +425,6 @@ dependencies = [ "serde_derive", ] -[[package]] -name = "serde_bytes" -version = "0.11.19" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a5d440709e79d88e51ac01c4b72fc6cb7314017bb7da9eeff678aa94c10e3ea8" -dependencies = [ - "serde", - "serde_core", -] - [[package]] name = "serde_core" version = "1.0.228" @@ -528,7 +518,6 @@ dependencies = [ "embedded-io", "proptest", "serde", - "serde_bytes", "thiserror", "utoipa", ] diff --git a/Cargo.toml b/Cargo.toml index fdf17747..a6bd96e2 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -20,11 +20,16 @@ authors = [ [features] default = ["std"] -std = ["alloc", "embedded-io/std", "thiserror/std"] -alloc = ["embedded-io/alloc"] -serde = ["dep:serde", "dep:serde_bytes"] -utoipa = ["dep:utoipa"] -clap = ["dep:clap"] +std = ["alloc", "embedded-io/std", "thiserror/std", "serde?/std"] +alloc = ["embedded-io/alloc", "serde?/alloc"] +# `serde` is the only optional integration usable on a bare-metal target. It is wired as a +# core-only dependency (`default-features = false`) and picks up serde's `alloc`/`std` layers +# only when this crate's own `alloc`/`std` features are on, via weak `serde?/..` features. +serde = ["dep:serde"] +# `utoipa` and `clap` both imply `std`. Their derive macros expand to `std::`, `String` and +# `Vec` paths inside this crate, which cannot compile under `#![no_std]`. +utoipa = ["std", "dep:utoipa"] +clap = ["std", "dep:clap"] [dependencies] automotive-wire-codec = { version = "0.3", default-features = false } @@ -32,8 +37,7 @@ bitmask-enum = "2" embedded-io = { version = "0.7", default-features = false } thiserror = { version = "2", default-features = false } # Optional dependencies -serde = { version = "1", optional = true, features = ["derive"] } -serde_bytes = { version = "0.11", optional = true } +serde = { version = "1", optional = true, default-features = false, features = ["derive"] } utoipa = { version = "5", optional = true } clap = { version = "4", optional = true, features = ["derive"] } diff --git a/README.md b/README.md index 4c887fb5..4144439e 100644 --- a/README.md +++ b/README.md @@ -13,33 +13,35 @@ It is not in a complete state yet with the 0.1.0 release, please check back soon This library provides serialization and deserialization of UDS messages. It is based on the ISO 14229-1:2020 standard. -| Service Name | Request SID | Response SID | Support | -| -------------------------------- | ----------- | ------------ | ------- | -| `DiagnosticSessionControl` | 0x10 | 0x50 | ✓ | -| `ECUReset` | 0x11 | 0x51 | ✓ | -| `ClearDiagnosticInformation` | 0x14 | 0x54 | ✓ | -| `ReadDTCInformation` | 0x19 | 0x59 | Partial | -| `ReadDataByIdentifier` | 0x22 | 0x62 | ✓ | -| `ReadMemoryByAddress` | 0x23 | 0x63 | | -| `ReadScalingDataByIdentifier` | 0x24 | 0x64 | | -| `SecurityAccess` | 0x27 | 0x67 | ✓ | -| `CommunicationControl` | 0x28 | 0x68 | ✓ | -| `Authentication` | 0x29 | 0x69 | | -| `ReadDataByPeriodicIdentifier` | 0x2A | 0x6A | | -| `WriteDataByIdentifier` | 0x2E | 0x6E | ✓ | -| `InputOutputControlByIdentifier` | 0x2F | 0x6F | | -| `RoutineControl` | 0x31 | 0x71 | ✓ | -| `RequestDownload` | 0x34 | 0x74 | ✓ | -| `RequestUpload` | 0x35 | 0x75 | | -| `TransferData` | 0x36 | 0x76 | ✓ | -| `RequestTransferExit` | 0x37 | 0x77 | ✓ | -| `RequestFileTransfer` | 0x38 | 0x78 | ✓ | -| `WriteMemoryByAddress` | 0x3D | 0x7D | | -| `TesterPresent` | 0x3E | 0x7E | ✓ | -| `SecuredDataTransmission` | 0x84 | 0xC4 | | -| `ControlDTCSetting` | 0x85 | 0xC5 | ✓ | -| `ResponseOnEvent` | 0x86 | 0xC6 | | -| `LinkControl` | 0x87 | 0xC7 | | +| Service Name | Request SID | Response SID | Support | +| ---------------------------------- | ----------- | ------------ | ------- | +| `DiagnosticSessionControl` | 0x10 | 0x50 | ✓ | +| `EcuReset` | 0x11 | 0x51 | ✓ | +| `ClearDiagnosticInformation` | 0x14 | 0x54 | ✓ | +| `ReadDTCInformation` | 0x19 | 0x59 | Partial | +| `ReadDataByIdentifier` | 0x22 | 0x62 | ✓ | +| `ReadMemoryByAddress` | 0x23 | 0x63 | | +| `ReadScalingDataByIdentifier` | 0x24 | 0x64 | | +| `SecurityAccess` | 0x27 | 0x67 | ✓ | +| `CommunicationControl` | 0x28 | 0x68 | ✓ | +| `Authentication` | 0x29 | 0x69 | | +| `ReadDataByPeriodicIdentifier` | 0x2A | 0x6A | | +| `DynamicallyDefinedDataIdentifier` | 0x2C | 0x6C | | +| `WriteDataByIdentifier` | 0x2E | 0x6E | ✓ | +| `InputOutputControlByIdentifier` | 0x2F | 0x6F | | +| `RoutineControl` | 0x31 | 0x71 | ✓ | +| `RequestDownload` | 0x34 | 0x74 | ✓ | +| `RequestUpload` | 0x35 | 0x75 | ✓ | +| `TransferData` | 0x36 | 0x76 | ✓ | +| `RequestTransferExit` | 0x37 | 0x77 | ✓ | +| `RequestFileTransfer` | 0x38 | 0x78 | ✓ | +| `WriteMemoryByAddress` | 0x3D | 0x7D | | +| `TesterPresent` | 0x3E | 0x7E | ✓ | +| `AccessTimingParameter` | 0x83 | 0xC3 | | +| `SecuredDataTransmission` | 0x84 | 0xC4 | | +| `ControlDtcSetting` | 0x85 | 0xC5 | ✓ | +| `ResponseOnEvent` | 0x86 | 0xC6 | | +| `LinkControl` | 0x87 | 0xC7 | | ## Integration @@ -83,11 +85,11 @@ you need to keep before the buffer is reused. These services decode into typed \[`Request`\]/\[`Response`\] variants: `DiagnosticSessionControl`, `EcuReset`, `SecurityAccess`, `CommunicationControl`, `TesterPresent`, `ControlDtcSetting`, `ReadDataByIdentifier`, `WriteDataByIdentifier`, `ClearDiagnosticInfo`, `ReadDtcInfo`, -`RoutineControl`, `RequestDownload`, `TransferData`, `RequestTransferExit`, `RequestFileTransfer`, -and `NegativeResponse`. +`RoutineControl`, `RequestDownload`, `RequestUpload`, `TransferData`, `RequestTransferExit`, +`RequestFileTransfer`, and `NegativeResponse`. All other services enumerated in \[`UdsServiceType`\] (e.g. `Authentication`, `ReadMemoryByAddress`, -`RequestUpload`, `ResponseOnEvent`) are not individually modeled. Frames for them decode into +`ResponseOnEvent`) are not individually modeled. Frames for them decode into \[`Request::Other`\] / \[`Response::Other`\], carrying the service type and raw payload bytes for pass-through. diff --git a/fuzz/Cargo.lock b/fuzz/Cargo.lock index adfe21e7..86d9f198 100644 --- a/fuzz/Cargo.lock +++ b/fuzz/Cargo.lock @@ -24,7 +24,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e6cbbb8f56245b5a479b30a62cdc86d26e2f35c2b9f594bc4671654b03851380" dependencies = [ "quote", - "syn", + "syn 2.0.117", ] [[package]] @@ -119,6 +119,36 @@ version = "5.3.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "69cdb34c158ceb288df11e18b4bd39de994f6657d83847bdffdbd7f346754b0f" +[[package]] +name = "serde" +version = "1.0.229" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4148590afebada386688f18773da617792bf2ef03ffc1e4cbd2b1d45b023e0ba" +dependencies = [ + "serde_core", + "serde_derive", +] + +[[package]] +name = "serde_core" +version = "1.0.229" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "67dca2c9c51e58a4791a4b1ed58308b39c64224d349a935ab5039aa360942a48" +dependencies = [ + "serde_derive", +] + +[[package]] +name = "serde_derive" +version = "1.0.229" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e7a5d71263a5a7d47b41f6b3f06ba276f10cc18b0931f1799f710578e2309348" +dependencies = [ + "proc-macro2", + "quote", + "syn 3.0.3", +] + [[package]] name = "shlex" version = "2.0.1" @@ -136,6 +166,17 @@ dependencies = [ "unicode-ident", ] +[[package]] +name = "syn" +version = "3.0.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "53e9bae58849f64dfa4f5d5ae372c8341f7305f82a3868709269343628b659a3" +dependencies = [ + "proc-macro2", + "quote", + "unicode-ident", +] + [[package]] name = "thiserror" version = "2.0.18" @@ -153,7 +194,7 @@ checksum = "ebc4ee7f67670e9b64d05fa4253e753e016c6c95ff35b89b7941d6b856dec1d5" dependencies = [ "proc-macro2", "quote", - "syn", + "syn 2.0.117", ] [[package]] @@ -163,6 +204,7 @@ dependencies = [ "automotive-wire-codec", "bitmask-enum", "embedded-io", + "serde", "thiserror", ] diff --git a/src/dtc/ext_data.rs b/src/dtc/ext_data.rs index 46313bd8..e16eca60 100644 --- a/src/dtc/ext_data.rs +++ b/src/dtc/ext_data.rs @@ -36,7 +36,7 @@ pub enum DtcExtDataRecordNumber { impl DtcExtDataRecordNumber { /// Create a new `DtcExtDataRecordNumber` from a raw byte, mapping it to the correct variant. #[must_use] - pub fn new(value: u8) -> Self { + pub const fn new(value: u8) -> Self { match value { 0x00 | 0xF0..=0xFD => Self::IsoSaeReserved(value), 0x01..=0x8F => Self::VehicleManufacturer(value), diff --git a/src/dtc/snapshot.rs b/src/dtc/snapshot.rs index 78d06383..bcd0d2a2 100644 --- a/src/dtc/snapshot.rs +++ b/src/dtc/snapshot.rs @@ -23,7 +23,7 @@ impl DtcSnapshotRecordNumber { /// (`0x00`/`0xF0`), `All` (`0xFF`), or `Number`. Every byte is accepted (decoding is /// deliberately liberal); no value is rejected. #[must_use] - pub fn new(record_number: u8) -> Self { + pub const fn new(record_number: u8) -> Self { match record_number { 0x00 | 0xF0 => Self::Reserved(record_number), 0xFF => Self::All, diff --git a/src/dtc/status.rs b/src/dtc/status.rs index 90de8b8b..49f54f2d 100644 --- a/src/dtc/status.rs +++ b/src/dtc/status.rs @@ -204,13 +204,32 @@ pub struct DtcRecord { impl DtcRecord { /// Create a `DtcRecord` from its three component bytes. #[must_use] - pub fn new(high_byte: u8, middle_byte: u8, low_byte: u8) -> Self { + pub const fn new(high_byte: u8, middle_byte: u8, low_byte: u8) -> Self { Self { high_byte, middle_byte, low_byte, } } + + /// The high byte, which ISO 14229-1 Annex D.1 uses to identify the system group + /// (powertrain, body, chassis, network). + #[must_use] + pub const fn high_byte(&self) -> u8 { + self.high_byte + } + + /// The middle byte of the DTC number. + #[must_use] + pub const fn middle_byte(&self) -> u8 { + self.middle_byte + } + + /// The low byte of the DTC number, which carries the failure type. + #[must_use] + pub const fn low_byte(&self) -> u8 { + self.low_byte + } } impl From for DtcRecord { @@ -431,19 +450,21 @@ impl<'a> Decode<'a> for DtcSeverityMask { } } -/// Indicates the number of the specific `DTCSnapshot` data record requested -/// Setting to 0xFF will return all `DTCStoredDataRecords` at once +/// Identifies which `DTCStoredDataRecord` is being requested. +/// +/// Setting to `0xFF` will return all `DTCStoredDataRecords` at once. #[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))] #[cfg_attr(feature = "utoipa", derive(utoipa::ToSchema))] #[derive(Clone, Copy, Debug, Eq, PartialEq)] pub struct DtcStoredDataRecordNumber(u8); -// create a constructor for DtcStoredDataRecordNumber impl DtcStoredDataRecordNumber { + /// Create a `DtcStoredDataRecordNumber` from a raw byte, rejecting the values ISO 14229-1 + /// reserves. /// /// # Errors - /// Will return `Err(Error::ReservedForLegislativeUse()` if the record number == 0x00 or 0xF0 - pub fn new(record_number: u8) -> Result { + /// Returns [`Error::ReservedForLegislativeUse`] if the record number is `0x00` or `0xF0`. + pub const fn new(record_number: u8) -> Result { if record_number == 0 || record_number == 0xF0 { return Err(Error::ReservedForLegislativeUse(record_number)); } @@ -551,6 +572,22 @@ mod encode_param_tests { mod dtc_status_tests { use super::*; + #[test] + fn dtc_record_exposes_its_three_wire_bytes() { + // A decoded DtcRecord has to be inspectable byte-wise: D.1 assigns meaning to the + // high byte (system group) separately from the middle and low bytes. + let record = DtcRecord::from(0x12_3456); + assert_eq!(record.high_byte(), 0x12); + assert_eq!(record.middle_byte(), 0x34); + assert_eq!(record.low_byte(), 0x56); + } + + #[test] + fn dtc_record_byte_accessors_are_usable_in_const_context() { + const HIGH: u8 = CLEAR_ALL_DTCS.high_byte(); + assert_eq!(HIGH, 0xFF); + } + #[test] fn status_mask() { let status_mask = DtcStatusMask::TestFailed | DtcStatusMask::PendingDtc; diff --git a/src/error.rs b/src/error.rs index faeb2796..14e2b902 100644 --- a/src/error.rs +++ b/src/error.rs @@ -3,6 +3,8 @@ use automotive_wire_codec::{ }; use thiserror::Error; +use crate::NegativeResponseCode; + /// Errors that can occur during UDS message encoding, decoding, or validation. #[derive(Debug, Error)] #[non_exhaustive] @@ -87,6 +89,74 @@ impl Error { pub(crate) fn io(e: E) -> Self { Self::IoError(e.kind()) } + + /// The [`NegativeResponseCode`] a server should return for this error. + /// + /// A server decodes an inbound request, and on failure has to answer with a negative + /// response. This is that mapping, so callers do not have to re-derive it against a + /// `#[non_exhaustive]` enum they cannot match exhaustively: + /// + /// ``` + /// use uds_protocol::{Decode, NegativeResponse, Request, UdsServiceType}; + /// + /// let frame = [0x11, 0x01, 0xAA]; // EcuReset with a trailing junk byte + /// if let Err(err) = Request::decode(&frame) { + /// let nack = NegativeResponse::new(UdsServiceType::EcuReset, err.negative_response_code()); + /// assert_eq!(u8::from(nack.nrc()), 0x13); + /// } + /// ``` + /// + /// # Classification + /// + /// Following ISO 14229-1: + /// + /// - **`0x13` `incorrectMessageLengthOrInvalidFormat`** — the frame itself is malformed: + /// too short, too long, or a declared width that does not fit. + /// - **`0x12` `subFunctionNotSupported`** — the *sub-function* byte is not a value this + /// service defines. Applies to the services whose sub-function the crate validates: + /// `0x10`, `0x11`, `0x19`, `0x27`, `0x28`, `0x31`, `0x3E`, `0x85`. + /// - **`0x31` `requestOutOfRange`** — a *parameter* (not a sub-function) carries a value + /// outside its permitted range. Note that `communicationType` is a parameter of + /// `CommunicationControl`, not its sub-function, so it lands here rather than on `0x12`. + /// - **`0x10` `generalReject`** — reserved for [`Error::IoError`]. A transport failure is + /// not a protocol error and no other code fits; ISO designates `generalReject` for + /// exactly the case where no other response code meets the implementation's needs. + /// + /// The mapping never returns [`NegativeResponseCode::PositiveResponse`] or any reserved + /// code, so the result is always a legal NRC to put on the wire. + #[must_use] + pub const fn negative_response_code(&self) -> NegativeResponseCode { + match self { + // The frame is malformed. + Self::InsufficientData(_) + | Self::TrailingBytes(_) + | Self::InvalidWidth(_) + | Self::IncorrectMessageLengthOrInvalidFormat + | Self::NoDataAvailable => NegativeResponseCode::IncorrectMessageLengthOrInvalidFormat, + + // The sub-function byte is not a defined value for the service. + Self::InvalidDiagnosticSessionType(_) + | Self::InvalidEcuResetType(_) + | Self::InvalidSecurityAccessType(_) + | Self::InvalidCommunicationControlType(_) + | Self::InvalidTesterPresentType(_) + | Self::InvalidRoutineControlSubFunction(_) + | Self::InvalidDtcSubfunctionType(_) + | Self::InvalidDtcSetting(_) => NegativeResponseCode::SubFunctionNotSupported, + + // A parameter value is outside its permitted range. + Self::InvalidCommunicationType(_) + | Self::InvalidMemoryAddress(_) + | Self::InvalidEncryptionCompressionMethod(_) + | Self::InvalidFileOperationMode(_) + | Self::InvalidFileSizeParameterLength(_) + | Self::InvalidDtcFormatIdentifier(_) + | Self::ReservedForLegislativeUse(_) => NegativeResponseCode::RequestOutOfRange, + + // Transport failure: not a protocol error, so no specific NRC applies. + Self::IoError(_) => NegativeResponseCode::GeneralReject, + } + } } impl From for Error { @@ -138,6 +208,155 @@ impl From for Error { } } +#[cfg(test)] +mod nrc_mapping_tests { + use super::*; + use crate::NegativeResponseCode; + + /// Every variant, with the NRC byte a server should send back. Grouped by the ISO 14229-1 + /// classification the mapping implements: 0x13 for length/format, 0x12 for an unsupported + /// sub-function byte, 0x31 for an in-range-but-unsupported parameter value. + fn cases() -> impl Iterator { + [ + // --- 0x13 incorrectMessageLengthOrInvalidFormat: the frame is malformed --- + ( + Error::InsufficientData(Incomplete { + needed: 4, + available: 1, + }), + 0x13, + "short read", + ), + (Error::TrailingBytes(TrailingBytes(2)), 0x13, "extra bytes"), + ( + Error::InvalidWidth(InvalidWidth { max: 4, got: 9 }), + 0x13, + "declared width too wide", + ), + ( + Error::IncorrectMessageLengthOrInvalidFormat, + 0x13, + "explicit length/format", + ), + ( + Error::NoDataAvailable, + 0x13, + "payload expected, none present", + ), + // --- 0x12 subFunctionNotSupported: the sub-function byte is not a known value --- + ( + Error::InvalidDiagnosticSessionType(0x99), + 0x12, + "0x10 sub-function", + ), + (Error::InvalidEcuResetType(0x99), 0x12, "0x11 sub-function"), + ( + Error::InvalidSecurityAccessType(0x99), + 0x12, + "0x27 sub-function", + ), + ( + Error::InvalidCommunicationControlType(0x99), + 0x12, + "0x28 sub-function", + ), + ( + Error::InvalidTesterPresentType(0x99), + 0x12, + "0x3E sub-function", + ), + ( + Error::InvalidRoutineControlSubFunction(0x99), + 0x12, + "0x31 sub-function", + ), + ( + Error::InvalidDtcSubfunctionType(0x99), + 0x12, + "0x19 sub-function", + ), + (Error::InvalidDtcSetting(0x99), 0x12, "0x85 sub-function"), + // --- 0x31 requestOutOfRange: a parameter value, not a sub-function --- + ( + Error::InvalidCommunicationType(0x99), + 0x31, + "0x28 communicationType parameter", + ), + ( + Error::InvalidMemoryAddress(0x1_0000_0000_0000), + 0x31, + "address beyond 5 bytes", + ), + ( + Error::InvalidEncryptionCompressionMethod(0x99), + 0x31, + "nibble overflow", + ), + ( + Error::InvalidFileOperationMode(0x99), + 0x31, + "0x38 modeOfOperation", + ), + ( + Error::InvalidFileSizeParameterLength(0x99), + 0x31, + "0x38 length parameter", + ), + ( + Error::InvalidDtcFormatIdentifier(0x99), + 0x31, + "DTC format identifier", + ), + ( + Error::ReservedForLegislativeUse(0x99), + 0x31, + "legislative reserved value", + ), + // --- 0x10 generalReject: transport failure, no protocol NRC applies --- + ( + Error::IoError(embedded_io::ErrorKind::WriteZero), + 0x10, + "I/O failure", + ), + ] + .into_iter() + } + + #[test] + fn every_error_maps_to_its_iso_negative_response_code() { + for (err, want, why) in cases() { + let got = u8::from(err.negative_response_code()); + assert_eq!( + got, want, + "{err:?} ({why}): got 0x{got:02X}, want 0x{want:02X}" + ); + } + } + + #[test] + fn mapping_only_produces_codes_the_iso_tables_allow() { + // A decode failure must never be reported as a positive response, and must never + // invent a reserved code — either would put an invalid NRC on the wire. + for (err, _, why) in cases() { + let nrc = err.negative_response_code(); + assert_ne!( + nrc, + NegativeResponseCode::PositiveResponse, + "{why}: decode failure mapped to PositiveResponse" + ); + assert!( + !matches!( + nrc, + NegativeResponseCode::IsoSaeReserved(_) + | NegativeResponseCode::ExtendedDataLinkSecurityReserved(_) + | NegativeResponseCode::ReservedForSpecificConditionsNotMet(_) + ), + "{why}: mapped to a reserved NRC ({nrc:?})" + ); + } + } +} + #[cfg(test)] mod tests { use super::*; diff --git a/src/lib.rs b/src/lib.rs index d8f5cfb2..02ffcb94 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -38,16 +38,17 @@ pub use services::{ CommunicationControlResponse, CommunicationControlType, CommunicationType, ControlDtcSettingRequest, ControlDtcSettingResponse, DiagnosticSessionControlRequest, DiagnosticSessionControlResponse, DiagnosticSessionType, DirSizePayload, DtcAndStatusIter, - DtcFaultDetectionCounterRecord, DtcFaultDetectionIter, DtcSettingType, - DtcSeverityAndStatusIter, EcuResetRequest, EcuResetResponse, FileOperationMode, - FileSizePayload, NamePayload, NegativeResponse, PositionPayload, ReadDataByIdentifierRequest, - ReadDataByIdentifierResponse, ReadDtcInfoRequest, ReadDtcInfoResponse, ReadDtcInfoSubFunction, - RequestDownloadRequest, RequestDownloadResponse, RequestFileTransferRequest, - RequestFileTransferResponse, RequestTransferExitRequest, RequestTransferExitResponse, - ResetType, RoutineControlRequest, RoutineControlResponse, RoutineControlSubFunction, - SecurityAccessLevel, SecurityAccessRequest, SecurityAccessResponse, SecurityAccessType, - SentDataPayload, SizePayload, TesterPresentRequest, TesterPresentResponse, TransferDataRequest, + DtcFaultDetectionCounterRecord, DtcFaultDetectionIter, DtcSettingType, EcuResetRequest, + EcuResetResponse, FileOperationMode, FileSizePayload, NamePayload, NegativeResponse, + PositionPayload, ReadDataByIdentifierRequest, ReadDataByIdentifierResponse, ReadDtcInfoRequest, + ReadDtcInfoResponse, ReadDtcInfoSubFunction, RequestDownloadRequest, RequestDownloadResponse, + RequestFileTransferRequest, RequestFileTransferResponse, RequestTransferExitRequest, + RequestTransferExitResponse, RequestUploadRequest, RequestUploadResponse, ResetType, + RoutineControlRequest, RoutineControlResponse, RoutineControlSubFunction, SecurityAccessLevel, + SecurityAccessRequest, SecurityAccessResponse, SecurityAccessType, SentDataPayload, + SizePayload, SubnetNumber, TesterPresentRequest, TesterPresentResponse, TransferDataRequest, TransferDataResponse, WriteDataByIdentifierRequest, WriteDataByIdentifierResponse, + WwhObdDtcSeverityIter, }; #[cfg(test)] @@ -165,6 +166,56 @@ mod no_std_api_tests { assert_eq!(&buf[..written], &wire); } + #[test] + fn request_upload_frames_roundtrip() { + // RequestUpload request: SID=0x35, DFI=0x00, ALFID=0x12 (size 1 byte, addr 2 bytes), + // addr=0xBEEF, size=0x10 + let wire = [0x35, 0x00, 0x12, 0xBE, 0xEF, 0x10]; + let (req, _) = Request::decode(&wire).unwrap(); + assert_eq!(req.service(), UdsServiceType::RequestUpload); + assert!(matches!(req, Request::RequestUpload(_))); + let mut buf = [0u8; 16]; + let written = req.encode_to_slice(&mut buf).unwrap(); + assert_eq!(&buf[..written], &wire); + assert_eq!(written, req.encoded_size().unwrap()); + + // Positive response: SID=0x75, LFID=0x20 (2-byte block length), 0x0800 + let wire = [0x75, 0x20, 0x08, 0x00]; + let (resp, _) = Response::decode(&wire).unwrap(); + assert_eq!(resp.service(), UdsServiceType::RequestUpload); + match resp { + Response::RequestUpload(ref up) => { + assert_eq!(up.max_number_of_block_length, &[0x08, 0x00]); + } + other => panic!("expected RequestUpload, got {other:?}"), + } + let written = resp.encode_to_slice(&mut buf).unwrap(); + assert_eq!(&buf[..written], &wire); + } + + #[test] + fn request_upload_is_distinct_from_request_download() { + // Same payload, different SID: the two must not collapse into one variant. + let payload = [0x00, 0x12, 0xBE, 0xEF, 0x10]; + let mut down = [0x34u8; 6]; + down[1..].copy_from_slice(&payload); + let mut up = [0x35u8; 6]; + up[1..].copy_from_slice(&payload); + + let (d, _) = Request::decode(&down).unwrap(); + let (u, _) = Request::decode(&up).unwrap(); + assert!(matches!(d, Request::RequestDownload(_))); + assert!(matches!(u, Request::RequestUpload(_))); + assert_eq!(d.service(), UdsServiceType::RequestDownload); + assert_eq!(u.service(), UdsServiceType::RequestUpload); + + let mut buf = [0u8; 8]; + let n = d.encode_to_slice(&mut buf).unwrap(); + assert_eq!(&buf[..n], &down); + let n = u.encode_to_slice(&mut buf).unwrap(); + assert_eq!(&buf[..n], &up); + } + #[test] fn read_dtc_info_response_frame_roundtrip() { // ReadDtcInfo response: SID=0x59, sub=0x02, mask=0xFF, then DTC records @@ -178,9 +229,10 @@ mod no_std_api_tests { #[test] fn read_dtc_info_request_encodes_through_public_api() { // Public-surface construction: types reached via crate root, not shared::/services::. - let req = ReadDtcInfoRequest::new(ReadDtcInfoSubFunction::ReportDtcByStatusMask( - DtcStatusMask::from(0xFF), - )); + let req = ReadDtcInfoRequest::new( + false, + ReadDtcInfoSubFunction::ReportDtcByStatusMask(DtcStatusMask::from(0xFF)), + ); let mut buf = [0u8; 8]; let written = req.encode_to_slice(&mut buf).unwrap(); // sub=0x02 ReportDtcByStatusMask, mask=0xFF @@ -225,4 +277,64 @@ mod no_std_api_tests { &[0xAA, 0xBB], ); } + + #[test] + fn wire_primitives_are_const_constructible() { + // These are the types a caller puts in a `const` table: DTC constants, record + // numbers, and the format identifier. Each must be reachable in const context. + const DTC: DtcRecord = DtcRecord::new(0x01, 0x02, 0x03); + const SNAPSHOT: DtcSnapshotRecordNumber = DtcSnapshotRecordNumber::new(0x02); + const EXT_DATA: DtcExtDataRecordNumber = DtcExtDataRecordNumber::new(0x90); + const STORED: DtcStoredDataRecordNumber = match DtcStoredDataRecordNumber::new(0x02) { + Ok(number) => number, + Err(_) => panic!("0x02 is not reserved"), + }; + const DFI: DataFormatIdentifier = match DataFormatIdentifier::new(0x01, 0x02) { + Ok(dfi) => dfi, + Err(_) => panic!("both nibbles are in range"), + }; + + assert_eq!(u32::from(DTC), 0x01_0203); + assert_eq!(SNAPSHOT.value(), 0x02); + assert_eq!(EXT_DATA.value(), 0x90); + assert_eq!(STORED.value(), 0x02); + assert_eq!(u8::from(DFI), 0x12); + } + + #[test] + fn sid_conversions_and_negative_responses_are_const() { + // The SID map is the crate's most reusable lookup; a server dispatch table wants it + // in const context. `NegativeResponse::new` builds on it, so it follows. + const SID: u8 = UdsServiceType::EcuReset.to_request_sid(); + const SERVICE: UdsServiceType = UdsServiceType::from_request_sid(0x11); + const RESP_SID: u8 = UdsServiceType::EcuReset.to_response_sid(); + const RESP_SERVICE: UdsServiceType = UdsServiceType::from_response_sid(0x51); + const NACK: NegativeResponse = NegativeResponse::new( + UdsServiceType::EcuReset, + NegativeResponseCode::ConditionsNotCorrect, + ); + + assert_eq!(SID, 0x11); + assert_eq!(SERVICE, UdsServiceType::EcuReset); + assert_eq!(RESP_SID, 0x51); + assert_eq!(RESP_SERVICE, UdsServiceType::EcuReset); + assert_eq!(NACK.request_service_sid(), 0x11); + } + + #[test] + fn transfer_setup_requests_are_const_constructible() { + const DOWNLOAD: RequestDownloadRequest = + match RequestDownloadRequest::new(DataFormatIdentifier::NONE, 0x1234, 0x10) { + Ok(req) => req, + Err(_) => panic!("address fits in 5 bytes"), + }; + const UPLOAD: RequestUploadRequest = + match RequestUploadRequest::new(DataFormatIdentifier::NONE, 0x1234, 0x10) { + Ok(req) => req, + Err(_) => panic!("address fits in 5 bytes"), + }; + + assert_eq!(DOWNLOAD.memory_address(), 0x1234); + assert_eq!(UPLOAD.memory_size(), 0x10); + } } diff --git a/src/request.rs b/src/request.rs index 45d16e65..0e47adb8 100644 --- a/src/request.rs +++ b/src/request.rs @@ -1,12 +1,13 @@ //! Module for making and handling UDS Requests use crate::{ - Decode, Encode, Error, Incomplete, + Decode, Encode, Error, Incomplete, NegativeResponseCode, services::{ ClearDiagnosticInfoRequest, CommunicationControlRequest, ControlDtcSettingRequest, DiagnosticSessionControlRequest, EcuResetRequest, ReadDataByIdentifierRequest, ReadDtcInfoRequest, RequestDownloadRequest, RequestFileTransferRequest, - RequestTransferExitRequest, RoutineControlRequest, SecurityAccessRequest, - TesterPresentRequest, TransferDataRequest, WriteDataByIdentifierRequest, + RequestTransferExitRequest, RequestUploadRequest, RoutineControlRequest, + SecurityAccessRequest, TesterPresentRequest, TransferDataRequest, + WriteDataByIdentifierRequest, }, }; use automotive_wire_codec::{write_all, write_u8}; @@ -25,7 +26,7 @@ pub enum Request<'a> { /// Communication control request. CommunicationControl(CommunicationControlRequest), /// Control DTC settings request. - ControlDtcSetting(ControlDtcSettingRequest), + ControlDtcSetting(ControlDtcSettingRequest<'a>), /// Diagnostic session control request. DiagnosticSessionControl(DiagnosticSessionControlRequest), /// ECU reset request. @@ -40,6 +41,8 @@ pub enum Request<'a> { RequestFileTransfer(RequestFileTransferRequest<'a>), /// Request transfer exit. RequestTransferExit(RequestTransferExitRequest<'a>), + /// Request upload. + RequestUpload(RequestUploadRequest), /// Routine control request. RoutineControl(RoutineControlRequest<'a>), /// Security access request. @@ -62,6 +65,15 @@ pub enum Request<'a> { }, } +/// # Remainder +/// +/// The returned remainder is **always empty**. A UDS frame is not self-delimiting — its length +/// comes from the transport (ISO-TP, `DoIP`, ...), not from the message — so one buffer is exactly +/// one frame, and every payload is decoded with `decode_exact`. This means `decode` behaves as +/// `decode_exact` despite the streaming shape of the [`Decode`] contract: do **not** feed it +/// concatenated frames expecting it to consume one at a time, and note that +/// [`DecodeIter`](crate::DecodeIter) over such a buffer would treat the whole thing as a single +/// frame. Split frames at the transport layer before calling this. impl<'a> Decode<'a> for Request<'a> { type Error = crate::Error; @@ -106,6 +118,9 @@ impl<'a> Decode<'a> for Request<'a> { UdsServiceType::RequestTransferExit => Self::RequestTransferExit( ::decode_exact(payload)?, ), + UdsServiceType::RequestUpload => { + Self::RequestUpload(::decode_exact(payload)?) + } UdsServiceType::RoutineControl => { Self::RoutineControl(::decode_exact(payload)?) } @@ -151,6 +166,7 @@ impl Encode for Request<'_> { Self::RequestDownload(req) => req.encode(writer)?, Self::RequestFileTransfer(req) => req.encode(writer)?, Self::RequestTransferExit(req) => req.encode(writer)?, + Self::RequestUpload(req) => req.encode(writer)?, Self::Other { data, .. } => write_all(writer, data).map_err(Error::io)?, Self::RoutineControl(req) => req.encode(writer)?, Self::SecurityAccess(req) => req.encode(writer)?, @@ -166,10 +182,11 @@ impl Request<'_> { #[must_use] pub fn is_positive_response_suppressed(&self) -> bool { match self { - Self::CommunicationControl(req) => req.suppress_positive_response(), + Self::CommunicationControl(req) => req.suppress_positive_response, Self::ControlDtcSetting(req) => req.suppress_positive_response, Self::DiagnosticSessionControl(req) => req.suppress_positive_response, Self::EcuReset(req) => req.suppress_positive_response, + Self::ReadDtcInfo(req) => req.suppress_positive_response, Self::RoutineControl(req) => req.suppress_positive_response, Self::SecurityAccess(req) => req.suppress_positive_response, Self::TesterPresent(req) => req.suppress_positive_response, @@ -177,6 +194,40 @@ impl Request<'_> { } } + /// The [`NegativeResponseCode`]s this request's service is allowed to be answered with. + /// + /// Each request type also exposes this as an associated function (e.g. + /// [`EcuResetRequest::allowed_nack_codes`]); this dispatches to it for a decoded + /// [`Request`], so a server does not have to re-match every variant to reach it. + /// + /// Returns an empty slice for [`Request::Other`], which covers services the crate does not + /// model. That means "the NRC set is unknown", not "no codes apply" — consult ISO 14229-1 + /// for those services. + #[must_use] + pub fn allowed_nack_codes(&self) -> &'static [NegativeResponseCode] { + match self { + Self::ClearDiagnosticInfo(_) => ClearDiagnosticInfoRequest::allowed_nack_codes(), + Self::CommunicationControl(_) => CommunicationControlRequest::allowed_nack_codes(), + Self::ControlDtcSetting(_) => ControlDtcSettingRequest::allowed_nack_codes(), + Self::DiagnosticSessionControl(_) => { + DiagnosticSessionControlRequest::allowed_nack_codes() + } + Self::EcuReset(_) => EcuResetRequest::allowed_nack_codes(), + Self::ReadDataByIdentifier(_) => ReadDataByIdentifierRequest::allowed_nack_codes(), + Self::ReadDtcInfo(_) => ReadDtcInfoRequest::allowed_nack_codes(), + Self::RequestDownload(_) => RequestDownloadRequest::allowed_nack_codes(), + Self::RequestFileTransfer(_) => RequestFileTransferRequest::allowed_nack_codes(), + Self::RequestTransferExit(_) => RequestTransferExitRequest::allowed_nack_codes(), + Self::RequestUpload(_) => RequestUploadRequest::allowed_nack_codes(), + Self::RoutineControl(_) => RoutineControlRequest::allowed_nack_codes(), + Self::SecurityAccess(_) => SecurityAccessRequest::allowed_nack_codes(), + Self::TesterPresent(_) => TesterPresentRequest::allowed_nack_codes(), + Self::TransferData(_) => TransferDataRequest::allowed_nack_codes(), + Self::WriteDataByIdentifier(_) => WriteDataByIdentifierRequest::allowed_nack_codes(), + Self::Other { .. } => &[], + } + } + /// Returns the [`UdsServiceType`] corresponding to this request variant. #[must_use] pub fn service(&self) -> UdsServiceType { @@ -191,6 +242,7 @@ impl Request<'_> { Self::RequestDownload(_) => UdsServiceType::RequestDownload, Self::RequestFileTransfer(_) => UdsServiceType::RequestFileTransfer, Self::RequestTransferExit(_) => UdsServiceType::RequestTransferExit, + Self::RequestUpload(_) => UdsServiceType::RequestUpload, Self::RoutineControl(_) => UdsServiceType::RoutineControl, Self::SecurityAccess(_) => UdsServiceType::SecurityAccess, Self::TesterPresent(_) => UdsServiceType::TesterPresent, @@ -251,6 +303,64 @@ mod tests { assert_eq!(&buf[..written], &wire); } + #[test] + fn allowed_nack_codes_dispatches_for_every_modeled_variant() { + // Every one of the 15 request types has an inherent `allowed_nack_codes()`, but + // without this dispatcher a caller holding a *decoded* `Request` had to re-match all + // 15 variants to reach it — on a `#[non_exhaustive]` enum they cannot match + // exhaustively. Frames are minimal-but-valid for each service. + let frames: [&[u8]; 16] = [ + &[0x14, 0xFF, 0xFF, 0xFF, 0x00], // ClearDiagnosticInfo (groupOfDTC + memorySelection) + &[0x28, 0x00, 0x01], // CommunicationControl + &[0x85, 0x01], // ControlDtcSetting + &[0x10, 0x01], // DiagnosticSessionControl + &[0x11, 0x01], // EcuReset + &[0x22, 0xF1, 0x90], // ReadDataByIdentifier + &[0x19, 0x02, 0xFF], // ReadDtcInfo + &[0x34, 0x00, 0x12, 0xBE, 0xEF, 0x10], // RequestDownload + &[0x38, 0x02, 0x00, 0x01, b'a'], // RequestFileTransfer + &[0x35, 0x00, 0x12, 0xBE, 0xEF, 0x10], // RequestUpload + &[0x37], // RequestTransferExit + &[0x31, 0x01, 0xFF, 0x00], // RoutineControl + &[0x27, 0x01, 0xAA], // SecurityAccess + &[0x3E, 0x00], // TesterPresent + &[0x36, 0x01, 0xAA], // TransferData + &[0x2E, 0xF1, 0x90, 0x01], // WriteDataByIdentifier + ]; + for frame in frames { + let (req, _) = Request::decode(frame).unwrap_or_else(|e| { + panic!("frame {frame:02X?} should decode, got {e:?}"); + }); + assert!( + !matches!(req, Request::Other { .. }), + "frame {frame:02X?} decoded to Other; the table needs updating" + ); + assert!( + !req.allowed_nack_codes().is_empty(), + "{:?} returned no NRCs", + req.service() + ); + } + } + + #[test] + fn allowed_nack_codes_agrees_with_the_inherent_method() { + let (req, _) = Request::decode(&[0x11, 0x01]).unwrap(); + assert_eq!( + req.allowed_nack_codes(), + EcuResetRequest::allowed_nack_codes() + ); + } + + #[test] + fn allowed_nack_codes_is_empty_for_pass_through() { + // 0x23 ReadMemoryByAddress is enumerated but unmodeled, so the crate has no NRC table + // for it. An empty slice says "unknown", not "none apply". + let (req, _) = Request::decode(&[0x23, 0xAA]).unwrap(); + assert!(matches!(req, Request::Other { .. })); + assert!(req.allowed_nack_codes().is_empty()); + } + #[test] fn unmodeled_service_decodes_to_other() { // 0x23 = ReadMemoryByAddress, enumerated but not modeled. diff --git a/src/response.rs b/src/response.rs index 054f2cc4..c1aaeae3 100644 --- a/src/response.rs +++ b/src/response.rs @@ -2,9 +2,9 @@ use crate::{ ClearDiagnosticInfoResponse, CommunicationControlResponse, ControlDtcSettingResponse, Decode, DiagnosticSessionControlResponse, EcuResetResponse, Encode, Error, Incomplete, NegativeResponse, ReadDataByIdentifierResponse, ReadDtcInfoResponse, RequestDownloadResponse, - RequestFileTransferResponse, RequestTransferExitResponse, RoutineControlResponse, - SecurityAccessResponse, TesterPresentResponse, TransferDataResponse, UdsServiceType, - WriteDataByIdentifierResponse, + RequestFileTransferResponse, RequestTransferExitResponse, RequestUploadResponse, + RoutineControlResponse, SecurityAccessResponse, TesterPresentResponse, TransferDataResponse, + UdsServiceType, WriteDataByIdentifierResponse, }; use automotive_wire_codec::{write_all, write_u8}; @@ -45,6 +45,8 @@ pub enum Response<'a> { RequestFileTransfer(RequestFileTransferResponse<'a>), /// Positive response to `RequestTransferExit`. RequestTransferExit(RequestTransferExitResponse<'a>), + /// Positive response to `RequestUpload`. + RequestUpload(RequestUploadResponse<'a>), /// Positive response to `RoutineControl`. RoutineControl(RoutineControlResponse<'a>), /// Positive response to `SecurityAccess`. @@ -67,6 +69,15 @@ pub enum Response<'a> { }, } +/// # Remainder +/// +/// The returned remainder is **always empty**. A UDS frame is not self-delimiting — its length +/// comes from the transport (ISO-TP, `DoIP`, ...), not from the message — so one buffer is exactly +/// one frame, and every payload is decoded with `decode_exact`. This means `decode` behaves as +/// `decode_exact` despite the streaming shape of the [`Decode`] contract: do **not** feed it +/// concatenated frames expecting it to consume one at a time, and note that +/// [`DecodeIter`](crate::DecodeIter) over such a buffer would treat the whole thing as a single +/// frame. Split frames at the transport layer before calling this. impl<'a> Decode<'a> for Response<'a> { type Error = crate::Error; @@ -114,6 +125,9 @@ impl<'a> Decode<'a> for Response<'a> { UdsServiceType::RequestTransferExit => Self::RequestTransferExit( ::decode_exact(payload)?, ), + UdsServiceType::RequestUpload => { + Self::RequestUpload(::decode_exact(payload)?) + } UdsServiceType::RoutineControl => { Self::RoutineControl(::decode_exact(payload)?) } @@ -167,6 +181,7 @@ impl Response<'_> { Self::RequestDownload(_) => UdsServiceType::RequestDownload.to_response_sid(), Self::RequestFileTransfer(_) => UdsServiceType::RequestFileTransfer.to_response_sid(), Self::RequestTransferExit(_) => UdsServiceType::RequestTransferExit.to_response_sid(), + Self::RequestUpload(_) => UdsServiceType::RequestUpload.to_response_sid(), Self::RoutineControl(_) => UdsServiceType::RoutineControl.to_response_sid(), Self::SecurityAccess(_) => UdsServiceType::SecurityAccess.to_response_sid(), Self::TesterPresent(_) => UdsServiceType::TesterPresent.to_response_sid(), @@ -187,6 +202,7 @@ impl Encode for Response<'_> { let payload = match self { Self::ClearDiagnosticInfo(resp) => resp.encode(writer)?, Self::RequestTransferExit(resp) => resp.encode(writer)?, + Self::RequestUpload(resp) => resp.encode(writer)?, Self::CommunicationControl(resp) => resp.encode(writer)?, Self::ControlDtcSetting(resp) => resp.encode(writer)?, Self::DiagnosticSessionControl(resp) => resp.encode(writer)?, diff --git a/src/service.rs b/src/service.rs index 04942e4c..e5a97b97 100644 --- a/src/service.rs +++ b/src/service.rs @@ -145,7 +145,7 @@ impl UdsServiceType { /// /// Unrecognised bytes map to [`UdsServiceType::UnsupportedDiagnosticService`]. #[must_use] - pub fn from_request_sid(value: u8) -> Self { + pub const fn from_request_sid(value: u8) -> Self { match value { 0x10 => Self::DiagnosticSessionControl, 0x11 => Self::EcuReset, @@ -187,7 +187,7 @@ impl UdsServiceType { /// `0x7F`, which is not a valid request SID. To re-encode an unmodeled request without /// loss, use [`Request::Other`](crate::Request::Other), which echoes the raw byte. #[must_use] - pub fn to_request_sid(self) -> u8 { + pub const fn to_request_sid(self) -> u8 { match self { Self::DiagnosticSessionControl => 0x10, Self::EcuReset => 0x11, @@ -223,7 +223,7 @@ impl UdsServiceType { /// /// Unrecognised bytes map to [`UdsServiceType::UnsupportedDiagnosticService`]. #[must_use] - pub fn from_response_sid(value: u8) -> Self { + pub const fn from_response_sid(value: u8) -> Self { match value { 0x50 => Self::DiagnosticSessionControl, 0x51 => Self::EcuReset, @@ -262,7 +262,7 @@ impl UdsServiceType { /// [`UdsServiceType::UnsupportedDiagnosticService`] has no response SID and returns /// `0x7F`; see [`Response::Other`](crate::Response::Other) for lossless pass-through. #[must_use] - pub fn to_response_sid(self) -> u8 { + pub const fn to_response_sid(self) -> u8 { match self { Self::DiagnosticSessionControl => 0x50, Self::EcuReset => 0x51, diff --git a/src/services/clear_dtc_information.rs b/src/services/clear_dtc_information.rs index 8a7bc896..815783c0 100644 --- a/src/services/clear_dtc_information.rs +++ b/src/services/clear_dtc_information.rs @@ -1,5 +1,5 @@ //! `ClearDiagnosticInformation` (0x14) service implementation -use crate::{CLEAR_ALL_DTCS, Decode, DtcRecord, Encode, Incomplete, NegativeResponseCode}; +use crate::{CLEAR_ALL_DTCS, Decode, DtcRecord, Encode, NegativeResponseCode}; use automotive_wire_codec::write_u8; /// Positive response to `ClearDiagnosticInformation`. Carries no payload. @@ -50,29 +50,46 @@ const CLEAR_DIAG_INFO_NEGATIVE_RESPONSE_CODES: [NegativeResponseCode; 4] = [ pub struct ClearDiagnosticInfoRequest { /// Can be either a DTC group (such as chassis/powertrain) or a single DTC pub group_of_dtc: DtcRecord, - /// Used to address a specific memory location of user-defined DTC memory - pub memory_selection: u8, + /// Addresses a user-defined DTC memory, when the client is targeting one. + /// + /// `None` is the ordinary case and the only form in ISO 14229-1:2013: the parameter is + /// marked `U` (user option) in ISO 14229-1:2020 Table 296, so it is absent from the wire + /// unless the client is addressing user-defined DTC memory. + pub memory_selection: Option, } impl ClearDiagnosticInfoRequest { - /// Create a request to clear a specific DTC group from the given memory location. + /// Create a request to clear a specific DTC group, without addressing a user-defined + /// DTC memory. #[must_use] - pub const fn new(group_of_dtc: DtcRecord, memory_selection: u8) -> Self { + pub const fn new(group_of_dtc: DtcRecord) -> Self { Self { group_of_dtc, - memory_selection, + memory_selection: None, } } - /// Create a request to clear all DTCs from the given memory location. + /// Create a request to clear a specific DTC group from a user-defined DTC memory. #[must_use] - pub const fn clear_all(memory_selection: u8) -> Self { + pub const fn new_with_memory_selection(group_of_dtc: DtcRecord, memory_selection: u8) -> Self { Self { - group_of_dtc: CLEAR_ALL_DTCS, - memory_selection, + group_of_dtc, + memory_selection: Some(memory_selection), } } + /// Create a request to clear all DTCs, without addressing a user-defined DTC memory. + #[must_use] + pub const fn clear_all() -> Self { + Self::new(CLEAR_ALL_DTCS) + } + + /// Create a request to clear all DTCs from a user-defined DTC memory. + #[must_use] + pub const fn clear_all_in_memory(memory_selection: u8) -> Self { + Self::new_with_memory_selection(CLEAR_ALL_DTCS, memory_selection) + } + /// Get the allowed [`NegativeResponseCode`] variants for this request #[must_use] pub fn allowed_nack_codes() -> &'static [NegativeResponseCode] { @@ -85,7 +102,9 @@ impl Encode for ClearDiagnosticInfoRequest { fn encode(&self, writer: &mut impl embedded_io::Write) -> Result { let mut written = Encode::encode(&self.group_of_dtc, writer)?; - written += write_u8(writer, self.memory_selection).map_err(crate::Error::io)?; + if let Some(memory_selection) = self.memory_selection { + written += write_u8(writer, memory_selection).map_err(crate::Error::io)?; + } Ok(written) } } @@ -93,21 +112,20 @@ impl Encode for ClearDiagnosticInfoRequest { impl<'a> Decode<'a> for ClearDiagnosticInfoRequest { type Error = crate::Error; + /// The 3 `groupOfDTC` bytes are mandatory; a 4th byte, if present, is the optional + /// `MemorySelection` (ISO 14229-1:2020 Table 296, `Cvt` = `U`). fn decode(buf: &'a [u8]) -> Result<(Self, &'a [u8]), crate::Error> { - let (group_of_dtc, buf) = ::decode(buf)?; - if buf.is_empty() { - return Err(crate::Error::InsufficientData(Incomplete { - needed: 4, - available: buf.len(), - })); - } - let memory_selection = buf[0]; + let (group_of_dtc, rest) = ::decode(buf)?; + let (memory_selection, rest) = match rest { + [] => (None, rest), + [selection, tail @ ..] => (Some(*selection), tail), + }; Ok(( Self { group_of_dtc, memory_selection, }, - &buf[1..], + rest, )) } } @@ -116,7 +134,7 @@ impl<'a> Decode<'a> for ClearDiagnosticInfoRequest { #[cfg(test)] mod request { use super::*; - use crate::{Decode, Encode, test_util::assert_encode_size_agrees}; + use crate::{Decode, Encode, Incomplete, test_util::assert_encode_size_agrees}; #[cfg(feature = "alloc")] use alloc::vec; @@ -124,7 +142,7 @@ mod request { #[test] fn decode_clear_dtc_info_request() { let bytes = [0xFF, 0xFF, 0xFF, 0x00]; - let compare = ClearDiagnosticInfoRequest::new(CLEAR_ALL_DTCS, 0); + let compare = ClearDiagnosticInfoRequest::clear_all_in_memory(0); let (req, _) = ::decode(&bytes).unwrap(); assert_eq!(req, compare); @@ -137,10 +155,85 @@ mod request { #[test] fn clear_all() { - let all = ClearDiagnosticInfoRequest::clear_all(0); - let compare = ClearDiagnosticInfoRequest::new(CLEAR_ALL_DTCS, 0); + let all = ClearDiagnosticInfoRequest::clear_all(); + let compare = ClearDiagnosticInfoRequest::new(CLEAR_ALL_DTCS); assert_eq!(all, compare); } + + #[test] + fn three_byte_request_decodes_without_a_memory_selection() { + // ISO 14229-1:2020 Table 296 marks MemorySelection `U` (user option), and the + // Table 300 flow example is exactly this 3-byte form. + let (req, rest) = + ::decode(&[0xFF, 0xFF, 0xFF]).unwrap(); + assert_eq!(req.memory_selection, None); + assert_eq!(req.group_of_dtc, CLEAR_ALL_DTCS); + assert!(rest.is_empty()); + } + + #[test] + fn a_request_without_a_memory_selection_encodes_three_bytes() { + let req = ClearDiagnosticInfoRequest::clear_all(); + let mut buf = [0u8; 8]; + let written = Encode::encode(&req, &mut buf.as_mut_slice()).unwrap(); + assert_eq!(&buf[..written], &[0xFF, 0xFF, 0xFF]); + assert_encode_size_agrees(&req); + } + + #[test] + fn four_byte_request_decodes_the_memory_selection() { + let (req, rest) = + ::decode(&[0x01, 0x02, 0x03, 0x2A]).unwrap(); + assert_eq!(req.memory_selection, Some(0x2A)); + assert_eq!(req.group_of_dtc, DtcRecord::from(0x01_0203)); + assert!(rest.is_empty()); + } + + #[test] + fn a_request_with_a_memory_selection_encodes_four_bytes() { + let req = + ClearDiagnosticInfoRequest::new_with_memory_selection(DtcRecord::from(0x01_0203), 0x2A); + let mut buf = [0u8; 8]; + let written = Encode::encode(&req, &mut buf.as_mut_slice()).unwrap(); + assert_eq!(&buf[..written], &[0x01, 0x02, 0x03, 0x2A]); + assert_encode_size_agrees(&req); + } + + #[test] + fn clear_all_in_memory_targets_a_user_defined_memory() { + let req = ClearDiagnosticInfoRequest::clear_all_in_memory(0x02); + assert_eq!(req.group_of_dtc, CLEAR_ALL_DTCS); + assert_eq!(req.memory_selection, Some(0x02)); + } + + #[test] + fn a_truncated_group_of_dtc_is_still_rejected() { + // The 3 groupOfDTC bytes stay mandatory; only the 4th byte is optional. + let got = ::decode(&[0xFF, 0xFF]); + assert!( + matches!( + got, + Err(crate::Error::InsufficientData(Incomplete { + needed: 3, + available: 2 + })) + ), + "expected a 3-byte shortfall, got {got:?}" + ); + } + + #[test] + fn both_wire_forms_round_trip_through_the_request_frame() { + for wire in [ + [0x14, 0xFF, 0xFF, 0xFF].as_slice(), + [0x14, 0x01, 0x02, 0x03, 0x2A].as_slice(), + ] { + let (req, _) = crate::Request::decode(wire).unwrap(); + let mut buf = [0u8; 8]; + let written = req.encode_to_slice(&mut buf).unwrap(); + assert_eq!(&buf[..written], wire, "round trip failed for {wire:02X?}"); + } + } } #[cfg(test)] diff --git a/src/services/communication_control.rs b/src/services/communication_control.rs index c52d2b16..17146bb1 100644 --- a/src/services/communication_control.rs +++ b/src/services/communication_control.rs @@ -7,7 +7,7 @@ use automotive_wire_codec::{write_all, write_u8, write_u16_be}; /// /// *Note*: /// -/// Conversions from `u8` to `CommunicationControlType` are fallible and will return an [`Error`](crate::Error) if the +/// Conversions from `u8` to `CommunicationControlType` are fallible and will return an [`Error`] if the /// Suppress Positive Response bit is set. #[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))] #[cfg_attr(feature = "clap", derive(clap::ValueEnum))] @@ -177,14 +177,65 @@ mod communication_control_type_tests { } } +/// Which network the `communicationType` byte applies to — its high nibble (bits 7-4). +/// +/// See ISO 14229-1:2020 Annex B Table B.1. The low nibble of the same byte is the +/// [`CommunicationType`]. +#[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))] +#[cfg_attr(feature = "utoipa", derive(utoipa::ToSchema))] +#[derive(Clone, Copy, Debug, Default, Eq, PartialEq)] +#[non_exhaustive] +pub enum SubnetNumber { + /// `0x0` — apply to the receiving node, including communication to all connected networks. + /// + /// The default, and what a request that does not target a particular subnet carries. + #[default] + AllConnectedNetworks, + /// `0x1`-`0xE` — apply to the specific subnet identified by this number. + /// + /// Construct through [`SubnetNumber::try_from`] so the value is range-checked and cannot + /// collide with the other two variants. + #[non_exhaustive] + Specific(u8), + /// `0xF` — apply to the network the request was received on. + ReceivedOn, +} + +impl SubnetNumber { + /// The high nibble this subnet occupies, as a value in `0x0..=0xF`. + #[must_use] + pub const fn value(&self) -> u8 { + match self { + Self::AllConnectedNetworks => 0x0, + Self::Specific(subnet) => *subnet, + Self::ReceivedOn => 0xF, + } + } +} + +impl TryFrom for SubnetNumber { + type Error = Error; + + /// # Errors + /// Returns [`Error::InvalidCommunicationType`] if `value` does not fit in a nibble. + fn try_from(value: u8) -> Result { + match value { + 0x0 => Ok(Self::AllConnectedNetworks), + 0x1..=0xE => Ok(Self::Specific(value)), + 0xF => Ok(Self::ReceivedOn), + _ => Err(Error::InvalidCommunicationType(value)), + } + } +} + /// `CommunicationType` is used to specify the type of communication behavior to be modified. /// -/// TODO: Note that this implementation is incomplete and does not properly handle the behavior of the upper 4 bits of the field. -/// This implementation is a placeholder and will be updated in the future, which will also be a breaking API change. +/// This is the low nibble (bits 1-0) of the `communicationType` byte; the high nibble is the +/// [`SubnetNumber`]. Bits 3-2 are `ISOSAEReserved` and must be zero. /// /// Note: /// -/// Conversions from `u8` to `CommunicationType` are fallible and will return an [`Error`](crate::Error) if the value is not a valid `CommunicationType` +/// Conversions from `u8` to `CommunicationType` are fallible and will return an [`Error`] if the value is not a valid `CommunicationType` #[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))] #[cfg_attr(feature = "clap", derive(clap::ValueEnum))] #[cfg_attr(feature = "utoipa", derive(utoipa::ToSchema))] @@ -214,13 +265,24 @@ impl From for u8 { impl TryFrom for CommunicationType { type Error = Error; + + /// Reads bits 1-0 of a `communicationType` byte, rejecting a byte whose reserved bits 3-2 + /// are set. Use [`SubnetNumber::try_from`] on the high nibble for the rest of the byte. + /// + /// # Errors + /// Returns [`Error::InvalidCommunicationType`] if bits 3-2 are non-zero, which Annex B + /// Table B.1 marks `ISOSAEReserved`. fn try_from(value: u8) -> Result { - match value { + if value & RESERVED_BITS_MASK != 0 { + return Err(Error::InvalidCommunicationType(value)); + } + match value & MESSAGE_TYPE_MASK { 0x00 => Ok(Self::IsoSaeReserved), 0x01 => Ok(Self::Normal), 0x02 => Ok(CommunicationType::NetworkManagement), 0x03 => Ok(CommunicationType::NormalAndNetworkManagement), - val => Err(Error::InvalidCommunicationType(val)), + // `MESSAGE_TYPE_MASK` keeps only two bits, so no other value can reach here. + _ => unreachable!(), } } } @@ -231,34 +293,64 @@ mod communication_type_tests { /// Check that we properly decode and encode hex bytes #[test] fn communication_type_from_all_u8_values() { + // `CommunicationType` is bits 1-0 of the byte, so the subnet nibble is ignored here and + // only the reserved bits 3-2 can make a byte invalid (Annex B Table B.1). for i in 0..=u8::MAX { let msg_type = CommunicationType::try_from(i); - match i { + if i & RESERVED_BITS_MASK != 0 { + assert!( + matches!(msg_type, Err(Error::InvalidCommunicationType(_))), + "{i:#04X} sets a reserved bit but was accepted" + ); + continue; + } + match i & MESSAGE_TYPE_MASK { 0x00 => assert!(matches!(msg_type, Ok(CommunicationType::IsoSaeReserved))), 0x01 => assert!(matches!(msg_type, Ok(CommunicationType::Normal))), 0x02 => assert!(matches!(msg_type, Ok(CommunicationType::NetworkManagement))), - 0x03 => assert!(matches!( + _ => assert!(matches!( msg_type, Ok(CommunicationType::NormalAndNetworkManagement) )), - _ => assert!(matches!(msg_type, Err(Error::InvalidCommunicationType(_)))), } } } #[test] fn communication_type_round_trip_all_values() { + // A full byte round-trips only once the subnet nibble is put back, which is what + // `CommunicationControlRequest`'s codec does. for i in 0..=u8::MAX { - let value = CommunicationType::try_from(i); - match value { - Ok(value) => assert_eq!(u8::from(value), i), - Err(Error::InvalidCommunicationType(value)) => assert_eq!(value, i), - _ => panic!("Invalid error type"), + let message_type = CommunicationType::try_from(i); + let subnet = SubnetNumber::try_from((i & SUBNET_MASK) >> 4); + match (message_type, subnet) { + (Ok(message_type), Ok(subnet)) => { + assert_eq!((subnet.value() << 4) | u8::from(message_type), i); + } + (Err(Error::InvalidCommunicationType(value)), _) => assert_eq!(value, i), + other => panic!("unexpected result for {i:#04X}: {other:?}"), } } } + + #[test] + fn every_subnet_nibble_round_trips() { + for nibble in 0x0..=0xFu8 { + let subnet = SubnetNumber::try_from(nibble).unwrap(); + assert_eq!(subnet.value(), nibble); + } + assert_eq!(SubnetNumber::default(), SubnetNumber::AllConnectedNetworks); + assert!(SubnetNumber::try_from(0x10).is_err()); + } } +/// Bits 1-0 of the `communicationType` byte: the message type. +const MESSAGE_TYPE_MASK: u8 = 0b0000_0011; +/// Bits 3-2 of the `communicationType` byte, `ISOSAEReserved` per Annex B Table B.1. +const RESERVED_BITS_MASK: u8 = 0b0000_1100; +/// Bits 7-4 of the `communicationType` byte: the subnet number. +const SUBNET_MASK: u8 = 0b1111_0000; + const COMMUNICATION_CONTROL_NEGATIVE_RESPONSE_CODES: [NegativeResponseCode; 4] = [ NegativeResponseCode::SubFunctionNotSupported, NegativeResponseCode::IncorrectMessageLengthOrInvalidFormat, @@ -267,18 +359,21 @@ const COMMUNICATION_CONTROL_NEGATIVE_RESPONSE_CODES: [NegativeResponseCode; 4] = ]; /// Request for the server to change communication behavior -/// -/// # TODO -/// -/// Communication Control is not fully implemented. -/// `CommunicationType` has more complex behavior than is currently implemented. #[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))] #[cfg_attr(feature = "utoipa", derive(utoipa::ToSchema))] #[derive(Clone, Copy, Debug, Eq, PartialEq)] #[non_exhaustive] pub struct CommunicationControlRequest { - control_type: SuppressablePositiveResponse, + /// Whether the server should suppress a positive response (SPRMIB). + /// + /// Public because it carries no invariant with the other fields: it occupies bit 7 of the + /// sub-function byte and is fused onto `control_type` only at the wire boundary. The + /// remaining fields stay private because `node_id` must be present exactly when + /// `control_type` is an enhanced-address variant. + pub suppress_positive_response: bool, + control_type: CommunicationControlType, communication_type: CommunicationType, + subnet: SubnetNumber, node_id: Option, } @@ -300,11 +395,10 @@ impl CommunicationControlRequest { ))); } Ok(Self { - control_type: SuppressablePositiveResponse::new( - suppress_positive_response, - control_type, - ), + suppress_positive_response, + control_type, communication_type, + subnet: SubnetNumber::AllConnectedNetworks, node_id: None, }) } @@ -327,33 +421,50 @@ impl CommunicationControlRequest { ))); } Ok(Self { - control_type: SuppressablePositiveResponse::new( - suppress_positive_response, - control_type, - ), + suppress_positive_response, + control_type, communication_type, + subnet: SubnetNumber::AllConnectedNetworks, node_id: Some(node_id), }) } - /// Getter for whether a positive response should be suppressed + /// The requested [`CommunicationControlType`]. + /// + /// Private field with a getter, not a public field: `node_id` must be present exactly when + /// this is an enhanced-address variant, so the two are set together through + /// [`new`](Self::new) / [`new_with_node_id`](Self::new_with_node_id). #[must_use] - pub fn suppress_positive_response(&self) -> bool { - self.control_type.suppress_positive_response() + pub const fn control_type(&self) -> CommunicationControlType { + self.control_type } - /// Getter for the requested [`CommunicationControlType`] + /// Target a particular subnet instead of all connected networks. + /// + /// Offered as a builder rather than a fourth constructor: the subnet is independent of the + /// `control_type`/`node_id` pairing, so folding it into the constructors would double them + /// without adding a rule to enforce. #[must_use] - pub fn control_type(&self) -> CommunicationControlType { - self.control_type.value() + pub const fn with_subnet(mut self, subnet: SubnetNumber) -> Self { + self.subnet = subnet; + self } /// The [`CommunicationType`] the control applies to. + /// + /// This is the low nibble of the `communicationType` byte; see [`subnet`](Self::subnet) + /// for the high nibble. #[must_use] pub const fn communication_type(&self) -> CommunicationType { self.communication_type } + /// Which network the control applies to — the high nibble of the `communicationType` byte. + #[must_use] + pub const fn subnet(&self) -> SubnetNumber { + self.subnet + } + /// The node identifier, present only for enhanced-address control types. #[must_use] pub const fn node_id(&self) -> Option { @@ -370,11 +481,14 @@ impl Encode for CommunicationControlRequest { type Error = crate::Error; fn encode(&self, writer: &mut impl embedded_io::Write) -> Result { + // Fuse the SPRMIB bit onto the sub-function at the wire boundary. + let sub_function = + SuppressablePositiveResponse::new(self.suppress_positive_response, self.control_type); let mut written = write_all( writer, &[ - u8::from(self.control_type), - u8::from(self.communication_type), + u8::from(sub_function), + (self.subnet.value() << 4) | u8::from(self.communication_type), ], ) .map_err(Error::io)?; @@ -397,6 +511,7 @@ impl<'a> Decode<'a> for CommunicationControlRequest { } let communication_enable = SuppressablePositiveResponse::try_from(buf[0])?; let communication_type = CommunicationType::try_from(buf[1])?; + let subnet = SubnetNumber::try_from((buf[1] & SUBNET_MASK) >> 4)?; match communication_enable.value() { CommunicationControlType::EnableRxAndDisableTxWithEnhancedAddressInfo | CommunicationControlType::EnableRxAndTxWithEnhancedAddressInfo => { @@ -409,8 +524,11 @@ impl<'a> Decode<'a> for CommunicationControlRequest { let node_id = Some(u16::from_be_bytes([buf[2], buf[3]])); Ok(( Self { - control_type: communication_enable, + suppress_positive_response: communication_enable + .suppress_positive_response(), + control_type: communication_enable.value(), communication_type, + subnet, node_id, }, &buf[4..], @@ -418,8 +536,10 @@ impl<'a> Decode<'a> for CommunicationControlRequest { } _ => Ok(( Self { - control_type: communication_enable, + suppress_positive_response: communication_enable.suppress_positive_response(), + control_type: communication_enable.value(), communication_type, + subnet, node_id: None, }, &buf[2..], @@ -435,6 +555,9 @@ impl<'a> Decode<'a> for CommunicationControlRequest { #[non_exhaustive] // Prevent direct construction externally pub struct CommunicationControlResponse { /// The communication control type echoed from the request. + /// + /// Public here although [`CommunicationControlRequest::control_type`] is a getter: the + /// response carries no `node_id`, so there is no cross-field invariant to protect. pub control_type: CommunicationControlType, } @@ -476,6 +599,82 @@ mod request { #[cfg(feature = "alloc")] use alloc::vec::Vec; + #[test] + fn the_communication_type_byte_carries_a_subnet_number() { + // ISO 14229-1:2020 Annex B Table B.1 splits this byte: bits 0-1 are the message type, + // bits 4-7 the subnet number (0 = the specified types on all connected networks, + // 1-E = a specific subnet, F = the network the request arrived on). The whole byte was + // matched against 0x00..=0x03, so 0xF3 -- "network management and normal messages on + // the network this request came in on", a common real-world value -- was rejected. + for (byte, message_type, subnet) in [ + ( + 0x03u8, + CommunicationType::NormalAndNetworkManagement, + SubnetNumber::AllConnectedNetworks, + ), + ( + 0x13, + CommunicationType::NormalAndNetworkManagement, + SubnetNumber::Specific(0x1), + ), + (0xE1, CommunicationType::Normal, SubnetNumber::Specific(0xE)), + ( + 0xF3, + CommunicationType::NormalAndNetworkManagement, + SubnetNumber::ReceivedOn, + ), + ( + 0xF2, + CommunicationType::NetworkManagement, + SubnetNumber::ReceivedOn, + ), + ] { + let wire = [0x28, 0x03, byte]; + let (req, _) = crate::Request::decode(&wire).unwrap(); + let crate::Request::CommunicationControl(inner) = req else { + panic!("expected a CommunicationControl request, got {req:?}"); + }; + assert_eq!( + inner.communication_type(), + message_type, + "wrong message type for {byte:#04X}" + ); + assert_eq!(inner.subnet(), subnet, "wrong subnet for {byte:#04X}"); + + let mut buf = [0u8; 8]; + let written = req.encode_to_slice(&mut buf).unwrap(); + assert_eq!(&buf[..written], &wire, "round trip failed for {byte:#04X}"); + } + } + + #[test] + fn the_reserved_bits_of_the_communication_type_byte_must_be_zero() { + // Table B.1 marks bits 2-3 ISOSAEReserved, so a conformant client leaves them clear. + for byte in [0x07u8, 0x0B, 0x0F, 0xFF] { + assert!( + CommunicationType::try_from(byte).is_err(), + "{byte:#04X} sets a reserved bit but was accepted" + ); + } + } + + #[test] + fn a_subnet_can_be_attached_without_a_second_constructor() { + let req = CommunicationControlRequest::new( + false, + CommunicationControlType::DisableRxAndTx, + CommunicationType::NormalAndNetworkManagement, + ) + .unwrap() + .with_subnet(SubnetNumber::ReceivedOn); + assert_eq!(req.subnet(), SubnetNumber::ReceivedOn); + + let mut buf = [0u8; 8]; + let written = Encode::encode(&req, &mut buf.as_mut_slice()).unwrap(); + assert_eq!(&buf[..written], &[0x03, 0xF3]); + assert_encode_size_agrees(&req); + } + #[cfg(feature = "alloc")] #[test] fn simple_request() { @@ -530,7 +729,7 @@ mod request { ) .unwrap(); assert_eq!(req.node_id(), Some(258)); - assert!(req.suppress_positive_response()); + assert!(req.suppress_positive_response); } #[test] @@ -541,7 +740,7 @@ mod request { CommunicationType::NetworkManagement, ) .unwrap(); - assert!(!req.suppress_positive_response()); + assert!(!req.suppress_positive_response); assert_eq!(CommunicationControlRequest::allowed_nack_codes().len(), 4); } diff --git a/src/services/control_dtc_settings.rs b/src/services/control_dtc_settings.rs index 5e38d63e..d8d9b15e 100644 --- a/src/services/control_dtc_settings.rs +++ b/src/services/control_dtc_settings.rs @@ -1,21 +1,35 @@ //! `ControlDTCSetting` (0x85) service implementation use crate::shared::SuppressablePositiveResponse; use crate::{Decode, Encode, Error, Incomplete, NegativeResponseCode}; -use automotive_wire_codec::write_u8; +use automotive_wire_codec::{write_all, write_u8}; +/// Controls whether the server should enable or disable DTC status-bit updates. +/// +/// Used by [`ControlDtcSettingRequest`] to instruct the server. #[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))] #[cfg_attr(feature = "utoipa", derive(utoipa::ToSchema))] #[cfg_attr(feature = "clap", derive(clap::ValueEnum))] #[derive(Clone, Copy, Debug, Eq, PartialEq)] #[non_exhaustive] -/// Controls whether the server should enable or disable DTC status-bit updates. -/// -/// Used by [`ControlDtcSettingRequest`] to instruct the server. pub enum DtcSettingType { /// Re-enable DTC status-bit updates. On, /// Disable DTC status-bit updates. Off, + /// Reserved for use by vehicle manufacturers (`0x40`-`0x5F`, ISO 14229-1:2020 Table 128). + /// + /// Construct through [`DtcSettingType::try_from`] so the raw byte is range-checked and can + /// never collide with the SPRMIB bit. + #[cfg_attr(feature = "clap", clap(skip))] + #[non_exhaustive] + VehicleManufacturerSpecific(u8), + /// Reserved for use by system suppliers (`0x60`-`0x7E`, ISO 14229-1:2020 Table 128). + /// + /// Construct through [`DtcSettingType::try_from`] so the raw byte is range-checked and can + /// never collide with the SPRMIB bit. + #[cfg_attr(feature = "clap", clap(skip))] + #[non_exhaustive] + SystemSupplierSpecific(u8), } impl From for u8 { @@ -23,16 +37,27 @@ impl From for u8 { match value { DtcSettingType::On => 0x01, DtcSettingType::Off => 0x02, + DtcSettingType::VehicleManufacturerSpecific(value) + | DtcSettingType::SystemSupplierSpecific(value) => value, } } } impl TryFrom for DtcSettingType { type Error = Error; + + /// ISO 14229-1:2020 Table 128 defines `0x01`/`0x02` and two manufacturer-defined ranges, + /// and reserves `0x00`, `0x03`-`0x3F` and `0x7F`. + /// + /// # Errors + /// Returns [`Error::InvalidDtcSetting`] for a reserved value, which maps to + /// [`NegativeResponseCode::SubFunctionNotSupported`] as Table 132 requires. fn try_from(value: u8) -> Result { match value { 0x01 => Ok(Self::On), 0x02 => Ok(Self::Off), + 0x40..=0x5F => Ok(Self::VehicleManufacturerSpecific(value)), + 0x60..=0x7E => Ok(Self::SystemSupplierSpecific(value)), _ => Err(Error::InvalidDtcSetting(value)), } } @@ -43,11 +68,19 @@ impl TryFrom for DtcSettingType { #[cfg_attr(feature = "utoipa", derive(utoipa::ToSchema))] #[derive(Clone, Copy, Debug, Eq, PartialEq)] #[non_exhaustive] -pub struct ControlDtcSettingRequest { +pub struct ControlDtcSettingRequest<'d> { /// Whether the server should suppress the positive response (SPRMIB). pub suppress_positive_response: bool, /// The requested DTC logging setting. pub setting: DtcSettingType, + /// Optional `DTCSettingControlOptionRecord`, empty when absent. + /// + /// Marked `U` (user option) in ISO 14229-1:2020 Table 127. Table 129 describes it as + /// vehicle-manufacturer specific data qualifying the request — for example a list of the + /// DTCs to turn on or off — and Table 132 reserves + /// [`NegativeResponseCode::RequestOutOfRange`] for a server that detects an error in it. + #[cfg_attr(feature = "serde", serde(borrow))] + pub option_record: &'d [u8], } const CONTROL_DTC_SETTING_NEGATIVE_RESPONSE_CODES: [NegativeResponseCode; 4] = [ @@ -57,13 +90,30 @@ const CONTROL_DTC_SETTING_NEGATIVE_RESPONSE_CODES: [NegativeResponseCode; 4] = [ NegativeResponseCode::RequestOutOfRange, ]; -impl ControlDtcSettingRequest { - /// Create a new `ControlDtcSettingRequest`. +impl<'d> ControlDtcSettingRequest<'d> { + /// Create a new `ControlDtcSettingRequest` with no `DTCSettingControlOptionRecord`. #[must_use] pub const fn new(suppress_positive_response: bool, setting: DtcSettingType) -> Self { Self { suppress_positive_response, setting, + option_record: &[], + } + } + + /// Create a request carrying a `DTCSettingControlOptionRecord`. + /// + /// The record's contents are vehicle-manufacturer specific (ISO 14229-1:2020 Table 129). + #[must_use] + pub const fn new_with_option_record( + suppress_positive_response: bool, + setting: DtcSettingType, + option_record: &'d [u8], + ) -> Self { + Self { + suppress_positive_response, + setting, + option_record, } } @@ -74,33 +124,39 @@ impl ControlDtcSettingRequest { } } -impl Encode for ControlDtcSettingRequest { +impl Encode for ControlDtcSettingRequest<'_> { type Error = crate::Error; fn encode(&self, writer: &mut impl embedded_io::Write) -> Result { let sub_function = SuppressablePositiveResponse::new(self.suppress_positive_response, self.setting); - write_u8(writer, u8::from(sub_function)).map_err(Error::io) + let mut written = write_u8(writer, u8::from(sub_function)).map_err(Error::io)?; + written += write_all(writer, self.option_record).map_err(Error::io)?; + Ok(written) } } -impl<'a> Decode<'a> for ControlDtcSettingRequest { +impl<'a> Decode<'a> for ControlDtcSettingRequest<'a> { type Error = crate::Error; + /// The sub-function byte is mandatory; everything after it is the optional + /// `DTCSettingControlOptionRecord` (ISO 14229-1:2020 Table 127, `Cvt` = `U`), whose length + /// is not on the wire — the record runs to the end of the message. fn decode(buf: &'a [u8]) -> Result<(Self, &'a [u8]), Error> { - if buf.is_empty() { + let [sub_function, option_record @ ..] = buf else { return Err(Error::InsufficientData(Incomplete { needed: 1, available: buf.len(), })); - } - let sub_function = SuppressablePositiveResponse::::try_from(buf[0])?; + }; + let sub_function = SuppressablePositiveResponse::::try_from(*sub_function)?; Ok(( Self { suppress_positive_response: sub_function.suppress_positive_response(), setting: sub_function.value(), + option_record, }, - &buf[1..], + &[], )) } } @@ -171,6 +227,81 @@ mod request { assert_encode_size_agrees(&req); } + #[test] + fn manufacturer_and_supplier_specific_setting_types_are_supported() { + // ISO 14229-1:2020 Table 128 reserves 0x40-0x5F for vehicle-manufacturer use and + // 0x60-0x7E for system-supplier use. Both were rejected outright, so a server could + // not even see the byte in order to answer SubFunctionNotSupported for it, and a + // client could not send a manufacturer-defined setting at all. Every sibling + // sub-function enum in the crate models these ranges. + for (byte, expected) in [ + (0x40u8, DtcSettingType::VehicleManufacturerSpecific(0x40)), + (0x5F, DtcSettingType::VehicleManufacturerSpecific(0x5F)), + (0x60, DtcSettingType::SystemSupplierSpecific(0x60)), + (0x7E, DtcSettingType::SystemSupplierSpecific(0x7E)), + ] { + let wire = [byte]; + let (req, _) = ::decode(&wire).unwrap(); + assert_eq!(req.setting, expected, "for sub-function {byte:#04X}"); + + let mut buf = [0u8; 4]; + let written = Encode::encode(&req, &mut buf.as_mut_slice()).unwrap(); + assert_eq!(&buf[..written], &[byte], "round trip for {byte:#04X}"); + } + } + + #[test] + fn reserved_setting_types_are_still_rejected() { + // Table 128 reserves 0x00, 0x03-0x3F and 0x7F. ControlDTCSetting and RoutineControl are + // the two services that validate their sub-function, so both must answer NRC 0x12. + for byte in [0x00u8, 0x03, 0x3F, 0x7F] { + let err = ::decode(&[byte]) + .expect_err("a reserved DTCSettingType must be rejected"); + assert!( + matches!(err, Error::InvalidDtcSetting(got) if got == byte), + "wrong error for {byte:#04X}: {err:?}" + ); + assert_eq!( + err.negative_response_code(), + NegativeResponseCode::SubFunctionNotSupported + ); + } + } + + #[test] + fn a_dtc_setting_control_option_record_round_trips() { + // Table 127 marks DTCSettingControlOptionRecord `U`, and Table 129 describes it as + // carrying e.g. a list of DTCs to turn on or off. Table 132 reserves NRC 0x31 for an + // error *in that record*, which a server can only detect if it is decoded at all. + // Without it, `85 02 AA BB CC` was rejected as having trailing bytes. + let wire = [0x85, 0x02, 0xAA, 0xBB, 0xCC]; + let (req, _) = crate::Request::decode(&wire).unwrap(); + let crate::Request::ControlDtcSetting(inner) = req else { + panic!("expected a ControlDtcSetting request, got {req:?}"); + }; + assert_eq!(inner.setting, DtcSettingType::Off); + assert_eq!(inner.option_record, &[0xAA, 0xBB, 0xCC]); + + let mut buf = [0u8; 8]; + let written = req.encode_to_slice(&mut buf).unwrap(); + assert_eq!(&buf[..written], &wire); + } + + #[test] + fn an_absent_option_record_is_an_empty_slice() { + // Table 133's flow example is the bare two-byte form, so absent must stay absent + // rather than becoming a zero byte on the wire. + let (req, _) = crate::Request::decode(&[0x85, 0x02]).unwrap(); + let crate::Request::ControlDtcSetting(inner) = req else { + panic!("expected a ControlDtcSetting request"); + }; + assert!(inner.option_record.is_empty()); + + let mut buf = [0u8; 8]; + let written = req.encode_to_slice(&mut buf).unwrap(); + assert_eq!(&buf[..written], &[0x85, 0x02]); + } + #[test] fn invalid_setting_byte_carries_the_value() { // An unrecognized setting must surface the offending byte, like every other diff --git a/src/services/ecu_reset.rs b/src/services/ecu_reset.rs index bdd72984..814bfeee 100644 --- a/src/services/ecu_reset.rs +++ b/src/services/ecu_reset.rs @@ -1,7 +1,7 @@ //! `ECUReset` (0x11) service implementation use crate::shared::SuppressablePositiveResponse; use crate::{Decode, Encode, Error, Incomplete, NegativeResponseCode}; -use automotive_wire_codec::{write_all, write_u8}; +use automotive_wire_codec::write_u8; /// UDS defines a number of different types of resets that can be requested /// The reset type is used to specify the type of reset that the ECU should perform @@ -251,17 +251,39 @@ impl<'a> Decode<'a> for EcuResetRequest { pub struct EcuResetResponse { /// The reset type echoed from the request. pub reset_type: ResetType, - /// Time in seconds before the server powers down (`0x00` = not available). - pub power_down_time: u8, + /// Minimum stand-by time the server will remain in the power-down sequence, at one second + /// per count. + /// + /// `0x00`-`0xFE` are 0 to 254 seconds; `0xFF` indicates a failure or that the time is not + /// available (ISO 14229-1:2020 Table 36). + /// + /// `None` means the byte is absent from the wire, which is the ordinary case: the parameter + /// is marked `C` (conditional) in Table 35 and is present only when `reset_type` is + /// [`ResetType::EnableRapidPowerShutDown`]. `None` is therefore distinct from `Some(0)`, + /// which is a server reporting zero seconds. + pub power_down_time: Option, } impl EcuResetResponse { - /// Create a new '`EcuResetResponse`' + /// Create a response that carries no `powerDownTime`, which is every reset type except + /// [`ResetType::EnableRapidPowerShutDown`]. #[must_use] - pub const fn new(reset_type: ResetType, power_down_time: u8) -> Self { + pub const fn new(reset_type: ResetType) -> Self { Self { reset_type, - power_down_time, + power_down_time: None, + } + } + + /// Create a response that reports a `powerDownTime`, as + /// [`ResetType::EnableRapidPowerShutDown`] requires. + /// + /// Pass `0xFF` to report a failure or that the time is not available. + #[must_use] + pub const fn new_with_power_down_time(reset_type: ResetType, power_down_time: u8) -> Self { + Self { + reset_type, + power_down_time: Some(power_down_time), } } } @@ -270,30 +292,41 @@ impl Encode for EcuResetResponse { type Error = crate::Error; fn encode(&self, writer: &mut impl embedded_io::Write) -> Result { - write_all(writer, &[u8::from(self.reset_type), self.power_down_time]).map_err(Error::io) + let mut written = write_u8(writer, u8::from(self.reset_type)).map_err(Error::io)?; + if let Some(power_down_time) = self.power_down_time { + written += write_u8(writer, power_down_time).map_err(Error::io)?; + } + Ok(written) } } impl<'a> Decode<'a> for EcuResetResponse { type Error = crate::Error; + /// The `resetType` echo is mandatory; a second byte, if present, is the conditional + /// `powerDownTime` (ISO 14229-1:2020 Table 35, `Cvt` = `C`). + /// + /// Presence is taken from the wire rather than inferred from `resetType`, so a response + /// from a server that sends the byte outside `enableRapidPowerShutDown` still round-trips + /// unchanged. fn decode(buf: &'a [u8]) -> Result<(Self, &'a [u8]), Error> { - if buf.is_empty() { + let [reset_type, rest @ ..] = buf else { return Err(Error::InsufficientData(Incomplete { needed: 1, available: buf.len(), })); - } - let reset_type = ResetType::try_from(buf[0])?; - // powerDownTime is conditional per ISO 14229-1 - let power_down_time = buf.get(1).copied().unwrap_or(0); - let consumed = core::cmp::min(buf.len(), 2); + }; + let reset_type = ResetType::try_from(*reset_type)?; + let (power_down_time, rest) = match rest { + [] => (None, rest), + [time, tail @ ..] => (Some(*time), tail), + }; Ok(( Self { reset_type, power_down_time, }, - &buf[consumed..], + rest, )) } } @@ -331,15 +364,57 @@ mod response { #[cfg(feature = "alloc")] #[test] fn ecu_reset_response() { - let bytes: [u8; 2] = [0x01, 0x20]; - let resp = EcuResetResponse::new(ResetType::HardReset, 0x20); + let bytes: [u8; 2] = [0x04, 0x20]; + let resp = + EcuResetResponse::new_with_power_down_time(ResetType::EnableRapidPowerShutDown, 0x20); let mut buffer = Vec::new(); let written = Encode::encode(&resp, &mut buffer).unwrap(); let (result, _) = ::decode(&bytes).unwrap(); assert_eq!(result, resp); + // The encoded bytes themselves, not just their count: without this the two halves of + // the test are disconnected and swapping the two written bytes goes unnoticed. + assert_eq!(buffer, bytes); assert_eq!(written, 2); assert_eq!(written, resp.encoded_size().unwrap()); assert_encode_size_agrees(&resp); } + + #[test] + fn a_reset_type_without_a_power_down_time_encodes_one_byte() { + // ISO 14229-1:2020 Table 35 marks powerDownTime `C`, present only when the + // sub-function is enableRapidPowerShutDown (0x04). Table 39's positive-response flow + // example for hardReset is two bytes on the wire: `51 01`. + let resp = EcuResetResponse::new(ResetType::HardReset); + assert_eq!(resp.power_down_time, None); + + let mut buf = [0u8; 4]; + let written = Encode::encode(&resp, &mut buf.as_mut_slice()).unwrap(); + assert_eq!(&buf[..written], &[0x01]); + assert_encode_size_agrees(&resp); + } + + #[test] + fn a_response_without_a_power_down_time_round_trips_unchanged() { + // Decoding and re-encoding must not invent a powerDownTime byte. This used to append + // a spurious 0x00, so any proxy that decoded and re-encoded ECU traffic rewrote every + // positive response except enableRapidPowerShutDown's. + for wire in [[0x51, 0x01].as_slice(), [0x51, 0x04, 0x20].as_slice()] { + let (resp, _) = crate::Response::decode(wire).unwrap(); + let mut buf = [0u8; 8]; + let written = resp.encode_to_slice(&mut buf).unwrap(); + assert_eq!(&buf[..written], wire, "round trip failed for {wire:02X?}"); + } + } + + #[test] + fn an_absent_power_down_time_is_distinguishable_from_a_reported_zero() { + // 0x00 means "0 seconds" per Table 36; 0xFF is the failure/not-available sentinel. + // Conflating absent with 0 loses that distinction. + let (absent, _) = ::decode(&[0x01]).unwrap(); + let (zero, _) = ::decode(&[0x01, 0x00]).unwrap(); + assert_eq!(absent.power_down_time, None); + assert_eq!(zero.power_down_time, Some(0)); + assert_ne!(absent, zero); + } } diff --git a/src/services/mod.rs b/src/services/mod.rs index 49fc767b..09cf45a1 100644 --- a/src/services/mod.rs +++ b/src/services/mod.rs @@ -4,7 +4,7 @@ pub use clear_dtc_information::{ClearDiagnosticInfoRequest, ClearDiagnosticInfoR mod communication_control; pub use communication_control::{ CommunicationControlRequest, CommunicationControlResponse, CommunicationControlType, - CommunicationType, + CommunicationType, SubnetNumber, }; mod control_dtc_settings; @@ -28,12 +28,14 @@ pub use read_data_by_identifier::{ReadDataByIdentifierRequest, ReadDataByIdentif mod read_dtc_information; pub use read_dtc_information::{ - DtcAndStatusIter, DtcFaultDetectionCounterRecord, DtcFaultDetectionIter, - DtcSeverityAndStatusIter, ReadDtcInfoRequest, ReadDtcInfoResponse, ReadDtcInfoSubFunction, + DtcAndStatusIter, DtcFaultDetectionCounterRecord, DtcFaultDetectionIter, ReadDtcInfoRequest, + ReadDtcInfoResponse, ReadDtcInfoSubFunction, WwhObdDtcSeverityIter, }; -mod request_download; -pub use request_download::{RequestDownloadRequest, RequestDownloadResponse}; +mod upload_download; +pub use upload_download::{ + RequestDownloadRequest, RequestDownloadResponse, RequestUploadRequest, RequestUploadResponse, +}; mod request_file_transfer; pub use request_file_transfer::{ diff --git a/src/services/negative_response.rs b/src/services/negative_response.rs index d2df8685..4f416c39 100644 --- a/src/services/negative_response.rs +++ b/src/services/negative_response.rs @@ -15,6 +15,11 @@ use automotive_wire_codec::write_all; #[non_exhaustive] pub struct NegativeResponse { /// Raw echoed request-service byte from the wire, preserved verbatim. + /// + /// Private, unlike the public data-bag fields on most response types: it is a raw byte + /// whose typed meaning is derived ([`request_service`](Self::request_service)), and the two + /// constructors deliberately offer different guarantees — [`new`](Self::new) takes a typed + /// service, [`new_with_sid`](Self::new_with_sid) takes the byte. request_service_sid: u8, /// The negative response code indicating why the request failed. nrc: NegativeResponseCode, @@ -22,21 +27,48 @@ pub struct NegativeResponse { impl NegativeResponse { /// Create a new `NegativeResponse` for a modeled request service. + /// + /// Note that [`UdsServiceType::UnsupportedDiagnosticService`] and + /// [`UdsServiceType::NegativeResponse`] have no request SID and both echo `0x7F`. To NACK a + /// service byte the crate does not model — the `sid` of a + /// [`Request::Other`](crate::Request::Other) — use [`new_with_sid`](Self::new_with_sid) so + /// the original byte is echoed. #[must_use] - pub fn new(request_service: UdsServiceType, nrc: NegativeResponseCode) -> Self { + pub const fn new(request_service: UdsServiceType, nrc: NegativeResponseCode) -> Self { Self { request_service_sid: request_service.to_request_sid(), nrc, } } + /// Create a new `NegativeResponse` echoing a raw request-service byte. + /// + /// For the pass-through case: a server that decoded a + /// [`Request::Other`](crate::Request::Other) can answer + /// [`ServiceNotSupported`](NegativeResponseCode::ServiceNotSupported) while echoing the + /// byte it actually received, which [`new`](Self::new) cannot express. + /// + /// ``` + /// use uds_protocol::{NegativeResponse, NegativeResponseCode}; + /// + /// let nack = NegativeResponse::new_with_sid(0x40, NegativeResponseCode::ServiceNotSupported); + /// assert_eq!(nack.request_service_sid(), 0x40); + /// ``` + #[must_use] + pub const fn new_with_sid(request_service_sid: u8, nrc: NegativeResponseCode) -> Self { + Self { + request_service_sid, + nrc, + } + } + /// The service that triggered this negative response, as a typed [`UdsServiceType`]. /// /// An unmodeled/reserved echoed byte maps to /// [`UdsServiceType::UnsupportedDiagnosticService`]; the original byte remains available /// from [`request_service_sid`](Self::request_service_sid) and is what gets re-encoded. #[must_use] - pub fn request_service(&self) -> UdsServiceType { + pub const fn request_service(&self) -> UdsServiceType { UdsServiceType::from_request_sid(self.request_service_sid) } @@ -95,6 +127,33 @@ mod tests { assert_encode_size_agrees(&value); } + #[test] + fn a_server_can_nack_an_unmodeled_service_byte() { + // The pass-through case this type advertises, from the *construction* side. A server + // that decodes `Request::Other { sid: 0x40 }` must be able to answer + // serviceNotSupported echoing 0x40. `new()` cannot express that: it routes through + // `to_request_sid()`, which collapses every unmodeled service to 0x7F. + let (req, _) = ::decode(&[0x40, 0xAA]).unwrap(); + let sid = match req { + crate::Request::Other { sid, .. } => sid, + other => panic!("expected Other, got {other:?}"), + }; + let nack = NegativeResponse::new_with_sid(sid, NegativeResponseCode::ServiceNotSupported); + assert_eq!(nack.request_service_sid(), 0x40); + + let mut buf = [0u8; 2]; + let n = Encode::encode(&nack, &mut buf.as_mut_slice()).unwrap(); + assert_eq!(&buf[..n], &[0x40, 0x11]); + + // The typed constructor still collapses unmodeled services, which is why the raw + // constructor has to exist. + let via_typed = NegativeResponse::new( + UdsServiceType::UnsupportedDiagnosticService, + NegativeResponseCode::ServiceNotSupported, + ); + assert_eq!(via_typed.request_service_sid(), 0x7F); + } + #[test] fn unknown_echoed_service_round_trips_losslessly() { // 0x40 is not a modeled request service. The echoed byte must survive diff --git a/src/services/read_data_by_identifier.rs b/src/services/read_data_by_identifier.rs index 6ee22dc5..443be8ba 100644 --- a/src/services/read_data_by_identifier.rs +++ b/src/services/read_data_by_identifier.rs @@ -13,8 +13,13 @@ use automotive_wire_codec::{write_all, write_u16_be}; #[derive(Clone, Copy, Debug, Eq, PartialEq)] #[non_exhaustive] pub struct ReadDataByIdentifierResponse<'a> { + /// The raw `[DID][data record]…` bytes, to be parsed caller-side. + /// + /// Public: an opaque byte blob with no invariant to uphold, matching every other + /// response that carries one (`RequestDownloadResponse::max_number_of_block_length`, + /// `TransferDataResponse::data`, `RequestTransferExitResponse::parameter_record`, ...). #[cfg_attr(feature = "serde", serde(borrow))] - records: &'a [u8], + pub records: &'a [u8], } impl<'a> ReadDataByIdentifierResponse<'a> { @@ -23,12 +28,6 @@ impl<'a> ReadDataByIdentifierResponse<'a> { pub const fn new(records: &'a [u8]) -> Self { Self { records } } - - /// The raw `[DID][data record]…` bytes, to be parsed caller-side. - #[must_use] - pub const fn records(&self) -> &'a [u8] { - self.records - } } impl Encode for ReadDataByIdentifierResponse<'_> { @@ -217,7 +216,7 @@ mod test { let raw = [0xF1, 0x90, 0x01, 0x02]; let (resp, remaining) = ::decode(&raw).unwrap(); assert!(remaining.is_empty()); - assert_eq!(resp.records(), &raw); + assert_eq!(resp.records, &raw); let mut buf = [0u8; 8]; let n = Encode::encode(&resp, &mut buf.as_mut_slice()).unwrap(); assert_eq!(&buf[..n], &raw); diff --git a/src/services/read_dtc_information.rs b/src/services/read_dtc_information.rs index 70c7d1b3..152e7c90 100644 --- a/src/services/read_dtc_information.rs +++ b/src/services/read_dtc_information.rs @@ -2,6 +2,7 @@ use automotive_wire_codec::{read_u8, write_all, write_u8, write_u16_be}; +use crate::shared::{fuse_sprmib, split_sprmib}; use crate::{ Decode, DtcExtDataRecordNumber, DtcFormatIdentifier, DtcRecord, DtcSeverityMask, DtcSnapshotRecordNumber, DtcStatusMask, DtcStoredDataRecordNumber, Encode, Error, @@ -20,6 +21,11 @@ const READ_DTC_INFO_NEGATIVE_RESPONSE_CODES: [NegativeResponseCode; 3] = [ #[derive(Clone, Copy, Debug, Eq, PartialEq)] #[non_exhaustive] pub struct ReadDtcInfoRequest { + /// Whether the server should suppress a positive response (SPRMIB). + /// + /// ISO 14229-1:2020 Table 13 requires a server to support both values for every + /// sub-function it supports, so this is independent of `dtc_subfunction`. + pub suppress_positive_response: bool, /// The sub-function specifying what DTC information to report. pub dtc_subfunction: ReadDtcInfoSubFunction, } @@ -27,8 +33,14 @@ pub struct ReadDtcInfoRequest { impl ReadDtcInfoRequest { /// Create a new `ReadDtcInfoRequest`. #[must_use] - pub const fn new(dtc_subfunction: ReadDtcInfoSubFunction) -> Self { - Self { dtc_subfunction } + pub const fn new( + suppress_positive_response: bool, + dtc_subfunction: ReadDtcInfoSubFunction, + ) -> Self { + Self { + suppress_positive_response, + dtc_subfunction, + } } /// Get the allowed [`NegativeResponseCode`] variants for this request. @@ -42,7 +54,15 @@ impl Encode for ReadDtcInfoRequest { type Error = crate::Error; fn encode(&self, writer: &mut impl embedded_io::Write) -> Result { - self.dtc_subfunction.encode(writer) + // The sub-function byte carries SPRMIB in bit 7, so it is written here rather than by + // `ReadDtcInfoSubFunction::encode`, which has no way to know the flag. + let sub_function = fuse_sprmib( + self.suppress_positive_response, + self.dtc_subfunction.value(), + ); + let mut written = write_u8(writer, sub_function).map_err(Error::io)?; + written += self.dtc_subfunction.encode_parameters(writer)?; + Ok(written) } } @@ -58,7 +78,8 @@ impl<'a> Decode<'a> for ReadDtcInfoRequest { available: buf.len(), })); } - let sub = buf[0]; + // Bit 7 is SPRMIB, not part of the sub-function value (ISO 14229-1:2020 Table 13). + let (suppress_positive_response, sub) = split_sprmib(buf[0]); let rest = &buf[1..]; let (dtc_subfunction, rest) = match sub { 0x01 => { @@ -111,7 +132,8 @@ impl<'a> Decode<'a> for ReadDtcInfoRequest { } 0x17 => { let (m, r) = DtcStatusMask::decode(rest)?; - (S::ReportUserDefMemoryDtcByStatusMask(m), r) + let (mem, r) = read_u8(r)?; + (S::ReportUserDefMemoryDtcByStatusMask(m, mem), r) } 0x18 => { let (rec, r) = DtcRecord::decode(rest)?; @@ -155,7 +177,10 @@ impl<'a> Decode<'a> for ReadDtcInfoRequest { } other => (S::IsoSaeReserved(other), rest), }; - Ok((ReadDtcInfoRequest::new(dtc_subfunction), rest)) + Ok(( + ReadDtcInfoRequest::new(suppress_positive_response, dtc_subfunction), + rest, + )) } } @@ -167,7 +192,7 @@ mod read_dtc_info_request_encode_tests { #[test] fn encode_no_param_subfunction() { // 0x0A ReportSupportedDtc, no parameters. - let req = ReadDtcInfoRequest::new(ReadDtcInfoSubFunction::ReportSupportedDtc); + let req = ReadDtcInfoRequest::new(false, ReadDtcInfoSubFunction::ReportSupportedDtc); let mut buf = [0u8; 8]; let written = Encode::encode(&req, &mut buf.as_mut_slice()).unwrap(); assert_eq!(&buf[..written], &[0x0A]); @@ -178,21 +203,90 @@ mod read_dtc_info_request_encode_tests { fn encode_single_param_subfunction() { // 0x02 ReportDtcByStatusMask(mask). DtcStatusMask is 1 byte. let mask = DtcStatusMask::from(0xFF); - let req = ReadDtcInfoRequest::new(ReadDtcInfoSubFunction::ReportDtcByStatusMask(mask)); + let req = + ReadDtcInfoRequest::new(false, ReadDtcInfoSubFunction::ReportDtcByStatusMask(mask)); let mut buf = [0u8; 8]; let written = Encode::encode(&req, &mut buf.as_mut_slice()).unwrap(); assert_eq!(&buf[..written], &[0x02, 0xFF]); assert_encode_size_agrees(&req); } + #[test] + fn both_sprmib_values_round_trip_through_the_request_frame() { + // ISO 14229-1:2020 Table 13 requires that "values of both '0' and '1' shall be + // supported for all SubFunction parameter values ... supported by the server for any + // given service", and clause 12.3.2.2 introduces 0x19's sub-function table with + // "(suppressPosRspMsgIndicationBit (bit 7) not shown)". A suppressed + // reportDTCByStatusMask used to be rejected with TrailingBytes, because the raw 0x82 + // fell through to IsoSaeReserved, which consumes no payload. + for (wire, suppressed) in [ + ([0x19, 0x02, 0xFF].as_slice(), false), + ([0x19, 0x82, 0xFF].as_slice(), true), + ] { + let (req, _) = crate::Request::decode(wire).unwrap(); + assert_eq!( + req.is_positive_response_suppressed(), + suppressed, + "wrong SPRMIB for {wire:02X?}" + ); + let mut buf = [0u8; 8]; + let written = req.encode_to_slice(&mut buf).unwrap(); + assert_eq!(&buf[..written], wire, "round trip failed for {wire:02X?}"); + } + } + + #[test] + fn a_suppressed_sub_function_is_not_mistaken_for_a_reserved_one() { + // 0x8A is reportSupportedDTC with SPRMIB set. Matching on the raw byte decoded this as + // IsoSaeReserved(0x8A) and reported suppression as false, so a server built on this + // answered SubFunctionNotSupported to a request it was required to execute. + let (req, _) = ::decode(&[0x8A]).unwrap(); + assert_eq!( + req.dtc_subfunction, + ReadDtcInfoSubFunction::ReportSupportedDtc + ); + assert!(req.suppress_positive_response); + } + + #[test] + fn user_def_memory_dtc_by_status_mask_carries_a_memory_selection() { + // ISO 14229-1:2020 Table 310 marks both DTCStatusMask and MemorySelection `M`. The + // sibling sub-functions 0x18 and 0x19 (Tables 311/312) already carried theirs. + let wire = [0x17, 0xFF, 0x01]; + let (req, rest) = ::decode(&wire).unwrap(); + assert_eq!( + req.dtc_subfunction, + ReadDtcInfoSubFunction::ReportUserDefMemoryDtcByStatusMask( + DtcStatusMask::from(0xFF), + 0x01 + ) + ); + assert!(rest.is_empty()); + + let mut buf = [0u8; 8]; + let written = Encode::encode(&req, &mut buf.as_mut_slice()).unwrap(); + assert_eq!(&buf[..written], &wire); + assert_encode_size_agrees(&req); + } + + #[test] + fn user_def_memory_dtc_by_status_mask_rejects_a_missing_memory_selection() { + // Without the MemorySelection byte the frame is malformed, and used to be accepted + // while the conformant 3-parameter form was rejected as having trailing bytes. + assert!(::decode_exact(&[0x17, 0xFF]).is_err()); + } + #[test] fn encode_multi_param_subfunction() { // 0x42 ReportWwhObdDtcByMaskRecord(group, status, severity). - let req = ReadDtcInfoRequest::new(ReadDtcInfoSubFunction::ReportWwhObdDtcByMaskRecord( - FunctionalGroupIdentifier::EmissionsSystemGroup, - DtcStatusMask::from(0x08), - DtcSeverityMask::CheckImmediately, - )); + let req = ReadDtcInfoRequest::new( + false, + ReadDtcInfoSubFunction::ReportWwhObdDtcByMaskRecord( + FunctionalGroupIdentifier::EmissionsSystemGroup, + DtcStatusMask::from(0x08), + DtcSeverityMask::CheckImmediately, + ), + ); let mut buf = [0u8; 8]; let written = Encode::encode(&req, &mut buf.as_mut_slice()).unwrap(); assert_eq!(&buf[..written], &[0x42, 0x33, 0x08, 0b1000_0000]); @@ -202,7 +296,7 @@ mod read_dtc_info_request_encode_tests { #[test] fn encode_reserved_subfunction() { // IsoSaeReserved carries the sub-function byte itself, no params. - let req = ReadDtcInfoRequest::new(ReadDtcInfoSubFunction::IsoSaeReserved(0x57)); + let req = ReadDtcInfoRequest::new(false, ReadDtcInfoSubFunction::IsoSaeReserved(0x57)); let mut buf = [0u8; 8]; let written = Encode::encode(&req, &mut buf.as_mut_slice()).unwrap(); assert_eq!(&buf[..written], &[0x57]); @@ -214,20 +308,27 @@ mod read_dtc_info_request_encode_tests { use crate::Decode; // Encode into a scratch buffer (oracle), then decode_exact and assert round-trip fidelity. let cases = [ - ReadDtcInfoRequest::new(ReadDtcInfoSubFunction::ReportSupportedDtc), - ReadDtcInfoRequest::new(ReadDtcInfoSubFunction::ReportDtcByStatusMask( - DtcStatusMask::from(0xFF), - )), - ReadDtcInfoRequest::new(ReadDtcInfoSubFunction::ReportWwhObdDtcByMaskRecord( - FunctionalGroupIdentifier::EmissionsSystemGroup, - DtcStatusMask::from(0x08), - DtcSeverityMask::CheckImmediately, - )), - ReadDtcInfoRequest::new(ReadDtcInfoSubFunction::IsoSaeReserved(0x57)), - ReadDtcInfoRequest::new(ReadDtcInfoSubFunction::ReportDtcSnapshotRecordByDtcNumber( - DtcRecord::new(0x12, 0x34, 0x56), - DtcSnapshotRecordNumber::new(0x01), - )), + ReadDtcInfoRequest::new(false, ReadDtcInfoSubFunction::ReportSupportedDtc), + ReadDtcInfoRequest::new( + false, + ReadDtcInfoSubFunction::ReportDtcByStatusMask(DtcStatusMask::from(0xFF)), + ), + ReadDtcInfoRequest::new( + false, + ReadDtcInfoSubFunction::ReportWwhObdDtcByMaskRecord( + FunctionalGroupIdentifier::EmissionsSystemGroup, + DtcStatusMask::from(0x08), + DtcSeverityMask::CheckImmediately, + ), + ), + ReadDtcInfoRequest::new(false, ReadDtcInfoSubFunction::IsoSaeReserved(0x57)), + ReadDtcInfoRequest::new( + false, + ReadDtcInfoSubFunction::ReportDtcSnapshotRecordByDtcNumber( + DtcRecord::new(0x12, 0x34, 0x56), + DtcSnapshotRecordNumber::new(0x01), + ), + ), ]; for req in cases { let mut buf = [0u8; 16]; @@ -258,6 +359,21 @@ pub struct DtcFaultDetectionCounterRecord { pub dtc_fault_detection_counter: u8, } +impl DtcFaultDetectionCounterRecord { + /// Create a `DtcFaultDetectionCounterRecord`. + /// + /// This type is `#[non_exhaustive]`, so downstream crates cannot use a struct literal and + /// need this constructor — for a test fixture, or for a server building the record list + /// that [`DtcFaultDetectionIter`] reads back. + #[must_use] + pub const fn new(dtc_record: DtcRecord, dtc_fault_detection_counter: u8) -> Self { + Self { + dtc_record, + dtc_fault_detection_counter, + } + } +} + /// Subfunctions for the `ReadDTCInformation` service #[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))] #[cfg_attr(feature = "utoipa", derive(utoipa::ToSchema))] @@ -327,9 +443,12 @@ pub enum ReadDtcInfoSubFunction { ReportDtcExtDataRecordByRecordNumber(DtcExtDataRecordNumber), /// * Parameter: `DtcStatusMask` + /// * Parameter: `memorySelection`(1) — addresses the user-defined DTC memory to read from + /// + /// Both parameters are mandatory (ISO 14229-1:2020 Table 310). /// /// 0x17 - ReportUserDefMemoryDtcByStatusMask(DtcStatusMask), + ReportUserDefMemoryDtcByStatusMask(DtcStatusMask, u8), /// Parameter: `DtcRecord` (3 bytes) /// Parameter: `DtcSnapshotRecordNumber`(1) @@ -369,7 +488,15 @@ pub enum ReadDtcInfoSubFunction { /// /// 0x56 ReportDtcInformationByDtcReadinessGroupIdentifier(FunctionalGroupIdentifier, u8), - /// 0x42-0x54, 0x57-0x7F + /// A sub-function byte this crate does not model. + /// + /// ISO 14229-1:2020 Table 317 reserves `0x00`, `0x1B`-`0x41`, `0x43`-`0x54` and + /// `0x57`-`0x7F`. The remaining bytes that land here are report types the crate has not + /// implemented yet; a server should answer those with + /// [`NegativeResponseCode::SubFunctionNotSupported`]. + /// + /// The value never has bit 7 set: that bit is SPRMIB and is split off into + /// [`ReadDtcInfoRequest::suppress_positive_response`] before the sub-function is decoded. IsoSaeReserved(u8), } @@ -395,7 +522,7 @@ impl ReadDtcInfoSubFunction { Self::ReportDtcFaultDetectionCounter => 0x14, Self::ReportDtcWithPermanentStatus => 0x15, Self::ReportDtcExtDataRecordByRecordNumber(_) => 0x16, - Self::ReportUserDefMemoryDtcByStatusMask(_) => 0x17, + Self::ReportUserDefMemoryDtcByStatusMask(_, _) => 0x17, Self::ReportUserDefMemoryDtcSnapshotRecordByDtcNumber(_, _, _) => 0x18, Self::ReportUserDefMemoryDtcExtDataRecordByDtcNumber(_, _, _) => 0x19, Self::ReportSupportedDtcExtDataRecord(_) => 0x1A, @@ -407,19 +534,22 @@ impl ReadDtcInfoSubFunction { } } -impl Encode for ReadDtcInfoSubFunction { - type Error = crate::Error; - - fn encode(&self, writer: &mut impl embedded_io::Write) -> Result { +impl ReadDtcInfoSubFunction { + /// Write only this sub-function's parameter bytes, not its leading sub-function byte. + /// + /// [`ReadDtcInfoRequest::encode`] writes that byte itself, because it has to fuse SPRMIB + /// into bit 7 and this type does not carry the flag. + fn encode_parameters(self, writer: &mut impl embedded_io::Write) -> Result { use ReadDtcInfoSubFunction as S; - writer.write_all(&[self.value()]).map_err(Error::io)?; - let mut written = 1; + let mut written = 0; match self { - S::ReportNumberOfDtcByStatusMask(m) - | S::ReportDtcByStatusMask(m) - | S::ReportUserDefMemoryDtcByStatusMask(m) => { + S::ReportNumberOfDtcByStatusMask(m) | S::ReportDtcByStatusMask(m) => { written += m.encode(writer)?; } + S::ReportUserDefMemoryDtcByStatusMask(m, mem) => { + written += m.encode(writer)?; + written += write_u8(writer, mem).map_err(Error::io)?; + } S::ReportDtcSnapshotRecordByDtcNumber(r, n) => { written += r.encode(writer)?; written += n.encode(writer)?; @@ -445,12 +575,12 @@ impl Encode for ReadDtcInfoSubFunction { S::ReportUserDefMemoryDtcSnapshotRecordByDtcNumber(r, n, mem) => { written += r.encode(writer)?; written += n.encode(writer)?; - written += write_u8(writer, *mem).map_err(Error::io)?; + written += write_u8(writer, mem).map_err(Error::io)?; } S::ReportUserDefMemoryDtcExtDataRecordByDtcNumber(r, n, mem) => { written += r.encode(writer)?; written += n.encode(writer)?; - written += write_u8(writer, *mem).map_err(Error::io)?; + written += write_u8(writer, mem).map_err(Error::io)?; } S::ReportWwhObdDtcByMaskRecord(g, m, s) => { written += g.encode(writer)?; @@ -462,7 +592,7 @@ impl Encode for ReadDtcInfoSubFunction { } S::ReportDtcInformationByDtcReadinessGroupIdentifier(g, rg) => { written += g.encode(writer)?; - written += write_u8(writer, *rg).map_err(Error::io)?; + written += write_u8(writer, rg).map_err(Error::io)?; } S::ReportDtcSnapshotIdentification | S::ReportSupportedDtc @@ -478,6 +608,22 @@ impl Encode for ReadDtcInfoSubFunction { } } +impl Encode for ReadDtcInfoSubFunction { + type Error = crate::Error; + + /// Writes the sub-function byte with SPRMIB clear, followed by this sub-function's + /// parameters. + /// + /// Encode a [`ReadDtcInfoRequest`] instead to control the suppress-positive-response bit; + /// this impl always leaves it clear, because the flag lives on the request rather than on + /// the sub-function. + fn encode(&self, writer: &mut impl embedded_io::Write) -> Result { + let mut written = write_u8(writer, self.value()).map_err(Error::io)?; + written += self.encode_parameters(writer)?; + Ok(written) + } +} + // --------------------------------------------------------------------------- // no_std RX types with lazy iterators // --------------------------------------------------------------------------- @@ -485,6 +631,15 @@ impl Encode for ReadDtcInfoSubFunction { /// Lazy iterator over `(DtcRecord, DtcStatusMask)` pairs from raw bytes. /// /// Each pair is 4 bytes: 3 for the DTC record + 1 for the status mask. +/// +/// # Length +/// +/// [`len`](DtcAndStatusIter::len) counts **complete records**; [`size_hint`](Iterator::size_hint) counts +/// **items yielded**, which is one greater when a partial record trails the buffer (that tail +/// surfaces as a single `Err`, after which the iterator is exhausted). The two therefore differ +/// on malformed input, which is why this deliberately does not implement `ExactSizeIterator` — +/// its `len()` would contradict the inherent one. It does implement +/// [`FusedIterator`](core::iter::FusedIterator). #[derive(Clone, Debug)] pub struct DtcAndStatusIter<'a> { remaining: &'a [u8], @@ -527,6 +682,10 @@ impl Iterator for DtcAndStatusIter<'_> { return None; } if self.remaining.len() < 4 { + // Consume the partial tail so the error is reported exactly once and the + // iterator terminates. Returning without advancing would yield this error + // forever. + self.remaining = &[]; return Some(Err(Error::IncorrectMessageLengthOrInvalidFormat)); } let record = DtcRecord::new(self.remaining[0], self.remaining[1], self.remaining[2]); @@ -534,11 +693,28 @@ impl Iterator for DtcAndStatusIter<'_> { self.remaining = &self.remaining[4..]; Some(Ok((record, status))) } + + fn size_hint(&self) -> (usize, Option) { + // One item per complete record, plus one final `Err` if a partial tail remains. + let n = self.remaining.len().div_ceil(4); + (n, Some(n)) + } } +impl core::iter::FusedIterator for DtcAndStatusIter<'_> {} + /// Lazy iterator over `DtcFaultDetectionCounterRecord` from raw bytes. /// /// Each record is 4 bytes: 3 for the DTC record + 1 for the fault detection counter. +/// +/// # Length +/// +/// [`len`](DtcFaultDetectionIter::len) counts **complete records**; [`size_hint`](Iterator::size_hint) counts +/// **items yielded**, which is one greater when a partial record trails the buffer (that tail +/// surfaces as a single `Err`, after which the iterator is exhausted). The two therefore differ +/// on malformed input, which is why this deliberately does not implement `ExactSizeIterator` — +/// its `len()` would contradict the inherent one. It does implement +/// [`FusedIterator`](core::iter::FusedIterator). #[derive(Clone, Debug)] pub struct DtcFaultDetectionIter<'a> { remaining: &'a [u8], @@ -581,6 +757,8 @@ impl Iterator for DtcFaultDetectionIter<'_> { return None; } if self.remaining.len() < 4 { + // See `DtcAndStatusIter::next`: consume the tail so this terminates. + self.remaining = &[]; return Some(Err(Error::IncorrectMessageLengthOrInvalidFormat)); } let dtc_record = DtcRecord::new(self.remaining[0], self.remaining[1], self.remaining[2]); @@ -591,17 +769,38 @@ impl Iterator for DtcFaultDetectionIter<'_> { dtc_fault_detection_counter, })) } + + fn size_hint(&self) -> (usize, Option) { + let n = self.remaining.len().div_ceil(4); + (n, Some(n)) + } } -/// Lazy iterator over `(DtcSeverityMask, DtcRecord, DtcStatusMask)` triples from raw bytes. +impl core::iter::FusedIterator for DtcFaultDetectionIter<'_> {} + +/// Lazy iterator over the WWH-OBD `(DtcSeverityMask, DtcRecord, DtcStatusMask)` triples of a +/// [`ReadDtcInfoResponse::WwhObdDtcByMaskRecord`] (sub-function `0x42`). /// /// Each triple is 5 bytes: 1 severity + 3 DTC record + 1 status mask. +/// +/// This applies **only** to the WWH-OBD variant. The `0x08`/`0x09` +/// [`DtcSeverityList`](ReadDtcInfoResponse::DtcSeverityList) records are 6 bytes and carry an +/// extra DTC functional-unit byte, so they need a different iterator (not yet wired). +/// +/// # Length +/// +/// [`len`](WwhObdDtcSeverityIter::len) counts **complete records**; [`size_hint`](Iterator::size_hint) counts +/// **items yielded**, which is one greater when a partial record trails the buffer (that tail +/// surfaces as a single `Err`, after which the iterator is exhausted). The two therefore differ +/// on malformed input, which is why this deliberately does not implement `ExactSizeIterator` — +/// its `len()` would contradict the inherent one. It does implement +/// [`FusedIterator`](core::iter::FusedIterator). #[derive(Clone, Debug)] -pub struct DtcSeverityAndStatusIter<'a> { +pub struct WwhObdDtcSeverityIter<'a> { remaining: &'a [u8], } -impl<'a> DtcSeverityAndStatusIter<'a> { +impl<'a> WwhObdDtcSeverityIter<'a> { /// Create an iterator over severity/DTC/status triples. #[must_use] pub const fn new(data: &'a [u8]) -> Self { @@ -632,7 +831,7 @@ impl<'a> DtcSeverityAndStatusIter<'a> { } } -impl Iterator for DtcSeverityAndStatusIter<'_> { +impl Iterator for WwhObdDtcSeverityIter<'_> { type Item = Result<(DtcSeverityMask, DtcRecord, DtcStatusMask), Error>; fn next(&mut self) -> Option { @@ -640,6 +839,8 @@ impl Iterator for DtcSeverityAndStatusIter<'_> { return None; } if self.remaining.len() < 5 { + // See `DtcAndStatusIter::next`: consume the tail so this terminates. + self.remaining = &[]; return Some(Err(Error::IncorrectMessageLengthOrInvalidFormat)); } let severity = DtcSeverityMask::from(self.remaining[0]); @@ -648,8 +849,15 @@ impl Iterator for DtcSeverityAndStatusIter<'_> { self.remaining = &self.remaining[5..]; Some(Ok((severity, record, status))) } + + fn size_hint(&self) -> (usize, Option) { + let n = self.remaining.len().div_ceil(5); + (n, Some(n)) + } } +impl core::iter::FusedIterator for WwhObdDtcSeverityIter<'_> {} + /// Zero-copy parsed response for `ReadDTCInformation` (0x19). /// /// Stores raw bytes for record collections and provides lazy iterators @@ -677,6 +885,12 @@ pub enum ReadDtcInfoResponse<'a> { /// that does not support [`DtcStatusMask::WarningIndicatorRequested`] leaves that bit /// 'off' and sets the rest. status_availability_mask: DtcStatusMask, + /// How the server's DTC numbers are formatted and encoded. + /// + /// Mandatory in this response (ISO 14229-1:2020 Table 319), and the only thing that + /// says how to interpret the three bytes of each DTC — ISO 14229-1 itself defines no + /// decoding method for them. + format_identifier: DtcFormatIdentifier, /// Number of matching DTCs. count: u16, }, @@ -689,13 +903,16 @@ pub enum ReadDtcInfoResponse<'a> { /// that does not support [`DtcStatusMask::WarningIndicatorRequested`] leaves that bit /// 'off' and sets the rest. status_availability_mask: DtcStatusMask, - /// Raw record bytes — use [`DtcAndStatusIter`] to iterate. + /// Raw record bytes, 4 per record (3-byte DTC + status) — use [`DtcAndStatusIter`]. + /// Decoding rejects a length that is not a whole number of records. #[cfg_attr(feature = "serde", serde(borrow))] raw_records: &'a [u8], }, /// Sub-function 0x14: list of DTC fault detection counter records. DtcFaultDetectionCounterList { - /// Raw record bytes — use [`DtcFaultDetectionIter`] to iterate. + /// Raw record bytes, 4 per record (3-byte DTC + counter) — use + /// [`DtcFaultDetectionIter`]. Decoding rejects a length that is not a whole number of + /// records. #[cfg_attr(feature = "serde", serde(borrow))] raw_records: &'a [u8], }, @@ -708,10 +925,10 @@ pub enum ReadDtcInfoResponse<'a> { /// that does not support [`DtcStatusMask::WarningIndicatorRequested`] leaves that bit /// 'off' and sets the rest. status_availability_mask: DtcStatusMask, - /// Raw `DTCAndSeverityRecord` bytes (6 bytes each: severity + DTC functional unit + - /// 3-byte DTC + status). These differ from the 5-byte WWH-OBD records, so - /// [`DtcSeverityAndStatusIter`] does **not** apply here; no severity-list iterator is - /// wired yet, so parse these bytes caller-side until one is added. + /// Raw `DTCAndSeverityRecord` bytes: 6 each — severity + DTC functional unit + + /// 3-byte DTC + status. No iterator is wired for this variant yet, so parse them + /// caller-side. Note these are *not* the 5-byte WWH-OBD records read by + /// [`WwhObdDtcSeverityIter`]. Decoding rejects a length that is not a multiple of 6. #[cfg_attr(feature = "serde", serde(borrow))] raw_records: &'a [u8], }, @@ -728,7 +945,8 @@ pub enum ReadDtcInfoResponse<'a> { severity_availability_mask: DtcSeverityMask, /// DTC format identifier. format_identifier: DtcFormatIdentifier, - /// Raw record bytes (5 bytes per record) — use [`DtcSeverityAndStatusIter`]. + /// Raw record bytes, 5 per record — use [`WwhObdDtcSeverityIter`]. Decoding rejects a length + /// that is not a whole number of records. #[cfg_attr(feature = "serde", serde(borrow))] raw_records: &'a [u8], }, @@ -766,16 +984,34 @@ impl<'a> ReadDtcInfoResponse<'a> { /// records are 6 bytes because they carry a `DTCFunctionalUnit` byte this one /// does not. #[must_use] - pub fn severity_and_status_iter(&self) -> Option> { + pub fn wwh_obd_dtc_severity_iter(&self) -> Option> { match self { Self::WwhObdDtcByMaskRecord { raw_records, .. } => { - Some(DtcSeverityAndStatusIter::new(raw_records)) + Some(WwhObdDtcSeverityIter::new(raw_records)) } _ => None, } } } +/// Validate that a record list divides evenly into whole records. +/// +/// A UDS frame is complete when it arrives — its length comes from the transport — so a +/// trailing partial record means the frame is malformed, not that more bytes are coming. +/// Rejecting it here matches how the crate treats every other length mismatch. +/// +/// Consequently the iterators reached from a **decoded** [`ReadDtcInfoResponse`] never see a +/// partial tail. A hand-constructed variant still can — the enum's `#[non_exhaustive]` stops +/// exhaustive matching, not variant construction, and `Encode` writes `raw_records` verbatim — +/// so the iterators keep their `Result` item type and their one-error-then-terminate behaviour. +fn whole_records(raw: &[u8], record_len: usize) -> Result<&[u8], Error> { + if raw.len() % record_len == 0 { + Ok(raw) + } else { + Err(Error::IncorrectMessageLengthOrInvalidFormat) + } +} + impl<'a> Decode<'a> for ReadDtcInfoResponse<'a> { type Error = crate::Error; @@ -791,21 +1027,25 @@ impl<'a> Decode<'a> for ReadDtcInfoResponse<'a> { match subfunction_id { 0x01 | 0x07 => { - if buf.len() < 3 { + // Table 319: DTCStatusAvailabilityMask, DTCFormatIdentifier, then a 2-byte + // DTCCount -- four mandatory bytes after the sub-function echo. + if buf.len() < 4 { return Err(Error::InsufficientData(Incomplete { needed: 4, available: buf.len(), })); } let status_availability_mask = DtcStatusMask::from(buf[0]); - let count = u16::from_be_bytes([buf[1], buf[2]]); + let format_identifier = DtcFormatIdentifier::from(buf[1]); + let count = u16::from_be_bytes([buf[2], buf[3]]); Ok(( Self::NumberOfDtcs { sub_function_id: subfunction_id, status_availability_mask, + format_identifier, count, }, - &buf[3..], + &buf[4..], )) } 0x02 | 0x0A | 0x0B | 0x0C | 0x0D | 0x0E | 0x15 => { @@ -820,12 +1060,17 @@ impl<'a> Decode<'a> for ReadDtcInfoResponse<'a> { Self::DtcList { sub_function_id: subfunction_id, status_availability_mask, - raw_records: &buf[1..], + raw_records: whole_records(&buf[1..], 4)?, }, &[], )) } - 0x14 => Ok((Self::DtcFaultDetectionCounterList { raw_records: buf }, &[])), + 0x14 => Ok(( + Self::DtcFaultDetectionCounterList { + raw_records: whole_records(buf, 4)?, + }, + &[], + )), 0x08 | 0x09 => { if buf.is_empty() { return Err(Error::InsufficientData(Incomplete { @@ -838,7 +1083,7 @@ impl<'a> Decode<'a> for ReadDtcInfoResponse<'a> { Self::DtcSeverityList { sub_function_id: subfunction_id, status_availability_mask, - raw_records: &buf[1..], + raw_records: whole_records(&buf[1..], 6)?, }, &[], )) @@ -860,7 +1105,7 @@ impl<'a> Decode<'a> for ReadDtcInfoResponse<'a> { status_availability_mask, severity_availability_mask, format_identifier, - raw_records: &buf[4..], + raw_records: whole_records(&buf[4..], 5)?, }, &[], )) @@ -879,10 +1124,18 @@ impl Encode for ReadDtcInfoResponse<'_> { Self::NumberOfDtcs { sub_function_id, status_availability_mask, + format_identifier, count, } => { - written += write_all(writer, &[*sub_function_id, status_availability_mask.bits()]) - .map_err(Error::io)?; + written += write_all( + writer, + &[ + *sub_function_id, + status_availability_mask.bits(), + u8::from(*format_identifier), + ], + ) + .map_err(Error::io)?; written += write_u16_be(writer, *count).map_err(Error::io)?; } Self::DtcList { @@ -936,6 +1189,7 @@ mod derive_contract { use crate::test_util::assert_impl_serde; const _: ReadDtcInfoRequest = ReadDtcInfoRequest::new( + false, ReadDtcInfoSubFunction::ReportDtcByStatusMask(DtcStatusMask::TestFailed), ); @@ -953,6 +1207,171 @@ mod derive_contract { } } +#[cfg(test)] +mod response_decode_tests { + use super::*; + use crate::{Decode, Encode, Response}; + + #[test] + fn the_dtc_count_response_carries_a_format_identifier() { + // ISO 14229-1:2020 Table 341 (flow example #1) is exactly these six bytes: + // SID, reportType, DTCStatusAvailabilityMask, DTCFormatIdentifier, count high, low. + // Table 319 marks all of them `M`. The format identifier used to be missing from the + // model, so it was read as the count high byte: this frame was rejected outright, and + // a count of 1 came back as 0x0100. + let wire = [0x59, 0x01, 0x2F, 0x01, 0x00, 0x01]; + let (resp, _) = Response::decode(&wire).unwrap(); + let Response::ReadDtcInfo(ReadDtcInfoResponse::NumberOfDtcs { + sub_function_id, + status_availability_mask, + format_identifier, + count, + }) = resp + else { + panic!("expected a NumberOfDtcs response, got {resp:?}"); + }; + assert_eq!(sub_function_id, 0x01); + assert_eq!(status_availability_mask.bits(), 0x2F); + assert_eq!( + format_identifier, + DtcFormatIdentifier::Iso14229_1DtcFormat, + "0x01 is ISO_14229-1_DTCFormat per Table D.14" + ); + assert_eq!(count, 1); + + let mut buf = [0u8; 8]; + let written = resp.encode_to_slice(&mut buf).unwrap(); + assert_eq!(&buf[..written], &wire); + } + + #[test] + fn a_dtc_count_response_missing_the_count_is_rejected() { + // Four payload bytes are mandatory after the sub-function echo; three is a truncated + // frame, not a frame whose format identifier happens to be the count's high byte. + assert!(::decode(&[0x01, 0x2F, 0x01]).is_err()); + } + + /// `(label, sub-function payload prefix, record width)` for every variant that carries a + /// record list. The payload here is what follows the sub-function byte. + const LISTS: [(&str, &[u8], usize); 4] = [ + // 0x02: status availability mask, then 4-byte (DTC, status) records. + ("DtcList 0x02", &[0x02, 0xFF], 4), + // 0x14: no mask byte — records start immediately. + ("DtcFaultDetectionCounterList 0x14", &[0x14], 4), + // 0x08: status availability mask, then 6-byte DTCAndSeverityRecord entries. + ("DtcSeverityList 0x08", &[0x08, 0xFF], 6), + // 0x42: fgid + status mask + severity mask + format id, then 5-byte WWH-OBD records. + ( + "WwhObdDtcByMaskRecord 0x42", + &[0x42, 0x33, 0xFF, 0xF0, 0x01], + 5, + ), + ]; + + /// Enough for the widest prefix (5 bytes) plus the longest record list these tests build + /// (3 x 6-byte records). + const FRAME_CAP: usize = 32; + + /// Build a frame into a fixed-size buffer, returning it with its used length. A stack buffer + /// rather than a `Vec` so the tests compile without the `alloc` feature. + fn frame(prefix: &[u8], record_bytes: usize) -> ([u8; FRAME_CAP], usize) { + let len = prefix.len() + record_bytes; + assert!(len <= FRAME_CAP, "frame does not fit the test buffer"); + let mut buf = [0u8; FRAME_CAP]; + buf[..prefix.len()].copy_from_slice(prefix); + for (i, byte) in buf[prefix.len()..len].iter_mut().enumerate() { + *byte = u8::try_from(i % 251).unwrap_or(0); + } + (buf, len) + } + + #[test] + fn record_lists_must_divide_evenly_into_records() { + // A trailing partial record means the frame is malformed. Rejecting it here matches how + // the crate treats every other length mismatch, and means the iterators returned by the + // accessors can never see a partial tail. + for (label, prefix, width) in LISTS { + for extra in 1..width { + let (buf, len) = frame(prefix, width + extra); + let got = ::decode(&buf[..len]); + assert!( + matches!(got, Err(Error::IncorrectMessageLengthOrInvalidFormat)), + "{label}: {} record bytes ({width}+{extra}) should be rejected, got {got:?}", + width + extra + ); + } + } + } + + #[test] + fn aligned_record_lists_decode_with_the_expected_record_count() { + for (label, prefix, width) in LISTS { + for records in 0..=3usize { + let (buf, len) = frame(prefix, width * records); + let (resp, _) = ::decode(&buf[..len]) + .unwrap_or_else(|e| panic!("{label}: {records} records should decode: {e:?}")); + let counted = resp + .dtc_and_status_iter() + .map(Iterator::count) + .or_else(|| resp.fault_detection_iter().map(Iterator::count)) + .or_else(|| resp.wwh_obd_dtc_severity_iter().map(Iterator::count)); + // DtcSeverityList has no iterator wired yet, so it has no count to check. + if let Some(counted) = counted { + assert_eq!(counted, records, "{label}: wrong record count"); + } + } + } + } + + #[test] + fn empty_record_lists_are_valid() { + // A server with no matching DTCs answers with the header and no records. + for (label, prefix, _) in LISTS { + let got = ::decode(prefix); + assert!( + got.is_ok(), + "{label}: empty record list must decode, got {got:?}" + ); + } + } + + #[test] + fn a_misaligned_list_is_rejected_at_the_frame_layer_too() { + // SID 0x59, sub 0x02, mask 0xFF, then 5 record bytes — one byte past a whole record. + let wire = [0x59, 0x02, 0xFF, 0x01, 0x02, 0x03, 0x0A, 0xEE]; + assert!(matches!( + Response::decode(&wire), + Err(Error::IncorrectMessageLengthOrInvalidFormat) + )); + // The aligned frame still round-trips. + let wire = [0x59, 0x02, 0xFF, 0x01, 0x02, 0x03, 0x0A]; + let (resp, _) = Response::decode(&wire).unwrap(); + let mut buf = [0u8; 16]; + let n = resp.encode_to_slice(&mut buf).unwrap(); + assert_eq!(&buf[..n], &wire); + } + + #[test] + fn iterators_from_a_decoded_response_never_yield_an_error() { + // The payoff: with decode validating alignment, every iterator obtained through an + // accessor is error-free. The `Err` arm remains reachable only via `Iter::new` on + // arbitrary bytes, which `iter_tests` covers. + for (label, prefix, width) in LISTS { + let (buf, len) = frame(prefix, width * 2); + let (resp, _) = ::decode(&buf[..len]).unwrap(); + if let Some(mut it) = resp.dtc_and_status_iter() { + assert!(it.all(|r| r.is_ok()), "{label}"); + } + if let Some(mut it) = resp.fault_detection_iter() { + assert!(it.all(|r| r.is_ok()), "{label}"); + } + if let Some(mut it) = resp.wwh_obd_dtc_severity_iter() { + assert!(it.all(|r| r.is_ok()), "{label}"); + } + } + } +} + #[cfg(test)] mod iter_tests { use super::*; @@ -980,7 +1399,149 @@ mod iter_tests { assert_eq!(DtcFaultDetectionIter::new(&[0u8; 8]).len(), 2); assert!(DtcFaultDetectionIter::new(&[0u8; 3]).is_empty()); // 5-byte severity/DTC/status records. - assert_eq!(DtcSeverityAndStatusIter::new(&[0u8; 10]).len(), 2); - assert!(DtcSeverityAndStatusIter::new(&[0u8; 4]).is_empty()); + assert_eq!(WwhObdDtcSeverityIter::new(&[0u8; 10]).len(), 2); + assert!(WwhObdDtcSeverityIter::new(&[0u8; 4]).is_empty()); + } + + #[test] + fn partial_tail_yields_one_error_then_terminates() { + // Previously `next()` returned `Some(Err(..))` on a partial record *without* + // advancing `remaining`, so the iterator yielded that error forever: `for _ in iter` + // looped, `count()` hung, and `collect::>>()` allocated without bound. + // `collect_all()` happened to terminate only because `collect::>()` + // short-circuits on the first error. Bounded with `take` so a regression fails + // instead of hanging the suite. + let data = [0x01, 0x02, 0x03, 0x0A, 0xFF]; // one complete record + 1 stray byte + let items: heapless_vec::Bounded<8> = + DtcAndStatusIter::new(&data).take(8).collect_bounded(); + assert_eq!(items.len(), 2, "expected 1 record + 1 error, got {items:?}"); + assert!(items.oks == 1 && items.errs == 1); + + // Same shape for the other two. + let five: heapless_vec::Bounded<8> = DtcFaultDetectionIter::new(&[0u8; 5]) + .take(8) + .collect_bounded(); + assert_eq!((five.oks, five.errs), (1, 1)); + let six: heapless_vec::Bounded<8> = WwhObdDtcSeverityIter::new(&[0u8; 6]) + .take(8) + .collect_bounded(); + assert_eq!((six.oks, six.errs), (1, 1)); + } + + #[test] + fn iterators_terminate_when_shorter_than_one_record() { + let mut iter = DtcAndStatusIter::new(&[0x01, 0x02]); + assert!(matches!(iter.next(), Some(Err(_)))); + assert!(iter.next().is_none(), "iterator must be exhausted"); + assert!(iter.next().is_none(), "and stay exhausted (fused)"); + } + + #[test] + fn size_hint_matches_the_number_of_items_yielded() { + // All three iterators, every buffer length across several record boundaries. The two + // record widths differ (4 bytes vs 5), so each needs its own `div_ceil` checked. + // `take` bounds the count so a non-termination regression fails rather than hangs. + let data = [0u8; 16]; + for len in 0usize..=16 { + let it = DtcAndStatusIter::new(&data[..len]); + let actual = it.clone().take(32).count(); + assert_eq!( + it.size_hint(), + (actual, Some(actual)), + "DtcAndStatusIter, {len} bytes" + ); + assert_eq!( + actual, + len.div_ceil(4), + "DtcAndStatusIter count, {len} bytes" + ); + + let it = DtcFaultDetectionIter::new(&data[..len]); + let actual = it.clone().take(32).count(); + assert_eq!( + it.size_hint(), + (actual, Some(actual)), + "DtcFaultDetectionIter, {len} bytes" + ); + assert_eq!( + actual, + len.div_ceil(4), + "DtcFaultDetectionIter count, {len} bytes" + ); + + let it = WwhObdDtcSeverityIter::new(&data[..len]); + let actual = it.clone().take(32).count(); + assert_eq!( + it.size_hint(), + (actual, Some(actual)), + "WwhObdDtcSeverityIter, {len} bytes" + ); + assert_eq!( + actual, + len.div_ceil(5), + "WwhObdDtcSeverityIter count, {len} bytes" + ); + } + } + + #[test] + fn all_three_iterators_terminate_and_stay_exhausted() { + // FusedIterator is only sound if `next()` keeps returning None once drained. + let data = [0u8; 7]; // not a whole number of records for either width + let mut a = DtcAndStatusIter::new(&data); + while a.next().is_some() {} + assert!(a.next().is_none() && a.next().is_none()); + + let mut b = DtcFaultDetectionIter::new(&data); + while b.next().is_some() {} + assert!(b.next().is_none() && b.next().is_none()); + + let mut c = WwhObdDtcSeverityIter::new(&data); + while c.next().is_some() {} + assert!(c.next().is_none() && c.next().is_none()); + } + + /// Minimal counting collector so the termination tests need no allocator. + mod heapless_vec { + use super::*; + use core::fmt; + + pub struct Bounded { + pub oks: usize, + pub errs: usize, + } + + impl Bounded { + pub fn len(&self) -> usize { + self.oks + self.errs + } + } + + impl fmt::Debug for Bounded { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "{} ok, {} err", self.oks, self.errs) + } + } + + pub trait CollectBounded { + fn collect_bounded(self) -> Bounded; + } + + impl CollectBounded for I + where + I: Iterator>, + { + fn collect_bounded(self) -> Bounded { + let mut b = Bounded:: { oks: 0, errs: 0 }; + for item in self { + match item { + Ok(_) => b.oks += 1, + Err(_) => b.errs += 1, + } + } + b + } + } } + use heapless_vec::CollectBounded; } diff --git a/src/services/request_download.rs b/src/services/request_download.rs deleted file mode 100644 index 7af58aeb..00000000 --- a/src/services/request_download.rs +++ /dev/null @@ -1,340 +0,0 @@ -//! `RequestDownload` (0x34) service implementation - -use crate::shared::{DataFormatIdentifier, LengthFormatIdentifier, MemoryFormatIdentifier}; -use crate::{Decode, Encode, Error, Incomplete, NegativeResponseCode}; -use automotive_wire_codec::{read_be_uint_into, write_all, write_be_uint, write_u8}; - -const REQUEST_DOWNLOAD_NEGATIVE_RESPONSE_CODES: [NegativeResponseCode; 6] = [ - NegativeResponseCode::IncorrectMessageLengthOrInvalidFormat, - NegativeResponseCode::ConditionsNotCorrect, - NegativeResponseCode::RequestOutOfRange, - NegativeResponseCode::SecurityAccessDenied, - NegativeResponseCode::AuthenticationRequired, - NegativeResponseCode::UploadDownloadNotAccepted, -]; - -/// A request to the server for it to download data from the client -/// -/// A positive response to this request ([`RequestDownloadResponse`]) will happen -/// after the server takes all necessary actions to receive the data once the server is ready to receive -/// -/// This is a variable length Request, determined by the `address_and_length_format_identifier` value -/// See ISO-14229-1:2020, Table H.1 for format information -#[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))] -#[cfg_attr(feature = "utoipa", derive(utoipa::ToSchema))] -#[derive(Clone, Copy, Debug, Eq, PartialEq)] -#[non_exhaustive] -pub struct RequestDownloadRequest { - /// compression method (high nibble) and encrypting method (low nibble). 0x00 is no compression or encryption - data_format_identifier: DataFormatIdentifier, - /// 7-4: length (# of bytes) of `memory_size` param, 3-0: length (# of bytes) of `memory_address` param - address_and_length_format_identifier: MemoryFormatIdentifier, - /// Starting address of the server memory. The on-wire byte width is derived from this - /// value (max 5 bytes), so it is private to keep it in sync with the format identifier. - memory_address: u64, - /// Size of the data to be downloaded. The on-wire byte width is derived from this value - /// (max 4 bytes), so it is private to keep it in sync with the format identifier. - memory_size: u32, -} - -impl RequestDownloadRequest { - /// Create a new `RequestDownloadRequest` - /// - /// # Errors - /// Returns an error if `memory_address` exceeds 5 bytes (> `0xFF_FFFF_FFFF`). - #[allow(clippy::cast_possible_truncation)] - pub fn new( - data_format_identifier: DataFormatIdentifier, - memory_address: u64, - memory_size: u32, - ) -> Result { - if memory_address > 0xFF_FFFF_FFFF { - return Err(Error::InvalidMemoryAddress(memory_address)); - } - // A length of 0 produces an invalid `MemoryFormatIdentifier` (the nibbles - // must be >=1 per ISO-14229), so clamp to at least one byte even when the - // address or size is 0. - let memory_address_length = - ((u64::BITS - memory_address.leading_zeros()).div_ceil(8) as u8).max(1); - let memory_size_length = - ((u32::BITS - memory_size.leading_zeros()).div_ceil(8) as u8).max(1); - let address_and_length_format_identifier = MemoryFormatIdentifier { - memory_size_length, - memory_address_length, - }; - Ok(Self { - data_format_identifier, - address_and_length_format_identifier, - memory_address, - memory_size, - }) - } - - /// Starting address of the server memory. - #[must_use] - pub const fn memory_address(&self) -> u64 { - self.memory_address - } - - /// Size of the data to be downloaded. - #[must_use] - pub const fn memory_size(&self) -> u32 { - self.memory_size - } - - /// Get the allowed [`NegativeResponseCode`] variants for this request - #[must_use] - pub fn allowed_nack_codes() -> &'static [NegativeResponseCode] { - &REQUEST_DOWNLOAD_NEGATIVE_RESPONSE_CODES - } -} -impl Encode for RequestDownloadRequest { - type Error = crate::Error; - - fn encode(&self, writer: &mut impl embedded_io::Write) -> Result { - let mut written = write_all( - writer, - &[ - self.data_format_identifier.into(), - self.address_and_length_format_identifier.into(), - ], - ) - .map_err(Error::io)?; - - let addr_len = self - .address_and_length_format_identifier - .memory_address_length as usize; - let size_len = self.address_and_length_format_identifier.memory_size_length as usize; - written += write_be_uint(writer, u128::from(self.memory_address), addr_len)?; - written += write_be_uint(writer, u128::from(self.memory_size), size_len)?; - - Ok(written) - } -} - -impl<'a> Decode<'a> for RequestDownloadRequest { - type Error = crate::Error; - - fn decode(buf: &'a [u8]) -> Result<(Self, &'a [u8]), Error> { - if buf.len() < 2 { - return Err(Error::InsufficientData(Incomplete { - needed: 2, - available: buf.len(), - })); - } - let data_format_identifier = DataFormatIdentifier::from(buf[0]); - let memory_identifier = MemoryFormatIdentifier::try_from(buf[1])?; - let addr_len = memory_identifier.memory_address_length as usize; - let size_len = memory_identifier.memory_size_length as usize; - let total = 2 + addr_len + size_len; - if buf.len() < total { - return Err(Error::InsufficientData(Incomplete { - needed: total, - available: buf.len(), - })); - } - - let (memory_address, rest) = read_be_uint_into::(&buf[2..], addr_len)?; - let (memory_size, _rest) = read_be_uint_into::(rest, size_len)?; - - Ok(( - Self { - data_format_identifier, - address_and_length_format_identifier: memory_identifier, - memory_address, - memory_size, - }, - &buf[total..], - )) - } -} - -/// Zero-alloc response for request download. Borrows from the caller. -/// -/// Positive response to a [`RequestDownloadRequest`] indicating the server is ready to receive data. -#[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))] -#[cfg_attr(feature = "utoipa", derive(utoipa::ToSchema))] -#[derive(Clone, Copy, Debug, Eq, PartialEq)] -#[non_exhaustive] -pub struct RequestDownloadResponse<'d> { - /// Maximum number of bytes per [`TransferDataRequest`](crate::TransferDataRequest). - /// - /// The on-wire `lengthFormatIdentifier` nibble is derived from this slice's length - /// at encode time, so the declared length can never disagree with the bytes present. - #[cfg_attr(feature = "serde", serde(borrow))] - pub max_number_of_block_length: &'d [u8], -} - -impl<'d> RequestDownloadResponse<'d> { - /// Create a new request download response. The `lengthFormatIdentifier` is derived - /// from `max_number_of_block_length` during encoding. - #[must_use] - pub const fn new(max_number_of_block_length: &'d [u8]) -> Self { - Self { - max_number_of_block_length, - } - } -} - -impl Encode for RequestDownloadResponse<'_> { - type Error = crate::Error; - - fn encode(&self, writer: &mut impl embedded_io::Write) -> Result { - // The block-length field width is carried in a single nibble, so the slice - // can be at most 0x0F bytes long. - let nibble = u8::try_from(self.max_number_of_block_length.len()) - .ok() - .filter(|n| *n <= 0x0F) - .ok_or(Error::IncorrectMessageLengthOrInvalidFormat)?; - let length_format_identifier = LengthFormatIdentifier { - max_number_of_block_length: nibble, - }; - let mut written = write_u8(writer, length_format_identifier.into()).map_err(Error::io)?; - written += write_all(writer, self.max_number_of_block_length).map_err(Error::io)?; - Ok(written) - } -} - -impl<'a> Decode<'a> for RequestDownloadResponse<'a> { - type Error = crate::Error; - - fn decode(buf: &'a [u8]) -> Result<(Self, &'a [u8]), Error> { - if buf.is_empty() { - return Err(Error::InsufficientData(Incomplete { - needed: 1, - available: buf.len(), - })); - } - let length_format_identifier = LengthFormatIdentifier::from(buf[0]); - let len = length_format_identifier.max_number_of_block_length as usize; - let total = 1 + len; - if buf.len() < total { - return Err(Error::InsufficientData(Incomplete { - needed: total, - available: buf.len(), - })); - } - Ok(( - Self { - max_number_of_block_length: &buf[1..total], - }, - &buf[total..], - )) - } -} - -#[cfg(test)] -mod tests { - use super::*; - use crate::{Decode, Encode, test_util::assert_encode_size_agrees}; - #[cfg(feature = "alloc")] - use alloc::vec; - - #[test] - fn simple_request() { - let bytes: [u8; 7] = [ - 0x00, // No compression or encryption - 0x14, // 1 byte for memory size, 4 bytes for memory address - 0xF0, 0xFF, 0xFF, 0x67, // memory address - 0x0A, - ]; - let (req, _) = ::decode(&bytes).unwrap(); - - assert_eq!(u8::from(req.data_format_identifier), 0); - assert_eq!(u8::from(req.address_and_length_format_identifier), 0x14); - assert_eq!( - req.address_and_length_format_identifier.memory_size_length, - 1 - ); - assert_eq!( - req.address_and_length_format_identifier - .memory_address_length, - 4 - ); - - assert_eq!(req.memory_address(), 0xF0FF_FF67); - assert_eq!(req.memory_size(), 0x0A); - } - - #[test] - fn bad_request() { - let bytes: [u8; 3] = [ - 0x00, // No compression or encryption - 0x11, // 1 byte for memory size, 1 byte for memory address - 0x67, - ]; - let result = ::decode(&bytes); - assert!(result.is_err()); - } - - #[test] - fn read_memory_identifier() { - let memory_format_identifier = MemoryFormatIdentifier::try_from(0x23).unwrap(); - assert_eq!(memory_format_identifier.memory_size_length, 2); - assert_eq!(memory_format_identifier.memory_address_length, 3); - - assert_eq!(u8::from(memory_format_identifier), 0x23); - } - - #[test] - fn read_length_identifier() { - let length_format_identifier = LengthFormatIdentifier::from(0xF0); - assert_eq!(length_format_identifier.max_number_of_block_length, 15); - - assert_eq!(u8::from(length_format_identifier), 0xF0); - } - - #[test] - fn zero_address_and_size_clamp_to_one_byte() { - // A 0 address/size must still produce a valid (>=1 byte) length nibble, - // otherwise the encoded frame cannot be decoded back. - let req = RequestDownloadRequest::new(0x00.into(), 0, 0).unwrap(); - assert_eq!( - req.address_and_length_format_identifier - .memory_address_length, - 1 - ); - assert_eq!( - req.address_and_length_format_identifier.memory_size_length, - 1 - ); - - let mut buf = [0u8; 8]; - let written = Encode::encode(&req, &mut buf.as_mut_slice()).unwrap(); - let (decoded, _) = ::decode(&buf[..written]).unwrap(); - assert_eq!(decoded.memory_address(), 0); - assert_eq!(decoded.memory_size(), 0); - } - - #[cfg(feature = "alloc")] - #[test] - fn check_message_size() { - let req = RequestDownloadRequest::new(0x00.into(), 0xF0_FF_FF_67, 0x0A).unwrap(); - - let mut vec = vec![]; - Encode::encode(&req, &mut vec).unwrap(); - - assert_eq!(vec.len(), req.encoded_size().unwrap()); - assert_encode_size_agrees(&req); - } - - #[test] - fn response_encode_size_agrees() { - let block = [0x10u8, 0x00, 0x00]; - let resp = RequestDownloadResponse::new(&block); - assert_encode_size_agrees(&resp); - } - - #[test] - fn derive_contract() { - use crate::test_util::assert_impl_eq; - assert_impl_eq::(); - assert_impl_eq::>(); - #[cfg(feature = "serde")] - { - use crate::test_util::assert_impl_serde; - assert_impl_serde::(); - assert_impl_serde::>(); - } - } -} diff --git a/src/services/request_file_transfer.rs b/src/services/request_file_transfer.rs index 2833f609..5c8b473a 100644 --- a/src/services/request_file_transfer.rs +++ b/src/services/request_file_transfer.rs @@ -217,10 +217,16 @@ pub enum RequestFileTransferRequest<'a> { ), } -const REQUEST_FILE_TRANSFER_NEGATIVE_RESPONSE_CODES: [NegativeResponseCode; 4] = [ +/// Permitted NRCs for `RequestFileTransfer` (0x38), per ISO 14229-1:2020 Table 484. +const REQUEST_FILE_TRANSFER_NEGATIVE_RESPONSE_CODES: [NegativeResponseCode; 7] = [ NegativeResponseCode::IncorrectMessageLengthOrInvalidFormat, NegativeResponseCode::ConditionsNotCorrect, + // Table 484: "shall be returned when modeOfOperation is 06 (ResumeFile) and the requested + // file has already been completely transferred". + NegativeResponseCode::RequestSequenceError, NegativeResponseCode::RequestOutOfRange, + NegativeResponseCode::SecurityAccessDenied, + NegativeResponseCode::AuthenticationRequired, NegativeResponseCode::UploadDownloadNotAccepted, ]; @@ -859,13 +865,21 @@ impl<'a> Decode<'a> for RequestFileTransferResponse<'a> { #[cfg(test)] mod request_tests { use super::*; - use crate::NegativeResponseCode; use crate::test_util::assert_encode_size_agrees; #[test] fn test_allowed_nack_codes() { + // ISO 14229-1:2020 Table 484 lists exactly these seven codes. Pinned as a set rather + // than spot-checked, because a `contains` assertion cannot notice a missing code -- + // and three were missing, including the requestSequenceError that Table 484 defines + // specifically for ResumeFile on an already-complete transfer. let codes = RequestFileTransferRequest::allowed_nack_codes(); - assert!(codes.contains(&NegativeResponseCode::UploadDownloadNotAccepted)); + let mut bytes = [0u8; 7]; + assert_eq!(codes.len(), bytes.len(), "wrong number of codes: {codes:?}"); + for (slot, code) in bytes.iter_mut().zip(codes) { + *slot = u8::from(*code); + } + assert_eq!(bytes, [0x13, 0x22, 0x24, 0x31, 0x33, 0x34, 0x70]); } #[test] diff --git a/src/services/routine_control.rs b/src/services/routine_control.rs index abfb602d..72c50a92 100644 --- a/src/services/routine_control.rs +++ b/src/services/routine_control.rs @@ -40,12 +40,20 @@ impl From for u8 { impl TryFrom for RoutineControlSubFunction { type Error = Error; + + /// ISO 14229-1:2020 Table 426 defines `0x01`-`0x03` and reserves everything else, with no + /// vehicle-manufacturer or system-supplier range — so unlike most sub-function enums in + /// this crate there is nothing legitimate to model beyond the three named values. + /// + /// # Errors + /// Returns [`Error::InvalidRoutineControlSubFunction`] for any other value, which maps to + /// [`NegativeResponseCode::SubFunctionNotSupported`] as Table 430 requires. fn try_from(value: u8) -> Result { match value { 0x01 => Ok(RoutineControlSubFunction::StartRoutine), 0x02 => Ok(RoutineControlSubFunction::StopRoutine), 0x03 => Ok(RoutineControlSubFunction::RequestRoutineResults), - _ => Err(Error::IncorrectMessageLengthOrInvalidFormat), + _ => Err(Error::InvalidRoutineControlSubFunction(value)), } } } @@ -230,6 +238,29 @@ mod test { } } + #[test] + fn an_unsupported_sub_function_is_answered_with_sub_function_not_supported() { + // ISO 14229-1:2020 Table 426 defines only 0x01-0x03; 0x00 and 0x04-0x7F are + // ISOSAEReserved. Table 430 requires NRC 0x12 for a sub-function that "is either + // generally not supported or is not supported for the requested RoutineIdentifier". + // Reporting IncorrectMessageLengthOrInvalidFormat sent 0x13 instead, for a request + // whose length was perfectly correct -- and disagreed with ControlDTCSetting, the only + // other service that validates its sub-function. + for byte in [0x00u8, 0x04, 0x10, 0x7F] { + let err = crate::Request::decode(&[0x31, byte, 0xF0, 0x0F]) + .expect_err("a reserved routineControlType must be rejected"); + assert!( + matches!(err, Error::InvalidRoutineControlSubFunction(got) if got == byte), + "wrong error for sub-function {byte:#04X}: {err:?}" + ); + assert_eq!( + err.negative_response_code(), + NegativeResponseCode::SubFunctionNotSupported, + "wrong NRC for sub-function {byte:#04X}" + ); + } + } + #[test] fn rc_request_round_trips_with_suppress() { let req = RoutineControlRequest::new( diff --git a/src/services/security_access.rs b/src/services/security_access.rs index 29e45506..ea32756d 100644 --- a/src/services/security_access.rs +++ b/src/services/security_access.rs @@ -29,7 +29,7 @@ impl SecurityAccessLevel { /// The raw level byte (always `0x00..=0x7F`). #[must_use] - pub const fn value(self) -> u8 { + pub const fn value(&self) -> u8 { self.0 } } diff --git a/src/services/tester_present.rs b/src/services/tester_present.rs index 9f0898ef..57f75035 100644 --- a/src/services/tester_present.rs +++ b/src/services/tester_present.rs @@ -31,15 +31,22 @@ impl Default for ZeroSubFunction { } } -impl From for u8 { - fn from(sub_function: ZeroSubFunction) -> Self { - match sub_function { +impl ZeroSubFunction { + /// The raw sub-function byte. `const` so callers' accessors can be `const` too. + const fn value(self) -> u8 { + match self { ZeroSubFunction::NoSubFunctionSupported => NO_SUBFUNCTION_VALUE, ZeroSubFunction::IsoSaeReserved(value) => value, } } } +impl From for u8 { + fn from(sub_function: ZeroSubFunction) -> Self { + sub_function.value() + } +} + impl TryFrom for ZeroSubFunction { type Error = Error; fn try_from(value: u8) -> Result { @@ -58,21 +65,34 @@ impl TryFrom for ZeroSubFunction { #[non_exhaustive] pub struct TesterPresentRequest { /// Whether the server should suppress a positive response (SPRMIB). - /// - /// `TesterPresent` defines only the zero sub-function, so the suppression flag is - /// the request's sole degree of freedom. pub suppress_positive_response: bool, + /// The sub-function byte with SPRMIB stripped. `TesterPresent` defines only the zero + /// sub-function, so conformant traffic always carries `0x00`; this is kept private so a + /// caller cannot mint a reserved value, but is retained on decode so that a reserved byte + /// re-encodes unchanged. Read it back with [`TesterPresentRequest::sub_function`]. + zero_sub_function: ZeroSubFunction, } impl TesterPresentRequest { - /// Create a new `TesterPresentRequest` + /// Create a new `TesterPresentRequest` carrying the zero sub-function. #[must_use] pub const fn new(suppress_positive_response: bool) -> Self { Self { suppress_positive_response, + zero_sub_function: ZeroSubFunction::NoSubFunctionSupported, } } + /// The sub-function byte, with the SPRMIB bit stripped. + /// + /// `0x00` for conformant traffic. `0x01..=0x7F` is reserved by ISO/SAE: the value is + /// retained rather than normalized so it re-encodes unchanged, and so a server can report + /// [`NegativeResponseCode::SubFunctionNotSupported`] against the byte it actually received. + #[must_use] + pub const fn sub_function(&self) -> u8 { + self.zero_sub_function.value() + } + /// Get the allowed [`NegativeResponseCode`] variants for this request #[must_use] pub fn allowed_nack_codes() -> &'static [NegativeResponseCode] { @@ -84,11 +104,11 @@ impl Encode for TesterPresentRequest { type Error = crate::Error; fn encode(&self, writer: &mut impl embedded_io::Write) -> Result { - // The only defined sub-function is the zero sub-function; fuse the SPRMIB bit - // onto it at the wire boundary. + // Fuse the SPRMIB bit back onto the sub-function at the wire boundary. The retained + // sub-function is written verbatim so a reserved value round-trips unchanged. let sub_function = SuppressablePositiveResponse::new( self.suppress_positive_response, - ZeroSubFunction::NoSubFunctionSupported, + self.zero_sub_function, ); write_u8(writer, u8::from(sub_function)).map_err(Error::io) } @@ -105,12 +125,13 @@ impl<'a> Decode<'a> for TesterPresentRequest { })); } // Split out the SPRMIB flag. Once SPRMIB is stripped the low 7 bits are always a - // valid zero sub-function, so this never rejects; the sub-function value itself is - // discarded and normalized to the zero sub-function on re-encode. + // valid zero sub-function, so this never rejects; the sub-function value is retained + // so that a reserved byte re-encodes unchanged. let sub_function = SuppressablePositiveResponse::::try_from(buf[0])?; Ok(( Self { suppress_positive_response: sub_function.suppress_positive_response(), + zero_sub_function: sub_function.value(), }, &buf[1..], )) @@ -129,11 +150,19 @@ pub struct TesterPresentResponse { impl TesterPresentResponse { /// Create a new `TesterPresentResponse` #[must_use] - pub fn new() -> Self { + pub const fn new() -> Self { Self { - zero_sub_function: ZeroSubFunction::default(), + zero_sub_function: ZeroSubFunction::NoSubFunctionSupported, } } + + /// The sub-function byte echoed by the server. `0x00` for conformant traffic; a reserved + /// `0x01..=0x7F` value is retained verbatim, mirroring + /// [`TesterPresentRequest::sub_function`]. + #[must_use] + pub const fn sub_function(&self) -> u8 { + self.zero_sub_function.value() + } } impl Default for TesterPresentResponse { @@ -221,22 +250,69 @@ mod test { assert_eq!(result.unwrap(), expected); } 0x01..=0x7F => { - // Reserved sub-function bytes decode (SPRMIB clear); the reserved value - // is not retained — re-encoding normalizes to the zero sub-function. - assert!(!result.unwrap().suppress_positive_response); + // Reserved sub-function bytes decode with SPRMIB clear, and the reserved + // value is retained verbatim (see + // `reserved_sub_function_survives_a_round_trip`). + let req = result.unwrap(); + assert!(!req.suppress_positive_response); + assert_eq!(req.sub_function(), i); } 0x80 => { let expected = TesterPresentRequest::new(true); assert_eq!(result.unwrap(), expected); } 0x81..=0xFF => { - // SPRMIB set over a reserved value: suppression is retained. - assert!(result.unwrap().suppress_positive_response); + // SPRMIB set over a reserved value: both the flag and the value are kept. + let req = result.unwrap(); + assert!(req.suppress_positive_response); + assert_eq!(req.sub_function(), i & 0x7F); } } } } + #[test] + fn reserved_sub_function_survives_a_round_trip() { + // Reserved sub-function bytes must re-encode byte-for-byte. Previously the value was + // discarded and normalized to 0x00, so `[0x3E, 0x01]` came back out as `[0x3E, 0x00]` — + // the request silently rewrote the tester's frame, while `TesterPresentResponse` + // preserved the same values. A server needs the original byte to report + // subFunctionNotSupported against it. + for raw in [0x00u8, 0x01, 0x42, 0x7F] { + for suppress in [false, true] { + let wire = [raw | if suppress { 0x80 } else { 0x00 }]; + let (req, rest) = ::decode(&wire).unwrap(); + assert!(rest.is_empty()); + assert_eq!(req.suppress_positive_response, suppress); + assert_eq!(req.sub_function(), raw, "sub-function byte not retained"); + + let mut buf = [0u8; 4]; + let n = Encode::encode(&req, &mut buf.as_mut_slice()).unwrap(); + assert_eq!(&buf[..n], &wire, "lossy re-encode for {wire:02X?}"); + assert_encode_size_agrees(&req); + } + } + } + + #[test] + fn reserved_sub_function_round_trips_through_the_request_frame() { + use crate::Request; + let wire = [0x3E, 0x01]; + let (req, _) = Request::decode(&wire).unwrap(); + let mut buf = [0u8; 4]; + let n = req.encode_to_slice(&mut buf).unwrap(); + assert_eq!(&buf[..n], &wire); + } + + #[test] + fn new_still_produces_the_zero_sub_function() { + // The common case keeps its one-argument constructor and its 0x00 encoding. + assert_eq!(TesterPresentRequest::new(false).sub_function(), 0x00); + let mut buf = [0u8; 4]; + let n = Encode::encode(&TesterPresentRequest::new(true), &mut buf.as_mut_slice()).unwrap(); + assert_eq!(&buf[..n], &[0x80]); + } + #[cfg(feature = "alloc")] #[test] fn write_request_type() { diff --git a/src/services/upload_download.rs b/src/services/upload_download.rs new file mode 100644 index 00000000..b89b3dc1 --- /dev/null +++ b/src/services/upload_download.rs @@ -0,0 +1,521 @@ +//! `RequestDownload` (0x34) and `RequestUpload` (0x35) service implementations. +//! +//! ISO 14229-1 gives the two services identical message layouts — request: +//! `dataFormatIdentifier`, `addressAndLengthFormatIdentifier`, `memoryAddress`, `memorySize`; +//! positive response: `lengthFormatIdentifier`, `maxNumberOfBlockLength` — differing only in +//! service identifier and in which direction the subsequent `TransferData` sequence moves the +//! bytes. Both pairs are generated from one macro so the wire codec has a single source of +//! truth; a fix to the width-derivation logic cannot land on one service and miss the other. + +use crate::shared::{DataFormatIdentifier, LengthFormatIdentifier, MemoryFormatIdentifier}; +use crate::{Decode, Encode, Error, Incomplete, NegativeResponseCode}; +use automotive_wire_codec::{read_be_uint_into, write_all, write_be_uint, write_u8}; + +/// Permitted NRCs for `RequestDownload` (0x34). +const REQUEST_DOWNLOAD_NEGATIVE_RESPONSE_CODES: [NegativeResponseCode; 6] = [ + NegativeResponseCode::IncorrectMessageLengthOrInvalidFormat, + NegativeResponseCode::ConditionsNotCorrect, + NegativeResponseCode::RequestOutOfRange, + NegativeResponseCode::SecurityAccessDenied, + NegativeResponseCode::AuthenticationRequired, + NegativeResponseCode::UploadDownloadNotAccepted, +]; + +/// Permitted NRCs for `RequestUpload` (0x35). ISO 14229-1 specifies the same set as +/// `RequestDownload`; kept as a separate constant so either service can diverge later without +/// silently changing the other. +const REQUEST_UPLOAD_NEGATIVE_RESPONSE_CODES: [NegativeResponseCode; 6] = [ + NegativeResponseCode::IncorrectMessageLengthOrInvalidFormat, + NegativeResponseCode::ConditionsNotCorrect, + NegativeResponseCode::RequestOutOfRange, + NegativeResponseCode::SecurityAccessDenied, + NegativeResponseCode::AuthenticationRequired, + NegativeResponseCode::UploadDownloadNotAccepted, +]; + +macro_rules! upload_download_service { + ( + request: $req:ident, + response: $resp:ident, + nrcs: $nrcs:ident, + request_doc: $req_doc:literal, + response_doc: $resp_doc:literal, + verb: $verb:literal, + tests: $test_mod:ident, + ) => { + #[doc = $req_doc] + /// + /// This is a variable length request, determined by the + /// `address_and_length_format_identifier` value. + /// See ISO-14229-1:2020, Table H.1 for format information. + #[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))] + #[cfg_attr(feature = "utoipa", derive(utoipa::ToSchema))] + #[derive(Clone, Copy, Debug, Eq, PartialEq)] + #[non_exhaustive] + pub struct $req { + /// compression method (high nibble) and encrypting method (low nibble). 0x00 is no + /// compression or encryption + data_format_identifier: DataFormatIdentifier, + /// 7-4: length (# of bytes) of `memory_size` param, 3-0: length (# of bytes) of + /// `memory_address` param + address_and_length_format_identifier: MemoryFormatIdentifier, + /// Starting address of the server memory. The on-wire byte width is derived from + /// this value (max 5 bytes), so it is private to keep it in sync with the format + /// identifier. + memory_address: u64, + #[doc = concat!("Size of the data to be ", $verb, ". The on-wire byte width is")] + /// derived from this value (max 4 bytes), so it is private to keep it in sync with + /// the format identifier. + memory_size: u32, + } + + impl $req { + #[doc = concat!("Create a new `", stringify!($req), "`")] + /// + /// # Errors + /// Returns an error if `memory_address` exceeds 5 bytes (> `0xFF_FFFF_FFFF`). + #[allow(clippy::cast_possible_truncation)] + pub const fn new( + data_format_identifier: DataFormatIdentifier, + memory_address: u64, + memory_size: u32, + ) -> Result { + if memory_address > 0xFF_FFFF_FFFF { + return Err(Error::InvalidMemoryAddress(memory_address)); + } + // A length of 0 produces an invalid `MemoryFormatIdentifier` (the nibbles + // must be >=1 per ISO-14229), so clamp to at least one byte even when the + // address or size is 0. Written as `if` rather than `.max(1)` because + // `Ord::max` is not callable in a `const fn`. + let address_bytes = (u64::BITS - memory_address.leading_zeros()).div_ceil(8) as u8; + let memory_address_length = if address_bytes == 0 { 1 } else { address_bytes }; + let size_bytes = (u32::BITS - memory_size.leading_zeros()).div_ceil(8) as u8; + let memory_size_length = if size_bytes == 0 { 1 } else { size_bytes }; + let address_and_length_format_identifier = MemoryFormatIdentifier { + memory_size_length, + memory_address_length, + }; + Ok(Self { + data_format_identifier, + address_and_length_format_identifier, + memory_address, + memory_size, + }) + } + + /// The compression and encryption methods the client asked the server to use. + /// + /// A server has to act on this, so it must be readable back off a decoded request; + /// use [`DataFormatIdentifier::compression_method`] and + /// [`DataFormatIdentifier::encryption_method`] for the individual nibbles. + #[must_use] + pub const fn data_format_identifier(&self) -> DataFormatIdentifier { + self.data_format_identifier + } + + /// Starting address of the server memory. + #[must_use] + pub const fn memory_address(&self) -> u64 { + self.memory_address + } + + #[doc = concat!("Size of the data to be ", $verb, ".")] + #[must_use] + pub const fn memory_size(&self) -> u32 { + self.memory_size + } + + /// Get the allowed [`NegativeResponseCode`] variants for this request + #[must_use] + pub fn allowed_nack_codes() -> &'static [NegativeResponseCode] { + &$nrcs + } + } + + impl Encode for $req { + type Error = crate::Error; + + fn encode(&self, writer: &mut impl embedded_io::Write) -> Result { + let mut written = write_all( + writer, + &[ + self.data_format_identifier.into(), + self.address_and_length_format_identifier.into(), + ], + ) + .map_err(Error::io)?; + + let addr_len = self + .address_and_length_format_identifier + .memory_address_length as usize; + let size_len = + self.address_and_length_format_identifier.memory_size_length as usize; + written += write_be_uint(writer, u128::from(self.memory_address), addr_len)?; + written += write_be_uint(writer, u128::from(self.memory_size), size_len)?; + + Ok(written) + } + } + + impl<'a> Decode<'a> for $req { + type Error = crate::Error; + + fn decode(buf: &'a [u8]) -> Result<(Self, &'a [u8]), Error> { + if buf.len() < 2 { + return Err(Error::InsufficientData(Incomplete { + needed: 2, + available: buf.len(), + })); + } + let data_format_identifier = DataFormatIdentifier::from(buf[0]); + let memory_identifier = MemoryFormatIdentifier::try_from(buf[1])?; + let addr_len = memory_identifier.memory_address_length as usize; + let size_len = memory_identifier.memory_size_length as usize; + let total = 2 + addr_len + size_len; + if buf.len() < total { + return Err(Error::InsufficientData(Incomplete { + needed: total, + available: buf.len(), + })); + } + + let (memory_address, rest) = read_be_uint_into::(&buf[2..], addr_len)?; + let (memory_size, _rest) = read_be_uint_into::(rest, size_len)?; + + Ok(( + Self { + data_format_identifier, + address_and_length_format_identifier: memory_identifier, + memory_address, + memory_size, + }, + &buf[total..], + )) + } + } + + #[doc = $resp_doc] + #[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))] + #[cfg_attr(feature = "utoipa", derive(utoipa::ToSchema))] + #[derive(Clone, Copy, Debug, Eq, PartialEq)] + #[non_exhaustive] + pub struct $resp<'d> { + /// Maximum number of bytes per [`TransferDataRequest`](crate::TransferDataRequest). + /// + /// The on-wire `lengthFormatIdentifier` nibble is derived from this slice's length + /// at encode time, so the declared length can never disagree with the bytes present. + #[cfg_attr(feature = "serde", serde(borrow))] + pub max_number_of_block_length: &'d [u8], + } + + impl<'d> $resp<'d> { + #[doc = concat!("Create a new `", stringify!($resp), "`. The `lengthFormatIdentifier`")] + /// is derived from `max_number_of_block_length` during encoding. + #[must_use] + pub const fn new(max_number_of_block_length: &'d [u8]) -> Self { + Self { + max_number_of_block_length, + } + } + } + + impl Encode for $resp<'_> { + type Error = crate::Error; + + fn encode(&self, writer: &mut impl embedded_io::Write) -> Result { + // The block-length field width is carried in a single nibble, so the slice + // can be at most 0x0F bytes long. + let nibble = u8::try_from(self.max_number_of_block_length.len()) + .ok() + .filter(|n| *n <= 0x0F) + .ok_or(Error::IncorrectMessageLengthOrInvalidFormat)?; + let length_format_identifier = LengthFormatIdentifier { + max_number_of_block_length: nibble, + }; + let mut written = + write_u8(writer, length_format_identifier.into()).map_err(Error::io)?; + written += write_all(writer, self.max_number_of_block_length).map_err(Error::io)?; + Ok(written) + } + } + + impl<'a> Decode<'a> for $resp<'a> { + type Error = crate::Error; + + fn decode(buf: &'a [u8]) -> Result<(Self, &'a [u8]), Error> { + if buf.is_empty() { + return Err(Error::InsufficientData(Incomplete { + needed: 1, + available: buf.len(), + })); + } + let length_format_identifier = LengthFormatIdentifier::from(buf[0]); + let len = length_format_identifier.max_number_of_block_length as usize; + let total = 1 + len; + if buf.len() < total { + return Err(Error::InsufficientData(Incomplete { + needed: total, + available: buf.len(), + })); + } + Ok(( + Self { + max_number_of_block_length: &buf[1..total], + }, + &buf[total..], + )) + } + } + + // Both services get the same coverage, generated alongside them so neither can drift. + #[cfg(test)] + mod $test_mod { + use super::*; + use crate::{Decode, Encode, test_util::assert_encode_size_agrees}; + + #[test] + fn simple_request() { + let bytes: [u8; 7] = [ + 0x00, // No compression or encryption + 0x14, // 1 byte for memory size, 4 bytes for memory address + 0xF0, 0xFF, 0xFF, 0x67, // memory address + 0x0A, + ]; + let (req, _) = <$req as Decode>::decode(&bytes).unwrap(); + + assert_eq!(u8::from(req.data_format_identifier), 0); + assert_eq!(u8::from(req.address_and_length_format_identifier), 0x14); + assert_eq!( + req.address_and_length_format_identifier.memory_size_length, + 1 + ); + assert_eq!( + req.address_and_length_format_identifier + .memory_address_length, + 4 + ); + + assert_eq!(req.memory_address(), 0xF0FF_FF67); + assert_eq!(req.memory_size(), 0x0A); + } + + #[test] + fn bad_request() { + let bytes: [u8; 3] = [ + 0x00, // No compression or encryption + 0x11, // 1 byte for memory size, 1 byte for memory address + 0x67, + ]; + let result = <$req as Decode>::decode(&bytes); + assert!(result.is_err()); + } + + #[test] + fn zero_address_and_size_clamp_to_one_byte() { + // A 0 address/size must still produce a valid (>=1 byte) length nibble, + // otherwise the encoded frame cannot be decoded back. + let req = $req::new(0x00.into(), 0, 0).unwrap(); + assert_eq!( + req.address_and_length_format_identifier + .memory_address_length, + 1 + ); + assert_eq!( + req.address_and_length_format_identifier.memory_size_length, + 1 + ); + + let mut buf = [0u8; 8]; + let written = Encode::encode(&req, &mut buf.as_mut_slice()).unwrap(); + let (decoded, _) = <$req as Decode>::decode(&buf[..written]).unwrap(); + assert_eq!(decoded.memory_address(), 0); + assert_eq!(decoded.memory_size(), 0); + } + + #[test] + fn check_message_size() { + let req = $req::new(0x00.into(), 0xF0_FF_FF_67, 0x0A).unwrap(); + let mut buf = [0u8; 16]; + let written = Encode::encode(&req, &mut buf.as_mut_slice()).unwrap(); + + assert_eq!(written, req.encoded_size().unwrap()); + assert_encode_size_agrees(&req); + } + + #[test] + fn new_derives_the_alfid_nibbles_in_wire_order() { + // The two nibbles are asymmetric here on purpose: the high nibble is the + // memorySize width and the low nibble the memoryAddress width, so transposing + // them changes these bytes. A symmetric case (or a length-only assertion) + // cannot tell the two apart, and a swap silently truncates the address. + for (address, size, want) in [ + // 4-byte address, 1-byte size -> ALFID 0x14 + ( + 0xF0FF_FF67u64, + 0x0Au32, + [0x00, 0x14, 0xF0, 0xFF, 0xFF, 0x67, 0x0A].as_slice(), + ), + // 1-byte address, 2-byte size -> ALFID 0x21 + (0xBE, 0x0100, [0x00, 0x21, 0xBE, 0x01, 0x00].as_slice()), + ] { + let req = $req::new(DataFormatIdentifier::NONE, address, size).unwrap(); + let mut buf = [0u8; 16]; + let written = Encode::encode(&req, &mut buf.as_mut_slice()).unwrap(); + assert_eq!( + &buf[..written], + want, + "wrong wire bytes for address {address:#X} size {size:#X}" + ); + + // ...and the frame the crate emits must be one the crate can read back. + let decoded = <$req as Decode>::decode_exact(&buf[..written]).unwrap(); + assert_eq!(decoded.memory_address(), address); + assert_eq!(decoded.memory_size(), size); + } + } + + #[test] + fn the_widest_legal_address_and_size_round_trip() { + // Annex H Table H.1 permits a 4-byte memorySize and a 5-byte memoryAddress + // (ALFID 0x45), which `new` derives for these values. The decoder used to + // reject both widths, so the crate could not read back its own output for any + // transfer above 16 MB or to an address above 4 GB. + let req = + $req::new(DataFormatIdentifier::NONE, 0xFF_FFFF_FFFF, 0xFFFF_FFFF).unwrap(); + let mut buf = [0u8; 16]; + let written = Encode::encode(&req, &mut buf.as_mut_slice()).unwrap(); + assert_eq!( + &buf[..written], + &[ + 0x00, 0x45, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF + ], + ); + + let decoded = <$req as Decode>::decode_exact(&buf[..written]).unwrap(); + assert_eq!(decoded.memory_address(), 0xFF_FFFF_FFFF); + assert_eq!(decoded.memory_size(), 0xFFFF_FFFF); + } + + #[test] + fn address_beyond_five_bytes_is_rejected() { + assert!(matches!( + $req::new(0x00.into(), 0x1_00_0000_0000, 0x0A), + Err(Error::InvalidMemoryAddress(_)) + )); + } + + #[test] + fn response_encode_size_agrees() { + let block = [0x10u8, 0x00, 0x00]; + let resp = $resp::new(&block); + assert_encode_size_agrees(&resp); + } + + #[test] + fn response_round_trips() { + let block = [0x02u8, 0x00]; + let resp = $resp::new(&block); + let mut buf = [0u8; 8]; + let n = Encode::encode(&resp, &mut buf.as_mut_slice()).unwrap(); + assert_eq!(&buf[..n], &[0x20, 0x02, 0x00]); + let (decoded, rest) = <$resp as Decode>::decode(&buf[..n]).unwrap(); + assert!(rest.is_empty()); + assert_eq!(decoded.max_number_of_block_length, &block); + } + + #[test] + fn data_format_identifier_is_readable_off_a_decoded_request() { + // A server has to act on the compression/encryption methods it was asked for. + // All fields are private (the width nibbles must stay in sync with the values), + // so without the getter the DFI was write-only: constructible but unreadable + // after a decode. + // Wire: DFI=0x21 (compression 2, encryption 1), ALFID=0x12 (size 1, addr 2), + // addr=0xBEEF, size=0x10 + let wire = [0x21, 0x12, 0xBE, 0xEF, 0x10]; + let req = <$req as Decode>::decode_exact(&wire).unwrap(); + let dfi = req.data_format_identifier(); + assert_eq!(dfi.compression_method(), 0x02); + assert_eq!(dfi.encryption_method(), 0x01); + assert_eq!(u8::from(dfi), 0x21); + assert_eq!(req.memory_address(), 0xBEEF); + assert_eq!(req.memory_size(), 0x10); + } + + #[test] + fn data_format_identifier_survives_construction() { + // new(compression, encryption) — wire order. + let dfi = DataFormatIdentifier::new(0x0A, 0x0B).unwrap(); + let req = $req::new(dfi, 0x1234, 0x10).unwrap(); + assert_eq!(req.data_format_identifier(), dfi); + assert_eq!(req.data_format_identifier().compression_method(), 0x0A); + assert_eq!(req.data_format_identifier().encryption_method(), 0x0B); + } + + #[test] + fn exposes_allowed_nack_codes() { + assert!(!$req::allowed_nack_codes().is_empty()); + assert!( + $req::allowed_nack_codes() + .contains(&NegativeResponseCode::UploadDownloadNotAccepted) + ); + } + + #[test] + fn derive_contract() { + use crate::test_util::assert_impl_eq; + assert_impl_eq::<$req>(); + assert_impl_eq::<$resp<'static>>(); + #[cfg(feature = "serde")] + { + use crate::test_util::assert_impl_serde; + assert_impl_serde::<$req>(); + assert_impl_serde::<$resp<'static>>(); + } + } + } + }; +} + +upload_download_service! { + request: RequestDownloadRequest, + response: RequestDownloadResponse, + nrcs: REQUEST_DOWNLOAD_NEGATIVE_RESPONSE_CODES, + request_doc: "A request to the server for it to download data from the client.\n\nA positive response ([`RequestDownloadResponse`]) is sent once the server has taken all necessary actions and is ready to receive the data.", + response_doc: "Zero-alloc positive response to a [`RequestDownloadRequest`], indicating the server is ready to receive data. Borrows from the caller.", + verb: "downloaded", + tests: request_download_tests, +} + +upload_download_service! { + request: RequestUploadRequest, + response: RequestUploadResponse, + nrcs: REQUEST_UPLOAD_NEGATIVE_RESPONSE_CODES, + request_doc: "A request to the server for it to upload data to the client.\n\nA positive response ([`RequestUploadResponse`]) is sent once the server is ready to transmit; the client then drives the transfer with [`TransferDataRequest`](crate::TransferDataRequest), reading the data out of each positive response, and finishes with [`RequestTransferExitRequest`](crate::RequestTransferExitRequest).", + response_doc: "Zero-alloc positive response to a [`RequestUploadRequest`], indicating the server is ready to transmit data. Borrows from the caller.", + verb: "uploaded", + tests: request_upload_tests, +} + +#[cfg(test)] +mod shared_layout_tests { + use super::*; + use crate::Decode; + + #[test] + fn download_and_upload_share_one_wire_layout() { + // The two services are byte-identical apart from the service identifier, which is + // added by the `Request`/`Response` frame layer, not by these payload codecs. This + // pins that equivalence so the macro cannot silently drift for one of them. + let wire = [0x00, 0x14, 0xF0, 0xFF, 0xFF, 0x67, 0x0A]; + let down = ::decode_exact(&wire).unwrap(); + let up = ::decode_exact(&wire).unwrap(); + assert_eq!(down.memory_address(), up.memory_address()); + assert_eq!(down.memory_size(), up.memory_size()); + assert_eq!(down.data_format_identifier(), up.data_format_identifier()); + assert_eq!( + down.encoded_size().unwrap(), + up.encoded_size().unwrap(), + "payload widths diverged" + ); + } +} diff --git a/src/services/write_data_by_identifier.rs b/src/services/write_data_by_identifier.rs index f2d8d752..2790a5a0 100644 --- a/src/services/write_data_by_identifier.rs +++ b/src/services/write_data_by_identifier.rs @@ -31,9 +31,17 @@ pub struct WriteDataByIdentifierRequest<'d> { impl<'d> WriteDataByIdentifierRequest<'d> { /// Create a request to write `data` to the given Data Identifier. - #[must_use] - pub const fn new(identifier: u16, data: &'d [u8]) -> Self { - Self { identifier, data } + /// + /// # Errors + /// Returns [`Error::IncorrectMessageLengthOrInvalidFormat`] if `data` is empty. ISO + /// 14229-1:2020 Table 277 marks the first `dataRecord` byte mandatory, so a request with + /// no data record is malformed — and would encode to a frame this crate's own decoder + /// rejects. + pub const fn new(identifier: u16, data: &'d [u8]) -> Result { + if data.is_empty() { + return Err(Error::IncorrectMessageLengthOrInvalidFormat); + } + Ok(Self { identifier, data }) } /// Get the allowed [`NegativeResponseCode`] variants for this request. @@ -56,10 +64,12 @@ impl Encode for WriteDataByIdentifierRequest<'_> { impl<'a> Decode<'a> for WriteDataByIdentifierRequest<'a> { type Error = crate::Error; + /// The 2-byte DID and at least one `dataRecord` byte are mandatory (ISO 14229-1:2020 + /// Table 277, and Figure 26's "minimum length is 4 byte (SI + DID + DREC)"). fn decode(buf: &'a [u8]) -> Result<(Self, &'a [u8]), Error> { - if buf.len() < 2 { + if buf.len() < 3 { return Err(Error::InsufficientData(Incomplete { - needed: 2, + needed: 3, available: buf.len(), })); } @@ -167,7 +177,7 @@ mod test { #[test] fn wdbi_request_round_trips() { - let req = WriteDataByIdentifierRequest::new(0xF190, &[0x01, 0x02, 0x03]); + let req = WriteDataByIdentifierRequest::new(0xF190, &[0x01, 0x02, 0x03]).unwrap(); let mut buf = [0u8; 8]; let n = Encode::encode(&req, &mut buf.as_mut_slice()).unwrap(); assert_eq!(&buf[..n], &[0xF1, 0x90, 0x01, 0x02, 0x03]); @@ -179,17 +189,35 @@ mod test { } #[test] - fn wdbi_request_allows_empty_data() { - let (decoded, _) = ::decode(&[0xF1, 0x90]).unwrap(); - assert_eq!(decoded.identifier, 0xF190); - assert!(decoded.data.is_empty()); + fn wdbi_request_requires_at_least_one_data_byte() { + // ISO 14229-1:2020 Table 277 marks dataRecord `data#1` as `M` (only `data#2..#m` are + // `U`), and Figure 26's key states "minimum length is 4 byte (SI + DID + DREC)". A + // 3-byte message used to decode into an empty data record, so a server would attempt a + // zero-length write rather than answering NRC 0x13. + let err = ::decode(&[0xF1, 0x90]) + .expect_err("a write with no data record must be rejected"); + assert!( + matches!( + err, + Error::InsufficientData(Incomplete { + needed: 3, + available: 2 + }) + ), + "expected a 3-byte shortfall, got {err:?}" + ); + + // ...and the constructor must not mint a request the decoder would reject. + assert!(WriteDataByIdentifierRequest::new(0xF190, &[]).is_err()); + let req = WriteDataByIdentifierRequest::new(0xF190, &[0x01]).unwrap(); + assert_eq!(req.data, &[0x01]); } #[test] fn wdbi_request_rejects_short_buffer() { assert!(matches!( ::decode(&[0xF1]), - Err(Error::InsufficientData(i)) if i.needed == 2 && i.available == 1 + Err(Error::InsufficientData(i)) if i.needed == 3 && i.available == 1 )); } } diff --git a/src/shared/format_identifiers.rs b/src/shared/format_identifiers.rs index 38731e14..f69a813f 100644 --- a/src/shared/format_identifiers.rs +++ b/src/shared/format_identifiers.rs @@ -3,10 +3,25 @@ use crate::Error; const LOW_NIBBLE_MASK: u8 = 0b0000_1111; const HIGH_NIBBLE_MASK: u8 = 0b1111_0000; +/// Largest value that fits in a single nibble. +const NIBBLE_MAX: u8 = 0x0F; + /// Address and length format identifier const MEMORY_SIZE_NIBBLE_MASK: u8 = HIGH_NIBBLE_MASK; const MEMORY_ADDRESS_NIBBLE_MASK: u8 = LOW_NIBBLE_MASK; +/// Widest `memorySize` the `addressAndLengthFormatIdentifier` may declare, in bytes. +/// +/// ISO 14229-1:2020 Annex H Table H.1 marks high-nibble values 1 through 4 applicable +/// (manageable size 256 bytes through 4 GB) and everything else "not applicable". +pub(crate) const MAX_MEMORY_SIZE_LENGTH: u8 = 4; + +/// Widest `memoryAddress` the `addressAndLengthFormatIdentifier` may declare, in bytes. +/// +/// Table H.1 marks low-nibble values 1 through 5 applicable (addressable memory 256 bytes +/// through 1024 GB - 1). +pub(crate) const MAX_MEMORY_ADDRESS_LENGTH: u8 = 5; + /// Length format identifier const BLOCK_LENGTH_NIBBLE_MASK: u8 = HIGH_NIBBLE_MASK; @@ -17,7 +32,11 @@ const ENCRYPTION_NIBBLE_MASK: u8 = LOW_NIBBLE_MASK; /// Takes in the actual memory address to be used and the size of the memory to be used /// and computes how many bytes are needed to represent them /// -/// Decoded from the `address_and_length_format_identifier` field of the [`crate::RequestDownloadRequest`] struct +/// Carried by the `addressAndLengthFormatIdentifier` byte of +/// [`RequestDownloadRequest`](crate::RequestDownloadRequest) and +/// [`RequestUploadRequest`](crate::RequestUploadRequest), which share one message layout. +/// Derived from the address and size rather than set by the caller, so it is not part of +/// either type's public surface. /// /// See ISO-14229-1:2020, Table H.1 for format information #[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))] @@ -30,26 +49,23 @@ pub(crate) struct MemoryFormatIdentifier { impl TryFrom for MemoryFormatIdentifier { type Error = Error; - // NRC::RequestOutOfRange if address_and_length_format_identifier is not valid fn try_from(value: u8) -> Result { + // High nibble: bytes used for the memorySize parameter. Table H.1 marks 1 through 4 + // applicable (manageable size 256 bytes to 4 GB); 0 and 5..=15 are "not applicable". let memory_size_length = (value & MEMORY_SIZE_NIBBLE_MASK) >> 4; + // Low nibble: bytes used for the memoryAddress parameter. Table H.1 marks 1 through 5 + // applicable (addressable memory 256 bytes to 1024 GB - 1). let memory_address_length = value & MEMORY_ADDRESS_NIBBLE_MASK; - match memory_size_length { - 1..4 => (), - _ => return Err(Error::IncorrectMessageLengthOrInvalidFormat), + if !matches!(memory_size_length, 1..=MAX_MEMORY_SIZE_LENGTH) { + return Err(Error::IncorrectMessageLengthOrInvalidFormat); } - match memory_address_length { - 1..5 => (), - _ => return Err(Error::IncorrectMessageLengthOrInvalidFormat), + if !matches!(memory_address_length, 1..=MAX_MEMORY_ADDRESS_LENGTH) { + return Err(Error::IncorrectMessageLengthOrInvalidFormat); } Ok(Self { - // get the low nibble of address_and_length_format_identifier - // Memory size length is 1 through 4 bytes (manageable size: 256 bytes to 4GB) memory_size_length, - // get the high nibble of address_and_length_format_identifier - // Memory address is 1 through 5 bytes (addressable memory: 256 bytes - 1024GB) - memory_address_length: value & MEMORY_ADDRESS_NIBBLE_MASK, + memory_address_length, }) } } @@ -61,10 +77,13 @@ impl From for u8 { } } -/// Decoded from the `length_format_identifier` field of the [`RequestDownloadResponse`] struct. -/// The format is similar to the `address_and_length_format_identifier` field in the [`RequestDownloadRequest`] struct. -/// Specifically, it is a byte where the high nibble represents the byte length of the `max_number_of_block_length` field, -/// i.e, a value of `0x20` indicates that the `max_number_of_block_length` field is 2 bytes long. +/// The leading byte of a [`RequestDownloadResponse`](crate::RequestDownloadResponse) or +/// [`RequestUploadResponse`](crate::RequestUploadResponse), which share one message layout. +/// +/// The format mirrors [`MemoryFormatIdentifier`]: a byte whose high nibble gives the byte +/// length of `max_number_of_block_length`, i.e. `0x20` means that field is 2 bytes long. +/// Derived from the slice length when encoding, so it is not part of either response's +/// public surface. #[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))] #[cfg_attr(feature = "utoipa", derive(utoipa::ToSchema))] #[derive(Clone, Copy, Debug, Eq, PartialEq)] @@ -85,11 +104,18 @@ impl From for u8 { } } -/// Used by [`crate::RequestDownloadRequest`] for the compression method (high nibble) and encrypting method (low nibble) -/// - 0x00 is no compression or encryption, which is the default +/// The compression method (high nibble) and encryption method (low nibble) a client asks the +/// server to use for a transfer. /// -/// Decoded from the `data_format_identifier` field of the [`crate::RequestDownloadRequest`] struct -/// Values other than 0x00 are Vehicle Manufacturer specific according to ISO-14229-1:2020 +/// - `0x00` for both means no compression and no encryption, which is the default; prefer +/// [`DataFormatIdentifier::NONE`]. +/// - Values other than `0x00` are Vehicle Manufacturer specific according to ISO-14229-1:2020. +/// +/// Supplied to [`RequestDownloadRequest::new`](crate::RequestDownloadRequest::new) and +/// [`RequestUploadRequest::new`](crate::RequestUploadRequest::new), and read back with +/// [`RequestDownloadRequest::data_format_identifier`](crate::RequestDownloadRequest::data_format_identifier). +/// Also carried by the `AddFile`, `ReplaceFile`, `ReadFile` and `ResumeFile` variants of +/// [`RequestFileTransferRequest`](crate::RequestFileTransferRequest). #[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))] #[cfg_attr(feature = "utoipa", derive(utoipa::ToSchema))] #[derive(Clone, Copy, Debug, Eq, PartialEq)] @@ -101,25 +127,56 @@ pub struct DataFormatIdentifier { } impl DataFormatIdentifier { + /// No compression and no encryption — the `0x00` byte, and the overwhelmingly common case. + pub const NONE: Self = Self { + compression_method: 0, + encryption_method: 0, + }; + /// Build a `DataFormatIdentifier` from its compression and encryption method nibbles. /// - /// `0x00` for both means no compression and no encryption (the default). Both values - /// occupy a single nibble on the wire. + /// Arguments are in **wire order**: compression is the high nibble, encryption the low + /// nibble. `0x00` for both means no compression and no encryption — prefer + /// [`DataFormatIdentifier::NONE`] for that. Each value occupies a single nibble. + /// + /// Both parameters are `u8`, so the compiler cannot catch a transposition; if you are + /// converting a byte you already have, use `DataFormatIdentifier::from(byte)` instead. /// /// # Errors /// Returns [`Error::InvalidEncryptionCompressionMethod`] if either value does not fit /// in a nibble (i.e. is greater than `0x0F`). - pub fn new(encryption_method: u8, compression_method: u8) -> Result { + // Written as explicit range checks rather than a `?` on a helper: `?` is not permitted in + // a `const fn`, and const construction is what lets callers put a `DataFormatIdentifier` + // in a `const` table. + pub const fn new(compression_method: u8, encryption_method: u8) -> Result { + if compression_method > NIBBLE_MAX { + return Err(Error::InvalidEncryptionCompressionMethod( + compression_method, + )); + } + if encryption_method > NIBBLE_MAX { + return Err(Error::InvalidEncryptionCompressionMethod(encryption_method)); + } Ok(Self { - encryption_method: Self::check_value(encryption_method)?, - compression_method: Self::check_value(compression_method)?, + encryption_method, + compression_method, }) } - fn check_value(value: u8) -> Result { - match value { - 0..=15 => Ok(value), - _ => Err(Error::InvalidEncryptionCompressionMethod(value)), - } + + /// The compression method nibble (the high nibble on the wire). + /// + /// `0x00` means no compression. Other values are vehicle-manufacturer specific. + #[must_use] + pub const fn compression_method(&self) -> u8 { + self.compression_method + } + + /// The encryption method nibble (the low nibble on the wire). + /// + /// `0x00` means no encryption. Other values are vehicle-manufacturer specific. + #[must_use] + pub const fn encryption_method(&self) -> u8 { + self.encryption_method } } impl From for DataFormatIdentifier { @@ -168,6 +225,35 @@ mod tests { )); } + #[test] + fn every_alfid_legal_per_table_h1_is_accepted() { + // ISO 14229-1:2020 Annex H Table H.1 runs from 0x11 to 0x45: the high nibble + // (memorySize) is applicable for 1..=4 bytes and the low nibble (memoryAddress) for + // 1..=5. Anything outside that is "not applicable" and must be rejected. + for size_len in 1..=4u8 { + for addr_len in 1..=5u8 { + let byte = (size_len << 4) | addr_len; + let mfi = MemoryFormatIdentifier::try_from(byte) + .unwrap_or_else(|e| panic!("Table H.1 lists {byte:#04X} as valid, got {e:?}")); + assert_eq!(mfi.memory_size_length, size_len, "for {byte:#04X}"); + assert_eq!(mfi.memory_address_length, addr_len, "for {byte:#04X}"); + assert_eq!(u8::from(mfi), byte, "round trip for {byte:#04X}"); + } + } + } + + #[test] + fn alfid_nibbles_outside_table_h1_are_rejected() { + // A zero nibble is "not applicable" on either side, and the widths stop at 4 (size) + // and 5 (address). + for byte in [0x00, 0x01, 0x10, 0x05, 0x50, 0x46, 0x55, 0xF5, 0x5F] { + assert!( + MemoryFormatIdentifier::try_from(byte).is_err(), + "{byte:#04X} is not applicable per Table H.1 but was accepted" + ); + } + } + #[test] fn length_format_identifier() { let length_format_identifier = LengthFormatIdentifier::from(0xF0); @@ -192,6 +278,19 @@ mod tests { data_format_identifier, Err(Error::InvalidEncryptionCompressionMethod(0x1F)) )); + + // Arguments are in wire order: compression is the high nibble. + let dfi = DataFormatIdentifier::new(0x02, 0x01).unwrap(); + assert_eq!(dfi.compression_method(), 0x02); + assert_eq!(dfi.encryption_method(), 0x01); + assert_eq!( + u8::from(dfi), + 0x21, + "compression must land in the high nibble" + ); + + assert_eq!(u8::from(DataFormatIdentifier::NONE), 0x00); + assert_eq!(DataFormatIdentifier::NONE, DataFormatIdentifier::from(0x00)); } mod prop { @@ -217,8 +316,11 @@ mod tests { #[test] fn prop_memory_format_identifier_roundtrip( - size_len in 1u8..=3, - addr_len in 1u8..=4, + // The full range Annex H Table H.1 declares applicable. Narrowing these to + // what the decoder happened to accept is what previously let an off-by-one in + // the range checks sit underneath a passing property test. + size_len in 1u8..=MAX_MEMORY_SIZE_LENGTH, + addr_len in 1u8..=MAX_MEMORY_ADDRESS_LENGTH, ) { let byte = (size_len << 4) | addr_len; let mfi = MemoryFormatIdentifier::try_from(byte).unwrap(); diff --git a/src/shared/mod.rs b/src/shared/mod.rs index f658b86e..ff4da0bc 100644 --- a/src/shared/mod.rs +++ b/src/shared/mod.rs @@ -5,7 +5,9 @@ mod negative_response_code; pub use negative_response_code::NegativeResponseCode; mod suppressable_positive_response; -pub(crate) use suppressable_positive_response::SuppressablePositiveResponse; +pub(crate) use suppressable_positive_response::{ + SuppressablePositiveResponse, fuse_sprmib, split_sprmib, +}; mod format_identifiers; pub use format_identifiers::DataFormatIdentifier; diff --git a/src/shared/suppressable_positive_response.rs b/src/shared/suppressable_positive_response.rs index 5fff45b0..7a7e9d97 100644 --- a/src/shared/suppressable_positive_response.rs +++ b/src/shared/suppressable_positive_response.rs @@ -7,6 +7,25 @@ const SPRMIB: u8 = 0x80; /// Mask to recover value in byte with SPRMIB pub(crate) const SPRMIB_VALUE_MASK: u8 = 0x7F; +/// Split a sub-function byte into its SPRMIB flag and its value bits. +/// +/// For sub-function enumerations that implement `TryFrom`, prefer +/// [`SuppressablePositiveResponse`], which carries both halves as one value. This is for the +/// services whose sub-function byte does not by itself determine the variant, because the +/// variant also depends on the parameter bytes that follow it (`ReadDTCInformation`). +pub(crate) const fn split_sprmib(byte: u8) -> (bool, u8) { + (byte & SPRMIB == SPRMIB, byte & SPRMIB_VALUE_MASK) +} + +/// Fuse a SPRMIB flag back into a sub-function byte. The inverse of [`split_sprmib`]. +pub(crate) const fn fuse_sprmib(suppress_positive_response: bool, value: u8) -> u8 { + if suppress_positive_response { + value | SPRMIB + } else { + value + } +} + /// `SuppressablePositiveResponse` is used to encapsulate subfunction enumerations that can also encode the response suppression bit. /// This eliminates bit masking logic from a number of subfunction enumerations. #[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))] diff --git a/tests/public_api.rs b/tests/public_api.rs new file mode 100644 index 00000000..3ba00116 --- /dev/null +++ b/tests/public_api.rs @@ -0,0 +1,35 @@ +//! Checks the public API from outside the crate. +//! +//! An integration test is a separate crate, so `#[non_exhaustive]` applies here exactly as it +//! does for a downstream user. Inline `#[cfg(test)]` modules cannot check this: inside the +//! defining crate a `#[non_exhaustive]` struct literal compiles fine, so a type that is +//! impossible for anyone else to build still looks constructible from there. + +use uds_protocol::{ + DirSizePayload, DtcFaultDetectionCounterRecord, DtcRecord, FileOperationMode, FileSizePayload, + NamePayload, PositionPayload, SentDataPayload, SizePayload, +}; + +#[test] +fn dtc_fault_detection_counter_record_is_constructible_downstream() { + // This is the `Item` type of a public iterator and is re-exported at the crate root so + // callers can name it. Without a constructor, `#[non_exhaustive]` made the only way to + // obtain one decoding bytes through `DtcFaultDetectionIter` — E0639 out here. + let record = DtcFaultDetectionCounterRecord::new(DtcRecord::new(0x01, 0x02, 0x03), 0x2A); + assert_eq!(record.dtc_record, DtcRecord::new(0x01, 0x02, 0x03)); + assert_eq!(record.dtc_fault_detection_counter, 0x2A); +} + +#[test] +fn every_non_exhaustive_payload_type_has_a_reachable_constructor() { + // One commit added `#[non_exhaustive]` to seven public structs with `pub` fields. Six had + // a constructor and one did not, which made it unbuildable outside the crate. Pin all + // seven so the next `#[non_exhaustive]` cannot reintroduce the gap silently. + let _ = SizePayload::new(0xC350, 0x7530); + let _ = NamePayload::new(FileOperationMode::AddFile, "/a"); + let _ = SentDataPayload::new(&[0x02, 0x00]); + let _ = FileSizePayload::new(0xC350, 0x7530); + let _ = DirSizePayload::new(0x20); + let _ = PositionPayload::new(0x10); + let _ = DtcFaultDetectionCounterRecord::new(DtcRecord::new(0, 0, 0), 0); +} diff --git a/tests/spec_conformance.rs b/tests/spec_conformance.rs new file mode 100644 index 00000000..5849ef87 --- /dev/null +++ b/tests/spec_conformance.rs @@ -0,0 +1,236 @@ +//! Round-trips the message-flow example byte sequences printed in ISO 14229-1:2020. +//! +//! Every frame below is quoted from a numbered example table in the standard, so this suite +//! answers a question the rest of the tests cannot: does the crate agree with the document, +//! rather than merely with itself? A layout error that both `encode` and `decode` share is +//! invisible to a round-trip test written against the crate's own output, and that is exactly +//! how several real defects survived — a missing mandatory `DTCFormatIdentifier`, a +//! `MemorySelection` byte read from the wrong sub-functions, a `powerDownTime` written +//! unconditionally. +//! +//! This is an integration test, so it sees the crate as a downstream user does: anything it +//! needs must be reachable and constructible through the public API. + +use uds_protocol::{Decode, Encode, Request, Response}; + +/// A frame quoted from the standard, with the table it came from. +struct Example { + /// Clause and table the bytes are quoted from, used in failure messages. + cite: &'static str, + /// The complete application-layer frame, service identifier first. + bytes: &'static [u8], +} + +const REQUESTS: &[Example] = &[ + Example { + cite: "Table 61 - CommunicationControl request (enableRxAndDisableTxWithEnhancedAddressInformation)", + bytes: &[0x28, 0x04, 0x01, 0x00, 0x0A], + }, + Example { + cite: "Table 63 - CommunicationControl request (enableRxAndTxWithEnhancedAddressInformation)", + bytes: &[0x28, 0x05, 0x01, 0x00, 0x0A], + }, + Example { + cite: "Table 124 - TesterPresent request example #1", + bytes: &[0x3E, 0x00], + }, + Example { + cite: "Table 126 - TesterPresent request example #2 (suppressPosRspMsgIndicationBit = TRUE)", + bytes: &[0x3E, 0x80], + }, + Example { + cite: "Table 133 - ControlDTCSetting request example #1 (DTCSettingType = off)", + bytes: &[0x85, 0x02], + }, + Example { + cite: "Table 135 - ControlDTCSetting request example #2 (DTCSettingType = on)", + bytes: &[0x85, 0x01], + }, + Example { + cite: "Table 300 - ClearDiagnosticInformation request example #1 (groupOfDTC = FFFF33, no MemorySelection)", + bytes: &[0x14, 0xFF, 0xFF, 0x33], + }, + Example { + cite: "Table 354 - ReadDTCInformation request example #5 (reportDTCSnapshotRecordByDTCNumber)", + bytes: &[0x19, 0x04, 0x12, 0x34, 0x56, 0x02], + }, + Example { + cite: "Table 361 - ReadDTCInformation request example #7 (reportDTCExtDataRecordByDTCNumber)", + bytes: &[0x19, 0x06, 0x12, 0x34, 0x56, 0xFF], + }, + Example { + cite: "Table 404 - ReadDataByIdentifier request example #1 step #1", + bytes: &[0x22, 0x9B, 0x00], + }, + Example { + cite: "Table 282 - WriteDataByIdentifier request example #1 (VIN)", + bytes: &[ + 0x2E, 0xF1, 0x90, 0x57, 0x30, 0x4C, 0x30, 0x30, 0x30, 0x30, 0x34, 0x33, 0x4D, 0x42, + 0x35, 0x34, 0x31, 0x33, 0x32, 0x36, + ], + }, + Example { + cite: "Table 431 - RoutineControl request example #1 (startRoutine)", + bytes: &[0x31, 0x01, 0x02, 0x01], + }, + Example { + cite: "Table 433 - RoutineControl request example #2 (stopRoutine)", + bytes: &[0x31, 0x02, 0x02, 0x01], + }, + Example { + cite: "Table 462 - RequestDownload request (addressAndLengthFormatIdentifier = 0x33)", + bytes: &[0x34, 0x11, 0x33, 0x60, 0x20, 0x00, 0x00, 0xFF, 0xFF], + }, + Example { + cite: "Table 466 - TransferData request (blockSequenceCounter = 5, no data)", + bytes: &[0x36, 0x05], + }, + Example { + cite: "Table 468 - RequestTransferExit request", + bytes: &[0x37], + }, + // Table 486: modeOfOperation = AddFile, filePathAndNameLength = 0x001E, + // filePathAndName = "D:\mapdata\europe\germany1.yxz", dataFormatIdentifier = 0x11, + // fileSizeParameterLength = 2, fileSizeUnCompressed = 0xC350, fileSizeCompressed = 0x7530. + Example { + cite: "Table 486 - RequestFileTransfer request example (AddFile)", + bytes: &[ + 0x38, 0x01, 0x00, 0x1E, 0x44, 0x3A, 0x5C, 0x6D, 0x61, 0x70, 0x64, 0x61, 0x74, 0x61, + 0x5C, 0x65, 0x75, 0x72, 0x6F, 0x70, 0x65, 0x5C, 0x67, 0x65, 0x72, 0x6D, 0x61, 0x6E, + 0x79, 0x31, 0x2E, 0x79, 0x78, 0x7A, 0x11, 0x02, 0xC3, 0x50, 0x75, 0x30, + ], + }, +]; + +const RESPONSES: &[Example] = &[ + Example { + cite: "Table 32 - DiagnosticSessionControl positive response (P2 = 0x0032, P2* = 0x01F4)", + bytes: &[0x50, 0x02, 0x00, 0x32, 0x01, 0xF4], + }, + Example { + cite: "Table 39 - ECUReset positive response example #1 (hardReset, no powerDownTime)", + bytes: &[0x51, 0x01], + }, + Example { + cite: "Table 50 - SecurityAccess positive response example #1 step #2 (sendKey)", + bytes: &[0x67, 0x02], + }, + Example { + cite: "Table 52 - SecurityAccess positive response example #2 step #2 (requestSeed)", + bytes: &[0x67, 0x01, 0x00, 0x00], + }, + Example { + cite: "Table 60 - CommunicationControl positive response", + bytes: &[0x68, 0x01], + }, + Example { + cite: "Table 62 - CommunicationControl positive response", + bytes: &[0x68, 0x04], + }, + Example { + cite: "Table 125 - TesterPresent positive response example #1", + bytes: &[0x7E, 0x00], + }, + Example { + cite: "Table 136 - ControlDTCSetting positive response example #2", + bytes: &[0xC5, 0x01], + }, + Example { + cite: "Table 341 - ReadDTCInformation positive response example #1 (reportNumberOfDTCByStatusMask)", + bytes: &[0x59, 0x01, 0x2F, 0x01, 0x00, 0x01], + }, + Example { + cite: "Table 405 - ReadDataByIdentifier positive response example #1 step #1", + bytes: &[0x62, 0x9B, 0x00, 0x0A], + }, + Example { + cite: "Table 283 - WriteDataByIdentifier positive response example #1", + bytes: &[0x6E, 0xF1, 0x90], + }, + Example { + cite: "Table 432 - RoutineControl positive response example #1", + bytes: &[0x71, 0x01, 0x02, 0x01, 0x32], + }, + Example { + cite: "Table 434 - RoutineControl positive response example #2", + bytes: &[0x71, 0x02, 0x02, 0x01, 0x30], + }, + Example { + cite: "Table 463 - RequestDownload positive response (maxNumberOfBlockLength = 0x0081)", + bytes: &[0x74, 0x20, 0x00, 0x81], + }, + Example { + cite: "Table 465 - TransferData positive response", + bytes: &[0x76, 0x01], + }, + Example { + cite: "Table 467 - TransferData positive response", + bytes: &[0x76, 0x05], + }, + Example { + cite: "Table 469 - RequestTransferExit positive response", + bytes: &[0x77], + }, + Example { + cite: "Table 487 - RequestFileTransfer positive response example (AddFile)", + bytes: &[0x78, 0x01, 0x02, 0xC3, 0x50, 0x11], + }, +]; + +/// Largest example frame, plus room to catch an encoder that writes too much. +const BUF: usize = 64; + +#[test] +fn every_spec_request_example_decodes_and_re_encodes_unchanged() { + for Example { cite, bytes } in REQUESTS { + let req = Request::decode_exact(bytes) + .unwrap_or_else(|e| panic!("{cite}: decode of {bytes:02X?} failed: {e:?}")); + let mut buf = [0u8; BUF]; + let written = req + .encode_to_slice(&mut buf) + .unwrap_or_else(|e| panic!("{cite}: encode failed: {e:?}")); + assert_eq!(&buf[..written], *bytes, "{cite}: re-encoded bytes differ"); + } +} + +#[test] +fn every_spec_response_example_decodes_and_re_encodes_unchanged() { + for Example { cite, bytes } in RESPONSES { + let resp = Response::decode_exact(bytes) + .unwrap_or_else(|e| panic!("{cite}: decode of {bytes:02X?} failed: {e:?}")); + let mut buf = [0u8; BUF]; + let written = resp + .encode_to_slice(&mut buf) + .unwrap_or_else(|e| panic!("{cite}: encode failed: {e:?}")); + assert_eq!(&buf[..written], *bytes, "{cite}: re-encoded bytes differ"); + } +} + +#[test] +fn the_spec_examples_cover_every_service_the_crate_models() { + // Guards against a service quietly dropping out of this suite. Update the expected set + // deliberately when a service gains or loses spec-example coverage. + let mut sids: Vec = REQUESTS.iter().map(|e| e.bytes[0]).collect(); + sids.sort_unstable(); + sids.dedup(); + assert_eq!( + sids, + vec![ + 0x14, 0x19, 0x22, 0x28, 0x2E, 0x31, 0x34, 0x36, 0x37, 0x38, 0x3E, 0x85 + ], + "request-side spec-example coverage changed" + ); + + // 0x10, 0x11 and 0x27 appear on the response side only: the standard's request examples + // for those services are folded into prose rather than given as byte tables. + let mut response_sids: Vec = RESPONSES.iter().map(|e| e.bytes[0]).collect(); + response_sids.sort_unstable(); + response_sids.dedup(); + assert_eq!( + response_sids, + vec![ + 0x50, 0x51, 0x59, 0x62, 0x67, 0x68, 0x6E, 0x71, 0x74, 0x76, 0x77, 0x78, 0x7E, 0xC5 + ], + "response-side spec-example coverage changed" + ); +}