diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a1756fac..cd94ab07 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -127,6 +127,7 @@ jobs: --strict crates/simple_doip --strict crates/uds_session --strict crates/uds_protocol + --strict crates/uds_services # =================================================================================== # Fuzz Tests (hand-rolled: the reusable workflow's fuzz-tests job cannot reach these) diff --git a/crates/uds_services/Cargo.toml b/crates/uds_services/Cargo.toml index 20510906..66ee692d 100644 --- a/crates/uds_services/Cargo.toml +++ b/crates/uds_services/Cargo.toml @@ -3,7 +3,7 @@ name = "uds_services" version.workspace = true edition = "2024" rust-version.workspace = true -description = "ISO 14229-1 behaviour: typed UDS service dispatch over caller-defined identifiers, and the clause 8.7 server response implementation rules" +description = "ISO 14229-1 behavior: typed UDS service dispatch over caller-defined identifiers, and the clause 8.7 server response implementation rules" license = "MIT OR Apache-2.0" repository.workspace = true homepage.workspace = true @@ -24,20 +24,16 @@ uds_session = { workspace = true } automotive-wire-codec = { workspace = true } # No [features] table. This crate is no_std and allocation-free in every -# configuration, and nothing in src/ reads a cfg — `alloc` and `std` only forwarded to -# uds_protocol, so they promised a variation this crate's surface never made. An -# application already depends on uds_protocol directly (its handlers return -# NegativeResponseCode) and enables those there. +# configuration, and nothing in src/ reads a cfg. An application enables uds_protocol's +# `alloc` and `std` on its own dependency, which its handlers already have. # -# No binding is ever a dependency either: UDSSVC_ARCH_0002 points the dependency arrow -# one way and UDSSVC_ARCH_0003 makes bindings additive at the application. No runtime is -# named (UDSSVC_ARCH_0030) — an `async fn` implies neither std nor an executor, so a -# bare-metal target stays reachable. +# No binding is a dependency either: UDSSVC_ARCH_0002 points the dependency arrow one +# way and UDSSVC_ARCH_0003 makes bindings additive at the application. No runtime is +# named (UDSSVC_ARCH_0030). [dev-dependencies] -# The in-place `static` pattern the docs prescribe for a server: `ConstStaticCell` -# yields the `&'static mut` that `Server::step` needs without a stack temporary. A -# dev-dependency only, for the doctest and the composition test that show it. +# The in-place `static` pattern the docs show for a server: `ConstStaticCell` yields +# the `&'static mut` that `Server::step` needs without a stack temporary. static_cell = { workspace = true } [lints] diff --git a/crates/uds_services/README.md b/crates/uds_services/README.md index 32ed2b82..fd4bc05f 100644 --- a/crates/uds_services/README.md +++ b/crates/uds_services/README.md @@ -14,10 +14,10 @@ understands UDS: everything below it deals in bytes. It depends on [`uds_protocol`](https://crates.io/crates/uds_protocol) (ISO 14229-1 messages) and [`uds_session`](https://github.com/luminartech/uds_stack/tree/main/crates/uds_session) (ISO 14229-2 session timing), and drives the latter by value. Everything above -it is application code; everything it calls through is a transport binding — -today that means +it is application code; everything it calls through is a transport binding, +such as [`uds_on_ip`](https://github.com/luminartech/uds_stack/tree/main/crates/uds_on_ip) -(ISO 14229-5) — that implements the `UdsTransport` trait this crate declares. +(ISO 14229-5), that implements the `UdsTransport` trait this crate declares. This crate never depends on a binding: the Cargo edge runs the other way. ## Status @@ -30,47 +30,40 @@ the [workspace README](https://github.com/luminartech/uds_stack#status) for how that compares to the rest of the stack. -The architecture is authored as a sphinx-needs set under +The architecture is a sphinx-needs set under [`docs/architecture/uds_services/`](https://github.com/luminartech/uds_stack/tree/main/docs/architecture/uds_services) -at the workspace root — not inside this crate's own directory — and is meant to -be read rendered, at -. It carries -forty-five architecture elements (`UDSSVC_ARCH_####`), each recording either -the clause that forces it or the reasoning behind choosing it, and an +at the workspace root, rendered at +. +Each architecture element (`UDSSVC_ARCH_####`) records the clause that forces +it or the reasoning behind choosing it, and an [open questions](https://github.com/luminartech/uds_stack/blob/main/docs/architecture/uds_services/open-questions.rst) -page for what is still unsettled. That set is provisional in the same sense as -every crate's: it supersedes any `ARCHITECTURE.md`, and nothing here carries -one. +page holds what is still unsettled. ## What this is The point where an application meets a diagnostic stack, in both directions: the interface by which a server integrates the stack, and the set of requests available to a client -application. Everything below it deals in bytes — this is the last layer that understands +application. Everything below it deals in bytes; this is the last layer that understands UDS. -Its scope is ISO 14229-1's *behaviour*. That document is split in two across the stack: -[`uds_protocol`](https://crates.io/crates/uds_protocol) owns the format — the bits, -the bytes, and which messages are valid — and this crate owns everything else in it. -Clause 8.7, the server response implementation rules, is the densest part of that scope -and the reason the crate exists, but it is not the boundary. +Its scope is ISO 14229-1's *behavior*. That document is split in two across the stack: +[`uds_protocol`](https://crates.io/crates/uds_protocol) owns the format, the bits, +the bytes, and which messages are valid, and this crate owns everything else in it, +clause 8.7's server response implementation rules above all. ### The server side -A UDS server has to do more than answer the requests it supports. ISO 14229-1 -clause 8.7 specifies the whole validation sequence — which checks run in which -order, which negative response code each failure produces, and when the correct -answer is no response at all. Those rules are easy to get subtly wrong, and the -mistakes are invisible when testing against a cooperative client. - -This crate centralises them, so an application defines its identifiers, -implements the services it supports, and everything else follows: +ISO 14229-1 clause 8.7 specifies the whole validation sequence: which checks +run in which order, which negative response code each failure produces, and +when the correct answer is no response at all. This crate runs it, so an +application defines its identifiers, implements the services it supports, and +everything else follows: - A service that is not implemented answers `serviceNotSupported`. - An identifier outside the supported set answers `requestOutOfRange`. - Session and security preconditions produce the right in-session codes. - A functionally addressed request that the server does not support gets - **silence**, not a negative response — because the same request reached every + **silence**, not a negative response, because the same request reached every server on the bus. Identifiers are the application's own, so adding one breaks the application's @@ -82,26 +75,23 @@ A client application names a service and its parameters in the same identifier vocabulary its server handlers use, and gets back a typed result. It never assembles request bytes or parses responses. -That includes interpreting negative responses, which the layer below explicitly -declines: `uds_on_ip`'s transport has no view of a negative response at all — it -maps bytes onto `DoIP`, and interpreting what came back belongs to this layer. +That includes interpreting negative responses: a transport binding maps bytes +onto its protocol and has no view of a negative response at all. -One identifier catalogue, both roles. A vehicle programme that builds an ECU and +One identifier catalog, both roles. A vehicle program that builds an ECU and a tester from the same definitions gets a compiler error when only one of them was updated, rather than a mismatch found on a bench. -## What this is not +## Scope -Not a codec — messages are [`uds_protocol`](https://crates.io/crates/uds_protocol). -Not a session layer — session timing is -[`uds_session`](https://github.com/luminartech/uds_stack/tree/main/crates/uds_session), -though deciding to send a response-pending is this crate's (ISO 14229-2 makes it turn on -whether the server supports the service). Not a -connection manager: the diagnostic conversation is portable across transports, -connection setup is not, and no API here pretends otherwise. Not -transport-aware: a handler that knows how to fetch a data identifier has nothing -to say about IP, so the transport binding is an optional feature and the same -server works over DoIP or CAN. +Messages are [`uds_protocol`](https://crates.io/crates/uds_protocol)'s to encode +and decode, and session timing is +[`uds_session`](https://github.com/luminartech/uds_stack/tree/main/crates/uds_session)'s. +Deciding to send a response-pending is this crate's, because ISO 14229-2 makes it +turn on whether the server supports the service. Connection setup is the +binding's: the diagnostic conversation is portable across transports, and +connection setup is not. The application names the binding, so a handler that +fetches a data identifier is written once for every transport. ## Layering @@ -121,18 +111,16 @@ server works over DoIP or CAN. ``` One crate per ISO document, so "does this belong here?" is answered by asking -which document specifies the behaviour. ISO 14229-1 is the single exception — -too large for that rule to settle, so `uds_protocol` takes its format and this -crate takes the rest. `uds_on_can` is drawn to show the shape the design buys, -not a crate that exists in this workspace yet. +which document specifies the behavior. ISO 14229-1 is the single exception: +`uds_protocol` takes its format and this crate takes the rest. `uds_on_can` is +drawn to show the shape the design admits, not a crate that exists in this +workspace. ## Design constraints -`no_std` and allocation-free. Responses are written into a caller-supplied sink -rather than returned as a `Vec`, and no public type carries a `Vec` or a -`String`. This is designed in rather than deferred: alloc-freedom cannot be -retrofitted, because the signatures that make an API alloc-free are the ones -callers depend on. +`no_std` and allocation-free. Responses are written into a sink this crate +owns rather than returned as a `Vec`, and no public type carries a `Vec` or a +`String`. There is no feature table: this crate has no `[features]` in `Cargo.toml`. It is `no_std` and allocation-free in every configuration, with no cfg anywhere in @@ -140,7 +128,7 @@ is `no_std` and allocation-free in every configuration, with no cfg anywhere in ## Relationship to the standards -Neither licence granted here conveys any right in the ISO standards this crate +Neither license granted here conveys any right in the ISO standards this crate implements or cites, which remain ISO's. No standard's text is reproduced. See ["Relationship to the standards"](https://github.com/luminartech/uds_stack#relationship-to-the-standards) in the workspace README for the fuller statement that applies to every crate in @@ -148,12 +136,12 @@ this stack. ## Contributing -Pull requests, bug reports and questions are welcome — see +Pull requests, bug reports and questions are welcome; see [`CONTRIBUTING.md`](https://github.com/luminartech/uds_stack/blob/main/CONTRIBUTING.md). 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. diff --git a/crates/uds_services/src/access.rs b/crates/uds_services/src/access.rs index 64afdd62..06b23302 100644 --- a/crates/uds_services/src/access.rs +++ b/crates/uds_services/src/access.rs @@ -1,11 +1,10 @@ //! Where a sub-function or identifier is available, and which security levels unlock it. //! //! ISO 14229-1:2020 8.7.3.1 Figure 6 asks three questions of a sub-function before its -//! service runs — supported at all (0x12), supported in the active session (0x7E), and -//! unlocked (0x33) — and Figures 26 and 30 ask the same of an identifier. A service trait -//! answers all three at once with an [`Access`], or `None` where it does not support the -//! sub-function or identifier at all, so the answers cannot disagree and the pipeline asks -//! them in the figure's order. +//! service runs (supported at all, 0x12; supported in the active session, 0x7E; unlocked, +//! 0x33), and Figures 26 and 30 ask the same of an identifier. A service trait answers +//! all three at once with an [`Access`], or `None` where it does not support the +//! sub-function or identifier at all, and the pipeline asks them in the figure's order. use crate::SecurityLevel; use uds_protocol::DiagnosticSessionType; diff --git a/crates/uds_services/src/assembly.rs b/crates/uds_services/src/assembly.rs index 7a5d54cd..dba13496 100644 --- a/crates/uds_services/src/assembly.rs +++ b/crates/uds_services/src/assembly.rs @@ -1,25 +1,18 @@ //! Assembly, and the const arithmetic it performs. //! -//! ``UDSSVC_ARCH_0013`` — a server is assembled by naming its services, so the set is a -//! list this crate can read rather than an inference from which methods were overridden. -//! `serviceNotSupported` (0x11) is decided from a list, and this is that list. +//! ``UDSSVC_ARCH_0013``: a server is assembled by naming its services, and that list is +//! what `serviceNotSupported` (0x11) is decided from. //! -//! The macro does something the rest of the crate cannot. An associated const of a -//! *generic* parameter is not usable as an array length — `[u8; S::MAX_REQUEST]` is -//! rejected without the unstable `generic_const_exprs` — but at this macro's expansion -//! site the types are concrete, so `::MAX_BLOCK_LEN` is an -//! ordinary const and folding a list of them into an array length is stable Rust. That is -//! why assembly is a macro and not a blanket impl, and it is how [`crate::Storage`]'s -//! sizes are derived without the application picking a number. +//! Assembly is a macro because at its expansion site the types are concrete, so +//! `::MAX_BLOCK_LEN` is an ordinary const and folding a list of them +//! into [`crate::Storage`]'s array lengths is stable Rust; an associated const of a +//! generic parameter is not usable as an array length. /// The largest value in `values`, or zero where there are none. /// -/// `#[doc(hidden)]`: macro plumbing. It is `pub` only because [`crate::uds_server`] -/// expands in the application's crate and has to name it there, the same reason -/// `__uds_request_bound!` and its siblings are. -/// -/// Walked by splitting rather than by index: a `const fn` cannot use an iterator, and an -/// index plus a counter needs two prose invariants that this needs none of. +/// `#[doc(hidden)]`: macro plumbing, `pub` only because [`crate::uds_server`] expands in +/// the application's crate and names it there, as `__uds_request_bound!` and its siblings +/// are. #[doc(hidden)] #[must_use] pub const fn max_of(mut values: &[usize]) -> usize { @@ -36,8 +29,7 @@ pub const fn max_of(mut values: &[usize]) -> usize { /// The smaller of two values. Caps a derived size at /// [`crate::UdsTransport::MAX_PDU`]. /// -/// `#[doc(hidden)]`: macro plumbing, as [`max_of`] is. `core::cmp::min` is not `const` on -/// stable, which is why this exists at all. +/// `#[doc(hidden)]`: macro plumbing, as [`max_of`] is; `core::cmp::min` is not `const`. #[doc(hidden)] #[must_use] pub const fn min2(a: usize, b: usize) -> usize { @@ -50,9 +42,8 @@ pub const fn min2(a: usize, b: usize) -> usize { /// in bytes, as [`DtcReportKind::record_len`](crate::DtcReportKind::record_len) tells /// them apart. /// -/// `#[doc(hidden)]`: macro plumbing, as [`max_of`] is. Saturating throughout because this -/// crate denies `arithmetic_side_effects`, and a declared maximum large enough to -/// overflow `usize` is capped by [`min2`] against +/// `#[doc(hidden)]`: macro plumbing, as [`max_of`] is. Saturating: a declared maximum +/// large enough to overflow `usize` is capped by [`min2`] against /// [`MAX_PDU`](crate::UdsTransport::MAX_PDU) immediately afterwards. #[doc(hidden)] #[must_use] @@ -201,7 +192,7 @@ pub const fn assert_client_fits( /// /// The byte for each is `uds_protocol`'s to know (``UDSSVC_ARCH_0001`` gives it the /// format), so this list says only *which services this server implements*, which is -/// the behaviour half and the fact ``UDSSVC_ARCH_0013`` needs. +/// the behavior half and the fact ``UDSSVC_ARCH_0013`` needs. #[doc(hidden)] #[macro_export] macro_rules! __uds_sids { @@ -249,10 +240,8 @@ macro_rules! __uds_sids { /// What one service contributes to the in-flight buffer's length. /// -/// `#[rustfmt::skip]`: rustfmt 1.9.0 does not converge on the two-level associated-type -/// chains below (`<<$ty as _>::Did as _>::MAX_RECORD_LEN`) — each `cargo fmt` run adds -/// further indentation to the continuation line rather than reaching a fixed point. The -/// arms are hand-formatted instead, verified to stay within the crate's 92-column limit. +/// `#[rustfmt::skip]`: rustfmt does not converge on the two-level associated-type chains +/// below (`<<$ty as _>::Did as _>::MAX_RECORD_LEN`), so the arms are hand-formatted. #[doc(hidden)] #[macro_export] #[rustfmt::skip] @@ -294,8 +283,8 @@ macro_rules! __uds_request_bound { /// What one service contributes to the response buffer's length. /// -/// `#[rustfmt::skip]`: see `__uds_request_bound!`'s note above — the same non-convergent -/// chain formatting affects the `ReadDataByIdentifier` and `RoutineControl` arms here. +/// `#[rustfmt::skip]`: as `__uds_request_bound!`, for the `ReadDataByIdentifier` and +/// `RoutineControl` arms. #[doc(hidden)] #[macro_export] #[rustfmt::skip] @@ -336,8 +325,8 @@ macro_rules! __uds_response_bound { #[doc(hidden)] #[macro_export] macro_rules! __uds_may_pend { - // Neither service can be in progress when a deadline passes — see their trait docs — - // so neither declares the constant and the answer is not the application's to give. + // Neither service can be in progress when a deadline passes (see their trait docs), + // so neither declares the constant. ($ty:ty, TesterPresent) => { false }; @@ -496,7 +485,7 @@ macro_rules! __uds_ahead_of_decode { /// The checks a listed service's stage makes ahead of the request's total length, for a /// request too long to have been received whole: an early `return` of the first one that /// fails, from the closure the truncated branch of `dispatch` calls. Only a service whose -/// stage checks something before the length emits one — an identifier in its +/// stage checks something before the length emits one: an identifier in its /// data-parameters (WDBI, RoutineControl) or the request sequence (SecurityAccess's /// and the transfer services' 0x24, `TransferData`'s 0x73 and 0x31); for the others /// `begin` has already asked every such question, and the request is 0x13. @@ -663,25 +652,20 @@ macro_rules! __uds_assert { /// /// ``UDSSVC_ARCH_0013``, ``UDSSVC_ARCH_0035``. /// -/// Naming the transport lets its [`MAX_PDU`](crate::UdsTransport::MAX_PDU) join the fold. -/// It introduces no dependency on a binding: the *application* names the type, and it -/// already depends on both crates, so ``UDSSVC_ARCH_0002`` and ``0003`` are untouched. -/// +/// `transport = T` lets the transport's [`MAX_PDU`](crate::UdsTransport::MAX_PDU) join +/// the fold; the *application* names the type, so ``UDSSVC_ARCH_0002`` and ``0003`` hold. /// `peers = N` sizes the association array of the `uds_session::Server` the driver /// owns. `services = Assembly` names the marker type the macro declares and implements /// this crate's sealed [`ServiceSet`](crate::ServiceSet) on, with the application as its /// [`App`](crate::ServiceSet::App). `server = Name` is the alias the server is reached -/// through: the macro emits `type Name = Server`, so the count is -/// written once, where it acts. Both are private to the invoking module. -/// -/// The application type may come from any crate: the marker is what is local, so one +/// through: the macro emits `type Name = Server`. Both are private +/// to the invoking module. The application type may come from any crate, so one /// application shared by firmware, a simulator and tests is assembled in each. /// -/// **Milestone 1 accepts `peers = 1` only**, and any other count fails to compile. The -/// driver keeps one slot for a `DiagnosticSessionControl` or `ECUReset` response awaiting -/// its confirmation, and with a second tester that tester's selecting response could -/// overwrite the slot while the first is still unconfirmed, so the first session would -/// never be applied. With no peer at all, no response could ever be sent. +/// `peers = 1` is the only count that compiles: the driver keeps one slot for a +/// `DiagnosticSessionControl` or `ECUReset` response awaiting its confirmation, and a +/// second tester's selecting response could overwrite it while the first is still +/// unconfirmed. /// /// The assembly also fails to compile where the response buffer, or the transport's /// [`MAX_PDU`](crate::UdsTransport::MAX_PDU), is shorter than @@ -820,33 +804,25 @@ macro_rules! __uds_assert { /// ``` /// /// The emitted `dispatch` only routes: `pipeline::begin`, then the listed service's -/// stage, and it returns what they left as an [`Unsettled`](crate::Unsettled). It calls -/// `pipeline::settle` nowhere; the driver does, once the handler has finished -/// (``UDSSVC_ARCH_0016``). Every clause 8.7 decision is the pipeline's. +/// stage, and it returns what they left as an [`Unsettled`](crate::Unsettled). The driver +/// calls `pipeline::settle` once the handler has finished (``UDSSVC_ARCH_0016``). Every +/// clause 8.7 decision is the pipeline's. /// -/// **Figure 6's sub-function questions are answered by one lookup per service trait** — -/// an [`Access`](crate::Access) for a sub-function the service supports, typed in that -/// service's own sub-function, carrying the sessions it is available in and the levels -/// that unlock it (``UDSSVC_ARCH_0007`` rows 2 and 4, then Figure 6's optional 0x33). -/// `dispatch` hands `pipeline::begin` one closure that routes to the listed service's -/// trait, and the pipeline decides 0x12, 0x7E or 0x33 from the answer. The lookup is -/// `access` on [`EcuReset`] and on every other staged service with a sub-function, except -/// three: `DiagnosticSessionControl` answers with [`supports`] and [`supported_from`], -/// because whether a session may be entered turns on the session it is entered from; -/// `SecurityAccess` with [`sessions`], because it is the service that unlocks; and -/// `TesterPresent` with nothing, because its only sub-function is available everywhere. -/// `RoutineControl` is not asked here at all: Figure 5 keeps 0x31 out of the -/// sub-function stage, so its `access` is keyed by routine and asked by its own stage. +/// Figure 6's sub-function questions are answered by one lookup per service trait, an +/// [`Access`](crate::Access) for a sub-function the service supports (``UDSSVC_ARCH_0007`` +/// rows 2 and 4, then Figure 6's optional 0x33), from which the pipeline decides 0x12, +/// 0x7E or 0x33. The lookup is `access` on [`EcuReset`] and on every other staged service +/// with a sub-function, except three: `DiagnosticSessionControl` answers with +/// [`supports`] and [`supported_from`], `SecurityAccess` with [`sessions`], and +/// `TesterPresent` with nothing, its only sub-function being available everywhere. +/// `RoutineControl` is not asked here: Figure 5 keeps 0x31 out of the sub-function stage, +/// so its `access` is keyed by routine and asked by its own stage. /// /// [`supports`]: crate::DiagnosticSessionControl::supports /// [`supported_from`]: crate::DiagnosticSessionControl::supported_from /// [`sessions`]: crate::SecurityAccess::sessions /// [`EcuReset`]: crate::EcuReset /// -/// The syntax is `Ecu: ..; transport = T, ..` rather than `Ecu over T: ..` because -/// `$ty:ty` cannot be followed by a bare identifier — the legal followers are -/// `{ [ => , > = : ; | as where`. -/// /// # Examples /// /// Two services, a transport, and the buffer lengths the macro folds from what they @@ -1009,13 +985,12 @@ macro_rules! uds_server { <$transport as $crate::UdsTransport>::MAX_PDU, ); // The only message clause 8.7.6 admits while a service is in progress is a - // two-byte TesterPresent. Anything larger is occupancy, and is answered - // busyRepeatRequest from its service identifier alone -- so this buffer need - // only be wide enough to read that identifier and reject what follows. + // two-byte TesterPresent; anything larger is answered busyRepeatRequest from + // its service identifier alone, so this buffer is wide enough to read that + // identifier. const CONCURRENT: usize = 8; - // A negative response always fits only if the buffer under the sink holds - // one; `ResponseSink::new` can raise its bound to the floor but not past the - // buffer. + // A negative response fits only if the buffer under the sink holds one: + // `ResponseSink::new` raises its bound to the floor but not past the buffer. const _: () = ::core::assert!( <$transport as $crate::UdsTransport>::MAX_PDU >= $crate::ResponseSink::MIN_BOUND, @@ -1026,8 +1001,8 @@ macro_rules! uds_server { "the response buffer cannot hold a negative response", ); $( $crate::__uds_assert!($ty, $transport, $svc); )+ - // Milestone 1 keeps one slot for a selecting response awaiting confirmation, - // so a second peer could overwrite it; zero peers can never answer. + // One slot for a selecting response awaiting confirmation, so a second peer + // could overwrite it; zero peers can never answer. const _: () = ::core::assert!( { $peers } == 1, "milestone 1 supports `peers = 1` only", @@ -1227,8 +1202,8 @@ macro_rules! uds_server { /// Assemble a client from the identifier vocabulary it speaks. /// -/// ``UDSSVC_ARCH_0020``, ``UDSSVC_ARCH_0024`` — the same enumeration a server's handlers -/// are written against names a client's requests, so a vehicle programme that builds both +/// ``UDSSVC_ARCH_0020``, ``UDSSVC_ARCH_0024``: the same enumeration a server's handlers +/// are written against names a client's requests, so a vehicle program that builds both /// from one definition gets a compile error when only one was updated. /// /// `vocabulary = Name` names the marker type the macro declares and implements this @@ -1249,7 +1224,7 @@ macro_rules! uds_server { /// compile, naming the limit it cannot carry, as does one with no room for a six-byte /// `DiagnosticSessionControl` response ([`crate::Client::diagnostic_session_control`]); /// six bytes also hold every negative response. The response buffer is capped at -/// `MAX_PDU`, since no longer response can arrive. +/// `MAX_PDU`, since no response longer than that can arrive. /// /// `client = Name` is the alias the client is reached through, so the channel counts and /// the keep-alive mode are written once, here, rather than at every signature that names @@ -1643,9 +1618,7 @@ mod tests { assert_eq!(NONE, 0); } - /// The fold the macro performs, as a `const` so it fails to *compile* rather than - /// to run if it ever stops being const-evaluable — which is the property that - /// matters. + /// The fold the macro performs is const-evaluable: this fails to compile otherwise. #[test] fn the_maximum_is_const_evaluable() { const SIZES: [usize; 3] = [7, 1_026, 300]; @@ -1653,9 +1626,7 @@ mod tests { assert_eq!(MAX, 1_026); } - /// An empty set folds to zero rather than panicking. A server with no services is - /// useless but not malformed, and a panic in a const is a compile error with a very - /// poor message. + /// An empty set folds to zero rather than panicking. #[test] fn an_empty_set_folds_to_zero() { const MAX: usize = max_of(&[]); diff --git a/crates/uds_services/src/client.rs b/crates/uds_services/src/client.rs index 7b15d2c1..fafc73e0 100644 --- a/crates/uds_services/src/client.rs +++ b/crates/uds_services/src/client.rs @@ -1,37 +1,18 @@ //! The client role. //! -//! ``UDSSVC_ARCH_0020`` — a client issues requests over the application's own +//! ``UDSSVC_ARCH_0020``: a client issues requests over the application's own //! identifiers, and ``UDSSVC_ARCH_0024`` makes that the same vocabulary a server's //! handlers are written against. A client implements no service trait. +//! ``UDSSVC_ARCH_0021``: a negative response is interpreted here; nothing below reads a +//! response code. //! -//! ``UDSSVC_ARCH_0021`` — a negative response is interpreted here; nothing below reads -//! a response code. -//! -//! The exchanges themselves run in `client::driver` over `uds_session`'s client role and -//! [`UdsTransport`], as [`crate::Server`]'s do; `client::encode` holds the sans-io halves -//! (``UDSSVC_ARCH_0028``). -//! -//! [`Client`] has a call for every service the sensor stages, each one physical exchange -//! with one server: [`Client::read_data_by_identifier`], -//! [`Client::diagnostic_session_control`], [`Client::write_data_by_identifier`], -//! [`Client::ecu_reset`], [`Client::security_access_seed`], -//! [`Client::security_access_key`], [`Client::routine_control`], -//! [`Client::read_dtc_information`], [`Client::clear_diagnostic_information`], -//! [`Client::communication_control`] and [`Client::control_dtc_setting`]; the transfer -//! steps [`Client::request_download`], [`Client::transfer_data`] and -//! [`Client::request_transfer_exit`]. [`Client::read_data_by_identifier_functional`] is one -//! functional exchange with every server at once. Beside the exchanges, -//! [`Client::idle_until`] sends the `TesterPresent` keep-alives as they fall due, +//! The exchanges run in `client::driver` over `uds_session`'s client role and +//! [`UdsTransport`]; `client::encode` holds the sans-io halves (``UDSSVC_ARCH_0028``). +//! [`Client`] has a call for every service a server stages, each one physical exchange +//! with one server, and the `_functional` calls are one exchange with every server at +//! once. [`Client::idle_until`] sends the `TesterPresent` keep-alives as they fall due, //! [`Client::connection_replaced`] reports why a server's connection was replaced, and //! [`Client::close`] ends the session. -//! -//! **The storage shape mirrors `uds_session`'s.** That crate splits channels by kind: -//! `PhysicalSlot` carries no responder table where `FunctionalSlot` does, and a -//! physical channel's `tS3_Client` is an argument of `open_physical_channel` in physical -//! keep-alive and absent from it in functional keep-alive. -//! Mirroring the split costs a third const parameter and buys the same thing it buys -//! them — a physical-only client pays nothing for responder tables, and a functional -//! channel cannot be given a session reload it has no use for. mod driver; mod encode; @@ -66,15 +47,11 @@ pub const KEEP_ALIVE_LEN: usize = encode::KEEP_ALIVE.len(); /// One application's identifier vocabulary, with the storage derived from it. /// -/// ``UDSSVC_ARCH_0013`` for the client half — implemented by [`crate::uds_client`], +/// ``UDSSVC_ARCH_0013`` for the client half: implemented by [`crate::uds_client`], /// never by hand, on the marker type it declares in the invoking crate. The identifier -/// enumerations, [`Self::Did`] and [`Self::Routine`], may live in any crate: a client has -/// no handler state, so they and its [`ClientLimits`] are all the application declares, -/// and they are what the buffer lengths are folded from. -/// -/// Sealed through [`crate::sealed`], for the reason -/// [`ServiceSet`](crate::ServiceSet) is: the derivation argument holds only while the -/// macro is what chooses [`Self::Store`]'s lengths. +/// enumerations, [`Self::Did`] and [`Self::Routine`], may live in any crate; they and the +/// [`ClientLimits`] are what the buffer lengths are folded from. Sealed through +/// [`crate::sealed`], as [`ServiceSet`](crate::ServiceSet) is. pub trait ClientSet: crate::sealed::Sealed { /// The data identifiers the client's requests name, declared as /// [`crate::uds_client`]'s leading type. @@ -93,10 +70,8 @@ pub trait ClientSet: crate::sealed::Sealed { /// identifier vocabularies declare. /// /// [`crate::uds_client`] folds the client's buffers from these. Built in a `const` from -/// [`Self::new`] and its setters, and read back through the field each setter sets. A -/// limit not set keeps its default, which carries nothing beyond the smallest request: -/// one identifier per read, and no seed, key, option record, DTC record or transfer -/// block. +/// [`Self::new`] and its setters. A limit not set keeps its default: one identifier per +/// read, and no seed, key, option record, DTC record or transfer block. /// /// A limit a request carries bounds it: a call given more is [`ClientError::Request`], /// and nothing is sent. A response longer than the response buffer is @@ -226,7 +201,7 @@ impl Default for ClientLimits { /// Why a call produced no response. /// -/// ``UDSSVC_ARCH_0023`` — a timeout is a fault, as are a request that could not be sent +/// ``UDSSVC_ARCH_0023``: a timeout is a fault, as are a request that could not be sent /// and a connection that closed under it. A negative response is not here: it is a /// response, [`Response::Negative`] or [`Answer::Negative`]. #[derive(Debug, Clone, Copy, PartialEq, Eq)] @@ -495,18 +470,14 @@ pub struct ClientTiming { /// standard gives no default. pub network_delay: u32, /// The ceiling on a physically addressed request's repeats under which an expired - /// response window is repeated rather than reported as [`ClientError::Timeout`]. - /// ISO 14229-2:2021 9.7 Table 9 has the client repeat it and leaves performing the - /// repeat to the application (``UDSS_LLR_0177`` caps it). Table 9 performs its error - /// handling "for a maximum of two times" whatever the error, so the ceiling counts - /// every repeat of the request: a failed transmission's repeat leaves one fewer for - /// an expired window. A failed transmission is itself repeated under - /// [`Self::transmission_repeats`], that cell being the link's, not the request's. - /// A functionally addressed request's window is not repeated, so this ceiling does - /// not reach it. [`Self::new`] sets [`Repeats::Twice`]. - /// [`Repeats::Never`] also forgoes the one repeat the standard designs a server to - /// absorb, a [`Client::transfer_data`] block with the same counter (ISO 14229-1:2020 - /// 15.4); set it, call, and restore it where that repeat is wanted. + /// response window is repeated rather than reported as [`ClientError::Timeout`] + /// (ISO 14229-2:2021 9.7 Table 9, ``UDSS_LLR_0177``). Table 9 caps its error handling + /// at two times whatever the error, so the ceiling counts every repeat of the + /// request: a failed transmission's repeat, made under + /// [`Self::transmission_repeats`], leaves one fewer for an expired window. A + /// functionally addressed request's window is not repeated. [`Self::new`] sets + /// [`Repeats::Twice`]; [`Repeats::Never`] also forgoes the repeat of a + /// [`Client::transfer_data`] block with the same counter (ISO 14229-1:2020 15.4). pub repeats: Repeats, /// The ceiling on a request's repeats after a failed transmission, physical or /// functional; past it the request is [`ClientError::NotSent`]. [`Self::new`] sets @@ -664,7 +635,7 @@ impl From for MalformedResponse { /// What one server said. /// -/// ``UDSSVC_ARCH_0021`` — three cases, and none is an error. +/// ``UDSSVC_ARCH_0021``: three cases, and none is an error. #[derive(Debug, Clone, Copy, PartialEq, Eq)] #[non_exhaustive] pub enum Response { @@ -704,12 +675,9 @@ impl Response { /// One server's answer to a functionally addressed request. /// -/// ``UDSSVC_ARCH_0022`` — each responding server sets its own source address, and that is -/// how answers are told apart, so every variant carries one. -/// -/// There is no "nothing came back" case here, unlike [`Response`]: a server that stays -/// silent produces no answer at all, which is the response window closing rather than a -/// value to match on. +/// ``UDSSVC_ARCH_0022``: each responding server sets its own source address, which is +/// how answers are told apart, so every variant carries one. A server that stays silent +/// produces no answer at all. #[derive(Debug, Clone, PartialEq, Eq)] #[non_exhaustive] pub enum Answer<'d, D: DataIdentifier> { @@ -732,8 +700,7 @@ pub enum Answer<'d, D: DataIdentifier> { /// identifiers, or that carries a record the request did not ask for. /// /// Not a [`UdsTransport::Error`]: the transport delivered the message intact and the - /// server answered. What failed is the agreement about what the bytes mean, which is - /// a disagreement between two applications rather than a link fault. + /// server answered. Malformed { /// The responding server's `S_AI[SA]`. from: Address, @@ -756,15 +723,10 @@ impl Answer<'_, D> { /// The answers to one functionally addressed request. /// -/// ``UDSSVC_ARCH_0022`` — a functional request reaches every server on the bus, so zero +/// ``UDSSVC_ARCH_0022``: a functional request reaches every server on the bus, so zero /// or more may answer, and by ``UDSSVC_ARCH_0009`` those that do not support it answer -/// with silence. There is no single response to return, so typing one would force the API -/// to lie about the common case. -/// -/// The sequence is **lending**: an answer borrows the receive buffer and is valid only -/// until the next is taken. ``UDSSVC_ARCH_0017``'s reasoning — an owned sequence -/// allocates per response. An inherent `async fn` rather than a `Stream`, because a -/// `Stream` item cannot borrow the receive buffer. +/// with silence. The sequence is **lending**: an answer borrows the receive buffer and is +/// valid only until the next is taken (``UDSSVC_ARCH_0017``). /// /// The request goes out on the first [`Self::next`]. The request in progress is recorded /// in the [`Client`], not here, so dropping this part-way is safe: the client's next call @@ -841,7 +803,7 @@ impl< /// One server's answer to a functionally addressed request whose positive response /// carries nothing to read, such as [`Client::control_dtc_setting_functional`]'s. /// -/// ``UDSSVC_ARCH_0022`` — each responding server sets its own source address, so every +/// ``UDSSVC_ARCH_0022``: each responding server sets its own source address, so every /// variant carries one. A server that stays silent produces no reply at all. #[derive(Debug, Clone, Copy, PartialEq, Eq)] #[non_exhaustive] @@ -962,20 +924,14 @@ where /// The identifier/record pairs in one `ReadDataByIdentifier` response. /// -/// ``UDSSVC_ARCH_0026`` — the application declared each identifier's record structure, so -/// splitting a multi-identifier response needs nothing this crate knows about the data. -/// A response is a concatenation of `(identifier, record)` with no length prefixes, and -/// [`DataIdentifier::split_record`] is what makes it walkable. +/// ``UDSSVC_ARCH_0026``: a response is a concatenation of `(identifier, record)` with no +/// length prefixes, and [`DataIdentifier::split_record`] is what makes it walkable. /// -/// **Walking one cannot fail.** The whole response is checked when this is built, so a -/// framing error is one [`MalformedResponse`] reported once — as [`Answer::Malformed`] or -/// [`Response::Malformed`] — rather than a `Result` at every step of a walk that could -/// only ever fail once and then end. There is no half-walked response: either every pair -/// is reachable or none is. -/// -/// Items borrow the receive buffer, not the iterator, so this is an ordinary [`Iterator`] -/// rather than a lending one. `Clone` but not `Copy`: a copied iterator silently restarts -/// the walk, which is the whole hazard `clippy::copy_iterator` names. +/// Walking one cannot fail: the whole response is checked when this is built, so a +/// framing error is one [`MalformedResponse`], reported as [`Answer::Malformed`] or +/// [`Response::Malformed`], and either every pair is reachable or none is. Items borrow +/// the receive buffer, not the iterator, so this is an ordinary [`Iterator`]. `Clone` but +/// not `Copy`: a copied iterator silently restarts the walk. #[derive(Debug, Clone, PartialEq, Eq)] #[must_use = "the records are the response; dropping this discards it"] pub struct Records<'d, D> { @@ -984,11 +940,8 @@ pub struct Records<'d, D> { } impl<'d, D: DataIdentifier> Records<'d, D> { - /// Check `response` — the bytes after the echoed service identifier — and hold it - /// for walking. - /// - /// Not public: a [`Records`] comes from a read, so the offset this expects is never - /// a caller's to get right. + /// Check `response`, the bytes after the echoed service identifier, and hold it for + /// walking. /// /// # Errors /// @@ -1024,9 +977,7 @@ impl<'d, D: DataIdentifier> Iterator for Records<'d, D> { let (identifier, tail) = self.rest.split_first_chunk::<2>()?; let did = D::from_u16(u16::from_be_bytes(*identifier))?; // `validate` walked this same path already, so none of the three fallible steps - // here can fail. Ending the walk is the honest way to say so: this crate denies - // both `panic` and `unreachable`, and stopping cannot be worse than the half-walk - // the validation exists to prevent. + // here fails; a failure would end the walk. let (record, rest) = did.split_record(tail).ok()?; self.rest = rest; Some((did, record)) @@ -1035,23 +986,15 @@ impl<'d, D: DataIdentifier> Iterator for Records<'d, D> { /// A UDS client over any transport. /// -/// ``UDSSVC_ARCH_0028`` — the core is sans-io; awaiting is the layer above it, so the -/// encode and interpret halves are usable without a transport at all. +/// ``UDSSVC_ARCH_0028``: the core is sans-io, and awaiting is the layer above it. /// /// `C` is the identifier vocabulary and, through [`ClientSet::Store`], the buffers folded /// from it. `K`, `PHYS`, `FUNC` and `R` mirror `uds_session::Client`: -/// the keep-alive mode, physical channels, functional channels, and responders per -/// functional channel. `R` defaults to `0`, as it does there, so a physical-only client -/// does not write a count for tables it has none of. -/// -/// An application never spells any of this. [`crate::uds_client`] emits -/// `type Tester = Client<..>` and that alias is the name at every call site, the same way -/// [`crate::uds_server`]'s `server = Name` works. -/// -/// `K` is `uds_session::FunctionalKeepAlive` or `PhysicalKeepAlive`, sealed there to those -/// two. Holding it in the type is that crate's `UDSS_LLR_0149`: the mode is fixed at -/// creation, so the methods that supply a `tS3_Client` reload exist only on the mode that -/// gives one a meaning. +/// the keep-alive mode (`FunctionalKeepAlive` or `PhysicalKeepAlive`, fixed at creation, +/// `UDSS_LLR_0149`), physical channels, functional channels, and responders per +/// functional channel, `R` defaulting to `0`. An application never spells any of this: +/// [`crate::uds_client`] emits `type Tester = Client<..>`, and that alias is the name at +/// every call site. #[derive(Debug)] pub struct Client< C: ClientSet, @@ -1087,16 +1030,11 @@ impl< { /// A client over `transport`, owning its session-layer and buffer storage by value. /// - /// A `const fn`, which is load-bearing for the reason - /// [`crate::Server::new`] is one: the buffers are inline and a runtime constructor - /// would build a stack temporary before the move. - /// - /// The channel slots are built here rather than supplied, so an application never - /// names `uds_session` — the same commitment [`crate::Server::new`] already makes by - /// building its own session. Channels are opened as calls need them, one per server - /// and kind, with the reloads [`UdsTransport::channel_timing`] dictates. No channel - /// handle crosses this API, so two clients cannot confuse each other's, and - /// `uds_session`'s handle tag is left at its default. + /// A `const fn`, as [`crate::Server::new`] is, so a client whose buffers are inline is + /// built in a `static` initializer rather than as a stack temporary. The channel slots + /// are built here, so an application never names `uds_session`; channels are opened as + /// calls need them, one per server and kind, with the reloads + /// [`UdsTransport::channel_timing`] dictates, and no channel handle crosses this API. /// /// # Arguments /// @@ -1156,9 +1094,7 @@ impl< /// Read one or more data identifiers from one server. /// /// The response comes back as [`Records`], the identifier/record pairs it carries, - /// not as the undivided bytes: the application already declared each record's - /// structure through [`DataIdentifier::split_record`], so re-walking the response by - /// hand would be the caller redoing work this crate can do. + /// split by the application's [`DataIdentifier::split_record`]. /// /// A response-pending message (`0x78`) extends the wait to the enhanced response /// window, which [`Self::diagnostic_session_control`] sets from the server's own @@ -1176,7 +1112,7 @@ impl< /// /// # Errors /// - /// A negative response is **not** an error — it arrives as [`Response::Negative`]. + /// A negative response is **not** an error: it arrives as [`Response::Negative`]. /// /// - [`ClientError::Request`] where `identifiers` is empty or too long; nothing is /// sent. @@ -1221,7 +1157,7 @@ impl< /// Change one server's diagnostic session. /// - /// ISO 14229-1:2020 10.2 — the positive response carries the session's + /// ISO 14229-1:2020 10.2: the positive response carries the session's /// [`SessionTiming`], read in Table 29's units. Its `P2Server_max` and /// `P2*Server_max`, each plus [`ClientTiming::network_delay`], become the waits this /// client allows for a response from `target` and after a response-pending message @@ -1681,18 +1617,15 @@ impl< } /// Ask `target` to take a download of `size` bytes to `address` (ISO 14229-1:2020 - /// 15.2): the first of clause 17's standardized transfer steps. The + /// 15.2), the first of clause 17's standardized transfer steps. The /// `addressAndLengthFormatIdentifier` declares the narrowest widths that hold /// `address` and `size`; [`Self::request_download_with_format`] fixes them instead. /// /// The response is the server's `maxNumberOfBlockLength`: the length of one /// `TransferData` request, service identifier and counter included (Table 443), so /// each [`Self::transfer_data`] block carries exactly this minus two data bytes, - /// except possibly the last. The server need accept nothing shorter: "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)." A block longer than the - /// client's [`ClientLimits::max_block_len`] cannot be sent. + /// except possibly the last; the server need accept nothing shorter (15.2.3.2). A + /// block longer than the client's [`ClientLimits::max_block_len`] cannot be sent. /// /// The client keeps no transfer state: the caller owns the block sequence counter /// and its roll from `0xFF` to `0x00`. @@ -1794,10 +1727,9 @@ impl< /// Send one block of a transfer to `target` (ISO 14229-1:2020 15.4). The response /// echoes `counter`, then carries the server's `transferResponseParameterRecord`. /// - /// This is the one request the standard designs a repeat for: a server that already - /// wrote the block answers its repeat, same `counter`, without writing again (15.4). - /// A caller holding [`ClientTiming::repeats`] at [`Repeats::Never`] raises it for the - /// blocks, through [`Self::timing_mut`], and restores it after. + /// A server that already wrote the block answers its repeat, same `counter`, without + /// writing again (15.4), so a caller holding [`ClientTiming::repeats`] at + /// [`Repeats::Never`] raises it for the blocks through [`Self::timing_mut`]. /// /// # Arguments /// @@ -1885,13 +1817,12 @@ impl< /// Wait until `until`, sending every keep-alive that falls due meanwhile. /// - /// The client's `sleep`: a keep-alive is only ever sent from inside a call, so an - /// application with a server in a non-default session waits here between requests. - /// One that does not still has an overdue keep-alive sent during its next call, since - /// the session layer holds it until then: a physical one as soon as that call first - /// waits on the transport (to the server the call addresses, the request itself - /// stands in for it), the functional one once the call's exchange has ended. What - /// arrives meanwhile is handed to the session layer and otherwise discarded. + /// A keep-alive is only ever sent from inside a call, so an application with a server + /// in a non-default session waits here between requests. One that does not still has + /// an overdue keep-alive sent during its next call: a physical one as soon as that + /// call first waits on the transport (the request itself stands in for it to the + /// server the call addresses), the functional one once the call's exchange has ended. + /// What arrives meanwhile is handed to the session layer and otherwise discarded. /// /// # Arguments /// @@ -1923,35 +1854,27 @@ impl< /// the channel to it was opened over. /// /// A positive [`Self::ecu_reset`] response is reported [`Replaced::SessionLost`] from - /// that exchange itself, before the transport reports the close, since the server - /// always closes after one (ISO 14229-5:2022 REQ 7.11); the next close the transport - /// reports to `target` is that same one, unless the server answers first. Any other - /// close is reported once the transport has been polled past it, by - /// [`Self::idle_until`] or by any call: [`Replaced::SessionLost`] where the transport - /// marks it expected, and [`Replaced::SessionUnknown`] otherwise. The `DoIP` - /// transport marks the close that follows a positive `DiagnosticSessionControl` - /// (REQ 7.9), a close a server makes only where the change leaves its running - /// software, until it acknowledges any request between the response and the close, - /// a keep-alive falling due included; that close is then - /// [`Replaced::SessionUnknown`]. On a transport whose first poll after a close may be - /// `Pending`, as the std `DoIP` stack's is, one poll may not reach it. + /// that exchange itself, since the server always closes after one (ISO 14229-5:2022 + /// REQ 7.11); the next close the transport reports to `target` is that same one, + /// unless the server answers first. Any other close is reported once the transport + /// has been polled past it, by [`Self::idle_until`] or by any call: + /// [`Replaced::SessionLost`] where the transport marks it expected, and + /// [`Replaced::SessionUnknown`] otherwise. The `DoIP` transport marks the close that + /// follows a positive `DiagnosticSessionControl` (REQ 7.9) until it acknowledges any + /// request between the response and the close, a keep-alive included. On a transport + /// whose first poll after a close may be `Pending`, one poll may not reach it. /// /// [`Replaced::SessionUnknown`] is the normal result of a request that went unanswered - /// over `DoIP`: its transport puts the first request after it, the repeat - /// ISO 14229-2:2021 Table 9 asks for where [`ClientTiming::repeats`] allows one or the - /// next call where it does not, on a new connection, so a late answer cannot be taken - /// for that request's. It is also the result of a call cancelled after its request - /// went. - /// + /// over `DoIP`, whose transport puts the next request on a new connection so that a + /// late answer cannot be taken for it, and of a call canceled after its request went. /// Where closes of both kinds happened since the last asking, the answer is /// [`Replaced::SessionLost`], once. /// - /// This first settles what an earlier call left, waiting out a functional window a - /// dropped call left open, then takes every event the transport holds at the asking, - /// until it reports the asking's time as its deadline or its clock passes it, sends a - /// keep-alive that fell due, and answers. Asking clears the report. - /// [`Replaced::No`] where no channel to `target` is open, and where the channel was - /// withdrawn for another server before the asking. + /// This first settles what an earlier call left, then takes every event the transport + /// holds at the asking, until it reports the asking's time as its deadline or its + /// clock passes it, sends a keep-alive that fell due, and answers. Asking clears the + /// report. [`Replaced::No`] where no channel to `target` is open, and where the + /// channel was withdrawn for another server before the asking. /// /// # Arguments /// @@ -2012,12 +1935,10 @@ impl< /// Read one or more data identifiers from every server on a functional address. /// - /// ``UDSSVC_ARCH_0022`` — a functional request reaches every server, so zero or - /// more may answer and there is no single response to return. The answers come back - /// as a lending sequence: this is the only way to obtain a [`Responses`], and - /// draining it is how each server's answer is read. Nothing is sent until its first - /// [`Responses::next`], which is also where a refusal surfaces. A negative response is - /// [`Answer::Negative`], which is not an error. + /// ``UDSSVC_ARCH_0022``: zero or more servers answer, so the answers come back as a + /// [`Responses`] sequence; nothing is sent until its first [`Responses::next`], which + /// is also where a refusal surfaces. A negative response is [`Answer::Negative`], + /// which is not an error. /// /// # Arguments /// @@ -2045,7 +1966,7 @@ impl< /// the suppress bit set so that none answers (ISO 14229-1:2020 9.2.2), and awaiting /// nothing beyond the transmission's confirmation. /// - /// ``UDSSVC_ARCH_0009`` and ISO 14229-1:2020 Table 5 — a server that performed the + /// ``UDSSVC_ARCH_0009`` and ISO 14229-1:2020 Table 5: a server that performed the /// request is silent, as is one that does not support it, so success cannot be told /// from silent non-support; a server that refuses it for any other reason still /// answers with a negative response, which the [`Replies`] sequence lends, as it @@ -2078,7 +1999,7 @@ impl< /// Set DTC recording on every server on a functional address, with the suppress bit /// clear so that each that supports it answers. /// - /// ``UDSSVC_ARCH_0022`` — zero or more servers answer, so the replies come back as a + /// ``UDSSVC_ARCH_0022``: zero or more servers answer, so the replies come back as a /// [`Replies`] sequence; nothing is sent until its first [`Replies::next`], which is /// also where a refusal surfaces. /// @@ -2243,7 +2164,6 @@ const fn within(len: usize, max: usize) -> Result<(), RequestError> { } } -/// The bytes `answered` occupies in the response buffer. /// The response `answered` holds, with the identifiers of the `ReadDataByIdentifier` /// request it answers: the first `count` pairs after the request's service identifier, /// which nothing overwrites until the next request is encoded. @@ -2262,6 +2182,7 @@ fn answered_read<'s, S: ClientStorage>( ) } +/// The bytes `answered` occupies in the response buffer. fn answered_bytes<'s, S: ClientStorage>(store: &'s mut S, answered: &Answered) -> &'s [u8] { let ClientBuffers { response, .. } = store.split(); response.get(answered.range.clone()).map_or(&[], |m| m) @@ -2304,9 +2225,8 @@ mod tests { } } - /// ``UDSSVC_ARCH_0021`` — a negative response is a response, and this crate is the - /// layer that interprets it: `uds_session` indicates the message and `UdsTransport` - /// carries its bytes, and neither reads a response code. + /// ``UDSSVC_ARCH_0021``: a negative response is a response, and this crate is the + /// layer that interprets it. #[test] fn a_negative_response_is_a_response_not_an_error() { let p: Response<&[u8]> = Response::Positive(&[0x62]); @@ -2316,10 +2236,8 @@ mod tests { assert!(matches!(n, Response::Negative(_))); } - /// ``UDSSVC_ARCH_0022`` — each answer carries the source address of the server that - /// produced it. For a functionally addressed request every responding server sets - /// its own, and a sequence of decoded responses without their senders would be - /// unattributable — so `from` is on every variant and reachable without matching. + /// ``UDSSVC_ARCH_0022``: each answer carries the source address of the server that + /// produced it, reachable without matching. #[test] fn every_answer_names_its_sender() { let declined: Answer<'_, TestDid> = Answer::Negative { @@ -2334,13 +2252,8 @@ mod tests { assert_eq!(broken.from(), Address(0x0E02)); } - /// ``UDSSVC_ARCH_0026`` — the application declared the record widths, so the client - /// hands back typed pairs rather than the undivided response. Two identifiers of - /// different widths in one response is the case that makes the point: nothing but - /// `split_record` can tell where the first record ends. - /// - /// Walking yields pairs, not `Result`s: the response was checked when the walk was - /// built. + /// ``UDSSVC_ARCH_0026``: two identifiers of different widths in one response split + /// into typed pairs, and walking yields pairs, not `Result`s. #[test] fn a_response_splits_into_the_records_the_application_declared() { // 0xF40D and its one byte, then 0xF190 and its seventeen. @@ -2370,8 +2283,7 @@ mod tests { } /// A record shorter than the application declared is rejected when the walk is - /// built, not discovered part-way through one. The distinction is the point of - /// validating: a caller never sees a response half-walked. + /// built, not discovered part-way through one. #[test] fn a_truncated_record_is_rejected_before_the_walk() { assert_eq!( @@ -2380,9 +2292,8 @@ mod tests { ); } - /// An identifier this application never defined is its own error. On a request that - /// is `requestOutOfRange`; in a response it is a server naming something unasked - /// for, and it too is caught before any pair is handed out. + /// An identifier this application never defined is its own error, caught before any + /// pair is handed out. #[test] fn an_identifier_the_application_does_not_define_is_rejected() { assert_eq!( @@ -2391,10 +2302,8 @@ mod tests { ); } - /// The records before a bad one are not handed out either. A response whose first - /// pair is whole and whose second is truncated is rejected entire — which is what - /// "there is no half-walked response" means, and what a per-record `Result` could - /// not express. + /// The records before a bad one are not handed out either: a response whose first + /// pair is whole and whose second is truncated is rejected entire. #[test] fn a_good_record_before_a_bad_one_is_not_yielded() { let response = [0xF4, 0x0D, 0x40, 0xF1, 0x90, 0x00]; diff --git a/crates/uds_services/src/client/driver.rs b/crates/uds_services/src/client/driver.rs index a003d3c2..e95a0738 100644 --- a/crates/uds_services/src/client/driver.rs +++ b/crates/uds_services/src/client/driver.rs @@ -1,18 +1,14 @@ //! The pump under a [`Client`]'s calls: one exchange at a time over `uds_session`'s //! client role, its outputs drained into the transport. //! -//! Every call goes through here. The exchange it runs is recorded in the client, not in -//! the call's future, so a call or a [`super::Responses`] dropped part-way leaves a -//! record the next call settles: a physical exchange is reset (``UDSS_LLR_0180``) and a -//! functional window is drained until it closes, its answers discarded. -//! -//! `Confirm` and `ResponseTimeout` name no channel, so they are matched to the exchange by -//! its [`Ai`]. That is exact here, because ``UDSS_LLR_0122`` makes an addressing unique -//! per channel and this driver runs one exchange at a time. -//! -//! `TransportEvent` has no start-of-message, so `t_data_som_ind` is never called and a -//! functional channel's responder table tracks only response-pending entries -//! (``UDSS_LLR_0137``, ``UDSS_LLR_0146``). +//! The exchange it runs is recorded in the client, not in the call's future, so a call +//! or a [`super::Responses`] dropped part-way leaves a record the next call settles: a +//! physical exchange is reset (``UDSS_LLR_0180``) and a functional window is drained +//! until it closes, its answers discarded. `Confirm` and `ResponseTimeout` are matched to +//! the exchange by its [`Ai`], unique per channel (``UDSS_LLR_0122``). `TransportEvent` +//! has no start-of-message, so `t_data_som_ind` is never called and a functional +//! channel's responder table tracks only response-pending entries (``UDSS_LLR_0137``, +//! ``UDSS_LLR_0146``). use super::encode::{self, Arrived, KEEP_ALIVE}; use super::{Client, ClientError, ClientKeepAlive, ClientSet, Replaced}; @@ -131,10 +127,8 @@ pub(super) enum Heard { pub(super) struct Open { ai: Ai, pub(super) id: Id, - /// A transmission on it awaits its confirmation; withdrawing it would leave that - /// confirmation to match a later channel with the same addressing (the open question - /// "A late confirmation can match a reopened channel's request"), so it is never - /// withdrawn meanwhile. + /// A transmission on it awaits its confirmation, so it is not withdrawn meanwhile: + /// the confirmation would match a later channel with the same addressing. unconfirmed: bool, /// Its server was put in a non-default session through this client. pub(super) in_session: bool, @@ -508,10 +502,8 @@ impl< Ok(ai) } - /// The functional channel to `group`, opened if it is not yet. - /// - /// The keep-alive's own group is never withdrawn (the open question "Functional - /// keep-alive falls due with no functional channel open"). + /// The functional channel to `group`, opened if it is not yet. The keep-alive's own + /// group is never withdrawn. pub(super) fn functional_channel( &mut self, group: Address, @@ -898,21 +890,14 @@ impl< /// Wait for one transport event and hand it to the session layer, reporting what it /// meant for `subject`. `until` bounds the wait beside the session's own deadline. /// - /// ``UDSS_LLR_0026`` — the caller identifies an indication's channel. A response is - /// the functional window's while one is open, and otherwise the physical channel to - /// its sender's; one from a server no channel names is not indicated at all. - /// It answers `subject` only where it echoes its service; see [`encode::classify`]. - /// - /// An expected close releases the keep-alive of every channel to that peer - /// (``UDSS_LLR_0184``): the server's session ended with the connection, so a - /// `TesterPresent` would keep nothing alive, and the functional keep-alive goes once no - /// server is left in session, unless a session change is running, which settles it - /// when it ends. Any other close ends no session, since a server's session outlives its - /// connection, and the keep-alive goes on over the next one. A close ends a physical - /// exchange with that peer whose window is open, since its answer cannot come, but not - /// one awaiting its confirmation, which the transport still owes and which says - /// whether the request went; nor a functional window, which awaits every server and - /// closes at its own timeout. + /// ``UDSS_LLR_0026``: the caller identifies an indication's channel, the functional + /// window's while one is open and otherwise the physical channel to its sender's. It + /// answers `subject` only where it echoes its service; see [`encode::classify`]. + /// An expected close ends the server's session, so it releases the keep-alive of every + /// channel to that peer (``UDSS_LLR_0184``), and the functional keep-alive once no + /// server is left in session, unless a session change is running. Any close ends a + /// physical exchange with that peer whose window is open, but not one awaiting its + /// confirmation, nor a functional window, which closes at its own timeout. pub(super) async fn pump( &mut self, subject: Option, diff --git a/crates/uds_services/src/lib.rs b/crates/uds_services/src/lib.rs index 4d65a356..100a1594 100644 --- a/crates/uds_services/src/lib.rs +++ b/crates/uds_services/src/lib.rs @@ -5,66 +5,55 @@ //! set of requests available to a client application. Everything below deals //! in bytes; this is the last layer that understands UDS. //! -//! Its scope is ISO 14229-1:2020's *behaviour*: `uds_protocol` owns that -//! document's format — the bits, the bytes, and which messages are valid — and -//! this crate owns everything else in it. Clause 8.7, the server response -//! implementation rules, is the densest part of that and the reason the crate -//! exists, but it is not the boundary. A table, figure, clause or requirement +//! Its scope is ISO 14229-1:2020's *behavior*: `uds_protocol` owns that +//! document's format, the bits, the bytes, and which messages are valid, and +//! this crate owns everything else in it, clause 8.7's server response +//! implementation rules above all. A table, figure, clause or requirement //! cited in this crate with no document named is ISO 14229-1:2020's, and any //! other document is named where it is cited. //! -//! A UDS server must do more than answer the requests it supports. Clause 8.7 -//! specifies the whole validation sequence: which checks run in which order, -//! which negative response code each failure produces, and when the correct -//! answer is no response at all. This crate centralises those rules so an -//! application defines its identifiers, implements the services it supports, -//! and the rest follows. +//! Clause 8.7 specifies the whole validation sequence: which checks run in +//! which order, which negative response code each failure produces, and when +//! the correct answer is no response at all. This crate runs that sequence, so +//! an application defines its identifiers and implements the services it +//! supports, and the rest follows. //! //! A client application names services in that same identifier vocabulary and //! gets its results back in it: a `ReadDataByIdentifier` response arrives as the //! identifier/record pairs the application declared, not as the bytes the server -//! concatenated. Negative responses come back too — the layer below explicitly -//! declines to interpret them, and this is the layer that does. +//! concatenated. A negative response is interpreted here, by the one layer that +//! reads a response code. //! //! # Status //! -//! The server role runs end to end over any `UdsTransport` (see -//! `tests/end_to_end.rs`), and every service trait has a pipeline stage (see -//! `tests/stages.rs`); `DataTransfer` stages the download, and its upload and file -//! transfer are not supported (0x11). The client role -//! runs end to end over any `ClientTransport`, a `UdsTransport` it can close (see -//! `tests/client.rs`). -//! `tests/composition.rs` assembles a server against the whole surface. +//! The server role runs end to end over any [`UdsTransport`], and every +//! service trait has a pipeline stage; `DataTransfer` stages the download, and +//! its upload and file transfer are not supported (0x11). The client role runs +//! end to end over any [`ClientTransport`], a `UdsTransport` it can close. //! //! # Scope //! -//! ISO 14229-1's behaviour is what is implemented; being the stack's -//! integration surface is what the crate is. Message encode/decode is -//! `uds_protocol`; session timing is `uds_session`; framing and transport are -//! the binding's. Deciding to -//! answer `requestCorrectlyReceivedResponsePending` (0x78) is this crate's, -//! because ISO 14229-2 makes that decision turn on whether the server supports -//! the service; *when* one is due remains the session layer's. -//! -//! A handler that knows how to fetch a data identifier has nothing to say -//! about IP. A binding is never a feature of this crate — it implements -//! [`UdsTransport`], and the application names it (`UDSSVC_ARCH_0003`). +//! Message encode/decode is `uds_protocol`'s; session timing is `uds_session`'s; +//! framing and transport are the binding's. Deciding to answer +//! `requestCorrectlyReceivedResponsePending` (0x78) is this crate's, because +//! ISO 14229-2 makes that decision turn on whether the server supports the +//! service; *when* one is due is the session layer's. A binding is never a +//! feature of this crate: it implements [`UdsTransport`], and the application +//! names it (`UDSSVC_ARCH_0003`). //! //! # Design constraints //! //! `no_std` and allocation-free. A handler writes its response into a -//! [`ResponseSink`] this crate owns rather than returning a `Vec`, -//! and no public type carries a `Vec` or a `String`. This is designed in -//! rather than deferred: the signatures that make an API alloc-free are the -//! ones callers depend on, so it cannot be retrofitted later. +//! [`ResponseSink`] this crate owns rather than returning a `Vec`, and no +//! public type carries a `Vec` or a `String`. //! //! # Exhaustive matching //! //! An enumeration is exhaustive only where ISO 14229 fixes the set it mirrors, such as //! [`SessionTransition`]'s four transitions of clause 10.2 Figure 7, [`KeyVerdict`]'s //! Annex I key outcomes, or [`Repeats`]'s ISO 14229-2:2021 Table 9 range. Every other -//! one — an error, an outcome of a call, an event at the transport seam, a set this -//! crate chose — is `#[non_exhaustive]`, because it can grow without a new edition of +//! one (an error, an outcome of a call, an event at the transport seam, a set this +//! crate chose) is `#[non_exhaustive]`, because it can grow without a new edition of //! the standard. #![no_std] @@ -100,18 +89,14 @@ pub use state::{ProtocolState, State}; /// The write vocabulary a handler needs to use a [`ResponseSink`]. /// -/// [`Sink`] is re-exported because `write_all` is one of its methods: without the trait -/// in scope a handler cannot write a byte, and importing it meant declaring a dependency -/// on a crate the application otherwise never names. -/// -/// [`Encode`] is the reason that matters beyond convenience. `uds_protocol`'s types -/// encode *into* a [`Sink`], and [`ResponseSink`] is one — so a handler writes -/// `DtcRecord::new(0xC0, 0x01, 0x23).encode(out)` rather than assembling the bytes by -/// hand, and the length cannot disagree with the value. +/// `write_all` is a [`Sink`] method, so the trait has to be in scope for a handler to +/// write a byte. `uds_protocol`'s types [`Encode`] into a [`Sink`], and [`ResponseSink`] +/// is one, so a handler writes `DtcRecord::new(0xC0, 0x01, 0x23).encode(out)` rather than +/// assembling the bytes by hand. /// -/// A handler discards the write's `Result`. A refused write is recorded by the sink, and -/// the pipeline answers `responseTooLong` (0x14) in its place, so it needs no handling; -/// there is no `From` conversion to a [`NegativeResponseCode`] for `?` to use either. +/// A handler discards the write's `Result`: a refused write is recorded by the sink, and +/// the pipeline answers `responseTooLong` (0x14) in its place. There is no `From` +/// conversion to a [`NegativeResponseCode`] for `?` to use. /// /// ``` /// use uds_services::{ @@ -155,9 +140,8 @@ pub use transport::{ /// The keep-alive modes a client is built in, from `uds_session`. /// -/// Re-exported for the reason the protocol vocabulary above is: [`crate::uds_client`]'s -/// `keep_alive = ..` names one, and an application should not take a dependency on -/// `uds_session` to write it. +/// Re-exported so that [`crate::uds_client`]'s `keep_alive = ..` can name one without the +/// application depending on `uds_session`. pub use uds_session::{FunctionalKeepAlive, KeepAliveMode, PhysicalKeepAlive}; mod select; @@ -179,10 +163,8 @@ pub use services::{Received, Unsettled}; /// The protocol vocabulary this crate's handler signatures are written in. /// /// Re-exported so an application implementing a service trait names one crate, not two. -/// Every one of these is `uds_protocol`'s — this crate defines no sub-function, no reset -/// type and no negative response code (``UDSSVC_ARCH_0002``). They are re-exported rather -/// than re-modelled because a parallel enumeration here would be a second place for the -/// same clause to be written down, and the two would drift. +/// Every one of these is `uds_protocol`'s: this crate defines no sub-function, no reset +/// type and no negative response code (``UDSSVC_ARCH_0002``). /// /// Each sub-function type is range-checked to `0x00`-`0x7F` on construction, so none of /// them can carry `suppressPosRspMsgIndication`: that bit is the pipeline's and never diff --git a/crates/uds_services/src/pipeline.rs b/crates/uds_services/src/pipeline.rs index a32973cd..21ec1c53 100644 --- a/crates/uds_services/src/pipeline.rs +++ b/crates/uds_services/src/pipeline.rs @@ -1,14 +1,12 @@ -//! The clause 8.7 pipeline. ``UDSSVC_ARCH_0004`` — an ordered sequence of stages, each -//! settling the request or passing it on; ``UDSSVC_ARCH_0018`` — nothing outside this +//! The clause 8.7 pipeline. ``UDSSVC_ARCH_0004``: an ordered sequence of stages, each +//! settling the request or passing it on; ``UDSSVC_ARCH_0018``: nothing outside this //! crate calls it. Every item is `pub` only because `uds_server!` expands in the //! application's crate and routes here; none is API an application writes against. //! -//! **The pipeline takes no context struct.** Under ``UDSSVC_ARCH_0035`` this crate -//! already holds the active session, the security level and the authentication state, -//! because it implements the services that own them — so reading them back out of a -//! struct it had just built would be a copy, not an input. What the driver does pass is -//! `uds_session::Ai`, which ISO 14229-1:2020 clause 7.4.1 makes a mandatory parameter of -//! every application layer service primitive anyway. +//! The stages read the active session and the security state from this crate's own +//! [`State`] (``UDSSVC_ARCH_0035``) and take the request's `uds_session::Ai`, which +//! ISO 14229-1:2020 clause 7.4.1 makes a parameter of every application layer service +//! primitive. use crate::services::transfer::{DataTransfer, TransferRequest}; use crate::services::{Effect, SessionTransition}; @@ -117,12 +115,13 @@ pub enum Stage<'a> { /// The common stages, in ISO 14229-1:2020 8.7.2 Figure 5's order (``UDSSVC_ARCH_0006``): /// -/// 1. No service identifier at all: [`Stage::Empty`] (``UDSSVC_ARCH_0005``'s declared -/// reading — the standard does not model the case). -/// 2. Service identifier supported? An unmodelled byte, or a service the assembly list +/// 1. No service identifier at all: [`Stage::Empty`] (``UDSSVC_ARCH_0005``; the standard +/// does not model the case). +/// 2. Service identifier supported? An unmodeled byte, or a service the assembly list /// does not name, settles `serviceNotSupported` (0x11). /// 3. Supported in the active session? Table 23's refusals settle -/// `serviceNotSupportedInActiveSession` (0x7F). +/// `serviceNotSupportedInActiveSession` (0x7F). Figure 5's authentication check +/// (0x34), between 2 and 3, is unconditionally true. /// 3a. The service's checks ahead of its decode (`ahead`): `DataTransfer::SESSIONS`'s /// 0x7F for the three staged transfer services, and Figure 31's minimum length (0x13) /// and format checks (0x31) for `RequestDownload`, both before its full length (see @@ -137,22 +136,15 @@ pub enum Stage<'a> { /// All three read the byte after the service identifier, with /// `suppressPosRspMsgIndicationBit` stripped, before any exact-length test, so a /// trailing byte cannot turn 0x12, 0x7E or 0x33 into 0x13. A request with no such byte -/// fails Figure 6's minimum-length check, which the decode in 5 settles. Row 3, -/// authentication (0x34), is unconditionally true, as in Figure 5. +/// fails Figure 6's minimum-length check, which the decode in 5 settles. /// 5. Only then the service-specific check, where length and format live: a decode /// failure settles the code `uds_protocol` assigns it, `requestOutOfRange` (0x31) for /// a parameter outside its range and otherwise `incorrectMessageLengthOrInvalidFormat` /// (0x13), as do bytes left over (``UDSSVC_ARCH_0005``). /// -/// Clause 8.7.5's pseudo-code agrees: its outer `SWITCH` on the service identifier falls -/// to `DEFAULT: responseCode = SNS` before any `message_length` test, so a malformed -/// request for a service this server lacks is 0x11, not 0x13; and its inner `SWITCH` on -/// the sub-function falls to `DEFAULT: responseCode = SFNS` before the length test of a -/// supported sub-function's arm. Figure 5's authentication check (0x34), between 2 and 3, -/// is unconditionally true here (architecture open question 4). Figure 5's optional SID -/// security check is not evaluated here: no service this crate stages requires a level -/// for its service identifier alone, and a service whose requirement turns on an -/// identifier in its data-parameters checks it in its own stage. +/// Figure 5's optional SID security check is not evaluated: no service this crate stages +/// requires a level for its service identifier alone, and a service whose requirement +/// turns on an identifier in its data-parameters checks it in its own stage. #[doc(hidden)] #[must_use] pub fn begin_with<'a, F, G, H>( @@ -214,7 +206,7 @@ where } match Request::decode(request) { // A service `uds_protocol` names but does not model has no stage either: 0x11, - // as for an unmodelled byte, and never 0x13 for a request that may be well formed. + // as for an unmodeled byte, and never 0x13 for a request that may be well formed. Ok((Request::Other { .. }, _)) => Stage::Settle { sid, nrc: NegativeResponseCode::ServiceNotSupported, @@ -533,7 +525,7 @@ pub async fn read_data_by_identifier( } } -/// ISO 14229-1:2020 clause 11.7, Figure 26 — the check `WriteDataByIdentifier` makes +/// ISO 14229-1:2020 clause 11.7, Figure 26: the check `WriteDataByIdentifier` makes /// ahead of the record's length: an identifier the application does not define, or one /// [`WriteDataByIdentifier::access`] does not admit in the active session, is /// `requestOutOfRange` (0x31). Asked by [`write_data_by_identifier`], and by `dispatch` @@ -555,18 +547,14 @@ pub fn writable_identifier( .ok_or(NegativeResponseCode::RequestOutOfRange) } -/// ISO 14229-1:2020 clause 11.7 — `WriteDataByIdentifier`'s own stage, in Figure 26's -/// order. The decode settled a request without a data record (0x13); then: -/// -/// * [`writable_identifier`]'s check (0x31); -/// * a record [`DataIdentifier::split_record`] finds short, or followed by further bytes, -/// is `incorrectMessageLengthOrInvalidFormat` (0x13); -/// * an identifier none of whose [`Access::levels`] `state` holds unlocked is -/// `securityAccessDenied` (0x33); -/// * a record `split_record` finds malformed is `requestOutOfRange` (0x31). -/// -/// The handler's verdict decides the rest, and the positive response is `6E` and the -/// echoed identifier (Table 279). +/// ISO 14229-1:2020 clause 11.7: `WriteDataByIdentifier`'s own stage, in Figure 26's +/// order. The decode settled a request without a data record (0x13); then +/// [`writable_identifier`]'s check (0x31); a record [`DataIdentifier::split_record`] +/// finds short, or followed by further bytes, is `incorrectMessageLengthOrInvalidFormat` +/// (0x13); an identifier none of whose [`Access::levels`] `state` holds unlocked is +/// `securityAccessDenied` (0x33); a record `split_record` finds malformed is +/// `requestOutOfRange` (0x31). The handler's verdict decides the rest, and the positive +/// response is `6E` and the echoed identifier (Table 279). #[doc(hidden)] pub async fn write_data_by_identifier( services: &mut A, @@ -614,7 +602,7 @@ impl Routine { } } -/// ISO 14229-1:2020 clause 14.2, Figure 30 — the checks `RoutineControl` makes ahead of +/// ISO 14229-1:2020 clause 14.2, Figure 30: the checks `RoutineControl` makes ahead of /// the option record's length, in order: an identifier the application does not define, /// or one [`RoutineControl::access`] does not admit in the active session, is /// `requestOutOfRange` (0x31); a routine none of whose [`Access::levels`] `state` holds @@ -656,13 +644,10 @@ fn admitted( .ok_or(NegativeResponseCode::SubFunctionNotSupported) } -/// ISO 14229-1:2020 clause 14.2 — `RoutineControl`'s own stage, in Figure 30's order. -/// The decode settled a request shorter than its routine identifier (0x13); then: -/// -/// * [`admitted_routine`]'s checks (0x31, 0x33, 0x12); -/// * an option record longer than [`RoutineControl::MAX_OPTION_LEN`] is -/// `incorrectMessageLengthOrInvalidFormat` (0x13). -/// +/// ISO 14229-1:2020 clause 14.2: `RoutineControl`'s own stage, in Figure 30's order. +/// The decode settled a request shorter than its routine identifier (0x13); then +/// [`admitted_routine`]'s checks (0x31, 0x33, 0x12), and an option record longer than +/// [`RoutineControl::MAX_OPTION_LEN`] is `incorrectMessageLengthOrInvalidFormat` (0x13). /// The pipeline then writes `71`, the echoed `routineControlType` and the identifier, and /// the sub-function's own method writes the rest and decides Figure 30's remaining checks. #[doc(hidden)] @@ -722,14 +707,11 @@ pub async fn clear_diagnostic_information( Ok(None) } -/// ISO 14229-1:2020 clause 10.2 — `DiagnosticSessionControl`'s own stage +/// ISO 14229-1:2020 clause 10.2: `DiagnosticSessionControl`'s own stage /// (``UDSSVC_ARCH_0035``): the positive response carries the session and the -/// application's timing, and the session is selected. Whether the session is supported -/// at all (0x12) and from the active one (0x7E) was settled by [`begin`] before the -/// request was decoded, so it is not asked again here. -/// -/// [`SessionTiming`](crate::SessionTiming) is held in Table 29's wire form, so it is sent -/// as stated. +/// application's [`SessionTiming`](crate::SessionTiming), in Table 29's wire form, and +/// the session is selected. Whether the session is supported at all (0x12) and from the +/// active one (0x7E) was settled by [`begin`] before the request was decoded. #[doc(hidden)] pub fn diagnostic_session_control( services: &mut A, @@ -951,7 +933,7 @@ pub fn transfer_before_length( } } -/// ISO 14229-1:2020 clause 10.4 and Annex I — `SecurityAccess`'s own stage +/// ISO 14229-1:2020 clause 10.4 and Annex I: `SecurityAccess`'s own stage /// (``UDSSVC_ARCH_0037``). Whether the level is supported (0x12) and in the active session /// (0x7E) was settled by [`begin`]; this runs Figure I.1 against `state`. /// @@ -966,8 +948,8 @@ pub fn transfer_before_length( /// [`SecurityAccess::MAX_SEED_LEN`] bytes, discarding any seed awaiting a key /// (transitions 7 and 10); /// * otherwise is answered with the application's seed, and its level becomes the one -/// whose key is awaited (transitions 2, 5 and 8). A delay supported by the policy and -/// no longer running has expired, so an attempt count at the limit is reset first. +/// whose key is awaited (transitions 2, 5 and 8). A delay supported by the policy that +/// has stopped running has expired, so an attempt count at the limit is reset first. /// /// A `sendKey` discards the awaited seed whatever its outcome (transitions 9 and 10), and /// is: @@ -1300,8 +1282,7 @@ mod tests { type Did = Did; const MAY_RESPOND_PENDING: bool = false; const MAX_DIDS_PER_REQUEST: usize = 2; - // A plain `fn` returning a ready future: an `async fn` with no `.await` is - // `clippy::unused_async_trait_impl`, which pedantic denies. + // A plain `fn` returning a ready future, since the body never awaits. fn read( &mut self, did: Did, @@ -1545,7 +1526,7 @@ mod tests { )); } - /// ``UDSSVC_ARCH_0005``, ``UDSSVC_ARCH_0006`` check 1 — an unmodelled or unlisted + /// ``UDSSVC_ARCH_0005``, ``UDSSVC_ARCH_0006`` check 1 — an unmodeled or unlisted /// SID settles 0x11, not 0x13. #[test] fn an_unsupported_service_settles_0x11() { @@ -1944,8 +1925,7 @@ mod tests { } } - /// The list is closed. Silencing any other code would be a server that never - /// reports the failure it actually had. + /// The list is closed: no other code is silenced. #[test] fn other_codes_are_never_silenced() { for code in [ diff --git a/crates/uds_services/src/sealed.rs b/crates/uds_services/src/sealed.rs index cb5f9229..5dd4fbbc 100644 --- a/crates/uds_services/src/sealed.rs +++ b/crates/uds_services/src/sealed.rs @@ -1,22 +1,12 @@ -//! The seal on [`ServiceSet`](crate::ServiceSet) and [`Storage`](crate::Storage). +//! The seal on [`ServiceSet`](crate::ServiceSet), [`ClientSet`](crate::ClientSet), +//! [`Storage`](crate::Storage) and [`ClientStorage`](crate::ClientStorage). //! -//! Both traits are implemented by [`crate::uds_server`] and never by hand, and the seal -//! is what makes "never" a compile error rather than a request. What rests on it is the -//! whole derivation argument: an application that writes its own -//! `impl ServiceSet { type Store = Store<1, 1, 1>; .. }` compiles, truncates every request -//! at one byte, and still advertises a block length taken from `MAX_BLOCK_LEN`. -//! -//! [`Storage`](crate::Storage) is sealed outright — `Store` is its only implementor and -//! nothing outside this crate can add one. [`ClientStorage`](crate::ClientStorage) and -//! `ClientStore` are the client's pair, sealed for the same reason: a hand-written -//! implementor could pick buffer lengths that disagree with the maxima the assembly -//! folded. -//! -//! [`ServiceSet`](crate::ServiceSet) is sealed as far as a macro-implemented trait can be. -//! The expansion happens in the application's crate, so the path it names has to be -//! reachable from there; what the seal buys is that implementing `ServiceSet` by hand now -//! requires deliberately naming a `#[doc(hidden)]` item this module documents as not -//! yours. It is a lock, not a wall. +//! The macros implement these traits and nothing else does: a hand-written implementor +//! could pick buffer lengths that disagree with the maxima the assembly folded. +//! [`Storage`](crate::Storage) and [`ClientStorage`](crate::ClientStorage) have their +//! implementors in this crate. The macro-implemented traits are sealed as far as a trait +//! implemented in the application's crate can be: implementing one by hand requires +//! naming a `#[doc(hidden)]` item this module documents as not yours. /// Implemented by [`crate::uds_server`], [`crate::uds_client`], `Store` and /// `ClientStore`. Not yours to implement. diff --git a/crates/uds_services/src/select.rs b/crates/uds_services/src/select.rs index fc6b5092..763ab507 100644 --- a/crates/uds_services/src/select.rs +++ b/crates/uds_services/src/select.rs @@ -1,19 +1,13 @@ //! Waiting on two futures at once, in `core` alone. //! -//! ``UDSSVC_ARCH_0016`` promises that a handler outrunning `tP2_Server` yields and the -//! driver submits a response-pending in that window. Reaching that window means waiting -//! on "the handler completes **or** the deadline passes", and ``UDSSVC_ARCH_0030`` -//! forbids depending on a runtime, so this crate writes its own. It is the only -//! machinery here that is not UDS. +//! The driver waits on "the handler completes **or** the deadline passes" to answer +//! `requestCorrectlyReceivedResponsePending` in that window (``UDSSVC_ARCH_0016``), and +//! ``UDSSVC_ARCH_0030`` forbids depending on a runtime for it. Crate-private: the driver +//! is its only caller. //! -//! **Not public.** The driver is its only caller, and a UDS crate publishing a -//! general-purpose two-way select would be answering questions about executor semantics -//! that are no part of what it promises. -//! -//! The `Unpin` bounds keep it free of `unsafe`. An `async fn` future is `!Unpin`, but -//! `Pin<&mut F>` is always `Unpin` and is a `Future` when `F` is, so a caller wraps each -//! side in [`core::pin::pin!`] and the bound is met safely. The driver additionally uses -//! `Pin::as_mut` so one handler survives several deadlines. +//! The `Unpin` bounds keep it free of `unsafe`: an `async fn` future is `!Unpin`, but +//! `Pin<&mut F>` is `Unpin` and a `Future` when `F` is, so a caller wraps each side in +//! [`core::pin::pin!`], and `Pin::as_mut` lets one handler survive several deadlines. use core::future::Future; use core::pin::Pin; @@ -53,10 +47,10 @@ impl Future for Select2 { /// Complete when either future does, reporting which. /// -/// `first` is polled first, so it wins a tie. The driver passes the handler as `first` -/// deliberately: a handler that finished in the same wake as the deadline has a final -/// response to send, and a response-pending for work already done is a message the -/// standard does not ask for. +/// `first` is polled first, so it wins a tie. The driver passes the handler as `first`: +/// a handler that finished in the same wake as the deadline has a final response to +/// send, and a response-pending for work already done is a message the standard does +/// not ask for. pub(crate) fn select2( first: F, second: G, @@ -120,9 +114,8 @@ mod tests { )); } - /// Left is polled first, so it wins a tie — deliberately. A handler that finished - /// in the same wake as the deadline has a final response to send, and a - /// response-pending for work already done is a message nothing asked for. + /// Left is polled first, so it wins a tie: a handler that finished in the same wake + /// as the deadline has a final response to send. #[test] fn the_left_future_wins_a_tie() { let waker = Waker::noop(); @@ -143,8 +136,8 @@ mod tests { assert!(Pin::new(&mut s).poll(&mut cx).is_pending()); } - /// An `async fn` future is `!Unpin`; `core::pin::pin!` is what satisfies the bound - /// without `unsafe`. This is the shape the driver uses, so it is compiled here. + /// An `async fn` future is `!Unpin`; `core::pin::pin!` satisfies the bound without + /// `unsafe`, in the shape the driver uses. #[test] fn a_pinned_async_block_satisfies_the_unpin_bound() { let waker = Waker::noop(); @@ -158,8 +151,8 @@ mod tests { )); } - /// The driver polls one handler across several deadlines, so the left future must - /// survive being selected on more than once. `Pin::as_mut` is how. + /// The left future survives being selected on more than once, through `Pin::as_mut`, + /// as the driver polls one handler across several deadlines. #[test] fn the_left_future_survives_repeated_selection() { let waker = Waker::noop(); diff --git a/crates/uds_services/src/server.rs b/crates/uds_services/src/server.rs index 0ab612a3..5f02db21 100644 --- a/crates/uds_services/src/server.rs +++ b/crates/uds_services/src/server.rs @@ -1,14 +1,10 @@ //! The run loop. //! -//! ``UDSSVC_ARCH_0040`` — this crate owns the loop. It supplies `uds_session` with +//! ``UDSSVC_ARCH_0040``: this crate owns the loop. It supplies `uds_session` with //! timestamps and drains its outputs, calls a transport to send and receive bytes, and //! dispatches inbound requests to the application's handlers. No other crate in the stack -//! contains a driver, and a consuming application writes none. -//! -//! ISO 14229-2 defines a service interface between the session layer and its user and -//! names that user as the ISO 14229-1 layer. A third party sitting between them is a -//! component no standard describes, and it is the component that went a full design cycle -//! with no owner. +//! contains a driver, and a consuming application writes none: ISO 14229-2 names the +//! session layer's user as the ISO 14229-1 layer, which is this crate. use crate::pipeline::settle; use crate::select::{Either, select2}; @@ -30,17 +26,10 @@ use uds_session::{ /// The UDS server: an application's services, its storage, a session layer and a /// transport. /// -/// Named `Server` although `uds_session::Server` exists, because a consuming application -/// never imports `uds_session` — only a binding author does, and a binding author can -/// alias. -/// /// `A` is the [`ServiceSet`] marker [`crate::uds_server`] generates; the application it -/// holds is `A::App`. -/// -/// `PEERS` is a parameter rather than a derived constant because the session layer's type -/// is `uds_session::Server`, and an associated const of a generic cannot be a const -/// generic argument. An application does not write it: [`crate::uds_server`]'s -/// `peers = N` supplies it and the macro emits the alias, so the count appears once. +/// holds is `A::App`. `PEERS` sizes the session layer's `uds_session::Server`; +/// [`crate::uds_server`]'s `peers = N` supplies it through the alias it emits, so an +/// application never writes it. #[derive(Debug)] pub struct Server { services: A::App, @@ -63,29 +52,18 @@ pub struct Server { impl Server { /// Assemble a server. /// - /// A `const fn`, so a server can be built in place, in a `static` initialiser, rather - /// than on the stack: the storage is inline and can be several kilobytes, which a - /// small-stack target could not hold as a temporary. [`Storage::EMPTY`] being an - /// associated const is what allows it. - /// - /// [`Self::step`] takes `&mut self`, so the `static` has to yield a `&'static mut` - /// once. `static_cell::ConstStaticCell::new(EcuServer::new(..))` and its `take()` do - /// that without a stack temporary, as [`crate::uds_server`]'s example shows. A plain - /// `static SERVER: EcuServer` lands in read-only memory and can never be stepped, and - /// `StaticCell::init(EcuServer::new(..))` builds the value on the stack before moving - /// it. In a `ConstStaticCell` the server lives in initialised data, so its initial - /// image, buffers included, is also stored in flash. - /// - /// A transport built at run time, such as `uds_on_ip`'s `DoIpTransport` over - /// `simple_doip`'s entity, cannot be in a `const` initialiser. Its server goes in a - /// `StaticCell` through `init_with(|| EcuServer::new(..))`, and is built on the - /// caller's stack and copied in, so that stack holds the whole server once; see - /// `DoIpTransport`'s bare-metal notes for what that costs. + /// A `const fn`, so a server whose inline storage runs to kilobytes is built in a + /// `static` initializer rather than as a stack temporary. [`Self::step`] takes + /// `&mut self`, so the `static` is a `static_cell::ConstStaticCell` whose `take()` + /// yields the one `&'static mut`, as [`crate::uds_server`]'s example shows; a plain + /// `static` is read-only and cannot be stepped. A transport built at run time, such + /// as `uds_on_ip`'s `DoIpTransport`, cannot be in a `const` initializer: its server + /// goes in a `StaticCell` through `init_with(|| EcuServer::new(..))`, built on the + /// caller's stack and copied in. /// /// The driver builds its own session, so an application never names `uds_session`. /// - /// `PEERS` must be 1, checked at compile time: milestone 1 keeps one slot for a - /// selecting response awaiting its confirmation (see [`crate::uds_server`]). + /// `PEERS` must be 1, checked at compile time (see [`crate::uds_server`]). /// /// # Arguments /// @@ -96,8 +74,7 @@ impl Server { /// * `own` - the [`Address`] every response is sent from: its `S_SA`, which /// ``UDSS_LLR_0051`` makes the sending entity's. It is not derived from the request, /// because a functionally addressed request's `S_TA` is the functional group - /// address, and a response sent from that address would not say which server - /// answered. + /// address. /// * `params` - the session layer's [`ServerParams`]: `tS3_Server` and the /// response-pending lead, which the server enforces, and the `P2Server_max` and /// `P2*Server_max` it enforces only where `services` has no @@ -129,8 +106,7 @@ impl Server { &mut self.services } - /// The transport, for inspection between events — the counterpart of - /// [`Self::services`]. + /// The transport, for inspection between events. #[must_use] pub fn transport(&self) -> &T { &self.transport @@ -147,24 +123,18 @@ impl Server { /// Handle one transport event. Returns [`Stepped::Stopped`] where a restarting reset /// was confirmed during this step, which the caller acts on before stepping again. /// - /// **An `Err` is terminal for this server instance** (``UDSSVC_ARCH_0040``, a - /// milestone-1 limit). A transport error unwinds through the drains with `?`, so a - /// session-hook decision a drain already recorded — a `tS3_Server` expiry, a - /// confirmed session — may not have been applied, and the application's state and - /// the session layer may disagree afterwards. Do not call `step` on this instance - /// again. Nothing here recovers it in place; [`Self::into_parts`] takes the services - /// and the transport back out. + /// **An `Err` is terminal for this server instance** (``UDSSVC_ARCH_0040``). A + /// transport error unwinds through the drains, so a session-hook decision a drain + /// recorded (a `tS3_Server` expiry, a confirmed session) may not have been applied, + /// and the application's state and the session layer may disagree afterwards. Do not + /// call `step` on this instance again; [`Self::into_parts`] takes the services and + /// the transport back out. /// /// # Cancel safety /// /// The returned future is **not** cancel-safe: drop it only between `step` calls. /// Dropped at any await other than the transport's `next_event`, it leaves the - /// instance in the state the terminal-`Err` paragraph above describes — outputs - /// drained but not applied, a `Pending` slot cleared without its confirmation - /// applied, a handler dropped with its association still outstanding — and - /// `uds_services::State` may disagree with the session layer. Tracked in - /// `luminartech/uds_stack#19` (), - /// together with recovery after an `Err`. + /// instance in the state an `Err` does. /// /// # Errors /// @@ -327,19 +297,15 @@ impl Server { /// Run until a restarting reset is confirmed, or the transport fails. /// - /// **The `Err` this returns is terminal for this server instance** - /// (``UDSSVC_ARCH_0040``, a milestone-1 limit), as an `Err` from [`Self::step`] is: a - /// session-hook decision already recorded may not have been applied, so the - /// application's state and the session layer may disagree afterwards. Do not call - /// `run` or `step` on this instance again. Nothing here recovers it in place; + /// **The `Err` this returns is terminal for this server instance**, as an `Err` from + /// [`Self::step`] is. Do not call `run` or `step` on this instance again; /// [`Self::into_parts`] takes the services and the transport back out. /// /// # Cancel safety /// - /// Not cancel-safe, for the reason [`Self::step`] is not: dropping this future drops - /// a `step` future, and not necessarily at the transport's `next_event`. See - /// `step`'s `# Cancel safety` section, which also names the tracking issue, - /// `luminartech/uds_stack#19`. To stop between events, drive `step` in your own loop. + /// Not cancel-safe: dropping this future drops a `step` future, and not necessarily + /// at the transport's `next_event`. To stop between events, drive `step` in your own + /// loop. /// /// # Errors /// @@ -391,12 +357,10 @@ pub enum Stopped { } /// Load the `P2` pair of the session in force into the session layer, so the request about -/// to be indicated at `now` opens the window its session advertised. -/// -/// The session layer is ticked at `now` first and its expiries applied, so a `tS3_Server` -/// running out at that instant returns to the default session before its pair is read. -/// The parameters are then set at the same `now`, which can expire nothing further, and -/// ``UDSS_LLR_0076`` leaves any window already open on the value it was loaded with. +/// to be indicated at `now` opens the window its session advertised. The session layer is +/// ticked at `now` first, so a `tS3_Server` running out at that instant returns to the +/// default session before its pair is read; ``UDSS_LLR_0076`` leaves any window already +/// open on the value it was loaded with. /// /// # Panics /// @@ -440,15 +404,10 @@ async fn retime( /// The response in flight that carries an effect, such as a `DiagnosticSessionControl` /// response's session: the effect takes place on the `Confirm` that reports it sent -/// (``UDSS_LLR_0085``). One slot: a server answers one request at a time -/// (``UDSS_LLR_0108``), so with one peer one such response is in flight at most. -/// -/// **Milestone 1 accepts `peers = 1` only** ([`Server::new`] fails to compile for any -/// other `PEERS`). A server answers one request at a time, but a response awaiting its -/// confirmation is no longer the request in progress: with a second tester, that -/// tester's selecting response could be accepted while the first is still unconfirmed, -/// and it would overwrite this slot. The first confirmation would then match nothing and -/// its session would never be applied. +/// (``UDSS_LLR_0085``). One slot, which is why [`Server::new`] accepts `PEERS == 1` only: +/// a server answers one request at a time (``UDSS_LLR_0108``), but a response awaiting +/// its confirmation is not the request in progress, and a second tester's selecting +/// response could overwrite this slot before the first was confirmed. #[derive(Debug, Clone, Copy)] struct Pending { /// The response's addressing, exactly as submitted: `t_data_conf` matches by it. @@ -457,17 +416,11 @@ struct Pending { effect: Effect, } -/// Where a response to `request` goes: the request went tester -> ECU, the response goes -/// back the other way. The exact `Ai` `s_data_req` registers, and so the one the -/// response's `DataConf` must carry to match it. -/// -/// Sent from `own`, never from the request's `S_TA`: for a functional request that is -/// the group address, and ``UDSS_LLR_0051`` makes `S_SA` the sending entity's address. -/// -/// Physically addressed whatever the request was: a server answers the one client that -/// asked, so a response to a functional request is a physical message to that client. -/// That is an observation the requirement set relies on, not a clause of -/// ISO 14229-2:2021 9.6 (``UDSS_LLR_0026``'s rationale). +/// Where a response to `request` goes: the `Ai` `s_data_req` registers, and so the one +/// the response's `DataConf` carries to match it. Sent from `own`, never from the +/// request's `S_TA`, which for a functional request is the group address +/// (``UDSS_LLR_0051``), and physically addressed whatever the request was, since a server +/// answers the one client that asked (``UDSS_LLR_0026``). const fn reply_address(own: Address, request: Ai) -> Ai { Ai { sa: own, @@ -495,12 +448,11 @@ impl Deferred { /// Both records, in arrival order: `self` was recorded first. /// - /// A later timeout clears an earlier session confirmation. [`apply`] runs the timeout - /// before the confirmation, so keeping both would leave the application in the - /// confirmed session while the session layer, which expired it afterwards, is in the - /// default one. A confirmed reset is kept: its response was sent, and both end in the - /// default session. A later confirmation after an earlier timeout keeps both, which is - /// already the order `apply` runs them in; of two confirmations the later wins. + /// A later timeout clears an earlier session confirmation, since [`apply`] runs the + /// timeout first and the session layer has expired the session. A confirmed reset is + /// kept: its response was sent, and both end in the default session. A later + /// confirmation after an earlier timeout keeps both; of two confirmations the later + /// wins. fn merge(self, later: Self) -> Self { Self { timed_out: self.timed_out || later.timed_out, @@ -532,7 +484,7 @@ struct Drained<'d> { /// What follows a transmission: [`AfterSend::ServerLeaves`] for the final positive /// `DiagnosticSessionControl` response to `leaving`, and [`AfterSend::Continue`] for -/// everything else — a 0x78 to the same addressing and a negative response included, as +/// everything else, a 0x78 to the same addressing and a negative response included, as /// neither starts with the positive response identifier. fn after_send(leaving: Option, ai: Ai, data: &[u8]) -> AfterSend { let positive = UdsServiceType::DiagnosticSessionControl.to_response_sid(); @@ -543,15 +495,11 @@ fn after_send(leaving: Option, ai: Ai, data: &[u8]) -> AfterSend { } } -/// The one place every session output is handled (``UDSSVC_ARCH_0040``). Every reaction, +/// The one place every session output is handled (``UDSSVC_ARCH_0040``): every reaction, /// from every input, passes through here, because an expiry can surface from any of -/// them. Acts on what needs only the transport and the pending record; records the rest. -/// -/// Inside the drain, not after it: a reaction may yield several `Transmit`s and all of -/// them must reach the transport. The reaction borrows the session and `t_data_req` -/// borrows the transport, which is why they arrive as separate arguments rather than as -/// one `&mut self` — disjoint fields are disjoint borrows only while nothing has merged -/// them. +/// them. Acts on what needs only the transport and the pending record, every `Transmit` +/// included, and records the rest. The reaction borrows the session and `t_data_req` +/// borrows the transport, so they arrive as separate arguments. async fn drain<'d, T: UdsTransport, const PEERS: usize>( mut reaction: uds_session::ServerReaction<'_, 'd, PEERS>, transport: &mut T, @@ -612,13 +560,9 @@ async fn drain<'d, T: UdsTransport, const PEERS: usize>( /// restarting reset is confirmed or its completion is reported (``UDSSVC_ARCH_0002``, /// ``UDSSVC_ARCH_0038``, ``UDSSVC_ARCH_0045``). Returns `Some` where that reset restarts. /// -/// That fixed order is the arrival order. Within one drain, ``UDSS_LLR_0081`` puts the -/// expiry snapshots ahead of the input's own outputs, so a `SessionTimeout` always -/// precedes the `Confirm` beside it. Across drains, [`Deferred::merge`] drops a session -/// confirmation that a later timeout overtook, so a surviving session confirmation is -/// one a timeout preceded. A reset confirmation is kept, since its response was sent, -/// and running it after a timeout that overtook it ends in the same state: a restart -/// ends in the default session either way, and a continuing reset changes no session. +/// That fixed order is the arrival order: within one drain ``UDSS_LLR_0081`` puts the +/// expiry snapshots ahead of the input's own outputs, and across drains +/// [`Deferred::merge`] drops a session confirmation that a later timeout overtook. fn apply( services: &mut A::App, state: &mut A::State, @@ -666,7 +610,7 @@ struct Serving { reply_to: Ai, /// The request's service identifier, which a negative response echoes. sid: u8, - /// ``UDSSVC_ARCH_0032`` — whether the service admits a 0x78 at all, as + /// ``UDSSVC_ARCH_0032``: whether the service admits a 0x78 at all, as /// `ServiceSet::may_respond_pending` reported it. may_pend: bool, /// `reply_to`, where the final positive response selects a session that @@ -677,9 +621,7 @@ struct Serving { impl Serving { /// The request `ai` sent, its service identifier `sid`, before it is dispatched. - /// - /// ``UDSSVC_ARCH_0032`` — whether 0x78 is admissible is the service's to say, never - /// the driver's. + /// ``UDSSVC_ARCH_0032``: whether 0x78 is admissible is the service's to say. fn new(own: Address, ai: Ai, sid: u8) -> Self { Self { ai, @@ -746,16 +688,12 @@ struct Served { /// /// Returns how the handling ended, whether a 0x78 was accepted meanwhile, and what the /// drains recorded for the application, which waits until the handler releases -/// `&mut services`/`&mut state`. The caller settles: the handler future holds the sink -/// until it drops. `waiting` is scoped per iteration so it drops before the 0x78 path -/// needs `&mut transport`. -/// -/// A message arriving meanwhile finds the protocol instance occupied (ISO 14229-1:2020 -/// 8.7.6): the keep-alive `TesterPresent` bypasses it ([`keep_alive`]) and anything else -/// is refused ([`refuse_busy`]). The requesting client's close ends the handling; another -/// client's ends nothing. Every event other than the requesting client's close first -/// answers an overrun due by its own timestamp ([`overrun_at`]), so none is lost to the -/// event's own input. +/// `&mut services`/`&mut state`. The caller settles, once the handler has released the +/// sink. A message arriving meanwhile finds the protocol instance occupied (ISO +/// 14229-1:2020 8.7.6): the keep-alive `TesterPresent` bypasses it ([`keep_alive`]) and +/// anything else is refused ([`refuse_busy`]). The requesting client's close ends the +/// handling; another client's ends nothing. Every other event first answers an overrun due +/// by its own timestamp ([`overrun_at`]), so none is lost to the event's own input. async fn serve< A: ServiceSet, H: Future, @@ -772,17 +710,14 @@ async fn serve< let mut deferred = Deferred::NONE; let mut pending_sent = false; let mut owed = false; - // A loop over `handler.as_mut()`, not a one-shot: a plain - // `select2(handler, waiting)` drops the handler the moment the deadline wins, - // which is the opposite of what a response-pending is for. + // A loop over `handler.as_mut()`: the handler survives each deadline it loses. loop { let deadline = session.next_deadline(); let event = { let waiting = core::pin::pin!(transport.next_event(concurrent, deadline)); select2(handler.as_mut(), waiting).await }; - // milestone-1 limit: an `Err` at any `?` below drops what was recorded; see - // `Server::step`'s doc. + // An `Err` at any `?` below drops what was recorded; see `Server::step`'s doc. match event { Either::Left(unsettled) => { return Ok(Served { @@ -795,12 +730,10 @@ async fn serve< Either::Right(Ok(TransportEvent::Closed { peer, .. })) if peer == serving.ai.sa => { - // The exchange is over either way: a server does not reconnect - // (ISO 14229-5:2022 REQ 7.8 and REQ 7.10 have the client reconnect and - // activate routing). A close reaching *this* arm is never - // ISO 14229-5:2022 REQ 7.9's or 7.11's — those follow a positive response, - // and nothing positive has been sent yet — so whether it was expected - // changes nothing. + // The exchange is over: a server does not reconnect (ISO 14229-5:2022 + // REQ 7.8 and REQ 7.10 have the client reconnect). Nothing positive has + // been sent yet, so this close is never REQ 7.9's or 7.11's, and whether + // it was expected changes nothing. return Ok(Served { ended: Ended::Closed, pending_sent, @@ -810,8 +743,7 @@ async fn serve< Either::Right(Ok(event)) => { let now = transport.now(); let o = overrun_at(session, transport, pending, now, serving).await?; - // UDSSVC_ARCH_0009 rule 3 — kept for `settle`, which runs once the - // handler has finished. Never cleared: one accepted 0x78 is enough. + // UDSSVC_ARCH_0009 rule 3, kept for `settle`: one accepted 0x78 is enough. pending_sent |= o.sent_pending; owed |= o.owed; deferred = deferred.merge(o.deferred); @@ -875,16 +807,13 @@ async fn keep_alive( Ok(drain(reaction, transport, pending, None).await?.deferred) } -/// ISO 14229-1:2020 8.7.6 — any other message arriving while a service is in progress +/// ISO 14229-1:2020 8.7.6: any other message arriving while a service is in progress /// finds the protocol instance occupied and is refused `busyRepeatRequest` (Annex A) from -/// its service identifier alone; the service in progress continues. -/// -/// Never indicated as a request, which ``UDSS_LLR_0108`` would have replace the service -/// in progress, and submitted as a busy refusal, which answers no service -/// (``UDSS_LLR_0187``). A refusal the session layer refuses in turn, the client's -/// addressing still awaiting a confirmation, is dropped: ignoring the request is Annex J -/// Figure J.2's other branch, and the client's own timeout recovers it. A message with no -/// service identifier has nothing to echo and is dropped too (``UDSSVC_ARCH_0005``). +/// its service identifier alone; the service in progress continues. Never indicated as a +/// request (``UDSS_LLR_0108``), and submitted as a busy refusal, which answers no service +/// (``UDSS_LLR_0187``). A refusal the session layer refuses in turn is dropped, which is +/// Annex J Figure J.2's other branch; so is a message with no service identifier +/// (``UDSSVC_ARCH_0005``). async fn refuse_busy( session: &mut SessionServer, transport: &mut T, @@ -933,18 +862,13 @@ async fn conclude( } /// Submit the handler's outcome: a final response, or the completion report a -/// suppressed request owes. -/// -/// A free function rather than a method because `response` borrows the server's store. -/// Returns what the application must hear about, for the caller to [`apply`]. +/// suppressed request owes. Returns what the application must hear about, for the caller +/// to [`apply`]. /// -/// A final response refused under ``UDSS_LLR_0061`` or ``UDSS_LLR_0062`` is waited out -/// and resubmitted once. The first is reachable because [`select2`] lets the handler win -/// a tie, so a handler can finish after a 0x78 was accepted and before that 0x78's -/// `DataConf` was drained; the second wherever every association is still awaiting a -/// confirmation. A refusal for any other cause loses the response. The tester has had -/// at most a 0x78, and the service stays in progress until its timer runs out. That is -/// a milestone-1 limit; nothing retries it. +/// A final response refused under ``UDSS_LLR_0061`` (the 0x78's `DataConf` not yet +/// drained) or ``UDSS_LLR_0062`` (every association awaiting a confirmation) is waited +/// out and resubmitted once. A refusal for any other cause loses the response: the tester +/// has had at most a 0x78, and the service stays in progress until its timer runs out. async fn answer( session: &mut SessionServer, transport: &mut T, @@ -961,8 +885,7 @@ async fn answer( let now = transport.now(); let reaction = final_response(session, now, serving.reply_to, response, selection(effect)); - // milestone-1 limit: an `Err` at any `?` below drops what was recorded; see - // `Server::step`'s doc. + // An `Err` at any `?` below drops what was recorded; see `Server::step`'s doc. let mut d = drain(reaction, transport, pending, serving.leaving).await?; let mut deferred = d.deferred; let awaited = match d.rejected { @@ -1001,9 +924,8 @@ async fn answer( }); } } - // UDSSVC_ARCH_0040 — a milestone-1 limit: a final response refused for any - // cause but an outstanding or unavailable association (or refused again after - // waiting one out) is lost, with no retry. See `answer`'s doc. + // UDSSVC_ARCH_0040: a final response refused for any other cause, or refused + // again after waiting one out, is lost. See `answer`'s doc. Some(_refused) => {} } Ok(deferred) @@ -1076,21 +998,16 @@ enum Awaited { } /// Wait for a transmission outstanding to be confirmed, so the final response -/// ``UDSS_LLR_0061`` or ``UDSS_LLR_0062`` refused can be resubmitted. -/// -/// Each event takes the path it takes mid-handler: an overrun due by its timestamp is -/// answered first ([`overrun_at`]), a `DataConf` then reaches `t_data_conf`, and a -/// message arriving is bypassed or refused as in [`serve`]. A 0x78 refused here is not -/// owed: the final response the awaited confirmation frees the way for supersedes it. +/// ``UDSS_LLR_0061`` or ``UDSS_LLR_0062`` refused can be resubmitted. Each event takes +/// the path it takes mid-handler: an overrun due by its timestamp is answered first +/// ([`overrun_at`]), a `DataConf` reaches `t_data_conf`, and a message arriving is +/// bypassed or refused as in [`serve`]. A 0x78 refused here is not owed: the final +/// response supersedes it. /// /// Returns what the drains recorded once the awaited `DataConf` has been drained and -/// accepted — `serving.reply_to`'s for [`Awaited::Reply`], any for [`Awaited::Any`] — or -/// `None` where the requesting client's link closed first. -/// -/// **Unbounded** (milestone-1 limit, ``UDSSVC_ARCH_0040``). The wait ends on the awaited -/// `DataConf`, that client's `Closed`, or a transport error, and on nothing else: a -/// transport that never confirms stalls the driver here. That is the assumption of use -/// [`UdsTransport`] states, a `DataConf` for every accepted `t_data_req`. +/// accepted, or `None` where the requesting client's link closed first. The wait is +/// unbounded (``UDSSVC_ARCH_0040``): it ends on the awaited `DataConf`, that client's +/// `Closed`, or a transport error, which [`UdsTransport`]'s obligations make certain. async fn await_confirmation( session: &mut SessionServer, transport: &mut T, @@ -1108,8 +1025,8 @@ async fn await_confirmation( { return Ok(None); } - // The handler's outcome is already settled, so whether a 0x78 goes out no - // longer reaches `settle`; only what the drains recorded is kept. + // The handler's outcome is already settled, so whether a 0x78 goes out does + // not reach `settle`; only what the drains recorded is kept. let now = transport.now(); let o = overrun_at(session, transport, pending, now, serving).await?; deferred = deferred.merge(o.deferred); @@ -1183,7 +1100,7 @@ async fn overrun_at( /// Answer `requestCorrectlyReceivedResponsePending` (0x78) if one has come due. /// -/// ``UDSSVC_ARCH_0031``/``0032`` — the deadline passing is not itself the overrun: the +/// ``UDSSVC_ARCH_0031``/``0032``: the deadline passing is not itself the overrun; the /// session layer is ticked and ``UDSS_LLR_0117``'s `ResponseOverrun` is the output that /// says a 0x78 is owed. It is an ordinary transmission, and whether it was *accepted* is /// the verdict ``UDSSVC_ARCH_0009`` rule 3 reads. @@ -1213,8 +1130,8 @@ async fn answer_overrun( /// Submit the 0x78 for `serving`. /// /// The overrun that makes it due is reported once, so a 0x78 refused because the -/// client's addressing awaits a confirmation — a busy refusal's, or a final response's -/// to another request — is reported `owed`, for the caller to resubmit once a +/// client's addressing awaits a confirmation (a busy refusal's, or a final response's +/// to another request) is reported `owed`, for the caller to resubmit once a /// confirmation has freed it. async fn submit_pending( session: &mut SessionServer, @@ -1297,9 +1214,7 @@ mod tests { } /// A confirmation recorded mid-handler and then overtaken by a `tS3_Server` expiry - /// is dropped. `apply` runs the timeout first, so keeping it would leave the - /// application in the confirmed session while the session layer is in the default - /// one. + /// is dropped. #[test] fn a_later_timeout_clears_an_earlier_confirmation() { let merged = confirm(S::ExtendedDiagnosticSession).merge(TIMEOUT); diff --git a/crates/uds_services/src/services/data.rs b/crates/uds_services/src/services/data.rs index a6afc832..3c42013a 100644 --- a/crates/uds_services/src/services/data.rs +++ b/crates/uds_services/src/services/data.rs @@ -1,13 +1,13 @@ -//! Data transmission — ISO 14229-1:2020 clause 11. +//! Data transmission, ISO 14229-1:2020 clause 11. use crate::{Access, DataIdentifier, ResponseSink}; use uds_protocol::NegativeResponseCode; /// `ReadDataByIdentifier` (0x22). /// -/// ``UDSSVC_ARCH_0008`` — a request naming several identifiers of which some are -/// supported yields a **positive** response carrying the supported ones, which is why the -/// response buffer must hold the worst case rather than one record. +/// ``UDSSVC_ARCH_0008``: a request naming several identifiers of which some are +/// supported yields a **positive** response carrying the supported ones, so the response +/// buffer holds the worst case rather than one record. pub trait ReadDataByIdentifier { /// This application's data identifier enumeration. type Did: DataIdentifier; @@ -42,7 +42,7 @@ pub trait ReadDataByIdentifier { /// `WriteDataByIdentifier` (0x2E). /// -/// ISO 14229-1:2020 clause 11.7, Figure 26 — the pipeline settles, in order: a request +/// ISO 14229-1:2020 clause 11.7, Figure 26: the pipeline settles, in order, a request /// with no data record (0x13), an identifier [`DataIdentifier::from_u16`] rejects or /// [`Self::access`] does not admit in the active session (0x31), a record /// [`DataIdentifier::split_record`] finds short or followed by more bytes (0x13), an @@ -52,14 +52,13 @@ pub trait WriteDataByIdentifier { /// This application's data identifier enumeration. type Did: DataIdentifier; - /// ``UDSSVC_ARCH_0033`` — a write reaching non-volatile memory usually cannot accept - /// further requests while it completes. + /// ``UDSSVC_ARCH_0033``. const MAY_RESPOND_PENDING: bool; /// Where `did` may be written and which levels unlock writing it, or `None` where it /// is not writable at all. /// - /// Figure 26 — `None`, or an active session outside [`Access::sessions`], settles the + /// Figure 26: `None`, or an active session outside [`Access::sessions`], settles the /// request `requestOutOfRange` (0x31), as clause 11.7.4 has it for a read-only /// identifier, before the record's length is checked; none of [`Access::levels`] /// unlocked settles it `securityAccessDenied` (0x33), after the length is checked and diff --git a/crates/uds_services/src/services/dtc.rs b/crates/uds_services/src/services/dtc.rs index f0bb52f1..b8541dc5 100644 --- a/crates/uds_services/src/services/dtc.rs +++ b/crates/uds_services/src/services/dtc.rs @@ -1,4 +1,4 @@ -//! Stored data transmission — ISO 14229-1:2020 clause 12. +//! Stored data transmission, ISO 14229-1:2020 clause 12. use crate::{Access, ResponseSink}; use uds_protocol::{ @@ -7,33 +7,30 @@ use uds_protocol::{ /// Which shape of `ReadDTCInformation` response a report type produces. /// -/// Each ISO 14229-1:2020 clause 12.3 report type named here has one of seven response -/// layouts, and the layout fixes the header and record widths — they are not the -/// application's to choose. A server declares the layouts it answers in -/// [`ReadDtcInformation::REPORTS`] and [`crate::uds_server`] folds the widest of them into -/// the response buffer. -/// -/// Each corresponds to one or more of [`uds_protocol::ReadDtcInfoResponse`]'s variants, -/// which is where the widths below are read from rather than restated. +/// Each ISO 14229-1:2020 clause 12.3 report type has one of seven response layouts, and +/// the layout fixes the header and record widths. A server declares the layouts it +/// answers in [`ReadDtcInformation::REPORTS`] and [`crate::uds_server`] folds the widest +/// of them into the response buffer. Each corresponds to one or more of +/// [`uds_protocol::ReadDtcInfoResponse`]'s variants. #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] #[non_exhaustive] pub enum DtcReportKind { - /// A count and no records — sub-functions `0x01` and `0x07`. + /// A count and no records: sub-functions `0x01` and `0x07`. Count, - /// `(DTC, status)` pairs — `0x02`, `0x0A`-`0x0E`, `0x15`. + /// `(DTC, status)` pairs: `0x02`, `0x0A`-`0x0E`, `0x15`. DtcList, - /// `(DTC, fault detection counter)` pairs — `0x14`. + /// `(DTC, fault detection counter)` pairs: `0x14`. FaultDetectionCounters, /// `DTCAndSeverityRecord`s, which carry a severity and a `DTCFunctionalUnit` byte - /// that [`Self::DtcList`]'s records do not — `0x08` and `0x09`. + /// that [`Self::DtcList`]'s records do not: `0x08` and `0x09`. SeverityList, - /// WWH-OBD severity records — `0x42`. + /// WWH-OBD severity records: `0x42`. WwhObdSeverity, - /// One DTC's snapshot records, raw — `0x04`. Bounded in bytes, not records: a + /// One DTC's snapshot records, raw: `0x04`. Bounded in bytes, not records, since a /// record's width is the application's identifiers' to know. A client reads it; a /// server cannot list it in [`ReadDtcInformation::REPORTS`]. SnapshotRecords, - /// One DTC's extended data records, raw — `0x06`. Bounded in bytes, and read only by + /// One DTC's extended data records, raw: `0x06`. Bounded in bytes, and read only by /// a client, as [`Self::SnapshotRecords`] is. ExtDataRecords, } @@ -74,37 +71,28 @@ impl DtcReportKind { /// implements is the application's. Figure 6's questions are asked of the report type /// alone, a [`ReadDtcInfoReportType`], before the request's parameters are decoded; the /// report then arrives decoded as a [`ReadDtcInfoSubFunction`], which carries that report -/// type's own parameters — a status mask, a record number, a -/// [`DtcRecord`] — so a handler matches on the report it was asked for rather than -/// parsing the bytes behind it. +/// type's own parameters (a status mask, a record number, a [`DtcRecord`]). pub trait ReadDtcInformation { /// ``UDSSVC_ARCH_0033``. const MAY_RESPOND_PENDING: bool; - /// The most DTC records this server returns in one response. - /// - /// Folded into the response buffer. A server with more stored DTCs than this cannot - /// report them all in one message, which is a deployment decision rather than a - /// defect — clause 12.3 provides paged report types for that case. + /// The most DTC records this server returns in one response, folded into the + /// response buffer. Clause 12.3 provides paged report types for a server with more + /// stored DTCs than one message holds. const MAX_DTCS: usize; /// The response layouts this server answers; see [`DtcReportKind`]. /// /// [`crate::uds_server`] folds `max(header + MAX_DTCS * record)` over these into the - /// response buffer. Declaring the layouts rather than a width is what makes the - /// buffer right: a server answering [`DtcReportKind::SeverityList`] needs six-byte - /// records where [`DtcReportKind::DtcList`] needs four, and neither width is the - /// application's to know. - /// - /// The assembly fails to compile where this lists a layout whose - /// [`DtcReportKind::record_len`] is `None`: [`MAX_DTCS`](Self::MAX_DTCS) does not - /// bound it. + /// response buffer, each layout fixing its own record width. The assembly fails to + /// compile where this lists a layout whose [`DtcReportKind::record_len`] is `None`: + /// [`MAX_DTCS`](Self::MAX_DTCS) does not bound it. const REPORTS: &'static [DtcReportKind]; /// Where `report` is available and which levels unlock it, or `None` where this /// server does not support it. /// - /// ISO 14229-1:2020 8.7.3.1 Figure 6, ``UDSSVC_ARCH_0007`` — in that order, `None` + /// ISO 14229-1:2020 8.7.3.1 Figure 6, ``UDSSVC_ARCH_0007``: in that order, `None` /// settles the request `subFunctionNotSupported` (0x12), an active session outside /// [`Access::sessions`] `subFunctionNotSupportedInActiveSession` (0x7E), and none of /// [`Access::levels`] unlocked `securityAccessDenied` (0x33), each before the @@ -122,22 +110,20 @@ pub trait ReadDtcInformation { /// # Arguments /// /// * `request` - the report type and its parameters; see - /// [`ReadDtcInfoSubFunction`], whose [`report_type`] [`Self::access`] admitted. - /// There is no separate parameter slice: a report type's parameters are - /// fixed-width and ride on its variant, so a malformed one is rejected with + /// [`ReadDtcInfoSubFunction`], whose [`report_type`] [`Self::access`] admitted. A + /// report type's parameters are fixed-width, so a malformed one is rejected with /// `incorrectMessageLengthOrInvalidFormat` (0x13) before it reaches here. /// * `out` - where the response is written after the `59` service identifier and - /// the echoed report type, which the pipeline writes: everything after `59 xx` is - /// the handler's, whatever the report type's layout puts first. Clause 12.3.3 gives - /// each report type's layout; [`DtcReportKind::header_len`] counts the pipeline's - /// two bytes with the rest of the header. + /// the echoed report type, which the pipeline writes. Clause 12.3.3 gives each + /// report type's layout; [`DtcReportKind::header_len`] counts the pipeline's two + /// bytes with the rest of the header. /// /// [`report_type`]: ReadDtcInfoSubFunction::report_type /// /// # Errors /// /// The [`NegativeResponseCode`] where the report cannot be given, clause 12.3.4: - /// `requestOutOfRange` (0x31) for a `DTCMaskRecord` the server does not recognise or + /// `requestOutOfRange` (0x31) for a `DTCMaskRecord` the server does not recognize or /// an invalid record number. /// /// A refused write to `out` needs no handling: the sink records the refusal and the @@ -152,10 +138,10 @@ pub trait ReadDtcInformation { /// `ClearDiagnosticInformation` (0x14). /// -/// ISO 14229-1:2020 clause 12.2, Figure 28 — the pipeline settles a request that is not +/// ISO 14229-1:2020 clause 12.2, Figure 28: the pipeline settles a request that is not /// four or five bytes (0x13); every other check is the handler's. pub trait ClearDiagnosticInformation { - /// ``UDSSVC_ARCH_0033`` — clearing stored data is frequently slow enough to need one. + /// ``UDSSVC_ARCH_0033``. const MAY_RESPOND_PENDING: bool; /// Clear the DTCs `group` selects, optionally restricted to one memory. diff --git a/crates/uds_services/src/services/mod.rs b/crates/uds_services/src/services/mod.rs index 98c36969..7d6ef661 100644 --- a/crates/uds_services/src/services/mod.rs +++ b/crates/uds_services/src/services/mod.rs @@ -1,21 +1,14 @@ //! The traits an application implements. //! -//! ``UDSSVC_ARCH_0012`` — each service is its own trait, so the assembly in -//! [`crate::uds_server`] knows exactly which a server supports. A single large trait with -//! defaulted methods would make "supported" mean "did not override", which is not a -//! distinction clause 8.7 can act on: `serviceNotSupported` (0x11) is decided from a -//! list, and the assembly is that list. +//! ``UDSSVC_ARCH_0012``: each service is its own trait, so the assembly in +//! [`crate::uds_server`] is the list `serviceNotSupported` (0x11) is decided from. Every +//! handler returns `Result<(), NegativeResponseCode>`, `uds_protocol`'s code, and writes +//! into a [`ResponseSink`]. //! -//! Every handler returns `Result<(), NegativeResponseCode>` and writes into a -//! [`ResponseSink`]. The negative response code is `uds_protocol`'s; -//! this crate defines none. -//! -//! **A service trait whose handler can be in progress declares `MAY_RESPOND_PENDING` -//! with no default**, so omitting it fails to compile. A default of `false` would let a -//! forgotten declaration silently mean "never 0x78", which is a conformance decision -//! nobody made. [`DiagnosticSessionControl`] and [`TesterPresent`] do not declare it: -//! neither is ever awaited, so no response-pending can come due and the constant would -//! have had one possible value and no effect. +//! A service trait whose handler can be in progress declares `MAY_RESPOND_PENDING` with +//! no default, so omitting it fails to compile. [`DiagnosticSessionControl`] and +//! [`TesterPresent`] do not declare it: neither is ever awaited, so no response-pending +//! can come due. use crate::pipeline::Settling; use crate::state::ProtocolState; @@ -71,11 +64,10 @@ impl From> for Effect { /// What clause 8.7 decided, and which effect the response carries. /// -/// ``UDSSVC_ARCH_0016`` — a negative response is written bytes and arrives as -/// [`Responded::Yes`]; silence is a distinguishable outcome because only it is a reason -/// not to transmit. The effect rides on both arms because a suppressed -/// `DiagnosticSessionControl` still changes session, through the completion report -/// (``UDSS_LLR_0086``, ``UDSS_LLR_0098``). +/// ``UDSSVC_ARCH_0016``: a negative response is written bytes and arrives as +/// [`Responded::Yes`]; silence is the one outcome that is a reason not to transmit. The +/// effect rides on both arms because a suppressed `DiagnosticSessionControl` still +/// changes session, through the completion report (``UDSS_LLR_0086``, ``UDSS_LLR_0098``). #[doc(hidden)] #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum Responded { @@ -104,20 +96,14 @@ impl Responded { /// What a request's handler left, before clause 8.7's last stages have run. /// -/// ``UDSSVC_ARCH_0016`` — [`ServiceSet::dispatch`] returns this, and the driver hands it +/// ``UDSSVC_ARCH_0016``: [`ServiceSet::dispatch`] returns this, and the driver hands it /// to the pipeline's `settle`, which applies "response fits" (``UDSSVC_ARCH_0017``) and /// ``UDSSVC_ARCH_0009``'s three suppression rules and yields the [`Responded`] the driver -/// acts on. **The driver settles, not `dispatch`, because rule 3's input is known only to -/// the driver**: whether a `requestCorrectlyReceivedResponsePending` (0x78) for this -/// request was accepted for transmission is learned in the driver's loop while the -/// handler runs, and it is final only once the handler has finished. Settling after the -/// handler future completes reads it then, as a plain `bool`, with nothing shared between -/// the handler and the loop. +/// acts on. The driver settles because rule 3's input, whether a 0x78 for this request +/// was accepted for transmission, is final only once the handler has finished. /// -/// Public only because the trait an application's assembly implements names it; an -/// application never builds or reads one. Its fields are private and its constructors -/// hidden, for [`crate::uds_server`]'s emitted `dispatch` alone, which decides nothing: -/// it records which of three shapes the request took — no service identifier +/// Public only because the assembly's trait names it; an application never builds or +/// reads one. It records which of three shapes the request took: no service identifier /// (``UDSSVC_ARCH_0005``), a code the common stages settled, or a service stage's outcome. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub struct Unsettled(Shape); @@ -192,9 +178,8 @@ impl Unsettled { /// A change of diagnostic session, classified. /// -/// ``UDSSVC_ARCH_0038`` — ISO 14229-1:2020 clause 10.2 Figure 7's key names these four -/// cases, and which occurred determines what configuration state resets. Classifying it -/// here is what stops every application reimplementing Table 23. +/// ``UDSSVC_ARCH_0038``: ISO 14229-1:2020 clause 10.2 Figure 7's key names these four +/// cases, and which occurred determines what configuration state resets. #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] pub enum SessionTransition { /// Default to default: a re-entry. @@ -250,14 +235,10 @@ impl<'a> Received<'a> { /// One application's assembled service implementations. /// -/// ``UDSSVC_ARCH_0013`` — implemented by [`crate::uds_server`], never by hand, on the +/// ``UDSSVC_ARCH_0013``: implemented by [`crate::uds_server`], never by hand, on the /// marker type it generates in the invoking crate. The application, [`Self::App`], may -/// live in any crate. -/// -/// Sealed through [`crate::sealed`]. The whole derivation argument — that -/// [`Self::Store`]'s lengths are folded from this application's declared maxima — holds -/// only while the macro is what chooses them; a hand-written impl can pick any lengths it -/// likes and still advertise the declared maxima on the wire. +/// live in any crate. Sealed through [`crate::sealed`], so that [`Self::Store`]'s lengths +/// are always the ones the macro folded from this application's declared maxima. pub trait ServiceSet: crate::sealed::Sealed { /// The application whose service traits this assembly calls. #[doc(hidden)] @@ -274,10 +255,10 @@ pub trait ServiceSet: crate::sealed::Sealed { /// Run the clause 8.7 pipeline for `request` up to its last stages, writing any /// response into `out`, and return the handler's outcome [`Unsettled`]. /// - /// ``UDSSVC_ARCH_0004`` — a function of the request, its addressing and this crate's - /// state, with no transport, clock or session layer involved. ``UDSSVC_ARCH_0015`` is - /// why `ai` is here. ``UDSSVC_ARCH_0016`` — the driver, not this future, settles the - /// result: see [`Unsettled`] for why. + /// ``UDSSVC_ARCH_0004``: a function of the request, its addressing + /// (``UDSSVC_ARCH_0015``) and this crate's state, with no transport, clock or session + /// layer involved. ``UDSSVC_ARCH_0016``: the driver, not this future, settles the + /// result; see [`Unsettled`]. /// /// # Arguments /// @@ -300,11 +281,10 @@ pub trait ServiceSet: crate::sealed::Sealed { /// The server is starting: what Annex I transition 1 owes at start-up. /// /// [`crate::Server`] calls it on its first step and on the first step after a - /// [`Stopped`](crate::Stopped), because [`Server::new`](crate::Server::new) is a - /// `const fn` and cannot call into the application. Emitted by the assembly; where - /// `SecurityAccess` is listed, it starts the delay of every level whose stored attempt - /// count is at its limit (see [`crate::SecurityAccess::delay`]), and otherwise it does - /// nothing. + /// [`Stopped`](crate::Stopped), since [`Server::new`](crate::Server::new) is `const`. + /// Where `SecurityAccess` is listed, it starts the delay of every level whose stored + /// attempt count is at its limit (see [`crate::SecurityAccess::delay`]); otherwise it + /// does nothing. /// /// # Arguments /// @@ -313,11 +293,9 @@ pub trait ServiceSet: crate::sealed::Sealed { #[doc(hidden)] fn start_up(app: &mut Self::App, state: &mut Self::State); - /// `tS3_Server` expired: return to the default session and tell the application. - /// - /// ``UDSS_LLR_0100`` reports the expiry; ``UDSSVC_ARCH_0038`` has the application - /// told through `DiagnosticSessionControl::on_transition`. Emitted by the assembly, - /// because only it knows whether that service is implemented; a no-op where it is not. + /// `tS3_Server` expired (``UDSS_LLR_0100``): return to the default session and tell + /// the application through `DiagnosticSessionControl::on_transition` + /// (``UDSSVC_ARCH_0038``), where that service is listed. /// /// # Arguments /// @@ -328,9 +306,9 @@ pub trait ServiceSet: crate::sealed::Sealed { #[doc(hidden)] fn session_timed_out(app: &mut Self::App, state: &mut Self::State); - /// A response selecting `selected` was confirmed sent: enter it and tell the - /// application. ``UDSS_LLR_0085``/``0086`` is the moment; ``UDSSVC_ARCH_0038`` the - /// call. Emitted by the assembly, as [`Self::session_timed_out`] is. + /// A response selecting `selected` was confirmed sent (``UDSS_LLR_0085``/``0086``): + /// enter it and tell the application (``UDSSVC_ARCH_0038``), as + /// [`Self::session_timed_out`] does. /// /// # Arguments /// @@ -346,10 +324,10 @@ pub trait ServiceSet: crate::sealed::Sealed { ); /// The response accepting the reset `kind` was confirmed sent, or its suppression - /// completed: tell the application, and for [`AfterReset::Restart`] return to the - /// default session (telling the application, Figure 7 key 4) and then to - /// [`ProtocolState::INITIAL`]. ``UDSSVC_ARCH_0045``. Emitted by the assembly; called - /// only where `EcuReset` is listed. + /// completed (``UDSSVC_ARCH_0045``): tell the application, and for + /// [`AfterReset::Restart`] return to the default session (telling the application, + /// Figure 7 key 4) and then to [`ProtocolState::INITIAL`]. Called only where + /// `EcuReset` is listed. /// /// # Arguments /// @@ -393,12 +371,9 @@ pub trait ServiceSet: crate::sealed::Sealed { #[doc(hidden)] fn leaves_running_software(app: &Self::App, session: DiagnosticSessionType) -> bool; - /// Whether this server implements `service` at all. - /// - /// ``UDSSVC_ARCH_0006`` — Figure 5's first mandatory check, and the one only the - /// assembly list can answer. A byte that names no service arrives as - /// [`UdsServiceType::UnsupportedDiagnosticService`], which no assembly list contains, - /// so it answers `false` without a special case. + /// Whether this server implements `service` at all: ``UDSSVC_ARCH_0006``, Figure 5's + /// first mandatory check. A byte that names no service arrives as + /// [`UdsServiceType::UnsupportedDiagnosticService`], which no assembly list contains. #[doc(hidden)] fn supports(service: UdsServiceType) -> bool; @@ -408,25 +383,17 @@ pub trait ServiceSet: crate::sealed::Sealed { #[doc(hidden)] fn may_respond_pending(service: UdsServiceType) -> bool; - /// Whether `request`, addressed this way, may proceed while a service is already in - /// progress. - /// - /// True for exactly the **functionally addressed** `TesterPresent` `3E 80`, the - /// "valid `TesterPresent` message with SPRMIB=true" that ISO 14229-1:2020 8.7.6 lets - /// bypass the occupied diagnostic protocol instance. The addressing is a parameter - /// because that condition turns on it and the bytes do not carry it; `ai` is the one - /// the driver drained from the transport event alongside `request`. + /// Whether `request`, addressed `ai`, may proceed while a service is already in + /// progress: true for exactly the **functionally addressed** `TesterPresent` `3E 80`, + /// the "valid `TesterPresent` message with SPRMIB=true" that ISO 14229-1:2020 8.7.6 + /// lets bypass the occupied diagnostic protocol instance. [`crate::Server`] answers + /// anything else arriving mid-service `busyRepeatRequest` (0x21). /// - /// Clause 8.7.6 admits a second exception — a request in `0x00`–`0x0F`, which aborts - /// the active service — and no assembled server can meet it. That range is OBD - /// territory, which `uds_protocol` does not model, so [`crate::uds_server`] can - /// assemble no service in it and the case cannot arise. Its absence here is coverage, - /// not an omission. + /// Clause 8.7.6's second exception, a request in `0x00`-`0x0F` that aborts the active + /// service, cannot arise: that range is OBD, which `uds_protocol` does not model, so + /// [`crate::uds_server`] assembles no service in it. /// - /// An associated function, because the driver asks while a handler holds the - /// services: the answer depends on the assembly, never on a handler's state. - /// [`crate::Server`] answers anything else arriving mid-service `busyRepeatRequest` - /// (0x21). + /// An associated function, because the driver asks while a handler holds the services. #[doc(hidden)] fn is_concurrent_exception(request: &[u8], ai: Ai) -> bool; } @@ -435,9 +402,8 @@ pub trait ServiceSet: crate::sealed::Sealed { mod tests { use super::{Effect, Responded}; - /// ``UDSSVC_ARCH_0016`` — silence must be distinguishable from an empty response, - /// because only one of them is a reason not to transmit. A `bool` would leave the - /// driver guessing which, and this test is what fails if someone replaces it. + /// ``UDSSVC_ARCH_0016``: silence is distinguishable from an empty response, because + /// only one of them is a reason not to transmit. #[test] fn silence_and_an_empty_response_are_different_outcomes() { assert_ne!( diff --git a/crates/uds_services/src/services/routine.rs b/crates/uds_services/src/services/routine.rs index c4fca9c4..1bfacf64 100644 --- a/crates/uds_services/src/services/routine.rs +++ b/crates/uds_services/src/services/routine.rs @@ -1,38 +1,28 @@ -//! Routine — ISO 14229-1:2020 clause 14. +//! Routine, ISO 14229-1:2020 clause 14. use crate::{Access, ResponseSink, RoutineIdentifier}; use uds_protocol::{NegativeResponseCode, RoutineControlSubFunction}; /// `RoutineControl` (0x31). /// -/// ``UDSSVC_ARCH_0007`` — Figure 5 excludes service identifier 0x31 from the centralised -/// sub-function stage, because a routine's sub-function is meaningful only together with -/// its routine identifier: whether `stopRoutine` is supported is a property of the -/// routine, not of the service. **Three methods rather than one with a sub-function -/// parameter**, so that asymmetry is visible in the type rather than documented. Settles -/// open question 5. Whether a routine supports a sub-function at all is nonetheless also a -/// lookup, [`Self::supports`], because Figure 30 answers it before the option record's -/// length is checked, and the pipeline must not hand a method a record longer than -/// [`Self::MAX_OPTION_LEN`]. It is separate from [`Self::access`] because Figure 30 asks -/// it after the routine's security check, and keyed by routine for the reason above. +/// ``UDSSVC_ARCH_0007``: Figure 5 excludes 0x31 from the centralized sub-function stage, +/// since whether `stopRoutine` is supported is a property of the routine, so each +/// sub-function is its own method and [`Self::supports`] is keyed by routine. /// -/// ISO 14229-1:2020 clause 14.2, Figure 30 — the pipeline settles, in order: a request +/// ISO 14229-1:2020 clause 14.2, Figure 30: the pipeline settles, in order, a request /// shorter than its routine identifier (0x13), an identifier /// [`RoutineIdentifier::from_u16`] rejects or [`Self::access`] does not admit in the /// active session (0x31), a routine none of whose levels is unlocked (0x33), a -/// `routineControlType` Table 426 reserves or [`Self::supports`] refuses for this routine -/// (0x12), and an option record longer than [`Self::MAX_OPTION_LEN`] (0x13). Only then -/// is the sub-function's method asked, and Figure 30's remaining checks are its own: the -/// record's length for this routine (0x13), -/// conditions (0x22), the record's content (0x31) and the request sequence (0x24). Each -/// method writes `routineInfo` and any `routineStatusRecord` into its `out`, after the -/// `71`, the echoed `routineControlType` and the identifier the pipeline wrote (Table -/// 428). +/// `routineControlType` Table 426 reserves or [`Self::supports`] refuses (0x12), and an +/// option record longer than [`Self::MAX_OPTION_LEN`] (0x13). The sub-function's method +/// then decides Figure 30's remaining checks (0x13, 0x22, 0x31, 0x24) and writes +/// `routineInfo` and any `routineStatusRecord` after the `71`, the echoed +/// `routineControlType` and the identifier the pipeline wrote (Table 428). pub trait RoutineControl { /// This application's routine identifier enumeration. type Rid: RoutineIdentifier; - /// ``UDSSVC_ARCH_0033`` — a routine outrunning `tP2_Server` is the paradigm case. + /// ``UDSSVC_ARCH_0033``. const MAY_RESPOND_PENDING: bool; /// The longest option record this server accepts; a longer one is @@ -42,7 +32,7 @@ pub trait RoutineControl { /// Where `routine` is available and which levels unlock it, or `None` where this /// server does not define it. /// - /// Figure 30 — `None`, or an active session outside [`Access::sessions`], settles the + /// Figure 30: `None`, or an active session outside [`Access::sessions`], settles the /// request `requestOutOfRange` (0x31); none of [`Access::levels`] unlocked settles it /// `securityAccessDenied` (0x33). Both come before [`Self::supports`] is asked. /// @@ -53,11 +43,11 @@ pub trait RoutineControl { /// Whether `routine` supports `control` at all. /// - /// Figure 30, "`SubFunction` supported for `routineIdentifier`?" — asked only for a + /// Figure 30, "`SubFunction` supported for `routineIdentifier`?": asked only for a /// `routine` [`Self::access`] admitted, and only for `startRoutine`, `stopRoutine` - /// and `requestRoutineResults`: a reserved `routineControlType` is 0x12 without being - /// asked. A `false` settles the request `subFunctionNotSupported` (0x12) before the - /// option record's length is checked and without the matching method being asked. + /// and `requestRoutineResults`, a reserved `routineControlType` being 0x12 without + /// this asked. A `false` settles the request `subFunctionNotSupported` (0x12) before + /// the option record's length is checked and without the matching method being asked. /// /// # Arguments /// @@ -182,15 +172,9 @@ mod tests { } } - /// ``UDSSVC_ARCH_0007`` — Figure 5 excludes 0x31 from the centralised sub-function - /// stage, because whether `stopRoutine` is supported is a property of the *routine*, - /// not of the service. Three separate methods make that asymmetry structural, and - /// `supports` answers 0x12 for the pair. Settles open question 5. - /// - /// This is a trait-bound check and nothing more: it establishes that `start`, `stop` - /// and `results` are each required, because the fixture above satisfies - /// `RoutineControl` only by implementing all three. No 0x12 behaviour is observed - /// here; `tests/stages.rs` observes it through the pipeline. + /// ``UDSSVC_ARCH_0007``: `start`, `stop` and `results` are each required. A + /// trait-bound check only; `tests/stages.rs` observes the 0x12 behavior through the + /// pipeline. #[test] fn routine_control_requires_start_stop_and_results_separately() { fn assert_three_methods() {} diff --git a/crates/uds_services/src/services/security.rs b/crates/uds_services/src/services/security.rs index f835d622..2a7b4d0c 100644 --- a/crates/uds_services/src/services/security.rs +++ b/crates/uds_services/src/services/security.rs @@ -1,17 +1,14 @@ -//! Security access — ISO 14229-1:2020 clause 10.4 and Annex I. +//! Security access, ISO 14229-1:2020 clause 10.4 and Annex I. //! -//! ``UDSSVC_ARCH_0037`` — the sequence is Annex I's Figure I.1, a four-state machine with -//! ten transitions, identical for every server, and it lives here. Its corrected topology -//! matters: transition 9 is `B → A` and transition 10 is `D → C`, so a failed `sendKey` -//! **discards the stored seed** and the client must request a new one. +//! ``UDSSVC_ARCH_0037``: the sequence is Annex I's Figure I.1, a four-state machine with +//! ten transitions, identical for every server, and it lives here. Transition 9 is +//! `B -> A` and transition 10 is `D -> C`, so a failed `sendKey` **discards the stored +//! seed** and the client must request a new one. The application supplies the +//! cryptography and the non-volatile storage; it never sees a state, never counts an +//! attempt, and never decides between 0x35 and 0x36. //! -//! The application supplies the cryptography and the non-volatile storage, which are the -//! two things this crate cannot have. It never sees a state, never counts an attempt, and -//! never decides between 0x35 and 0x36. -//! -//! **There is no `Authentication` trait.** Service 0x29 has no `uds_protocol` message -//! type in either direction, so it decodes to `Request::Other` and settles 0x11 — a -//! server that wants it cannot implement it. +//! There is no `Authentication` trait: service 0x29 has no `uds_protocol` message type +//! in either direction, so it decodes to `Request::Other` and settles 0x11. use crate::{ResponseSink, Sessions}; use core::num::NonZeroU8; @@ -20,10 +17,9 @@ use uds_protocol::NegativeResponseCode; /// How a level's attempts and delay are governed. /// /// ISO 14229-1:2020 Table I.1 makes the attempt counter and the delay optional, and -/// states a fallback for declining both — "a random seed shall always be used" — which -/// [`SecurityAccess::seed`] must honour under [`SecurityPolicy::RandomSeedOnly`]. Whether -/// a seed is static is otherwise the seed's own property, and [`SecurityAccess::seed`]'s -/// to decide. +/// states a fallback for declining both, "a random seed shall always be used", which +/// [`SecurityAccess::seed`] must honor under [`SecurityPolicy::RandomSeedOnly`]. Whether +/// a seed is static is otherwise [`SecurityAccess::seed`]'s to decide. #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] pub enum SecurityPolicy { /// Neither counter nor delay: Table I.1's fallback, so the seed is always random. @@ -62,11 +58,9 @@ pub struct SecurityLevel(u8); impl SecurityLevel { /// The level a `requestSeed` sub-function names, or `None` where it is not one. /// - /// A `requestSeed` sub-function is odd by construction, and must leave room for its - /// `sendKey` partner: a sub-function byte is seven bits, because bit 7 is - /// `suppressPosRspMsgIndication`, so both halves of the pair have to fall in - /// `0x00`–`0x7F`. `0x7F` itself is rejected because its partner would be `0x80`, - /// which is not a sub-function value at all. + /// A `requestSeed` sub-function is odd, and its `sendKey` partner has to fall in the + /// seven-bit sub-function range too, so `0x7F` is rejected: its partner would be + /// `0x80`, which is not a sub-function value. /// /// # Examples /// @@ -109,8 +103,8 @@ impl SecurityLevel { /// Whether a key matched. /// /// A verdict rather than a `Result`, because a wrong key is a normal outcome of the -/// exchange. Which negative response it produces — `invalidKey` (0x35) or -/// `exceedNumberOfAttempts` (0x36) — depends on the attempt count this crate maintains. +/// exchange. Which negative response it produces, `invalidKey` (0x35) or +/// `exceedNumberOfAttempts` (0x36), depends on the attempt count this crate maintains. #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] pub enum KeyVerdict { /// Correct; Figure I.1 transition 3 or 10. @@ -122,8 +116,7 @@ pub enum KeyVerdict { /// `SecurityAccess` (0x27). /// /// ``UDSSVC_ARCH_0034`` puts the attempt counter in the stack rather than the -/// application: clause 10.4 makes it server-global, so a per-application implementation -/// would be the same code written many times, wrong differently each time. +/// application: clause 10.4 makes it server-global. pub trait SecurityAccess { /// ``UDSSVC_ARCH_0033``. const MAY_RESPOND_PENDING: bool; @@ -149,13 +142,12 @@ pub trait SecurityAccess { /// The sessions `level` is available in, or `None` where this server does not /// support it. /// - /// ISO 14229-1:2020 8.7.3.1 Figure 6, ``UDSSVC_ARCH_0007`` — asked for the - /// `requestSeed` and the `sendKey` of the level alike: `None` settles the request + /// ISO 14229-1:2020 8.7.3.1 Figure 6, ``UDSSVC_ARCH_0007``: asked for the + /// `requestSeed` and the `sendKey` of the level alike, `None` settles the request /// `subFunctionNotSupported` (0x12), and an active session outside the set /// `subFunctionNotSupportedInActiveSession` (0x7E). A set rather than an - /// [`Access`](crate::Access) because this is the service that unlocks: no level is - /// required to ask for one. The default session never reaches here: Table 23 refuses - /// the service there with 0x7F. + /// [`Access`](crate::Access): no level is required to ask for one. The default + /// session never reaches here: Table 23 refuses the service there with 0x7F. /// /// # Arguments /// @@ -177,11 +169,9 @@ pub trait SecurityAccess { /// * `level` - the level the `requestSeed` names; see [`SecurityLevel`]. fn preconditions_met(&self, level: SecurityLevel) -> bool; - /// The attempt count for `level`, as last stored. - /// - /// The application never computes one — it persists the number this crate hands it, - /// because Annex I requires the count to survive a power cycle and only the - /// application has non-volatile storage. + /// The attempt count for `level`, as last stored. The application persists the + /// number this crate hands it, since Annex I requires the count to survive a power + /// cycle. fn load_attempts(&self, level: SecurityLevel) -> u8; /// Store the count this crate computed. @@ -193,10 +183,10 @@ pub trait SecurityAccess { /// each `requestSeed` once its pre-conditions are met. [`Delay::Running`] answers the /// request `requiredTimeDelayNotExpired` (0x37) (Table I.2 transition 4). /// [`Delay::Expired`] has this crate reset the attempt count, as the timer's - /// expiry does in Table I.2. [`Delay::Idle`] with the stored count at the limit — the - /// state after a restart, which a RAM timer does not survive — has this crate call - /// [`Self::start_delay`] (transition 1, "Start `Delay_Timer` … if required on start - /// up"). So a lockout outlives a power cycle. + /// expiry does in Table I.2. [`Delay::Idle`] with the stored count at the limit, the + /// state after a restart, has this crate call [`Self::start_delay`] (transition 1, + /// "Start `Delay_Timer` ... if required on start up"), so a lockout outlives a power + /// cycle. /// /// # Arguments /// @@ -209,10 +199,9 @@ pub trait SecurityAccess { /// Write a seed for `level` into `out`. /// /// Never called for an already-unlocked level: Figure I.1 transition 7 fixes that - /// answer as an all-zero seed, and this crate supplies it, because it knows what is - /// unlocked and the application does not. So the seed written here must not be all - /// zero: clause 10.4.1 forbids that for a locked level, and a debug build panics on - /// an empty or all-zero seed that `out` accepted. + /// answer as an all-zero seed, which this crate supplies. The seed written here must + /// not be all zero: clause 10.4.1 forbids that for a locked level, and a debug build + /// panics on an empty or all-zero seed that `out` accepted. /// /// Whether a repeated `requestSeed` gets the same seed is this method's to decide /// (Table I.1 `Static_Seed`, vehicle-manufacturer specific). A static seed is returned @@ -268,10 +257,8 @@ pub trait SecurityAccess { mod tests { use super::{KeyVerdict, SecurityLevel}; - /// Clause 10.4.2 pairs `requestSeed` (odd) with `sendKey` (the next even value). - /// `SecurityLevel` holds the pair, so `yy == xx + 1` is structural and Annex I's - /// 0x24 answers only what it is meant to: a `sendKey` for a level whose seed was - /// never requested. + /// Clause 10.4.2 pairs `requestSeed` (odd) with `sendKey` (the next even value), and + /// `SecurityLevel` holds the pair. #[test] fn a_level_is_the_pair_not_a_raw_sub_function() { let l = SecurityLevel::from_request_seed(0x01).expect("0x01 is a requestSeed"); @@ -292,8 +279,7 @@ mod tests { } /// A wrong key is a verdict, not an error: which negative response it produces - /// (0x35 or 0x36) depends on the attempt count this crate maintains, so the - /// application cannot choose it. + /// (0x35 or 0x36) depends on the attempt count this crate maintains. #[test] fn a_wrong_key_is_a_verdict() { assert_ne!(KeyVerdict::Valid, KeyVerdict::Invalid); diff --git a/crates/uds_services/src/services/session.rs b/crates/uds_services/src/services/session.rs index 62979183..46ecc129 100644 --- a/crates/uds_services/src/services/session.rs +++ b/crates/uds_services/src/services/session.rs @@ -1,6 +1,6 @@ -//! Diagnostics and communication management — ISO 14229-1:2020 clause 10. +//! Diagnostics and communication management, ISO 14229-1:2020 clause 10. //! -//! ``UDSSVC_ARCH_0012`` — one trait per service. +//! ``UDSSVC_ARCH_0012``: one trait per service. use crate::{Access, SessionTransition}; use uds_protocol::{ @@ -8,7 +8,7 @@ use uds_protocol::{ NegativeResponseCode, P2StarServerMax, ResetType, SubnetNumber, }; -/// ``UDSS_LLR_0073`` — the session layer learns only default/non-default. +/// ``UDSS_LLR_0073``: the session layer learns only default/non-default. pub(crate) fn selection_of( session: DiagnosticSessionType, ) -> uds_session::SessionSelection { @@ -22,10 +22,9 @@ pub(crate) fn selection_of( /// The `P2` pair a session advertises in its `DiagnosticSessionControl` response, and /// that [`crate::Server`] enforces while the session is in force. /// -/// ISO 14229-1:2020 clause 10.2 — the positive response carries `P2Server_max` and -/// `P2*Server_max` for the session being entered, so they are a property of the session. -/// Held in Table 29's wire form, so every value is advertised exactly and the window -/// enforced is the one advertised. +/// ISO 14229-1:2020 clause 10.2: the positive response carries `P2Server_max` and +/// `P2*Server_max` for the session being entered. Held in Table 29's wire form, so the +/// window enforced is the one advertised. #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] pub struct SessionTiming { /// `P2Server_max`, in 1 ms units. @@ -52,21 +51,15 @@ impl SessionTiming { /// classifies the change before the application sees it, so an application never tracks /// a session itself. /// -/// **No `MAY_RESPOND_PENDING`**, unlike the services in ``UDSSVC_ARCH_0033``. A -/// response-pending is what the driver sends while it is still awaiting a handler, and -/// nothing here is awaited: [`Self::supports`], [`Self::supported_from`], -/// [`Self::timing`] and [`Self::leaves_running_software`] are lookups the driver makes -/// before composing or sending the response, and -/// [`Self::on_transition`] runs after that response has gone out. There is no window in -/// which a 0x78 could come due, so the constant would have had one possible value and no -/// effect. +/// No `MAY_RESPOND_PENDING`, unlike the services in ``UDSSVC_ARCH_0033``: nothing here is +/// awaited, so there is no window in which a 0x78 could come due. pub trait DiagnosticSessionControl { /// The longest positive response beyond the mandatory four timing bytes. const MAX_RESPONSE_LEN: usize; /// Whether this server supports `session` at all, from whichever session. /// - /// ISO 14229-1:2020 8.7.3.1 Figure 6, "`SubFunction` supported ever for the SID?" — + /// ISO 14229-1:2020 8.7.3.1 Figure 6, "`SubFunction` supported ever for the SID?", /// ``UDSSVC_ARCH_0007`` row 2. A `false` settles the request /// `subFunctionNotSupported` (0x12) before [`Self::supported_from`] is asked. /// @@ -80,15 +73,11 @@ pub trait DiagnosticSessionControl { /// Whether `session` may be entered from `active`. /// /// ISO 14229-1:2020 8.7.3.1 Figure 6, "`SubFunction` supported in active session - /// for the SID?" — ``UDSSVC_ARCH_0007`` row 4. Asked only for a `session` that - /// [`Self::supports`] accepted, so a `false` here is always "supported, but not from - /// this session": the pipeline settles it `subFunctionNotSupportedInActiveSession` - /// (0x7E), which Annex A reserves for a sub-function "known to be supported in - /// another session". A `session` this server never supports is 0x12, however this - /// answers. - /// - /// No default: the table of which session is reachable from which is the - /// application's to state. A server that restricts no transition returns `true`. + /// for the SID?", ``UDSSVC_ARCH_0007`` row 4. Asked only for a `session` that + /// [`Self::supports`] accepted, so a `false` settles the request + /// `subFunctionNotSupportedInActiveSession` (0x7E), which Annex A reserves for a + /// sub-function "known to be supported in another session". A server that restricts + /// no transition returns `true`. /// /// # Arguments /// @@ -103,10 +92,9 @@ pub trait DiagnosticSessionControl { /// The `P2` pair `session` advertises and, while it is in force, the server enforces. /// /// Asked for the entered session when composing the positive response, and for the - /// session in force each time a request arrives: [`crate::Server`] loads the pair - /// into the session layer then, so the request's window is the one its session - /// advertised, and any session the server is in — the default one included — is - /// timed by this and not by the [`ServerParams`](crate::ServerParams) passed to + /// session in force each time a request arrives, when [`crate::Server`] loads the pair + /// into the session layer. Every session the server is in, the default one included, + /// is timed by this and not by the [`ServerParams`](crate::ServerParams) passed to /// [`Server::new`](crate::Server::new). The returned pair must keep those params' /// `response_pending_lead` well formed /// ([`ServerParams::lead_fits`](crate::ServerParams::lead_fits)); the server panics @@ -120,20 +108,17 @@ pub trait DiagnosticSessionControl { /// Whether entering `session` leaves the software this server is running. /// /// ISO 14229-1:2020 10.2.2.2 Table 25 lets the programming session run in boot - /// software, left only by `ECUReset`, `10 01` or a session timeout, with the - /// application software restarted where it is valid. So it is each binary's to say: - /// an application jumping to its bootloader answers `true` for - /// [`ProgrammingSession`](DiagnosticSessionType::ProgrammingSession), and the + /// software, so each binary says: an application jumping to its bootloader answers + /// `true` for [`ProgrammingSession`](DiagnosticSessionType::ProgrammingSession), the /// bootloader `true` for [`DefaultSession`](DiagnosticSessionType::DefaultSession) - /// where valid application software exists. A server that programs from within its - /// application answers `false` for every session. [`crate::Server`] then hands the - /// final positive response [`AfterSend::ServerLeaves`](crate::AfterSend::ServerLeaves), - /// on which a connection-oriented transport closes (ISO 14229-5:2022 REQ 7.9). + /// where valid application software exists, and a server that programs from within + /// its application `false` for every session. [`crate::Server`] hands the final + /// positive response [`AfterSend::ServerLeaves`](crate::AfterSend::ServerLeaves), on + /// which a connection-oriented transport closes (ISO 14229-5:2022 REQ 7.9). /// - /// **A suppressed session change sends no response**, so no message carries the fact - /// and no orderly close precedes the departure: a server left by `10 82` drops its - /// connection unannounced. Nothing here prevents that; a client that needs the close - /// must not suppress the response. + /// A suppressed session change sends no response, so no orderly close precedes the + /// departure: a server left by `10 82` drops its connection unannounced. A client + /// that needs the close must not suppress the response. /// /// # Arguments /// @@ -144,10 +129,8 @@ pub trait DiagnosticSessionControl { /// /// ``UDSSVC_ARCH_0038``. The application applies what clause 10.2 Figure 7 owes the /// state it holds: `transition` says which of the four cases occurred, and `entered` - /// which session, because what a case owes can turn on it — `ControlDTCSetting` and - /// `CommunicationControl` state is the application's, and resumes on entering a - /// session where the service is not supported, and a session may run in other - /// software than the one now running (see [`Self::leaves_running_software`]). + /// which session, since `ControlDTCSetting` and `CommunicationControl` state resumes + /// on entering a session where the service is not supported. /// /// # Arguments /// @@ -189,13 +172,13 @@ pub enum AfterReset { /// identifier and echoed `resetType`; [`Self::reset`] decides whether the reset is /// accepted and supplies the `powerDownTime` that follows. pub trait EcuReset { - /// ``UDSSVC_ARCH_0033`` — a reset legitimately sets this. + /// ``UDSSVC_ARCH_0033``. const MAY_RESPOND_PENDING: bool; /// Where `kind` is available and which levels unlock it, or `None` where this /// server does not support it. /// - /// ISO 14229-1:2020 8.7.3.1 Figure 6, ``UDSSVC_ARCH_0007`` — in that order, `None` + /// ISO 14229-1:2020 8.7.3.1 Figure 6, ``UDSSVC_ARCH_0007``: in that order, `None` /// settles the request `subFunctionNotSupported` (0x12), an active session outside /// [`Access::sessions`] `subFunctionNotSupportedInActiveSession` (0x7E), and none of /// [`Access::levels`] unlocked `securityAccessDenied` (0x33), each without @@ -207,12 +190,11 @@ pub trait EcuReset { /// variants carry the raw byte for a server that defines its own. fn access(&self, kind: ResetType) -> Option; - /// Accept or refuse `kind`. + /// Accept or refuse `kind`, and say what follows the response: [`AfterReset`]. /// /// **Must not perform the reset before returning.** ISO 14229-1:2020 clause 10.3.1 /// strongly recommends that the positive response is sent before the reset is - /// executed, and it is sent only after this returns `Ok`. Return what follows the - /// response: [`AfterReset`]. + /// executed, and it is sent only after this returns `Ok`. /// /// # Arguments /// @@ -251,25 +233,14 @@ pub trait EcuReset { fn confirmed(&mut self, kind: ResetType); } -/// `TesterPresent` (0x3E). -/// -/// Restarting `tS3_Server` is deliberately absent: ISO 14229-2 puts it in the session -/// layer and ``UDSSVC_ARCH_0002`` keeps it there. This exists for servers that act on it. +/// `TesterPresent` (0x3E), for servers that act on it. Restarting `tS3_Server` is the +/// session layer's (ISO 14229-2, ``UDSSVC_ARCH_0002``), not this trait's. /// -/// **No `MAY_RESPOND_PENDING`**, for the reason -/// [`DiagnosticSessionControl`] has none: [`Self::on_tester_present`] is synchronous, so -/// the driver is never awaiting it when a deadline passes and no 0x78 can become due. -/// Which is as well — a server too busy to answer the message whose only purpose is to -/// say it is still there has a larger problem than a response-pending. -/// -/// **No sub-function lookups**, unlike [`DiagnosticSessionControl`]'s -/// [`supports`](DiagnosticSessionControl::supports) and -/// [`supported_from`](DiagnosticSessionControl::supported_from). The service's only -/// sub-function is `zeroSubFunction`, which ISO 14229-1:2020 clause 10.7 makes mandatory, -/// and clause 10.2's Table 23 makes the service available in the default and every -/// non-default session. So Figure 6's "supported ever" and "supported in active session" -/// (``UDSSVC_ARCH_0007`` rows 2 and 4) have nothing deployment-specific to ask: the -/// pipeline answers row 2 from the byte, and row 4 is always yes. +/// No `MAY_RESPOND_PENDING`: [`Self::on_tester_present`] is synchronous, so no 0x78 can +/// come due. No sub-function lookups either: the service's only sub-function is +/// `zeroSubFunction`, which ISO 14229-1:2020 clause 10.7 makes mandatory and Table 23 +/// makes available in every session, so the pipeline answers ``UDSSVC_ARCH_0007`` row 2 +/// from the byte and row 4 is always yes. pub trait TesterPresent { /// Called on each accepted `TesterPresent`. fn on_tester_present(&mut self); @@ -283,14 +254,12 @@ pub trait CommunicationControl { /// Where `control_type` is available and which levels unlock it, or `None` where this /// server does not support it. /// - /// ISO 14229-1:2020 8.7.3.1 Figure 6, ``UDSSVC_ARCH_0007`` — in that order, `None` + /// ISO 14229-1:2020 8.7.3.1 Figure 6, ``UDSSVC_ARCH_0007``: in that order, `None` /// settles the request `subFunctionNotSupported` (0x12), an active session outside /// [`Access::sessions`] `subFunctionNotSupportedInActiveSession` (0x7E), and none of /// [`Access::levels`] unlocked `securityAccessDenied` (0x33), each without - /// [`Self::control`] being asked. - /// - /// The default session never reaches here: Table 23 refuses the service there with - /// 0x7F. + /// [`Self::control`] being asked. The default session never reaches here: Table 23 + /// refuses the service there with 0x7F. /// /// # Arguments /// @@ -302,9 +271,9 @@ pub trait CommunicationControl { /// names. /// /// Clause 10.5.1: the positive response is owed even where the requested state is - /// already in effect. Clause 10.2 Figure 7 resets it on a return to the default - /// session and leaves it on any other transition; that state is the application's to - /// reset, and `DiagnosticSessionControl::on_transition` tells it the session entered. + /// already in effect. Clause 10.2 Figure 7 resets that state on a return to the + /// default session, which the application does when + /// `DiagnosticSessionControl::on_transition` tells it the session entered. /// /// # Arguments /// @@ -337,26 +306,21 @@ pub trait ControlDtcSetting { /// ``UDSSVC_ARCH_0033``. const MAY_RESPOND_PENDING: bool; - /// The longest `DTCSettingControlOptionRecord` this server accepts. - /// - /// Clause 10.8.2.3 leaves the record manufacturer-specific and gives it no fixed - /// width, so the ceiling is the application's to state — and stating it is what puts - /// this service's real contribution into the derived in-flight buffer rather than the - /// catch-all's six bytes. A longer record is `incorrectMessageLengthOrInvalidFormat` - /// (0x13) without [`Self::control_dtc_setting`] being asked. + /// The longest `DTCSettingControlOptionRecord` this server accepts, folded into the + /// in-flight buffer; clause 10.8.2.3 gives the record no fixed width. A longer record + /// is `incorrectMessageLengthOrInvalidFormat` (0x13) without + /// [`Self::control_dtc_setting`] being asked. const MAX_OPTION_RECORD_LEN: usize; /// Where `setting` is available and which levels unlock it, or `None` where this /// server does not support it. /// - /// ISO 14229-1:2020 8.7.3.1 Figure 6, ``UDSSVC_ARCH_0007`` — in that order, `None` + /// ISO 14229-1:2020 8.7.3.1 Figure 6, ``UDSSVC_ARCH_0007``: in that order, `None` /// settles the request `subFunctionNotSupported` (0x12), an active session outside /// [`Access::sessions`] `subFunctionNotSupportedInActiveSession` (0x7E), and none of /// [`Access::levels`] unlocked `securityAccessDenied` (0x33), each without - /// [`Self::control_dtc_setting`] being asked. - /// - /// The default session never reaches here: Table 23 refuses the service there with - /// 0x7F. + /// [`Self::control_dtc_setting`] being asked. The default session never reaches + /// here: Table 23 refuses the service there with 0x7F. /// /// # Arguments /// @@ -368,8 +332,8 @@ pub trait ControlDtcSetting { /// /// Clause 10.8.1: the positive response is owed even where `setting` is already in /// effect, and updating resumes on a transition to a session where this service is - /// not supported. That state is the application's to resume: it is told the session - /// entered by `DiagnosticSessionControl::on_transition`. + /// not supported, which the application does when + /// `DiagnosticSessionControl::on_transition` tells it the session entered. /// /// # Arguments /// @@ -454,10 +418,8 @@ mod tests { fn confirmed(&mut self, _k: ResetType) {} } - /// ``UDSSVC_ARCH_0038`` — the application receives a classified transition, not a - /// sub-function value. Which of Figure 7's four cases occurred determines what - /// configuration state resets, and classifying it here stops every application - /// reimplementing Table 23. + /// ``UDSSVC_ARCH_0038``: the application receives a classified transition, not a + /// sub-function value. #[test] fn the_application_receives_a_classified_transition() { let mut ecu = Ecu; @@ -476,9 +438,8 @@ mod tests { )); } - /// P2 values are a property of the session being entered, so the application states - /// them and this crate composes the response; the session layer reads them in - /// milliseconds, `P2*` being sent in tens of them. + /// The application states a session's P2 pair in Table 29's units, and the session + /// layer reads both in milliseconds. #[test] fn a_session_carries_its_own_timing() { let t = Ecu.timing(DiagnosticSessionType::ProgrammingSession); diff --git a/crates/uds_services/src/services/transfer.rs b/crates/uds_services/src/services/transfer.rs index f57d4463..5afc732f 100644 --- a/crates/uds_services/src/services/transfer.rs +++ b/crates/uds_services/src/services/transfer.rs @@ -1,4 +1,4 @@ -//! Upload and download — ISO 14229-1:2020 clause 15. +//! Upload and download, ISO 14229-1:2020 clause 15. use crate::{Access, ResponseSink, Sessions}; use uds_protocol::{ @@ -7,24 +7,19 @@ use uds_protocol::{ /// What a transfer is being requested for. /// -/// ``UDSSVC_ARCH_0036`` puts direction here rather than in a separate field, because a -/// repeated block is handled differently by direction: a repeated download is answered -/// without calling the handler, and a repeated upload will call it again. A -/// `RequestUpload` variant joins these when that service is served. +/// ``UDSSVC_ARCH_0036`` puts direction here: a repeated download block is answered +/// without calling the handler, where a repeated upload block would call it again. /// -/// Address and size arrive decoded. Their wire widths are declared by the -/// `addressAndLengthFormatIdentifier`, which `uds_protocol` reads to size the fields — -/// so the widths are a decoding concern that is already settled by the time a handler is -/// called, and the identifier itself never reaches one. Each is up to eight bytes wide, -/// so each is a [`u64`]; a request declaring a wider one is answered `requestOutOfRange` -/// (0x31) before any handler is asked. +/// Address and size arrive decoded, each up to eight bytes wide and so a [`u64`]; the +/// `addressAndLengthFormatIdentifier` that declared their widths is `uds_protocol`'s to +/// read and never reaches a handler. A request declaring a wider one is answered +/// `requestOutOfRange` (0x31) before any handler is asked. /// -/// Not [`Hash`]: [`RequestFileTransferRequest`] is not, and a transfer request is a thing -/// to match on rather than a key to look one up by. +/// Not [`Hash`]: [`RequestFileTransferRequest`] is not. #[derive(Debug, Clone, Copy, PartialEq, Eq)] #[non_exhaustive] pub enum TransferRequest<'a> { - /// `RequestDownload` (0x34) — the client sends data to the server. + /// `RequestDownload` (0x34): the client sends data to the server. Download { /// `dataFormatIdentifier`; see [`DataFormatIdentifier`], whose /// [`NONE`](DataFormatIdentifier::NONE) is the uncompressed, unencrypted case. @@ -39,25 +34,20 @@ pub enum TransferRequest<'a> { File(RequestFileTransferRequest<'a>), } -/// The download services (0x34, 0x36, 0x37). `RequestUpload` (0x35) and -/// `RequestFileTransfer` (0x38) are not served: both are `serviceNotSupported` (0x11). +/// The download services (0x34, 0x36, 0x37), one trait because they are one state +/// machine. `RequestUpload` (0x35) and `RequestFileTransfer` (0x38) are not served: both +/// are `serviceNotSupported` (0x11). /// -/// ``UDSSVC_ARCH_0036`` — the transfer lifecycle is a state machine this crate owns: -/// which requests are admissible in which order, the block sequence counter and its -/// `wrongBlockSequenceCounter` (0x73), the `0xFF → 0x00` roll, and rejecting a -/// `TransferData` with no transfer in progress. One trait rather than four, because the -/// services are one state machine and implementing three of them would be meaningless. -/// -/// A response lost after it settled is not undone: the client's repeat is answered from -/// the state the lost response left. ``UDSSVC_ARCH_0036`` draws the transfer's states and -/// tabulates each lost response's repeat. -/// -/// Unless another listed service needs more, the response buffer is 6 bytes, so the -/// `transferResponseParameterRecord` [`Self::block`] writes after `76 ` is at -/// most 4 bytes and the one [`Self::exit`] writes after `77` at most 5; a longer one is -/// `responseTooLong` (0x14). +/// ``UDSSVC_ARCH_0036``: the transfer lifecycle is this crate's, which requests are +/// admissible in which order, the block sequence counter with its 0x73 and its +/// `0xFF -> 0x00` roll, and a `TransferData` with no transfer in progress. A response +/// lost after it settled is not undone: the client's repeat is answered from the state +/// the lost response left. Unless another listed service needs more, the response +/// buffer is 6 bytes, so the `transferResponseParameterRecord` [`Self::block`] writes +/// after `76 ` is at most 4 bytes and the one [`Self::exit`] writes after `77` +/// at most 5; a longer one is `responseTooLong` (0x14). pub trait DataTransfer { - /// ``UDSSVC_ARCH_0033`` — erasing flash before a download usually needs one. + /// ``UDSSVC_ARCH_0033``. const MAY_RESPOND_PENDING: bool; /// The most data bytes one `TransferData` block carries, excluding the service @@ -98,8 +88,7 @@ pub trait DataTransfer { /// ISO 14229-1:2020 Figure 31, in its order: `None` is `requestOutOfRange` (0x31), an /// active session outside [`Access::sessions`] is 0x31 too, as for an identifier /// (Figures 26 and 30), and none of [`Access::levels`] unlocked is - /// `securityAccessDenied` (0x33). - /// [`Self::begin`] is asked only after all three pass. + /// `securityAccessDenied` (0x33). [`Self::begin`] is asked only after all three pass. /// /// # Arguments /// @@ -113,7 +102,7 @@ pub trait DataTransfer { /// /// # Errors /// - /// The [`NegativeResponseCode`] — `requestOutOfRange` (0x31) for an unwritable + /// The [`NegativeResponseCode`]: `requestOutOfRange` (0x31) for an unwritable /// region, `uploadDownloadNotAccepted` (0x70) where conditions forbid it. fn begin( &mut self, @@ -121,15 +110,12 @@ pub trait DataTransfer { ) -> impl core::future::Future>; /// Accept one block of a download, writing any `transferResponseParameterRecord` into - /// `out`. - /// - /// The block sequence counter is **not** a parameter: validating it and answering - /// `wrongBlockSequenceCounter` (0x73) is this crate's, and a handler that received - /// the counter could disagree with the state machine maintaining it. + /// `out`. The block sequence counter is not a parameter: validating it and answering + /// `wrongBlockSequenceCounter` (0x73) is this crate's. /// /// # Errors /// - /// The [`NegativeResponseCode`] — `generalProgrammingFailure` (0x72) where the write + /// The [`NegativeResponseCode`]: `generalProgrammingFailure` (0x72) where the write /// failed, `transferDataSuspended` (0x71) where it cannot continue, which ends the /// transfer. /// @@ -146,8 +132,8 @@ pub trait DataTransfer { /// /// # Errors /// - /// The [`NegativeResponseCode`] where the transfer cannot complete — a checksum - /// failure is the usual case. + /// The [`NegativeResponseCode`] where the transfer cannot complete, such as a + /// checksum failure. /// /// A refused write to `out` needs no handling: the sink records the refusal and the /// pipeline answers `responseTooLong` (0x14) in place of the response, so the write's @@ -204,9 +190,8 @@ mod tests { } } - /// Decision 4 of the design: the advertised `maxNumberOfBlockLength` is - /// `MAX_BLOCK_LEN + 2` (15.2.3.2), the length of the in-flight buffer that must hold - /// the block. + /// The advertised `maxNumberOfBlockLength` is `MAX_BLOCK_LEN + 2` (15.2.3.2), the + /// length of the in-flight buffer that holds the block. #[test] fn the_block_length_gives_both_the_buffer_and_the_advertisement() { const BLOCK: usize = ::MAX_BLOCK_LEN; diff --git a/crates/uds_services/src/sink.rs b/crates/uds_services/src/sink.rs index bb987605..8322d9d2 100644 --- a/crates/uds_services/src/sink.rs +++ b/crates/uds_services/src/sink.rs @@ -1,30 +1,19 @@ //! The one sink a handler writes into. //! //! ``UDSSVC_ARCH_0017``. Neither `dyn` nor generic: [`crate::storage`] owns the response -//! buffer, so the sink is never the caller's and there is nothing for a type parameter to -//! vary. Dispatch stays fully monomorphised, with no vtable. +//! buffer, so there is nothing for a type parameter to vary. use automotive_wire_codec::{InsufficientBuffer, Sink, WriteError}; /// A bounded writer over this crate's response buffer. /// -/// The bound is the peer's `outbound_max`, raised to at least [`Self::MIN_BOUND`] and -/// capped at the buffer's length. Both terms are this crate's, so nothing is fabricated -/// and nothing is asked of a layer that does not know it — which is what -/// ``UDSSVC_ARCH_0017`` requires after ISO 13400-2:2019 Table 11 made *Max. data size* -/// optional. -/// -/// The floor is there because a negative response must always fit; [`crate::uds_server`] -/// rejects at compile time an assembly whose response buffer is shorter than it. A write -/// that would cross the bound is refused whole, and the sink records the refusal before -/// returning the error, so it is not lost if a handler ignores the result. Such a refusal -/// means the response was too long, not that nothing was written; the pipeline reads it -/// after the handler returns and answers `responseTooLong` (0x14) in place of the partial -/// response. -/// -/// The bound is not a field. [`Self::new`] truncates the buffer to it, so "written never -/// exceeds the limit, which never exceeds the buffer" is one slice length rather than an -/// ordering between two numbers that each method had to be trusted to maintain. +/// The bound is the peer's `outbound_max` (``UDSSVC_ARCH_0017``), raised to at least +/// [`Self::MIN_BOUND`] so a negative response always fits, and capped at the buffer's +/// length; [`crate::uds_server`] rejects at compile time an assembly whose response buffer +/// is shorter than the floor. A write that would cross the bound is refused whole, and +/// the sink records the refusal, so a handler that ignores the error loses nothing: the +/// pipeline reads it after the handler returns and answers `responseTooLong` (0x14) in +/// place of the partial response. /// /// # Examples /// @@ -77,9 +66,8 @@ impl<'a> ResponseSink<'a> { /// Whether any write was refused since creation or the last rewind. /// - /// ``UDSSVC_ARCH_0017`` — the pipeline reads this after the handler returns and - /// settles `responseTooLong` (0x14). A handler ignores `write_all`'s result; the - /// refusal is recorded here and decided there. + /// ``UDSSVC_ARCH_0017``: the pipeline reads this after the handler returns and + /// settles `responseTooLong` (0x14). #[must_use] pub(crate) const fn refused(&self) -> bool { self.refused @@ -99,10 +87,7 @@ impl<'a> ResponseSink<'a> { self.written } - /// What the handler wrote. - /// - /// The driver needs this because [`Self::new`] moves the slice in, leaving no - /// other route back to the bytes it must transmit. + /// What the handler wrote: the bytes the driver transmits. #[must_use] pub fn written_bytes(&self) -> &[u8] { self.buffer.get(..self.written).unwrap_or(&[]) @@ -145,9 +130,7 @@ mod tests { use uds_protocol::{DtcRecord, DtcStatusMask, Encode}; /// `uds_protocol`'s types encode into an `automotive_wire_codec::Sink`, and this is - /// one — so a handler writes a value and its length cannot disagree with its bytes. - /// Both traits are re-exported from the crate root so reaching this needs no third - /// dependency. + /// one. #[test] fn a_protocol_value_encodes_straight_into_the_sink() { let mut buf = [0_u8; 8]; @@ -162,8 +145,8 @@ mod tests { assert_eq!(sink.written_bytes(), &[0xC0, 0x01, 0x23, 0x01]); } - /// ``UDSSVC_ARCH_0017`` — with no peer advertisement the buffer alone bounds the - /// response, which is a correct outcome rather than a degraded one. + /// ``UDSSVC_ARCH_0017``: with no peer advertisement the buffer alone bounds the + /// response. #[test] fn an_unadvertised_maximum_leaves_the_buffer_as_the_bound() { let mut buf = [0_u8; 8]; @@ -171,7 +154,7 @@ mod tests { } /// A peer that advertised less lowers the bound; one that advertised more does - /// not raise it. The buffer is physical and the advertisement is a claim. + /// not raise it. #[test] fn the_smaller_of_the_two_bounds_wins() { let mut buf = [0_u8; 8]; @@ -180,8 +163,7 @@ mod tests { assert_eq!(ResponseSink::new(&mut buf, Some(64)).remaining(), 8); } - /// `written_bytes` is how the driver reaches what it must transmit: `new` moves - /// the slice in, so there is no other route back to it. + /// `written_bytes` is what the driver transmits. #[test] #[allow(clippy::unwrap_used, reason = "testing success path")] fn written_bytes_is_what_the_driver_transmits() { @@ -193,9 +175,8 @@ mod tests { assert_eq!(sink.remaining(), 5); } - /// Over-running is the failure the pipeline turns into responseTooLong (0x14). - /// `needed_at_least` is a lower bound by awc's own definition, and 0x14 carries - /// no length, so a lower bound is sufficient. + /// Over-running reports `needed_at_least` and `available`; 0x14 carries no length, + /// so a lower bound is sufficient. #[test] #[allow(clippy::unwrap_used, reason = "testing success path of first write")] #[allow(clippy::panic, reason = "testing failure path")] @@ -218,8 +199,8 @@ mod tests { assert_eq!(sink.written(), 0); } - /// ``UDSSVC_ARCH_0017`` — ISO 14229-1 fixes a negative response at three bytes, so a - /// bound below three is not one the protocol admits: it is raised to three. + /// ``UDSSVC_ARCH_0017``: ISO 14229-1 fixes a negative response at three bytes, so a + /// bound below three is raised to three. #[test] fn the_bound_is_never_below_three_bytes() { let mut buffer = [0_u8; 8]; @@ -230,8 +211,8 @@ mod tests { assert_eq!(sink.remaining(), 2); // the buffer itself is the hard limit } - /// ``UDSSVC_ARCH_0017`` — a refused write is recorded for the pipeline to read; the - /// handler need not report it. Rewinding discards everything written. + /// ``UDSSVC_ARCH_0017``: a refused write is recorded for the pipeline to read, and + /// rewinding discards everything written. #[test] fn a_refused_write_is_recorded_and_rewind_discards() { let mut buffer = [0_u8; 4]; diff --git a/crates/uds_services/src/state.rs b/crates/uds_services/src/state.rs index f9665e9d..4c2f085a 100644 --- a/crates/uds_services/src/state.rs +++ b/crates/uds_services/src/state.rs @@ -1,24 +1,17 @@ //! Protocol state this crate keeps and the application cannot reach. //! -//! ``UDSSVC_ARCH_0035`` — bookkeeping ISO 14229-1 needs across requests belongs here, out -//! of the application's reach: `State`'s fields are private to this crate and the -//! [`crate::Server`] that holds one keeps it in a private field. The assembly names the -//! type; the macro expands in the application's crate and could not keep a field private -//! there, which is why the type is declared here and only *named* by the macro. +//! ``UDSSVC_ARCH_0035``: the bookkeeping ISO 14229-1 needs across requests is this +//! crate's. `State`'s fields are private to it, the [`crate::Server`] that holds one keeps +//! it in a private field, and the macro, which expands in the application's crate, only +//! names the type. use crate::{SecurityLevel, Sessions}; use uds_protocol::DiagnosticSessionType; /// The state an assembled server keeps between requests. ``UDSSVC_ARCH_0035``. /// -/// Sealed, so the only implementor is [`State`]; `INITIAL` rather than `EMPTY` because -/// the initial state is not zero — it is `defaultSession`. -/// -/// The seal is this module's own, not [`crate::sealed`]'s. That one has to be nameable -/// from the application's crate, because `uds_server!` implements [`crate::ServiceSet`] -/// there; nothing outside this crate implements `ProtocolState` — the macro only names -/// [`State`] — so this seal can be one no other crate can reach, and a hand-written -/// state cannot slip past ``UDSSVC_ARCH_0035``. +/// Sealed with a seal no other crate can name, so [`State`] is the only implementor. +/// `INITIAL` rather than `EMPTY`: the initial state is `defaultSession`, not zero. /// /// Implementing it from outside this crate does not compile, even through the crate-wide /// seal the macro uses: diff --git a/crates/uds_services/src/storage.rs b/crates/uds_services/src/storage.rs index 75120241..8d2ad510 100644 --- a/crates/uds_services/src/storage.rs +++ b/crates/uds_services/src/storage.rs @@ -1,31 +1,21 @@ //! Where the bytes live. //! -//! ``UDSSVC_ARCH_0017`` makes this crate the owner of the buffers. The sizes are not -//! chosen here and not chosen by the application either: [`crate::uds_server`] folds -//! each service's declared worst case into the array lengths below, at a site where -//! the types are concrete and const evaluation is ordinary stable Rust. +//! ``UDSSVC_ARCH_0017`` makes this crate the owner of the buffers, and +//! [`crate::uds_server`] folds each service's declared worst case into their lengths. //! -//! **Three buffers, and the reason is the response-pending window.** A handler holds a -//! decoded request borrowing the in-flight buffer, so that buffer cannot be handed back -//! to [`crate::UdsTransport::next_event`] while the handler runs. `next_event` is the -//! only way this crate can wait (``UDSSVC_ARCH_0041``), so without a second buffer the -//! driver cannot observe the `tP2_Server` deadline and ``UDSSVC_ARCH_0031``'s 0x78 never -//! happens. Clause 8.7.6's two exceptions -//! are the second reason, not the first. -//! -//! The in-flight buffer bounds the request a handler decodes, not the request the server -//! receives: a longer one arrives truncated, as -//! [`crate::TransportEvent::DataTooLong`], and is answered by ISO 14229-1's rules, an -//! unsupported service `0x11` before a wrong length `0x13`. What a transport can hold, -//! a `DoIP` entity's ISO 13400-2:2019 Table 11 *Max. data size* among them, is the -//! transport's. +//! A server has three: the handler holds a decoded request borrowing the in-flight +//! buffer, so the driver waits on [`crate::UdsTransport::next_event`] with the concurrent +//! buffer meanwhile (``UDSSVC_ARCH_0041``), which is how it observes the `tP2_Server` +//! deadline and clause 8.7.6's two exceptions. The in-flight buffer bounds the request a +//! handler decodes, not the request the server receives: a longer one arrives truncated, +//! as [`crate::TransportEvent::DataTooLong`], and is answered by ISO 14229-1's rules. /// The three buffers, together. #[derive(Debug)] pub struct Buffers<'a> { - /// The request being served. Bounds MDS. + /// The request being served. pub in_flight: &'a mut [u8], - /// What may arrive while a service is in progress — clause 8.7.6's two exceptions + /// What may arrive while a service is in progress: clause 8.7.6's two exceptions /// only, so a handful of bytes. pub concurrent: &'a mut [u8], /// Where a handler writes. @@ -34,20 +24,13 @@ pub struct Buffers<'a> { /// The storage one assembled application needs. /// -/// ``UDSSVC_ARCH_0013`` — implemented by the type [`crate::uds_server`] generates, -/// never by hand. Sealed, so "never" is a compile error rather than a request: [`Store`] -/// is the only implementor, and a hand-written one could pick lengths that disagree with -/// the maxima the assembly folded. +/// ``UDSSVC_ARCH_0013``: implemented by [`Store`], which [`crate::uds_server`] names with +/// the lengths it folded, and sealed so that nothing else can. pub trait Storage: crate::sealed::Sealed { /// Zeroed storage. /// - /// An associated const rather than a `const fn`, because trait methods cannot be - /// `const` on stable and [`crate::Server::new`] must work in a `static` initialiser: - /// a multi-kilobyte server built at runtime is a stack temporary before the move, - /// which is fatal on a small-stack target, and building it in place needs `unsafe`. - /// - /// This is `uds_session`'s own convention — `Association::EMPTY`, - /// `PhysicalSlot::EMPTY`, `FunctionalSlot::EMPTY`. + /// An associated const rather than a `const fn`, so that [`crate::Server::new`] can + /// be `const` and build a server in a `static` initializer without a stack temporary. const EMPTY: Self; /// All three buffers at once, because all three are live at once. @@ -98,27 +81,19 @@ pub struct ClientBuffers<'a> { /// The storage one assembled client needs. /// -/// ``UDSSVC_ARCH_0017`` for the client half: the buffers are this crate's, and -/// [`crate::uds_client`] folds their lengths from the identifier enumeration's declared -/// maximum record and the most identifiers one request may name. Sealed for the reason -/// [`Storage`] is — a hand-written implementor could pick lengths that disagree with the -/// maxima the assembly folded. -/// -/// [`Debug`] is a supertrait because [`crate::Responses`] holds a `&mut Client` and so -/// needs the store's `Debug` one projection deeper than a derive can infer, and this -/// crate denies `missing_debug_implementations`. +/// ``UDSSVC_ARCH_0017`` for the client half: implemented by [`ClientStore`], which +/// [`crate::uds_client`] names with the lengths it folded from the vocabulary and its +/// [`crate::ClientLimits`], and sealed so that nothing else can. [`Debug`] is a supertrait +/// because [`crate::Responses`] holds a `&mut Client` and derives `Debug`. /// -/// **Two buffers rather than one.** A single buffer would be the larger of the two and -/// therefore smaller than this pair, and the request would not survive the response -/// landing on top of it. Keeping them apart is what lets a retransmission after a -/// `tP_Client` expiry re-send the bytes it already encoded, rather than encoding them -/// again into storage the caller may be holding as [`crate::Records`]. +/// The request keeps its own buffer so that a repeat after a `tP_Client` expiry re-sends +/// the bytes already encoded while the caller may still hold the response as +/// [`crate::Records`]. pub trait ClientStorage: crate::sealed::Sealed + core::fmt::Debug { /// Zeroed storage. /// - /// An associated const for the reason [`Storage::EMPTY`] is one: a client built at - /// runtime is a stack temporary before the move, and - /// [`crate::Client::new`] must work in a `static` initialiser. + /// An associated const for the reason [`Storage::EMPTY`] is one: [`crate::Client::new`] + /// is `const` and builds a client in a `static` initializer. const EMPTY: Self; /// Both buffers at once. @@ -167,15 +142,14 @@ mod tests { assert_eq!((b.request[0], b.response[0]), (0x22, 0x62)); } - /// A client constructs in a `static` for the same reason a server does. + /// `EMPTY` initializes a `static`, as a server's does. #[test] fn client_storage_is_const_constructible() { static STORE: ClientStore<9, 77> = as ClientStorage>::EMPTY; assert_eq!(core::mem::size_of_val(&STORE), 86); } - /// The three arrays are exactly the declared sizes. The whole derivation scheme - /// rests on this, so it is asserted rather than assumed. + /// The three arrays are exactly the declared sizes. #[test] fn the_buffers_are_the_declared_lengths() { let mut s = as Storage>::EMPTY; @@ -185,10 +159,8 @@ mod tests { assert_eq!(b.response.len(), 11); } - /// All three come back from one call because all three are live at once: the - /// request is read while the response is written and the concurrent slot is - /// receiving. Three accessors would not borrow, and this test is what fails if - /// someone later "simplifies" `split` into separate methods. + /// All three come back from one call and are usable at once: the request is read + /// while the response is written and the concurrent slot is receiving. #[test] fn all_three_are_usable_at_once() { let mut s = as Storage>::EMPTY; @@ -199,9 +171,7 @@ mod tests { assert_eq!((b.response[0], b.concurrent[0]), (0x22, 0x3E)); } - /// `EMPTY` is an associated const, not a `const fn`, so `Server::new` can be a - /// `const fn` in a generic context — trait methods cannot be `const` on stable. - /// A `static` initialiser is the thing that must work. + /// `EMPTY` initializes a `static`. #[test] fn storage_is_const_constructible() { static STORE: Store<8, 2, 8> = as Storage>::EMPTY; diff --git a/crates/uds_services/src/transport.rs b/crates/uds_services/src/transport.rs index 3f3122df..8d9f4367 100644 --- a/crates/uds_services/src/transport.rs +++ b/crates/uds_services/src/transport.rs @@ -1,56 +1,52 @@ //! The seam below this crate. //! -//! ``UDSSVC_ARCH_0029`` — this crate calls out to a transport, so this crate declares -//! what it calls. The trait is a transport's whole obligation to the stack: carry bytes -//! both ways, say what the peer will accept, say what timing it dictates, and tell the -//! time. It carries no notion of a service, a data identifier or a negative response -//! code, and nothing in it is `DoIP`-shaped, which is the test of whether it is the -//! right seam. It serves both roles. +//! ``UDSSVC_ARCH_0029``: this crate calls out to a transport, so this crate declares +//! what it calls. The trait is a transport's whole obligation to the stack, for both +//! roles: carry bytes both ways, say what the peer will accept, say what timing it +//! dictates, and tell the time. It carries no notion of a service, a data identifier or +//! a negative response code, and nothing in it is `DoIP`-shaped. pub use uds_session::{Address, Ai, Mtype, Reloads, SResult, TaType, Timestamp}; /// What the transport reports, or that the driver's deadline passed first. /// -/// **The lifetime is the driver's buffer, never the transport.** A message arrives as the -/// subslice of the supplied buffer that it occupies, so the borrow ends when the buffer's -/// does — which is what lets the driver call [`UdsTransport::t_data_req`] while the -/// request bytes that provoked the response are still live. Reporting a length instead -/// would put that tie in prose: the driver would have to re-slice its own buffer and -/// defend against a length larger than what it lent. +/// A message arrives as the subslice of the supplied buffer that it occupies, so the +/// borrow is the buffer's, not the transport's, and the driver calls +/// [`UdsTransport::t_data_req`] while the request bytes that provoked the response are +/// still live. #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] #[non_exhaustive] pub enum TransportEvent<'b> { /// A complete inbound message. DataInd { - /// Addressing, with the peer's `S_AI[SA]` — the only way to tell functional - /// responses apart. + /// Addressing, with the peer's `S_AI[SA]`, which tells functional responses + /// apart. ai: Ai, /// The message, in the buffer the driver supplied. data: &'b [u8], }, /// A message longer than the supplied buffer, truncated to what fit. /// - /// A variant rather than a flag on [`Self::DataInd`], because destructuring - /// `DataInd { ai, data, .. }` is idiomatic and would silently discard a flag, - /// leaving a fragment decoded as a message. Reached when the driver is serving a - /// request and offers only the small concurrent buffer, where by clause 8.7.6 the - /// server is occupied and owes `busyRepeatRequest` (0x21) — a decision needing only - /// the service identifier and the addressing. + /// A variant rather than a flag on [`Self::DataInd`], so that destructuring + /// `DataInd { ai, data, .. }` cannot decode a fragment as a message. The normal + /// outcome while the driver is serving a request and offers only the small + /// concurrent buffer: by clause 8.7.6 the server is occupied and owes + /// `busyRepeatRequest` (0x21), decided from the service identifier and the + /// addressing alone. DataTooLong { /// Addressing. ai: Ai, /// What fit. data: &'b [u8], - /// How long the message actually was, where the transport knows. `DoIP` reads - /// it from the generic header before the payload, so it is free there; a - /// transport that cannot know without reading the whole message reports `None` - /// rather than inventing one. + /// How long the message was, where the transport knows (`DoIP` reads it from the + /// generic header); `None` where it cannot know without reading the whole + /// message. declared: Option, }, /// The outcome of a requested transmission. /// - /// Raised from the acknowledgement rather than from the socket write returning, - /// because the acknowledgement is what starts `tP_Client` + /// Raised from the acknowledgment rather than from the socket write returning, + /// because the acknowledgment is what starts `tP_Client` /// (ISO 14229-2:2021 REQ 5.9). DataConf { /// The addressing of the transmission being confirmed. @@ -58,17 +54,13 @@ pub enum TransportEvent<'b> { /// The outcome. result: SResult, }, - /// A periodic response — ISO 14229-5:2022 REQ 7.16's `0x8004` payload type. + /// A periodic response, ISO 14229-5:2022 REQ 7.16's `0x8004` payload type. /// - /// **Cannot be a [`Self::DataInd`].** ISO 14229-5:2022 REQ 7.20 requires a periodic - /// response not to reset `tS3_Server`, and `DataInd` is exactly what feeds the session - /// layer and resets it — so delivering one as a `DataInd` is a conformance failure, not - /// a shortcut. `DoIP`-specific in its *payload type* but not in its shape: a CAN - /// binding implementing periodic responses reports the same three things. - /// - /// The driver carries this variant and does nothing with it yet: - /// `ReadDataByPeriodicIdentifier` (0x2A) has no `uds_protocol` message type, so no - /// server can implement it and no client can decode the result. + /// Never a [`Self::DataInd`]: ISO 14229-5:2022 REQ 7.20 requires a periodic response + /// not to reset `tS3_Server`, and `DataInd` is what feeds the session layer and + /// resets it. The driver does nothing with it: `ReadDataByPeriodicIdentifier` (0x2A) + /// has no `uds_protocol` message type, so no server can implement it and no client + /// can decode the result. Periodic { /// Addressing, with the responding server's `S_AI[SA]`. ai: Ai, @@ -79,27 +71,22 @@ pub enum TransportEvent<'b> { }, /// The connection to `peer` went away. /// - /// ISO 14229-5:2022 REQ 7.9 and REQ 7.11 make a close **part of the normal - /// `DiagnosticSessionControl` and `ECUReset` flows**: the server initiates one after - /// sending every positive `ECUReset` response, and after the positive - /// `DiagnosticSessionControl` response sent with [`AfterSend::ServerLeaves`], before - /// executing the service. So a close is not a failure and must not arrive as `Err`. - /// A transport with no connections never emits this, exactly as one that never - /// truncates never emits [`Self::DataTooLong`]. + /// Not a failure, so never an `Err`: ISO 14229-5:2022 REQ 7.9 and REQ 7.11 make a + /// close part of the normal `DiagnosticSessionControl` and `ECUReset` flows, the + /// server closing after every positive `ECUReset` response and after the positive + /// `DiagnosticSessionControl` response sent with [`AfterSend::ServerLeaves`]. A + /// transport with no connections never emits this. /// - /// **Reported once per close.** It is an edge, not a state: after it, the next - /// [`UdsTransport::next_event`] waits for an event or the deadline as usual, and a - /// transport whose connection layer keeps reporting a closed connection until it - /// reconnects reports it here once. A request handed to the transport meanwhile is - /// still confirmed, failed, as the trait requires. + /// Reported once per close, as an edge: the next [`UdsTransport::next_event`] waits + /// for an event or the deadline as usual, and a request handed to the transport + /// meanwhile is still confirmed, failed, as the trait requires. Closed { /// The peer whose connection closed: for a server, the client whose requests /// it carried; for a client, the server it sent them to. Only an exchange with /// this peer ends, a client's functional request counting as one with every - /// peer, since any of them may be answering it. A transport carrying several - /// peers' connections reports each close as its own peer's, and one that cannot - /// name the peer of a connection reports no close for it, because nothing is - /// waiting on a connection whose peer never spoke. + /// peer. A transport carrying several peers' connections reports each close as + /// its own peer's, and one that cannot name the peer of a connection reports no + /// close for it. peer: Address, /// Whether the close was one the standard prescribes, which ends the server's /// session: for a server, one it owed its client; for a client, one following the @@ -107,37 +94,29 @@ pub enum TransportEvent<'b> { /// (ISO 14229-5:2022 REQ 7.9, REQ 7.11). Any other close, a failure or a connection /// given up, ends a connection and not a session. /// - /// **Informational: re-establishing the connection is never this crate's.** - /// ISO 14229-5:2022 REQ 7.8 and REQ 7.10 have the client open a new connection - /// and activate routing again, so a server is reconnected *to* and never - /// reconnects, and a client's own reconnection is its transport's business - /// below this seam (``UDSSVC_ARCH_0002``). Nothing here asks a transport to - /// reconnect, and no method to do so exists. + /// Informational: ISO 14229-5:2022 REQ 7.8 and REQ 7.10 have the client open a + /// new connection, so a server never reconnects, and a client's reconnection is + /// its transport's, below this seam (``UDSSVC_ARCH_0002``). expected: bool, }, /// The deadline the driver supplied passed before anything arrived. /// - /// **Not itself an overrun.** The driver ticks the session layer and acts on what - /// that reports; `uds_session::ServerOutput::ResponseOverrun` is the output that - /// says a response-pending is due. + /// Not itself an overrun: the driver ticks the session layer, and + /// `uds_session::ServerOutput::ResponseOverrun` is the output that says a + /// response-pending is due. Deadline, } /// What follows a message handed to [`UdsTransport::t_data_req`]. /// /// The server says it; a transport acts on it. Whether a session change leaves the -/// software the server runs is a property of the server implementation — an application -/// jumping to its bootloader for `10 02`, a bootloader restarting the application for -/// `10 01` (ISO 14229-1:2020 10.2.2.2 Table 25) — and not of the response's octets, so a -/// transport cannot derive it. ISO 14229-5:2022 REQ 7.9 has a `DoIP` server close the TCP -/// connection after the `DiagnosticSessionControl` positive response "if the TCP connection -/// is disconnected due to a session change", so a `DoIP` transport closes on -/// [`Self::ServerLeaves`] and not otherwise. -/// -/// **`ECUReset` is never flagged.** ISO 14229-5:2022 REQ 7.11 closes the connection after -/// *every* `ECUReset` positive response, with no condition — including the resets that -/// restart nothing — so a `DoIP` transport keys that close on the `0x51` itself. Flagging -/// resets here would make an unconditional close depend on a fact it does not depend on. +/// software the server runs (ISO 14229-1:2020 10.2.2.2 Table 25) is the server's to know, +/// not derivable from the response's octets. ISO 14229-5:2022 REQ 7.9 has a `DoIP` server +/// close the TCP connection after a `DiagnosticSessionControl` positive response "if the +/// TCP connection is disconnected due to a session change", so a `DoIP` transport closes +/// on [`Self::ServerLeaves`] and not otherwise. `ECUReset` is never flagged: REQ 7.11 +/// closes after *every* `ECUReset` positive response, so a `DoIP` transport keys that +/// close on the `0x51` itself. #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] #[non_exhaustive] pub enum AfterSend { @@ -151,10 +130,9 @@ pub enum AfterSend { /// A transport's obligation to the stack. /// -/// ``UDSSVC_ARCH_0030`` for the asynchrony — the trait is `async` and the runtime is -/// the caller's. ``UDSSVC_ARCH_0041`` for the time: a transport that can report an -/// inbound event *or* a timer expiry already measures time, so asking it for the clock -/// adds no capability and avoids two implementors holding two timebases. +/// The trait is `async` and the runtime is the caller's (``UDSSVC_ARCH_0030``). The +/// transport tells the time (``UDSSVC_ARCH_0041``): one that can report an inbound event +/// *or* a timer expiry already measures it. /// /// Four obligations the signatures cannot state, which the drivers rely on: /// @@ -163,42 +141,34 @@ pub enum AfterSend { /// is dropped must not be lost: it is delivered by a later call. /// - **Every accepted [`Self::t_data_req`] is followed by exactly one /// [`TransportEvent::DataConf`] for the same [`Ai`]**, including a failed one when the -/// connection closes before the transmission completed. That a transport reports a -/// `T_Data.conf` for every `T_Data.req` is an assumption of use: an association whose +/// connection closes before the transmission completed. An association whose /// confirmation never arrives has no server exit (``UDSS_LLR_0060``), and the driver /// waits on it. /// - **A [`Self::t_data_req`] future dropped before it completes has accepted its /// request or done nothing.** An accepted request is owed its -/// [`TransportEvent::DataConf`] as any other is; one not accepted is never sent. A -/// transmission begun and then abandoned without either would leave a confirmation -/// nothing can supply. +/// [`TransportEvent::DataConf`] as any other is; one not accepted is never sent. /// - **That [`TransportEvent::DataConf`] comes before any message answering the /// request.** The session layer opens a request's response window at its confirmation, -/// so a client takes nothing earlier as its answer. A transmission is complete before -/// its server can answer it, on `DoIP` as on CAN. +/// so a client takes nothing earlier as its answer. pub trait UdsTransport { /// What this transport's failures are. Never interpreted by this crate. /// - /// `Debug` is the one bound, because it surfaces from [`crate::Server::step`] and - /// every client method: a caller that receives one and cannot render it has an error - /// it can only discard. Not interpreting a transport's failures and not being able to - /// print them are different commitments, and this crate makes only the first. On - /// `no_std` there is no wider trait to ask for. + /// `Debug` is the one bound: the error surfaces from [`crate::Server::step`] and + /// every client method, and a caller has to be able to render it. type Error: core::fmt::Debug; /// The largest `A_PDU` this transport can carry, where its protocol caps it. /// - /// Participates in the const fold at [`crate::uds_server`]'s expansion site. The - /// default, for a transport with no cap, is then ignored; a `DoIP` transport takes - /// its entity's message buffer. + /// Joins the const fold at [`crate::uds_server`]'s expansion site. The default, for + /// a transport with no cap, is then ignored; a `DoIP` transport takes its entity's + /// message buffer. const MAX_PDU: usize = usize::MAX; - /// `T_Data.req` — hand a `T_PDU` to the transport. + /// `T_Data.req`: hand a `T_PDU` to the transport. /// /// An `Ok` is an acceptance, and obliges exactly one [`TransportEvent::DataConf`] - /// for `ai` later — failed, where the connection closed first. Dropped before it - /// completes, the future has accepted the request or done nothing. See the trait's - /// doc. + /// for `ai` later, failed where the connection closed first. Dropped before it + /// completes, the future has accepted the request or done nothing. /// /// # Arguments /// @@ -220,27 +190,21 @@ pub trait UdsTransport { /// Fill `buffer` with the next inbound message, or return /// [`TransportEvent::Deadline`] when `deadline` passes first. /// - /// The event borrows `buffer`, not the transport, so `&mut self` is released when the - /// returned future completes and the driver may transmit while the request is live. - /// A message is reported as the subslice it occupies; there is no length to overstate. + /// The event borrows `buffer`, not the transport, so the driver may transmit while + /// the request is live. A message is reported as the subslice it occupies. /// - /// `deadline` is `uds_session::Server::next_deadline`'s value passed through - /// untouched, and the transport is the side that compares it with the clock. The - /// clock wraps (``UDSS_LLR_0019``), so the comparison is - /// [`now.has_reached(deadline)`](Timestamp::has_reached) and the time left to wait - /// is [`now.until(deadline)`](Timestamp::until), never a comparison of the raw counts, - /// which is wrong either side of the wrap and either spins or misses the deadline by - /// 49.7 days. The deadline may already have been reached when this is - /// called, and `until` is then zero. `None` means no timer is running: wait for an - /// event alone. + /// The clock wraps (``UDSS_LLR_0019``), so the transport compares `deadline` with it + /// through [`now.has_reached(deadline)`](Timestamp::has_reached) and waits + /// [`now.until(deadline)`](Timestamp::until), never by comparing the raw counts. The + /// deadline may already have been reached when this is called, and `until` is then + /// zero. `None` means no timer is running: wait for an event alone. /// /// A driver serving a request offers only its small concurrent buffer, so /// [`TransportEvent::DataTooLong`] is the normal outcome there rather than a fault. /// - /// **Cancel-safe.** The driver drops this future, often unpolled, whenever the - /// handler it races wins. Dropping it must lose nothing: a message partly read is - /// kept and delivered by a later call, and a future that consumed input on creation - /// rather than on its first poll would lose that input. + /// **Must be cancel-safe.** The driver drops this future, often unpolled, whenever + /// the handler it races wins: a message partly read is kept and delivered by a later + /// call, and the future consumes no input before its first poll. /// /// # Errors /// @@ -257,20 +221,16 @@ pub trait UdsTransport { /// conformant and leaves the response buffer as the only bound. MDS is defined for /// *requests*, so a server asking what it may send is asking about the client. /// - /// There is deliberately no `inbound_max`. A request longer than the server's - /// in-flight buffer still reaches the server, as [`TransportEvent::DataTooLong`], - /// so the pipeline answers it in ISO 14229-1's order: an unsupported service - /// `0x11` before a wrong length `0x13`. A transport refuses only what it cannot - /// hold itself. + /// There is no `inbound_max`: a request longer than the server's in-flight buffer + /// still reaches the server, as [`TransportEvent::DataTooLong`], so the pipeline + /// answers it in ISO 14229-1's order, an unsupported service `0x11` before a wrong + /// length `0x13`. A transport refuses only what it cannot hold itself. fn outbound_max(&self) -> Option; - /// The `tP_Client` reload pair this transport dictates. - /// - /// The pair alone: a transport dictates it — `DoIP` has no `T_DataSOM.ind`, hence - /// `tP6` rather than `tP2`. `tP3` spacing is not part of it; the client configures it - /// in [`crate::ClientTiming`]. ISO 14229-2:2021 Table 4 sets its minimum per transport, - /// from the same network delay, and its footnotes d and e leave the maximum to the - /// client. + /// The `tP_Client` reload pair this transport dictates (`DoIP` has no `T_DataSOM.ind`, + /// hence `tP6` rather than `tP2`). `tP3` spacing is not part of it; the client + /// configures that in [`crate::ClientTiming`]. ISO 14229-2:2021 Table 4 sets the + /// minimum per transport, and its footnotes d and e leave the maximum to the client. fn channel_timing(&self) -> Reloads; /// Monotonic milliseconds, 32-bit and wrapping. @@ -290,10 +250,9 @@ pub trait UdsTransport { /// A transport a client sends over: a [`UdsTransport`] whose connection the client can /// end. /// -/// Ending a session is the client's alone. A server is closed *to*, and the one close it -/// makes, the prescribed close of ISO 14229-5:2022 REQ 7.9 and REQ 7.11, its transport -/// makes itself on [`AfterSend::ServerLeaves`] or a positive `ECUReset` response. So -/// [`crate::Client`] needs this and [`crate::Server`] does not. +/// Only a client ends a session. The one close a server makes, the prescribed close of +/// ISO 14229-5:2022 REQ 7.9 and REQ 7.11, its transport makes itself on +/// [`AfterSend::ServerLeaves`] or a positive `ECUReset` response. pub trait ClientTransport: UdsTransport { /// Ends the connection to the server gracefully, if there is one. /// @@ -372,14 +331,10 @@ mod tests { } } - /// ``UDSSVC_ARCH_0029`` as amended — the event borrows the *buffer*, so the driver - /// answers a request while its bytes are still live. This is the shape `Server::step` - /// needs and the one a borrow of the transport would forbid: an event tied to - /// `&mut self` keeps the transport mutably borrowed for as long as the request lives, - /// and `t_data_req` could never be called to answer it. - /// - /// This is a compile-time assertion wearing a test's clothes. It fails by not - /// building. + /// ``UDSSVC_ARCH_0029``: the event borrows the *buffer*, so the driver answers a + /// request while its bytes are still live. An event tied to `&mut self` would keep + /// the transport borrowed for as long as the request lives, and `t_data_req` could + /// never be called to answer it. This fails by not building. #[test] fn a_request_can_be_answered_while_its_bytes_are_live() { let mut buffer = [0_u8; 8]; @@ -395,10 +350,8 @@ mod tests { assert_eq!(poll_once(sent), Some(Ok(3))); } - /// A truncated message is a distinct variant rather than a flag on `DataInd`. - /// Destructuring `DataInd { ai, data, .. }` would silently discard a flag and - /// decode a fragment as a message; a swallowed variant only drops a message the - /// client is already required to repeat. One fails safe, the other dangerous. + /// A truncated message is a distinct variant rather than a flag on `DataInd`, so + /// destructuring `DataInd { ai, data, .. }` cannot decode a fragment as a message. #[test] fn truncation_is_a_variant_an_ordinary_arm_cannot_absorb() { let whole = TransportEvent::DataInd { diff --git a/crates/uds_services/tests/client.rs b/crates/uds_services/tests/client.rs index 7c5bbdb4..a2416851 100644 --- a/crates/uds_services/tests/client.rs +++ b/crates/uds_services/tests/client.rs @@ -3094,7 +3094,7 @@ fn a_refused_reset_is_no_replacement() { assert_eq!(block_on(t.connection_replaced(ECU)), Ok(Replaced::No)); } -/// A server with no open channel was replaced by nothing. +/// A server with no open channel reports no replacement. #[test] fn an_unknown_server_was_not_replaced() { let mut t = tester(&[Step::At(0)]); diff --git a/crates/uds_services/tests/composition.rs b/crates/uds_services/tests/composition.rs index 2239f727..18b092da 100644 --- a/crates/uds_services/tests/composition.rs +++ b/crates/uds_services/tests/composition.rs @@ -335,10 +335,9 @@ uds_server! { const PARAMS: ServerParams = ServerParams::new(5_000, 50, 5_000, 0); -/// The construction that matters: a multi-kilobyte server in a `static`, built in place -/// with no stack temporary. This is what `Storage::EMPTY` being an associated const buys. -/// The cell is what makes it steppable: it hands out the one `&'static mut` that -/// `Server::step` needs, where a plain `static` would be read-only. +/// A multi-kilobyte server in a `static`, built in place with no stack temporary. The +/// cell hands out the one `&'static mut` that `Server::step` needs, where a plain +/// `static` would be read-only. static SERVER: ConstStaticCell = ConstStaticCell::new(EcuServer::new( Ecu::new(), FakeTransport, @@ -346,18 +345,9 @@ static SERVER: ConstStaticCell = ConstStaticCell::new(EcuServer::new( PARAMS, )); -/// The in-flight buffer is dominated by `TransferData` — the same constant the server -/// must advertise as `maxNumberOfBlockLength`. The response buffer is dominated by a -/// four-identifier read, because this server is download-only and never sends a block. -/// -/// `ReadDTCInformation` is the other contender and loses: this server declares -/// [`DtcReportKind::SeverityList`], whose six-byte records make it the widest of the two -/// layouts it answers, for `3 + 10 * 6 = 63` against the read's 77. Under the -/// `DTC_RECORD_LEN` this replaced, the obvious declaration of `4` would have folded in -/// `3 + 10 * 4 = 43` and left a severity report twenty bytes short of its own buffer. -/// -/// **If these fail, the macro's bound arms are wrong, not these numbers.** Read a failure -/// as the design being wrong about a bound. +/// The in-flight buffer is `TransferData`'s, the constant the server advertises as +/// `maxNumberOfBlockLength`. The response buffer is a four-identifier read's, 77 bytes, +/// against `ReadDTCInformation`'s 63 for ten [`DtcReportKind::SeverityList`] records. #[test] fn the_buffers_are_derived_from_the_declared_maxima() { let mut store = <::Store as Storage>::EMPTY; @@ -373,7 +363,7 @@ fn the_buffers_are_derived_from_the_declared_maxima() { ); } -/// ``UDSSVC_ARCH_0006`` — `serviceNotSupported` is decided from the assembly list. +/// ``UDSSVC_ARCH_0006``: `serviceNotSupported` is decided from the assembly list. #[test] fn the_assembled_list_answers_service_supported() { assert!(EcuServices::supports(UdsServiceType::ReadDataByIdentifier)); @@ -393,7 +383,7 @@ fn the_assembled_list_answers_service_supported() { ))); } -/// ``UDSSVC_ARCH_0033`` — Annex A permission is per service and declared. +/// ``UDSSVC_ARCH_0033``: Annex A permission is per service and declared. #[test] fn response_pending_permission_follows_the_declaration() { assert!(EcuServices::may_respond_pending( @@ -405,23 +395,16 @@ fn response_pending_permission_follows_the_declaration() { assert!(!EcuServices::may_respond_pending( UdsServiceType::WriteDataByIdentifier )); - // 0x3E is false because the trait carries no constant to declare otherwise, not - // because this server declared it so. Nothing an application writes can flip it. + // 0x3E is false because the trait carries no constant to declare otherwise. assert!(!EcuServices::may_respond_pending( UdsServiceType::TesterPresent )); } /// The one message clause 8.7.6 admits mid-service is a **functionally addressed** -/// suppressed `TesterPresent`, `3E 80` exactly. The same bytes physically addressed are -/// not admitted, which is the distinction the classifier could not draw while its only -/// argument was the request. -/// -/// The 0x01 assertion covers the clause's other exception, a request in 0x00-0x0F. No -/// server `uds_server!` can assemble reaches it — the lowest SID in `__uds_sids!` is -/// 0x10, because that range is OBD territory and `uds_protocol` does not model it — so -/// what this pins is that such a request is ordinary occupancy here, owing -/// busyRepeatRequest like any other. +/// suppressed `TesterPresent`, `3E 80` exactly; the same bytes physically addressed are +/// not admitted. A request in 0x00-0x0F, the clause's other exception, is ordinary +/// occupancy here: no server `uds_server!` assembles has a service in that range. #[test] fn the_tester_present_exception_is_admitted_only_when_functionally_addressed() { fn ai(ta_type: TaType) -> Ai { @@ -461,13 +444,9 @@ fn the_server_in_a_static_can_be_stepped() { ); } -/// The handler seam speaks `uds_protocol`'s vocabulary, so a sub-function is a named -/// value rather than a byte. The two `CommunicationControl` sub-function bytes were the -/// hazard this retyping removes: as `u8`s they sat adjacent in the signature and swapping -/// them compiled, and as unrelated enums the swap is a type error. -/// -/// `ReadDtcInformation` is the other half of the change: its report type carries its own -/// parameters, so the handler no longer receives — or has to parse — a `&[u8]` beside it. +/// The handler seam speaks `uds_protocol`'s vocabulary: `CommunicationControl`'s +/// sub-functions are unrelated enums, so swapping them is a type error, and +/// `ReadDtcInformation`'s report type carries its own parameters. #[test] fn the_handler_seam_is_typed_not_byte_shaped() { fn poll_once(mut f: core::pin::Pin<&mut F>) -> Option { @@ -489,7 +468,7 @@ fn the_handler_seam_is_typed_not_byte_shaped() { ))); assert_eq!(denied, Some(Err(Nrc::ConditionsNotCorrect))); - // `SubnetNumber` distinguishes these two, where the old `Option` could not. + // `SubnetNumber` distinguishes these two. assert_ne!(SubnetNumber::ReceivedOn, SubnetNumber::AllConnectedNetworks); let mut buffer = [0_u8; 8]; @@ -544,7 +523,7 @@ static TESTER: Tester = Tester::new( /// the server folded in for `ReadDataByIdentifier`. /// /// **If these disagree, one side of the vocabulary moved without the other.** That is the -/// compile error one identifier catalogue serving both roles exists to produce. +/// compile error one identifier catalog serving both roles exists to produce. #[test] fn the_client_buffers_are_derived_from_the_same_declaration() { let mut store = <::Store as ClientStorage>::EMPTY; @@ -594,7 +573,7 @@ async fn read_the_vin(tester: &mut Tester) -> Result<&'static str, ClientError<( let _ = error; "unreadable" } - _ => "unrecognised", + _ => "unrecognized", }, ) } diff --git a/crates/uds_services/tests/driver.rs b/crates/uds_services/tests/driver.rs index f6b76753..18a07014 100644 --- a/crates/uds_services/tests/driver.rs +++ b/crates/uds_services/tests/driver.rs @@ -715,18 +715,10 @@ fn a_session_pair_the_lead_does_not_fit_panics() { /// A request longer than the in-flight buffer, with no service in progress, is checked as /// clause 8.7 checks any request, in ISO 14229-1:2020 Figure 5's and Figure 6's order: -/// `serviceNotSupported` (0x11) for a service this server lacks, `subFunctionNotSupported` -/// (0x12) for a sub-function it lacks, and only then -/// `incorrectMessageLengthOrInvalidFormat` (0x13), that buffer holding the longest request -/// any assembled service accepts. Never `busyRepeatRequest`, which would have the client -/// repeat it forever. 0x11 and 0x12 to a functional request are suppressed (8.7.5). The -/// session checks precede 0x13 too: 0x7F is pinned in `composition.rs`, and 0x7E in -/// [`an_over_long_request_for_a_sub_function_not_in_this_session_is_refused_0x7e`]. -/// -/// `Timed`'s in-flight buffer is six bytes, `DiagnosticSessionControl`'s bound, so what -/// fits of `SESSION` is itself too long and 0x13 comes from decoding it; -/// [`an_over_long_request_whose_front_decodes_is_not_handled`] pins the 0x13 a request -/// whose front decodes owes. +/// `serviceNotSupported` (0x11), `subFunctionNotSupported` (0x12), and only then +/// `incorrectMessageLengthOrInvalidFormat` (0x13); never `busyRepeatRequest`. 0x11 and +/// 0x12 to a functional request are suppressed (8.7.5). `Timed`'s in-flight buffer is six +/// bytes, so what fits of `SESSION` is itself too long and 0x13 comes from decoding it. #[test] fn a_request_too_long_for_any_service_is_refused_in_figure_5s_order() { const NOT_SUPPORTED: &[u8] = &[0x2E, 0xF1, 0x90, 1, 2, 3, 4, 5, 6]; diff --git a/crates/uds_services/tests/end_to_end.rs b/crates/uds_services/tests/end_to_end.rs index 56a39c45..42b5bba1 100644 --- a/crates/uds_services/tests/end_to_end.rs +++ b/crates/uds_services/tests/end_to_end.rs @@ -554,19 +554,16 @@ fn a_final_response_after_a_pending_whose_confirmation_is_late_is_still_delivere assert_eq!(t.sent(1).map(|s| s.after), Some(3)); } -/// ``UDSS_LLR_0108``, ``UDSS_LLR_0109`` — a slow request from the same tester whose +/// ``UDSS_LLR_0108``, ``UDSS_LLR_0109``: a slow request from the same tester whose /// indication precedes the previous response's confirmation keeps its `tP2_Server`, so /// it still gets its `7F 22 78` and then its answer. /// /// `3E 00` is answered at once and `7E 00` left unconfirmed; the slow read arrives at -/// 10 ms, so its `tP2_Server` runs to 60 ms. `7E 00`'s `DataConf` arrives mid-handler: -/// it frees the one association (`peers = 1`) and answers nothing, so the deadline the -/// handler is raced against is still 60 ms. It is scripted before that deadline because -/// the 0x78 shares `7E 00`'s addressing and ``UDSS_LLR_0061`` would refuse it while -/// `7E 00` was outstanding; likewise, had it come after the read finished, the final -/// response would have waited for it in `await_confirmation`. The handler pends three -/// times — the late confirmation, the deadline, the 0x78's confirmation — and its -/// final response is accepted at once. +/// 10 ms, so its `tP2_Server` runs to 60 ms. `7E 00`'s `DataConf` arrives mid-handler, +/// before that deadline, since the 0x78 shares its addressing and ``UDSS_LLR_0061`` +/// would refuse it while `7E 00` was outstanding. The handler pends three times (the +/// late confirmation, the deadline, the 0x78's confirmation) and its final response is +/// accepted at once. #[test] fn a_request_before_the_previous_confirmation_keeps_its_response_timer() { let mut ecu = Ecu::new(); diff --git a/crates/uds_services/tests/stages.rs b/crates/uds_services/tests/stages.rs index ae01a8d3..d8cd135f 100644 --- a/crates/uds_services/tests/stages.rs +++ b/crates/uds_services/tests/stages.rs @@ -1999,7 +1999,7 @@ fn read_dtc_a_wrong_length_is_0x13() { } } -/// Clause 12.3.4 — a `DTCMaskRecord` the server does not recognise is the handler's 0x31. +/// Clause 12.3.4 — a `DTCMaskRecord` the server does not recognize is the handler's 0x31. #[test] fn read_dtc_an_unknown_dtc_is_the_handler_0x31() { let mut ecu = Ecu::default(); @@ -2777,7 +2777,7 @@ fn a_refused_block_advances_nothing() { /// Table 451 — the counter rolls `FF` to `00`, and the block after the roll repeated is a /// repeat. #[test] -fn the_counter_rolls_over_and_a_repeat_after_the_roll_is_recognised() { +fn the_counter_rolls_over_and_a_repeat_after_the_roll_is_recognized() { let mut ecu = Ecu::default(); // 4 KiB at 0x2000, uncompressed: room for 300 one-byte blocks. let mut state = downloading(&mut ecu, &[0x34, 0x00, 0x22, 0x20, 0x00, 0x10, 0x00]);