diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 74997f07..a1756fac 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -126,6 +126,7 @@ jobs: --strict crates/uds_on_ip --strict crates/simple_doip --strict crates/uds_session + --strict crates/uds_protocol # =================================================================================== # Fuzz Tests (hand-rolled: the reusable workflow's fuzz-tests job cannot reach these) diff --git a/crates/uds_protocol/Cargo.toml b/crates/uds_protocol/Cargo.toml index 4d58908d..f8407bc8 100644 --- a/crates/uds_protocol/Cargo.toml +++ b/crates/uds_protocol/Cargo.toml @@ -32,10 +32,8 @@ serde = ["dep:serde"] # # `utoipa` additionally implies `serde`, because a `ToSchema` describes this crate's *serde* # representation: several types serialize as a single protocol byte rather than as their Rust -# shape, and their schemas are written to match. A `utoipa`-without-`serde` build would therefore -# publish a schema for a wire format that build cannot produce. It also cost real complexity — -# every type reachable only through a serde repr needed a second cfg predicate and a -# `allow(dead_code)` to stay compilable in a configuration nobody wants. +# shape, and their schemas are written to match. A `utoipa`-without-`serde` build would +# publish a schema for a wire format that build cannot produce. utoipa = ["std", "serde", "dep:utoipa"] clap = ["std", "dep:clap"] diff --git a/crates/uds_protocol/README.md b/crates/uds_protocol/README.md index 2b8b228f..90e2fd00 100644 --- a/crates/uds_protocol/README.md +++ b/crates/uds_protocol/README.md @@ -25,19 +25,22 @@ other crate in the stack. (ISO 14229-2) sits beside it in the stack but does not depend on it — the session layer never reads a message's content. - [`uds_services`](https://github.com/luminartech/uds_stack/tree/main/crates/uds_services) - (ISO 14229-1's behaviour, to this crate's format) and + (ISO 14229-1's behavior, to this crate's format) and [`uds_on_ip`](https://github.com/luminartech/uds_stack/tree/main/crates/uds_on_ip) (ISO 14229-5) both depend on this crate directly: they dispatch and transport the message types this crate encodes and decodes. -## What this crate does not do +## Scope -It is a synchronous, allocation-free codec, and nothing more. It owns no -sockets, buffers, or async runtime; it does not dispatch a request to a -handler, choose a negative response code, or hold any session state. Driving -the I/O loop, over `DoIP`, `UDSonIP`, ISO-TP, or anything else, is a caller's -job — see [`uds_services`](https://github.com/luminartech/uds_stack/tree/main/crates/uds_services) -for the layer that owns dispatch. +It is a synchronous, allocation-free codec. The sockets, buffers and async +runtime are the caller's, as is driving the I/O loop over `DoIP`, `UDSonIP`, +ISO-TP or anything else. Dispatching a request to a handler and choosing a +negative response code are +[`uds_services`](https://github.com/luminartech/uds_stack/tree/main/crates/uds_services)'s, +and session state is +[`uds_session`](https://github.com/luminartech/uds_stack/tree/main/crates/uds_session)'s. + +The services it encodes and decodes: | Service Name | Request SID | Response SID | Support | | ----------------------------------- | ----------- | ------------ | ------- | @@ -149,14 +152,14 @@ A checkmark in the table above means the service decodes into a typed [`Request`]/[`Response`] variant; [`NegativeResponse`] is typed as well. Every other service [`UdsServiceType`] enumerates decodes into [`Request::Other`] / [`Response::Other`], carrying the service type and the raw payload bytes for -pass-through — an unmodelled service is handed to you intact rather than +pass-through — an unmodeled service is handed to you intact rather than rejected. ## Wire codec dependency `uds_protocol` builds its byte-level decoding on top of the [`automotive-wire-codec`](https://crates.io/crates/automotive-wire-codec) crate, and re-exports its `Incomplete`, `TrailingBytes` and `InvalidWidth` types and its codec -traits (`Encode`, `Decode`, `DecodeIter`) at the crate root. These types are intentionally part of `uds_protocol`'s public API: +traits (`Encode`, `Decode`, `DecodeIter`) at the crate root. These types are part of `uds_protocol`'s public API: they are shared across the Luminar automotive protocol crates so that callers handling multiple protocols see one consistent short-read/trailing-data error shape. Because of this, a semver-major release of `automotive-wire-codec` is a breaking change for `uds_protocol` as well. @@ -177,12 +180,12 @@ Pull requests, bug reports and questions are welcome — see Security reports go through GitHub's private vulnerability reporting; see [`SECURITY.md`](https://github.com/luminartech/uds_stack/blob/main/SECURITY.md). -## Licence +## License Licensed under either of [MIT](LICENSE-MIT) or [Apache-2.0](LICENSE-APACHE) at your option. Both files are the workspace's, shared by every crate here. -Neither licence grants any right in ISO 14229-1 itself, which remains ISO's — +Neither license grants any right in ISO 14229-1 itself, which remains ISO's — see [Relationship to the standards](https://github.com/luminartech/uds_stack#relationship-to-the-standards) in the workspace README. diff --git a/crates/uds_protocol/fuzz/fuzz_targets/fuzz_request_decode.rs b/crates/uds_protocol/fuzz/fuzz_targets/fuzz_request_decode.rs index f13a2ed8..2239e8f9 100644 --- a/crates/uds_protocol/fuzz/fuzz_targets/fuzz_request_decode.rs +++ b/crates/uds_protocol/fuzz/fuzz_targets/fuzz_request_decode.rs @@ -1,15 +1,14 @@ // Fuzz the Request decode path with arbitrary bytes. -// Any panic here indicates a bug — decode should return Err, not crash. +// Any panic here is a bug: decode returns Err rather than crashing. #![no_main] use libfuzzer_sys::fuzz_target; use uds_protocol::{Decode, Request}; fuzz_target!(|data: &[u8]| { - // Attempt to decode arbitrary bytes as a UDS request. - // We don't care about the result — only that it doesn't panic. + // Attempt to decode arbitrary bytes as a UDS request. The result is irrelevant; the + // target checks only that decoding does not panic. // Reporting SPRMIB indexes into the payload of an unmodeled service, so run it on every - // frame that decodes. It is panic-free by construction today -- `data.first()`, not - // `data[0]` -- which is exactly why it is worth pinning before an edit changes that. + // frame that decodes. if let Ok((request, _)) = Request::decode(data) { let _ = request.is_positive_response_suppressed(); } diff --git a/crates/uds_protocol/fuzz/fuzz_targets/fuzz_response_decode.rs b/crates/uds_protocol/fuzz/fuzz_targets/fuzz_response_decode.rs index a88fb6e6..edb9d288 100644 --- a/crates/uds_protocol/fuzz/fuzz_targets/fuzz_response_decode.rs +++ b/crates/uds_protocol/fuzz/fuzz_targets/fuzz_response_decode.rs @@ -1,11 +1,11 @@ // Fuzz the Response decode path with arbitrary bytes. -// Any panic here indicates a bug — decode should return Err, not crash. +// Any panic here is a bug: decode returns Err rather than crashing. #![no_main] use libfuzzer_sys::fuzz_target; use uds_protocol::{Decode, Response}; fuzz_target!(|data: &[u8]| { - // Attempt to decode arbitrary bytes as a UDS response. - // We don't care about the result — only that it doesn't panic. + // Attempt to decode arbitrary bytes as a UDS response. The result is irrelevant; the + // target checks only that decoding does not panic. let _ = Response::decode(data); }); diff --git a/crates/uds_protocol/fuzz/fuzz_targets/fuzz_roundtrip.rs b/crates/uds_protocol/fuzz/fuzz_targets/fuzz_roundtrip.rs index ca3147e4..d2df60f4 100644 --- a/crates/uds_protocol/fuzz/fuzz_targets/fuzz_roundtrip.rs +++ b/crates/uds_protocol/fuzz/fuzz_targets/fuzz_roundtrip.rs @@ -1,16 +1,16 @@ -// Fuzz encode→decode roundtrip: decode arbitrary bytes into a Request, -// re-encode it, and verify the encoding is idempotent at the byte level. +// Fuzz the encode/decode round trip: decode arbitrary bytes into a Request, +// re-encode it, and check that the encoding is idempotent at the byte level. // -// `Request` does not implement `PartialEq`, so we compare the canonical wire -// bytes rather than the decoded values: encoding a decoded message, decoding -// those bytes again, and re-encoding must yield identical bytes. +// The comparison is on the canonical wire bytes rather than the decoded values: +// encoding a decoded message, decoding those bytes again, and re-encoding must +// yield identical bytes. #![no_main] use automotive_wire_codec::SliceSink; use libfuzzer_sys::fuzz_target; use uds_protocol::{Decode, Encode, Request}; fuzz_target!(|data: &[u8]| { - // Only proceed if we can decode the input. + // Only a decodable input is checked. let Ok((request, _rest)) = Request::decode(data) else { return; }; diff --git a/crates/uds_protocol/src/dtc/ext_data.rs b/crates/uds_protocol/src/dtc/ext_data.rs index 38c94762..497d34a9 100644 --- a/crates/uds_protocol/src/dtc/ext_data.rs +++ b/crates/uds_protocol/src/dtc/ext_data.rs @@ -1,8 +1,7 @@ use crate::{Decode, Encode, Error, Incomplete}; use automotive_wire_codec::write_u8; -/// The `DtcExtDataRecordNumber` is used in the request message to get a stored -/// `DTCExtDataRecord` It's used to specify the type of `DTCExtDataRecord` to be reported. +/// Which stored `DTCExtDataRecord` a `ReadDTCInformation` request asks for. #[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))] #[cfg_attr(feature = "serde", serde(from = "u8", into = "u8"))] #[derive(Clone, Copy, Debug, Eq, PartialEq)] @@ -17,25 +16,22 @@ pub enum DtcExtDataRecordNumber { #[non_exhaustive] VehicleManufacturerSpecific(u8), - /// Requests the server to report regulated emissions OBD stored - /// `DTCExtendedDataRecords`. The values are specified in SAE J1979-DA. + /// A regulated-emissions OBD extended data record, numbered as SAE J1979-DA defines. /// /// 0x90-0x9F #[non_exhaustive] RegulatedEmissionsObdDtcExtDataRecords(u8), - /// The `DtcExtDataRecordNumber` parameter is used to specify the DTC number of the - /// `DTCExtendedData` record to be reported. + /// Regulated stored `DTCExtendedDataRecord`s. /// /// 0xA0-0xEF #[non_exhaustive] RegulatedDtcExtDataRecords(u8), - /// Requests the server to report all regulated emissions OBD stored - /// `DTCExtendedDataRecords`. + /// Every regulated-emissions OBD extended data record the server stores. AllRegulatedEmissionsObdDtcExtDataRecords, - /// Requests the server to report all stored `DTCExtendedDataRecords` + /// Every extended data record the server stores. AllDtcExtDataRecords, } @@ -112,7 +108,6 @@ impl<'a> Decode<'a> for DtcExtDataRecordNumber { } } -// tests #[cfg(test)] mod tests { use super::*; diff --git a/crates/uds_protocol/src/dtc/snapshot.rs b/crates/uds_protocol/src/dtc/snapshot.rs index 73b64b73..667718df 100644 --- a/crates/uds_protocol/src/dtc/snapshot.rs +++ b/crates/uds_protocol/src/dtc/snapshot.rs @@ -22,8 +22,8 @@ pub enum DtcSnapshotRecordNumber { impl DtcSnapshotRecordNumber { /// Create a `DtcSnapshotRecordNumber`, classifying the raw byte into `Reserved` - /// (`0x00`/`0xF0`), `All` (`0xFF`), or `Number`. Every byte is accepted (decoding is - /// deliberately liberal); no value is rejected. + /// (`0x00`/`0xF0`), `All` (`0xFF`), or `Number`. Every byte is accepted, as it is on + /// decode. #[must_use] pub const fn new(record_number: u8) -> Self { match record_number { diff --git a/crates/uds_protocol/src/dtc/status.rs b/crates/uds_protocol/src/dtc/status.rs index 14494140..5aa7f6c9 100644 --- a/crates/uds_protocol/src/dtc/status.rs +++ b/crates/uds_protocol/src/dtc/status.rs @@ -3,17 +3,12 @@ use bitmask_enum::bitmask; use crate::{Decode, DecodeIter, Encode, Error, Incomplete}; use automotive_wire_codec::{write_bytes, write_u8}; -/// Bit-packed DTC status information used by the `ReadDTCInformation` service +/// The one-byte DTC status mask of the `ReadDTCInformation` service (ISO 14229-1:2020 +/// Annex D.2). /// -/// `DtcStatusMask` (1 byte) -/// 8 DTC status bits. Refer to D.2 -/// A DTC status matches the mask if any one of the DTCs actual status bits is set to `1` -/// and the corresponding on in the mask is set to 1 -/// if( `DtcStatusMask` & `DTCStatus` = !0) is a match -/// -/// Server note: -/// If the mask uses bits that the server does not support, -/// the server shall process the bits it does support and ignore the rest +/// A DTC matches the mask when any status bit set in the DTC is also set in the mask, that +/// is when `DtcStatusMask & DTCStatus != 0`. A server processes the mask bits it supports +/// and ignores the rest. /// /// ``` /// use uds_protocol::{DtcStatusMask, ReadDtcInfoSubFunction}; @@ -68,15 +63,13 @@ pub enum DtcStatusMask { /// * 1 - Test failed during the current operation cycle PendingDtc, - /// Indicates whether a malfunction was detected enough times to warrant the DTC being - /// stored in long term memory. This doesn't mean that the DTC failure is present at the - /// time of the request. Aging threshold for clearing itself depends on the vehicle - /// manufacturer or OBD regulations + /// Whether the malfunction has been seen often enough for the DTC to be stored in + /// long-term memory. A confirmed DTC need not be failing now; when it ages out is the + /// vehicle manufacturer's or the OBD regulations' to set. /// /// Bit state definition: - /// * 0 - DTC has **never been confirmed** since last `ClearDiagnosticInformation`, or - /// after aging criteria have been met - /// * 1 - DTC has been confirmed at least once + /// * 0 - Not confirmed since the last `ClearDiagnosticInformation`, or aged out since + /// * 1 - Confirmed at least once ConfirmedDtc, /// Indicates whether a test has run and completed since last @@ -107,8 +100,8 @@ pub enum DtcStatusMask { /// * 1 - Test has **not** run to completion during the current operation cycle TestNotCompletedThisOperationCycle, - /// Shall report the status of any warning indicators associated with a certain DTC. - /// Warning outputs may consist of indicator lamp(s), displayed text information, etc. + /// Whether the server is asking for a warning indicator tied to this DTC, such as a + /// lamp or a displayed message, to be on (Table D.10). /// /// Bit state definition: /// * 0 - Server is **not** requesting a warningIndicator to be active @@ -140,9 +133,7 @@ impl<'a> Decode<'a> for DtcStatusMask { } } -/// Specifies the format of the DTC reported by the server. -/// -/// A given server shall only support one `DtcFormatIdentifier`. +/// The format of the DTCs a server reports; each server uses exactly one. #[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))] #[cfg_attr(feature = "serde", serde(from = "u8", into = "u8"))] #[derive(Clone, Copy, Debug, Eq, PartialEq)] @@ -201,9 +192,7 @@ impl DtcFormatIdentifier { } impl PartialEq for DtcFormatIdentifier { - /// Wire equality: compares the byte this identifier encodes to. Variant equality is not - /// the same thing -- `IsoSaeReserved(0x01)` and `Iso14229_1DtcFormat` both encode - /// `0x01` but are different variants, so use this when the wire byte is what matters. + /// Wire equality: compares the byte this identifier encodes to. fn eq(&self, other: &u8) -> bool { self.value() == *other } @@ -217,7 +206,7 @@ impl From for u8 { DtcFormatIdentifier::SaeJ1939_73DtcFormat => 0x02, DtcFormatIdentifier::Iso11992_4DtcFormat => 0x03, DtcFormatIdentifier::SaeJ2012DaDtcFormat04 => 0x04, - DtcFormatIdentifier::IsoSaeReserved(value) => value, // Default value for reserved + DtcFormatIdentifier::IsoSaeReserved(value) => value, } } } @@ -256,13 +245,9 @@ impl DtcRecord { /// What it means depends on the server's [`DtcFormatIdentifier`]: ISO 14229-1 itself /// specifies no decoding method for the three bytes (clause 12.3.2.3), deferring to /// whichever of SAE J2012-DA, ISO 11992-4, SAE J1939-73 or ISO 15031-6 the format - /// identifier names. - /// - /// Annex D.1 Table D.1 does assign meaning, but to whole 3-byte `groupOfDTC` *values*, - /// not to this byte in isolation — and its powertrain/chassis/body/network rows are - /// explicitly "to be determined by vehicle manufacturer". The one byte-level assignment - /// it makes is to the *low* byte: for `0xFFFF00`-`0xFFFFFE` that byte is a - /// [`FunctionalGroupIdentifier`]. + /// identifier names. Annex D.1 Table D.1 assigns meaning to whole 3-byte `groupOfDTC` + /// values, not to this byte, and its one byte-level assignment is to the *low* byte: + /// for `0xFFFF00`-`0xFFFFFE` that byte is a [`FunctionalGroupIdentifier`]. #[must_use] pub const fn high_byte(&self) -> u8 { self.high_byte @@ -278,9 +263,8 @@ impl DtcRecord { /// The least significant of the three DTC bytes. /// /// In SAE J2012-DA format this is the failure type byte, and for a `groupOfDTC` in - /// `0xFFFF00`-`0xFFFFFE` it is a - /// [`FunctionalGroupIdentifier`] (ISO 14229-1:2020 Annex - /// D.1 Table D.1). Neither reading is universal — see [`high_byte`](Self::high_byte). + /// `0xFFFF00`-`0xFFFFFE` it is a [`FunctionalGroupIdentifier`] (ISO 14229-1:2020 Annex + /// D.1 Table D.1). Neither reading is universal; see [`high_byte`](Self::high_byte). #[must_use] pub const fn low_byte(&self) -> u8 { self.low_byte @@ -290,12 +274,8 @@ impl DtcRecord { impl TryFrom for DtcRecord { type Error = Error; - /// A DTC is three bytes, so only the low 24 bits of a `u32` are a valid DTC. - /// - /// This is `TryFrom` rather than `From` because masking the top byte away silently - /// would make `0xFF01_0203` and `0x0001_0203` the same record — a caller who has a DTC - /// in a `u32` from elsewhere and one byte too many would get a wrong DTC with no - /// signal. + /// A DTC is three bytes, so only the low 24 bits of a `u32` are a valid DTC. Fallible + /// rather than masking, so `0xFF01_0203` and `0x0001_0203` do not become one record. /// /// # Errors /// Returns [`Error::InvalidDtcRecord`] if `value` exceeds `0x00FF_FFFF`. @@ -315,8 +295,7 @@ impl TryFrom for DtcRecord { impl DtcRecord { /// The three DTC bytes as the low 24 bits of a `u32`, big-endian. /// - /// `const`, unlike `u32::from(record)`: trait methods are not callable in a `const fn` - /// on stable, so a `const` table of DTC values needs this. + /// `const`, unlike `u32::from(record)`, so a `const` table of DTC values can use it. #[must_use] pub const fn to_u32(&self) -> u32 { ((self.high_byte as u32) << 16) @@ -380,13 +359,8 @@ impl<'a> DecodeIter<'a> for DtcRecord { } } -/// Used to distinguish commands sent by the test equipment between different functional -/// system groups within an electrical architecture which consists of many different -/// servers. -/// -/// For the purpose of: -/// * Requesting DTC status from a vehicle -/// * Clearing DTC information in the vehicle +/// Addresses one functional system group, such as emissions or safety, across the servers +/// of a vehicle, when requesting DTC status or clearing DTC information. #[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))] #[cfg_attr(feature = "serde", serde(from = "u8", into = "u8"))] #[derive(Clone, Copy, Debug, Eq, PartialEq)] @@ -492,9 +466,9 @@ pub enum DtcSeverityMask { /// Unclassified DtcClass0, - /// Matches GTR module B Class A definition Malfunction is Class A when On-Board - /// Diagnostic (OBD) threshold limits (OTL) are assumed to be exceeded It is accepted - /// that the emissions may not be above the OTLs when this class of malfunction occurs + /// GTR module B Class A: a malfunction treated as exceeding the on-board diagnostic + /// threshold limits (OTLs), whether or not emissions are actually above them + /// (Table D.13). DtcClass1, /// Matches GTR module B Class B1 definition @@ -510,7 +484,7 @@ pub enum DtcSeverityMask { /// MO MaintenanceOnly = 0b0010_0000, // bit 5 - /// Indicates to the failure that a check of the vehicle is required at the next halt + /// The failure calls for a check of the vehicle at its next halt (Table D.12). /// /// CHKANH CheckAtNextHalt = 0b0100_0000, // bit 6 @@ -570,19 +544,17 @@ impl<'a> Decode<'a> for DtcSeverityMask { pub struct DtcStoredDataRecordNumber(u8); impl DtcStoredDataRecordNumber { - /// Create a `DtcStoredDataRecordNumber` from a raw byte. Every byte is accepted. - /// - /// Total, like [`DtcSnapshotRecordNumber::new`](crate::DtcSnapshotRecordNumber::new) - /// and [`DtcExtDataRecordNumber::new`](crate::DtcExtDataRecordNumber::new), because - /// decoding is deliberately liberal and `From` already accepted anything — so a - /// fallible `new` promised a guarantee the type did not actually hold. Use - /// [`is_reserved`](Self::is_reserved) when you need the check. + /// Create a `DtcStoredDataRecordNumber` from a raw byte. Every byte is accepted, as by + /// [`DtcSnapshotRecordNumber::new`](crate::DtcSnapshotRecordNumber::new) and + /// [`DtcExtDataRecordNumber::new`](crate::DtcExtDataRecordNumber::new), because + /// decoding accepts every byte too; use [`is_reserved`](Self::is_reserved) for the + /// check. /// /// Clause 12.3.3.2 reserves `0x00` for legislated purposes, makes `0x01`-`0xFE` /// available for vehicle-manufacturer use, and gives `0xFF` the meaning "report all - /// stored records". Note that `0xF0` is *not* reserved here — that belongs to the - /// [`DtcSnapshotRecordNumber`](crate::DtcSnapshotRecordNumber) space, which the spec - /// says does not share an address space with this one. + /// stored records". `0xF0` is *not* reserved here; it belongs to the + /// [`DtcSnapshotRecordNumber`](crate::DtcSnapshotRecordNumber) space, which does not + /// share an address space with this one. #[must_use] pub const fn new(record_number: u8) -> Self { Self(record_number) @@ -660,12 +632,10 @@ mod encode_param_tests { } #[test] - fn stored_data_record_number_accepts_every_byte_and_flags_the_reserved_one() { - // ISO 14229-1:2020 clause 12.3.3.2 reserves only 0x00 for this parameter: - // "DTCStoredData records in range of 0x01 through 0xFE shall be available for - // vehicle manufacturer specific usage", and 0xFF requests all records. 0xF0 belongs - // to the *snapshot* record-number space and used to be rejected here by a check - // copied from there. + fn stored_data_record_number_is_total() { + // ISO 14229-1:2020 clause 12.3.3.2 reserves only 0x00 for this parameter: 0x01 to + // 0xFE are the vehicle manufacturer's, and 0xFF requests all records. 0xF0 belongs + // to the *snapshot* record-number space and is an ordinary value here. for byte in [0x00u8, 0x01, 0xF0, 0xFE, 0xFF] { let number = DtcStoredDataRecordNumber::new(byte); assert_eq!(number.value(), byte); @@ -674,8 +644,8 @@ mod encode_param_tests { assert_eq!(number.is_reserved(), byte == 0x00, "for {byte:#04X}"); } - // Decoding is liberal for the same reason `new` is total: a response from a foreign - // implementation must still be inspectable. + // Decoding accepts every byte for the same reason `new` is total: a response from a + // foreign implementation stays inspectable. let (decoded, _) = ::decode(&[0x00]).unwrap(); assert!(decoded.is_reserved()); } @@ -706,7 +676,7 @@ mod encode_param_tests { #[test] fn functional_group_identifier_value_does_not_panic_on_reserved() { - // Regression: value() previously called todo!() for carried-byte variants. + // `value()` returns the carried byte for the byte-carrying variants. let g = FunctionalGroupIdentifier::from(0x10); // -> IsoSaeReserved(0x10) assert_eq!(g.value(), 0x10); let g2 = FunctionalGroupIdentifier::from(0xD5); // -> LegislativeSystemGroup(0xD5) @@ -720,8 +690,7 @@ mod dtc_status_tests { #[test] fn dtc_record_exposes_its_three_wire_bytes() { - // A decoded DtcRecord has to be inspectable byte-wise: D.1 assigns meaning to the - // high byte (system group) separately from the middle and low bytes. + // A decoded DtcRecord is inspectable byte-wise. let record = DtcRecord::try_from(0x12_3456).unwrap(); assert_eq!(record.high_byte(), 0x12); assert_eq!(record.middle_byte(), 0x34); diff --git a/crates/uds_protocol/src/error.rs b/crates/uds_protocol/src/error.rs index 4e59a059..81446a61 100644 --- a/crates/uds_protocol/src/error.rs +++ b/crates/uds_protocol/src/error.rs @@ -73,7 +73,7 @@ pub enum Error { /// A `u32` did not fit the three bytes of a DTC (i.e. exceeded `0x00FF_FFFF`). #[error("Invalid DTC record: {0:#010X} does not fit three bytes")] InvalidDtcRecord(u32), - /// The encryption or compression method byte is not recognised. + /// The encryption or compression method byte is not recognized. #[error("Invalid Encryption/Compression Method: {0}")] InvalidEncryptionCompressionMethod(u8), /// The `RequestFileTransfer` `modeOfOperation` byte is not valid. @@ -94,11 +94,9 @@ pub enum Error { } impl Error { - /// The [`NegativeResponseCode`] a server should return for this error. - /// - /// A server decodes an inbound request, and on failure has to answer with a negative - /// response. This is that mapping, so callers do not have to re-derive it against a - /// `#[non_exhaustive]` enum they cannot match exhaustively: + /// The [`NegativeResponseCode`] a server returns for this error, so a server that + /// failed to decode a request does not re-derive the mapping against a + /// `#[non_exhaustive]` enum: /// /// ``` /// use uds_protocol::{Decode, NegativeResponse, Request, UdsServiceType}; @@ -111,49 +109,24 @@ impl Error { /// ``` /// /// Returns `None` for [`Error::Write`]: a transport failure is not a protocol error, so - /// there is no NRC to send. Every other error maps to a code. - /// - /// # Classification - /// - /// This is a **default**, covering the lane ISO 14229-1 actually mandates — clause - /// 8.7.5's pseudo-code and the "shall" rows of Annex A.1. Clause 8.7.2 notes that "a - /// specific NRC is not guaranteed for all possible test pattern sequences", and several - /// outcomes in Tables 4 to 7 are specified only as "NRC = XX", so a server is free to - /// answer differently where its own tables allow: - /// - /// - **`0x13` `incorrectMessageLengthOrInvalidFormat`** — the frame itself is - /// malformed: too short, too long, or a declared width that does not fit. - /// - **`0x12` `subFunctionNotSupported`** — the *sub-function* byte is not a value this - /// service defines. - /// - /// Only two services reject a reserved sub-function at decode and so produce this code - /// themselves: `0x31` `RoutineControl` (Table 426 defines no manufacturer range, so - /// anything outside `0x01`-`0x03` is invalid) and `0x85` `ControlDTCSetting`. - /// - /// The others — `0x10`, `0x11`, `0x19`, `0x27`, `0x28`, `0x3E` — model the whole - /// `0x00..=0x7F` space, so a reserved byte decodes into an `IsoSaeReserved`-style - /// variant and round-trips unchanged rather than failing. That is deliberate: it lets a - /// server see the byte it was actually sent. **Answering `0x12` for those is the - /// server's job**, not this mapping's: match on the sub-function and use - /// [`NegativeResponseCode::SubFunctionNotSupported`] for a value you do not implement. - /// - **`0x31` `requestOutOfRange`** — a *parameter* (not a sub-function) carries a - /// value outside its permitted range. Note that `communicationType` is a parameter of - /// `CommunicationControl`, not its sub-function, so it lands here rather than on - /// `0x12`. - /// - /// Two codes this mapping deliberately never returns: + /// there is no NRC to send. Every other error maps to one of three codes, following + /// clause 8.7.5 and the "shall" rows of Annex A.1; clause 8.7.2 leaves a server free to + /// answer differently where its own tables allow. /// - /// - **`0x10` `generalReject`** — Annex A.1 says it "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 none of the per-service NRC tables, so no service's - /// `allowed_nack_codes` lists it. - /// - **`0x11` `serviceNotSupported`** — an unrecognised SID does not fail to decode; it - /// becomes [`Request::Other`](crate::Request::Other) so a server can see the byte. - /// Answering `0x11` is therefore the server's call, on a SID it does not implement. + /// - `0x13` `incorrectMessageLengthOrInvalidFormat`: the frame is malformed, too short, + /// too long, or declaring a width that does not fit. + /// - `0x12` `subFunctionNotSupported`: the sub-function byte is not a value the service + /// defines. Only `0x31` and `0x85` reject a reserved sub-function at decode; the + /// other sub-function services decode one into an `IsoSaeReserved` variant so the + /// server sees the byte, and answering `0x12` for it is the server's job. + /// - `0x31` `requestOutOfRange`: a parameter, not a sub-function, is outside its range. + /// `communicationType` is a parameter of `CommunicationControl`, so it lands here. /// - /// Every code this does return is a legal NRC to put on the wire: never - /// [`NegativeResponseCode::PositiveResponse`] and never a reserved value. + /// Never `0x10` `generalReject`, which Annex A.1 forbids as a general replacement, and + /// never `0x11` `serviceNotSupported`, since an unrecognized SID decodes to + /// [`Request::Other`](crate::Request::Other) and the server answers it. Every code + /// returned is legal on the wire: never [`NegativeResponseCode::PositiveResponse`] and + /// never a reserved value. #[must_use] pub const fn negative_response_code(&self) -> Option { match self { @@ -350,12 +323,10 @@ mod nrc_mapping_tests { #[test] fn mapping_never_produces_a_positive_or_reserved_code() { - // This is deliberately narrower than "the codes the ISO tables allow": the - // per-service tables are a floor, not a ceiling (clause 9.4 — the A.1 codes "shall - // be used in addition to" them), so table membership is not the property to assert. - // What must hold is that a decode failure never reports a positive response and - // never invents a reserved code, either of which would put an illegal byte on the - // wire. + // Narrower than "the codes the ISO tables allow": the per-service tables are a + // floor, not a ceiling (clause 9.4), so table membership is not the property to + // assert. A decode failure never reports a positive response and never invents a + // reserved code, either of which would put an illegal byte on the wire. for (err, _, why) in cases() { let nrc = err .negative_response_code() diff --git a/crates/uds_protocol/src/lib.rs b/crates/uds_protocol/src/lib.rs index c32ecb91..bf1284bc 100644 --- a/crates/uds_protocol/src/lib.rs +++ b/crates/uds_protocol/src/lib.rs @@ -1,20 +1,15 @@ #![doc = include_str!(concat!(env!("CARGO_MANIFEST_DIR"), "/README.md"))] -// Derogations from the workspace lint standard in the root Cargo.toml. Each is a -// gap to close, not a decision that the lint is wrong here. +// Derogations from the workspace lint standard in the root Cargo.toml. // -// The defensive three: a zero-copy decoder indexes borrowed slices and computes -// offsets on every path, and this crate predates the standard. 162 sites in -// production code as of this writing; closing it means `get()?` and checked -// arithmetic throughout the decode paths, which is its own change. +// A zero-copy decoder indexes borrowed slices and computes offsets on every decode +// path, so these three are allowed crate-wide rather than at each site. #![allow( clippy::indexing_slicing, clippy::arithmetic_side_effects, clippy::as_conversions )] -// Panic freedom holds in this crate's production code and is enforced there. Its -// test modules are older than the standard and use `unwrap` the way test code -// ordinarily does -- 281 sites, where a panic *is* the failure mechanism. Scoped to -// `test` so the production build stays strict. +// Test modules use `unwrap`, `expect` and `panic` as their failure mechanism. Scoped +// to `test` so the production build stays panic-free. #![cfg_attr( test, allow( @@ -143,19 +138,14 @@ mod no_std_api_tests { fn dtc_and_status_iter_roundtrip() { // 2 DTC records: (0x01,0x02,0x03, status=0x0A), (0x04,0x05,0x06, status=0x0B) // - // Deliberately not gated on `alloc`, and written with `next()` rather than - // `collect()` for that reason. While this was gated, mutating the status byte to a - // constant left `--no-default-features` fully green — so the assertion below - // existed but did not protect the config the crate targets first. + // Written with `next()` rather than `collect()` so it runs without `alloc`, the + // configuration the crate targets first. let data = [0x01, 0x02, 0x03, 0x0A, 0x04, 0x05, 0x06, 0x0B]; let mut iter = DtcAndStatusIter::new(&data); assert_eq!(iter.len(), 2); let (dtc, status) = iter.next().unwrap().unwrap(); assert_eq!(u32::from(dtc), 0x01_0203); - // The status byte matters as much as the DTC: 0x02 and 0x0A-0x0E are the most-used - // sub-functions, and nothing asserted it, so "every DTC reports status 0x00" - // passed. assert_eq!(status.bits(), 0x0A); let (dtc, status) = iter.next().unwrap().unwrap(); @@ -167,9 +157,9 @@ mod no_std_api_tests { #[test] fn fault_detection_counter_record_is_nameable_from_crate_root() { - // `DtcFaultDetectionCounterRecord` is the `Item` of `DtcFaultDetectionIter`. - // Without a crate-root path, callers can iterate but cannot name the type — no - // `Vec`, no struct field, no helper signature. This pins the re-export. + // `DtcFaultDetectionCounterRecord` is the `Item` of `DtcFaultDetectionIter`, so a + // caller needs a crate-root path to it for a `Vec`, a struct field or a helper + // signature. let data = [0x01, 0x02, 0x03, 0x2A]; let record: DtcFaultDetectionCounterRecord = DtcFaultDetectionIter::new(&data).next().unwrap().unwrap(); @@ -340,9 +330,8 @@ mod no_std_api_tests { Err(_) => panic!("both nibbles are in range"), }; - // Getting the byte back out must be const too, or a `const` dispatch table can be - // built but not read. These four had only a non-const `From`, so each of these - // lines was previously an E0015. + // Getting the byte back out is const too, or a `const` dispatch table could be + // built but not read. const DTC_U32: u32 = DTC.to_u32(); const DFI_BYTE: u8 = DFI.value(); const NRC_BYTE: u8 = NegativeResponseCode::ConditionsNotCorrect.value(); @@ -363,9 +352,8 @@ mod no_std_api_tests { #[test] fn communication_control_requests_are_const_constructible() { - // Both constructors were the crate's only non-`const` `new`s. The blocker was - // `u8::from(control_type)` in their error payload, which an inherent `const fn - // value()` on the enum removes. + // Both constructors build their error payload with the enum's `const fn value()`, + // so each is usable in a `const`. const REQ: CommunicationControlRequest = match CommunicationControlRequest::new( false, CommunicationControlType::DisableRxAndTx, diff --git a/crates/uds_protocol/src/request.rs b/crates/uds_protocol/src/request.rs index 3da023e1..4c9ee1a3 100644 --- a/crates/uds_protocol/src/request.rs +++ b/crates/uds_protocol/src/request.rs @@ -68,8 +68,8 @@ pub enum Request<'a> { /// # Remainder /// -/// The returned remainder is **always empty**. A UDS frame is not self-delimiting — its -/// length comes from the transport (ISO-TP, `DoIP`, ...), not from the message — so one +/// The returned remainder is **always empty**. A UDS frame is not self-delimiting: its +/// length comes from the transport (ISO-TP, `DoIP`, ...), not from the message, so one /// buffer is exactly one frame, and every payload is decoded with `decode_exact`. This /// means `decode` behaves as `decode_exact` despite the streaming shape of the [`Decode`] /// contract: do **not** feed it concatenated frames expecting it to consume one at a time, @@ -184,42 +184,15 @@ impl Encode for Request<'_> { impl Request<'_> { /// Whether the positive response for this request is suppressed (SPRMIB). /// - /// Only services ISO 14229-1 gives a sub-function can suppress a positive response, - /// because the suppressPosRspMsgIndicationBit is bit 7 of that sub-function byte. For a - /// service with no sub-function the answer is a definite `Some(false)`. - /// - /// # `None` means the question has no answer, not that it was not asked - /// - /// Returns `None` in exactly three situations, all of which are genuinely unanswerable - /// rather than merely unmodeled: - /// - /// - The service identifier is not one ISO 14229-1 assigns — the vendor-specific case. - /// The crate has no basis for an answer, and a caller who needs one has to supply it - /// from the application that originated the request. - /// - The service has a sub-function but the payload is empty, so the byte holding the - /// SPRMIB is not present. Such a frame is malformed for its service; either way there - /// is no bit to report. - /// - The service identifier is `0x83`, whose service the 2020 edition withdrew. See - /// [`UdsServiceType::has_sub_function`] for why that answer is deferred to the caller - /// rather than taken from the 2013 edition. - /// - /// A service this crate enumerates but does not model still gets a real answer, because - /// whether it carries a sub-function is a fact of the standard: see - /// [`UdsServiceType::has_sub_function`]. So a `0x2C` - /// [`DynamicallyDefineDataIdentifier`](UdsServiceType::DynamicallyDefineDataIdentifier) - /// request reports its SPRMIB even though it decodes to [`Request::Other`]. - /// - /// # Why this is `Option` when [`Request::allowed_nack_codes`] is not - /// - /// `allowed_nack_codes` reports "unknown" as an empty slice and documents it, which - /// works because no service has an empty table of listed codes — the sentinel cannot be - /// confused with a real answer. `false` has no such property: fourteen services - /// genuinely have no sub-function, so a bare `bool` cannot distinguish them from a - /// service the crate knows nothing about. That distinction has teeth at a layer - /// boundary. ISO 14229-2:2021 clause 10.3 gates `tP3_Client_Phys` on this bit, and - /// whether a response is expected decides whether `tP_Client` starts at all — so - /// answering `false` for a fire-and-forget vendor request costs three transmissions of - /// a request nobody wanted answered, per ISO 14229-2:2021 Table 9 retries. + /// Only a service ISO 14229-1 gives a sub-function can suppress a positive response, + /// because the suppressPosRspMsgIndicationBit is bit 7 of that sub-function byte. A + /// service with no sub-function answers `Some(false)`, and so does an enumerated but + /// unmodeled one, since [`UdsServiceType::has_sub_function`] is a fact of the standard + /// rather than of this crate's coverage. `None` means the crate cannot say: the SID is + /// one ISO 14229-1 does not assign, the sub-function byte is absent from the payload, + /// or the SID is the withdrawn `0x83`. `None` is kept apart from `Some(false)` because + /// ISO 14229-2:2021 clause 10.3 gates `tP3_Client_Phys` on this bit, so a wrong `false` + /// costs a session layer a timeout and retries for a request nobody wanted answered. #[must_use] pub fn is_positive_response_suppressed(&self) -> Option { match self { @@ -231,10 +204,8 @@ impl Request<'_> { Self::RoutineControl(req) => Some(req.suppress_positive_response), Self::SecurityAccess(req) => Some(req.suppress_positive_response), Self::TesterPresent(req) => Some(req.suppress_positive_response), - // The modeled services ISO gives no sub-function. Spelled out rather than - // folded into a wildcard: a wildcard is what made every unrecognized service - // silently report `false`, and it would do the same to the next variant added - // here. + // The modeled services ISO gives no sub-function, spelled out so that a variant + // added later has to choose an answer rather than inherit `false`. Self::ClearDiagnosticInfo(_) | Self::ReadDataByIdentifier(_) | Self::RequestDownload(_) @@ -255,28 +226,15 @@ impl Request<'_> { /// The [`NegativeResponseCode`]s ISO 14229-1 **lists** for this request's service. /// - /// Each request type also exposes this as an associated function (e.g. - /// [`EcuResetRequest::allowed_nack_codes`]); this dispatches to it for a decoded - /// [`Request`], so a server does not have to re-match every variant to reach it. - /// - /// # This is a floor, not a ceiling — do not use it as a validation whitelist - /// - /// Clause 9.4: the Annex A.1 codes "shall be used **in addition to** the negative - /// response codes specified in each service description", and A.1 itself says a server - /// "may also utilise additional and applicable negative response codes … as defined by - /// the vehicle manufacturer". A.1 deliberately keeps the generally-supported codes out - /// of the per-service tables and says so per code — including `0x78` - /// [`RequestCorrectlyReceivedResponsePending`](NegativeResponseCode::RequestCorrectlyReceivedResponsePending), - /// which appears in **none** of these tables and is one of the most common codes in - /// real traffic. A client that rejected any NRC absent from this slice would reject - /// every `ResponsePending` it ever saw. - /// - /// Read it as "the codes the standard tabulates for this service", which is useful for - /// building a tester UI or a conformance report, and not as the set a server may send. - /// - /// Returns an empty slice for [`Request::Other`], which covers services the crate does - /// not model. That means "the NRC set is unknown", not "no codes apply" — consult ISO - /// 14229-1 for those services. + /// Dispatches to the request type's own associated function (for example + /// [`EcuResetRequest::allowed_nack_codes`]) so a server holding a decoded [`Request`] + /// does not re-match every variant. The slice is a floor, not a ceiling: clause 9.4 + /// says the Annex A.1 codes "shall be used **in addition to**" the per-service tables, + /// and `0x78` + /// [`RequestCorrectlyReceivedResponsePending`](NegativeResponseCode::RequestCorrectlyReceivedResponsePending) + /// appears in none of them. Read it as the codes the standard tabulates for the + /// service, for a tester UI or a conformance report, never as a validation whitelist. + /// Empty for [`Request::Other`], meaning the set is unknown, not that no code applies. #[must_use] pub fn allowed_nack_codes(&self) -> &'static [NegativeResponseCode] { match self { @@ -401,11 +359,9 @@ mod tests { #[test] fn suppression_is_unknown_for_a_vendor_specific_service() { - // 0x40 is not in the ISO request table, so it is presumably vendor-specific and the - // crate has no basis for an answer. This is the case the `Option` exists for: - // reporting `false` here made a fire-and-forget vendor request look - // response-expected, so a session layer started tP_Client, timed out, and retried - // it twice. + // 0x40 is not in the ISO request table, so the crate has no basis for an answer. + // This is the case the `Option` exists for: a `false` here would make a + // fire-and-forget vendor request look response-expected to a session layer. let (req, _) = Request::decode(&[0x40, 0xAA, 0xBB]).unwrap(); assert!(matches!(req, Request::Other { .. })); assert_eq!(req.is_positive_response_suppressed(), None); @@ -451,23 +407,13 @@ mod tests { } /// One minimal-but-valid frame per modeled service, paired with the NRC table that - /// service must dispatch to. + /// service dispatches to. Shared by the two tests below so they cannot drift apart. /// - /// Shared by `allowed_nack_codes_dispatches_for_every_modeled_variant` and - /// `every_modeled_variant_agrees_with_the_sub_function_table` so the two cannot drift - /// apart. A function rather than a `const`, because `allowed_nack_codes` is not a - /// `const fn`. - /// - /// Every service that has a sub-function has bit 7 of that byte SET. That is - /// load-bearing for the sub-function assertion: with the bit clear every service - /// answers `Some(false)`, so a flipped table entry would go unnoticed. Setting it - /// changes neither which service a frame decodes to nor its NRC table, so the NRC - /// assertions are indifferent to it. - /// - /// Two pairs are deliberately indistinguishable, and that is correct rather than a gap: - /// ISO gives `CommunicationControl` and `ControlDTCSetting` the same four codes, and - /// Tables 444 and 449 give `RequestDownload` and `RequestUpload` the same six. Swapping - /// either pair's rows is unobservable because the answer is the same. + /// Every service with a sub-function has bit 7 of that byte set: with the bit clear + /// every service answers `Some(false)` and a flipped sub-function table entry goes + /// unnoticed. Two pairs share a table, `CommunicationControl` with `ControlDTCSetting` + /// and `RequestDownload` with `RequestUpload` (Tables 444 and 449), so swapping either + /// pair's rows is unobservable. fn modeled_frames() -> [(&'static [u8], &'static [NegativeResponseCode]); 16] { [ // ClearDiagnosticInfo, no sub-function @@ -546,14 +492,9 @@ mod tests { #[test] fn allowed_nack_codes_dispatches_for_every_modeled_variant() { - // Every one of the 16 request types has an inherent `allowed_nack_codes()`, but - // without this dispatcher a caller holding a *decoded* `Request` had to re-match - // all 16 variants to reach it — on a `#[non_exhaustive]` enum they cannot match - // exhaustively. Frames are minimal-but-valid for each service. - // - // Each frame is paired with the inherent table it must dispatch to. Asserting only - // `!is_empty()` made the test vacuous: every modeled service has a non-empty table, - // so any arm could return any *other* service's set and still pass. + // Each frame is compared against the inherent table it dispatches to, not merely + // against `!is_empty()`: every modeled service has a non-empty table, so an arm + // returning another service's set would otherwise pass. let frames = modeled_frames(); for (frame, expected) in frames { let (req, _) = Request::decode(frame).unwrap_or_else(|e| { @@ -575,12 +516,9 @@ mod tests { #[test] fn every_modeled_variant_agrees_with_the_sub_function_table() { // `has_sub_function` and the match in `is_positive_response_suppressed` are two - // copies of one ISO fact, and nothing else stops them from drifting. This ties them - // together as a biconditional: the table says a service has a sub-function exactly - // when the dispatch reports a set SPRMIB for a frame that sets one. - // - // `modeled_frames` sets the SPRMIB on every service that has one, which is what - // makes the equality below meaningful in both directions. + // copies of one ISO fact. This ties them together as a biconditional: the table + // says a service has a sub-function exactly when the dispatch reports a set SPRMIB + // for a frame that sets one, which every `modeled_frames` entry does. for (frame, _) in modeled_frames() { let (req, _) = Request::decode(frame).unwrap_or_else(|e| { panic!("frame {frame:02X?} should decode, got {e:?}"); @@ -603,10 +541,8 @@ mod tests { suppressed.is_some(), "{service:?} is modeled, so its SPRMIB is never unknown" ); - // The frames set the SPRMIB wherever the service has a sub-function to set it - // in, so the two must be equal: `Some(true)` for the eight services that have - // one, `Some(false)` for the eight that do not. Any table entry flipped either - // way breaks this. + // `Some(true)` for the eight services with a sub-function, `Some(false)` for + // the eight without; a table entry flipped either way breaks this. assert_eq!( suppressed, service.has_sub_function(), @@ -655,7 +591,7 @@ mod tests { #[test] fn unknown_request_byte_round_trips_losslessly() { - // 0x40 is not in the ISO request table; it must survive a decode→encode round-trip. + // 0x40 is not in the ISO request table; it survives a decode, encode round trip. let frame = [0x40, 0xAA, 0xBB]; let (req, rest) = Request::decode(&frame).unwrap(); assert_eq!(rest, [0u8; 0]); @@ -670,6 +606,6 @@ mod tests { let written = Encode::encode(&req, &mut automotive_wire_codec::SliceSink::new(&mut buf)) .unwrap(); - assert_eq!(&buf[..written], &frame); // previously re-encoded as 0x7F + assert_eq!(&buf[..written], &frame); } } diff --git a/crates/uds_protocol/src/response.rs b/crates/uds_protocol/src/response.rs index eae67c13..dc1dca52 100644 --- a/crates/uds_protocol/src/response.rs +++ b/crates/uds_protocol/src/response.rs @@ -32,11 +32,11 @@ pub enum Response<'a> { /// /// Unlike the request (a self-delimiting list of 2-byte DIDs, parsed by /// [`ReadDataByIdentifierRequest::dids`](crate::ReadDataByIdentifierRequest::dids)), - /// this response is left opaque **by design**: each data record's length is defined by - /// the ECU's configuration for that DID and is *not* present on the wire, so the - /// library cannot split it into `(DID, value)` pairs. Parse it caller-side once you - /// know each DID's record length — read the 2-byte big-endian DID, take the - /// application-defined number of data bytes, then repeat on the remainder. + /// this response is opaque: each data record's length is defined by the ECU's + /// configuration for that DID and is *not* present on the wire, so the library cannot + /// split it into `(DID, value)` pairs. Parse it caller-side once you know each DID's + /// record length: read the 2-byte big-endian DID, take the application-defined number + /// of data bytes, then repeat on the remainder. ReadDataByIdentifier(ReadDataByIdentifierResponse<'a>), /// Positive response to `ReadDTCInformation` with lazy iterators. ReadDtcInfo(ReadDtcInfoResponse<'a>), @@ -72,8 +72,8 @@ pub enum Response<'a> { /// # Remainder /// -/// The returned remainder is **always empty**. A UDS frame is not self-delimiting — its -/// length comes from the transport (ISO-TP, `DoIP`, ...), not from the message — so one +/// The returned remainder is **always empty**. A UDS frame is not self-delimiting: its +/// length comes from the transport (ISO-TP, `DoIP`, ...), not from the message, so one /// buffer is exactly one frame, and every payload is decoded with `decode_exact`. This /// means `decode` behaves as `decode_exact` despite the streaming shape of the [`Decode`] /// contract: do **not** feed it concatenated frames expecting it to consume one at a time, @@ -301,7 +301,7 @@ mod tests { let written = Encode::encode(&resp, &mut automotive_wire_codec::SliceSink::new(&mut buf)) .unwrap(); - assert_eq!(&buf[..written], &frame); // previously became 0x7F (NegativeResponse) + assert_eq!(&buf[..written], &frame); } #[test] @@ -311,7 +311,7 @@ mod tests { let (resp, remaining) = Response::decode(&[0x54]).unwrap(); assert_eq!(remaining, [0u8; 0]); assert!(matches!(resp, Response::ClearDiagnosticInfo(_))); - // Trailing bytes after the SID are now rejected (matches every other response arm). + // Trailing bytes after the SID are rejected, as in every other response arm. assert!(matches!( Response::decode(&[0x54, 0xAA]), Err(crate::Error::TrailingBytes(_)) diff --git a/crates/uds_protocol/src/service.rs b/crates/uds_protocol/src/service.rs index ca91fdc5..ef060171 100644 --- a/crates/uds_protocol/src/service.rs +++ b/crates/uds_protocol/src/service.rs @@ -10,31 +10,16 @@ pub enum UdsServiceType { // ======================================================================== // Diagnostics and Communications Management - /// UDS uses different session types which can be changed using "Diagnostic Session - /// Control". Different services are available depending on which session is active. - /// "Default Session" is the initial session type. Other session types are defined, but - /// may not be implemented depending on the device: - /// - /// - 0x01 "Default Session" used for basic uds - /// - 0x02 "Programming Session" used to upload software. - /// - 0x03 "Extended Diagnostic Session" used to unlock additional diagnostic functions, - /// such as the adjustment of sensors. - /// - 0x04 "Safety system diagnostic session" used to test all safety-critical - /// diagnostic functions, such as airbag tests. - /// - /// In addition, there are reserved session identifiers that can be defined for vehicle - /// manufacturers and vehicle suppliers specific use. + /// Switches the server's diagnostic session, which selects the set of services it + /// offers. The default session (0x01) is active at power-up; the programming (0x02), + /// extended diagnostic (0x03) and safety system diagnostic (0x04) sessions enable + /// further services, and other values are vehicle-manufacturer or system-supplier + /// specific. See [`DiagnosticSessionType`](crate::DiagnosticSessionType). DiagnosticSessionControl, - /// The service "ECU reset" is used to restart the control unit (ECU). Depending on the - /// control unit hardware and implementation, different forms of reset can be used: - /// - /// - "Hard Reset" simulates a shutdown of the power supply. - /// - "key off on Reset" simulates the drain and turn on the ignition with the key. - /// - Soft Reset" allows the initialization of certain program units and their storage - /// structures. - /// - /// Again, there are reserved values that can be defined for vehicle manufacturers and - /// vehicle suppliers specific use. + /// Restarts the server. The reset types are a hard reset (power-cycle), a key-off-on + /// reset (ignition cycle), a soft reset (application restart) and the rapid power + /// shutdown controls, plus vehicle-manufacturer and system-supplier specific values. + /// See [`ResetType`](crate::ResetType). EcuReset, /// Security check is available to enable the most security-critical uds. For this /// purpose a "Seed" is generated and sent to the client by the control unit. From this @@ -73,8 +58,8 @@ pub enum UdsServiceType { ControlDtcSetting, /// Request the server to start or stop sending responses when a specified event occurs. ResponseOnEvent, - /// The Service Link Control is used to set the baud rate of the diagnostic access. - /// It is usually implemented only at the central gateway. + /// Sets the baud rate of the diagnostic link. Usually implemented only at the central + /// gateway. LinkControl, // ======================================================================== @@ -146,9 +131,8 @@ pub enum UdsServiceType { /// transfer request. This will be used when the amount of data (set in "Request /// Download" or "Upload Request") has not been transferred. RequestTransferExit, - /// This service is used to initiate a file download from the client to the server or - /// upload from the server to the client. Additionally information about the file system - /// are available by this service. + /// Initiates a file download from the client to the server or an upload from the server + /// to the client, and reads information about the server's file system. RequestFileTransfer, /// This response is given when a service request could not be performed, /// for example a request for an unsupported Data Identifier. @@ -162,7 +146,7 @@ pub enum UdsServiceType { impl UdsServiceType { /// Map a request-message service identifier (SID) byte to its [`UdsServiceType`]. /// - /// Unrecognised bytes map to [`UdsServiceType::UnsupportedDiagnosticService`]. + /// Unrecognized bytes map to [`UdsServiceType::UnsupportedDiagnosticService`]. #[must_use] pub const fn from_request_sid(value: u8) -> Self { match value { @@ -240,7 +224,7 @@ impl UdsServiceType { } /// Map a positive-response service identifier (SID) byte to its [`UdsServiceType`]. /// - /// Unrecognised bytes map to [`UdsServiceType::UnsupportedDiagnosticService`]. + /// Unrecognized bytes map to [`UdsServiceType::UnsupportedDiagnosticService`]. #[must_use] pub const fn from_response_sid(value: u8) -> Self { match value { @@ -316,29 +300,14 @@ impl UdsServiceType { /// Whether ISO 14229-1 gives this service a sub-function byte, and therefore whether /// its request can carry a suppressPosRspMsgIndicationBit (SPRMIB) in bit 7 of that - /// byte. - /// - /// 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 this crate - /// models. - /// - /// # `None` means this crate cannot say - /// - /// Three variants have no answer, and `None` for them is a different report from - /// `Some(false)` — keeping the two apart is the whole point of + /// byte. A fact about the standard, so answered for all 26 services of the 2020 + /// edition, not only the 16 this crate models. `None` means the crate cannot say, which /// [`Request::is_positive_response_suppressed`](crate::Request::is_positive_response_suppressed) - /// returning an `Option`: - /// - /// - [`UdsServiceType::UnsupportedDiagnosticService`], where every SID ISO 14229-1 does - /// not assign lands — including the vendor-specific range. This is the case that - /// matters in practice. - /// - [`UdsServiceType::NegativeResponse`], which is not a request service at all. - /// - [`UdsServiceType::AccessTimingParameters`]. The 2020 edition, which this crate - /// targets, withdrew the service; the variant is retained only so a pre-2020 - /// `0x83`/`0xC3` byte round-trips as itself. Whether an earlier edition gave it a - /// sub-function is not a fact this table draws from — every other entry is checked - /// against the 2020 text alone — so the answer is deferred to the caller rather than - /// asserted here. + /// keeps apart from `Some(false)`: [`UdsServiceType::UnsupportedDiagnosticService`], + /// where every unassigned SID lands, the vendor-specific range included; + /// [`UdsServiceType::NegativeResponse`], which is not a request service; and + /// [`UdsServiceType::AccessTimingParameters`], withdrawn in the 2020 edition that every + /// other entry here is checked against. #[must_use] pub const fn has_sub_function(self) -> Option { match self { @@ -397,7 +366,7 @@ mod test { /// statement of what ISO 14229-1 assigns and a transposed pair fails instead of /// agreeing with itself. `NegativeResponse` and `UnsupportedDiagnosticService` are /// absent because neither has a request SID; both are covered by - /// `the_variants_that_are_not_a_2020_request_service_report_no_sub_function`. + /// `non_request_services_have_no_sub_function_answer`. const SERVICES: &[(UdsServiceType, u8, u8, Option)] = &[ ( UdsServiceType::DiagnosticSessionControl, @@ -503,8 +472,7 @@ mod test { #[test] fn the_response_sid_is_the_request_sid_with_bit_6_set() { // ISO 14229-1 derives every positive response SID by adding 0x40 to the request - // SID. Checking the rule as well as the table catches a single mistyped entry - // above, which a table compared only against itself would not. + // SID. Checking the rule as well as the table catches a single mistyped entry. for &(service, request_sid, response_sid, _) in SERVICES { assert_eq!( response_sid, @@ -516,10 +484,9 @@ mod test { #[test] fn the_two_services_without_a_request_sid_say_so() { - // Both have no request SID and return 0x7F, which is not a legal request SID -- it - // is the negative-response SID. Callers needing lossless round-tripping of an - // unmodeled service must use `Request::Other`, so this is pinned rather than left - // to be discovered. + // Both have no request SID and return 0x7F, which is not a legal request SID but + // the negative-response SID. Lossless round-tripping of an unmodeled service goes + // through `Request::Other`. assert_eq!( UdsServiceType::to_request_sid(UdsServiceType::NegativeResponse), 0x7F @@ -536,11 +503,9 @@ mod test { #[test] fn every_service_reports_the_sub_function_iso_gives_it() { - // The fourth column is an independent statement of the standard, not something - // derived from `has_sub_function` -- so a mistyped arm in the implementation fails - // here rather than agreeing with itself. Only the 12 services with a sub-function - // can carry a SPRMIB, which is what `Request::is_positive_response_suppressed` - // depends on. + // The fourth column is an independent statement of the standard, not derived from + // `has_sub_function`, so a mistyped arm fails here rather than agreeing with + // itself. Only the 12 services with a sub-function can carry a SPRMIB. for &(service, request_sid, _, has_sub_function) in SERVICES { assert_eq!( service.has_sub_function(), @@ -551,13 +516,11 @@ mod test { } #[test] - fn the_variants_that_are_not_a_2020_request_service_report_no_sub_function() { + fn non_request_services_have_no_sub_function_answer() { // None of these three is a request service in the edition this crate targets, so - // "does it have a sub-function" has no answer. - // - // `UnsupportedDiagnosticService` is the one that matters most: it is where every - // SID ISO does not assign lands, and returning `None` there is what lets a caller - // tell a vendor-specific service apart from one that genuinely has no sub-function. + // "does it have a sub-function" has no answer. `UnsupportedDiagnosticService` is + // where every SID ISO does not assign lands, and `None` there lets a caller tell a + // vendor-specific service apart from one that has no sub-function. assert_eq!(UdsServiceType::NegativeResponse.has_sub_function(), None); assert_eq!( UdsServiceType::UnsupportedDiagnosticService.has_sub_function(), @@ -568,8 +531,8 @@ mod test { None, "0x40 is unassigned" ); - // 0x83 is enumerated -- the variant exists so a 2013-era byte round-trips -- but - // the 2020 edition withdrew the service, so this crate has no basis for an answer. + // 0x83 is enumerated so a 2013-era byte round-trips, but the 2020 edition withdrew + // the service, so this crate has no basis for an answer. assert_eq!( UdsServiceType::AccessTimingParameters.has_sub_function(), None @@ -579,9 +542,8 @@ mod test { #[test] fn every_unassigned_byte_classifies_as_unsupported() { // The conversions are total: no byte panics, and anything outside the table above - // lands on `UnsupportedDiagnosticService` rather than being silently mapped to a - // real service. Written with `any` rather than collecting, so this compiles without - // `alloc`. + // lands on `UnsupportedDiagnosticService` rather than on a real service. Written + // with `any` rather than collecting, so this compiles without `alloc`. let is_request_sid = |b: u8| SERVICES.iter().any(|&(_, r, _, _)| r == b); let is_response_sid = |b: u8| b == 0x7F || SERVICES.iter().any(|&(_, _, s, _)| s == b); diff --git a/crates/uds_protocol/src/services/clear_dtc_information.rs b/crates/uds_protocol/src/services/clear_dtc_information.rs index e3b8b372..48d27699 100644 --- a/crates/uds_protocol/src/services/clear_dtc_information.rs +++ b/crates/uds_protocol/src/services/clear_dtc_information.rs @@ -140,7 +140,6 @@ impl<'a> Decode<'a> for ClearDiagnosticInfoRequest { } } -/// test #[cfg(test)] mod request { use super::*; @@ -224,7 +223,7 @@ mod request { #[test] fn a_truncated_group_of_dtc_is_still_rejected() { - // The 3 groupOfDTC bytes stay mandatory; only the 4th byte is optional. + // The 3 groupOfDTC bytes are mandatory; only the 4th byte is optional. let got = ::decode(&[0xFF, 0xFF]); assert!( matches!( diff --git a/crates/uds_protocol/src/services/communication_control.rs b/crates/uds_protocol/src/services/communication_control.rs index 5e7dd96f..07cf5bcc 100644 --- a/crates/uds_protocol/src/services/communication_control.rs +++ b/crates/uds_protocol/src/services/communication_control.rs @@ -3,37 +3,29 @@ use crate::shared::SuppressablePositiveResponse; use crate::{Decode, Encode, Error, Incomplete, NegativeResponseCode}; use automotive_wire_codec::{write_bytes, write_u8, write_u16_be}; -/// `CommunicationControlType` is used to specify the type of communication behavior to be -/// modified +/// The `controlType` sub-function of `CommunicationControl`: which communication behavior +/// the server is to change. /// -/// *Note*: -/// -/// Conversions from `u8` to `CommunicationControlType` are fallible and will return an -/// [`Error`] if the Suppress Positive Response bit is set. +/// Conversion from `u8` is fallible and returns an [`Error`] if bit 7, the suppress +/// positive response bit, is set. #[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))] #[cfg_attr(feature = "serde", serde(try_from = "u8", into = "u8"))] #[cfg_attr(feature = "clap", derive(clap::ValueEnum))] #[derive(Clone, Copy, Debug, Eq, PartialEq)] pub enum CommunicationControlType { - /// This value indicates that the reception and transmission of messages - /// shall be enabled for the specified [`CommunicationType`] + /// Turn on both receiving and sending for the [`CommunicationType`] (`0x00`, Table 54). EnableRxAndTx, - /// This value indicates that the reception of messages shall be enabled and the - /// transmission of messages shall be disabled for the specified [`CommunicationType`] + /// Keep receiving but stop sending for the [`CommunicationType`] (`0x01`, Table 54). EnableRxAndDisableTx, - /// This value indicates that the reception of messages shall be disabled and the - /// transmission of messages shall be enabled for the specified [`CommunicationType`] + /// Stop receiving but keep sending for the [`CommunicationType`] (`0x02`, Table 54). DisableRxAndEnableTx, - /// This value indicates that the reception and transmission of messages - /// shall be disabled for the specified [`CommunicationType`] + /// Stop both receiving and sending for the [`CommunicationType`] (`0x03`, Table 54). DisableRxAndTx, - /// This value indicates that the reception of messages shall be enabled and the - /// transmission of messages shall be disabled for the specified [`CommunicationType`] - /// Additionally, enhanced address information shall be included in the request + /// As [`EnableRxAndDisableTx`](Self::EnableRxAndDisableTx), for the node the request's + /// enhanced address information names (`0x04`, Table 54). EnableRxAndDisableTxWithEnhancedAddressInfo, - /// This value indicates that the reception and transmission of messages - /// shall be enabled for the specified [`CommunicationType`] - /// Additionally, enhanced address information shall be included in the request + /// As [`EnableRxAndTx`](Self::EnableRxAndTx), for the node the request's enhanced + /// address information names (`0x05`, Table 54). EnableRxAndTxWithEnhancedAddressInfo, /// These values are reserved by the ISO 14229-1 Specification. /// @@ -61,10 +53,9 @@ pub enum CommunicationControlType { impl CommunicationControlType { /// The raw sub-function byte, without the SPRMIB bit. /// - /// `const` so [`CommunicationControlRequest::new`] and - /// [`new_with_node_id`](CommunicationControlRequest::new_with_node_id) can be `const` - /// too: they need the byte for their error payload, and `u8::from` is a trait method, - /// which is not callable in a `const fn` on stable. + /// `const`, unlike `u8::from`, so [`CommunicationControlRequest::new`] and + /// [`new_with_node_id`](CommunicationControlRequest::new_with_node_id) can build their + /// error payload in a `const fn`. #[must_use] pub const fn value(&self) -> u8 { match self { @@ -118,7 +109,7 @@ impl TryFrom for CommunicationControlType { #[cfg(test)] mod communication_control_type_tests { use super::*; - /// Check that we properly decode and encode hex bytes + /// Every byte classifies into its named value or range, or fails for bit 7. #[test] fn from_all_u8_values() { for i in 0..=u8::MAX { @@ -187,7 +178,7 @@ mod communication_control_type_tests { } } -/// Which network the `communicationType` byte applies to — its high nibble (bits 7-4). +/// Which network the `communicationType` byte applies to: its high nibble (bits 7-4). /// /// See ISO 14229-1:2020 Annex B Table B.1. The low nibble of the same byte is the /// [`CommunicationType`]. @@ -195,19 +186,19 @@ mod communication_control_type_tests { #[cfg_attr(feature = "serde", serde(try_from = "u8", into = "u8"))] #[derive(Clone, Copy, Debug, Default, Eq, PartialEq)] pub enum SubnetNumber { - /// `0x0` — apply to the receiving node, including communication to all connected + /// `0x0`: apply to the receiving node, including communication to all connected /// networks. /// /// The default, and what a request that does not target a particular subnet carries. #[default] AllConnectedNetworks, - /// `0x1`-`0xE` — apply to the specific subnet identified by this number. + /// `0x1`-`0xE`: apply to the specific subnet identified by this number. /// /// Construct through [`SubnetNumber::try_from`] so the value is range-checked and /// cannot collide with the other two variants. #[non_exhaustive] Specific(u8), - /// `0xF` — apply to the network the request was received on. + /// `0xF`: apply to the network the request was received on. ReceivedOn, } @@ -244,7 +235,7 @@ impl TryFrom for SubnetNumber { } } -/// Which messages the control applies to — the low nibble (bits 3-0) of the +/// Which messages the control applies to: the low nibble (bits 3-0) of the /// `communicationType` byte. /// /// ISO 14229-1:2020 Annex B Table B.1 defines bits 1-0 and reserves bits 3-2; the high @@ -389,7 +380,7 @@ impl CommunicationControlRequest { /// /// # Errors /// Returns [`Error::InvalidCommunicationControlType`] if `control_type` is an - /// enhanced-address variant — those require a node identifier and must be built + /// enhanced-address variant; those require a node identifier and must be built /// with [`new_with_node_id`](Self::new_with_node_id). pub const fn new( suppress_positive_response: bool, @@ -412,7 +403,7 @@ impl CommunicationControlRequest { /// /// # Errors /// Returns [`Error::InvalidCommunicationControlType`] if `control_type` is not an - /// enhanced-address variant — a node identifier is only carried by the + /// enhanced-address variant; a node identifier is only carried by the /// `*WithEnhancedAddressInfo` variants. pub const fn new_with_node_id( suppress_positive_response: bool, @@ -462,7 +453,7 @@ impl CommunicationControlRequest { self.communication_type } - /// Which network the control applies to — the high nibble of the `communicationType` + /// Which network the control applies to: the high nibble of the `communicationType` /// byte. #[must_use] pub const fn subnet(&self) -> SubnetNumber { @@ -510,11 +501,11 @@ impl Encode for CommunicationControlRequest { impl<'a> Decode<'a> for CommunicationControlRequest { type Error = crate::Error; - /// Decodes a request, reading every `communicationType` Annex B Table B.1 reserves — - /// bits 1-0 clear, or bits 3-2 set — as [`CommunicationType::IsoSaeReserved`] rather + /// Decodes a request, reading every `communicationType` Annex B Table B.1 reserves + /// (bits 1-0 clear, or bits 3-2 set) as [`CommunicationType::IsoSaeReserved`] rather /// than rejecting it. A server answers it `requestOutOfRange` (0x31), but ISO /// 14229-1:2020 8.7.5 tests the message length first, so a request that also carries - /// a stray byte must still reach the length check and be 0x13. + /// a stray byte still reaches the length check and is answered 0x13. fn decode(buf: &'a [u8]) -> Result<(Self, &'a [u8]), Error> { if buf.len() < 2 { return Err(Error::InsufficientData(Incomplete { @@ -619,10 +610,8 @@ mod request { fn the_communication_type_byte_carries_a_subnet_number() { // ISO 14229-1:2020 Annex B Table B.1 splits this byte: bits 0-1 are the message // type, bits 4-7 the subnet number (0 = the specified types on all connected - // networks, 1-E = a specific subnet, F = the network the request arrived on). The - // whole byte was matched against 0x00..=0x03, so 0xF3 -- "network management and - // normal messages on the network this request came in on", a common real-world - // value -- was rejected. + // networks, 1-E = a specific subnet, F = the network the request arrived on). 0xF3, + // both message types on the network the request came in on, is a common value. for (byte, message_type, subnet) in [ ( 0x03u8, @@ -787,8 +776,8 @@ mod request { #[test] fn new_rejects_enhanced_address_variant() { - // An enhanced-address control type has no node id via `new`; it must error - // rather than silently encode a frame missing the mandatory node identifier. + // An enhanced-address control type has no node id via `new`, so `new` errors + // rather than encoding a frame without the mandatory node identifier. let result = CommunicationControlRequest::new( false, CommunicationControlType::EnableRxAndTxWithEnhancedAddressInfo, @@ -845,9 +834,9 @@ mod response { /// `node_id` must be present exactly when `control_type` is one of the two enhanced-address /// variants (`0x04`, `0x05`) and absent otherwise; a payload that breaks that rule is /// rejected rather than encoded into a frame this crate's own decoder would refuse. -/// -/// This doc comment is the published schema description for `CommunicationControlRequest`, -/// because its hand-written `PartialSchema` delegates here. +// +// This doc comment is the published schema description for `CommunicationControlRequest`, +// whose hand-written `PartialSchema` delegates here. #[cfg(feature = "serde")] #[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))] #[cfg_attr(feature = "utoipa", derive(utoipa::ToSchema))] @@ -909,10 +898,8 @@ impl utoipa::ToSchema for CommunicationControlRequest { /// Register the child schemas this one `$ref`s. /// - /// `ToSchema::schemas` defaults to a no-op, and the derive is what normally overrides - /// it. A hand-written impl that only implements `schema()` therefore emits `$ref`s to - /// types the document never defines, which every client generator either rejects or - /// degrades to an untyped object -- strictly worse than the derived schema it replaced. + /// `ToSchema::schemas` defaults to a no-op; a hand-written impl that forwards only + /// `schema()` emits `$ref`s to types the document never defines. fn schemas( schemas: &mut Vec<( String, diff --git a/crates/uds_protocol/src/services/control_dtc_settings.rs b/crates/uds_protocol/src/services/control_dtc_settings.rs index 92b637e7..84479295 100644 --- a/crates/uds_protocol/src/services/control_dtc_settings.rs +++ b/crates/uds_protocol/src/services/control_dtc_settings.rs @@ -71,7 +71,7 @@ impl TryFrom for DtcSettingType { } } -/// The `ControlDtcSetting` service is used to control the DTC settings of the ECU. +/// Request for the server to enable or disable DTC status-bit updates. #[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))] #[cfg_attr(feature = "utoipa", derive(utoipa::ToSchema))] #[derive(Clone, Copy, Debug, Eq, PartialEq)] @@ -84,8 +84,8 @@ pub struct ControlDtcSettingRequest<'d> { /// Optional `DTCSettingControlOptionRecord`, empty when absent. /// /// Marked `U` (user option) in ISO 14229-1:2020 Table 127. Table 129 describes it as - /// vehicle-manufacturer specific data qualifying the request — for example a list of - /// the DTCs to turn on or off — and Table 132 reserves + /// vehicle-manufacturer specific data qualifying the request, for example a list of + /// the DTCs to turn on or off, and Table 132 reserves /// [`NegativeResponseCode::RequestOutOfRange`] for a server that detects an error in /// it. #[cfg_attr(feature = "serde", serde(borrow))] @@ -156,7 +156,7 @@ impl<'a> Decode<'a> for ControlDtcSettingRequest<'a> { /// The sub-function byte is mandatory; everything after it is the optional /// `DTCSettingControlOptionRecord` (ISO 14229-1:2020 Table 127, `Cvt` = `U`), whose - /// length is not on the wire — the record runs to the end of the message. + /// length is not on the wire: the record runs to the end of the message. fn decode(buf: &'a [u8]) -> Result<(Self, &'a [u8]), Error> { let [sub_function, option_record @ ..] = buf else { return Err(Error::InsufficientData(Incomplete { @@ -252,10 +252,9 @@ mod request { #[test] fn manufacturer_and_supplier_specific_setting_types_are_supported() { // ISO 14229-1:2020 Table 128 reserves 0x40-0x5F for vehicle-manufacturer use and - // 0x60-0x7E for system-supplier use. Both were rejected outright, so a server could - // not even see the byte in order to answer SubFunctionNotSupported for it, and a - // client could not send a manufacturer-defined setting at all. Every sibling - // sub-function enum in the crate models these ranges. + // 0x60-0x7E for system-supplier use. Both decode, so a server sees the byte it + // answers SubFunctionNotSupported to and a client can send a manufacturer-defined + // setting. for (byte, expected) in [ (0x40u8, DtcSettingType::VehicleManufacturerSpecific(0x40)), (0x5F, DtcSettingType::VehicleManufacturerSpecific(0x5F)), @@ -294,8 +293,7 @@ mod request { fn a_dtc_setting_control_option_record_round_trips() { // Table 127 marks DTCSettingControlOptionRecord `U`, and Table 129 describes it as // carrying e.g. a list of DTCs to turn on or off. Table 132 reserves NRC 0x31 for - // an error *in that record*, which a server can only detect if it is decoded at - // all. Without it, `85 02 AA BB CC` was rejected as having trailing bytes. + // an error *in that record*, which a server can only detect if it is decoded. let wire = [0x85, 0x02, 0xAA, 0xBB, 0xCC]; let (req, _) = crate::Request::decode(&wire).unwrap(); let crate::Request::ControlDtcSetting(inner) = req else { @@ -311,8 +309,8 @@ mod request { #[test] fn an_absent_option_record_is_an_empty_slice() { - // Table 133's flow example is the bare two-byte form, so absent must stay absent - // rather than becoming a zero byte on the wire. + // Table 133's flow example is the bare two-byte form: an absent record encodes as + // nothing, not as a zero byte. let (req, _) = crate::Request::decode(&[0x85, 0x02]).unwrap(); let crate::Request::ControlDtcSetting(inner) = req else { panic!("expected a ControlDtcSetting request"); diff --git a/crates/uds_protocol/src/services/diagnostic_session_control.rs b/crates/uds_protocol/src/services/diagnostic_session_control.rs index 04bb7976..5a237ce0 100644 --- a/crates/uds_protocol/src/services/diagnostic_session_control.rs +++ b/crates/uds_protocol/src/services/diagnostic_session_control.rs @@ -1,26 +1,19 @@ -//! The `DiagnosticSessionControl` service is used to enable different diagnostic sessions -//! in the server. A diagnostic session enables a specific set of diagnostic services and/or -//! functionality in the server. This service provides the capability that the server can -//! report data link layer specific parameter values valid for the enabled diagnostic -//! session (e.g. timing parameter values). The user of this document shall define the exact -//! set of services and/or functionality enabled in each diagnostic session. There shall -//! always be exactly one diagnostic session active in a server. A server shall always start -//! the default diagnostic session when powered up. If no other diagnostic session is -//! started, then the default diagnostic session shall be running as long as the server is -//! powered. A server shall be capable of providing diagnostic functionality under normal -//! operating conditions, as well as in other operation conditions defined by the vehicle -//! manufacturer (e.g. limp home operation condition). +//! `DiagnosticSessionControl` (0x10) service implementation. +//! +//! A diagnostic session enables a set of services in the server, which the vehicle +//! manufacturer defines per session. ISO 14229-1:2020 clause 10.2 has exactly one session +//! active in a server at a time: the default session from power-up until another is +//! started. The positive response carries the session's timing parameters. use crate::shared::SuppressablePositiveResponse; use crate::{Decode, Encode, Error, Incomplete, NegativeResponseCode}; use automotive_wire_codec::{write_u8, write_u16_be}; -/// `DiagnosticSessionType` is used to specify or describe the session type of the server +/// The `diagnosticSessionType` sub-function: the session a request asks for, or a response +/// reports. /// -/// *Note*: -/// -/// Conversions from `u8` to `DiagnosticSessionType` are fallible and will return an -/// [`Error`] if the Suppress Positive Response bit is set. +/// Conversion from `u8` is fallible and returns an [`Error`] if bit 7, the suppress +/// positive response bit, is set. #[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))] #[cfg_attr(feature = "serde", serde(try_from = "u8", into = "u8"))] #[cfg_attr(feature = "clap", derive(clap::ValueEnum))] @@ -112,7 +105,7 @@ impl TryFrom for DiagnosticSessionType { #[cfg(test)] mod diagnostic_session_type_tests { use super::*; - /// Check that we properly decode and encode hex bytes + /// Every byte classifies as ISO 14229-1:2020 Table 24 assigns it, or fails for bit 7. #[test] fn from_all_u8_values() { for i in 0..=u8::MAX { @@ -284,7 +277,7 @@ impl P2StarServerMax { #[derive(Clone, Copy, Debug, Eq, PartialEq)] #[non_exhaustive] pub struct DiagnosticSessionControlResponse { - /// The session type that is now active. + /// The session type active after the request. pub session_type: DiagnosticSessionType, /// `P2Server_max`: how long the client should wait for a response, in milliseconds /// (ISO 14229-1:2020 Table 29). diff --git a/crates/uds_protocol/src/services/ecu_reset.rs b/crates/uds_protocol/src/services/ecu_reset.rs index 223054a2..d0d3943d 100644 --- a/crates/uds_protocol/src/services/ecu_reset.rs +++ b/crates/uds_protocol/src/services/ecu_reset.rs @@ -3,13 +3,11 @@ use crate::shared::SuppressablePositiveResponse; use crate::{Decode, Encode, Error, Incomplete, NegativeResponseCode}; use automotive_wire_codec::write_u8; -/// UDS defines a number of different types of resets that can be requested -/// The reset type is used to specify the type of reset that the ECU should perform +/// The `resetType` sub-function: which reset the server is to perform (ISO 14229-1:2020 +/// Table 34). /// -/// *Note*: -/// -/// Conversions from `u8` to `ResetType` are fallible and will return an [`Error`] if the -/// Suppress Positive Response bit is set. +/// Conversion from `u8` is fallible and returns an [`Error`] if bit 7, the suppress +/// positive response bit, is set. #[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))] #[cfg_attr(feature = "serde", serde(try_from = "u8", into = "u8"))] #[cfg_attr(feature = "clap", derive(clap::ValueEnum))] @@ -22,42 +20,25 @@ pub enum ResetType { #[cfg_attr(feature = "clap", clap(skip))] #[non_exhaustive] IsoSaeReserved(u8), - /// This `SubFunction` identifies a "hard reset" condition which simulates the - /// power-on/start-up sequence typically performed after a server has been previously - /// disconnected from its power supply (i.e. battery). The performed action is - /// implementation specific and not defined by the spec. It might result in the - /// re-initialization of both volatile memory and non-volatile memory locations to - /// predetermined values. + /// A "hard reset": simulates the power-on sequence after the server is disconnected + /// from its power supply. The action is implementation specific; it may reinitialize + /// both volatile and non-volatile memory to predetermined values. HardReset, - /// This `SubFunction` identifies a condition similar to the driver turning the ignition - /// key off and back on. This reset condition should simulate a key-off-on sequence - /// (i.e. interrupting the switched power supply). The performed action is - /// implementation specific and not defined by the spec. Typically the values of - /// non-volatile mmemory locations are preserved; volatile memory will be initialized. + /// Simulates an ignition off-and-on cycle, interrupting the switched power supply. The + /// action is implementation specific; typically volatile memory is initialized and + /// non-volatile memory preserved. KeyOffOnReset, - /// This `SubFunction` identifies a "soft reset" condition, which causes the server to - /// immediately restart the application program if applicable. The performed action is - /// implementation specific and not defined by the spec. A typical action is to restart - /// the application without reinitializing of previously applied configuration data, - /// adaptive factors and other long-term adjustments. + /// A "soft reset": the server restarts its application program immediately. The action + /// is implementation specific; typically configuration data, adaptive factors and other + /// long-term adjustments survive. SoftReset, - /// This `SubFunction` applies to ECUs which are not ignition powered but battery - /// powered only. Therefore a shutdown forces the sleep mode rather than a power off. - /// Sleep means power off but still ready for wake-up (battery powered). The intention - /// of the `SubFunction` is to reduce the stand-by time of an ECU after ignition is - /// turned into the off position. This value requests the server to enable and perform a - /// "rapid powershut down" function. The server shall execute the function immediately - /// once the "key/ignition" is switched off. While the server executes the power down - /// function, it shall transition either directly or after a defined stand-by-time to - /// sleep mode. If the client requires a response message and the server is already - /// prepared to execute the "rapid power shutdown" function, the server shall send the - /// positive response message prior to the start of the "rapid power shut down" - /// function. The next occurrence of a "key on" or "ignition on" signal terminates the - /// "rapid power shut down" function. **NOTE** This `SubFunction` is only applicable to - /// a server supporting a stand-by-mode. + /// Enables the "rapid power shut down" function on a server that is battery powered + /// rather than ignition powered: once the ignition is switched off the server + /// transitions, directly or after a defined stand-by time, to sleep mode, from which + /// the next ignition-on wakes it. If a response is required, the server sends it before + /// shutting down. Applies only to a server with a stand-by mode. EnableRapidPowerShutDown, - /// This `SubFunction` requests the server to disable the previously enabled "rapid - /// power shut down" function. + /// Disables the "rapid power shut down" function. DisableRapidPowerShutDown, /// Reserved for use by vehicle manufacturers. /// @@ -114,7 +95,7 @@ impl TryFrom for ResetType { #[cfg(test)] mod reset_type_tests { use super::*; - /// Check that we properly decode and encode hex bytes + /// Every byte classifies as ISO 14229-1:2020 Table 34 assigns it, or fails for bit 7. #[test] #[allow(clippy::match_same_arms)] fn reset_type_from_all_u8_values() { @@ -363,7 +344,7 @@ mod request { ) .unwrap(); // The request is one byte: the second byte of `bytes` is the next frame's, and the - // decoder is expected to leave it alone. + // decoder leaves it alone. assert_eq!(&buffer[..written], &bytes[..1]); let (result, _) = ::decode(&bytes).unwrap(); assert_eq!(result, req); @@ -395,8 +376,8 @@ mod response { let (result, _) = ::decode(&bytes).unwrap(); assert_eq!(result, resp); - // The encoded bytes themselves, not just their count: without this the two halves - // of the test are disconnected and swapping the two written bytes goes unnoticed. + // The encoded bytes themselves, not just their count, so a swap of the two written + // bytes fails. assert_eq!(&buffer[..written], &bytes); assert_eq!(written, 2); assert_eq!(written, resp.encoded_size().unwrap()); @@ -421,9 +402,8 @@ mod response { #[test] fn a_response_without_a_power_down_time_round_trips_unchanged() { - // Decoding and re-encoding must not invent a powerDownTime byte. This used to - // append a spurious 0x00, so any proxy that decoded and re-encoded ECU traffic - // rewrote every positive response except enableRapidPowerShutDown's. + // Decoding and re-encoding does not invent a powerDownTime byte, so a proxy that + // decodes and re-encodes ECU traffic passes every positive response unchanged. for wire in [[0x51, 0x01].as_slice(), [0x51, 0x04, 0x20].as_slice()] { let (resp, _) = crate::Response::decode(wire).unwrap(); let mut buf = [0u8; 8]; diff --git a/crates/uds_protocol/src/services/negative_response.rs b/crates/uds_protocol/src/services/negative_response.rs index fad5ed80..a566f204 100644 --- a/crates/uds_protocol/src/services/negative_response.rs +++ b/crates/uds_protocol/src/services/negative_response.rs @@ -18,8 +18,8 @@ pub struct NegativeResponse { /// /// Private, unlike the public data-bag fields on most response types: it is a raw byte /// whose typed meaning is derived ([`request_service`](Self::request_service)), and the - /// two constructors deliberately offer different guarantees — [`new`](Self::new) takes - /// a typed service, [`new_with_sid`](Self::new_with_sid) takes the byte. + /// two constructors offer different guarantees: [`new`](Self::new) takes a typed + /// service, [`new_with_sid`](Self::new_with_sid) takes the byte. request_service_sid: u8, /// The negative response code indicating why the request failed. nrc: NegativeResponseCode, @@ -30,8 +30,8 @@ impl NegativeResponse { /// /// Note that [`UdsServiceType::UnsupportedDiagnosticService`] and /// [`UdsServiceType::NegativeResponse`] have no request SID and both echo `0x7F`. To - /// NACK a service byte the crate does not model — the `sid` of a - /// [`Request::Other`](crate::Request::Other) — use [`new_with_sid`](Self::new_with_sid) + /// NACK a service byte the crate does not model, the `sid` of a + /// [`Request::Other`](crate::Request::Other), use [`new_with_sid`](Self::new_with_sid) /// so the original byte is echoed. #[must_use] pub const fn new(request_service: UdsServiceType, nrc: NegativeResponseCode) -> Self { @@ -41,12 +41,9 @@ impl NegativeResponse { } } - /// Create a new `NegativeResponse` echoing a raw request-service byte. - /// - /// For the pass-through case: a server that decoded a - /// [`Request::Other`](crate::Request::Other) can answer - /// [`ServiceNotSupported`](NegativeResponseCode::ServiceNotSupported) while echoing the - /// byte it actually received, which [`new`](Self::new) cannot express. + /// Create a new `NegativeResponse` echoing a raw request-service byte, so a server that + /// decoded a [`Request::Other`](crate::Request::Other) can answer + /// [`ServiceNotSupported`](NegativeResponseCode::ServiceNotSupported) echoing it: /// /// ``` /// use uds_protocol::{NegativeResponse, NegativeResponseCode}; @@ -136,10 +133,9 @@ mod tests { #[test] fn a_server_can_nack_an_unmodeled_service_byte() { - // The pass-through case this type advertises, from the *construction* side. A - // server that decodes `Request::Other { sid: 0x40 }` must be able to answer - // serviceNotSupported echoing 0x40. `new()` cannot express that: it routes through - // `to_request_sid()`, which collapses every unmodeled service to 0x7F. + // A server that decodes `Request::Other { sid: 0x40 }` answers serviceNotSupported + // echoing 0x40. `new()` cannot express that: it routes through `to_request_sid()`, + // which collapses every unmodeled service to 0x7F. let (req, _) = ::decode(&[0x40, 0xAA]).unwrap(); let sid = match req { crate::Request::Other { sid, .. } => sid, @@ -154,8 +150,8 @@ mod tests { .unwrap(); assert_eq!(&buf[..n], &[0x40, 0x11]); - // The typed constructor still collapses unmodeled services, which is why the raw - // constructor has to exist. + // The typed constructor collapses unmodeled services, which is why the raw + // constructor exists. let via_typed = NegativeResponse::new( UdsServiceType::UnsupportedDiagnosticService, NegativeResponseCode::ServiceNotSupported, @@ -165,8 +161,8 @@ mod tests { #[test] fn unknown_echoed_service_round_trips_losslessly() { - // 0x40 is not a modeled request service. The echoed byte must survive - // decode -> encode verbatim (it previously normalized to 0x7F). + // 0x40 is not a modeled request service; the echoed byte survives decode and + // encode verbatim. let wire = [0x40, 0x12]; let (nr, rest) = ::decode(&wire).unwrap(); assert_eq!(rest, [0u8; 0]); diff --git a/crates/uds_protocol/src/services/read_data_by_identifier.rs b/crates/uds_protocol/src/services/read_data_by_identifier.rs index 0083390b..aa93ea3b 100644 --- a/crates/uds_protocol/src/services/read_data_by_identifier.rs +++ b/crates/uds_protocol/src/services/read_data_by_identifier.rs @@ -7,10 +7,10 @@ use automotive_wire_codec::{write_bytes, write_u16_be}; /// Positive response to `ReadDataByIdentifier`: raw `[DID][data record]…` bytes. /// -/// Left opaque **by design**: each data record's length is defined by the ECU's -/// configuration for that DID and is not present on the wire, so the library cannot -/// split it into `(DID, value)` pairs. Read the 2-byte big-endian DID, take the -/// application-defined number of data bytes via [`records`](Self::records), then repeat. +/// Opaque: each data record's length is defined by the ECU's configuration for that DID +/// and is not present on the wire, so the library cannot split it into `(DID, value)` +/// pairs. Read the 2-byte big-endian DID, take the application-defined number of data +/// bytes via [`records`](Self::records), then repeat. #[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))] #[cfg_attr(feature = "utoipa", derive(utoipa::ToSchema))] #[derive(Clone, Copy, Debug, Eq, PartialEq)] @@ -73,15 +73,9 @@ const READ_DID_NEGATIVE_RESPONSE_CODES: [NegativeResponseCode; 5] = [ /// Read-DID request: a list of 16-bit Data Identifiers. Built from native `&[u16]` /// or borrowed from the wire as big-endian bytes; `dids()` yields `u16` either way. /// -/// # serde / utoipa carve-out -/// -/// serde derives are omitted: the zero-copy `Native(&[u16])` backing has no borrowed -/// `Deserialize` impl in serde (zero-copy borrowing via `#[serde(borrow)]` only works -/// for `&[u8]` and `&str`, not `&[u16]`), so `Dids::Native(&[u16])` cannot be -/// deserialized without an owned allocation. -/// -/// utoipa derives are omitted: `utoipa::ToSchema` cannot be derived on -/// `ReadDataByIdentifierRequest` without also deriving it on the private `Dids` enum. +/// No `serde` or `utoipa` implementation: `serde` can borrow only `&[u8]` and `&str`, not +/// the `&[u16]` backing, so deserializing would need an allocation, and `ToSchema` would +/// have to be derived on the private backing enum as well. #[derive(Clone, Copy, Debug, Eq, PartialEq)] #[non_exhaustive] pub struct ReadDataByIdentifierRequest<'d> { @@ -186,7 +180,7 @@ mod test { #[test] fn derive_contract() { assert_impl_eq::>(); - // serde: omitted — see struct-level doc comment for rationale + // No serde assertion: the type has no serde implementation (see its doc comment). } #[test] @@ -205,7 +199,7 @@ mod test { ::decode(&[0xF1, 0x90, 0xF1, 0x86]) .unwrap(); assert_eq!(rest, [0u8; 0]); - // Iterate without alloc (no_std-friendly): pull items directly. + // Pull items directly so the test runs without `alloc`. let mut it = req.dids(); assert_eq!(it.next(), Some(0xF190)); assert_eq!(it.next(), Some(0xF186)); diff --git a/crates/uds_protocol/src/services/read_dtc_information.rs b/crates/uds_protocol/src/services/read_dtc_information.rs index fed2eb3a..faf654ce 100644 --- a/crates/uds_protocol/src/services/read_dtc_information.rs +++ b/crates/uds_protocol/src/services/read_dtc_information.rs @@ -223,12 +223,9 @@ mod read_dtc_info_request_encode_tests { #[test] fn both_sprmib_values_round_trip_through_the_request_frame() { - // ISO 14229-1:2020 Table 11 requires that "values of both '0' and '1' shall be - // supported for all SubFunction parameter values ... supported by the server for - // any given service", and clause 12.3.2.2 introduces 0x19's sub-function table with - // "(suppressPosRspMsgIndicationBit (bit 7) not shown)". A suppressed - // reportDTCByStatusMask used to be rejected with TrailingBytes, because the raw - // 0x82 fell through to IsoSaeReserved, which consumes no payload. + // ISO 14229-1:2020 Table 11 has a server accept both values of the suppress bit on + // every sub-function it supports, and clause 12.3.2.2's sub-function table for 0x19 + // leaves bit 7 out. for (wire, suppressed) in [ ([0x19, 0x02, 0xFF].as_slice(), Some(false)), ([0x19, 0x82, 0xFF].as_slice(), Some(true)), @@ -247,9 +244,7 @@ mod read_dtc_info_request_encode_tests { #[test] fn a_suppressed_sub_function_is_not_mistaken_for_a_reserved_one() { - // 0x8A is reportSupportedDTC with SPRMIB set. Matching on the raw byte decoded this - // as IsoSaeReserved(0x8A) and reported suppression as false, so a server built on - // this answered SubFunctionNotSupported to a request it was required to execute. + // 0x8A is reportSupportedDTC with SPRMIB set, not a reserved sub-function. let (req, _) = ::decode(&[0x8A]).unwrap(); assert_eq!( req.dtc_subfunction, @@ -260,8 +255,8 @@ mod read_dtc_info_request_encode_tests { #[test] fn user_def_memory_dtc_by_status_mask_carries_a_memory_selection() { - // ISO 14229-1:2020 Table 310 marks both DTCStatusMask and MemorySelection `M`. The - // sibling sub-functions 0x18 and 0x19 (Tables 311/312) already carried theirs. + // ISO 14229-1:2020 Table 310 marks both DTCStatusMask and MemorySelection `M`, as + // Tables 311 and 312 do for the sibling sub-functions 0x18 and 0x19. let wire = [0x17, 0xFF, 0x01]; let (req, rest) = ::decode(&wire).unwrap(); assert_eq!( @@ -282,9 +277,8 @@ mod read_dtc_info_request_encode_tests { } #[test] - fn user_def_memory_dtc_by_status_mask_rejects_a_missing_memory_selection() { - // Without the MemorySelection byte the frame is malformed, and used to be accepted - // while the conformant 3-parameter form was rejected as having trailing bytes. + fn sub_function_0x17_requires_a_memory_selection() { + // Without the MemorySelection byte the frame is malformed. assert!(::decode_exact(&[0x17, 0xFF]).is_err()); } @@ -383,9 +377,8 @@ pub struct DtcFaultDetectionCounterRecord { impl DtcFaultDetectionCounterRecord { /// Create a `DtcFaultDetectionCounterRecord`. /// - /// This type is `#[non_exhaustive]`, so downstream crates cannot use a struct literal - /// and need this constructor — for a test fixture, or for a server building the record - /// list that [`DtcFaultDetectionIter`] reads back. + /// For a test fixture, or for a server building the record list that + /// [`DtcFaultDetectionIter`] reads back. #[must_use] pub const fn new(dtc_record: DtcRecord, dtc_fault_detection_counter: u8) -> Self { Self { @@ -465,8 +458,7 @@ pub enum ReadDtcInfoSubFunction { ReportDtcExtDataRecordByRecordNumber(DtcExtDataRecordNumber), /// * Parameter: `DtcStatusMask` - /// * Parameter: `memorySelection`(1) — addresses the user-defined DTC memory to read - /// from + /// * Parameter: `memorySelection`(1), the user-defined DTC memory to read from /// /// Both parameters are mandatory (ISO 14229-1:2020 Table 310). /// @@ -475,14 +467,14 @@ pub enum ReadDtcInfoSubFunction { /// Parameter: `DtcRecord` (3 bytes) /// Parameter: `DtcSnapshotRecordNumber`(1) - /// Parameter: `memorySelection`(1) — addresses the user-defined DTC memory to read from + /// Parameter: `memorySelection`(1), the user-defined DTC memory to read from /// /// 0x18 ReportUserDefMemoryDtcSnapshotRecordByDtcNumber(DtcRecord, DtcSnapshotRecordNumber, u8), /// Parameter: `DtcRecord` (3 bytes) /// Parameter: `DtcExtDataRecordNumber`(1) (0xFF for all records) - /// Parameter: `memorySelection`(1) — addresses the user-defined DTC memory to read from + /// Parameter: `memorySelection`(1), the user-defined DTC memory to read from /// /// 0x19 ReportUserDefMemoryDtcExtDataRecordByDtcNumber(DtcRecord, DtcExtDataRecordNumber, u8), @@ -528,17 +520,15 @@ pub enum ReadDtcInfoSubFunction { impl ReadDtcInfoSubFunction { /// Build the [`IsoSaeReserved`](Self::IsoSaeReserved) variant for a sub-function byte - /// this crate does not model. - /// - /// A tester needs this to originate a request for a report type the crate has not - /// implemented — `Self::IsoSaeReserved(byte)` is not writable outside this crate, - /// because the variant is `#[non_exhaustive]` to keep bit 7 out of it. + /// this crate does not model, which is how a tester originates a request for a report + /// type the crate has not implemented; the variant itself is `#[non_exhaustive]` so bit + /// 7 cannot be written into it. /// /// # Errors /// Returns [`Error::InvalidDtcSubfunctionType`] if bit 7 is set. That bit is the /// suppressPosRspMsgIndicationBit, not part of the sub-function value: pass it as the - /// first argument to [`ReadDtcInfoRequest::new`] instead. Letting it into the variant - /// would make `0x80` encode as a suppressed `0x00`. + /// first argument to [`ReadDtcInfoRequest::new`] instead, or `0x80` would encode as a + /// suppressed `0x00`. pub const fn try_reserved(byte: u8) -> Result { if byte & !SPRMIB_VALUE_MASK != 0 { return Err(Error::InvalidDtcSubfunctionType(byte)); @@ -659,7 +649,7 @@ impl ReadDtcInfoSubFunction { /// A `ReadDTCInformation` report type, without the parameters it carries. /// -/// ISO 14229-1:2020 clause 12.3.2.2, Table 317 — the sub-function byte alone. A server +/// ISO 14229-1:2020 clause 12.3.2.2, Table 317: the sub-function byte alone. A server /// answers Figure 6's sub-function questions (`subFunctionNotSupported`, 0x12, and /// `subFunctionNotSupportedInActiveSession`, 0x7E) from this byte, before the request's /// length is checked, so it needs the report type apart from the parameters @@ -881,7 +871,7 @@ mod read_dtc_info_report_type_tests { } /// A decoded sub-function's report type is the type its byte names, so the two - /// lookups a server makes — before and after decoding — agree. + /// lookups a server makes, before and after decoding, agree. #[test] fn a_sub_function_projects_onto_the_report_type_of_its_byte() { for byte in 0x00..=0x7F_u8 { @@ -924,14 +914,12 @@ impl Encode for ReadDtcInfoSubFunction { /// /// Each pair is 4 bytes: 3 for the DTC record + 1 for the status mask. /// -/// # Length -/// -/// [`len`](DtcAndStatusIter::len) counts **complete records**; -/// [`size_hint`](Iterator::size_hint) counts **items yielded**, which is one greater when a -/// partial record trails the buffer (that tail surfaces as a single `Err`, after which the -/// iterator is exhausted). The two therefore differ on malformed input, which is why this -/// deliberately does not implement `ExactSizeIterator` — its `len()` would contradict the -/// inherent one. It does implement [`FusedIterator`](core::iter::FusedIterator). +/// [`len`](DtcAndStatusIter::len) counts complete records; +/// [`size_hint`](Iterator::size_hint) counts items yielded, one more when a partial record +/// trails the buffer, since that tail surfaces as a single `Err` after which the iterator +/// is exhausted. The two differ on malformed input, so this is a +/// [`FusedIterator`](core::iter::FusedIterator) but not an `ExactSizeIterator`, whose +/// `len()` would contradict the inherent one. #[derive(Clone, Debug)] pub struct DtcAndStatusIter<'a> { remaining: &'a [u8], @@ -975,8 +963,7 @@ impl Iterator for DtcAndStatusIter<'_> { } if self.remaining.len() < 4 { // Consume the partial tail so the error is reported exactly once and the - // iterator terminates. Returning without advancing would yield this error - // forever. + // iterator terminates. self.remaining = &[]; return Some(Err(Error::IncorrectMessageLengthOrInvalidFormat)); } @@ -1000,14 +987,12 @@ impl core::iter::FusedIterator for DtcAndStatusIter<'_> {} /// /// Each record is 4 bytes: 3 for the DTC record + 1 for the fault detection counter. /// -/// # Length -/// -/// [`len`](DtcFaultDetectionIter::len) counts **complete records**; -/// [`size_hint`](Iterator::size_hint) counts **items yielded**, which is one greater when a -/// partial record trails the buffer (that tail surfaces as a single `Err`, after which the -/// iterator is exhausted). The two therefore differ on malformed input, which is why this -/// deliberately does not implement `ExactSizeIterator` — its `len()` would contradict the -/// inherent one. It does implement [`FusedIterator`](core::iter::FusedIterator). +/// [`len`](DtcFaultDetectionIter::len) counts complete records; +/// [`size_hint`](Iterator::size_hint) counts items yielded, one more when a partial record +/// trails the buffer, since that tail surfaces as a single `Err` after which the iterator +/// is exhausted. The two differ on malformed input, so this is a +/// [`FusedIterator`](core::iter::FusedIterator) but not an `ExactSizeIterator`, whose +/// `len()` would contradict the inherent one. #[derive(Clone, Debug)] pub struct DtcFaultDetectionIter<'a> { remaining: &'a [u8], @@ -1075,22 +1060,17 @@ impl Iterator for DtcFaultDetectionIter<'_> { impl core::iter::FusedIterator for DtcFaultDetectionIter<'_> {} /// Lazy iterator over the WWH-OBD `(DtcSeverityMask, DtcRecord, DtcStatusMask)` triples of -/// a [`ReadDtcInfoResponse::WwhObdDtcByMaskRecord`] (sub-function `0x42`). -/// -/// Each triple is 5 bytes: 1 severity + 3 DTC record + 1 status mask. -/// -/// This applies **only** to the WWH-OBD variant. The `0x08`/`0x09` -/// [`DtcSeverityList`](ReadDtcInfoResponse::DtcSeverityList) records are 6 bytes and carry -/// an extra DTC functional-unit byte, so they need a different iterator (not yet wired). -/// -/// # Length +/// a [`ReadDtcInfoResponse::WwhObdDtcByMaskRecord`] (sub-function `0x42`). Each triple is +/// 5 bytes: 1 severity + 3 DTC record + 1 status mask. The `0x08`/`0x09` +/// [`DtcSeverityList`](ReadDtcInfoResponse::DtcSeverityList) records are 6 bytes, with a +/// DTC functional-unit byte, and have no iterator. /// -/// [`len`](WwhObdDtcSeverityIter::len) counts **complete records**; -/// [`size_hint`](Iterator::size_hint) counts **items yielded**, which is one greater when a -/// partial record trails the buffer (that tail surfaces as a single `Err`, after which the -/// iterator is exhausted). The two therefore differ on malformed input, which is why this -/// deliberately does not implement `ExactSizeIterator` — its `len()` would contradict the -/// inherent one. It does implement [`FusedIterator`](core::iter::FusedIterator). +/// [`len`](WwhObdDtcSeverityIter::len) counts complete records; +/// [`size_hint`](Iterator::size_hint) counts items yielded, one more when a partial record +/// trails the buffer, surfacing as a single `Err` after which the iterator is exhausted. +/// The two differ on malformed input, so this is a +/// [`FusedIterator`](core::iter::FusedIterator) but not an `ExactSizeIterator`, whose +/// `len()` would contradict the inherent one. #[derive(Clone, Debug)] pub struct WwhObdDtcSeverityIter<'a> { remaining: &'a [u8], @@ -1277,16 +1257,12 @@ impl From for ReadDtcInfoReportType { /// Zero-copy parsed response for `ReadDTCInformation` (0x19). /// -/// Stores raw bytes for record collections and provides lazy iterators -/// that parse on demand without allocation. -/// -/// # Coverage -/// -/// The variants parse [`DtcCountReport`], [`DtcListReport`] and [`DtcSeverityReport`]'s -/// report types, `0x04` and `0x06` (one DTC's snapshot or extended data records), `0x14` -/// (fault detection counters) and `0x42` (WWH-OBD by mask). Any other report type of -/// [`ReadDtcInfoReportType`] decodes as [`Self::Unmodeled`], its parameters raw; a -/// reserved one is [`Error::InvalidDtcSubfunctionType`]. +/// Stores raw bytes for record collections and provides lazy iterators that parse on +/// demand without allocation. The variants parse [`DtcCountReport`], [`DtcListReport`] and +/// [`DtcSeverityReport`]'s report types, `0x04` and `0x06` (one DTC's snapshot or extended +/// data records), `0x14` (fault detection counters) and `0x42` (WWH-OBD by mask). Any other +/// report type of [`ReadDtcInfoReportType`] decodes as [`Self::Unmodeled`], its parameters +/// raw; a reserved one is [`Error::InvalidDtcSubfunctionType`]. #[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))] #[cfg_attr(feature = "utoipa", derive(utoipa::ToSchema))] #[derive(Clone, Debug, Eq, PartialEq)] @@ -1297,15 +1273,15 @@ pub enum ReadDtcInfoResponse<'a> { /// The report type echoed. report: DtcCountReport, /// Which status bits this server supports reporting. Same representation as - /// [`DtcStatusMask`], but a bit is 'on' when the server supports that status — a + /// [`DtcStatusMask`], but a bit is 'on' when the server supports that status: a /// server that does not support [`DtcStatusMask::WarningIndicatorRequested`] leaves /// that bit 'off' and sets the rest. status_availability_mask: DtcStatusMask, /// How the server's DTC numbers are formatted and encoded. /// /// Mandatory in this response (ISO 14229-1:2020 Table 319), and the only thing that - /// says how to interpret the three bytes of each DTC — ISO 14229-1 itself defines - /// no decoding method for them. + /// says how to interpret the three bytes of each DTC, for which ISO 14229-1 itself + /// defines no decoding method. format_identifier: DtcFormatIdentifier, /// Number of matching DTCs. count: u16, @@ -1315,18 +1291,18 @@ pub enum ReadDtcInfoResponse<'a> { /// The report type echoed. report: DtcListReport, /// Which status bits this server supports reporting. Same representation as - /// [`DtcStatusMask`], but a bit is 'on' when the server supports that status — a + /// [`DtcStatusMask`], but a bit is 'on' when the server supports that status: a /// server that does not support [`DtcStatusMask::WarningIndicatorRequested`] leaves /// that bit 'off' and sets the rest. status_availability_mask: DtcStatusMask, - /// Raw record bytes, 4 per record (3-byte DTC + status) — use [`DtcAndStatusIter`]. + /// Raw record bytes, 4 per record (3-byte DTC + status); use [`DtcAndStatusIter`]. /// Decoding rejects a length that is not a whole number of records. #[cfg_attr(feature = "serde", serde(borrow))] raw_records: &'a [u8], }, /// Sub-function 0x14: list of DTC fault detection counter records. DtcFaultDetectionCounterList { - /// Raw record bytes, 4 per record (3-byte DTC + counter) — use + /// Raw record bytes, 4 per record (3-byte DTC + counter); use /// [`DtcFaultDetectionIter`]. Decoding rejects a length that is not a whole number /// of records. #[cfg_attr(feature = "serde", serde(borrow))] @@ -1365,13 +1341,13 @@ pub enum ReadDtcInfoResponse<'a> { /// The report type echoed. report: DtcSeverityReport, /// Which status bits this server supports reporting. Same representation as - /// [`DtcStatusMask`], but a bit is 'on' when the server supports that status — a + /// [`DtcStatusMask`], but a bit is 'on' when the server supports that status: a /// server that does not support [`DtcStatusMask::WarningIndicatorRequested`] leaves /// that bit 'off' and sets the rest. status_availability_mask: DtcStatusMask, - /// Raw `DTCAndSeverityRecord` bytes: 6 each — severity + DTC functional unit + - /// 3-byte DTC + status. No iterator is wired for this variant yet, so parse them - /// caller-side. Note these are *not* the 5-byte WWH-OBD records read by + /// Raw `DTCAndSeverityRecord` bytes, 6 each: severity + DTC functional unit + + /// 3-byte DTC + status. No iterator is provided for this variant, so parse them + /// caller-side. These are *not* the 5-byte WWH-OBD records read by /// [`WwhObdDtcSeverityIter`]. Decoding rejects a length that is not a multiple of /// 6. #[cfg_attr(feature = "serde", serde(borrow))] @@ -1382,7 +1358,7 @@ pub enum ReadDtcInfoResponse<'a> { /// Functional group identifier echo. functional_group_identifier: FunctionalGroupIdentifier, /// Which status bits this server supports reporting. Same representation as - /// [`DtcStatusMask`], but a bit is 'on' when the server supports that status — a + /// [`DtcStatusMask`], but a bit is 'on' when the server supports that status: a /// server that does not support [`DtcStatusMask::WarningIndicatorRequested`] leaves /// that bit 'off' and sets the rest. status_availability_mask: DtcStatusMask, @@ -1390,7 +1366,7 @@ pub enum ReadDtcInfoResponse<'a> { severity_availability_mask: DtcSeverityMask, /// DTC format identifier. format_identifier: DtcFormatIdentifier, - /// Raw record bytes, 5 per record — use [`WwhObdDtcSeverityIter`]. Decoding rejects + /// Raw record bytes, 5 per record; use [`WwhObdDtcSeverityIter`]. Decoding rejects /// a length that is not a whole number of records. #[cfg_attr(feature = "serde", serde(borrow))] raw_records: &'a [u8], @@ -1449,17 +1425,6 @@ impl<'a> ReadDtcInfoResponse<'a> { } } -/// Validate that a record list divides evenly into whole records. -/// -/// A UDS frame is complete when it arrives — its length comes from the transport — so a -/// trailing partial record means the frame is malformed, not that more bytes are coming. -/// Rejecting it here matches how the crate treats every other length mismatch. -/// -/// Consequently the iterators reached from a **decoded** [`ReadDtcInfoResponse`] never see -/// a partial tail. A hand-constructed variant still can — the enum's `#[non_exhaustive]` -/// stops exhaustive matching, not variant construction, and `Encode` writes `raw_records` -/// verbatim — so the iterators keep their `Result` item type and their -/// one-error-then-terminate behaviour. /// The echo, DTC and status that open sub-function `0x04` or `0x06`, then its records. fn encode_by_dtc_number( writer: &mut impl automotive_wire_codec::Sink, @@ -1504,6 +1469,12 @@ fn encode_masked( Ok(written + write_bytes(writer, records)?) } +/// `raw` if it divides evenly into records of `record_len` bytes. +/// +/// A UDS frame is complete when it arrives, so a trailing partial record is a malformed +/// frame, rejected like every other length mismatch. The iterators reached from a decoded +/// [`ReadDtcInfoResponse`] therefore never see a partial tail; a hand-constructed variant +/// still can, so they keep their `Result` item type. fn whole_records(raw: &[u8], record_len: usize) -> Result<&[u8], Error> { if raw.len().is_multiple_of(record_len) { Ok(raw) @@ -1776,9 +1747,7 @@ mod response_decode_tests { fn the_dtc_count_response_carries_a_format_identifier() { // ISO 14229-1:2020 Table 341 (flow example #1) is exactly these six bytes: SID, // reportType, DTCStatusAvailabilityMask, DTCFormatIdentifier, count high, low. - // Table 319 marks all of them `M`. The format identifier used to be missing from - // the model, so it was read as the count high byte: this frame was rejected - // outright, and a count of 1 came back as 0x0100. + // Table 319 marks all of them `M`. let wire = [0x59, 0x01, 0x2F, 0x01, 0x00, 0x01]; let (resp, _) = Response::decode(&wire).unwrap(); let Response::ReadDtcInfo(ReadDtcInfoResponse::NumberOfDtcs { @@ -1808,12 +1777,8 @@ mod response_decode_tests { fn the_wwh_obd_response_header_and_records_decode_field_by_field() { // ISO 14229-1:2020 Table 332: FGID, DTCStatusAvailabilityMask, // DTCSeverityAvailabilityMask, DTCFormatIdentifier, then 5-byte records of - // severity + 3-byte DTC + status. - // - // Nothing had ever asserted a value this iterator yields, nor any of the four - // header fields — every test checked only counts and `is_ok()`. Reversing the - // header fields, or swapping severity with status and reversing the DTC bytes, - // passed the whole suite. + // severity + 3-byte DTC + status. Every header field and every record field is + // asserted by value, so a reordering of either fails. let wire = [ 0x59, 0x42, 0x33, 0xFF, 0xF0, 0x01, 0x80, 0x01, 0x02, 0x03, 0x0A, ]; @@ -1869,9 +1834,8 @@ mod response_decode_tests { #[test] fn an_insufficient_data_shortfall_measures_both_counts_on_one_buffer() { - // `needed` used to include the sub-function byte while `available` was measured - // after it was sliced off, so a caller computing `needed - available` got a - // shortfall one byte too large. Both are now relative to the payload. + // `needed` and `available` are both relative to the payload after the sub-function + // byte, so `needed - available` is the shortfall. for (label, frame, needed, available) in [ ("0x01 count", [0x59, 0x01, 0x2F].as_slice(), 4, 1), ("0x02 list", [0x59, 0x02].as_slice(), 1, 0), @@ -1903,7 +1867,7 @@ mod response_decode_tests { const LISTS: [(&str, &[u8], usize); 4] = [ // 0x02: status availability mask, then 4-byte (DTC, status) records. ("DtcList 0x02", &[0x02, 0xFF], 4), - // 0x14: no mask byte — records start immediately. + // 0x14: no mask byte; records start immediately. ("DtcFaultDetectionCounterList 0x14", &[0x14], 4), // 0x08: status availability mask, then 6-byte DTCAndSeverityRecord entries. ("DtcSeverityList 0x08", &[0x08, 0xFF], 6), @@ -1935,14 +1899,10 @@ mod response_decode_tests { #[test] fn record_lists_must_divide_evenly_into_records() { - // A trailing partial record means the frame is malformed. Rejecting it here matches - // how the crate treats every other length mismatch, and means the iterators - // returned by the accessors can never see a partial tail. Every misaligned length - // below two whole records, which crucially includes 1..width -- a list too short to - // hold even one record. That range was untested, so a `whole_records` that accepted - // any short list passed: 0 records was covered by `empty_record_lists_are_valid` - // and 1 record by the aligned test, but the 0-valid / 1-invalid boundary, the whole - // point of the check, was not. + // A trailing partial record means the frame is malformed, so the iterators returned + // by the accessors never see a partial tail. Every misaligned length below two + // whole records is tried, including 1..width, a list too short to hold even one + // record, so the 0-valid / 1-invalid boundary is covered. for (label, prefix, width) in LISTS { for record_bytes in (1..2 * width).filter(|n| n % width != 0) { let (buf, len) = frame(prefix, record_bytes); @@ -1969,7 +1929,7 @@ mod response_decode_tests { .map(Iterator::count) .or_else(|| resp.dtc_fault_detection_iter().map(Iterator::count)) .or_else(|| resp.wwh_obd_dtc_severity_iter().map(Iterator::count)); - // DtcSeverityList has no iterator wired yet, so it has no count to check. + // DtcSeverityList has no iterator, so it has no count to check. if let Some(counted) = counted { assert_eq!(counted, records, "{label}: wrong record count"); } @@ -1991,7 +1951,7 @@ mod response_decode_tests { #[test] fn a_misaligned_list_is_rejected_at_the_frame_layer_too() { - // SID 0x59, sub 0x02, mask 0xFF, then 5 record bytes — one byte past a whole + // SID 0x59, sub 0x02, mask 0xFF, then 5 record bytes: one byte past a whole // record. let wire = [0x59, 0x02, 0xFF, 0x01, 0x02, 0x03, 0x0A, 0xEE]; assert!(matches!( @@ -2008,9 +1968,9 @@ mod response_decode_tests { #[test] fn iterators_from_a_decoded_response_never_yield_an_error() { - // The payoff: with decode validating alignment, every iterator obtained through an - // accessor is error-free. The `Err` arm remains reachable only via `Iter::new` on - // arbitrary bytes, which `iter_tests` covers. + // Decode validates alignment, so every iterator obtained through an accessor is + // error-free. The `Err` arm is reachable only via `Iter::new` on arbitrary bytes, + // which `iter_tests` covers. for (label, prefix, width) in LISTS { let (buf, len) = frame(prefix, width * 2); let (resp, _) = ::decode(&buf[..len]).unwrap(); @@ -2041,7 +2001,7 @@ mod iter_tests { assert!(!iter.is_empty()); // A buffer shorter than one record has zero complete records, and is_empty() - // agrees with len() == 0 (the previous bug reported is_empty() == false here). + // agrees with len() == 0. let partial_only = [0x01, 0x02, 0x03]; let iter = DtcAndStatusIter::new(&partial_only); assert_eq!(iter.len(), 0); @@ -2060,12 +2020,9 @@ mod iter_tests { #[test] fn partial_tail_yields_one_error_then_terminates() { - // Previously `next()` returned `Some(Err(..))` on a partial record *without* - // advancing `remaining`, so the iterator yielded that error forever: `for _ in - // iter` looped, `count()` hung, and `collect::>>()` allocated - // without bound. `collect_all()` happened to terminate only because - // `collect::>()` short-circuits on the first error. Bounded with - // `take` so a regression fails instead of hanging the suite. + // A partial record yields one `Err` and then `None`, so `for _ in iter`, `count()` + // and `collect()` all terminate. Bounded with `take` so a non-terminating iterator + // fails the test instead of hanging it. let data = [0x01, 0x02, 0x03, 0x0A, 0xFF]; // one complete record + 1 stray byte let items: heapless_vec::Bounded<8> = DtcAndStatusIter::new(&data).take(8).collect_bounded(); @@ -2095,8 +2052,8 @@ mod iter_tests { fn size_hint_matches_the_number_of_items_yielded() { // All three iterators, every buffer length across several record boundaries. The // two record widths differ (4 bytes vs 5), so each needs its own `div_ceil` - // checked. `take` bounds the count so a non-termination regression fails rather - // than hangs. + // checked. `take` bounds the count so a non-terminating iterator fails rather than + // hangs. let data = [0u8; 16]; for len in 0usize..=16 { let it = DtcAndStatusIter::new(&data[..len]); @@ -2146,8 +2103,8 @@ mod iter_tests { // // The drains are bounded, like the ones in `size_hint_matches_the_number_of_items_ // yielded`: a 7-byte buffer can yield at most 2 items, so `take(8)` cannot hide a - // real result, and a regression that stops advancing `remaining` fails here instead - // of hanging `cargo test` forever with the other failures unreported. + // real result, and an iterator that stops advancing `remaining` fails here instead + // of hanging the test run. const CAP: usize = 8; let data = [0u8; 7]; // not a whole number of records for either width @@ -2234,7 +2191,7 @@ mod by_dtc_number { const EXT_DATA: [u8; 9] = [0x06, 0x12, 0x34, 0x56, 0x24, 0x05, 0x17, 0x10, 0x79]; #[test] - fn a_snapshot_response_decodes_to_its_dtc_status_and_raw_records() { + fn snapshot_response_decodes() { let decoded = ReadDtcInfoResponse::decode_exact(&SNAPSHOT).unwrap(); assert_eq!( decoded, @@ -2250,7 +2207,7 @@ mod by_dtc_number { } #[test] - fn an_ext_data_response_decodes_to_its_dtc_status_and_raw_records() { + fn ext_data_response_decodes() { let decoded = ReadDtcInfoResponse::decode_exact(&EXT_DATA).unwrap(); assert_eq!( decoded, diff --git a/crates/uds_protocol/src/services/request_file_transfer.rs b/crates/uds_protocol/src/services/request_file_transfer.rs index 27824009..b663bec7 100644 --- a/crates/uds_protocol/src/services/request_file_transfer.rs +++ b/crates/uds_protocol/src/services/request_file_transfer.rs @@ -8,8 +8,8 @@ use automotive_wire_codec::{ }; /// Minimum byte-width (clamped to at least 1) needed to hold the larger of two size -/// values. Used to derive the on-wire `parameterLength` prefix from the data itself, -/// so the declared length can never disagree with the value it describes. +/// values. Derives the on-wire `parameterLength` prefix from the data itself, so the +/// declared length can never disagree with the value it describes. fn size_param_width(a: u128, b: u128) -> usize { let a = minimal_be_len(a); let b = minimal_be_len(b); @@ -66,8 +66,7 @@ impl From for u8 { } /// Total, because every byte classifies: Table 485 defines `0x01` to `0x06` and everything -/// else is `ISOSAEReserved`, which this enum represents rather than rejects. This was a -/// `TryFrom` whose every arm returned `Ok`, i.e. a fallible signature that could not fail. +/// else is `ISOSAEReserved`, which this enum represents rather than rejects. impl From for FileOperationMode { fn from(value: u8) -> Self { match value { @@ -90,8 +89,7 @@ impl PartialEq for FileOperationMode { } } -/// Holds the sizes of the file to be transferred (if applicable) -/// Used for both [`RequestFileTransferRequest`] and [`RequestFileTransferResponse`] +/// The sizes of the file to be transferred, carried by the request modes marked below. /// /// | | [AddFile] | [DeleteFile] | [ReplaceFile] | [ReadFile] | [ReadDir] | [ResumeFile] | /// |--------------|-----------|--------------|---------------|------------|-----------|--------------| @@ -140,11 +138,9 @@ impl SizePayload { } } -/// Payload used for all [`RequestFileTransferRequest`] requests. +/// The file or directory name every [`RequestFileTransferRequest`] carries, borrowed from +/// the caller. /// -/// Borrows `file_path_and_name` from the caller. -/// -/// #### ***Request*** Message /// | | [AddFile] | [DeleteFile] | [ReplaceFile] | [ReadFile] | [ReadDir] | [ResumeFile] | /// |---------------|-----------|--------------|---------------|------------|-----------|--------------| /// |**[Request]** | Yes | Yes | Yes | Yes | Yes | Yes | @@ -179,18 +175,12 @@ pub struct NamePayload<'a> { impl<'a> NamePayload<'a> { /// Build a name payload. The on-wire length prefix is computed from - /// `file_path_and_name` at encode time. - /// - /// The `modeOfOperation` byte is **not** part of this payload: it is the - /// [`RequestFileTransferRequest`] variant, which writes it. Carrying it here as well - /// let the two disagree — and the field won, so `AddFile(NamePayload::new(DeleteFile, - /// ..), dfi, size)` silently encoded a `DeleteFile` request and dropped the format - /// identifier and both sizes. + /// `file_path_and_name` at encode time. The `modeOfOperation` byte is not part of this + /// payload: the [`RequestFileTransferRequest`] variant is that byte and writes it. /// /// # Errors /// Returns [`Error::IncorrectMessageLengthOrInvalidFormat`] if the name is longer than - /// `u16::MAX` bytes, which is the widest the on-wire `filePathAndNameLength` field can - /// declare. This is the invariant that earns this type its `#[non_exhaustive]`. + /// `u16::MAX` bytes, the widest the on-wire `filePathAndNameLength` field can declare. pub const fn new(file_path_and_name: &'a str) -> Result { if file_path_and_name.len() > u16::MAX as usize { return Err(Error::IncorrectMessageLengthOrInvalidFormat); @@ -205,20 +195,14 @@ impl<'a> NamePayload<'a> { } } -/// A request to the server to transfer a file, either upload or download. -/// -/// Capabilities: -/// * Receive information about the file system on the server -/// * Send/Receive files to/from the server -/// -/// Available as an alternative to [`RequestDownloadRequest`](crate::RequestDownloadRequest) -/// and `RequestUploadRequest` if the server implements a file system for data storage +/// A request to the server to transfer a file in either direction, or to read or change +/// its file system. /// -/// Use [`crate::UdsServiceType::TransferData`] to send the file data to the server and -/// [`crate::UdsServiceType::RequestTransferExit`] to end the transfer -/// -/// If this service is used to delete files or directories on the server, there is no need -/// to use the `TransferData` or [`crate::UdsServiceType::RequestTransferExit`] services. +/// An alternative to [`RequestDownloadRequest`](crate::RequestDownloadRequest) and +/// [`RequestUploadRequest`](crate::RequestUploadRequest) for a server with a file system. +/// The file data then moves with [`crate::UdsServiceType::TransferData`] and the transfer +/// ends with [`crate::UdsServiceType::RequestTransferExit`]; a `DeleteFile` request needs +/// neither. #[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))] #[cfg_attr(feature = "utoipa", derive(utoipa::ToSchema))] #[derive(Clone, Copy, Debug, Eq, PartialEq)] @@ -298,8 +282,8 @@ impl RequestFileTransferRequest<'_> { ///////////////////////////////////////// - Response - /////////////////////////////////////////////////// -/// Sent by the server to inform the client of the maximum number of bytes to include in -/// each `TransferData` request message +/// The maximum number of bytes to include in each `TransferData` message, sent by the +/// server in the response modes marked below. /// /// | | [AddFile] | [DeleteFile] | [ReplaceFile] | [ReadFile] | [ReadDir] | [ResumeFile] | /// |---------------|-----------|--------------|---------------|------------|-----------|--------------| @@ -317,27 +301,11 @@ impl RequestFileTransferRequest<'_> { #[derive(Clone, Copy, Debug, Eq, PartialEq)] #[non_exhaustive] pub struct SentDataPayload<'a> { - /// This parameter is used by the requestFileTransfer positive response message to - /// inform the client how many data bytes (maxNumberOfBlockLength) to include in each - /// `TransferData` request message from the client or how many data bytes the server - /// will include in a `TransferData` positive response when uploading data. This length - /// reflects the complete message length, including the service identifier and the data - /// parameters present in the `TransferData` request message or positive response - /// message. This parameter allows either the client to adapt to the receive buffer size - /// of the server before it starts transferring data to the server or to indicate how - /// many data bytes will be included in each `TransferData` positive response in the - /// event that data is uploaded. A server is required to accept transferData requests - /// that are equal in length to its reported maxNumberOfBlockLength. It is server - /// specific what transferData request lengths less than maxNumberOfBlockLength are - /// accepted (if any). - /// - /// NOTE The last transferData request within a given block can be required to be less - /// than maxNumberOfBlockLength. It is not allowed for a server to write additional data - /// bytes (i.e. pad bytes) not contained within the transferData message (either in a - /// compressed or uncompressed format), as this would affect the memory address of where - /// the subsequent transferData request data would be written. If the modeOfOperation - /// parameter equals to 0x02 (`DeleteFile`) this parameter shall be not be included in - /// the response message. + /// `maxNumberOfBlockLength`: how many bytes each `TransferData` message carries, + /// counting its service identifier and data parameters, whether the client sends them + /// (download) or the server does (upload). A server accepts requests of exactly this + /// length; whether it accepts shorter ones is server specific, and the last message of + /// a block may have to be shorter. Absent for `DeleteFile`. /// /// Private because the `lengthFormatIdentifier` that declares its length is one byte /// wide, which bounds it; read it back with @@ -358,10 +326,7 @@ impl<'a> SentDataPayload<'a> { /// /// # Errors /// Returns [`Error::IncorrectMessageLengthOrInvalidFormat`] if the slice is longer than - /// 255 bytes. The wire field that carries its length is one byte, so a longer slice was - /// constructible here and then failed in `encode` — the same construct-then-fail - /// asymmetry that `RequestDownloadResponse::new` was made fallible to close. This is - /// the invariant that earns this type its `#[non_exhaustive]`. + /// 255 bytes, the widest the one-byte wire field that carries its length can declare. pub const fn new(max_number_of_block_length: &'a [u8]) -> Result { if max_number_of_block_length.len() > u8::MAX as usize { return Err(Error::IncorrectMessageLengthOrInvalidFormat); @@ -379,7 +344,7 @@ impl<'a> SentDataPayload<'a> { } } -/// Used to inform the client of the size of the file to be transferred +/// The size of the file the server is about to send. /// /// | | [AddFile] | [DeleteFile] | [ReplaceFile] | [ReadFile] | [ReadDir] | [ResumeFile] | /// |---------------|-----------|--------------|---------------|------------|-----------|--------------| @@ -420,7 +385,7 @@ impl FileSizePayload { } } -/// Used to inform the client of the size of the directory to be transferred +/// The size of the directory information the server is about to send. /// /// | | [AddFile] | [DeleteFile] | [ReplaceFile] | [ReadFile] | [ReadDir] | [ResumeFile] | /// |---------------|-----------|--------------|---------------|------------|-----------|--------------| @@ -455,8 +420,7 @@ impl DirSizePayload { } } -/// Used to inform the client of the byte position within the file at which the Tester will -/// resume downloading after an initial download is suspended +/// The byte position within the file at which the client resumes a suspended download. /// /// | | [AddFile] | [DeleteFile] | [ReplaceFile] | [ReadFile] | [ReadDir] | [ResumeFile] | /// |---------------|-----------|--------------|---------------|------------|-----------|--------------| @@ -473,20 +437,13 @@ impl DirSizePayload { #[cfg_attr(feature = "utoipa", derive(utoipa::ToSchema))] #[derive(Clone, Copy, Debug, Eq, PartialEq)] pub struct PositionPayload { - /// Specifies the byte position within the file at which the Tester will resume - /// downloading after an initial download is suspended A download is suspended when the - /// ECU stops receiving [`crate::TransferDataRequest`] requests and does not receive the - /// `RequestTransferExit` request to end the transfer before returning to the default - /// session + /// The byte position within the file at which the client resumes downloading. A + /// download is suspended when the server stops receiving + /// [`crate::TransferDataRequest`]s and returns to the default session without a + /// `RequestTransferExit`. /// - /// Fixed size: 8 bytes - /// - /// Not included for [`AddFile`][FileOperationMode::AddFile], - /// [`DeleteFile`][FileOperationMode::DeleteFile], - /// [`ReplaceFile`][FileOperationMode::ReplaceFile], - /// [`ReadFile`][FileOperationMode::ReadFile], or - /// [`ReadDir`][FileOperationMode::ReadDir] Only present if `mode_of_operation` is - /// [`ResumeFile`][FileOperationMode::ResumeFile] (for ISO 14229-1:2020) + /// Eight bytes on the wire. Present only for + /// [`ResumeFile`][FileOperationMode::ResumeFile]. pub file_position: u64, } @@ -498,10 +455,8 @@ impl PositionPayload { } } -/// Response to a [`RequestFileTransferRequest`] from the server -/// -/// The server will respond with a [`RequestFileTransferResponse`] to indicate the status of -/// the request `DataFormatIdentifier` - Echoes the value of the request +/// Positive response to a [`RequestFileTransferRequest`]. Each variant echoes the +/// request's `modeOfOperation` and, where one was sent, its `DataFormatIdentifier`. #[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))] #[cfg_attr(feature = "utoipa", derive(utoipa::ToSchema))] #[derive(Clone, Copy, Debug, Eq, PartialEq)] @@ -867,10 +822,9 @@ impl<'a> Decode<'a> for RequestFileTransferRequest<'a> { FileOperationMode::AddFile => Self::AddFile(name, dfi, size), FileOperationMode::ReplaceFile => Self::ReplaceFile(name, dfi, size), FileOperationMode::ResumeFile => Self::ResumeFile(name, dfi, size), - // The outer arm admits only the three above. Kept as a panic - // rather than folded into one of them: a variant added to the - // outer pattern later would otherwise decode as the wrong file - // operation, silently, on wire data. + // The outer arm admits only the three above. A panic rather than a + // fallback to one of them, so a variant added to the outer pattern + // cannot decode as the wrong file operation on wire data. #[expect(clippy::unreachable, reason = "narrowed by the outer arm")] _ => unreachable!(), }; @@ -1005,8 +959,7 @@ mod request_tests { fn test_allowed_nack_codes() { // ISO 14229-1:2020 Table 484 lists exactly these seven codes. Pinned as a set // rather than spot-checked, because a `contains` assertion cannot notice a missing - // code -- and three were missing, including the requestSequenceError that Table 484 - // defines specifically for ResumeFile on an already-complete transfer. + // code. let codes = RequestFileTransferRequest::allowed_nack_codes(); let mut bytes = [0u8; 7]; assert_eq!(codes.len(), bytes.len(), "wrong number of codes: {codes:?}"); @@ -1064,9 +1017,8 @@ mod request_tests { #[test] fn size_payload_decode_rejects_overwide_width() { // A wire-declared parameter length n > 16 is a data error, surfaced as - // Error::InvalidWidth (NRC 0x13) in every build profile — never a panic - // (regression guard for automotive-wire-codec SE-1). The buffer is long - // enough (1 + 2*17) that width, not truncation, is the only fault. + // Error::InvalidWidth (NRC 0x13) in every build profile, never a panic. The buffer + // is long enough (1 + 2*17) that width, not truncation, is the only fault. let wire = [17u8; 1 + 2 * 17]; let err = SizePayload::decode(&wire).unwrap_err(); assert!(matches!(err, Error::InvalidWidth(w) if w.got == 17 && w.max == 16)); @@ -1074,8 +1026,7 @@ mod request_tests { #[test] fn size_payload_derives_minimal_width_from_data() { - // Small values must serialize with a 1-byte width derived from the data, - // not a caller-supplied length that could disagree with the value. + // Small values serialize with a 1-byte width derived from the data. let s = SizePayload::new(0x12, 0x34); let mut buf = [0u8; 8]; let written = @@ -1086,11 +1037,8 @@ mod request_tests { #[test] fn the_mode_byte_comes_from_the_variant() { - // `NamePayload` used to carry its own `mode_of_operation`, and the field won over - // the variant: an `AddFile` request built with a `DeleteFile` payload encoded as - // DeleteFile and silently dropped the format identifier and both sizes. With the - // field gone the variant is the single source of the byte, so that state is - // unrepresentable. + // The variant is the single source of the mode byte, so a request cannot carry a + // mode that disagrees with its payload. let req = RequestFileTransferRequest::AddFile( name_payload("/a"), DataFormatIdentifier::NONE, @@ -1131,7 +1079,7 @@ mod request_tests { let written = Encode::encode(&n, &mut automotive_wire_codec::SliceSink::new(&mut buf)) .unwrap(); - // length=0x0003, "abc" — the modeOfOperation byte belongs to the request, not here. + // length=0x0003, "abc"; the modeOfOperation byte belongs to the request, not here. assert_eq!(&buf[..written], &[0x00, 0x03, b'a', b'b', b'c']); } @@ -1320,7 +1268,7 @@ mod response_tests { fn derive_contract() { use crate::test_util::assert_impl_eq; - // Verify all nine types implement Eq + // All nine types implement Eq. assert_impl_eq::(); assert_impl_eq::(); assert_impl_eq::>(); diff --git a/crates/uds_protocol/src/services/routine_control.rs b/crates/uds_protocol/src/services/routine_control.rs index 7446400b..94b932b1 100644 --- a/crates/uds_protocol/src/services/routine_control.rs +++ b/crates/uds_protocol/src/services/routine_control.rs @@ -1,9 +1,8 @@ -//! Routine Control (0x31) Service is used to perform functions on the ECU that may not be -//! covered by other services. +//! `RoutineControl` (0x31) service implementation. //! -//! It can also be used to check the ECU's health, erase memory, or other custom -//! manufacturer/supplier routines. However, some routines may have side effects or require -//! certain preconditions to be met. +//! A routine is a server-resident function the client starts, stops or reads results from: +//! a health check, a memory erase, or a manufacturer- or supplier-defined procedure. A +//! routine may have preconditions, such as a session or an unlocked security level. use crate::shared::SuppressablePositiveResponse; use crate::{Decode, Encode, Error, Incomplete, NegativeResponseCode}; use automotive_wire_codec::{write_bytes, write_u8, write_u16_be}; @@ -23,9 +22,8 @@ pub enum RoutineControlSubFunction { /// the server using [`crate::SecurityAccessRequest`] before starting the routine. StartRoutine, - /// The server routine shall be stopped in the server's memory sometime between the - /// completion of the `StopRoutine` request and the completion of the 1st response - /// message which indicates that the routine has already been stopped, or is in progress + /// Stop the routine. The server has stopped it by the time it completes its first + /// response, which reports the routine stopped or stopping. StopRoutine, /// Request results for the specified routineIdentifier @@ -88,8 +86,7 @@ const ROUTINE_CONTROL_NEGATIVE_RESPONSE_CODES: [NegativeResponseCode; 7] = [ NegativeResponseCode::GeneralProgrammingFailure, ]; -/// Used by a client to execute a defined sequence of events and obtain any relevant -/// results. +/// Request for the server to start or stop a routine, or report its results. /// /// The 2-byte big-endian routine identifier is decoded into a typed `u16`, followed by /// optional routine input parameters in `option_record`. @@ -190,18 +187,14 @@ pub struct RoutineControlResponse<'d> { pub sub_function: RoutineControlSubFunction, /// The 16-bit routine identifier echoed from the request. pub routine_id: u16, - /// Everything the server sent after the routine identifier — bytes `#5` onward of - /// ISO 14229-1:2020 Table 428. May be empty. + /// Everything the server sent after the routine identifier: bytes `#5` onward of ISO + /// 14229-1:2020 Table 428. May be empty. /// - /// Note that this is **not** only the `routineStatusRecord`. Table 428 places an - /// optional `routineInfo` byte at `#5`, immediately before the status record, and - /// whether it is present is vehicle-manufacturer defined — nothing on the wire - /// distinguishes the two layouts, so this crate cannot split them for you. If the - /// server you are talking to sends `routineInfo`, it is the first byte here and the - /// status record starts at index 1. - /// - /// `routineInfo` itself is vehicle-manufacturer specific (Table 429); it exists so - /// generic test equipment can handle all implemented routines uniformly. + /// This is **not** only the `routineStatusRecord`. Table 428 places an optional, + /// vehicle-manufacturer defined `routineInfo` byte at `#5`, immediately before the + /// status record, and nothing on the wire distinguishes the two layouts, so this crate + /// cannot split them. If the server sends `routineInfo`, it is the first byte here and + /// the status record starts at index 1. #[cfg_attr(feature = "serde", serde(borrow))] pub status_record: &'d [u8], } @@ -290,8 +283,7 @@ mod test { // ISO 14229-1:2020 Table 428 puts an optional `routineInfo` byte at #5, before the // routineStatusRecord, with presence left to the vehicle manufacturer. Nothing on // the wire distinguishes "routineInfo + status" from "status only", so - // `status_record` deliberately spans both and the field docs say so. Pinned here - // because a future reader might otherwise "fix" it into a wrong split. + // `status_record` spans both. let wire = [0x71, 0x01, 0xF0, 0x0F, 0xAA, 0x32]; let (resp, _) = crate::Response::decode(&wire).unwrap(); let crate::Response::RoutineControl(inner) = resp else { @@ -301,7 +293,7 @@ mod test { assert_eq!( inner.status_record, &[0xAA, 0x32], - "routineInfo must stay in the slice rather than being silently dropped" + "routineInfo is part of the slice" ); let mut buf = [0u8; 8]; @@ -310,11 +302,10 @@ mod test { } #[test] - fn a_reserved_sub_function_decodes_so_the_routine_checks_can_come_first() { + fn reserved_routine_control_type_decodes() { // ISO 14229-1:2020 Table 426 defines only 0x01-0x03; 0x00 and 0x04-0x7F are // ISOSAEReserved. Figure 30 answers one 0x12, but only after the routine - // identifier's 0x31 and 0x33 checks, so the request has to decode: rejecting it - // here left a server no choice but to answer before those checks. It re-encodes + // identifier's 0x31 and 0x33 checks, so the request has to decode. It re-encodes // to the byte it came from, with the suppress bit kept apart. for byte in [0x00u8, 0x04, 0x10, 0x7F] { let request = [byte | 0x80, 0xF0, 0x0F]; @@ -331,7 +322,7 @@ mod test { } #[test] - fn a_sub_function_byte_with_bit_7_is_answered_with_sub_function_not_supported() { + fn bit_7_in_a_routine_control_type_maps_to_0x12() { // Bit 7 is the suppressPosRspMsgIndicationBit, so it never names a // routineControlType; `try_from` on a byte carrying it is the error, and the error // maps to 0x12 as Table 430 requires. diff --git a/crates/uds_protocol/src/services/security_access.rs b/crates/uds_protocol/src/services/security_access.rs index 4a82510f..b71190f0 100644 --- a/crates/uds_protocol/src/services/security_access.rs +++ b/crates/uds_protocol/src/services/security_access.rs @@ -63,15 +63,11 @@ impl From for u8 { } } -/// Security Access Type allows for multiple different security challenges within an ECU. +/// The `securityAccessType` sub-function: whether a seed is requested or a key sent, and +/// for which security level (ISO 14229-1:2020 Table 42). /// -/// The Security Access Type is used to determine both the sub function, -/// as well as ECU specific access type being requested -/// -/// *Note*: -/// -/// Conversions from `u8` to `SecurityAccessType` are fallible and will return an [`Error`] -/// if the Suppress Positive Response bit is set. +/// Conversion from `u8` is fallible and returns an [`Error`] if bit 7, the suppress +/// positive response bit, is set. #[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))] #[cfg_attr(feature = "serde", serde(try_from = "u8", into = "u8"))] #[derive(Clone, Copy, Debug, Eq, PartialEq)] @@ -88,12 +84,12 @@ pub enum SecurityAccessType { /// `sendKey` for a vehicle-manufacturer security level; encodes /// [`SecurityAccessLevel::send_key`]. SendKey(SecurityAccessLevel), - /// `RequestSeed` with different levels of security defined for end of life - /// activation of on-board pyrotechnic devices + /// `requestSeed` for the ISO 26021-2 security levels, which guard the end-of-life + /// deployment of on-board pyrotechnic devices (`0x5F`, Table 42). Iso26021_2Values, - /// `SendKey` with different levels of security defined for end of life activation + /// `sendKey` for the ISO 26021-2 security levels (`0x60`, Table 42). Iso26021_2SendKeyValues, - /// This range of values is reserved for system supplier specific use + /// A byte Table 42 sets aside for system suppliers. /// /// Construct through [`SecurityAccessType::try_from`] so the raw byte is range-checked /// and can never collide with the SPRMIB bit. @@ -153,7 +149,7 @@ mod security_access_type_tests { 0x1E, 0x20, 0x22, 0x24, 0x26, 0x28, 0x2A, 0x2C, 0x2E, 0x30, 0x32, 0x34, 0x36, 0x38, 0x3A, 0x3C, 0x3E, 0x40, 0x42, ]; - /// Check that we properly decode and encode hex bytes + /// Every byte classifies as Table 42 assigns it, or fails for bit 7. #[test] fn security_access_type_from_all_u8_values() { assert_eq!( @@ -229,7 +225,7 @@ mod security_access_type_tests { } /// A send-key built from a level encodes that level's even byte, so it cannot be - /// mistaken for the seed request; the review's probe sent `27 01` for "send key 1". + /// mistaken for the seed request. #[test] fn seed_and_key_for_one_level_encode_the_pair() { let level = SecurityAccessLevel::from_request_seed(0x01).unwrap(); @@ -266,26 +262,11 @@ const SECURITY_ACCESS_NEGATIVE_RESPONSE_CODES: [NegativeResponseCode; 8] = [ NegativeResponseCode::RequiredTimeDelayNotExpired, ]; -/// Client request to access a security level -/// -/// This service supports two primary types of request: -/// -/// ## Request Seed -/// -/// When requesting a seed, the request data represents implementation defined -/// `SecurityAccessDataRecord` values. -/// This data is optional, and its use is implementation defined. -/// Suppressing a positive response to this request is not supported. -/// -/// ## Send Key -/// -/// When sending a key, the request data represents the key to be sent. After receiving a -/// seed, the client must calculate the corresponding key and send it to the server. The -/// server will then validate the key and respond with a positive or negative response. -/// Successful verification of the key will result in the server unlocking the requested -/// security level. Suppressing a positive response to this request is allowed. +/// Client request to unlock a security level. Borrows its data from the caller. /// -/// Zero-alloc request for security access. Borrows from the caller. +/// A `requestSeed` carries an optional, implementation-defined `securityAccessDataRecord`; +/// the server answers with a seed. A `sendKey` carries the key the client computed from +/// that seed; a positive response means the level is unlocked. #[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))] #[cfg_attr(feature = "utoipa", derive(utoipa::ToSchema))] #[derive(Clone, Copy, Debug, Eq, PartialEq)] @@ -481,8 +462,7 @@ mod request { let written = Encode::encode(&req, &mut automotive_wire_codec::SliceSink::new(&mut buf)) .unwrap(); - // The bytes, not just the count: the two halves of this test were otherwise - // disconnected, so swapping what `encode` writes went unnoticed. + // The bytes, not just the count, so a swap of what `encode` writes fails. assert_eq!(&buf[..written], &bytes); assert_eq!(written, req.encoded_size().unwrap()); assert_encode_size_agrees(&req); diff --git a/crates/uds_protocol/src/services/tester_present.rs b/crates/uds_protocol/src/services/tester_present.rs index 2293f895..75941032 100644 --- a/crates/uds_protocol/src/services/tester_present.rs +++ b/crates/uds_protocol/src/services/tester_present.rs @@ -15,11 +15,8 @@ const NO_SUBFUNCTION_VALUE: u8 = 0x00; /// The range of values is only 7 of the 8 bits, with bit 7 being used as the /// Suppress Positive Response (SPR) Message Indication Bit. // -// `serde(try_from = "u8", into = "u8")` is what keeps a deserialized value inside -// 0x00..=0x7F: it routes through the `TryFrom` below, the same classifier `decode` -// uses. The two public types then just rename the field, so no mirror struct is needed to -// enforce the range -- the invariant is single-field, and `serde`/`utoipa` can both express -// that directly. +// `serde(try_from = "u8", into = "u8")` keeps a deserialized value inside 0x00..=0x7F by +// routing it through the `TryFrom` below, the same classifier `decode` uses. #[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))] #[cfg_attr(feature = "serde", serde(try_from = "u8", into = "u8"))] #[derive(Clone, Copy, Debug, Eq, PartialEq)] @@ -80,9 +77,8 @@ pub struct TesterPresentRequest { /// /// Serialized as `sub_function`: a byte in `0x00..=0x7F`. // - // The `rename` plus `value_type` is what keeps the private enum out of both the wire - // format and the schema. Two mirror structs used to do this; the invariant is - // single-field, so a field attribute expresses it directly. + // The `rename` plus `value_type` keeps the private enum out of both the wire format + // and the schema. #[cfg_attr(feature = "serde", serde(rename = "sub_function"))] #[cfg_attr( feature = "utoipa", @@ -146,9 +142,9 @@ impl<'a> Decode<'a> for TesterPresentRequest { available: buf.len(), })); } - // Split out the SPRMIB flag. Once SPRMIB is stripped the low 7 bits are always a - // valid zero sub-function, so this never rejects; the sub-function value is - // retained so that a reserved byte re-encodes unchanged. + // Once SPRMIB is stripped the low 7 bits are always a valid zero sub-function, so + // this never rejects; the value is retained so a reserved byte re-encodes + // unchanged. let sub_function = SuppressablePositiveResponse::::try_from(buf[0])?; Ok(( @@ -309,11 +305,8 @@ mod test { #[test] fn reserved_sub_function_survives_a_round_trip() { - // Reserved sub-function bytes must re-encode byte-for-byte. Previously the value - // was discarded and normalized to 0x00, so `[0x3E, 0x01]` came back out as `[0x3E, - // 0x00]` — the request silently rewrote the tester's frame, while - // `TesterPresentResponse` preserved the same values. A server needs the original - // byte to report subFunctionNotSupported against it. + // Reserved sub-function bytes re-encode byte-for-byte, as the response's do: a + // server needs the original byte to report subFunctionNotSupported against it. for raw in [0x00u8, 0x01, 0x42, 0x7F] { for suppress in [false, true] { let wire = [raw | if suppress { 0x80 } else { 0x00 }]; @@ -346,7 +339,7 @@ mod test { #[test] fn new_still_produces_the_zero_sub_function() { - // The common case keeps its one-argument constructor and its 0x00 encoding. + // The one-argument constructor builds the zero sub-function. assert_eq!(TesterPresentRequest::new(false).sub_function(), 0x00); let mut buf = [0u8; 4]; let n = Encode::encode( diff --git a/crates/uds_protocol/src/services/transfer_data.rs b/crates/uds_protocol/src/services/transfer_data.rs index 99988144..e6892302 100644 --- a/crates/uds_protocol/src/services/transfer_data.rs +++ b/crates/uds_protocol/src/services/transfer_data.rs @@ -12,36 +12,20 @@ const TRANSFER_DATA_NEGATIVE_RESPONSE_CODES: [NegativeResponseCode; 6] = [ NegativeResponseCode::WrongBlockSequenceCounter, ]; -/// A request to the server to transfer data (either upload or download) +/// One block of a transfer in either direction. Borrows its data from the caller. /// -/// Step 1: The client sends a `RequestDownload` or `RequestUpload` message to the server -/// 34 .. 11 .. 33 .. 60 20 00 .. 00 FF FF << -- Bytes sent by the client -/// RID .. DFI .. ALFID .. `MA_B`# .. `UCMS_B`# -/// -/// Step 1 Response: The server sends a -/// [`RequestDownloadResponse`](crate::RequestDownloadResponse) or `RequestUploadResponse` -/// message to the client -/// -/// Step 2: The client shall send many [`TransferDataRequest`] messages written in blocks to -/// the server with a max number of bytes equal to `MNROB_B`# from the -/// `RequestDownloadResponse` message 74 .. 20 .. 00 81 RSID .. LFID .. `MNROB_B`# -/// -/// Step 2 Response: The server sends a [`TransferDataResponse`] message confirming the -/// block sequence -/// -/// Step 3: The client sends a [`crate::UdsServiceType::RequestTransferExit`] message to the -/// server (SID 0x37) -/// -/// Step 3 Response: The server sends a [`crate::UdsServiceType::RequestTransferExit`] -/// response message to the client (RID 0x77) -/// -/// Zero-alloc request to transfer data. Borrows from the caller. +/// A transfer is three services in sequence: a `RequestDownload` or `RequestUpload`, whose +/// positive response ([`RequestDownloadResponse`](crate::RequestDownloadResponse), +/// [`RequestUploadResponse`](crate::RequestUploadResponse)) states the +/// `maxNumberOfBlockLength`; then one [`TransferDataRequest`] per block, each answered by +/// a [`TransferDataResponse`] echoing its counter; then a +/// [`RequestTransferExitRequest`](crate::RequestTransferExitRequest). #[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))] #[cfg_attr(feature = "utoipa", derive(utoipa::ToSchema))] #[derive(Clone, Copy, Debug, Eq, PartialEq)] #[non_exhaustive] pub struct TransferDataRequest<'d> { - /// Block sequence counter (wraps 0xFF → 0x00). + /// Block sequence counter, which wraps from 0xFF to 0x00. pub block_sequence_counter: u8, /// The data to be transferred. #[cfg_attr(feature = "serde", serde(borrow))] diff --git a/crates/uds_protocol/src/services/upload_download.rs b/crates/uds_protocol/src/services/upload_download.rs index baae1283..e429ee49 100644 --- a/crates/uds_protocol/src/services/upload_download.rs +++ b/crates/uds_protocol/src/services/upload_download.rs @@ -1,12 +1,11 @@ //! `RequestDownload` (0x34) and `RequestUpload` (0x35) service implementations. //! -//! ISO 14229-1 gives the two services identical message layouts — request: +//! ISO 14229-1 gives the two services identical message layouts (request: //! `dataFormatIdentifier`, `addressAndLengthFormatIdentifier`, `memoryAddress`, -//! `memorySize`; positive response: `lengthFormatIdentifier`, `maxNumberOfBlockLength` — +//! `memorySize`; positive response: `lengthFormatIdentifier`, `maxNumberOfBlockLength`), //! differing only in service identifier and in which direction the subsequent //! `TransferData` sequence moves the bytes. Both pairs are generated from one macro so the -//! wire codec has a single source of truth; a fix to the width-derivation logic cannot land -//! on one service and miss the other. +//! wire codec has a single source of truth. use crate::shared::{ AddressAndLengthFormatIdentifier, DataFormatIdentifier, LengthFormatIdentifier, @@ -27,9 +26,8 @@ const REQUEST_DOWNLOAD_NEGATIVE_RESPONSE_CODES: [NegativeResponseCode; 6] = [ NegativeResponseCode::UploadDownloadNotAccepted, ]; -/// Permitted NRCs for `RequestUpload` (0x35). ISO 14229-1 specifies the same set as -/// `RequestDownload`; kept as a separate constant so either service can diverge later -/// without silently changing the other. +/// Permitted NRCs for `RequestUpload` (0x35): the same set as `RequestDownload`, as a +/// separate constant because each service's table is its own. const REQUEST_UPLOAD_NEGATIVE_RESPONSE_CODES: [NegativeResponseCode; 6] = [ NegativeResponseCode::IncorrectMessageLengthOrInvalidFormat, NegativeResponseCode::ConditionsNotCorrect, @@ -103,19 +101,15 @@ macro_rules! upload_download_service { #[doc = concat!("Create a `", stringify!($req), "` with a caller-chosen")] /// `addressAndLengthFormatIdentifier`. /// - /// ISO 14229-1:2020 Table 441 makes the format identifier a client choice - /// rather than a function of the values, and clause 11.3.1 blesses "a **fixed** - /// addressAndLengthFormatIdentifier" with the unused bytes "padded with the - /// value 0x00". The standard's own examples are non-minimal — Table 462 - /// declares three `memorySize` bytes for `0x00FFFF`, which needs two — so - /// [`new`](Self::new), which always derives the narrowest widths, cannot - /// reproduce them. This can. + /// ISO 14229-1:2020 Table 441 makes the format identifier a client choice, and + /// clause 11.3.1 allows a fixed one with the unused bytes padded with `0x00`; + /// Table 462's example declares three `memorySize` bytes for `0x00FFFF`, which + /// [`new`](Self::new) would encode in two. /// /// # Errors /// Returns [`Error::InvalidMemoryAddress`] or [`Error::InvalidMemorySize`] if /// the value does not fit the width `alfid` declares for it, which would - /// otherwise truncate it on the wire. Both map to NRC `0x31`, as Tables 444 and - /// 449 require for a `memoryAddress`/`memorySize` that "is not valid". + /// truncate it on the wire. Both map to NRC `0x31` (Tables 444 and 449). pub const fn new_with_alfid( data_format_identifier: DataFormatIdentifier, alfid: AddressAndLengthFormatIdentifier, @@ -147,11 +141,9 @@ macro_rules! upload_download_service { self.address_and_length_format_identifier } - /// The compression and encryption methods the client asked the server to use. - /// - /// A server has to act on this, so it must be readable back off a decoded - /// request; use [`DataFormatIdentifier::compression_method`] and - /// [`DataFormatIdentifier::encryption_method`] for the individual nibbles. + /// The compression and encryption methods the client asked the server to use; + /// [`DataFormatIdentifier::compression_method`] and + /// [`DataFormatIdentifier::encryption_method`] give the individual nibbles. #[must_use] pub const fn data_format_identifier(&self) -> DataFormatIdentifier { self.data_format_identifier @@ -180,16 +172,12 @@ macro_rules! upload_download_service { /// memory size, plus the byte width each of the latter two is declared with on the /// wire. /// - /// A width that would truncate its value is rejected rather than silently - /// corrupting the address on the wire, and the crate-private - /// `addressAndLengthFormatIdentifier` that packs the two widths never surfaces - /// here. - /// - /// This doc comment is the published schema description for the request type, - /// because its hand-written `PartialSchema` delegates here. It deliberately uses - /// plain `///` lines: utoipa's derive reads only literal doc attributes, so a - /// `#[doc = concat!(..)]` naming the specific service is dropped from the schema -- - /// which is what left this description missing its verb. + /// A width that would truncate its value is rejected rather than corrupting the + /// address on the wire. + // + // This doc comment is the published schema description for the request type, whose + // hand-written `PartialSchema` delegates here. Plain `///` lines only: utoipa's + // derive reads literal doc attributes and drops a `#[doc = concat!(..)]`. #[cfg(feature = "serde")] #[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))] #[cfg_attr(feature = "utoipa", derive(utoipa::ToSchema))] @@ -240,8 +228,7 @@ macro_rules! upload_download_service { std::borrow::Cow::Borrowed(stringify!($req)) } - /// See the note on `CommunicationControlRequest::schemas`: a hand-written - /// `ToSchema` must forward this or its `$ref`s dangle. + /// Register the child schemas this one `$ref`s; without this they dangle. fn schemas( schemas: &mut Vec<( String, @@ -322,7 +309,7 @@ macro_rules! upload_download_service { #[non_exhaustive] pub struct $resp<'d> { /// The `maxNumberOfBlockLength` bytes, as sent. See the accessor of the same - /// name for what the value means for this service -- it differs between + /// name for what the value means for this service; it differs between /// download and upload. /// /// The on-wire `lengthFormatIdentifier` nibble is derived from this slice's @@ -407,8 +394,7 @@ macro_rules! upload_download_service { } } - // Both services get the same coverage, generated alongside them so neither can - // drift. + // Both services get the same coverage, generated alongside them. #[cfg(test)] mod $test_mod { use super::*; @@ -453,8 +439,8 @@ macro_rules! upload_download_service { #[test] fn zero_address_and_size_clamp_to_one_byte() { - // A 0 address/size must still produce a valid (>=1 byte) length nibble, - // otherwise the encoded frame cannot be decoded back. + // A 0 address/size produces a valid (>=1 byte) length nibble, so the + // encoded frame can be decoded back. let req = $req::new(0x00.into(), 0, 0); assert_eq!( req.address_and_length_format_identifier @@ -485,11 +471,10 @@ macro_rules! upload_download_service { #[test] fn new_derives_the_alfid_nibbles_in_wire_order() { - // The two nibbles are asymmetric here on purpose: the high nibble is the - // memorySize width and the low nibble the memoryAddress width, so - // transposing them changes these bytes. A symmetric case (or a length-only - // assertion) cannot tell the two apart, and a swap silently truncates the - // address. + // The two widths differ in each case: the high nibble is the memorySize + // width and the low nibble the memoryAddress width, so transposing them + // changes these bytes, which a symmetric case or a length-only assertion + // could not detect. for (address, size, want) in [ // 4-byte address, 1-byte size -> ALFID 0x14 ( @@ -509,7 +494,7 @@ macro_rules! upload_download_service { "wrong wire bytes for address {address:#X} size {size:#X}" ); - // ...and the frame the crate emits must be one the crate can read back. + // ...and the frame the crate emits is one the crate reads back. let decoded = <$req as Decode>::decode_exact(&buf[..written]).unwrap(); assert_eq!(decoded.memory_address(), address); assert_eq!(decoded.memory_size(), size); @@ -520,10 +505,8 @@ macro_rules! upload_download_service { fn explicit_widths_can_reproduce_the_spec_example_alfid() { // 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 the value 0x00FFFF, which needs only 2 - // -- so `new`, which always derives minimal widths, cannot produce that - // frame. Real bootloaders commonly mandate a fixed ALFID (often 0x44) and - // answer requestOutOfRange otherwise. + // declares 3 bytes of memorySize for the value 0x00FFFF, which needs only + // 2, so `new`, which derives minimal widths, cannot produce that frame. let req = $req::new_with_alfid( DataFormatIdentifier::from(0x11), AddressAndLengthFormatIdentifier::new(3, 3).unwrap(), @@ -566,8 +549,8 @@ macro_rules! upload_download_service { Err(Error::InvalidMemorySize(0x1_0000)) )); - // ...and the widths themselves must be 1 to 8. That check lives on the - // format identifier, so it cannot be reached with a bad width. + // ...and the widths themselves are 1 to 8. That check lives on the format + // identifier, so `new_with_alfid` cannot be reached with a bad width. assert!(AddressAndLengthFormatIdentifier::new(1, 0).is_err()); assert!(AddressAndLengthFormatIdentifier::new(1, 9).is_err()); assert!(AddressAndLengthFormatIdentifier::new(9, 1).is_err()); @@ -617,9 +600,8 @@ macro_rules! upload_download_service { #[test] fn a_reserved_length_nibble_is_zeroed_on_re_encode() { // ISO 14229-1:2020 Tables 443 and 448 both give the lengthFormatIdentifier - // the range 0x00 to 0xF0 and state that bits 3 to 0 are "reserved by - // document, to be set to '0'" -- "the lower nibble shall be set to '0'". So - // `74 25 08 00` is a malformed byte, not a value to preserve: the block + // the range 0x00 to 0xF0 and reserve bits 3 to 0, which are sent as zero. + // So `74 25 08 00` is a malformed byte, not a value to preserve: the block // length is still read from the high nibble, but re-encoding emits the // conformant `74 20 08 00`. let resp = <$resp as Decode>::decode_exact(&[0x25, 0x08, 0x00]).unwrap(); @@ -670,11 +652,10 @@ macro_rules! upload_download_service { #[test] fn data_format_identifier_is_readable_off_a_decoded_request() { - // A server has to act on the compression/encryption methods it was asked - // for. All fields are private (the width nibbles must stay in sync with the - // values), so without the getter the DFI was write-only: constructible but - // unreadable after a decode. Wire: DFI=0x21 (compression 2, encryption 1), - // ALFID=0x12 (size 1, addr 2), addr=0xBEEF, size=0x10 + // A server acts on the compression/encryption methods it was asked for, so + // the DFI is readable after a decode although every field is private. + // Wire: DFI=0x21 (compression 2, encryption 1), ALFID=0x12 (size 1, addr + // 2), addr=0xBEEF, size=0x10 let wire = [0x21, 0x12, 0xBE, 0xEF, 0x10]; let req = <$req as Decode>::decode_exact(&wire).unwrap(); let dfi = req.data_format_identifier(); @@ -687,7 +668,7 @@ macro_rules! upload_download_service { #[test] fn data_format_identifier_survives_construction() { - // new(compression, encryption) — wire order. + // new(compression, encryption): wire order. let dfi = DataFormatIdentifier::new(0x0A, 0x0B).unwrap(); let req = $req::new(dfi, 0x1234, 0x10); assert_eq!(req.data_format_identifier(), dfi); @@ -754,8 +735,7 @@ mod shared_layout_tests { #[test] fn download_and_upload_share_one_wire_layout() { // The two services are byte-identical apart from the service identifier, which is - // added by the `Request`/`Response` frame layer, not by these payload codecs. This - // pins that equivalence so the macro cannot silently drift for one of them. + // added by the `Request`/`Response` frame layer, not by these payload codecs. let wire = [0x00, 0x14, 0xF0, 0xFF, 0xFF, 0x67, 0x0A]; let down = ::decode_exact(&wire).unwrap(); let up = ::decode_exact(&wire).unwrap(); diff --git a/crates/uds_protocol/src/services/write_data_by_identifier.rs b/crates/uds_protocol/src/services/write_data_by_identifier.rs index 73daf265..2d1c354e 100644 --- a/crates/uds_protocol/src/services/write_data_by_identifier.rs +++ b/crates/uds_protocol/src/services/write_data_by_identifier.rs @@ -42,7 +42,7 @@ impl<'d> WriteDataByIdentifierRequest<'d> { /// # Errors /// Returns [`Error::IncorrectMessageLengthOrInvalidFormat`] if `data` is empty. ISO /// 14229-1:2020 Table 277 marks the first `dataRecord` byte mandatory, so a request - /// with no data record is malformed — and would encode to a frame this crate's own + /// with no data record is malformed and would encode to a frame this crate's own /// decoder rejects. pub const fn new(identifier: u16, data: &'d [u8]) -> Result { if data.is_empty() { @@ -221,8 +221,7 @@ mod test { fn wdbi_request_requires_at_least_one_data_byte() { // ISO 14229-1:2020 Table 277 marks dataRecord `data#1` as `M` (only `data#2..#m` // are `U`), and Figure 26's key states "minimum length is 4 byte (SI + DID + - // DREC)". A 3-byte message used to decode into an empty data record, so a server - // would attempt a zero-length write rather than answering NRC 0x13. + // DREC)", so a 3-byte message is answered NRC 0x13 rather than written. let err = ::decode(&[0xF1, 0x90]) .expect_err("a write with no data record must be rejected"); assert!( @@ -236,7 +235,7 @@ mod test { "expected a 3-byte shortfall, got {err:?}" ); - // ...and the constructor must not mint a request the decoder would reject. + // ...and the constructor does not mint a request the decoder would reject. assert!(WriteDataByIdentifierRequest::new(0xF190, &[]).is_err()); let req = WriteDataByIdentifierRequest::new(0xF190, &[0x01]).unwrap(); assert_eq!(req.data, &[0x01]); diff --git a/crates/uds_protocol/src/shared/bounded.rs b/crates/uds_protocol/src/shared/bounded.rs index 4bad331d..4c84497b 100644 --- a/crates/uds_protocol/src/shared/bounded.rs +++ b/crates/uds_protocol/src/shared/bounded.rs @@ -2,15 +2,9 @@ //! //! A few request and response fields are length-bounded because the wire field that //! declares their length is one or two bytes wide, or because ISO marks the first byte -//! mandatory. Their constructors enforce that, but `derive(Deserialize)` ignores -//! constructors — so without these, deserializing is a second way in that produces a value -//! `encode` rejects, or worse, one that encodes to a frame this crate's own decoder -//! refuses. -//! -//! These are field-level `deserialize_with` hooks rather than whole-type mirror structs: -//! every invariant here is a property of a single field, so there is nothing for an -//! aggregate to coordinate. Two mirror structs were used for a single-field invariant -//! elsewhere in this crate and cost about a hundred lines to say what one attribute says. +//! mandatory. Their constructors enforce that, and `derive(Deserialize)` bypasses +//! constructors, so these hooks enforce it on the way in. They are field-level because +//! each invariant is a property of a single field. use serde::{Deserialize, Deserializer, de::Error as _}; diff --git a/crates/uds_protocol/src/shared/byte_schema.rs b/crates/uds_protocol/src/shared/byte_schema.rs index ed88cc54..33db5513 100644 --- a/crates/uds_protocol/src/shared/byte_schema.rs +++ b/crates/uds_protocol/src/shared/byte_schema.rs @@ -1,19 +1,11 @@ //! `OpenAPI` schemas for the types that serialize as a single protocol byte or 16-bit //! identifier. //! -//! Several public types are one wire integer with a range invariant. Their `serde` impls -//! go through it (via `serde(try_from = "u8", into = "u8")`, `serde(from = "u16", into = -//! "u16")` and the like) so that deserializing cannot build a value their constructor -//! would reject. A -//! derived `ToSchema` would still describe the Rust shape — an object with nibble -//! properties, or a `oneOf` over variant names — and so disagree with what `serde` actually -//! reads and writes. These impls keep the two in step. -//! -//! Each schema carries the actual accepted range rather than `u8`'s bare `{"type": -//! "integer", "minimum": 0}`. Delegating to `` advertised `0..=2^31`, -//! so a generated client emitted an `i32` and discovered the real bound as a runtime -//! rejection. The whole reason these types exist is that their bytes have ranges; the -//! schema is the one place that has to say so. +//! Each such type's `serde` impl goes through the integer (`serde(try_from = "u8", into = +//! "u8")` and the like), so a derived `ToSchema`, which describes the Rust shape, would +//! disagree with what `serde` reads and writes. Each schema here is an integer carrying the +//! accepted range, so a generated client learns the bound from the document rather than +//! from a runtime rejection. /// Implement `PartialSchema`/`ToSchema` for a type whose serialized form is one integer. /// diff --git a/crates/uds_protocol/src/shared/diagnostic_identifier.rs b/crates/uds_protocol/src/shared/diagnostic_identifier.rs index a2d3064e..a8b32751 100644 --- a/crates/uds_protocol/src/shared/diagnostic_identifier.rs +++ b/crates/uds_protocol/src/shared/diagnostic_identifier.rs @@ -1,9 +1,9 @@ -//! DIDs are used to identify the data that is requested or sent in a diagnostic service. +//! Data and routine identifiers: the 16-bit values that name the data a diagnostic service +//! reads or writes, and the routines it controls. -/// C.1 DID - Diagnostic Data Identifier specified in ISO 14229-1 +/// A data identifier (DID), ISO 14229-1:2020 Annex C Table C.1. /// -/// The identifiers listed here are defined and should be implemented by the vehicle -/// manufacturer/system supplier. +/// Every `u16` classifies into a named identifier or the range it falls in. #[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))] #[cfg_attr(feature = "serde", serde(from = "u16", into = "u16"))] #[cfg_attr(feature = "clap", derive(clap::ValueEnum))] @@ -102,21 +102,17 @@ pub enum UdsIdentifier { VehicleManufacturerEcuSoftwareVersionNumber, /// System supplier identifier (`0xF18A`). SystemSupplierIdentifier, - /// This value shall be used to reference the ECU (server) manufacturing date. Record - /// data content and format shall be unsigned numeric, ASCII or BCD, and shall be - /// ordered as Year, Month, Day. + /// ECU manufacturing date (`0xF18B`): unsigned numeric, ASCII or BCD, ordered year, + /// month, day. EcuManufacturingDate, - /// Get the serial number of the ECU, format shall be server specific. + /// ECU serial number (`0xF18C`), in a server-specific format. EcuSerialNumber, - /// Request the supported functional units of the ECU. + /// Supported functional units (`0xF18D`). SupportedFunctionalUnits, - /// This value shall be used to reference the vehicle manufacturer order number for a - /// kit (assembled parts bought as a whole for production e.g. cockpit), when the spare - /// part number designates only the server (e.g. for aftersales). The record data - /// content and format shall be server specific and defined by the vehicle manufacturer. + /// Vehicle manufacturer order number for a kit (`0xF18E`), when the spare part number + /// designates only the server; server-specific content. VehicleManufacturerKitAssemblyPartNumber, - /// See 14229-1 C.1 for details on Regulation X information. - /// Recursive ASCII string + /// Regulation X software identification numbers (`0xF18F`); see Table C.1. RegulationXSoftwareIdentificationNumbers, /// Vehicle Identification Number (`0xF190`). Vin, @@ -136,8 +132,8 @@ pub enum UdsIdentifier { SystemNameOrEngineType, /// Repair shop or tester serial number (`0xF198`). RepairShopCodeOrTesterSerialNumber, - /// When the server was last programmed, the record data content and format shall be - /// unsigned numeric, ASCII or BCD, and shall be ordered as Year, Month, Day. + /// Programming date (`0xF199`): unsigned numeric, ASCII or BCD, ordered year, month, + /// day. ProgrammingDate, /// Calibration repair-shop code or calibration equipment serial number (`0xF19A`). CalibrationRepairShopCodeOrCalibrationEquipmentSerialNumber, @@ -149,7 +145,7 @@ pub enum UdsIdentifier { EcuInstallationDate, /// ODX file identifier (`0xF19E`). OdxFile, - /// Used to reference the entity data identifier for a secured data transfer + /// Entity data identifier for a secured data transfer (`0xF19F`). Entity, /// UDS version data (`0xFF00`). UdsVersionData, @@ -307,10 +303,10 @@ impl core::fmt::Debug for UdsIdentifier { } } -/// Standard UDS Routine Identifier for the `RoutineControl` (0x31, 0x71) service +/// A routine identifier for the `RoutineControl` (0x31, 0x71) service, ISO 14229-1:2020 +/// Annex F Table F.1. /// -/// Some services will be defined by the Vehicle manufacturer or a system supplier, -/// and they must be implemented by the tester system. +/// Every `u16` classifies into a named routine or the range it falls in. #[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))] #[cfg_attr(feature = "serde", serde(from = "u16", into = "u16"))] #[derive(Clone, Copy, Debug, Eq, PartialEq)] @@ -366,8 +362,7 @@ pub enum UdsRoutineIdentifier { CheckProgrammingDependencies, } -/// We know all values for the Routine Identifier, so we can implement `From` for -/// `UdsRoutineIdentifier` +/// Total: Table F.1 assigns every `u16` to a routine or a range. impl From for UdsRoutineIdentifier { fn from(value: u16) -> Self { match value { diff --git a/crates/uds_protocol/src/shared/format_identifiers.rs b/crates/uds_protocol/src/shared/format_identifiers.rs index 27f057b0..bd84236b 100644 --- a/crates/uds_protocol/src/shared/format_identifiers.rs +++ b/crates/uds_protocol/src/shared/format_identifiers.rs @@ -34,19 +34,15 @@ const ENCRYPTION_NIBBLE_MASK: u8 = LOW_NIBBLE_MASK; /// [`RequestDownloadRequest`](crate::RequestDownloadRequest) and /// [`RequestUploadRequest`](crate::RequestUploadRequest), which share one message layout. /// The high nibble is the `memorySize` width and the low nibble the `memoryAddress` width, -/// in bytes — so `0x44` declares four bytes each, and `0x12` one byte of size and two of +/// in bytes, so `0x44` declares four bytes each, and `0x12` one byte of size and two of /// address. /// /// ISO 14229-1:2020 Table 441 makes this a client choice rather than a function of the -/// values, so it is a parameter rather than something derived: clause 11.3.1 blesses "a -/// **fixed** addressAndLengthFormatIdentifier" with the unused bytes "padded with the value -/// 0x00", and the standard's own examples are non-minimal (Table 462 declares three -/// `memorySize` bytes for `0x00FFFF`, which needs two). Pass one to +/// values, and clause 11.3.1 allows a fixed identifier with the unused bytes padded with +/// `0x00`. Pass one to /// [`RequestDownloadRequest::new_with_alfid`](crate::RequestDownloadRequest::new_with_alfid); -/// [`new`](crate::RequestDownloadRequest::new) derives the minimal one for you. -/// -/// A bootloader that mandates a particular value states it as a byte, so build one the same -/// way: +/// [`new`](crate::RequestDownloadRequest::new) derives the narrowest one. A bootloader that +/// mandates a particular value states it as a byte, so build one the same way: /// /// ``` /// use uds_protocol::AddressAndLengthFormatIdentifier as Alfid; @@ -62,10 +58,6 @@ const ENCRYPTION_NIBBLE_MASK: u8 = LOW_NIBBLE_MASK; /// assert!(Alfid::try_from(0x88).is_ok()); /// assert!(Alfid::try_from(0x98).is_err()); /// ``` -/// -/// ISO 14229-1:2020 Annex H Table H.1 gives example values. Each width is 1 to 8 bytes -/// here: a zero nibble is "not applicable" there, and a wider one does not fit the `u64` -/// that carries an address or size. #[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))] #[cfg_attr(feature = "serde", serde(try_from = "u8", into = "u8"))] #[derive(Clone, Copy, Debug, Eq, PartialEq)] @@ -81,8 +73,7 @@ impl AddressAndLengthFormatIdentifier { /// /// Arguments are in **wire order**: `memory_size_length` is the high nibble, /// `memory_address_length` the low nibble. Both are `u8`, so the compiler cannot catch - /// a transposition — if you have the byte already, prefer `try_from`, which cannot be - /// transposed. + /// a transposition; if you have the byte already, prefer `try_from`. /// /// # Errors /// Returns [`Error::InvalidAddressAndLengthFormatIdentifier`] carrying the byte the @@ -96,7 +87,7 @@ impl AddressAndLengthFormatIdentifier { { // Report the byte, not the nibble, so the message matches what a tester // configures and what `try_from` would have been given. Nibbles wider than 4 - // bits are masked to keep the reported byte a faithful packing of the two. + // bits are masked so the reported byte is a packing of the two. return Err(Error::InvalidAddressAndLengthFormatIdentifier( ((memory_size_length & LOW_NIBBLE_MASK) << 4) | (memory_address_length & LOW_NIBBLE_MASK), @@ -209,21 +200,15 @@ impl From for u8 { } /// The compression method (high nibble) and encryption method (low nibble) a client asks -/// the server to use for a transfer. -/// -/// - `0x00` for both means no compression and no encryption, which is the default; prefer -/// [`DataFormatIdentifier::NONE`]. -/// - Values other than `0x00` are Vehicle Manufacturer specific according to -/// ISO-14229-1:2020. +/// the server to use for a transfer. `0x00` for both, [`DataFormatIdentifier::NONE`], +/// means neither; every other value is vehicle-manufacturer specific. /// /// Supplied to [`RequestDownloadRequest::new`](crate::RequestDownloadRequest::new) and -/// [`RequestUploadRequest::new`](crate::RequestUploadRequest::new), and read back with -/// [`RequestDownloadRequest::data_format_identifier`](crate::RequestDownloadRequest::data_format_identifier). -/// Also carried by the `AddFile`, `ReplaceFile`, `ReadFile` and `ResumeFile` variants of +/// [`RequestUploadRequest::new`](crate::RequestUploadRequest::new), read back with +/// [`RequestDownloadRequest::data_format_identifier`](crate::RequestDownloadRequest::data_format_identifier), +/// and carried by the `AddFile`, `ReplaceFile`, `ReadFile` and `ResumeFile` variants of /// [`RequestFileTransferRequest`](crate::RequestFileTransferRequest). Serializes as the -/// single wire byte it occupies. Going through the byte rather than the two private nibble -/// fields means `serde` cannot mint a nibble wider than `0x0F`, which `new` rejects — the -/// byte form simply has no such state to represent. +/// single wire byte it occupies, which has no state for a nibble wider than `0x0F`. #[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))] #[cfg_attr(feature = "serde", serde(from = "u8", into = "u8"))] #[derive(Clone, Copy, Debug, Eq, PartialEq)] @@ -235,8 +220,7 @@ pub struct DataFormatIdentifier { } impl DataFormatIdentifier { - /// No compression and no encryption — the `0x00` byte, and the overwhelmingly common - /// case. + /// No compression and no encryption: the `0x00` byte, and the common case. pub const NONE: Self = Self { compression_method: 0, encryption_method: 0, @@ -245,17 +229,14 @@ impl DataFormatIdentifier { /// Build a `DataFormatIdentifier` from its compression and encryption method nibbles. /// /// Arguments are in **wire order**: compression is the high nibble, encryption the low - /// nibble. `0x00` for both means no compression and no encryption — prefer - /// [`DataFormatIdentifier::NONE`] for that. Each value occupies a single nibble. - /// - /// Both parameters are `u8`, so the compiler cannot catch a transposition; if you are - /// converting a byte you already have, use `DataFormatIdentifier::from(byte)` instead. + /// nibble. `0x00` for both means no compression and no encryption; prefer + /// [`DataFormatIdentifier::NONE`] for that. Both parameters are `u8`, so the compiler + /// cannot catch a transposition; for a byte you already have, use + /// `DataFormatIdentifier::from(byte)` instead. /// /// # Errors /// Returns [`Error::InvalidEncryptionCompressionMethod`] if either value does not fit - /// in a nibble (i.e. is greater than `0x0F`). Written as explicit range checks rather - /// than a `?` on a helper: `?` is not permitted in a `const fn`, and const construction - /// is what lets callers put a `DataFormatIdentifier` in a `const` table. + /// in a nibble (is greater than `0x0F`). pub const fn new(compression_method: u8, encryption_method: u8) -> Result { if compression_method > NIBBLE_MAX { return Err(Error::InvalidEncryptionCompressionMethod( @@ -281,7 +262,7 @@ impl DataFormatIdentifier { /// The single byte this identifier occupies on the wire. /// - /// `const`, unlike `u8::from(identifier)`, so it can be used in a `const` context. + /// `const`, unlike `u8::from(identifier)`. #[must_use] pub const fn value(&self) -> u8 { self.encryption_method | (self.compression_method << 4) @@ -313,7 +294,6 @@ impl From for u8 { } } -// compare to a u8 value impl PartialEq for DataFormatIdentifier { fn eq(&self, other: &u8) -> bool { let other_data_format_identifier = DataFormatIdentifier::from(*other); @@ -444,8 +424,7 @@ mod tests { #[test] fn prop_memory_format_identifier_roundtrip( - // The full accepted range, so an off-by-one in the range checks cannot sit - // underneath a passing property test. + // The full accepted range, so an off-by-one in the range checks fails. size_len in 1u8..=MAX_MEMORY_SIZE_LENGTH, addr_len in 1u8..=MAX_MEMORY_ADDRESS_LENGTH, ) { diff --git a/crates/uds_protocol/src/shared/negative_response_code.rs b/crates/uds_protocol/src/shared/negative_response_code.rs index 9d5e3a6d..f24dd30a 100644 --- a/crates/uds_protocol/src/shared/negative_response_code.rs +++ b/crates/uds_protocol/src/shared/negative_response_code.rs @@ -10,8 +10,8 @@ #[cfg_attr(feature = "clap", derive(clap::ValueEnum))] #[derive(Clone, Copy, Debug, Eq, PartialEq)] pub enum NegativeResponseCode { - /// This response code shall not be used in a negative response message. - /// This positiveResponse parameter value is reserved for server internal implementation + /// `0x00`, which Table A.1 reserves for a server's internal use; it never appears in a + /// negative response. PositiveResponse, /// A byte Table A.1 reserves for future definition: `0x01`–`0x0F`, `0x15`–`0x20`, /// `0x23`, `0x27`–`0x30`, `0x32`, `0x3B`–`0x4F`, `0x5E`–`0x6F`, `0x74`–`0x77`, @@ -19,87 +19,54 @@ pub enum NegativeResponseCode { #[cfg_attr(feature = "clap", clap(skip))] #[non_exhaustive] IsoSaeReserved(u8), - /// This response code indicates that the requested action has been rejected by the - /// server. The `GeneralReject` response code shall only be implemented in the server if - /// none of the negative response codes meet the needs of the implementation. This - /// response code shall not be used as a general replacement for other response codes - /// defined. + /// The server rejected the request and no more specific code fits (`0x10`, Table A.1). + /// A server answers it only where no other code applies. GeneralReject, - /// This response code indicates that the requested action will not be taken because the - /// server does not support the requested service. The server shall send this response - /// code in the case where the client has sent a request message with a service - /// identifier which is either unknown or not supported. This negative response code is - /// not shown in the list of negative response codes to be supported for a diagnostic - /// service, because this negative response code is not applicable for supported - /// services. + /// The service identifier is unknown to the server or unsupported (`0x11`, Table A.1). + /// It applies only to a service the server does not support, so no service's own list + /// of response codes names it. ServiceNotSupported, - /// This response code indicates that the requested action will not be taken because the - /// server does not support the service specific parameters of the request message. The - /// server shall send this response code in case the client has sent a request message - /// with a known and supported service identifier but with "sub function“ which is - /// either unknown or not supported. + /// The service is supported but its sub-function is unknown or unsupported (`0x12`, + /// Table A.1). SubFunctionNotSupported, - /// This response code indicates that the requested action will not be taken because the - /// length of the received request message does not match the prescribed length for the - /// specified service, or that the format of the parameters do not match the prescribed - /// format for the specified service. + /// The request's length, or the format of its parameters, does not match what the + /// service prescribes (`0x13`, Table A.1). IncorrectMessageLengthOrInvalidFormat, - /// This response code shall be reported by the server if the response to be generated - /// exceeds the maximum number of bytes available by the underlying network layer. + /// The response would be longer than the underlying network layer can carry (`0x14`, + /// Table A.1). ResponseTooLong, - /// This response code indicates that the server is temporarily too busy to perform the - /// requested operation. In this circumstance the client shall perform repetition of the - /// request message or "try again later". The repetition of the request shall be delayed - /// by a time specified in the respective implementation documents. + /// The server is temporarily too busy; the client repeats the request after a delay its + /// implementation documents set (`0x21`, Table A.1). BusyRepeatRequest, - /// This response code indicates that the requested action will not be taken because the - /// server prerequisite conditions are not met. + /// A precondition the server checks before the action is not met (`0x22`, Table A.1). ConditionsNotCorrect, - /// This response code indicates that the requested action will not be taken because the - /// server expects a different sequence of request messages or message as sent by the - /// client. This may occur when sequence sensitive requests are issued in the wrong - /// order. + /// The request arrived out of the order the server expects, such as a + /// sequence-sensitive request sent too early (`0x24`, Table A.1). RequestSequenceError, /// A subnet component the action needed did not answer in time (`0x25`, Table A.1). NoResponseFromSubnetComponent, /// A fault the server holds as a DTC prevents the action (`0x26`, Table A.1). FailurePreventsExecutionOfRequestedAction, - /// This response code indicates that the requested action will not be taken because the - /// server has detected that the request message contains a parameter which attempts to - /// substitute a value beyond its range of authority (e.g. attempting to substitute a - /// data byte of 111 when the data is only defined to 100), or which attempts to access - /// a dataIdentifier/routineIdentifer that is not supported or not supported in active - /// session. This response code shall be implemented for all services which allow the - /// client to read data, write data, or adjust functions by data in the server. + /// A parameter is outside its permitted range, or names a data or routine identifier + /// that is unsupported, or unsupported in the active session (`0x31`, Table A.1). Every + /// service that reads, writes or adjusts data in the server supports it. RequestOutOfRange, - /// This response code indicates that the requested action will not be taken because the - /// server's security strategy has not been satisfied by the client. The server shall - /// send this response code if one of the following cases occur: - /// - the test conditions of the server are not met, - /// - the required message sequence e.g. `DiagnosticSessionControl`, securityAccess is - /// not met, - /// - the client has sent a request message which requires an unlocked server. - /// - /// Beside the mandatory use of this negative response code as specified in the - /// applicable services within this standard, this negative response code can also be - /// used for any case where security is required and is not yet granted to perform the - /// required service. + /// The server's security conditions are not met: its test conditions, the required + /// message sequence (for example `DiagnosticSessionControl` then `SecurityAccess`), or + /// an unlocked server (`0x33`, Table A.1). Beyond the services that mandate it, a + /// server may use it wherever security is required and not yet granted. SecurityAccessDenied, - /// This response code indicates that the requested action will not be taken because the - /// client has insufficient rights based on its Authentication state. + /// The client's authentication state does not grant the rights the request needs + /// (`0x34`, Table A.1). AuthenticationRequired, - /// This response code indicates that the server has not given security access because - /// the key sent by the client did not match with the key in the server's memory. This - /// counts as an attempt to gain security. The server shall remain locked and increment - /// its internal securityAccessFailed counter. + /// The key the client sent does not match the server's (`0x35`, Table A.1). The attempt + /// counts: the server stays locked and increments its `securityAccessFailed` counter. InvalidKey, - /// This response code indicates that the requested action will not be taken because the - /// client has unsuccessfully attempted to gain security access more times than the - /// server's security strategy will allow. + /// The client has failed security access more times than the server's security strategy + /// allows (`0x36`, Table A.1). ExceedNumberOfAttempts, - /// This response code indicates that the requested action will not be taken because the - /// client's latest attempt to gain security access was initiated before the server's - /// required timeout period had elapsed. + /// The client tried security access again before the server's required delay had + /// elapsed (`0x37`, Table A.1). RequiredTimeDelayNotExpired, /// The request must arrive over secured transmission (`0x38`, Table A.1). SecureDataTransmissionRequired, @@ -137,122 +104,81 @@ pub enum NegativeResponseCode { ConfigurationDataUsageFailed, /// De-authentication failed (`0x5D`). DeAuthenticationFailed, - /// This response code indicates that an attempt to upload/download to a server's memory - /// cannot be accomplished due to some fault conditions. + /// A fault prevents the upload or download to the server's memory (`0x70`, Table A.1). UploadDownloadNotAccepted, - /// This response code indicates that a data transfer operation was halted due to some - /// fault. The active transferData sequence shall be aborted. + /// A fault halted the data transfer, and the active `TransferData` sequence is aborted + /// (`0x71`, Table A.1). TransferDataSuspended, - /// This response code indicates that the server detected an error when erasing or - /// programming a memory location in the permanent memory device (e.g. Flash Memory). + /// Erasing or programming the server's non-volatile memory, such as flash, failed + /// (`0x72`, Table A.1). GeneralProgrammingFailure, - /// This response code indicates that the server detected an error in the sequence of - /// `BlockSequenceCounter` values. Note that the repetition of a `TransferDataRequest` - /// message with a `BlockSequenceCounter` equal to the one included in the previous - /// `TransferDataRequest` message shall be accepted by the server. + /// The `BlockSequenceCounter` is out of sequence (`0x73`, Table A.1). A repeated + /// `TransferDataRequest` carrying the previous request's counter is accepted, not + /// answered with this code. WrongBlockSequenceCounter, - /// The request was received correctly and all its parameters were valid, but the server - /// has not finished the requested action and is not yet ready to accept another - /// request. - /// - /// The server sends a final positive or negative response once the service completes, - /// and may repeat this code until then. It affects the application-layer timing - /// parameters (it extends P2\*), and per ISO 14229-1:2020 Annex A.1 the server "shall - /// always send a final response (positive or negative) independent of the - /// `suppressPosRspMsgIndicationBit` value" once it has used this code. - /// - /// Typically used while writing or erasing flash, when the programming routine cannot - /// service the bus. Generally supported by every service, which is why it is not listed - /// in the individual services' `allowed_nack_codes`. + /// The request was received correctly and its parameters are valid, but the action is + /// unfinished and the server cannot yet take another request (`0x78`, Table A.1). The + /// server may send it repeatedly, each one extending the client's timeout to P2\*, and + /// once it has sent one it sends a final response, positive or negative, even for a + /// request that suppressed its positive response. Every service supports it, so no + /// service's `allowed_nack_codes` lists it. RequestCorrectlyReceivedResponsePending, - /// This response code indicates that the requested action will not be taken because the - /// server does not support the requested sub-function in the session currently active. - /// Within the programmingSession negative response code 0x12 (subFunctionNotSupported) - /// may optionally be reported instead of negative response code 0x7F - /// (subFunctionNotSupportedInActiveSession). This response code shall only be used when - /// the requested sub-function is known to be supported in another session, otherwise - /// response code 0x12 (subFunctionNotSupported) shall be used. This response code shall - /// be supported by each diagnostic service with a sub-function parameter, if not - /// otherwise stated in the data link specific implementation document, therefore it is - /// not listed in the list of applicable response codes of the diagnostic services. + /// The sub-function is supported, but not in the active session (`0x7E`, Table A.1); + /// one no session supports is answered + /// [`SubFunctionNotSupported`](Self::SubFunctionNotSupported) instead, which a server + /// may also use in the programming session. Every service with a sub-function supports + /// it unless a data link document says otherwise, so no service's own list of response + /// codes names it. SubFunctionNotSupportedInActiveSession, - /// This response code indicates that the requested action will not be taken because the - /// server does not support the requested service in the session currently active. This - /// response code shall only be used when the requested service is known to be supported - /// in another session, otherwise response code 0x11 (serviceNotSupported) shall be - /// used. This response code is in general supported by each diagnostic service, as not - /// otherwise stated in the data link specific implementation document, therefore it is - /// not listed in the list of applicable response codes of the diagnostic services. + /// The service is supported, but not in the active session (`0x7F`, Table A.1); one no + /// session supports is answered [`ServiceNotSupported`](Self::ServiceNotSupported) + /// instead. Every service supports it unless a data link document says otherwise, so no + /// service's own list of response codes names it. ServiceNotSupportedInActiveSession, - /// This response code indicates that the requested action will not be taken because the - /// server prerequisite condition for RPM is not met (current RPM is above a - /// pre-programmed maximum threshold). + /// Engine speed is above the maximum the action allows (`0x81`, Table A.1). RpmTooHigh, - /// This response code indicates that the requested action will not be taken because the - /// server prerequisite condition for RPM is not met (current RPM is below a - /// pre-programmed minimum threshold). + /// Engine speed is below the minimum the action requires (`0x82`, Table A.1). RpmTooLow, - /// This is required for those actuator tests which cannot be actuated while the Engine - /// is running. This is different from RPM too high negative response and needs to be - /// allowed. + /// The engine is running, and the action, such as an actuator test, needs it stopped + /// (`0x83`, Table A.1). Distinct from [`RpmTooHigh`](Self::RpmTooHigh). EngineIsRunning, - /// This is required for those actuator tests which cannot be actuated unless the Engine - /// is running. This is different from RPM too low negative response, and needs to be - /// allowed. + /// The engine is not running, and the action, such as an actuator test, needs it + /// running (`0x84`, Table A.1). Distinct from [`RpmTooLow`](Self::RpmTooLow). EngineIsNotRunning, - /// This response code indicates that the requested action will not be taken because the - /// server prerequisite condition for engine run time is not met (current engine run - /// time is below a preprogrammed limit). + /// The engine has not run for as long as the action requires (`0x85`, Table A.1). EngineRunTimeTooLow, - /// This response code indicates that the requested action will not be taken because the - /// server prerequisite condition for temperature is not met (current temperature is - /// above a preprogrammed maximum threshold). + /// The temperature is above the maximum the action allows (`0x86`, Table A.1). TemperatureTooHigh, - /// This response code indicates that the requested action will not be taken because the - /// server prerequisite condition for temperature is not met (current temperature is - /// below a preprogrammed minimum threshold). + /// The temperature is below the minimum the action requires (`0x87`, Table A.1). TemperatureTooLow, - /// This response code indicates that the requested action will not be taken because the - /// server prerequisite condition for vehicle speed is not met (current VS is above a - /// pre-programmed maximum threshold). + /// Vehicle speed is above the maximum the action allows (`0x88`, Table A.1). VehicleSpeedTooHigh, - /// This response code indicates that the requested action will not be taken because the - /// server prerequisite condition for vehicle speed is not met (current VS is below a - /// pre-programmed minimum threshold). + /// Vehicle speed is below the minimum the action requires (`0x89`, Table A.1). VehicleSpeedTooLow, - /// This response code indicates that the requested action will not be taken because the - /// server prerequisite condition for throttle/pedal position is not met (current TP/APP - /// is above a preprogrammed maximum threshold). + /// The throttle or accelerator pedal position is above the maximum the action allows + /// (`0x8A`, Table A.1). ThrottleOrPedalTooHigh, - /// This response code indicates that the requested action will not be taken because the - /// server prerequisite condition for throttle/pedal position is not met (current TP/APP - /// is below a preprogrammed minimum threshold). + /// The throttle or accelerator pedal position is below the minimum the action requires + /// (`0x8B`, Table A.1). ThrottleOrPedalTooLow, - /// This response code indicates that the requested action will not be taken because the - /// server prerequisite condition for being in neutral is not met (current transmission - /// range is not in neutral). + /// The transmission is not in neutral, which the action requires (`0x8C`, Table A.1). TransmissionRangeNotInNeutral, - /// This response code indicates that the requested action will not be taken because the - /// server prerequisite condition for being in gear is not met (current transmission - /// range is not in gear). + /// The transmission is not in gear, which the action requires (`0x8D`, Table A.1). TransmissionRangeNotInGear, - /// For safety reasons, this is required for certain tests before it begins, - /// and must be maintained for the entire duration of the test. + /// The brake switch is not closed. Some tests require it closed, for safety, from + /// before they start until they end (`0x8F`, Table A.1). BrakeSwitchNotClosed, - /// For safety reasons, this is required for certain tests before it begins, - /// and must be maintained for the entire duration of the test. + /// The shift lever is not in park. Some tests require it in park, for safety, from + /// before they start until they end (`0x90`, Table A.1). ShifterLeverNotInPark, - /// This response code indicates that the requested action will not be taken because the - /// server prerequisite condition for torque converter clutch is not met (current TCC - /// status above a preprogrammed limit or locked). + /// The torque converter clutch is locked, or its status is above the limit the action + /// allows (`0x91`, Table A.1). TorqueConverterClutchLocked, - /// This response code indicates that the requested action will not be taken because the - /// server prerequisite condition for voltage at the primary pin of the server (ECU) is - /// not met (current voltage is above a pre-programmed maximum threshold). + /// The voltage at the server's primary supply pin is above the maximum the action + /// allows (`0x92`, Table A.1). VoltageTooHigh, - /// This response code indicates that the requested action will not be taken because the - /// server prerequisite condition for voltage at the primary pin of the server (ECU) is - /// not met (current voltage is below a pre-programmed maximum threshold). + /// The voltage at the server's primary supply pin is below the minimum the action + /// requires (`0x93`, Table A.1). VoltageTooLow, /// A resource the action needs is briefly unavailable; the client may retry /// (`0x94`, Table A.1). Any service may answer it, as any may answer response-pending. diff --git a/crates/uds_protocol/src/shared/suppressable_positive_response.rs b/crates/uds_protocol/src/shared/suppressable_positive_response.rs index 1cd06c22..03597746 100644 --- a/crates/uds_protocol/src/shared/suppressable_positive_response.rs +++ b/crates/uds_protocol/src/shared/suppressable_positive_response.rs @@ -1,7 +1,6 @@ -//! UDS uses a bit mask called the Suppress Positive Response Message Indication Bit -//! (SPRMIB) to indicate if a positive response message should be suppressed. This module -//! provides a generic implementation of the Suppress Positive Response Message Indication -//! Bit (SPRMIB) for UDS subfunction enumerations. +//! The suppressPosRspMsgIndicationBit (SPRMIB), bit 7 of a sub-function byte, which asks +//! the server not to send a positive response. This module splits and fuses it for the +//! sub-function enumerations. use crate::Error; /// Suppress Positive Response Message Indication Bit @@ -28,9 +27,8 @@ pub(crate) const fn fuse_sprmib(suppress_positive_response: bool, value: u8) -> } } -/// `SuppressablePositiveResponse` is used to encapsulate subfunction enumerations that can -/// also encode the response suppression bit. This eliminates bit masking logic from a -/// number of subfunction enumerations. +/// A sub-function value together with its SPRMIB flag, so the sub-function enumerations +/// carry no bit-masking logic of their own. #[derive(Clone, Copy, Debug, Eq, PartialEq)] pub(crate) struct SuppressablePositiveResponse + Into + Copy> { suppress_positive_response: bool, @@ -98,7 +96,7 @@ mod test { } } use super::*; - /// Check that we properly decode and encode hex bytes + /// Every byte splits into its flag and its low seven bits. #[test] fn test_all_u8_values() { for i in 0..=u8::MAX { diff --git a/crates/uds_protocol/src/test_util.rs b/crates/uds_protocol/src/test_util.rs index 4cfd8e3b..d0c6b7c3 100644 --- a/crates/uds_protocol/src/test_util.rs +++ b/crates/uds_protocol/src/test_util.rs @@ -2,20 +2,13 @@ use crate::Encode; -/// Assert that an [`Encode`] value writes exactly `encoded_size()` bytes — both the -/// returned count AND the number of bytes actually consumed from the writer. +/// Assert that an [`Encode`] value writes exactly `encoded_size()` bytes, both as the +/// returned count and as the bytes consumed from the writer. /// -/// **This says nothing about *which* bytes get written.** `encoded_size` is a provided -/// method on [`Encode`] that counts by encoding into a sink, and no type in this crate -/// overrides it, so every quantity compared here is derived from `encode` itself; the -/// upstream `debug_assert!` inside `encoded_size` already catches a count that disagrees -/// with the bytes written. What is left is a real but narrow guarantee: that `encode` is -/// deterministic across two invocations, that its count matches a write into a real slice, -/// and that the value fits the helper's buffer. -/// -/// A call to this is therefore **not** byte-correctness coverage. Assert the expected bytes -/// at the call site as well — the absence of that is what let a swapped pair of encoded -/// fields, and a transposed pair of format-identifier nibbles, pass the whole suite. +/// This says nothing about *which* bytes get written: `encoded_size` is derived from +/// `encode` itself, so the check is only that `encode` is deterministic, that its count +/// matches a write into a real slice, and that the value fits the helper's buffer. Assert +/// the expected bytes at the call site as well. pub(crate) fn assert_encode_size_agrees(value: &T) where T::Error: core::fmt::Debug, diff --git a/crates/uds_protocol/tests/openapi_schema.rs b/crates/uds_protocol/tests/openapi_schema.rs index 3acf98f2..6e0b6c7a 100644 --- a/crates/uds_protocol/tests/openapi_schema.rs +++ b/crates/uds_protocol/tests/openapi_schema.rs @@ -1,26 +1,13 @@ -//! Checks the generated `OpenAPI` document against what `serde` actually does. +//! Checks the generated `OpenAPI` document against what `serde` does. //! //! The `utoipa` half of this crate is hand-written for every type whose serialized form is -//! a single protocol byte, and for the four composite requests that deserialize through a -//! repr. Three defects shipped in that code because nothing here asserted anything about -//! it: -//! -//! - the document had four dangling `$ref`s, where the derive it replaced had none, because -//! a hand-written `ToSchema` must forward `schemas()` and none of them did; -//! - two schema descriptions were implementation notes rather than API documentation, one -//! of them naming a module-private type, and one had lost its verb because utoipa's -//! derive silently drops `#[doc = concat!(..)]`; -//! - every byte schema advertised `0..=2^31` while its deserializer rejected anything above -//! `0x7F`, or `0x0F`, so a generated client would emit an `i32` and get a runtime -//! rejection. -//! -//! A schema that lies is worse than a verbose one, so these are checked, not eyeballed. +//! a single protocol byte, and for the composite requests that deserialize through a repr. +//! These tests check that the document resolves every `$ref`, that each byte schema's +//! `maximum` is the largest byte its deserializer accepts, and that no published +//! description is empty, truncated or names a crate-private type. -// Test code. `unwrap`, `expect` and `panic` are the failure mechanism here, and a -// test that indexes a known-length vector reads better than one that does not. The -// workspace lint standard in the root Cargo.toml reaches this target too -- an -// integration test is its own crate, so lib.rs cannot speak for it -- which is why -// the relaxation is stated here rather than left implicit. +// Test code: `unwrap`, `expect` and `panic` are the failure mechanism. An integration test +// is its own crate, so lib.rs cannot relax the workspace lints for it. #![allow( clippy::unwrap_used, clippy::expect_used, @@ -95,12 +82,12 @@ fn the_document_has_no_dangling_refs() { assert!( dangling.is_empty(), "these schemas are referenced but never defined, so the document does not resolve: \ - {dangling:?}. A hand-written `ToSchema` must forward `schemas()` — it defaults to a \ - no-op, and only the derive overrides it." + {dangling:?}. A hand-written `ToSchema` must forward `schemas()`, which defaults \ + to a no-op." ); - // And the children really are pulled in, so the assertion above is not passing - // vacuously on a document that happens to contain no `$ref`s at all. + // And the children really are pulled in, so the assertion above does not pass + // vacuously on a document with no `$ref`s at all. assert!( schemas.contains_key("SubnetNumber"), "expected the composite request to register its child schemas, got {:?}", @@ -108,11 +95,8 @@ fn the_document_has_no_dangling_refs() { ); } -/// `maximum` on each byte schema must be the largest byte `serde` actually accepts. -/// -/// This is the assertion that keeps the two from drifting: it does not hard-code a bound, -/// it discovers it by deserializing all 256 bytes and compares that to what the schema -/// advertises. +/// `maximum` on each byte schema is the largest byte `serde` accepts, discovered by +/// deserializing all 256 bytes rather than hard-coded. macro_rules! assert_schema_bound_matches_serde { ($ty:ty, $name:literal) => {{ let accepted: Vec = (0u8..=0xFF) @@ -149,8 +133,8 @@ macro_rules! assert_schema_bound_matches_serde { $name ); - // A contiguous `minimum..=maximum` cannot express a set with gaps, so any type with - // gaps has to say so in prose or the schema is still lying. + // A contiguous `minimum..=maximum` cannot express a set with gaps, so a type with + // gaps says so in its description. let description = schema .get("description") .and_then(serde_json::Value::as_str) @@ -202,9 +186,8 @@ fn every_byte_schema_advertises_the_range_serde_enforces() { #[test] fn no_schema_description_leaks_a_private_type_or_is_empty() { - // Types a downstream crate cannot name. A schema that mentions one hands a client - // author a dead end, and these two are the whole reason the reprs exist — so leaking - // them into the published description defeats the point. + // Types a downstream crate cannot name; a schema that mentions one hands a client + // author a dead end. const PRIVATE: [&str; 2] = ["ZeroSubFunction", "MemoryFormatIdentifier"]; let json = Api::openapi().to_json().expect("serializes"); @@ -253,10 +236,8 @@ fn no_schema_description_leaks_a_private_type_or_is_empty() { {description:?}" ); } - // The verb-less description this test exists to catch began with a bare newline: - // utoipa dropped the `#[doc = concat!(..)]` fragment carrying the sentence's - // subject and left the blank line that had followed it. A leading blank line is - // that exact signature. + // utoipa drops a `#[doc = concat!(..)]` fragment and leaves the blank line that + // followed it, so a description starting with whitespace has lost a sentence. assert!( !description.starts_with(char::is_whitespace), "{name}'s description starts with whitespace, which is what a dropped \ diff --git a/crates/uds_protocol/tests/public_api.rs b/crates/uds_protocol/tests/public_api.rs index aad63017..4ccf321e 100644 --- a/crates/uds_protocol/tests/public_api.rs +++ b/crates/uds_protocol/tests/public_api.rs @@ -5,11 +5,8 @@ //! the defining crate a `#[non_exhaustive]` struct literal compiles fine, so a type that is //! impossible for anyone else to build still looks constructible from there. -// Test code. `unwrap`, `expect` and `panic` are the failure mechanism here, and a -// test that indexes a known-length vector reads better than one that does not. The -// workspace lint standard in the root Cargo.toml reaches this target too -- an -// integration test is its own crate, so lib.rs cannot speak for it -- which is why -// the relaxation is stated here rather than left implicit. +// Test code: `unwrap`, `expect` and `panic` are the failure mechanism. An integration test +// is its own crate, so lib.rs cannot relax the workspace lints for it. #![allow( clippy::unwrap_used, clippy::expect_used, @@ -24,11 +21,10 @@ use uds_protocol::{ }; #[test] -fn a_tester_can_still_request_an_unmodeled_read_dtc_information_sub_function() { - // Sealing `IsoSaeReserved` keeps bit 7 (SPRMIB) out of the sub-function value, but it - // also made `IsoSaeReserved(byte)` unwritable out here — so a tester could no longer - // originate a request for a report type the crate has not implemented. `try_reserved` - // is the door. +fn try_reserved_builds_an_unmodeled_sub_function() { + // `IsoSaeReserved(byte)` is not writable out here, which keeps bit 7 (SPRMIB) out of + // the sub-function value; `try_reserved` is how a tester originates a request for a + // report type the crate has not implemented. use uds_protocol::{Encode, ReadDtcInfoRequest, ReadDtcInfoSubFunction}; let sub = ReadDtcInfoSubFunction::try_reserved(0x57).expect("0x57 has bit 7 clear"); @@ -44,23 +40,21 @@ fn a_tester_can_still_request_an_unmodeled_read_dtc_information_sub_function() { let written = suppressed.encode_to_slice(&mut buf).expect("encodes"); assert_eq!(&buf[..written], &[0xD7]); - // Bit 7 is SPRMIB, so it must not be smuggled in as part of the sub-function value: - // 0x80 would otherwise encode as a suppressed 0x00. + // Bit 7 is SPRMIB, not part of the sub-function value: 0x80 would otherwise encode as + // a suppressed 0x00. assert!(ReadDtcInfoSubFunction::try_reserved(0x80).is_err()); assert!(ReadDtcInfoSubFunction::try_reserved(0xD7).is_err()); } #[test] fn a_session_layer_can_tell_not_suppressed_from_cannot_say() { - // The distinction this method exists to make, checked from outside the crate. ISO - // 14229-2:2021 clause 10.3 gates tP3_Client_Phys on the SPRMIB, and whether a - // response is expected decides whether tP_Client starts at all -- so a caller has to - // be able to reach all three answers, not just the two a `bool` could express. + // ISO 14229-2:2021 clause 10.3 gates tP3_Client_Phys on the SPRMIB, and whether a + // response is expected decides whether tP_Client starts at all, so a caller reaches + // all three answers, not just the two a `bool` could express. use uds_protocol::{Decode, Request, UdsServiceType}; - // The fact behind all of the above is reachable on its own, for a caller building a - // dispatch table rather than inspecting a decoded frame. It is `const`, so it can be - // one. + // The fact behind the answers is reachable on its own and in a `const`, for a caller + // building a dispatch table rather than inspecting a decoded frame. const ECU_RESET: Option = UdsServiceType::EcuReset.has_sub_function(); // Modeled, has a sub-function, bit clear: a definite "expect a response". @@ -77,7 +71,7 @@ fn a_session_layer_can_tell_not_suppressed_from_cannot_say() { Request::decode(&[0x2C, 0x81, 0xF3, 0x00]).expect("DDDI with SPRMIB"); assert_eq!(unmodeled.is_positive_response_suppressed(), Some(true)); - // Vendor-specific: unknown, and the caller must supply the answer itself. + // Vendor-specific: unknown, so the caller supplies the answer itself. let (vendor, _) = Request::decode(&[0x40, 0xAA]).expect("unassigned SIDs pass through"); assert_eq!(vendor.is_positive_response_suppressed(), None); @@ -91,9 +85,8 @@ fn a_session_layer_can_tell_not_suppressed_from_cannot_say() { #[test] fn dtc_fault_detection_counter_record_is_constructible_downstream() { - // This is the `Item` type of a public iterator and is re-exported at the crate root so - // callers can name it. Without a constructor, `#[non_exhaustive]` made the only way to - // obtain one decoding bytes through `DtcFaultDetectionIter` — E0639 out here. + // The `Item` type of a public iterator, re-exported at the crate root so callers can + // name it and given a constructor so a downstream can build one (E0639 otherwise). let record = DtcFaultDetectionCounterRecord::new(DtcRecord::new(0x01, 0x02, 0x03), 0x2A); assert_eq!(record.dtc_record, DtcRecord::new(0x01, 0x02, 0x03)); @@ -103,13 +96,9 @@ fn dtc_fault_detection_counter_record_is_constructible_downstream() { #[test] fn the_payload_data_bags_are_buildable_with_a_struct_literal() { // These five carry no invariant: every field is `pub`, and their encoders derive the - // wire widths from the values, so no combination fails. The crate's rule is to - // encapsulate a request/response struct *iff* it bears an invariant, so these are plain - // data bags and a struct literal must work from out here. They were briefly - // `#[non_exhaustive]` purely for symmetry with the invariant-bearing types, which cost - // a downstream the struct literal (E0639) and bought nothing -- - // `DtcFaultDetectionCounterRecord` became unbuildable entirely and took two follow-up - // commits to repair. + // wire widths from the values, so no combination fails. A request or response struct is + // encapsulated exactly when it bears an invariant, so these are plain data bags and a + // struct literal works from out here. let _ = SizePayload { file_size_uncompressed: 0xC350, file_size_compressed: 0x7530, @@ -141,11 +130,9 @@ fn the_payload_data_bags_are_buildable_with_a_struct_literal() { #[test] fn the_two_length_bounded_payloads_reject_an_unencodable_value() { - // These two are the ones that genuinely bear an invariant, so they keep both - // `#[non_exhaustive]` and a fallible constructor: the wire field carrying the length is - // one byte for `SentDataPayload` and two for `NamePayload`, and `encode` used to be the - // first thing to notice an over-long value. A struct literal here is correctly a - // compile error. + // These two bear an invariant, so they have both `#[non_exhaustive]` and a fallible + // constructor: the wire field carrying the length is one byte for `SentDataPayload` + // and two for `NamePayload`. A struct literal here is a compile error. assert!(SentDataPayload::new(&[0u8; 255]).is_ok()); assert!(SentDataPayload::new(&[0u8; 256]).is_err()); @@ -156,15 +143,13 @@ fn the_two_length_bounded_payloads_reject_an_unencodable_value() { #[cfg(feature = "serde")] #[test] fn serde_cannot_build_a_reserved_variant_that_aliases_a_named_one() { - // With a plain derived `Deserialize`, `{"IsoSaeReserved":1}` produced a value whose - // `value()` is 0x01 -- the same byte `Iso14229_1DtcFormat` encodes -- yet compared - // unequal to it. The variant seal does nothing about serde; routing serde through `u8` - // closes it, for every byte. + // serde goes through `u8`, so `{"IsoSaeReserved":1}` cannot build a value whose byte is + // the one `Iso14229_1DtcFormat` encodes yet compares unequal to it. let round: DtcFormatIdentifier = serde_json::from_str("1").expect("a bare byte"); assert_eq!(round, DtcFormatIdentifier::Iso14229_1DtcFormat); assert_eq!(serde_json::to_string(&round).unwrap(), "1"); - // The old escape route no longer parses at all. + // The variant form does not parse at all. assert!( serde_json::from_str::(r#"{"IsoSaeReserved":1}"#).is_err() ); @@ -180,8 +165,8 @@ fn serde_cannot_build_a_reserved_variant_that_aliases_a_named_one() { } /// A sub-function byte goes through serde as the byte, so the reserved variants' range -/// checks hold: `{"IsoSaeReserved":129}` built a `ResetType` that encoded `11 81`, which -/// reads back as suppressed while the request said it was not. +/// checks hold: `{"IsoSaeReserved":129}` cannot build a `ResetType` that encodes `11 81`, +/// which would read back as suppressed while the request said it was not. #[cfg(feature = "serde")] #[test] fn serde_cannot_build_a_sub_function_that_sets_the_suppress_bit() { @@ -217,8 +202,8 @@ fn serde_cannot_build_a_sub_function_that_sets_the_suppress_bit() { /// A value that carries a wire integer is built only from that integer, so it never holds /// one a named variant has, and `==` agrees with the wire. Through `serde` too: a -/// `{"IsoSaeReserved":49}` would have been a `NegativeResponseCode` unequal to -/// `RequestOutOfRange` that encoded as `0x31`. +/// `{"IsoSaeReserved":49}` would be a `NegativeResponseCode` unequal to +/// `RequestOutOfRange` that encodes as `0x31`. #[cfg(feature = "serde")] #[test] fn serde_builds_a_wire_valued_type_only_from_its_integer() { @@ -261,16 +246,12 @@ fn a_reserved_variant_is_matched_by_shape_and_read_by_value() { #[test] fn a_reserved_variant_cannot_alias_a_named_one() { - // `PartialEq` on these enums is variant equality, not wire equality, so being able to - // name a reserved variant with a byte that a named variant also encodes creates a trap: - // `DtcFormatIdentifier::IsoSaeReserved(0x01) != Iso14229_1DtcFormat` even though both - // encode 0x01. `#[non_exhaustive]` on the byte-carrying variants makes that - // unconstructible from out here, so a value can only be obtained through the - // classifier, which never aliases. - // - // Constructing one is a compile error, checked by `tests/ui` conventions in spirit; - // what this test pins is the positive half — the classifier and the byte accessor - // agree, and equality matches the wire for every byte. + // `PartialEq` on these enums is variant equality, so a reserved variant carrying a byte + // a named variant also encodes would compare unequal to it. `#[non_exhaustive]` on the + // byte-carrying variants makes that unconstructible from out here, so a value is only + // obtained through the classifier, which never aliases. Constructing one is a compile + // error; this pins the positive half: the classifier and the byte accessor agree, and + // equality matches the wire for every byte. for byte in 0x00..=0xFFu8 { let format = DtcFormatIdentifier::from(byte); assert_eq!(format.value(), byte, "classifier lost the byte {byte:#04X}"); @@ -291,10 +272,9 @@ mod serde_cannot_bypass_validation { fn a_length_bounded_borrowed_str_cannot_be_deserialized_out_of_bounds() { use uds_protocol::NamePayload; - // Making a constructor fallible does nothing on its own: `derive(Deserialize)` - // writes the fields directly, so it is a second constructor that skips the check. - // `filePathAndName` is declared by a two-byte length prefix, so a longer name - // cannot be encoded. + // `derive(Deserialize)` writes the fields directly, so it is a second constructor + // that would skip the check. `filePathAndName` is declared by a two-byte length + // prefix, so a longer name cannot be encoded. let too_long = "a".repeat(usize::from(u16::MAX) + 1); let json = serde_json::to_string(&serde_json::json!({ "file_path_and_name": too_long })) @@ -316,19 +296,15 @@ mod serde_cannot_bypass_validation { } #[test] - fn a_borrowed_byte_slice_field_is_not_reachable_through_json_at_all() { + fn json_cannot_borrow_a_byte_slice_field() { use uds_protocol::WriteDataByIdentifierRequest; - // Worth pinning because it is easy to mistake for validation working. `&'de [u8]` - // needs a format with a native byte-string type; JSON has none, so serde_json - // refuses a sequence before any length check runs -- "invalid type: sequence, - // expected a borrowed byte array". Both of these fail, and the empty one fails for - // that reason rather than because the data record is empty. - // - // So the `deserialize_with` guard on these fields is only exercised by a format - // that can borrow bytes (CBOR, MessagePack, bincode). It is still the right place - // for the check -- this crate does not get to choose the caller's format -- but do - // not read a passing serde_json test as evidence that it works. + // `&'de [u8]` needs a format with a native byte-string type; JSON has none, so + // serde_json refuses a sequence before any length check runs ("invalid type: + // sequence, expected a borrowed byte array"). Both of these fail, the empty one for + // that reason rather than because the data record is empty, so the + // `deserialize_with` guard on these fields is exercised only by a format that can + // borrow bytes (CBOR, MessagePack, bincode). assert!( serde_json::from_str::( r#"{"identifier":61840,"data":[]}"# @@ -340,8 +316,7 @@ mod serde_cannot_bypass_validation { r#"{"identifier":61840,"data":[1,2,3]}"# ) .is_err(), - "if this starts passing, JSON gained a byte type and the empty-record assertion \ - above needs to become a real length check" + "JSON has no byte type, so a byte-slice field is unreachable through it" ); } @@ -350,10 +325,8 @@ mod serde_cannot_bypass_validation { use uds_protocol::{TesterPresentRequest, TesterPresentResponse}; // Bit 7 of the sub-function byte is the SPRMIB and is carried in its own field, so - // the sub-function itself is a 7-bit value. This used to be enforced by a mirror - // struct whose `TryFrom` called the classifier; it is now enforced by the - // classifier being serde's entry point for the field. Same guarantee, so the same - // bytes must be refused. + // the sub-function itself is a 7-bit value, enforced by the classifier being + // serde's entry point for the field. for byte in 0x80..=0xFFu8 { let json = format!(r#"{{"suppress_positive_response":false,"sub_function":{byte}}}"#); @@ -369,7 +342,7 @@ mod serde_cannot_bypass_validation { } // And every legal byte round-trips, reserved values included, so the range check - // did not become a blanket rejection. + // is a bound and not a blanket rejection. for byte in 0x00..=0x7Fu8 { let json = format!(r#"{{"suppress_positive_response":true,"sub_function":{byte}}}"#); @@ -401,8 +374,7 @@ mod serde_cannot_bypass_validation { #[test] fn data_format_identifier_rejects_an_over_wide_nibble() { - // `new` returns Err when either method exceeds 0x0F; JSON must not be a way around - // it. + // `new` returns Err when either method exceeds 0x0F; JSON is not a way around it. assert!( serde_json::from_str::( r#"{"encryption_method":255,"compression_method":255}"# @@ -416,8 +388,8 @@ mod serde_cannot_bypass_validation { #[test] fn range_checked_enums_reject_out_of_range_bytes() { - // Each of these has a `try_from` that range-checks the byte; the derived impl let a - // caller name the reserved variant directly and skip it. + // Each of these has a `try_from` that range-checks the byte, and serde goes through + // it. assert!(serde_json::from_str::("16").is_err()); assert!(serde_json::from_str::("128").is_err()); assert!(serde_json::from_str::("16").is_err()); @@ -429,12 +401,11 @@ mod serde_cannot_bypass_validation { } #[test] - fn tester_present_cannot_be_given_a_sub_function_that_collides_with_sprmib() { + fn tester_present_serde_rejects_bit_7() { use uds_protocol::TesterPresentRequest; - // The sub-function field is private precisely so a caller cannot mint a value with - // bit 7 set, which is SPRMIB. The derived impl exposed the private field by name - // and accepted any variant payload. + // The sub-function field is private so a caller cannot mint a value with bit 7 + // set, which is SPRMIB; serde does not offer a way around that. assert!( serde_json::from_str::( r#"{"suppress_positive_response":false,"sub_function":128}"# @@ -454,9 +425,8 @@ mod serde_cannot_bypass_validation { use uds_protocol::CommunicationControlRequest; // `node_id` must be present exactly when `control_type` is an enhanced-address - // variant. That rule is the entire reason both fields are private, and - // deserializing ignored it: an enhanced type with a null node_id produced a request - // that encoded to two bytes, which this crate's own decoder then rejected. + // variant, which is why both fields are private; deserializing enforces the same + // rule, so no request encodes to a frame this crate's own decoder rejects. assert!( serde_json::from_str::( r#"{"suppress_positive_response":false,"control_type":5,"communication_type":1,"subnet":0,"node_id":null}"# @@ -482,9 +452,8 @@ mod serde_cannot_bypass_validation { fn transfer_requests_cannot_be_given_a_width_that_truncates_the_value() { use uds_protocol::RequestDownloadRequest; - // The width nibbles are private so they cannot fall out of step with the values. A - // deserialized 8-byte address with a 1-byte declared width silently truncated to - // one byte on the wire. + // The width nibbles are private so they cannot fall out of step with the values; + // an 8-byte address with a 1-byte declared width would truncate on the wire. assert!( serde_json::from_str::( r#"{"data_format_identifier":0,"memory_address":18446744073709551615,"memory_address_length":1,"memory_size":16,"memory_size_length":1}"# @@ -501,9 +470,8 @@ mod serde_cannot_bypass_validation { .is_err() ); - // A wider-than-minimal declaration survives the round trip, so the ALFID a server - // was sent is the one it gets back. 0x33 is three bytes each, for values needing - // two. + // A wider-than-minimal declaration survives the round trip. 0x33 is three bytes + // each, for values needing two. let json = r#"{"data_format_identifier":17,"address_and_length_format_identifier":51,"memory_address":6299648,"memory_size":65535}"#; let req: RequestDownloadRequest = serde_json::from_str(json).unwrap(); assert_eq!(req.memory_address(), 0x0060_2000); @@ -519,9 +487,8 @@ mod serde_cannot_bypass_validation { fn no_crate_private_type_appears_in_the_serialized_form() { use uds_protocol::{RequestDownloadRequest, TesterPresentRequest}; - // `ZeroSubFunction` is module-private, yet it used to be a named component of the - // serialized shape — and of the generated OpenAPI schema, giving client generators - // a type downstream Rust cannot reference. + // `ZeroSubFunction` is module-private, so it names nothing in the serialized shape + // or the generated OpenAPI schema. let tester = serde_json::to_string(&TesterPresentRequest::new(false)).unwrap(); assert!( !tester.contains("zero_sub_function"), @@ -529,10 +496,8 @@ mod serde_cannot_bypass_validation { ); assert!(tester.contains("sub_function"), "got {tester}"); - // The transfer requests' widths were the other half of this: they were reached - // through a `pub(crate)` type, so the repr flattened them into two derived scalars. - // The type is now public and serializes as the single ISO-named byte it is on the - // wire, so the shape matches Table 441 instead of paraphrasing it. + // The transfer requests' widths serialize as the single ISO-named byte they are on + // the wire, so the shape matches Table 441 instead of paraphrasing it. let download = RequestDownloadRequest::new(DataFormatIdentifier::NONE, 0x1234, 0x10); let json = serde_json::to_string(&download).unwrap(); @@ -548,9 +513,8 @@ mod serde_cannot_bypass_validation { #[test] fn the_serialized_form_is_the_wire_byte() { - // Round-tripping through serde must agree with the protocol's own representation, - // otherwise `serde(try_from)` on the way in and a struct shape on the way out would - // disagree with each other. + // Round-tripping through serde agrees with the protocol's own representation, so + // `serde(try_from)` on the way in and the shape on the way out match. assert_eq!( serde_json::to_string(&SubnetNumber::ReceivedOn).unwrap(), "15" diff --git a/crates/uds_protocol/tests/spec_conformance.rs b/crates/uds_protocol/tests/spec_conformance.rs index 46ba77ef..0def3603 100644 --- a/crates/uds_protocol/tests/spec_conformance.rs +++ b/crates/uds_protocol/tests/spec_conformance.rs @@ -1,29 +1,14 @@ //! Round-trips the message-flow example byte sequences printed in ISO 14229-1:2020. //! -//! Every frame below is quoted from a numbered example table in the standard, which gives -//! these tests an oracle the rest of the suite does not have: the bytes come from the -//! document rather than from the crate. -//! -//! **What that does and does not buy.** It catches two things a round-trip against the -//! crate's own output cannot: a legal frame the crate *rejects* (a missing mandatory -//! `DTCFormatIdentifier`, a `MemorySelection` byte read from the wrong sub-functions), and -//! an encode/decode pair that disagree with each other (a `powerDownTime` written -//! unconditionally, so `51 01` re-encoded as `51 01 00`). -//! -//! It does **not** verify field *meaning*. If `encode` and `decode` share a misreading -//! symmetrically — two same-width adjacent fields transposed in both directions, say — the -//! bytes still round-trip and every test here passes. Only an assertion on the decoded -//! *value* catches that, which is the unit tests' job. Do not read a green run here as "the -//! layout is right". -//! -//! This is an integration test, so it sees the crate as a downstream user does: anything it -//! needs must be reachable and constructible through the public API. +//! Every frame below is quoted from a numbered example table in the standard, so the bytes +//! are an oracle independent of the crate. That catches a legal frame the crate rejects, +//! and an encode/decode pair that disagree with each other. It does not verify field +//! meaning: two same-width adjacent fields transposed symmetrically in `encode` and +//! `decode` still round-trip, and only an assertion on a decoded value, in the unit tests, +//! catches that. As an integration test it reaches the crate only through its public API. -// Test code. `unwrap`, `expect` and `panic` are the failure mechanism here, and a -// test that indexes a known-length vector reads better than one that does not. The -// workspace lint standard in the root Cargo.toml reaches this target too -- an -// integration test is its own crate, so lib.rs cannot speak for it -- which is why -// the relaxation is stated here rather than left implicit. +// Test code: `unwrap`, `expect` and `panic` are the failure mechanism. An integration test +// is its own crate, so lib.rs cannot relax the workspace lints for it. #![allow( clippy::unwrap_used, clippy::expect_used, @@ -212,9 +197,8 @@ fn every_spec_request_example_decodes_and_re_encodes_unchanged() { for Example { cite, bytes } in REQUESTS { let req = Request::decode_exact(bytes) .unwrap_or_else(|e| panic!("{cite}: decode of {bytes:02X?} failed: {e:?}")); - // `Request::Other` carries the SID and payload verbatim, so it round-trips - // perfectly. Without this, a frame for a service the crate does not model would - // pass silently and this suite would report coverage it does not have. + // `Request::Other` carries the SID and payload verbatim, so a frame for a service + // the crate does not model would round-trip and report coverage it does not have. assert!( !matches!(req, Request::Other { .. }), "{cite}: decoded to Request::Other, so this service is not actually modeled" @@ -232,8 +216,7 @@ fn every_spec_response_example_decodes_and_re_encodes_unchanged() { for Example { cite, bytes } in RESPONSES { let resp = Response::decode_exact(bytes) .unwrap_or_else(|e| panic!("{cite}: decode of {bytes:02X?} failed: {e:?}")); - // Same trap as on the request side: `Response::Other` round-trips any SID - // unchanged. + // As on the request side: `Response::Other` round-trips any SID unchanged. assert!( !matches!(resp, Response::Other { .. }), "{cite}: decoded to Response::Other, so this service is not actually modeled" @@ -249,11 +232,9 @@ fn every_spec_response_example_decodes_and_re_encodes_unchanged() { #[test] fn the_set_of_services_with_spec_example_coverage_is_pinned() { // A tripwire on this file's own fixtures, not a property of the crate: it reads the two - // const arrays above, so no production change can make it fail. Its only job is to - // force a deliberate edit here when a service gains or loses spec-example coverage. The - // previous name claimed it checked coverage of "every service the crate models", which - // it does not and cannot -- the crate models 16 request services and 13 have frames - // below. + // const arrays above, so no production change can make it fail. Its job is to force an + // edit here when a service gains or loses spec-example coverage. The crate models 16 + // request services; 13 have frames here. let mut sids: Vec = REQUESTS.iter().map(|e| e.bytes[0]).collect(); sids.sort_unstable(); sids.dedup(); @@ -265,24 +246,11 @@ fn the_set_of_services_with_spec_example_coverage_is_pinned() { "request-side spec-example coverage changed" ); - // Where a modeled service has no request frame here, the reason is that its example - // table's byte-value column is not machine-readable in the markdown conversion, NOT - // that the standard omits the example. An earlier version of this comment claimed the - // latter about 0x10, 0x11 and 0x27, and that was simply wrong: Table 31 - // (DiagnosticSessionControl), Table 38 (ECUReset) and Tables 47/49/51 (SecurityAccess) - // are all request byte tables. - // - // In Tables 31, 38 and 47 the conversion merged the byte value into the description - // cell ("ECUReset Request SID 11 16"), so the values can only be recovered by - // cross-referencing the parameter-definition tables — that is inference, not quotation, - // and quotation is the whole point of this suite. Table 49 survived the conversion - // intact and is included above. Table 472 (RequestUpload) is the same story: bytes #7 - // and #8 of its memorySize are not legible, so 0x35 has no frame here despite being - // fully modeled. - // - // This is the same class of extraction hazard as the two-bytes-in-one-cell rows noted - // on Tables 486 and 487. Verify against the PDF before adding a frame, never against a - // guess. + // The standard prints request examples for the modeled services absent here too + // (Table 31 for 0x10, Table 38 for 0x11, Tables 47 and 51 for 0x27, Table 472 for + // 0x35), but their byte-value columns are not legible in the available conversion of + // the document, and a value recovered by cross-referencing the parameter tables is + // inference, not quotation. A frame is added only from bytes read off the standard. let mut response_sids: Vec = RESPONSES.iter().map(|e| e.bytes[0]).collect(); response_sids.sort_unstable(); response_sids.dedup();