Skip to content

Flatten suppressable request structs into public data bags#44

Merged
zheylmun merged 6 commits into
mainfrom
refactor/flatten-suppressable-requests
Jul 1, 2026
Merged

Flatten suppressable request structs into public data bags#44
zheylmun merged 6 commits into
mainfrom
refactor/flatten-suppressable-requests

Conversation

@zheylmun

@zheylmun zheylmun commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Summary

Follow-up to the no_std landing. The ported request/response types were split between two visibility styles; this applies our field-visibility convention — encapsulate only invariant-bearing structs, otherwise a public data bag — consistently across the services, and closes a SPRMIB-collision footgun at construction time.

What changed

Flattened to public data bags

Private SuppressablePositiveResponse<T> field + getters → public suppress_positive_response: bool + value field(s), getters removed:
EcuResetRequest, DiagnosticSessionControlRequest, ControlDTCSettingsRequest, RoutineControlRequest, SecurityAccessRequest, TesterPresentRequest, plus the non-invariant responses RoutineControlResponse and RequestTransferExit{Request,Response}.

SPRMIB packing moved to the wire boundary

SuppressablePositiveResponse<T> stays pub(crate) and is now constructed transiently inside encode/decode only. The generic wrapper never appears in the public API.

Reserved sub-function variants locked against SPRMIB collision

The open-ended (u8) variants in reserved ranges (ResetType, DiagnosticSessionType, CommunicationControlType, and the reserved arms of SecurityAccessType) are marked #[non_exhaustive], so callers must build them via the range-checked TryFrom<u8>. A value ≥ 0x80 that would collide with the SPRMIB bit is now unconstructable, rather than something to catch at encode time.

SecurityAccessLevel newtype

SecurityAccessType::RequestSeed/SendKey are user-facing (kept directly constructible), so locking them wasn't an option — instead they now carry a SecurityAccessLevel newtype (private u8 + validated new rejecting ≥ 0x80). This was the last SPRMIB-exposed enum with an unlocked user-facing variant; the colliding value is now unrepresentable. Const construction uses a const-match unwrap since new is fallible.

Deliberate decisions

  • CommunicationControlRequest stays encapsulated. node_id must be Some iff control_type is an enhanced-address variant — a genuine cross-field invariant — so it keeps private fields + fallible constructors. Its reserved CommunicationControlType variants are still locked.
  • RequestDownloadRequest stays encapsulated. The address/length format identifier must track the memory address/size widths (cross-field invariant) + fallible new.
  • ReadDataByIdentifierRequest stays encapsulated. Its field is a private Dids representation (native &[u16] vs borrowed wire bytes); exposing it would leak that abstraction, so the dids() iterator remains the accessor.
  • TesterPresentRequest is now { suppress_positive_response }. Its sub-function enum is private, so a non-zero (reserved) sub-function byte still decodes but normalizes to the zero sub-function on re-encode (previously preserved). Per ISO 14229 the sub-function is always 0x00, so this is a deliberate, minor behavior change.

Review feedback addressed

  • SecurityAccess SPRMIB collision (RequestSeed/SendKey could hold ≥ 0x80): fixed via the SecurityAccessLevel newtype above. Audited the rest of the baseline — of the six SuppressablePositiveResponse-wrapped enums this was the only remaining unlocked case; other raw-int enum variants (DIDs, NRCs, DTC numbers) span the full valid integer range and steal no bit, so carry no comparable invariant.
  • TesterPresent decode comment reworded — after SPRMIB masking the low 7 bits are always a valid zero sub-function, so the parse never rejects.
  • Intra-doc-link warnings on Type::try_from links: verified not an issue — cargo doc --all-features with -D rustdoc::broken_intra_doc_links builds clean; the links resolve to the unambiguous TryFrom<u8> impl.

Test plan

  • cargo test — 143 + 3 + doctests pass
  • cargo build --all-features and cargo build --no-default-features (no_std) both clean
  • cargo clippy --all-targets --all-features clean
  • cargo doc --all-features clean with -D rustdoc::broken_intra_doc_links

🤖 Generated with Claude Code

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<u8>, 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) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings June 30, 2026 00:18

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR standardizes the public API for “suppressable” UDS request types by flattening previously-encapsulated SPRMIB wrappers into public request fields, while keeping SPRMIB bit packing/unpacking at the encode/decode boundary. It also tightens construction of reserved sub-function values to prevent callers from constructing bytes that would collide with the SPRMIB bit.

Changes:

  • Flattened several request structs into public “data bag” fields (suppress_positive_response + sub-function/value fields) and removed now-redundant getters.
  • Moved SuppressablePositiveResponse<T> construction to encode/decode only (wire-boundary packing).
  • Marked reserved-range enum variants as #[non_exhaustive] to force range-checked construction via TryFrom<u8>.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/services/tester_present.rs Flattens request into a single public suppression flag and normalizes reserved sub-function bytes on re-encode.
src/services/security_access.rs Flattens request fields and shifts SPRMIB packing to encode/decode; adds #[non_exhaustive] on reserved variants.
src/services/routine_control.rs Flattens request fields and shifts SPRMIB packing to encode/decode.
src/services/ecu_reset.rs Flattens request fields, shifts SPRMIB packing to encode/decode, and locks reserved variants.
src/services/diagnostic_session_control.rs Flattens request fields, shifts SPRMIB packing to encode/decode, and locks reserved variants.
src/services/control_dtc_settings.rs Flattens request fields and shifts SPRMIB packing to encode/decode.
src/services/communication_control.rs Locks reserved control-type variants via #[non_exhaustive] (keeps request encapsulated).
src/request.rs Updates suppression detection to read new public fields for flattened request variants.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/services/security_access.rs
Comment thread src/services/tester_present.rs Outdated
zheylmun and others added 3 commits June 29, 2026 20:35
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) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings June 30, 2026 18:16

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 9 out of 10 changed files in this pull request and generated 11 comments.

Comment thread src/services/security_access.rs
Comment thread src/services/security_access.rs
Comment thread src/services/diagnostic_session_control.rs
Comment thread src/services/diagnostic_session_control.rs
Comment thread src/services/diagnostic_session_control.rs
Comment thread src/services/ecu_reset.rs
Comment thread src/services/ecu_reset.rs
Comment thread src/services/communication_control.rs
Comment thread src/services/communication_control.rs
Comment thread src/services/communication_control.rs
zheylmun and others added 2 commits June 30, 2026 21:16
…mment

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) <noreply@anthropic.com>
@zheylmun zheylmun enabled auto-merge July 1, 2026 13:07
@zheylmun zheylmun merged commit 91759f1 into main Jul 1, 2026
16 checks passed
@zheylmun zheylmun deleted the refactor/flatten-suppressable-requests branch July 1, 2026 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants