fix!: report unknown SPRMIB as None instead of not-suppressed - #55
Merged
Conversation
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.
Most of the crate already hedges against ISO adding values, but eleven public types were missed. `ReadDTCInfoSubFunction` is the notable one: it sits directly above `ReadDTCInfoResponse`, which does have the attribute, and it has an in-source TODO plus sub-functions the crate does not model yet — so adding 0x1B post-tag would have been a breaking change. Enums (ISO reserves value space each one will grow into): ReadDTCInfoSubFunction, FileOperationMode, DTCExtDataRecordNumber, DTCSnapshotRecordNumber Structs with `pub` fields, matching how every other request/response struct in the crate is already declared: DTCFaultDetectionCounterRecord, SizePayload, NamePayload, SentDataPayload, FileSizePayload, DirSizePayload, PositionPayload Deliberately excluded, where the attribute would constrain nothing: private-field newtypes that can only be built through `new()` (DTCRecord, DataFormatIdentifier, SecurityAccessLevel, DTCStoredDataRecordNumber, the three DTC iterators) and the bitmask_enum-generated masks, where all 256 values are valid.
Every other public request/response type carries `#[cfg_attr(feature = "serde", derive(..))]` and the utoipa equivalent. The two macro-generated `RequestTransferExit` types carried neither, with no documented reason and no obstacle — their only field is `&[u8]`, so `#[serde(borrow)]` applies exactly as it does on `TransferDataRequest`. Enabling `serde` therefore left 2 of 32 public types unserializable. Pinned with the existing `derive_contract` test convention. Also drops the `serde_bytes` optional dependency. It was activated by the `serde` feature but referenced nowhere in the crate. `ReadDataByIdentifierRequest` keeps its carve-out: the `Dids::Native(&[u16])` zero-copy backing genuinely has no borrowed `Deserialize` impl, and the existing comment documents that correctly.
`RequestDownloadRequest::new` / `RequestUploadRequest::new` derive the *minimal* width for the address and size, but ISO 14229-1:2020 Table 441 makes the addressAndLengthFormatIdentifier a client choice, not a function of the values. Table 462's own example declares 3 bytes of memorySize for 0x00FFFF, which needs only 2 -- so the crate could not reproduce the standard's example frame, and it cannot talk to the many bootloaders that mandate a fixed ALFID (commonly 0x44) and answer requestOutOfRange otherwise. Add `new_with_widths`, which validates both widths against Table H.1 and rejects a value too large for the width declared for it -- the case that would otherwise truncate silently on the wire. `new` stays as the convenience path. Also document what `RoutineControlResponse::status_record` actually spans. Table 428 places an optional `routineInfo` byte at #5, immediately before the status record, and leaves its presence to the vehicle manufacturer -- nothing on the wire distinguishes the two layouts, so a general-purpose decoder cannot split them. The field name suggested it held only the status record while it in fact holds both, which shifts every positional read by one for a server that sends routineInfo. Modelling it as `Option<u8>` would require the decoder to know something the wire does not carry, so the honest fix is to say so, and to pin it with a test before someone "corrects" it into a wrong split.
`derive(Deserialize)` ignores both field visibility and constructor validation,
so for every type whose invariant lives in `new`/`try_from` it was a second,
unchecked way in. Proven from an integration test, which sees the crate as a
downstream user does:
serde_json::from_str::<SecurityAccessLevel>("255") -> Ok before, Err now
That one mattered: a level must fit 0x00..=0x7F because bit 7 of the
sub-function byte is SPRMIB, so the deserialized 0xFF encoded to a byte that
decoded back as a *different* level with suppression set. `DataFormatIdentifier`
likewise accepted `{"encryption_method":255,...}` for a field `new` caps at
0x0F, and the range-checked enums let a caller name a reserved variant directly
and skip `try_from` entirely.
These six types are one protocol byte, so route their serde impls through `u8`
with `serde(try_from = "u8", into = "u8")` -- `from` for `DataFormatIdentifier`,
whose byte form is total. That makes the serialized form the wire byte, which is
both unambiguous and impossible to construct an out-of-range value from.
A derived `ToSchema` would then describe the Rust shape (an object of nibbles, or
a oneOf of variant names) and disagree with what serde actually reads, so the six
get a hand-written integer schema instead. `SecurityAccessLevel` keeps its derive:
as a newtype its schema is already the inner `u8`.
`serde_json` becomes a dev-dependency for these tests; it is not a dependency of
the crate.
Still outstanding from the same finding: the composite requests
(`TesterPresentRequest`, `RequestDownload`/`UploadRequest`,
`CommunicationControlRequest`) need shadow-struct deserialization routed through
their constructors, which will also remove `ZeroSubFunction` and
`MemoryFormatIdentifier` from the public serde/OpenAPI surface.
Completes the previous commit. `TesterPresentRequest`,
`CommunicationControlRequest` and `RequestDownload`/`RequestUploadRequest` seal
their fields to hold a rule, and a derived `Deserialize` walked straight past
each one:
{"control_type":5,...,"node_id":null} accepted -> encoded 2 bytes that this
crate's own decoder then rejected
{"memory_address":1<<64-1,"memory_address_length":1,...}
accepted -> address truncated to one
byte on the wire
{"zero_sub_function":{"IsoSaeReserved":66}}
named a module-private type and skipped
its 0x00..=0x7F range check
Each now serializes through a private repr of its public wire parameters and
deserializes back through `new` / `new_with_node_id` / `new_with_widths`, so the
constructor's validation is the only way in. That also removes `ZeroSubFunction`
(module-private) and `MemoryFormatIdentifier` (`pub(crate)`) from the serialized
shape and from the generated OpenAPI schema, where they had been giving client
generators types downstream Rust cannot name.
The schemas are hand-written to follow the repr rather than the Rust fields, for
the same reason. `LengthFormatIdentifier` loses its derives too, and
`SuppressablePositiveResponse` loses derives that were pure dead codegen -- it is
never a field of anything.
Verified across the whole feature matrix, including `utoipa` without `serde`,
which is why the reprs are gated on either feature rather than on `serde` alone.
Mutation testing found 11 of 23 mutants surviving. These close the ones that represent real coverage gaps rather than equivalent code. Values the iterators yield were never asserted. Reversing the DTC bytes in `WwhObdDtcSeverityIter` while also swapping severity with status passed the entire suite, because every test touching it checked only counts and `is_ok()`. Same for `DtcAndStatusIter`'s status byte -- "every DTC reports status 0x00" was invisible -- and for all four header fields of the 0x42 response. `allowed_nack_codes_dispatches_for_every_modeled_variant` asserted only `!is_empty()`. Every modeled service has a non-empty table, so any of the 16 arms could return any other service's set. Each frame is now paired with the inherent table it must dispatch to; verified by mis-wiring the ReadDtcInfo arm and watching it fail with "ReadDtcInfo dispatched to the wrong NRC table". `record_lists_must_divide_evenly_into_records` looped `1..width`, so it never tested a list too short to hold one record -- the 0-valid/1-invalid boundary that is the entire point of the check. `all_three_iterators_terminate_and_stay_exhausted` drained with an unbounded `while next().is_some()`, so reverting the iterator fix hung `cargo test` instead of failing it, hiding the other failures. Its sibling's comment says it is bounded precisely to avoid that; now this one is too. Six "round-trip" tests encoded into a buffer they never read, checking only the length. Swapping the two bytes `EcuResetResponse` writes survived in both feature configurations. They now assert the bytes. Ten codec tests were gated on `alloc` only because they used `Vec` as a writer; a stack buffer works identically. That leaves the no_std configuration -- the one the crate targets first -- covering 228 tests rather than 186 at the start of this review, and takes `clippy --no-default-features --all-targets` from 21 warnings to zero. Also correct `assert_encode_size_agrees`' doc comment: `encoded_size` is a provided method that counts by encoding into a sink, so every quantity the helper compares comes from `encode` itself and it proves nothing about *which* bytes are written. Reading its ~78 call sites as byte-correctness coverage is what made the two gaps above possible. And the `negative_response_code` doctest asserted nothing when decode succeeded; it now uses `expect_err`.
Three of these were introduced by this branch and are corrections to text I wrote. `DtcRecord::high_byte`'s doc said "ISO 14229-1 Annex D.1 uses [it] to identify the system group (powertrain, body, chassis, network)", and CHANGELOG repeated it. Annex D.1 Table D.1 defines whole 3-byte `groupOfDTC` *values*, and its powertrain/chassis/body/network rows are literally "to be determined by vehicle manufacturer" -- there is no high-byte mapping. The one byte-level assignment the table makes is the opposite one: for 0xFFFF00-0xFFFFFE the *low* byte is a FunctionalGroupIdentifier. Clause 12.3.2.3 is explicit that ISO 14229-1 specifies no decoding method for the three bytes; that is the DTCFormatIdentifier's job. `low_byte`'s "carries the failure type" was SAE J2012 semantics stated as universal. Also false, also mine: "the last `new()` in the crate that was not [const]" (`CommunicationControlRequest` has two), "All 15 request types" (16 -- RequestUpload landed two commits earlier), "the seven structs must be built through `new()`" (one had no constructor at all), Table D.1 cited for FunctionalGroupIdentifier where it is Table D.15, and two counts of enums/accessors that match nothing countable. Pre-existing and more consequential: NRC 0x78 carried 0x73's description, calling the response-pending code a BlockSequenceCounter error. It is the code that extends P2*, and Annex A.1 obliges the server to send a final response regardless of SPRMIB -- the worst single NRC to describe wrongly for an audience writing ECU-facing code. And `p2_star_server_max` documented its scaling backwards: Table 29 gives it a 10 ms resolution, so reading the stored value as milliseconds under-waits by a factor of ten. `DtcStoredDataRecordNumber::new` rejected 0xF0 citing ISO. Clause 12.3.3.2 reserves only 0x00 for this parameter; 0x00/0xF0 is the snapshot record-number space, and both the check and the doc had been copied from there -- so this is a behaviour fix, not just a wording one. `ReadDtcInfoResponse`'s InsufficientData shortfalls counted `needed` from the sub-function byte and `available` from after it, so `needed - available` was one too large at three sites. Now both are payload-relative, pinned by a test. README: `AccessTimingParameters` is footnoted as :2013-only (removed in the 2020 edition the crate claims to target), `DynamicallyDefinedDataIdentifier` is renamed to the `DynamicallyDefineDataIdentifier` of Table 23 in both the table and the enum, six escaped bracket pairs that rendered as dead `[Request]` text on the crate's own front page are real links again, the re-export list no longer promises the codec traits "eventually" when they are already there, and the stray 0.1.0 reference is gone.
`NamePayload` carried a `mode_of_operation` that the `RequestFileTransferRequest`
variant already *is*, with nothing keeping them in step -- and the field won:
RequestFileTransferRequest::AddFile(
NamePayload::new(FileOperationMode::DeleteFile, "/a"), dfi, size)
encoded a DeleteFile request and silently dropped the data-format identifier and
both file sizes; at frame level the leftover bytes surfaced as `TrailingBytes`
from a request the caller believed well-formed. The compiler could not catch it.
The mode byte now comes from the variant, via a new
`RequestFileTransferRequest::mode_of_operation()`, so the contradictory state is
unrepresentable. `NamePayload` is just the length-prefixed name, which is also
what makes it a coherent type rather than a bag.
`DtcStoredDataRecordNumber::new` returned `Result` while `From<u8>` accepted
anything, so the invariant it advertised was not one the type held --
`new(0x00)` failed and `from(0x00).value()` succeeded on the next line. It is now
total like both sibling record-number types, with an explicit `is_reserved()`
predicate for callers who want the check, and it gains the `PartialEq<u8>` both
siblings already had. All three now have the same shape.
That leaves `Error::ReservedForLegislativeUse` with no construction site, so it
goes the way of the other three unreachable variants. `FileOperationMode` gains
a `const value()` like its peers.
`PartialEq` on these enums is variant equality, not wire equality, so being able to name a byte-carrying reserved variant directly created a trap: `DtcFormatIdentifier::IsoSaeReserved(0x01)` is not equal to `Iso14229_1DtcFormat` yet encodes the same byte, so a downstream `if resp.format_identifier == DtcFormatIdentifier::Iso14229_1DtcFormat` returned false for a value that is byte-identical on the wire. `#[non_exhaustive]` on those variants makes the aliasing state unconstructible from outside, leaving the total `From<u8>`/`try_from` classifier -- which never aliases -- as the only way in. This is the pattern the four sub-function enums (`CommunicationControlType`, `SecurityAccessType`, `ResetType`, `DiagnosticSessionType`) already used; it now covers `DtcFormatIdentifier`, `FunctionalGroupIdentifier`, `FileOperationMode` and `ReadDtcInfoSubFunction`. The byte is still readable via `value()`. Pinned from the integration test, where the attribute actually applies, by checking the property that matters across all 256 bytes: the classifier preserves the byte, and equality with a named variant agrees with the wire. Also records this round's work in the CHANGELOG under three new sections.
Only visible under `cargo doc --document-private-items`, which CI does not run.
A per-commit validity review of this branch, one reviewer per slice, checked each
change against the ISO text rather than against the crate. Four of my changes were
wrong, not merely unnecessary. Each is reverted or corrected here rather than in the
commit that introduced it, so the correction is visible in the history instead of
hidden by a rewrite.
1. The lengthFormatIdentifier's reserved low nibble is not ours to keep.
ada8674 added `reserved_low_nibble` so a decode/re-encode would be byte-exact,
on the stated premise that "ISO 14229-1 leaves that low nibble undefined". It
does not. Tables 443 and 448 both say bits 3-0 are "reserved by document, to be
set to '0'", state that "the lower nibble shall be set to '0'", and give the byte
range as 0x00 to 0xF0. So 0x25 is not a legal lengthFormatIdentifier, and the old
zeroing behaviour was the conformant one -- the change made the crate re-emit a
byte the standard forbids, and paid for it with a private field on two public
response types and a PartialEq that distinguished responses by a nibble that must
always be zero.
The property test kept its full-u8 generator, which is the useful half of that
commit: it now asserts the normalization (`byte & 0xF0`) rather than a round trip.
The old `high_nibble << 4` generator held trivially whichever way the impl behaved.
2. An invalid addressAndLengthFormatIdentifier is NRC 0x31, not 0x13.
Tables 444 and 449 both list requestOutOfRange for "the specified
addressAndLengthFormatIdentifier is not valid". The code answered
IncorrectMessageLengthOrInvalidFormat, i.e. 0x13. Worse, bb80477 deleted the
`// NRC::RequestOutOfRange if ...` comment that recorded this, while editing those
exact lines -- so the fix removed the only in-tree note of a live defect. New
Error::InvalidAddressAndLengthFormatIdentifier carries the byte and maps to 0x31.
3. Error::IoError does not map to generalReject.
Annex A.1: 0x10 "shall only be implemented in the server if none of the negative
response codes defined in this document meet the needs of the implementation. At no
means shall this NRC be a general replacement." It also appears in no per-service
NRC table, including the crate's own -- so `err.negative_response_code()` returned a
code that every `allowed_nack_codes()` rejects. ISO does not model a transport
failure as an NRC at all; clause 7.4.1.6 surfaces it as `A_Result = error` to the
server application. There is no byte to send, so the method now returns
`Option<NegativeResponseCode>` and `None` for IoError.
Its doc block also claimed to be "Following ISO 14229-1" when the mapping is a
default over the clause 8.7.5 / Annex A.1 mandatory lane; clause 8.7.2 says a
specific NRC "is not guaranteed for all possible test pattern sequences". And
`mapping_only_produces_codes_the_iso_tables_allow` asserted something both weaker
than its name and refutable: the per-service tables are a floor, not a ceiling
(clause 9.4). Renamed to what it checks.
4. CommunicationType's serde had no hole to close, and the fix opened one.
d5a6180 routed it through `serde(try_from = "u8")` along with five types that
genuinely needed it. Its four unit variants are in bijection with 0x00..=0x03 and
TryFrom<u8> accepts all four, so nothing could be smuggled. But TryFrom<u8> reads
bits 1-0 of a whole communicationType byte and masks the rest, so deserializing 17
silently yielded Normal and re-serialized as 1 -- exactly the silent normalization
the commit set out to eliminate. Back to the derived variant-name form.
Also in scope, from the same review:
- ReadDtcInfoSubFunction::try_reserved. f260dee sealed IsoSaeReserved to keep bit 7
out of it, which is the one variant seal on this branch that guards a real
invariant -- but the enum has no public TryFrom<u8>, so sealing it left a tester
unable to originate a request for any unmodeled sub-function. That is a capability
removed, not a hole closed. try_reserved rejects bit 7 and is covered from
tests/public_api.rs, where #[non_exhaustive] actually applies.
- Table 13 was cited for two different things it does not say. The Cvt M/C/S/U legend
is Table 8 ("A_PDU parameter conventions"); the SPRMIB rule that "values of both '0'
and '1' shall be supported for all SubFunction parameter values" is Table 11
("SubFunction parameter structure"). Table 13 is "SubFunction parameter conventions"
and has only M and U, about whether the server supports a sub-function at all.
Corrected in three rustdoc sites and the CHANGELOG.
Two of the three test holes the review reported were real. I checked all three by
mutation rather than taking the reports at face value, and the third does not exist.
REAL: the DTC status byte was unprotected in the config the crate targets first.
`dtc_and_status_iter_roundtrip` asserted the status byte, but it was gated on
`alloc` because it used `collect()`. Mutating `DtcAndStatusIter::next` to
`DtcStatusMask::from(0)` left `cargo test --no-default-features` at 236 passed, 0
failed. So the assertion that commit c654ce8 added specifically to kill "every DTC
reports status 0x00" only ran under `--all-features`. Rewritten with `next()`, gate
removed; the same mutant now fails in both configs. no_std count 236 -> 237.
REAL: a frame for an unmodeled service would have passed spec_conformance silently.
`Request::Other` and `Response::Other` echo the SID and payload verbatim, so they
round-trip perfectly. Both suites now assert the decode is not `Other`, which is
what makes "this service has spec-example coverage" mean anything.
NOT REAL: the ECUReset powerDownTime mutant.
Reported as surviving all 264 tests. It does not: `[] => (Some(0), rest)` fails
`an_absent_power_down_time_is_distinguishable_from_a_reported_zero` and
`a_response_without_a_power_down_time_round_trips_unchanged`, in both
--all-features and --no-default-features. No change needed.
NOT REAL: the allowed_nack_codes dispatch swap.
Swapping the CommunicationControl and ControlDtcSetting arms does leave the suite
green, but that is correct rather than a gap: ISO gives those two services the same
four codes, and Tables 444/449 give RequestDownload and RequestUpload the same six.
The dispatch is unobservable for identical tables, so there is nothing to pin. Noted
in the test instead of chased.
Also corrected three claims that were false:
- spec_conformance said ISO gives no request byte tables for 0x10, 0x11 and 0x27, so
they appear "on the response side only". Wrong: Table 31, Table 38 and Tables
47/49/51 are all request byte tables. What actually happened is that the markdown
conversion merged the byte-value column into the description cell for three of them
("ECUReset Request SID 11 16") and I read an extraction artifact as a property of the
standard -- the same hazard the file already warns about for Tables 486/487. Table 49
survived intact and is now included (27 02 C9 A9), bringing the suite to 36 frames.
Table 472 (RequestUpload) stays out: bytes #7 and #8 of its memorySize are not
legible, and inferring them is exactly the mistake being corrected here.
- The suite's module doc claimed it catches "a layout error that both encode and decode
share". It does not, and the reviewer proved it: transposing p2_server_max and
p2_star_server_max in *both* directions leaves Table 32 round-tripping and all three
tests green. Rewritten to state what it does buy -- a legal frame the crate rejects,
and an encode/decode pair that disagree -- and what it does not.
- The CHANGELOG said 34 frames; it was 35 before this commit. Fixed to 36, which is the
same miscount class the docs commit exists to fix.
And one doc defect: Request::allowed_nack_codes read as a closed set. Clause 9.4 says
the Annex A.1 codes "shall be used in addition to" the per-service tables, and A.1
keeps the generally-supported codes out of them deliberately -- including 0x78
RequestCorrectlyReceivedResponsePending, which is in none of the 16 tables and is one
of the commonest codes in real traffic. A client using this slice as a validation
whitelist would reject every ResponsePending it saw. Documented as a floor.
The review found this branch added 15 #[non_exhaustive] attributes, of which 10 guard
nothing. Two reviewers disagreed about which, so I checked each encoder rather than
picking a side -- and the split is not the one either of them proposed.
Structs: five were data bags, two were mis-diagnosed.
SizePayload, FileSizePayload, DirSizePayload, PositionPayload and
DtcFaultDetectionCounterRecord have every field pub and derive their wire widths from
the values, so no combination can fail to encode. Under the crate's own rule --
encapsulate iff invariant-bearing -- they are data bags, and the seal only cost
downstream the struct literal (E0639). It cost more than that in practice:
DtcFaultDetectionCounterRecord became unbuildable outside the crate entirely and took
two follow-up commits and a new test file to repair. Seals dropped.
NamePayload and SentDataPayload are a different case, and the reviewer who called all
seven "no invariant" was wrong about these two. Their encoders do
`u16::try_from(name.len())` and `u8::try_from(block.len())` and fail -- so an
over-long value was constructible and only rejected later, which is precisely the
construct-then-fail asymmetry RequestDownloadResponse::new was made fallible to close.
Rather than drop their seals, `new` now enforces the bound. Now the seal guards
something and the type is consistent with its twin.
Variants: the seal did not work, and the mechanism that does work replaces it.
Verified from a real downstream crate: with a plain derived Deserialize,
`{"IsoSaeReserved":1}` builds a DtcFormatIdentifier whose value() is 0x01 -- the byte
Iso14229_1DtcFormat encodes -- that compares unequal to it. That is the exact aliasing
state f260dee claimed to make unconstructible, reachable in one line. The CHANGELOG
entry saying otherwise was false.
So the seal bought no guarantee, and it was not free: both `IsoSaeReserved(b)` and
`IsoSaeReserved(..)` are E0603 downstream, meaning a caller could not even read the
byte by matching. The only spelling that compiles is `IsoSaeReserved { 0: b, .. }`,
which is syntax most Rust developers have never written, let alone C developers new to
the language. And it was applied to 3 of roughly 30 byte-carrying variants, so
downstream could not predict whether naming one would compile.
DtcFormatIdentifier, FunctionalGroupIdentifier and FileOperationMode now do what
DtcSettingType and SubnetNumber already did: route serde through the byte with
serde(from = "u8", into = "u8"), which makes every byte classify through the same
function the wire uses, and add PartialEq<u8> so wire equality is directly
expressible. Seals dropped; the variants are nameable and destructurable again.
ReadDtcInfoSubFunction::IsoSaeReserved keeps its seal. It is the one that guards a
real invariant -- bit 7 is SPRMIB and is fused in at encode -- and it now has a door
(try_reserved, added in the previous commit).
Also: TryFrom<u8> for FileOperationMode never returned Err -- every arm was Ok, because
every byte outside 0x01-0x06 is ISOSAEReserved, which the enum represents rather than
rejects. A fallible signature that cannot fail. Now From<u8>.
Tests moved to where they mean something. The guard test that policed reachable
constructors for seven sealed structs is gone; in its place, tests/public_api.rs
constructs the five data bags with struct literals, checks the two bounded ones reject
an unencodable value, proves the serde bypass is closed for all 256 bytes, and proves a
reserved variant can be destructured downstream again.
The hand-written utoipa impls this branch introduced were a net regression on the
derived ones they replaced. Three defects, all reproduced against a real generated
document before fixing and all mutation-verified after:
1. Four dangling $refs, where the derive had zero.
ToSchema::schemas() defaults to a no-op and the derive is what overrides it, so a
hand-written impl that only provides schema() emits $refs to types the document never
defines. CommunicationControlType, CommunicationType, DataFormatIdentifier and
SubnetNumber were all referenced and undefined, which every client generator either
rejects outright or degrades to an untyped object. All four hand-written composites
now forward schemas() to their repr.
2. Two descriptions were implementation notes, and one had lost its verb.
utoipa's derive reads only literal doc attributes and silently drops
`#[doc = concat!(..)]`, so RequestDownloadRequest's published description began
"\nDeserializing routes through\nso a declared width..." -- the fragment carrying the
sentence's subject was gone, along with the type's real documentation. That macro's
repr doc is now plain `///` lines only, with a comment saying why.
TesterPresentRequest's description was the repr's internal rationale, and it named
ZeroSubFunction -- the module-private type the repr exists to keep out of the schema.
These docs are published verbatim, so the rationale moved to `//` lines and the `///`
text now reads as API documentation. Same for CommunicationControlRequest and for
CommunicationType, whose description had picked up my note about why it does not use
the byte form.
3. Every byte schema advertised a range four times wider than its deserializer accepts.
Delegating to `<u8 as PartialSchema>::schema()` emits
`{"type":"integer","format":"int32","minimum":0}` -- no maximum. But SubnetNumber
rejects anything above 0x0F, CommunicationControlType and SecurityAccessType above
0x7F, and DtcSettingType accepts only 65 of the 126 bytes in its range. A generated
client emitted an i32 and found the real bound as a runtime rejection. The whole
premise of these types is that their bytes have ranges, and the schema was the one
place that did not say so.
byte_schema! now takes a max and a description per type. DtcSettingType's set is not
contiguous, so minimum/maximum cannot express it and the description spells out the
four accepted ranges and says the bounds are looser than the real constraint.
The reason all three shipped is that tests/ contained no utoipa assertion at all, so
tests/openapi_schema.rs is the substantive part of this commit. It builds a real document
and checks that no $ref dangles, that no description is empty or names a private type or
starts with the whitespace a dropped concat! fragment leaves behind, and -- the strongest
of the three -- that each byte schema's `maximum` equals the largest byte serde actually
accepts. That last one does not hard-code a bound: it deserializes all 256 bytes to
discover it, so the schema cannot drift from the deserializer without failing.
Verified by mutation: dropping `.maximum(..)` fails the bound test; restoring
ZeroSubFunction to a description fails the leak test; removing the schemas() forwarding
from CommunicationControlRequest fails the dangling-ref test, naming all three children.
Removing TesterPresentRequest's forwarding does not fail, and correctly so -- its repr
has only scalar fields, so it $refs nothing and the forwarding there is defensive.
…ibutes
SubFunctionRepr and SubFunctionOnlyRepr were 108 lines: two mirror structs, four
conversion impls, four hand-written schema impls, two `cfg(any(serde, utoipa))` gates and
two `allow(dead_code)`. They existed to keep the module-private ZeroSubFunction out of the
serialized form and the schema while still range-checking the byte on the way in.
The invariant is single-field, so a field attribute expresses it directly. ZeroSubFunction
already had the TryFrom<u8> that does the checking -- the same classifier `decode` uses --
so routing serde through it with `serde(try_from = "u8", into = "u8")`, plus
`serde(rename = "sub_function")` and `schema(rename, value_type = u8, ...)` on the field,
does the whole job. The reprs re-implemented a check that was already one line away.
The commit that added them justified hand-writing whole-type schemas with "`value_type` is
field-level only". That is true, and it is exactly why it applies here: the leak *is* a
field. tester_present.rs goes 477 -> 369 lines with nothing hand-written left in it.
Verified equivalent, not assumed. I captured the serialized output, the rejection
behaviour and both schemas before the change and diffed after:
- serialization is byte-identical: `{"suppress_positive_response":false,"sub_function":0}`
and `{"sub_function":0}`, and a reserved 0x42 still round-trips unchanged;
- the rejection message is the same, now with serde_json's line/column appended, because
the error surfaces from a field rather than the container;
- the schemas are strictly better. They gained `maximum: 127`, which the reprs never
advertised despite existing to describe the range; the top-level description is now the
type's own doc instead of the repr's; and the fields have descriptions, which the reprs'
bare `u8` fields did not.
Two things the change exposed:
- Field descriptions are published too, and my first version of the field doc put the
implementation rationale in `///`, so it landed in the schema. Moved to `//`, and
tests/openapi_schema.rs now checks field descriptions for private type names as well as
top-level ones -- it had only covered the latter, which is why nothing caught it.
- The range check is now pinned from outside the crate. tests/public_api.rs asserts every
byte 0x80..=0xFF is rejected for both types, and every byte 0x00..=0x7F round-trips
unchanged, so the guarantee the reprs provided is tested rather than inferred from the
mechanism being present.
… set
A `ToSchema` in this crate describes the *serde* representation, not the Rust shape.
Several types serialize as a single protocol byte -- a `DataFormatIdentifier` is `33`, not
`{"compression_method":2,"encryption_method":1}` -- and their schemas are hand-written to
match. So a `utoipa`-without-`serde` build published a schema for a wire format that build
could not produce.
Supporting that combination also cost real complexity. Every type reachable only through a
serde repr needed `#[cfg(any(feature = "serde", feature = "utoipa"))]` rather than a single
predicate, plus `#[cfg_attr(not(feature = "serde"), allow(dead_code))]` and two lines of
comment explaining why fields that are never read still have to exist. That is four
attributes and a paragraph of justification apiece, to keep compiling a configuration
nobody wants. Both are gone; `cfg(feature = "serde")` now says the whole thing.
The feature powerset is 16 combinations rather than 20 as a result, all clean, and all four
`thumbv6m-none-eabi` builds still pass -- `serde` remains the one optional integration
usable on bare metal, which this does not change.
Also adds the feature table the README never had. The crate has five features and
documented none of them, which for a `no_std` crate is the first thing a reader needs; a
new implication between two of them with nowhere to record it is what made this the moment
to fix that. Both implications are called out with their reasons, since neither is
guessable: `utoipa` implying `serde` for the reason above, and `utoipa`/`clap` implying
`std` because their derive macros expand to `std::`, `String` and `Vec` paths that cannot
compile under `#![no_std]`.
The `DataFormatIdentifier` example in that table is checked, not assumed --
`serde_json::to_string(&DataFormatIdentifier::new(2, 1)?)` is `33`.
…ose widths
`new_with_widths(dfi, address, address_len, size, size_len)` had five positional
parameters, four of them numeric, containing two transposable value/width pairs. Nothing
could catch a swap: all four are integers, and the ALFID nibbles are asymmetric (high is
memorySize, low is memoryAddress), so transposing the two widths silently truncated a value
on the wire and produced the one parameter a bootloader answers requestOutOfRange for.
It also duplicated `new`. They were independent code paths, and they disagreed -- verified
before changing anything:
new(NONE, 0x1_0000_0000_0000, 0x10) -> InvalidMemoryAddress (NRC 0x31)
new_with_widths(NONE, 0x1_0000_0000_0000, 5, ..) -> IncorrectMessageLength... (NRC 0x13)
One input, one rejection, two different bytes on the wire depending on which constructor a
caller happened to reach for. No test covered it.
`AddressAndLengthFormatIdentifier` is now public and carries the widths as a unit:
- named for the ISO parameter it is, rather than the invented `MemoryFormatIdentifier`, which
is consistent with `DataFormatIdentifier` and `DtcFormatIdentifier` already following ISO;
- private nibbles behind `new(memory_size_length, memory_address_length)` and accessors, so a
value outside Table H.1 cannot exist -- the widths were `pub` fields on a `pub(crate)`
struct before, which is why the request had to re-check them;
- built from a byte with `try_from(0x44)`, which is how the requirement actually reaches a
caller ("this bootloader needs ALFID 0x44") and cannot be transposed;
- `PartialEq<u8>` so `alfid == 0x44` reads the way the spec does.
`new_with_alfid(dfi, alfid, address, size)` replaces the five-arg version, and `new` derives
the minimal widths and **delegates** to it, so exactly one function decides whether a
(value, width) pair is acceptable. New `Error::InvalidMemorySize` completes the pair with
`InvalidMemoryAddress`; both map to NRC 0x31, which is what Tables 444 and 449 require for a
memoryAddress/memorySize that "is not valid", and they name which parameter was at fault
rather than collapsing into one length error.
Pinned by `both_constructors_reject_an_over_wide_address_the_same_way`, which is the test
that did not exist. Mutation-verified: putting the old
`IncorrectMessageLengthOrInvalidFormat` back on the address path fails four tests across
both services.
The serde shape changes with it. The widths were two derived scalars, flattened by the repr
because the type behind them was `pub(crate)`; they are now the single
`address_and_length_format_identifier` byte, so the JSON matches Table 441 instead of
paraphrasing it, and the width validation happens in that type's own `try_from` rather than
being re-implemented in the repr's conversion. Its schema carries the real bounds and notes
that the accepted set is not contiguous -- 0x15, 0x25, 0x35 and 0x40 to 0x43 sit inside
0x11..=0x45 and are rejected -- which tests/openapi_schema.rs now enforces along with the
rest.
Making a constructor fallible does nothing while the field it guards is `pub`.
`WriteDataByIdentifierRequest::new` rejected an empty data record, and then:
let mut req = WriteDataByIdentifierRequest::new(0xF190, &[0x01])?;
req.data = &[]; // walks straight around the check
req.encode_to_slice(&mut buf) // Ok([0xF1, 0x90])
That is a 3-byte frame this crate's own decoder answers NRC 0x13 to -- ISO 14229-1:2020
Table 277 marks dataRecord byte #1 mandatory and Figure 26 states a four-byte minimum. So
the crate could be made to emit traffic it refuses to read, and the `Result` was theatre.
`#[non_exhaustive]` does not help: it blocks a struct literal, not assignment to a field of
a value you already have.
Two of the three were my own, added two commits ago when I made `NamePayload::new` and
`SentDataPayload::new` fallible to justify keeping their seals -- and left both fields `pub`,
which is the same mistake in the same round. All three fields are now private with
accessors, which is what the crate's rule (encapsulate iff invariant-bearing) already said
to do.
The serde half is closed with field-level `deserialize_with` hooks in a new
`shared::bounded`, not with mirror structs: each invariant is a property of one field, so
there is nothing for an aggregate to coordinate. That is the lesson from the TesterPresent
reprs, which spent ~100 lines on a single-field invariant.
One honest limitation, pinned by a test rather than papered over. `&'de [u8]` needs a format
with a native byte-string type, and JSON has none -- serde_json fails with "invalid type:
sequence, expected a borrowed byte array" before any length check runs. My first version of
this test asserted that an empty `"data":[]` was rejected and passed, which looked like the
guard working; it was rejecting every array, empty or not. So the byte-slice guards are only
exercised by a format that can borrow bytes (CBOR, MessagePack, bincode). They are still the
right place for the check, since the crate does not choose the caller's format, but the test
now says so explicitly and will fail if JSON ever starts accepting these.
The `&str` guard *is* reachable through JSON, because JSON strings borrow. That one is tested
for real -- a name one byte past u16::MAX is refused, u16::MAX exactly is accepted -- and
mutation-verified by disabling the bound.
…ssigns it
The shared macro emitted one un-parameterised doc for both services, written for download.
For `RequestUploadResponse` it was wrong on three counts, all verified verbatim against the
standard:
| download (Table 443) | upload (Table 448)
constrains | TransferData *requests* | TransferData *responses*
reports the server's | *receive* buffer | *send* buffer
must accept full len | the *server* | the *client*
pad bytes forbidden | yes, explicitly | clause absent
Table 443: "to include in each TransferData request message from the client ... allows the
client to adapt to the receive buffer size of the server ... A server is required to accept
transferData requests that are equal in length to its reported maxNumberOfBlockLength".
Table 448: "shall be included in each TransferData positive response message from the server
... allows the client to adapt to the send buffer size of the server ... A client is
required to accept transferData responses that are equal in length to the reported
maxNumberOfBlockLength".
So the old text pointed an upload user at the wrong message type, the wrong buffer, and the
wrong party, and carried a pad-byte prohibition that Table 448 does not state -- reasonably,
since nothing on the upload path is written to server memory. `$verb` had been threaded
through the request side of this macro; the response side got nothing.
Both now come from a `block_length_accessor_doc` parameter, and the upload one names the
download accessor explicitly so the difference is visible from the place someone would
confuse them.
One thing this exposed: my first version put the corrected prose on the **private** field,
where rustdoc does not publish it. The correction was invisible to exactly the audience it
was for. Checking the generated HTML rather than the source is what caught it -- the field
doc is now a one-liner pointing at the accessor, and the accessor carries the semantics.
The `docs: correct claims that were not true` commit on this branch renames `DynamicallyDefinedDataIdentifier` to `DynamicallyDefineDataIdentifier`, which is what ISO 14229-1:2020 Table 23 calls service 0x2C -- "Define", not "Defined". The SID conversion table added in #51 spells it the old way, so it stops compiling here. Worth noting that this is the table doing its job: it is written out independently of the conversions precisely so that a change to the enum has to be acknowledged rather than silently agreed with. Kept as its own commit rather than folded into the renaming commit, so the dependency between the two is visible. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
README.md is this crate's front page *and* its crate-level documentation, via
`#![doc = include_str!(concat!(env!("CARGO_MANIFEST_DIR"), "/README.md"))]`. So
`[`Request`]` in it is a rustdoc intra-doc link, not prose.
mdformat escapes `[` and `]`, which turns every one of those into literal text.
Measured on the generated HTML rather than assumed -- links from the crate index:
unescaped after mdformat
Request 3 1
Response 3 1
UdsServiceType 2 1
The hook has been configured in this repo for a while but was never enforced;
the reusable CI workflow adopted in #48 runs pre-commit, which is what surfaced
it. It first showed up as a plain formatting failure on this branch, because the
documentation commit here deliberately unescaped six bracket pairs to make those
links work -- so the hook and the commit want opposite things.
Excluding one file rather than reverting the unescaping: the links are worth more
than uniform prose formatting, and every other markdown file in the repo is still
formatted. The alternative -- writing every link with an explicit
`[`Error`](crate::Error)` target, which mdformat leaves alone -- is what an earlier
commit on this branch removed as redundant.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Request::is_positive_response_suppressed now returns Option<bool> instead of bool, so a vendor-specific service or a malformed frame missing its sub-function byte reports None (unknown) rather than the indistinguishable Some(false). The match is exhaustive over every Request variant, with no wildcard arm, so a new variant added later is a compile error rather than a silently wrong answer. Also fixes the one other call site (read_dtc_information's SPRMIB round-trip test) for the new return type.
…13 fact
Also fix a CHANGELOG wording that read as contradicting has_sub_function's
own doc comment ("the one enumerated variant" vs. three None-reporting
variants), and add a test: an assertion in
every_modeled_variant_agrees_with_the_sub_function_table that a modeled
sub-function frame actually sets bit 7, so the test fails pointing at a
missing SPRMIB bit rather than at the sub-function table.
Code review findings on the SPRMIB reporting change:
- src/service.rs: the AccessTimingParameters bullet asserted what the
ISO 14229-1:2013 edition said about a sub-function for 0x83. That
claim has no source in this repo or the adjacent spec collection.
Reworded to state only that no earlier-edition fact is drawn from,
without asserting what that fact is.
- CHANGELOG.md: "the one enumerated variant that reports None" reads as
"the one enum variant", contradicting the three-variant None list
above it. Reworded to the true distinction: the only variant with an
ISO-assigned request SID that reports None.
- src/request.rs: modeled_frames()'s doc comment states every
sub-function frame sets SPRMIB, but nothing checked it, so a future
frame missing the bit would fail with a message blaming the
sub-function table instead of the frame.
Contributor
|
Tick the box to add this pull request to the merge queue (same as
|
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## refactor/encapsulation-and-serde #55 +/- ##
====================================================================
+ Coverage 89.39% 90.00% +0.60%
====================================================================
Files 31 31
Lines 5432 5523 +91
====================================================================
+ Hits 4856 4971 +115
+ Misses 576 552 -24 |
… 0.1.0 Moves the const declaration flagged by clippy::items_after_statements to the top of a_session_layer_can_tell_not_suppressed_from_cannot_say, fixing the clippy::pedantic CI gate. Also removes the stale 0.1.0->0.2.0 bump note (the whole Unreleased body is folding into one 0.1.0 release) and deletes the LengthFormatIdentifier CHANGELOG entry, which described a low-nibble-zeroing behavior change that never actually happened relative to v0.0.2 -- the low nibble was already zeroed on re-encode before this branch.
JustinKovacich
approved these changes
Aug 10, 2026
JustinKovacich
left a comment
JustinKovacich
left a comment
Contributor
There was a problem hiding this comment.
Reviewed the incremental diff over #54 (opus review pass). Core change verified: is_positive_response_suppressed() bool -> Option with a new const fn UdsServiceType::has_sub_function() covering all 26 services; unknown/indeterminate SPRMIB now reports None instead of collapsing to false, definite set/clear still Some(true)/Some(false); consistent across decode/encode/public paths; matches ISO 14229-1 Table 11. Tests are non-vacuous (every known/unknown distinction covered; fuzz target exercises the method on every decoded frame) and #53's spec_conformance oracle is untouched. The clippy::items_after_statements lint (tests/public_api.rs) is fixed and the stale/incorrect CHANGELOG entries (0.1.0->0.2.0 note; backwards LengthFormatIdentifier claim) are cleaned up in 478f3a5; all CI now green. Integration to main will be a single merge of this tip per the agreed strategy.
JustinKovacich
changed the base branch from
refactor/encapsulation-and-serde
to
main
August 10, 2026 18:57
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
2 participants
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.
Commit Message Details
Request::is_positive_response_suppressed()returnedbool, and its match fell through_ => falsefor every service it did not name. EveryRequest::Othertherefore reported "notsuppressed" — which is indistinguishable from the correct answer for a service that genuinely has
no sub-function. One is knowledge, the other is absence of knowledge, and the type could not tell
them apart.
The consequence is on the wire rather than merely in the type. ISO 14229-2 clause 10.3 gates
tP3_Client_Physon this bit, and whether a response is expected decides whethertP_Clientstarts at all. So a vendor-specific service sent fire-and-forget read as response-expected: the
timer started, timed out, and ISO 14229-2 Table 9 retried the request twice — three transmissions
of a request the application never wanted answered.
Breaking: the method now returns
Option<bool>.Nonemeans the question has no answer,never that the crate declined to look. It arises in exactly three situations: the service
identifier is not one ISO 14229-1 assigns (the vendor-specific case); the service has a
sub-function but the frame carries no payload byte to read it from; or the identifier is
0x83,whose service the 2020 edition withdrew. Callers who cannot supply the answer themselves can treat
Noneas response-expected, which is the old behaviour — but that choice is now explicit ratherthan made for them.
New:
UdsServiceType::has_sub_functionA
pub const fnreturningOption<bool>, stating whether ISO 14229-1 gives each service asub-function byte — and therefore whether its request can carry a SPRMIB at all. This is a fact
about the standard rather than about this crate's coverage, so it is answered for all 26 services
the 2020 edition defines and not only the 16 modelled here. A
0x2CDynamicallyDefineDataIdentifierrequest consequently reports its SPRMIB correctly even though itdecodes to
Request::Other.Every value in that table was verified against the ISO 14229-1:2020 text, service by service,
rather than inferred from this crate's existing behaviour. Four entries were checked specifically
because their leading byte reads sub-function-like but is an ordinary parameter:
0x2A(
transmissionMode),0x2F(controlOptionRecord),0x38(modeOfOperation), and0x84. Finaltally: 12 services with a sub-function, 14 without, 3 variants with no answer.
AccessTimingParameters(0x83) is the only variant with an ISO-assigned request SID that reportsNone. The 2020 edition withdrew the service, and the variant is retained solely so a 2013-era0x83/0xC3byte round-trips as itself — so answering from an edition this table is not drawnfrom would be stating something it cannot support.
Both matches are exhaustive, deliberately
Neither
has_sub_functionnoris_positive_response_suppressedhas a wildcard arm. Replacing_ => falsewith_ => Some(false)would have fixed today'sOtherbug while leaving theidentical trap for the next variant added; spelling out all 29 service variants and all 17 request
variants is what actually closes it. Both enums are
#[non_exhaustive], which constrains onlydownstream crates — inside the crate the match must be exhaustive, so adding a service later is a
compile error until its sub-function fact is stated.
Why this is
Optionwhenallowed_nack_codesis notRequest::allowed_nack_codesreports "unknown" as an empty slice and is unchanged. Thedistinguishing test is whether the sentinel collides with a real answer: no service has an empty
table of listed codes, so
&[]is unambiguous.falsehas no such property — fourteen servicesgenuinely have no sub-function — so a bare
boolcould not distinguish them from a service thecrate knows nothing about.
One asymmetry this leaves, worth naming:
has_sub_functioncovers all 26 standard services, whileallowed_nack_codesreturns&[]for all ofOtherincluding services ISO does tabulate codesfor. The crate will therefore say "0x2C has a sub-function" and "I know no NRCs for 0x2C" in the
same breath. That is defensible — one fact is static, the other needs a modelled type — and is
already caveated in
allowed_nack_codes' own rustdoc, but it is the reason to revisit that methodif it is ever extended.
Issue URL
No associated issue — this came out of designing
uds_session(ISO 14229-2) against this crate,where the session layer needs a reliable answer to "is the positive response suppressed for this
request?" to gate
tP3_Client_Phys. Needs the No Issue label, or an issue filedretrospectively if that is preferred.
Testing
Both new guards were verified to be falsifiable by deliberately breaking the code and
observing the specific failure, then reverting. A guard never seen to fail is worth nothing:
every_service_reports_the_sub_function_iso_gives_it— theSERVICEStable is written out byhand rather than derived from
has_sub_function, so a mistyped arm fails against the tableinstead of agreeing with itself. Moving
TesterPresentbetween arms was confirmed to fail it.every_modeled_variant_agrees_with_the_sub_function_table— tieshas_sub_functionand therequest dispatch together, so the two copies of this ISO fact cannot drift apart. Its frames set
the SPRMIB on every service that has one, which is load-bearing: with those bits clear every
service answers
Some(false)and a flipped table entry would pass unnoticed. The sameTesterPresentbreak was confirmed to fail it, in addition to the table test.Other coverage added: five cases pinning the
Otherarm, which previously had none (SPRMIB set,SPRMIB clear, a no-sub-function service, an unassigned SID, and an empty payload); a
tests/public_api.rstest reaching all three states from outside the crate, where#[non_exhaustive]applies as it does for a real consumer; andfuzz_request_decodenow calls themethod on every frame that decodes, since it indexes into an unmodelled service's payload — via
data.first(), neverdata[0].The sixteen frames previously inlined in
allowed_nack_codes_dispatches_for_every_modeled_variantare now a shared
modeled_frames()helper used by both tests, so the two cannot diverge. Settingbit 7 on eight of them changes neither which service each decodes to nor its NRC table, which that
test re-confirms.
Verification at
66cf4e0cargo testpublic_api+ 3spec_conformance+ 6 doctestscargo test --no-default-featurescargo test --all-featuresopenapi_schema+ 19public_api+ 3 + 6cargo build --no-default-featurescargo clippy --all-targets --all-features -- -D warningscargo fmt --checkcargo doc --no-deps --all-featurespre-commit run --all-files