From d8a6dd171e9487c2e406c9d01ef5f0675856ca58 Mon Sep 17 00:00:00 2001 From: Zach Heylmun Date: Mon, 29 Jun 2026 20:17:54 -0400 Subject: [PATCH 1/6] Flatten suppressable request structs into public data bags MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The no_std port left request types split between two visibility styles. Per our convention (encapsulate only invariant-bearing structs, else public data bag), flatten the suppressable single-/multi-field requests to public fields and drop the redundant getters. The SPRMIB bit-packing now lives only at the encode/decode boundary via a transient SuppressablePositiveResponse, which stays pub(crate). Also harden the sub-function enums: the reserved open-ended (u8) variants are marked #[non_exhaustive] so callers must build them via the range-checked TryFrom, making a SPRMIB-colliding value (>= 0x80) unconstructable rather than caught at encode time. Notable exceptions: - CommunicationControlRequest stays encapsulated: node_id must be Some iff control_type is an enhanced-address variant — a real cross-field invariant. Its reserved CommunicationControlType variants are still locked. - SecurityAccessType::RequestSeed/SendKey stay directly constructible (user-facing values); only ISOSAEReserved/SystemSupplierSpecific are locked, so its construction-time guarantee is partial. - TesterPresentRequest is now just { suppress_positive_response }; a non-zero (reserved) sub-function byte still decodes but normalizes to the zero sub-function on re-encode (it was previously preserved). Co-Authored-By: Claude Opus 4.8 (1M context) --- src/request.rs | 12 ++--- src/services/communication_control.rs | 18 +++++-- src/services/control_dtc_settings.rs | 38 ++++++------- src/services/diagnostic_session_control.rs | 53 ++++++++++-------- src/services/ecu_reset.rs | 53 +++++++++++------- src/services/routine_control.rs | 59 ++++++++------------ src/services/security_access.rs | 50 +++++++++-------- src/services/tester_present.rs | 63 ++++++++++------------ 8 files changed, 178 insertions(+), 168 deletions(-) diff --git a/src/request.rs b/src/request.rs index 5fa2b3a..2429ca9 100644 --- a/src/request.rs +++ b/src/request.rs @@ -184,12 +184,12 @@ impl Request<'_> { pub fn is_positive_response_suppressed(&self) -> bool { match self { Self::CommunicationControl(req) => req.suppress_positive_response(), - Self::ControlDTCSettings(req) => req.suppress_positive_response(), - Self::DiagnosticSessionControl(req) => req.suppress_positive_response(), - Self::EcuReset(req) => req.suppress_positive_response(), - Self::RoutineControl(req) => req.suppress_positive_response(), - Self::SecurityAccess(req) => req.suppress_positive_response(), - Self::TesterPresent(req) => req.suppress_positive_response(), + Self::ControlDTCSettings(req) => req.suppress_positive_response, + Self::DiagnosticSessionControl(req) => req.suppress_positive_response, + Self::EcuReset(req) => req.suppress_positive_response, + Self::RoutineControl(req) => req.suppress_positive_response, + Self::SecurityAccess(req) => req.suppress_positive_response, + Self::TesterPresent(req) => req.suppress_positive_response, _ => false, } } diff --git a/src/services/communication_control.rs b/src/services/communication_control.rs index 5570acc..63c2088 100644 --- a/src/services/communication_control.rs +++ b/src/services/communication_control.rs @@ -34,14 +34,26 @@ pub enum CommunicationControlType { /// shall be enabled for the specified [`CommunicationType`] /// Additionally, enhanced address information shall be included in the request EnableRxAndTxWithEnhancedAddressInfo, - /// These values are reserved by the ISO 14229-1 Specification + /// These values are reserved by the ISO 14229-1 Specification. + /// + /// Construct through [`CommunicationControlType::try_from`] so the raw byte is + /// range-checked and can never collide with the SPRMIB bit. #[cfg_attr(feature = "clap", clap(skip))] + #[non_exhaustive] ISOSAEReserved(u8), - /// Values reserved for use by vehicle manufacturers + /// Values reserved for use by vehicle manufacturers. + /// + /// Construct through [`CommunicationControlType::try_from`] so the raw byte is + /// range-checked and can never collide with the SPRMIB bit. #[cfg_attr(feature = "clap", clap(skip))] + #[non_exhaustive] VehicleManufacturerSpecific(u8), - /// Values reserved for use by system suppliers + /// Values reserved for use by system suppliers. + /// + /// Construct through [`CommunicationControlType::try_from`] so the raw byte is + /// range-checked and can never collide with the SPRMIB bit. #[cfg_attr(feature = "clap", clap(skip))] + #[non_exhaustive] SystemSupplierSpecific(u8), } diff --git a/src/services/control_dtc_settings.rs b/src/services/control_dtc_settings.rs index 234515f..5a4c45d 100644 --- a/src/services/control_dtc_settings.rs +++ b/src/services/control_dtc_settings.rs @@ -43,7 +43,10 @@ impl TryFrom for DtcSettings { #[derive(Clone, Copy, Debug, Eq, PartialEq)] #[non_exhaustive] pub struct ControlDTCSettingsRequest { - setting: SuppressablePositiveResponse, + /// Whether the server should suppress the positive response (SPRMIB). + pub suppress_positive_response: bool, + /// The requested DTC logging setting. + pub setting: DtcSettings, } impl ControlDTCSettingsRequest { @@ -51,21 +54,10 @@ impl ControlDTCSettingsRequest { #[must_use] pub const fn new(suppress_positive_response: bool, setting: DtcSettings) -> Self { Self { - setting: SuppressablePositiveResponse::new(suppress_positive_response, setting), + suppress_positive_response, + setting, } } - - /// Returns the requested DTC logging setting. - #[must_use] - pub fn setting(&self) -> DtcSettings { - self.setting.value() - } - - /// Whether the server should suppress the positive response (SPRMIB). - #[must_use] - pub fn suppress_positive_response(&self) -> bool { - self.setting.suppress_positive_response() - } } impl Encode for ControlDTCSettingsRequest { @@ -74,8 +66,10 @@ impl Encode for ControlDTCSettingsRequest { } fn encode(&self, writer: &mut impl embedded_io::Write) -> Result { + let sub_function = + SuppressablePositiveResponse::new(self.suppress_positive_response, self.setting); writer - .write_all(&[u8::from(self.setting)]) + .write_all(&[u8::from(sub_function)]) .map_err(Error::io)?; Ok(1) } @@ -86,8 +80,14 @@ impl<'a> Decode<'a> for ControlDTCSettingsRequest { if buf.is_empty() { return Err(Error::InsufficientData(1)); } - let setting = SuppressablePositiveResponse::try_from(buf[0])?; - Ok((Self { setting }, &buf[1..])) + let sub_function = SuppressablePositiveResponse::::try_from(buf[0])?; + Ok(( + Self { + suppress_positive_response: sub_function.suppress_positive_response(), + setting: sub_function.value(), + }, + &buf[1..], + )) } } @@ -152,8 +152,8 @@ mod request { assert_eq!(req.encoded_size(), buffer.len()); let (parsed, _) = ::decode(&buffer).unwrap(); - assert_eq!(parsed.setting(), DtcSettings::On); - assert!(parsed.suppress_positive_response()); + assert_eq!(parsed.setting, DtcSettings::On); + assert!(parsed.suppress_positive_response); assert_encode_size_agrees(&req); } diff --git a/src/services/diagnostic_session_control.rs b/src/services/diagnostic_session_control.rs index 73a9039..01f37ac 100644 --- a/src/services/diagnostic_session_control.rs +++ b/src/services/diagnostic_session_control.rs @@ -25,7 +25,10 @@ use crate::{Decode, Encode, Error, NegativeResponseCode}; #[non_exhaustive] pub enum DiagnosticSessionType { /// This value is reserved by the ISO 14229-1 Specification + /// + /// Construct through [`DiagnosticSessionType::try_from`] so the raw byte is range-checked and can never collide with the SPRMIB bit. #[cfg_attr(feature = "clap", clap(skip))] + #[non_exhaustive] ISOSAEReserved(u8), /// The `DefaultSession` (0x01) enables the standard diagnostic functionality /// - No `TesterPresent` messages are required to remain in this session @@ -42,10 +45,16 @@ pub enum DiagnosticSessionType { /// The `SafetySystemDiagnosticSession` (0x04) enables diagnostics functionality for safety systems SafetySystemDiagnosticSession, /// Value reserved for use by vehicle manufacturers + /// + /// Construct through [`DiagnosticSessionType::try_from`] so the raw byte is range-checked and can never collide with the SPRMIB bit. #[cfg_attr(feature = "clap", clap(skip))] + #[non_exhaustive] VehicleManufacturerSpecificSession(u8), /// Value reserved for use by system suppliers + /// + /// Construct through [`DiagnosticSessionType::try_from`] so the raw byte is range-checked and can never collide with the SPRMIB bit. #[cfg_attr(feature = "clap", clap(skip))] + #[non_exhaustive] SystemSupplierSpecificSession(u8), } @@ -163,7 +172,10 @@ const DIAGNOSTIC_SESSION_CONTROL_NEGATIVE_RESPONSE_CODES: [NegativeResponseCode; #[derive(Clone, Copy, Debug, Eq, PartialEq)] #[non_exhaustive] pub struct DiagnosticSessionControlRequest { - session_type: SuppressablePositiveResponse, + /// Whether a positive response should be suppressed. + pub suppress_positive_response: bool, + /// The requested diagnostic session type. + pub session_type: DiagnosticSessionType, } impl DiagnosticSessionControlRequest { @@ -174,25 +186,11 @@ impl DiagnosticSessionControlRequest { session_type: DiagnosticSessionType, ) -> Self { Self { - session_type: SuppressablePositiveResponse::new( - suppress_positive_response, - session_type, - ), + suppress_positive_response, + session_type, } } - /// Getter for whether a positive response should be suppressed - #[must_use] - pub fn suppress_positive_response(&self) -> bool { - self.session_type.suppress_positive_response() - } - - /// Getter for the requested session type - #[must_use] - pub fn session_type(&self) -> DiagnosticSessionType { - self.session_type.value() - } - /// Get the allowed [`NegativeResponseCode`] variants for this request #[must_use] pub fn allowed_nack_codes() -> &'static [NegativeResponseCode] { @@ -205,8 +203,12 @@ impl Encode for DiagnosticSessionControlRequest { } fn encode(&self, writer: &mut impl embedded_io::Write) -> Result { + let sub_function = SuppressablePositiveResponse::new( + self.suppress_positive_response, + self.session_type, + ); writer - .write_all(&[u8::from(self.session_type)]) + .write_all(&[u8::from(sub_function)]) .map_err(Error::io)?; Ok(1) } @@ -217,8 +219,15 @@ impl<'a> Decode<'a> for DiagnosticSessionControlRequest { if buf.is_empty() { return Err(Error::InsufficientData(1)); } - let session_type = SuppressablePositiveResponse::try_from(buf[0])?; - Ok((Self { session_type }, &buf[1..])) + let sub_function = + SuppressablePositiveResponse::::try_from(buf[0])?; + Ok(( + Self { + suppress_positive_response: sub_function.suppress_positive_response(), + session_type: sub_function.value(), + }, + &buf[1..], + )) } } @@ -301,9 +310,9 @@ mod request { fn test_diagnostic_session_control_request() { let bytes: [u8; 1] = [0x02]; let (req, _) = ::decode(&bytes).unwrap(); - assert!(!req.suppress_positive_response()); + assert!(!req.suppress_positive_response); assert_eq!( - req.session_type(), + req.session_type, DiagnosticSessionType::ProgrammingSession ); diff --git a/src/services/ecu_reset.rs b/src/services/ecu_reset.rs index f24246f..9f44c21 100644 --- a/src/services/ecu_reset.rs +++ b/src/services/ecu_reset.rs @@ -15,8 +15,12 @@ use crate::{Decode, Encode, Error, NegativeResponseCode}; #[derive(Clone, Copy, Debug, Eq, PartialEq)] #[non_exhaustive] pub enum ResetType { - /// This value is reserved + /// This value is reserved. + /// + /// Construct reserved values through [`ResetType::try_from`] so the raw byte is + /// range-checked (`0x00..=0x7F`) and can never collide with the SPRMIB bit. #[cfg_attr(feature = "clap", clap(skip))] + #[non_exhaustive] ISOSAEReserved(u8), /// This `SubFunction` identifies a "hard reset" condition which simulates the power-on/start-up sequence /// typically performed after a server has been previously disconnected from its power supply (i.e. battery). @@ -49,11 +53,19 @@ pub enum ResetType { EnableRapidPowerShutDown, /// This `SubFunction` requests the server to disable the previously enabled "rapid power shut down" function. DisableRapidPowerShutDown, - /// Reserved for use by vehicle manufacturers + /// Reserved for use by vehicle manufacturers. + /// + /// Construct through [`ResetType::try_from`] so the raw byte is range-checked + /// (`0x40..=0x5F`) and can never collide with the SPRMIB bit. #[cfg_attr(feature = "clap", clap(skip))] + #[non_exhaustive] VehicleManufacturerSpecific(u8), - /// Reserved for use by system suppliers + /// Reserved for use by system suppliers. + /// + /// Construct through [`ResetType::try_from`] so the raw byte is range-checked + /// (`0x60..=0x7E`) and can never collide with the SPRMIB bit. #[cfg_attr(feature = "clap", clap(skip))] + #[non_exhaustive] SystemSupplierSpecific(u8), } @@ -175,7 +187,10 @@ const ECU_RESET_NEGATIVE_RESPONSE_CODES: [NegativeResponseCode; 4] = [ #[derive(Clone, Copy, Debug, Eq, PartialEq)] #[non_exhaustive] pub struct EcuResetRequest { - reset_type: SuppressablePositiveResponse, + /// Whether the server should suppress a positive response (SPRMIB). + pub suppress_positive_response: bool, + /// The type of reset being requested. + pub reset_type: ResetType, } impl EcuResetRequest { @@ -183,22 +198,11 @@ impl EcuResetRequest { #[must_use] pub const fn new(suppress_positive_response: bool, reset_type: ResetType) -> Self { Self { - reset_type: SuppressablePositiveResponse::new(suppress_positive_response, reset_type), + suppress_positive_response, + reset_type, } } - /// Getter for whether a positive response should be suppressed - #[must_use] - pub fn suppress_positive_response(&self) -> bool { - self.reset_type.suppress_positive_response() - } - - /// Getter for the requested [`ResetType`] - #[must_use] - pub fn reset_type(&self) -> ResetType { - self.reset_type.value() - } - /// Get the allowed [`NegativeResponseCode`] variants for this request #[must_use] pub fn allowed_nack_codes() -> &'static [NegativeResponseCode] { @@ -212,8 +216,11 @@ impl Encode for EcuResetRequest { } fn encode(&self, writer: &mut impl embedded_io::Write) -> Result { + // Fuse the SPRMIB bit into the sub-function byte only at the wire boundary. + let sub_function = + SuppressablePositiveResponse::new(self.suppress_positive_response, self.reset_type); writer - .write_all(&[u8::from(self.reset_type)]) + .write_all(&[u8::from(sub_function)]) .map_err(Error::io)?; Ok(1) } @@ -224,8 +231,14 @@ impl<'a> Decode<'a> for EcuResetRequest { if buf.is_empty() { return Err(Error::InsufficientData(1)); } - let reset_type = SuppressablePositiveResponse::try_from(buf[0])?; - Ok((Self { reset_type }, &buf[1..])) + let sub_function = SuppressablePositiveResponse::::try_from(buf[0])?; + Ok(( + Self { + suppress_positive_response: sub_function.suppress_positive_response(), + reset_type: sub_function.value(), + }, + &buf[1..], + )) } } diff --git a/src/services/routine_control.rs b/src/services/routine_control.rs index 22cdd16..c80e948 100644 --- a/src/services/routine_control.rs +++ b/src/services/routine_control.rs @@ -56,9 +56,14 @@ impl TryFrom for RoutineControlSubFunction { #[derive(Clone, Copy, Debug, Eq, PartialEq)] #[non_exhaustive] pub struct RoutineControlRequest<'d> { - sub_function: SuppressablePositiveResponse, - routine_id: u16, - option_record: &'d [u8], + /// Whether the server should suppress the positive response (SPRMIB). + pub suppress_positive_response: bool, + /// The routine control operation (start, stop, or request results). + pub sub_function: RoutineControlSubFunction, + /// The 16-bit routine identifier. + pub routine_id: u16, + /// Optional routine input parameters (may be empty). + pub option_record: &'d [u8], } impl<'d> RoutineControlRequest<'d> { @@ -71,38 +76,12 @@ impl<'d> RoutineControlRequest<'d> { option_record: &'d [u8], ) -> Self { Self { - sub_function: SuppressablePositiveResponse::new( - suppress_positive_response, - sub_function, - ), + suppress_positive_response, + sub_function, routine_id, option_record, } } - - /// Whether the server should suppress the positive response (SPRMIB). - #[must_use] - pub fn suppress_positive_response(&self) -> bool { - self.sub_function.suppress_positive_response() - } - - /// The routine control operation (start, stop, or request results). - #[must_use] - pub fn sub_function(&self) -> RoutineControlSubFunction { - self.sub_function.value() - } - - /// The 16-bit routine identifier. - #[must_use] - pub const fn routine_id(&self) -> u16 { - self.routine_id - } - - /// Optional routine input parameters (may be empty). - #[must_use] - pub const fn option_record(&self) -> &[u8] { - self.option_record - } } impl Encode for RoutineControlRequest<'_> { @@ -111,8 +90,10 @@ impl Encode for RoutineControlRequest<'_> { } fn encode(&self, writer: &mut impl embedded_io::Write) -> Result { + let sub_function = + SuppressablePositiveResponse::new(self.suppress_positive_response, self.sub_function); writer - .write_all(&[u8::from(self.sub_function)]) + .write_all(&[u8::from(sub_function)]) .map_err(Error::io)?; writer .write_all(&self.routine_id.to_be_bytes()) @@ -127,11 +108,13 @@ impl<'a> Decode<'a> for RoutineControlRequest<'a> { if buf.len() < 3 { return Err(Error::InsufficientData(3)); } - let sub_function = SuppressablePositiveResponse::try_from(buf[0])?; + let sub_function = + SuppressablePositiveResponse::::try_from(buf[0])?; let routine_id = u16::from_be_bytes([buf[1], buf[2]]); Ok(( Self { - sub_function, + suppress_positive_response: sub_function.suppress_positive_response(), + sub_function: sub_function.value(), routine_id, option_record: &buf[3..], }, @@ -241,10 +224,10 @@ mod test { assert_eq!(&buf[..n], &[0x81, 0xFF, 0x00, 0xAA]); // 0x81 = StartRoutine | SPRMIB let (d, rest) = ::decode(&buf[..n]).unwrap(); assert!(rest.is_empty()); - assert!(d.suppress_positive_response()); - assert_eq!(d.sub_function(), RoutineControlSubFunction::StartRoutine); - assert_eq!(d.routine_id(), 0xFF00); - assert_eq!(d.option_record(), &[0xAA]); + assert!(d.suppress_positive_response); + assert_eq!(d.sub_function, RoutineControlSubFunction::StartRoutine); + assert_eq!(d.routine_id, 0xFF00); + assert_eq!(d.option_record, &[0xAA]); assert_encode_size_agrees(&req); } diff --git a/src/services/security_access.rs b/src/services/security_access.rs index f00c49d..2348d02 100644 --- a/src/services/security_access.rs +++ b/src/services/security_access.rs @@ -17,6 +17,10 @@ use crate::{Decode, Encode, Error, NegativeResponseCode}; #[non_exhaustive] pub enum SecurityAccessType { /// This value is reserved for future definition + /// + /// Construct through [`SecurityAccessType::try_from`] so the raw byte is range-checked + /// and can never collide with the SPRMIB bit. + #[non_exhaustive] ISOSAEReserved(u8), /// `RequestSeed` with the level of security defined by the vehicle manufacturer RequestSeed(u8), @@ -28,6 +32,10 @@ pub enum SecurityAccessType { /// `SendKey` with different levels of security defined for end of life activation ISO26021_2SendKeyValues, /// This range of values is reserved for system supplier specific use + /// + /// Construct through [`SecurityAccessType::try_from`] so the raw byte is range-checked + /// and can never collide with the SPRMIB bit. + #[non_exhaustive] SystemSupplierSpecific(u8), } @@ -181,8 +189,12 @@ const SECURITY_ACCESS_NEGATIVE_RESPONSE_CODES: [NegativeResponseCode; 8] = [ #[derive(Clone, Copy, Debug, Eq, PartialEq)] #[non_exhaustive] pub struct SecurityAccessRequest<'d> { - access_type: SuppressablePositiveResponse, - request_data: &'d [u8], + /// Whether a positive response should be suppressed. + pub suppress_positive_response: bool, + /// The requested [`SecurityAccessType`]. + pub access_type: SecurityAccessType, + /// The implementation-defined request data (seed data record or key bytes). + pub request_data: &'d [u8], } impl<'d> SecurityAccessRequest<'d> { @@ -194,29 +206,12 @@ impl<'d> SecurityAccessRequest<'d> { request_data: &'d [u8], ) -> Self { Self { - access_type: SuppressablePositiveResponse::new(suppress_positive_response, access_type), + suppress_positive_response, + access_type, request_data, } } - /// Getter for whether a positive response should be suppressed - #[must_use] - pub fn suppress_positive_response(&self) -> bool { - self.access_type.suppress_positive_response() - } - - /// Getter for the requested [`SecurityAccessType`] - #[must_use] - pub fn access_type(&self) -> SecurityAccessType { - self.access_type.value() - } - - /// Getter for the request data - #[must_use] - pub const fn request_data(&self) -> &[u8] { - self.request_data - } - /// Get the allowed [`NegativeResponseCode`] variants for this request #[must_use] pub fn allowed_nack_codes() -> &'static [NegativeResponseCode] { @@ -230,8 +225,10 @@ impl Encode for SecurityAccessRequest<'_> { } fn encode(&self, writer: &mut impl embedded_io::Write) -> Result { + let sub_function = + SuppressablePositiveResponse::new(self.suppress_positive_response, self.access_type); writer - .write_all(&[u8::from(self.access_type)]) + .write_all(&[u8::from(sub_function)]) .map_err(Error::io)?; writer.write_all(self.request_data).map_err(Error::io)?; Ok(self.encoded_size()) @@ -243,10 +240,11 @@ impl<'a> Decode<'a> for SecurityAccessRequest<'a> { if buf.is_empty() { return Err(Error::InsufficientData(1)); } - let access_type = SuppressablePositiveResponse::try_from(buf[0])?; + let sub_function = SuppressablePositiveResponse::::try_from(buf[0])?; Ok(( Self { - access_type, + suppress_positive_response: sub_function.suppress_positive_response(), + access_type: sub_function.value(), request_data: &buf[1..], }, &[], @@ -321,8 +319,8 @@ mod request { ]; let (req, _) = ::decode(&bytes).unwrap(); - assert_eq!(req.access_type(), SecurityAccessType::RequestSeed(0x01)); - assert_eq!(req.request_data(), &[0x00, 0x01, 0x02, 0x03, 0x04]); + assert_eq!(req.access_type, SecurityAccessType::RequestSeed(0x01)); + assert_eq!(req.request_data, &[0x00, 0x01, 0x02, 0x03, 0x04]); let mut buf = Vec::new(); let written = Encode::encode(&req, &mut buf).unwrap(); diff --git a/src/services/tester_present.rs b/src/services/tester_present.rs index d2eeb71..04d77dc 100644 --- a/src/services/tester_present.rs +++ b/src/services/tester_present.rs @@ -56,34 +56,22 @@ impl TryFrom for ZeroSubFunction { #[derive(Clone, Copy, Debug, Eq, PartialEq)] #[non_exhaustive] pub struct TesterPresentRequest { - zero_sub_function: SuppressablePositiveResponse, + /// Whether the server should suppress a positive response (SPRMIB). + /// + /// `TesterPresent` defines only the zero sub-function, so the suppression flag is + /// the request's sole degree of freedom. + pub suppress_positive_response: bool, } impl TesterPresentRequest { /// Create a new `TesterPresentRequest` #[must_use] - pub fn new(suppress_positive_response: bool) -> Self { - Self::with_subfunction(suppress_positive_response, ZeroSubFunction::default()) - } - - fn with_subfunction( - suppress_positive_response: bool, - zero_sub_function: ZeroSubFunction, - ) -> Self { + pub const fn new(suppress_positive_response: bool) -> Self { Self { - zero_sub_function: SuppressablePositiveResponse::new( - suppress_positive_response, - zero_sub_function, - ), + suppress_positive_response, } } - /// Getter for whether a positive response should be suppressed - #[must_use] - pub fn suppress_positive_response(&self) -> bool { - self.zero_sub_function.suppress_positive_response() - } - /// Get the allowed [`NegativeResponseCode`] variants for this request #[must_use] pub fn allowed_nack_codes() -> &'static [NegativeResponseCode] { @@ -97,8 +85,14 @@ impl Encode for TesterPresentRequest { } fn encode(&self, writer: &mut impl embedded_io::Write) -> Result { + // The only defined sub-function is the zero sub-function; fuse the SPRMIB bit + // onto it at the wire boundary. + let sub_function = SuppressablePositiveResponse::new( + self.suppress_positive_response, + ZeroSubFunction::NoSubFunctionSupported, + ); writer - .write_all(&[u8::from(self.zero_sub_function)]) + .write_all(&[u8::from(sub_function)]) .map_err(Error::io)?; Ok(1) } @@ -109,8 +103,16 @@ impl<'a> Decode<'a> for TesterPresentRequest { if buf.is_empty() { return Err(Error::InsufficientData(1)); } - let zero_sub_function = SuppressablePositiveResponse::try_from(buf[0])?; - Ok((Self { zero_sub_function }, &buf[1..])) + // Validate the sub-function byte (rejects values outside 0x00..=0x7F once SPRMIB is + // stripped); only the suppression flag is retained, since the sub-function is always + // the zero sub-function on re-encode. + let sub_function = SuppressablePositiveResponse::::try_from(buf[0])?; + Ok(( + Self { + suppress_positive_response: sub_function.suppress_positive_response(), + }, + &buf[1..], + )) } } @@ -218,24 +220,17 @@ mod test { assert_eq!(result.unwrap(), expected); } 0x01..=0x7F => { - let result = result.unwrap(); - assert!(!result.suppress_positive_response()); - assert!(matches!( - result.zero_sub_function.value(), - ZeroSubFunction::ISOSAEReserved(_) - )); + // Reserved sub-function bytes decode (SPRMIB clear); the reserved value + // is not retained — re-encoding normalizes to the zero sub-function. + assert!(!result.unwrap().suppress_positive_response); } 0x80 => { let expected = TesterPresentRequest::new(true); assert_eq!(result.unwrap(), expected); } 0x81..=0xFF => { - let result = result.unwrap(); - assert!(result.suppress_positive_response()); - assert!(matches!( - result.zero_sub_function.value(), - ZeroSubFunction::ISOSAEReserved(_) - )); + // SPRMIB set over a reserved value: suppression is retained. + assert!(result.unwrap().suppress_positive_response); } } } From 42b5277ea2f527680d96af7ce9a60eecee1282d5 Mon Sep 17 00:00:00 2001 From: Zach Heylmun Date: Mon, 29 Jun 2026 20:35:22 -0400 Subject: [PATCH 2/6] Flatten remaining non-invariant request/response data bags MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Finish the field-visibility convention sweep for §1A: - RoutineControlResponse: echoed-data response, no invariant — fields made public, getters dropped. - RequestTransferExit{Request,Response}: macro-generated parameter_record is opaque pass-through data — made public, getter dropped. Left encapsulated by design (invariant- or abstraction-bearing): - ReadDataByIdentifierRequest stores a private `Dids` representation (native &[u16] vs borrowed wire bytes); exposing it would leak that abstraction, so the `dids()` iterator stays the accessor. - RequestDownloadRequest keeps private fields + fallible `new`: the address/length format identifier must track the memory address/size widths (cross-field invariant), like CommunicationControlRequest. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/services/request_transfer_exit.rs | 10 +++------ src/services/routine_control.rs | 31 +++++++-------------------- 2 files changed, 11 insertions(+), 30 deletions(-) diff --git a/src/services/request_transfer_exit.rs b/src/services/request_transfer_exit.rs index ec28285..2b17e5f 100644 --- a/src/services/request_transfer_exit.rs +++ b/src/services/request_transfer_exit.rs @@ -7,7 +7,8 @@ macro_rules! transfer_exit_descriptor { #[derive(Clone, Copy, Debug, Eq, PartialEq)] #[non_exhaustive] pub struct $name<'d> { - parameter_record: &'d [u8], + /// The optional, opaque parameter record (empty slice if absent). + pub parameter_record: &'d [u8], } impl<'d> $name<'d> { /// Create from the optional parameter record (empty slice if absent). @@ -15,11 +16,6 @@ macro_rules! transfer_exit_descriptor { pub const fn new(parameter_record: &'d [u8]) -> Self { Self { parameter_record } } - /// The optional, opaque parameter record. - #[must_use] - pub const fn parameter_record(&self) -> &[u8] { - self.parameter_record - } } impl Encode for $name<'_> { fn encoded_size(&self) -> usize { @@ -66,7 +62,7 @@ mod tests { assert_eq!(&buf[..n], rec); let (d, rest) = ::decode(&buf[..n]).unwrap(); assert!(rest.is_empty()); - assert_eq!(d.parameter_record(), rec); + assert_eq!(d.parameter_record, rec); assert_encode_size_agrees(&req); } } diff --git a/src/services/routine_control.rs b/src/services/routine_control.rs index c80e948..6c46b20 100644 --- a/src/services/routine_control.rs +++ b/src/services/routine_control.rs @@ -130,9 +130,12 @@ impl<'a> Decode<'a> for RoutineControlRequest<'a> { #[derive(Clone, Copy, Debug, Eq, PartialEq)] #[non_exhaustive] pub struct RoutineControlResponse<'d> { - sub_function: RoutineControlSubFunction, - routine_id: u16, - status_record: &'d [u8], + /// The routine control operation echoed from the request (start, stop, or request results). + pub sub_function: RoutineControlSubFunction, + /// The 16-bit routine identifier echoed from the request. + pub routine_id: u16, + /// Optional routine status bytes (may be empty). + pub status_record: &'d [u8], } impl<'d> RoutineControlResponse<'d> { @@ -149,24 +152,6 @@ impl<'d> RoutineControlResponse<'d> { status_record, } } - - /// The routine control operation echoed from the request (start, stop, or request results). - #[must_use] - pub const fn sub_function(&self) -> RoutineControlSubFunction { - self.sub_function - } - - /// The 16-bit routine identifier echoed from the request. - #[must_use] - pub const fn routine_id(&self) -> u16 { - self.routine_id - } - - /// Optional routine status bytes (may be empty). - #[must_use] - pub const fn status_record(&self) -> &[u8] { - self.status_record - } } impl Encode for RoutineControlResponse<'_> { @@ -244,8 +229,8 @@ mod test { let n = Encode::encode(&resp, &mut buf.as_mut_slice()).unwrap(); assert_eq!(&buf[..n], &[0x01, 0xFF, 0x00, 0x10]); let (d, _) = ::decode(&buf[..n]).unwrap(); - assert_eq!(d.sub_function(), RoutineControlSubFunction::StartRoutine); - assert_eq!(d.routine_id(), 0xFF00); + assert_eq!(d.sub_function, RoutineControlSubFunction::StartRoutine); + assert_eq!(d.routine_id, 0xFF00); // A response with the SPRMIB bit set (0x81) is malformed and rejected. assert!(::decode(&[0x81, 0xFF, 0x00]).is_err()); assert_encode_size_agrees(&resp); From 8582acf2b687dff3f0491a9a539954dc589a8cba Mon Sep 17 00:00:00 2001 From: Zach Date: Tue, 30 Jun 2026 14:11:18 -0400 Subject: [PATCH 3/6] add tooling to .gitignore --- .gitignore | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.gitignore b/.gitignore index 2ea0c23..f69dcae 100644 --- a/.gitignore +++ b/.gitignore @@ -326,3 +326,7 @@ Testing/ # Fuzz corpus fuzz/corpus/ fuzz/artifacts/ + +.omniscient/ + +.DS_Store From a9da8f896f0ccc761728d8a9425a14a58b6ab843 Mon Sep 17 00:00:00 2001 From: Zach Date: Tue, 30 Jun 2026 14:15:56 -0400 Subject: [PATCH 4/6] cargo fmt... --all --- src/services/diagnostic_session_control.rs | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/src/services/diagnostic_session_control.rs b/src/services/diagnostic_session_control.rs index 01f37ac..1e7ac95 100644 --- a/src/services/diagnostic_session_control.rs +++ b/src/services/diagnostic_session_control.rs @@ -203,10 +203,8 @@ impl Encode for DiagnosticSessionControlRequest { } fn encode(&self, writer: &mut impl embedded_io::Write) -> Result { - let sub_function = SuppressablePositiveResponse::new( - self.suppress_positive_response, - self.session_type, - ); + let sub_function = + SuppressablePositiveResponse::new(self.suppress_positive_response, self.session_type); writer .write_all(&[u8::from(sub_function)]) .map_err(Error::io)?; @@ -219,8 +217,7 @@ impl<'a> Decode<'a> for DiagnosticSessionControlRequest { if buf.is_empty() { return Err(Error::InsufficientData(1)); } - let sub_function = - SuppressablePositiveResponse::::try_from(buf[0])?; + let sub_function = SuppressablePositiveResponse::::try_from(buf[0])?; Ok(( Self { suppress_positive_response: sub_function.suppress_positive_response(), @@ -311,10 +308,7 @@ mod request { let bytes: [u8; 1] = [0x02]; let (req, _) = ::decode(&bytes).unwrap(); assert!(!req.suppress_positive_response); - assert_eq!( - req.session_type, - DiagnosticSessionType::ProgrammingSession - ); + assert_eq!(req.session_type, DiagnosticSessionType::ProgrammingSession); let mut buffer = Vec::new(); Encode::encode(&req, &mut buffer).unwrap(); From 4f4c5470b83aca01c2007f33bfa1586dae7e14b9 Mon Sep 17 00:00:00 2001 From: Zach Heylmun Date: Tue, 30 Jun 2026 21:16:15 -0400 Subject: [PATCH 5/6] Make SecurityAccess levels SPRMIB-safe by construction; fix decode comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses PR review: - SecurityAccessType::RequestSeed/SendKey now carry a SecurityAccessLevel newtype (private u8 + validated `new` rejecting >= 0x80) instead of a raw u8. This was the last suppressable sub-function enum with a user-facing variant that could hold a SPRMIB-colliding value; the value is now unrepresentable rather than caught at encode time. The decode path constructs the level directly (range already guaranteed). - Reword the TesterPresent decode comment: after SPRMIB masking the low 7 bits are always a valid zero sub-function, so the parse never rejects — the previous comment implied validation that cannot fail. Audited the rest of the baseline: of the six SuppressablePositiveResponse -wrapped enums, this was the only remaining unlocked case. The other raw -int enum variants (DIDs, NRCs, DTC numbers) span the full valid integer range and steal no bit, so they carry no comparable invariant. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/lib.rs | 13 ++++-- src/services/mod.rs | 4 +- src/services/security_access.rs | 72 ++++++++++++++++++++++++++++----- src/services/tester_present.rs | 6 +-- 4 files changed, 78 insertions(+), 17 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index edc2d04..d34312e 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -47,7 +47,8 @@ pub use services::{ RequestDownloadRequest, RequestDownloadResponse, RequestFileTransferRequest, RequestFileTransferResponse, RequestTransferExitRequest, RequestTransferExitResponse, ResetType, RoutineControlRequest, RoutineControlResponse, RoutineControlSubFunction, - SecurityAccessRequest, SecurityAccessResponse, SecurityAccessType, SentDataPayload, + SecurityAccessLevel, SecurityAccessRequest, SecurityAccessResponse, SecurityAccessType, + SentDataPayload, SizePayload, TesterPresentRequest, TesterPresentResponse, TransferDataRequest, TransferDataResponse, WriteDataByIdentifierRequest, WriteDataByIdentifierResponse, }; @@ -206,7 +207,13 @@ mod no_std_api_tests { fn const_construction() { // Verify const construction works at compile time const _REQ: TransferDataRequest<'static> = TransferDataRequest::new(1, &[0x01, 0x02, 0x03]); - const _SEC: SecurityAccessRequest<'static> = - SecurityAccessRequest::new(false, SecurityAccessType::RequestSeed(0x01), &[0xAA, 0xBB]); + const _SEC: SecurityAccessRequest<'static> = SecurityAccessRequest::new( + false, + SecurityAccessType::RequestSeed(match SecurityAccessLevel::new(0x01) { + Ok(level) => level, + Err(_) => panic!("0x01 is a valid security access level"), + }), + &[0xAA, 0xBB], + ); } } diff --git a/src/services/mod.rs b/src/services/mod.rs index 2812ef3..d79de5f 100644 --- a/src/services/mod.rs +++ b/src/services/mod.rs @@ -47,7 +47,9 @@ pub use routine_control::{ }; mod security_access; -pub use security_access::{SecurityAccessRequest, SecurityAccessResponse, SecurityAccessType}; +pub use security_access::{ + SecurityAccessLevel, SecurityAccessRequest, SecurityAccessResponse, SecurityAccessType, +}; mod tester_present; pub use tester_present::{TesterPresentRequest, TesterPresentResponse}; diff --git a/src/services/security_access.rs b/src/services/security_access.rs index 2348d02..11d823d 100644 --- a/src/services/security_access.rs +++ b/src/services/security_access.rs @@ -2,6 +2,37 @@ use crate::shared::SuppressablePositiveResponse; use crate::{Decode, Encode, Error, NegativeResponseCode}; +/// A `SecurityAccess` level byte, guaranteed to fit the 7-bit sub-function field +/// (`0x00..=0x7F`). +/// +/// A request fuses the suppress-positive-response flag into bit 7 of this byte at the wire +/// boundary, so a level with bit 7 already set would be ambiguous. Constraining construction +/// here makes that collision unrepresentable rather than something to catch at encode time. +#[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))] +#[cfg_attr(feature = "utoipa", derive(utoipa::ToSchema))] +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub struct SecurityAccessLevel(u8); + +impl SecurityAccessLevel { + /// Create a level, rejecting any value that would set the SPRMIB bit (`>= 0x80`). + /// + /// # Errors + /// Returns [`Error::InvalidSecurityAccessType`] if `value >= 0x80`. + pub const fn new(value: u8) -> Result { + if value > 0x7F { + Err(Error::InvalidSecurityAccessType(value)) + } else { + Ok(Self(value)) + } + } + + /// The raw level byte (always `0x00..=0x7F`). + #[must_use] + pub const fn get(self) -> u8 { + self.0 + } +} + /// Security Access Type allows for multiple different security challenges within an ECU. /// /// The Security Access Type is used to determine both the sub function, @@ -23,9 +54,9 @@ pub enum SecurityAccessType { #[non_exhaustive] ISOSAEReserved(u8), /// `RequestSeed` with the level of security defined by the vehicle manufacturer - RequestSeed(u8), + RequestSeed(SecurityAccessLevel), /// `SendKey` with the level of security defined by the vehicle manufacturer - SendKey(u8), + SendKey(SecurityAccessLevel), /// `RequestSeed` with different levels of security defined for end of life /// activation of on-board pyrotechnic devices ISO26021_2Values, @@ -44,8 +75,8 @@ impl From for u8 { fn from(value: SecurityAccessType) -> Self { match value { SecurityAccessType::ISOSAEReserved(val) => val, - SecurityAccessType::RequestSeed(val) => val, - SecurityAccessType::SendKey(val) => val, + SecurityAccessType::RequestSeed(level) => level.get(), + SecurityAccessType::SendKey(level) => level.get(), SecurityAccessType::ISO26021_2Values => 0x5F, SecurityAccessType::ISO26021_2SendKeyValues => 0x60, SecurityAccessType::SystemSupplierSpecific(val) => val, @@ -61,10 +92,11 @@ impl TryFrom for SecurityAccessType { // Security requests alternate, with odd numbers being seed requests, // and even numbers being send key requests 0x01..=0x42 => { + // `value` is 0x01..=0x42 here, so it always fits `SecurityAccessLevel`. if value % 2 == 1 { - Ok(Self::RequestSeed(value)) + Ok(Self::RequestSeed(SecurityAccessLevel(value))) } else { - Ok(Self::SendKey(value)) + Ok(Self::SendKey(SecurityAccessLevel(value))) } } 0x5F => Ok(Self::ISO26021_2Values), @@ -99,13 +131,13 @@ mod security_access_type_tests { for value in &REQUEST_SEED_VALUES { assert_eq!( SecurityAccessType::try_from(*value).unwrap(), - SecurityAccessType::RequestSeed(*value) + SecurityAccessType::RequestSeed(SecurityAccessLevel(*value)) ); } for value in &SEND_KEY_VALUES { assert_eq!( SecurityAccessType::try_from(*value).unwrap(), - SecurityAccessType::SendKey(*value) + SecurityAccessType::SendKey(SecurityAccessLevel(*value)) ); } for i in 0x43..=0x5E { @@ -140,6 +172,20 @@ mod security_access_type_tests { } } + #[test] + fn security_access_level_rejects_sprmib_colliding_values() { + // 0x00..=0x7F are constructible; 0x80..=0xFF (SPRMIB bit set) are rejected. + for value in 0x00..=0x7F { + assert_eq!(SecurityAccessLevel::new(value).unwrap().get(), value); + } + for value in 0x80..=0xFF { + assert!(matches!( + SecurityAccessLevel::new(value), + Err(Error::InvalidSecurityAccessType(v)) if v == value + )); + } + } + #[test] fn security_access_type_round_trip_all_values() { for i in 0..=u8::MAX { @@ -319,7 +365,10 @@ mod request { ]; let (req, _) = ::decode(&bytes).unwrap(); - assert_eq!(req.access_type, SecurityAccessType::RequestSeed(0x01)); + assert_eq!( + req.access_type, + SecurityAccessType::RequestSeed(SecurityAccessLevel(0x01)) + ); assert_eq!(req.request_data, &[0x00, 0x01, 0x02, 0x03, 0x04]); let mut buf = Vec::new(); @@ -346,7 +395,10 @@ mod response { ]; let (resp, _) = ::decode(&bytes).unwrap(); - assert_eq!(resp.access_type, SecurityAccessType::SendKey(0x02)); + assert_eq!( + resp.access_type, + SecurityAccessType::SendKey(SecurityAccessLevel(0x02)) + ); assert_eq!(resp.security_seed, &[0x00, 0x01, 0x02, 0x03, 0x04]); let mut buf = Vec::new(); diff --git a/src/services/tester_present.rs b/src/services/tester_present.rs index 04d77dc..5049c45 100644 --- a/src/services/tester_present.rs +++ b/src/services/tester_present.rs @@ -103,9 +103,9 @@ impl<'a> Decode<'a> for TesterPresentRequest { if buf.is_empty() { return Err(Error::InsufficientData(1)); } - // Validate the sub-function byte (rejects values outside 0x00..=0x7F once SPRMIB is - // stripped); only the suppression flag is retained, since the sub-function is always - // the zero sub-function on re-encode. + // Split out the SPRMIB flag. Once SPRMIB is stripped the low 7 bits are always a + // valid zero sub-function, so this never rejects; the sub-function value itself is + // discarded and normalized to the zero sub-function on re-encode. let sub_function = SuppressablePositiveResponse::::try_from(buf[0])?; Ok(( Self { From 56d7f6b7c9e60caf1bc03315a7db361edce6a76e Mon Sep 17 00:00:00 2001 From: Zach Date: Wed, 1 Jul 2026 08:19:27 -0400 Subject: [PATCH 6/6] cargo fmt --- src/lib.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index d34312e..bfb11ab 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -48,8 +48,7 @@ pub use services::{ RequestFileTransferResponse, RequestTransferExitRequest, RequestTransferExitResponse, ResetType, RoutineControlRequest, RoutineControlResponse, RoutineControlSubFunction, SecurityAccessLevel, SecurityAccessRequest, SecurityAccessResponse, SecurityAccessType, - SentDataPayload, - SizePayload, TesterPresentRequest, TesterPresentResponse, TransferDataRequest, + SentDataPayload, SizePayload, TesterPresentRequest, TesterPresentResponse, TransferDataRequest, TransferDataResponse, WriteDataByIdentifierRequest, WriteDataByIdentifierResponse, };