fix!: align the wire format with ISO 14229-1:2020 - #53
Open
zheylmun wants to merge 23 commits into
Open
Conversation
Contributor
|
Tick the box to add this pull request to the merge queue (same as
|
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## main #53 +/- ##
==========================================
+ Coverage 81.57% 89.19% +7.61%
==========================================
Files 29 29
Lines 4076 5014 +938
==========================================
+ Hits 3325 4472 +1147
+ Misses 751 542 -209 |
zheylmun
force-pushed
the
fix/wire-conformance
branch
from
July 30, 2026 19:53
74c2592 to
9d4cc95
Compare
Three feature-graph defects, all invisible to the existing CI matrix, which only ever built --all-features, --no-default-features, and --no-default-features --features alloc. 1. `utoipa` and `clap` did not compile without `std` at all. `cargo build --no-default-features --features utoipa` failed with 318 resolution errors (`std::`, `String`, `Vec` emitted by the derive macros inside a `#![no_std]` crate); `clap` failed the same way. Both features now imply `std`. Verified pre-existing on `main`, so this is not a regression from the API consistency pass. 2. The `serde` feature could not build for a bare-metal target. The dep was declared with serde's default features on, which pulls `serde/std`. Every host build passed — the host has `std` for serde to compile against no matter what this crate declares — while `--features serde --target thumbv6m-none-eabi` failed. serde is now `default-features = false` and picks up its alloc/std layers via weak `serde?/alloc` and `serde?/std` features, gated on this crate's own. 3. `FunctionalGroupIdentifier::VODBSystem` transposed two letters. ISO 14229-1 Table D.1 names 0xFE `VOBDSystem`. Renamed in 92e1f96 as part of the casing sweep; promoted to its own CHANGELOG entry here because it is a semantic correction, not a casing one. On the coverage gap that hid all three: this commit originally added a `features` CI job running `cargo hack check --feature-powerset --no-dev-deps`, plus bare-metal builds of `serde` and `alloc,serde` for thumbv6m-none-eabi. Both were dropped when this branch rebased onto the reusable org CI workflow (#48), which replaces this repo's main.yml wholesale and provides neither yet. They are tracked as gaps to add upstream in luminartech/rust_workflow, where every protocol crate benefits. Until then both are verified locally rather than in CI: all 16 feature combinations pass `cargo hack check --feature-powerset`, and all four bare-metal combinations build for thumbv6m-none-eabi. Defect 2 is observable nowhere else, which is worth knowing when reviewing this.
All of `RequestDownloadRequest`'s fields are private, correctly — the width nibbles in `address_and_length_format_identifier` are derived from the address and size, so letting callers set those independently would desynchronise them. But only `memory_address` and `memory_size` had getters, which left `data_format_identifier` write-only: a server could decode a download request and had no way to find out which compression or encryption method it had been asked to use, the one field it must act on. `DataFormatIdentifier` had no accessors for its nibbles either, so a getter alone would not have been enough. Adds `compression_method()` and `encryption_method()` there too.
`Error::negative_response_code()` closes the gap between the two halves of a server loop: decode an inbound request, and on failure answer with a negative response. The mapping was implied but not provided — three variants documented "Corresponds to NRC 0x13" in prose and the other eighteen said nothing, so every caller had to re-derive it against a `#[non_exhaustive]` enum they cannot match exhaustively. Classification follows ISO 14229-1: - 0x13 incorrectMessageLengthOrInvalidFormat - the frame is malformed (short read, trailing bytes, unrepresentable declared width). - 0x12 subFunctionNotSupported - the sub-function byte is not a defined value for the service (0x10, 0x11, 0x19, 0x27, 0x28, 0x31, 0x3E, 0x85). - 0x31 requestOutOfRange - a parameter, not a sub-function, is out of range. `communicationType` belongs here rather than on 0x12: it is a parameter of CommunicationControl, not its sub-function. - 0x10 generalReject - only for `IoError`. A transport failure is not a protocol error, and ISO designates generalReject for exactly the case where no other code meets the implementation's needs. Tested per variant against the expected byte, plus a guard that no variant can map to PositiveResponse or to a reserved code, either of which would put an illegal NRC on the wire. The match is exhaustive inside the crate, so a future variant fails to compile until mapped.
`[0x3E, 0x01]` decoded and re-encoded as `[0x3E, 0x00]`. Decode parsed the reserved sub-function into `ZeroSubFunction::IsoSaeReserved(1)` and then threw it away, keeping only the SPRMIB flag, so the request silently rewrote the tester's frame. The normalization was deliberate — decode said as much — but it left the service contradicting itself: `TesterPresentResponse` retains the very same reserved values in a private field and re-encodes them intact. Every other service preserves reserved values too (`ResetType::IsoSaeReserved`, `DiagnosticSessionType::IsoSaeReserved`, ...), and the crate tests lossless re-encode as an invariant for `Request::Other`. Resolved in the preserving direction, which is also what a server wants: it can now answer subFunctionNotSupported naming the byte it actually received instead of 0x00. - `TesterPresentRequest` retains the sub-function in a private field, so callers still cannot mint a reserved value, and `new(suppress)` keeps its signature and its 0x00 encoding. - Added `sub_function()` to both the request and the response to read the byte back with SPRMIB stripped. - `ZeroSubFunction::value()` is now a `const fn` that `From<_> for u8` delegates to, so the accessors can be `const` like the rest of the crate without duplicating the match. - `TesterPresentResponse::new()` is now `const`, which it should have been already — it was the only `new()` in the crate that was not.
All 15 request types expose `allowed_nack_codes()` as an associated function, which is a complete and useful convention — but there was no way to reach it from a decoded `Request` without re-matching every variant, on a `#[non_exhaustive]` enum that downstream code cannot match exhaustively. `Request` already dispatches `service()` and `is_positive_response_suppressed()`; this belongs beside them. `Request::Other` returns an empty slice, documented as "NRC set unknown" rather than "no codes apply", since the crate has no table for services it does not model. The test decodes a real frame per service and asserts a non-empty result, so it fails if a variant is ever wired to the wrong type's table or a new service is added without one.
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 0x34 and 0x35 the same message layout — request: dataFormatIdentifier, addressAndLengthFormatIdentifier, memoryAddress, memorySize; positive response: lengthFormatIdentifier, maxNumberOfBlockLength — differing only in the SID and in which direction the subsequent TransferData sequence moves bytes. Rather than duplicate ~160 lines of codec and leave two places to fix any wire bug, both pairs are now generated from one macro in `services/upload_download.rs`, following the `transfer_exit_descriptor!` precedent already in the crate. `request_download.rs` is subsumed by that file. The two NRC tables are kept as separate constants, identical today per ISO, so either service can diverge later without silently changing the other. The macro also generates the test module, so both services get identical coverage and neither can drift. Plus a `shared_layout_tests` module pinning the payload equivalence, and frame-level tests asserting the two do not collapse into one variant despite identical payloads. Also corrects the README service table, which is the crate-level doc: it was missing rows for DynamicallyDefinedDataIdentifier (0x2C) and AccessTimingParameter (0x83) — both enumerated in `UdsServiceType` — and named two services differently from the code (`ECUReset`, `ControlDTCSetting`). Verified the table's 27 rows now correspond exactly to the 27 request SIDs in `UdsServiceType`.
All three DTC iterators returned `Some(Err(..))` for a trailing partial
record *without* advancing `remaining`, so they yielded that error
forever. Reachable from untrusted wire input, because
`ReadDtcInfoResponse::decode` passes the record tail through verbatim
without checking it divides evenly:
let (resp, _) = Response::decode(&[0x59, 0x02, 0xFF, 0x01, 0x02])?;
for r in resp.dtc_and_status_iter().unwrap() { ... } // never returns
`for` loops hung, `count()` hung, and `collect::<Vec<Result<_, _>>>()`
allocated without bound. `collect_all()` was the one safe path, and only
by accident: `collect::<Result<Vec, _>>()` 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, so the error is reported
exactly once and iteration ends. Tests are bounded with `take` so a
regression fails loudly instead of hanging the suite — the pre-fix run
reported "1 ok, 7 err" against `take(8)`.
Also adds the iterator traits that were missing, and documents the one
that is deliberately absent:
- `size_hint` returning an exact (n, Some(n)), so `collect` can
pre-allocate.
- `FusedIterator`, which now holds: once the buffer is consumed, `next()`
keeps returning `None`.
- Not `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()` that two existing tests deliberately
pin. Documented on each iterator so this is not re-litigated.
The old name pointed at the wrong response variant. It reads as "the
severity iterator", but it only handles the 5-byte records of
`WwhObdDtcByMaskRecord` (0x42) — the 0x08/0x09 `DtcSeverityList` records
are 6 bytes and carry an extra DTC functional-unit byte. The variant's own
doc had to carry a warning that the iterator does **not** apply to it,
which is a sign the name was doing damage.
DtcSeverityAndStatusIter -> WwhObdDtcSeverityIter
ReadDtcInfoResponse::severity_and_status_iter -> wwh_obd_dtc_severity_iter
The iterator doc now states its scope up front, and the `DtcSeverityList`
doc no longer needs a warning — just a note on the record shape and that
no iterator is wired for it yet.
The name pins the severity content as well as the WWH-OBD scoping. Sub-functions
0x55 (reportWWHOBDDTCWithPermanentStatus) and 0x56
(reportDTCByReadinessGroupIdentifier) are also WWH-OBD and also
functional-group-addressed, but they return 4-byte DTCAndStatusRecords -- so
WwhObdDtcIter would claim the whole WWH-OBD family while handling only the 5-byte
0x42 record, which is the same trap one axis over.
Also fixes the accessor doc, which asserted the opposite of what the code does:
it said None is returned "if this is not a severity variant", but DtcSeverityList
(0x08/0x09) is a severity variant and does return None, because its records carry
an extra DTCFunctionalUnit byte.
The crate's rule is: encapsulate a request/response field iff it carries an invariant, otherwise expose it as a public data bag. Three types broke it in ways a caller could not predict, and applying the rule uncovered a functional gap. `CommunicationControlRequest` exposed SPRMIB as a method while the other six suppressable requests expose it as a field. The encapsulation was justified — but by the `control_type`/`node_id` invariant, not by SPRMIB, which is independent and only fused onto the sub-function byte at the wire boundary. SPRMIB is now a public field, `control_type`/`node_id` stay private, continuing the direction of d8a6dd1/42b5277 and matching the shape TesterPresentRequest now has. `Request::is_positive_response_suppressed` reads identically for all seven variants as a result. `ReadDataByIdentifierResponse::records` was private with a getter, while every other opaque response slice is a public field (`RequestDownloadResponse::max_number_of_block_length`, `TransferDataResponse::data`, ...). It holds no invariant; now a public field and the redundant getter is gone. Two asymmetries are *kept* because the rule justifies them, and are now documented so they are not mistaken for oversights: - `CommunicationControlResponse::control_type` is public while the request's is a getter: the response carries no node_id, so there is no cross-field invariant. - `NegativeResponse` keeps private fields: the SID is a raw byte whose typed meaning is derived, and the constructors offer different guarantees. Which exposed the gap. `NegativeResponse::new` routes through `to_request_sid()`, collapsing every unmodeled service to 0x7F — so a server that decoded `Request::Other { sid: 0x40 }` could not answer serviceNotSupported echoing 0x40, despite the type's own docs advertising lossless handling of unmodeled SIDs on the decode side. Adds `NegativeResponse::new_with_sid`, the construction-side counterpart to `Request::Other { sid }`.
…inders Three remaining polish items. `DataFormatIdentifier::new` took `(encryption, compression)` — the reverse of the wire layout (compression is the high nibble) and the reverse of the type's own doc comment. Both parameters are `u8`, so a transposition compiled silently and produced a byte with the nibbles swapped. Now `(compression, encryption)`, matching the wire. `From<u8>` is unaffected and is the path every decode site already uses, so the blast radius is small — but any caller passing two different non-zero values needs review, which the CHANGELOG says explicitly. Also adds `DataFormatIdentifier::NONE` for the no-compression/no-encryption case, which removes most reasons to call `new` at all. `Request::decode`/`Response::decode` now document that the remainder is always empty. A UDS frame is not self-delimiting — its length comes from the transport — so one buffer is one frame and every payload goes through `decode_exact`. The streaming shape of the `Decode` contract otherwise invites feeding concatenated frames, which would be silently swallowed as one, including via `DecodeIter`. This commit also used to gate publication on the local `no-std` and `features` jobs, since a tag could otherwise publish a crate whose no_std build or feature graph was broken -- which is what happened before the powerset job existed. That part is dropped in the rebase onto the reusable org CI workflow (#48): release-plz owns publishing there, and gating is upstream's to configure. The concern still stands, and is tracked with the two missing checks noted on that PR.
…tests Two gaps found reviewing my own diff. The `[Unreleased]` CHANGELOG section had grown to 14 subsections with three "Changed", three "Fixed", two "Added" and two "Removed" headings, which is hard to read for anyone working through the 0.2.0 migration. Consolidated to the canonical Added/Changed/Fixed/Removed plus CI, folding in the pre-existing wire-codec entries that were already there. Verified all 41 bullets survive and every section's distinctive phrases are still present. The `size_hint` test only covered `DtcAndStatusIter`, but the three iterators use two different record widths (4 bytes vs 5), so each needs its own `div_ceil` exercised. Now covers all three across every buffer length from 0..=16, asserting size_hint equals the actual item count and that the count matches `len.div_ceil(width)`. Added an explicit test that all three stay exhausted after draining, which is the precondition `FusedIterator` asserts.
`ReadDtcInfoResponse::decode` passed the record tail through verbatim
without checking it divides evenly, so a malformed frame decoded
successfully and only failed later during iteration. That was the
inconsistency behind the iterator hang fixed in 91147dc: every other
length mismatch in this crate is rejected at the frame boundary
(`TrailingBytes`, short reads), but a DTC list with a stray byte was
accepted.
Now rejected with `Error::IncorrectMessageLengthOrInvalidFormat`, which
maps to NRC 0x13 — the same code the iterators used for the same
condition. All four record-carrying variants are checked at their own
width, which the merged encode arm obscures:
DtcList 4 bytes (3-byte DTC + status)
DtcFaultDetectionCounterList 4 bytes (3-byte DTC + counter)
DtcSeverityList 6 bytes (extra functional-unit byte)
WwhObdDtcByMaskRecord 5 bytes
Empty record lists stay valid: a server with no matching DTCs answers
with the header and no records, so 0 is a legal length for every width.
Encode remains permissive, deliberately. The enum's `#[non_exhaustive]`
blocks exhaustive matching, not variant construction, so a caller can
still build a misaligned `DtcList` and encode it — the same latitude
`Request::Other { sid }` has. The iterators therefore keep their `Result`
item type and their one-error-then-terminate behaviour, and the docs now
say precisely that the no-partial-tail guarantee covers *decoded*
responses.
Response decode previously had almost no test coverage — one aligned frame
in lib.rs. Adds a table-driven module over all four variants: every
misalignment from 1..width rejected, 0..=3 records accepted with the
record count verified through the iterators, empty lists accepted, the
error surfacing at the frame layer, and iterators from decoded responses never yielding an error.
ISO 14229-1:2020 Table 296 marks the `MemorySelection` byte `U` (user option, per the
Table 13 convention legend: "may or may not be present, depending on dynamic usage by
the user"), and the standard's own message-flow example for the service (Table 300) is
the 3-byte form with no such byte. The crate required it, so the ordinary request — the
only form the 2013 edition defines at all — did not decode:
[0x14, 0xFF, 0xFF, 0xFF] -> Err(InsufficientData(Incomplete { needed: 4, available: 0 }))
and every encode emitted a spurious 4th byte, which a conformant server reads as a
memory selection the client never asked for.
`memory_selection` becomes `Option<u8>`. The constructors split along the same seam
rather than making every caller thread a `None` through: `new`/`clear_all` for the
ordinary case, `new_with_memory_selection`/`clear_all_in_memory` when addressing
user-defined DTC memory — following the `new_with_node_id` and `new_with_sid`
precedents already in the crate.
The 3 `groupOfDTC` bytes stay mandatory, so a truncated record is still rejected — by
`DtcRecord::decode`, which reports the shortfall correctly. That also retires a bogus
error payload: the old code reported `needed: 4` against an `available` measured on the
post-`DtcRecord` remainder, the crate's only site where those two were counted against
different slices.
Also takes `&self` in `SecurityAccessLevel::value`, matching the other twenty accessors
in the crate. No call site changes, since the type is `Copy`.
Three loose ends from an audit of the public surface, none of them breaking. `const fn` gaps. The crate is otherwise uniformly `const fn new` (32 of 40 constructors), and `lib.rs` advertises const construction — but the gaps fell exactly on the primitives a caller wants in a `const` table: `DtcRecord`, the three record numbers, `DataFormatIdentifier`, and all four `UdsServiceType` SID conversions that a server dispatch table is built from. `NegativeResponse::new` was non-const only because `to_request_sid` was, right beside a `new_with_sid` that already was. Two needed more than the keyword: `DataFormatIdentifier::new` used `?` on a helper, so the range checks are now written out (`?` is not permitted in a `const fn`), and the `upload_download` constructors used `Ord::max`, which is not const-callable, so the clamp-to-one-byte is an `if`. `CommunicationControlRequest::new` is deliberately left alone: it needs `u8::from(control_type)`, and trait methods cannot be called in a `const fn` on stable. `DtcRecord` accessors. The fields are private and there were no accessors at all, so a decoded DTC could only be inspected by round-tripping through `u32` — awkward when Annex D.1 gives the high byte its own meaning. Adds `high_byte`, `middle_byte` and `low_byte` rather than making the fields public, matching the sibling wire primitives (`FunctionalGroupIdentifier`, `SecurityAccessLevel`, `DtcStoredDataRecordNumber`), which are all sealed with accessors. Rustdoc. `DataFormatIdentifier` named only `RequestDownloadRequest` though `RequestUploadRequest` and four `RequestFileTransferRequest` variants also carry it, and pointed at a `data_format_identifier` *field* that is now private behind an accessor; `MemoryFormatIdentifier` and `LengthFormatIdentifier` had the same staleness. `DtcStoredDataRecordNumber` described itself as a `DTCSnapshot` record and its `new()` had an empty summary and a malformed `Error::ReservedForLegislativeUse` link. `DtcSettingType` was the only type whose doc comment sat after its derives. With two redundant intra-doc link targets removed, `cargo doc --document-private-items` is clean.
`MemoryFormatIdentifier::try_from` used exclusive range patterns where the adjacent comments said inclusive, so it accepted a memorySize width of 1-3 and a memoryAddress width of 1-4. Annex H Table H.1 runs to 4 and 5 respectively, and `RequestDownloadRequest::new` / `RequestUploadRequest::new` already documented and derived those wider values -- so the crate emitted frames it could not decode. Any transfer above 16 MB, or to an address above 4 GB, was unrepresentable, and ALFID 0x44 (the usual value in a real programming session) was rejected outright. Name the two bounds as constants and use them in both the range checks and the property test. `prop_memory_format_identifier_roundtrip` had its generators narrowed to `1..=3` / `1..=4` -- exactly the buggy accepted set -- which is why a property test sat on top of this without finding it. Add byte-exact assertions on the ALFID that `new` derives. The existing frame tests all ran decode -> encode, so the derivation was never checked: swapping the two nibbles passed the whole suite while silently truncating a 4-byte memory address to one byte. `check_message_size` no longer needs `alloc`.
ISO 14229-1:2020 Table 35 marks `powerDownTime` `Cvt` = `C`, present only when the sub-function is `enableRapidPowerShutDown` (0x04), and Table 39's flow example for `hardReset` is the two-byte frame `51 01`. The field was a bare `u8` that `encode` always wrote, so every positive response except 0x04's gained a spurious trailing 0x00 -- decoding `51 01` and re-encoding it produced `51 01 00`. A gateway that decoded and re-encoded ECU traffic silently rewrote those frames, and a strict server answers the result with NRC 0x13. Model it as `Option<u8>` with the constructor split along the same seam as `ClearDiagnosticInfoRequest`'s `memorySelection`: `new` for the ordinary case, `new_with_power_down_time` when the byte is required. Decode takes presence from the wire rather than inferring it from `resetType`, so a response from a server that sends the byte anyway still round-trips unchanged. `None` is now distinct from `Some(0)`. The old decode substituted 0 for an absent byte, which conflated "not present" with a server reporting 0 seconds -- and the field doc named 0x00 as the not-available sentinel when Table 36 defines that as 0xFF. Also assert the encoded bytes in `ecu_reset_response`, which encoded into a buffer it never read: swapping the two written bytes passed before.
Two mandatory pieces of the 0x19 request were missing. SPRMIB: `decode` matched the raw sub-function byte, so bit 7 was treated as part of the sub-function value. ISO 14229-1:2020 Table 13 requires a server to support both bit values "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)". The consequences were both directions of wrong: `19 82 FF` (a suppressed reportDTCByStatusMask) was rejected as having trailing bytes, because 0x82 fell through to `IsoSaeReserved`, which consumes no payload; and `19 8A` decoded successfully as a *reserved* sub-function, so a server answered SubFunctionNotSupported to a request it was required to execute. `Request::is_positive_response_suppressed` also reported false for all 0x19 traffic. 0x19 was the only one of the eight sub-function services not routing the byte through the shared helper. MemorySelection: Table 310 marks it `M` for reportUserDefMemoryDTCByStatusMask (0x17), but the variant carried only the status mask -- so the conformant 4-byte request was rejected and the malformed 3-byte one accepted, exactly inverted. The sibling sub-functions 0x18 and 0x19 already read theirs. `ReadDtcInfoSubFunction` does not implement `TryFrom<u8>` (the byte alone does not determine the variant), so the generic `SuppressablePositiveResponse<T>` does not fit. Add `split_sprmib`/`fuse_sprmib` beside it instead, keeping the bit masking in that module as its own docs intend, and split the sub-function's parameter encoding out so the request can write the fused byte itself. Also correct the `IsoSaeReserved` doc, which listed 0x42 -- a modeled report type -- as reserved, and omitted most of the ranges Table 317 actually reserves.
ISO 14229-1:2020 Table 319 gives the reportNumberOfDTCByStatusMask and reportNumberOfDTCBySeverityMaskRecord positive response four mandatory bytes after the sub-function echo: DTCStatusAvailabilityMask, DTCFormatIdentifier, then a two-byte DTCCount. Clause 12.3.1.2 spells the order out in prose as well. `NumberOfDtcs` had no format-identifier field, so decode read that byte as the count's high byte. Both directions were wrong. Table 341's own flow example, `59 01 2F 01 00 01`, was rejected with TrailingBytes, so no conformant DTC count could be read at all; through the non-exact decode it came back as 0x0100 rather than 1. A server built on this crate emitted a five-byte frame, one short of the mandatory layout. The format identifier is also the only thing that says how to interpret a DTC's three bytes -- ISO 14229-1 defines no decoding method for them itself -- so dropping it lost the information a client needs to make sense of the DTCs that follow. Side effect worth noting: this response's `Incomplete` shortfall is now self-consistent, since four mandatory payload bytes are measured against a payload-relative `available`.
Adds an integration suite that decodes and re-encodes the byte sequences the
standard itself prints in its numbered example tables -- 34 frames across the
twelve request services and fourteen response service identifiers the crate
models, each carrying its table citation in the failure message.
This answers a question the existing tests cannot. A round-trip written against
the crate's own output passes whenever `encode` and `decode` share a
misreading, which is precisely how the defects fixed in the preceding commits
survived: the ECUReset response re-encoded `51 01` as `51 01 00`, and the DTC
count response rejected Table 341's own six bytes outright. Both are in this
suite now.
Two frames were mis-extracted on the first pass and are worth flagging for
anyone adding more: Table 486 and Table 487 each end in a cell holding two
bytes at once ("C3 16 50 16"), so a naive one-byte-per-row read silently
truncates the frame. Every sequence here was checked against the table text by
hand.
Being an integration test, this also exercises the crate as an external user
sees it, which is a property the inline unit tests structurally cannot check.
`82f2949` added `#[non_exhaustive]` to seven public structs with `pub` fields. Six of them have a `pub const fn new`; this one has no `impl` block at all, so outside the crate a struct literal is E0639 and there is no constructor to fall back on. The only way a downstream user could obtain the `Item` type of a public iterator was to decode bytes through `DtcFaultDetectionIter` -- and with `serde` enabled, absurdly, to deserialize one from JSON. `0cf58fe` had added the crate-root re-export specifically so callers could name this type, and pinned it with `fault_detection_counter_record_is_nameable_from_ crate_root`. That test is a unit test, and inside the defining crate `#[non_exhaustive]` does not apply -- so it could never have caught this. Add `tests/public_api.rs` for checks that only mean something from outside the crate, and pin all seven types there so the next `#[non_exhaustive]` cannot reintroduce the gap.
RequestFileTransfer NRC set (Table 484): three of the seven mandated codes were missing -- requestSequenceError (0x24), securityAccessDenied (0x33) and authenticationRequired (0x34). 0x24 in particular exists specifically for ResumeFile against an already-complete transfer, so a server consulting this table would have judged its own conformant NRC illegitimate. The test asserted `contains` on a single code, which by construction cannot notice an absent one; it now pins the whole set in order. RoutineControl sub-function (Tables 426, 430): a reserved routineControlType produced `IncorrectMessageLengthOrInvalidFormat`, so a server answered `7F 31 13` where Table 430 mandates `7F 31 12` -- for a request whose length was perfectly correct. It also disagreed with ControlDTCSetting, the only other service that validates its sub-function. Return `Error::InvalidRoutineControlSubFunction` instead, which the error map already classifies as SubFunctionNotSupported and which nothing in the crate had ever constructed. Table 426 gives 0x31 no vehicle-manufacturer or system-supplier range, so rejecting rather than modelling reserved values is right here. WriteDataByIdentifier length (Table 277, Figure 26): `dataRecord` byte #1 is `M` and the stated minimum message length is four bytes, but a three-byte message decoded into an empty data record -- so a server would attempt a zero-length write instead of answering NRC 0x13. Require three payload bytes, and make `new` reject an empty record so the constructor cannot mint a frame the decoder refuses. The old test asserted the empty case was allowed, locking the defect in.
Two parameters from ISO 14229-1:2020 clause 10.8.2 were missing. DTCSettingControlOptionRecord (Table 127, `Cvt` = `U`): not modelled at all, so `85 02 AA BB CC` was rejected as having trailing bytes. Table 129 describes the record as vehicle-manufacturer specific data qualifying the request -- e.g. the list of DTCs to turn on or off -- and Table 132 reserves NRC 0x31 for a server that "detects an error in the DTCSettingControlOptionRecord", which it can only do if the record reaches it. This is the same `Cvt = U` omission that `ClearDiagnosticInformation`'s memorySelection was; the sweep that found that one should have caught this. Modelled as `&'d [u8]`, empty when absent, as RoutineControl already does for its own optional record -- which makes the request type borrow, hence the new lifetime parameter. DTCSettingType ranges (Table 128): 0x40-0x5F is reserved for vehicle manufacturers and 0x60-0x7E for system suppliers, but `try_from` rejected both, so a client could not send a manufacturer-defined setting and a server never saw the byte. Every sibling sub-function enum (0x10, 0x11, 0x27, 0x28) models these ranges; 0x85 was the exception. Reserved values (0x00, 0x03-0x3F, 0x7F) are still rejected with NRC 0x12, which keeps this service aligned with RoutineControl -- the two services in the crate that validate their sub-function rather than passing reserved bytes through.
…nType
ISO 14229-1:2020 Annex B Table B.1 splits this byte three ways: bits 1-0 are the
message type, bits 3-2 are ISOSAEReserved, and bits 7-4 carry the subnet number
-- 0x0 for the receiving node and all connected networks, 0x1-0xE for a specific
subnet, 0xF for the network the request arrived on. `CommunicationType::try_from`
matched the whole byte against 0x00..=0x03, so anything with a subnet set was
rejected. 0xF3 ("network management and normal messages on the network this
request came in on") is a common real-world value and was unusable, while the
README advertised 0x28 as fully supported. The file carried a TODO admitting the
gap.
Add `SubnetNumber` for the high nibble, keep `CommunicationType` as the low
nibble, and reject a byte whose reserved bits 3-2 are set. The subnet is exposed
as `with_subnet` rather than a fourth constructor: it is independent of the
`control_type`/`node_id` pairing, so folding it in would have doubled the
constructors without adding an invariant to enforce.
The two whole-byte `CommunicationType` tests encoded the old semantics -- one
asserted every byte above 0x03 was invalid -- so both are rewritten against the
nibble split, and the full-byte round trip now reassembles both halves.
zheylmun
force-pushed
the
fix/wire-conformance
branch
from
July 31, 2026 14:00
9d4cc95 to
f8bde8c
Compare
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Second of the follow-on branches splitting
api/consistency-pass-1. Stacked on #51 — reviewthat first; this PR's diff is only its own 23 commits.
This is the branch that most needs real scrutiny. It changes bytes on the wire. Every fix carries an
ISO 14229-1:2020 citation, and the citation is the thing to check — a fix that looks sensible but
cites a table saying something else is the failure mode that matters here.
Commit Message Details
The wire fixes, with the table that settles each
powerDownTime→Option<u8>u8thatencodealways wrote, so decoding51 01and re-encoding produced51 01 00— any proxy rewrote every positive response except 0x04'sCvt = C, present only forenableRapidPowerShutDown; Table 39's example is the 2-byte51 01. Table 36 defines0xFF, not0x00, as not-availableReadDtcInfoRequestgainedsuppress_positive_responsedecodematched the raw sub-function byte, so bit 7 (SPRMIB) was read as part of the value.19 82 FFwas rejected as trailing bytes and19 8Adecoded as reserved, so a server answeredSubFunctionNotSupportedto a request it must executeReportUserDefMemoryDtcByStatusMaskgainedMemorySelection#3 #4:DTCStatusMask M,MemorySelection MNumberOfDtcsgainedformat_identifier59 01 2F 01 00 01was rejected and a count of 1 came back as0x0100M, format identifier between mask and countmemorySize1-3 andmemoryAddress1-4 accepted, so the crate emitted frames it could not decode. ALFID0x44— the usual value in a programming session — was rejected outrightClearDiagnosticInformationmemorySelection→ optionalCvtrow isM M M U; Table 300's example is14 FF FF 33ControlDtcSettingRequestgainedoption_recordCvt = U; Table 132 reserves NRC 0x31 for "an error in the DTCSettingControlOptionRecord"CommunicationControldecodes the subnet nibble0x00..=0x03, so0xF3— a routine real-world value — was rejected, and encode wrote only the low nibbleC1/C2gate whole records only. Note 0x14's array has no availability-mask byte (Table 326 starts records at#3)routineControlTypewas answered 0x13 for a correctly-sized message; a 3-byteWriteDataByIdentifierdecoded to an empty data record so a server would attempt a zero-length writeNot a spec matter, but the worst bug on the branch: the DTC iterators looped forever on a
partial record.
next()returnedSome(Err(..))without advancing, sofor,count()andcollect()all diverged. Reachable from wire input.Coverage of the fixes
tests/spec_conformance.rsround-trips 36 byte sequences quoted from the standard's own numberedexample tables. That is the oracle the rest of the suite lacks: a round-trip written against the
crate's own output passes whenever
encodeanddecodeshare a misreading, which is how five ofthese defects survived.
Read its module doc for where the line falls — it catches a legal frame the crate rejects and an
encode/decode pair that disagree with each other, but not a misreading the two share
symmetrically. Transposing two same-width adjacent fields in both directions still round-trips.
Two extraction hazards are documented in that file because I hit both: Tables 486 and 487 put two
bytes in a single cell (
C3 16 50 16), which a naive one-byte-per-row read truncates; and forTables 31/38/47 the markdown conversion merged the byte-value column into the description, which I
initially misread as the standard not providing request examples at all. It does. Table 49 survived
intact and is included; Table 472 stays out because two of its
memorySizebytes are not legible,and inferring them is exactly the mistake being avoided.
Why RequestUpload (0x35/0x75) is in a wire-conformance branch
Two reasons. It is the one service the crate did not model at all, which is a conformance gap
rather than a feature. And mechanically, its commit renames
request_download.rs→upload_download.rs, so every later commit touching that file depends on it — the ALFID fix cannotbe lifted out without it.
I verified the shared macro is safe before keeping it: Tables 440 vs 445 (request) and 442 vs 447
(positive response) have the same rows with the same
Cvtand byte ranges, and Tables 444 vs 449give both services the same six NRCs.
Issue URL
No linked issue — second split of the
api/consistency-pass-1work. The No Issue labelpr-lint.ymloffers still does not exist in this repo (noted on #48 and #51).Testing
--all-features(235 lib + 2spec_conformance+ 3 + 5 doctests) and 225--no-default-features, all passing on this branch in its own right rather than inherited fromthe source branch.
cargo clippy --all-targets -D warnings -D clippy::pedanticclean under--all-featuresanddefault features.
--no-default-featuresreports 17, down frommain's 22 — this branch improvesit; the rest is cleaned up in the test-integrity commits later in the split.
cargo fmt --all --check,cargo doc --no-deps --all-features, andcargo hack check --feature-powerset(16/16) all clean.thumbv6m-none-eabi: no-default,alloc,serde,and
alloc,serde. That matters on this branch specifically — one of its commits rewiresserdetodefault-features = false, and theserdebare-metal build is the only place that defect isobservable. It passed every host-side build while broken, and the reusable CI workflow does not
cover it (raised on ci: replace the hand-rolled workflow with the org reusable Rust CI #48), so it is verified here by hand.
conflicts that arose were resolved against the upstream file content, and the result was checked
by diffing this branch's tree against the upstream commit it corresponds to: the only differences
are the CI files from
mainand the two commits deliberately left for a later branch. That checkis what caught a resolver bug that had silently truncated
Cargo.toml's[dependencies].#[non_exhaustive]sealing and theRequestTransferExitserde derives — commits that are not on this branch. Left in, they wouldhave described changes absent from the diff.