chore(standardization): codec migration - #153
Open
zheylmun wants to merge 18 commits into
Open
Conversation
Adds automotive-wire-codec = "0.3" (crates.io) and updates MIGRATION_PLAN.md from the 0.2-era draft to the shipped 0.3.0 API: encoded_size now Result, encode_to_slice is a codec-provided default (F5 InsufficientBuffer landed), DecodeIter::WIRE_SIZE for fixed-stride entries. Records the cross-crate comparison resolutions and supersedes the do-not-execute gate.
Add tests/wire_golden.rs with hand-derived, byte-exact snapshots of the current SOME/IP + SD wire encoding (Header, Message<RawPayload>, sd::Header for every EntryType, one test per OptionType, and three sd_codec datagram builders). These are the safety net for the upcoming automotive-wire-codec migration: later phases must keep producing these exact bytes. Test-only change; no src/ behavior modified.
Replace the crate's generic UnexpectedEof with Incomplete/TrailingBytes/
InsufficientBuffer variants sourced from automotive-wire-codec 0.3, so
truncation sites report needed/available byte counts instead of a bare
"eof". sd::Error::IncorrectOptionsSize becomes a struct variant carrying
both needed and available for the same reason. Also fixes
byte_order::read_bytes, which previously mismapped a truncated read to a
generic Error::Io(Other); it now yields Incomplete { needed: buf.len(),
available: 0 } since a streaming reader can't report how far it got.
No trait swaps; Decode/Encode impls land in a later phase.
Hard cutover from the crate-local `WireFormat` trait to `automotive_wire_codec::Encode` (0.3.0). `WireFormat` is deleted; all encode impls and call sites (client, server, sd_codec, tests) migrate atomically. On-wire bytes are byte-identical (Phase 0 golden tests pass unchanged). - protocol::Error gains `From<embedded_io::ErrorKind>` and `From<EncodeToSliceError<Error>>` so leaf write helpers and encode_to_slice lift through `?`. - New `EncodeExt` (crate-local) carries `encode_to_vec`; `encode_to_slice` now comes from the codec trait. `Encode`/`EncodeToSliceError` are re-exported from the crate root. - Encode impls: Header, Message<P>, ServiceEntry, EventGroupEntry, Entry, sd::Header, Options (new), VecSdHeader, HeaplessSdHeader, TestSdHeader. `required_size` becomes closed-form `encoded_size() -> Result<usize>`. - Fix long-standing entry size-count bug under golden protection: Entry=16, ServiceEntry/EventGroupEntry body=15 (were 17/16/16); wire output unchanged. - Send/Sync: the deleted supertrait's guarantee is restated on PayloadWireFormat::SdHeader (`+ Send + Sync`) rather than threaded as where-clauses, since SdHeader flows through the client's Send-bounded channel types pervasively, not only at the one spawn site. - Add size-exactness unit tests (encoded_size == CountingSink count; too-small slice yields InsufficientBuffer, never panics).
Phase 2 of the codec migration removed the Options::write inherent method in favor of the codec Encode impl, but two callers in the server-tokio test module were missed, breaking that feature's test build.
Give HeaderView and MessageView a codec `Decode<'a>` impl as the single source of decode logic; the old inherent `parse` fns become thin wrappers. - HeaderView::parse delegates to Decode::decode (unchanged behavior). - MessageView::decode returns (message, rest) so trailing bytes past the SOME/IP length field are the next message rather than silently discarded; MessageView::parse keeps its discard-trailing behavior via decode().0, and decode_exact provides strict single-message decode. - Re-export Decode/DecodeIter/DecodeIterator from the crate root.
Give the SD wire-element views a codec `Decode<'a>` + `DecodeIter<'a>` impl as the single decode source for each element: - EntryView: fixed-stride slice with `WIRE_SIZE = Some(ENTRY_SIZE)`, enabling DecodeIterator::remaining_len. - OptionView: variable stride from the length field (default WIRE_SIZE). Both defer content validation (entry-type byte, option type/length/ transport-protocol) to the existing lazy accessors, keeping decode a pure zero-copy slice. decode_next follows the clean-end convention (empty -> Ok(None), partial-after-good-start -> Err).
Add `SdBody<'a>` with a codec `Decode<'a>` impl that performs only the O(1) flag decode + section slicing (buffer minimum, entries_size multiple-of-16, section bounds). It does NOT walk entry-type bytes or validate option contents; those are exposed lazily via entries()/options() DecodeIterators or handled by the L2 validation pass. SdHeaderView::parse now delegates its slicing to SdBody::decode as the single decode source, then runs only the eager L2 validation walks over the already-sliced sections. Those infallible-iterator walks stay here until Phase 4 re-founds them on the lazy DecodeIter path.
MessageView::decode computed header.payload_size() as length - 8 with no guard, so a hostile/truncated datagram with length < 8 would panic on arithmetic underflow under overflow-checks (or wrap to a huge usize otherwise). Reject length < 8 up front with a new protocol::Error::InvalidLength variant, and harden payload_size() in both Header and HeaderView to use saturating_sub(8) as defense in depth.
Re-found SdHeaderView's validated-view semantics on top of the Phase 3 lazy L1 layer (candidate "c"): construction runs one eager validating walk by draining the L1 entry/option DecodeIterators (surfacing the first Err via `?`) and caches the entry/option counts; the infallible accessors re-slice the already-validated buffers without re-running entry-type / option validation. - Add cached entry_count/option_count fields + option_count() accessor; entry_count() now returns the cached count. EntryIter keeps its free ExactSizeIterator; options expose the cached count instead (no fixed stride). - Add OptionView::validate() so the L2 walk can validate options via L1. - Harden SdBody::decode section-bound arithmetic with checked_add against 32-bit usize overflow on hostile entries_size/options_size (Incomplete on overflow); defense-in-depth for no_std targets. - Truncated sections now surface the L1 Incomplete (needed/available unchanged) instead of the hand-rolled IncorrectOptionsSize; tests updated. Golden bytes round-trip unchanged. Call sites already on intended layers: std RX uses sd_header() (L2), bare-metal RX uses header-only parse_someip_datagram (L1).
Drop PayloadWireFormat's inherent `required_size`/`encode` methods in favor of an `automotive_wire_codec::Encode<Error = protocol::Error>` supertrait. `from_payload_bytes` stays (SOME/IP payloads are not self-identifying, so the MessageId must come from the caller and Encode alone cannot reconstruct them). - RawPayload, HeaplessPayload, TestPayload now `impl Encode` (encoded_size + encode) instead of inherent methods. - Message<P>::encoded_size uses `payload.encoded_size()?`; the Error bound on the supertrait lets `?` convert cleanly. - required_size() call sites become encoded_size()?/.unwrap().
sd_codec changes over the codec's Encode/Decode traits: - encode_sd_datagram flips to header-first, body-second: compute sd_header.encoded_size() (exact, no write), size-check the buffer once, encode the SOME/IP header into buf[..16] then the SD body into buf[16..] in one linear forward pass. No backfill. Golden datagram bytes (--features server) unchanged. - BuildError::BufferTooSmall now carries automotive_wire_codec:: InsufficientBuffer (needed/available), matching the rest of the crate. Adds Display/Error impls and From<EncodeToSliceError<protocol::Error>>. - parse_someip_datagram / parse_someip_sd_datagram return Result instead of Option: Incomplete (need more bytes), UnsupportedMessageID (well- formed but not SD), and Sd/validation errors (malformed) are now distinguishable. bare_metal_tasks caller updated to the Result shape.
…al misfits
Phase 6 of the automotive-wire-codec 0.3.0 migration: E2E stays on its own
protect/check API (in-place mutation + status-not-error results don't fit
Encode/Decode), but its error shape is now bridgeable into protocol::Error.
- Add `impl From<e2e::Error> for protocol::Error`, mapping
`BufferTooSmall { needed, actual }` to `Error::InsufficientBuffer`
(the semantically correct counterpart — output-buffer-too-small during a
write, not `Incomplete`'s decode-direction "ran out of input to read").
- Document Profile 5's intentional little-endian DataID/CRC framing in
`e2e/crc.rs` (and the LE read/write call sites in the protector/checker) as
spec-correct and recorded as codec feedback (F2), not a bug to "fix" by
reaching for the codec's BE-only leaf helpers.
- Document the post-hoc SOME/IP length-field backfill in
`event_publisher::publish_event` as the reason E2E cannot be a single-pass
`Encode` impl, per the codec README's own two-phase-API carve-out for
size-changing post-hoc transforms.
- Add a "why no Encode/Decode" section to the e2e module docs.
- Add a unit test for the new From<e2e::Error> bridge.
No on-wire bytes, CRC framing, or E2E protect/check behavior changed —
verified via diff (comment-only changes to crc.rs/e2e_protector.rs/
e2e_checker.rs) and the full golden/nextest/clippy/fmt/doc gate suite.
…9.0 CHANGELOG
Final phase of the simple_someip -> automotive-wire-codec 0.3.0 migration.
Pure hygiene, no behavior changes; all golden-bytes tests stay green.
- Sweep the last `WireFormat` prose stragglers in comments (traits.rs,
protocol/message.rs, protocol/header.rs) to reference the current
`Encode`/`EncodeExt` API. `grep -rn 'WireFormat' src/ | grep -v
PayloadWireFormat` is now empty.
- Update README's module table: `traits` now describes `PayloadWireFormat`
(built on `automotive_wire_codec::Encode`) instead of the removed
`WireFormat` trait.
- Tighten `protocol::sd::header::parse_rejects_trailing_partial_option` from
a bare `.is_err()` to assert the exact `Incomplete { needed: 16,
available: 12 }` variant, matching its sibling truncation tests.
- Add `sd_codec::parse_sd_datagram_structurally_invalid_entry_is_sd_error`,
pinning the `protocol::Error::Sd(sd::Error::InvalidEntryType(_))` branch
via a length-consistent but structurally-invalid SD entry (the existing
truncated-SD test short-circuits as `Incomplete` before reaching
`SdHeaderView::parse`'s entry-type validation).
- Add the 0.9.0 breaking-release CHANGELOG entry describing the WireFormat
-> Encode/Decode migration, the reworked `protocol::Error`, the
Result-returning `sd_codec` parsers, and the `PayloadWireFormat` Encode
supertrait. Descriptive only — release-plz owns the actual version bump.
- Verified `simple-someip-embassy-net` builds and its full test suite
(`loopback.rs`, doctests) passes unchanged against the migrated crate;
no version-pin bump needed since the workspace crate is still 0.8.0.
…c comment Final cleanup from the whole-branch codec-migration review (READY TO MERGE, two Minor findings): - Remove the now-dead `ReadBytesExt` trait and its blanket `embedded_io::Read` impl from `src/protocol/byte_order.rs`. Decode is fully slice-based now (`take`/`ensure_len`); grepping the workspace (including `simple-someip-embassy-net/` and `tests/`) turned up zero callers outside this module's own unit tests, which are removed alongside it. `WriteBytesExt` is untouched. Since this is a breaking 0.9.0 release, it's the right time to drop the dead public surface rather than ship it. Noted the removal in CHANGELOG.md. - Reword the stale doc comment atop `tests/wire_golden.rs` describing the historical `Entry::required_size()` off-by-one bug in past tense now that it was fixed in Phase 2 (the method no longer exists).
main's device-IP-keying test (handle_discovery_datagram_keys_offers_by_device_ip, added in the source-keyed registry work) used the pre-migration WireFormat/ required_size API; the rebase kept main's version of those hunks. Re-apply the codec-migration change here: use Encode + encoded_size().
Post-rebase sweep of the warnings this branch added on top of main:
- Drop three now-unused test-only imports left behind by the codec
migration (`alloc::sync::Arc` in server/event_publisher.rs,
`sd::{Entry, Flags, ServiceEntry}` and `std::vec::Vec` in server/mod.rs).
- `range_plus_one` in the MessageView trailing-bytes tests: `..n + 1` → `..=n`.
- `items_after_statements` in the sd_codec invalid-entry-type test: the
offset is a local, not a mid-function `const`.
Remaining clippy warnings under `--features server,client` are all
pre-existing on the rebase base.
Two independent breakages, both surfaced only by feature/toolchain
combinations that `cargo clippy --features server,client` does not cover.
**Windows / host lane (`$HOST_FEATURES`) — E0425 `Arc` not found.**
The post-rebase warning sweep removed three test-only imports on the
strength of a `--features server,client` clippy run. They are live under
`server-tokio`, which gates the `#[cfg(all(test, feature = "server-tokio"))]`
test modules that use them. Restored, but scoped into those test modules
rather than back at file level (`std::sync::{Arc, Mutex}` in
`event_publisher::tests`; `sd::{Entry, Flags, ServiceEntry}` and
`std::vec::Vec` in `server::tests`), so they are neither unused in the
narrower configs nor missing in the wider ones.
**Both doc lanes — five `-D warnings` rustdoc errors.**
These came in with the codec migration; no local lane runs rustdoc with
`RUSTDOCFLAGS=-D warnings`, so they were invisible until CI.
- `e2e/mod.rs`: `[`E2ECheckResult`]` → explicit `crate::e2e::` path. The
`e2e` module carries both an outer `///` at its `lib.rs` declaration and
inner `//!` docs; the merged docs resolve in `lib.rs`'s scope, where
`E2ECheckStatus` is re-exported (hence resolving) and `E2ECheckResult`
is not.
- `protocol/sd/header.rs` ×2: bare `[`parse`]` inside an inherent impl —
intra-doc links resolve at module scope, not impl scope. Qualified to
`[`parse`](SdHeaderView::parse)`.
- `traits.rs`: `[`EncodeExt::encode_to_vec`]` is `#[cfg(feature = "std")]`,
so it does not exist in the `--features client` doc build. Links the
trait and leaves the method as plain code, matching the sentence's own
"under `std`" caveat.
- `protocol/sd/entry.rs`: `ENTRY_SIZE` is `pub` inside the private `entry`
module and is not among `sd`'s re-exports, so the link is private.
De-linked to match the plain-backtick style used for it twelve lines
down.
Verified against every lane in .github/workflows/ci.yml that this machine
can run: fmt, all five clippy invocations, all three doc lanes plus the
nightly bare-metal-runtime doc, the partial-feature build matrix, the
thumbv7em and build-std core gates, no_alloc_witness, the embassy-net
adapter, the SD TX conformance test, and the host/alloc test lanes (whose
remaining failures are identical to origin/main's).
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## main #153 +/- ##
==========================================
+ Coverage 81.30% 81.87% +0.57%
==========================================
Files 47 48 +1
Lines 15514 16014 +500
==========================================
+ Hits 12613 13112 +499
- Misses 2901 2902 +1
|
zheylmun
enabled auto-merge
August 24, 2026 17:07
JustinKovacich
requested changes
Aug 24, 2026
JustinKovacich
left a comment
JustinKovacich
left a comment
Contributor
There was a problem hiding this comment.
Reviewed the codec migration. The migration itself is well-executed: on-wire bytes are held constant, tests/wire_golden.rs pins them, the eager-validation invariant in SdHeaderView::parse is preserved and now documented, and the E2E diff is comments-only (correctly leaving Profile 5's LE framing alone). a917c80 cleared the two CI breakages I had queued up, so this is what's left.
Requesting changes on the first one below — it's a reachable panic in newly-added public API. The other two are robustness regressions I'd want addressed but won't die on.
Two smaller notes, no inline anchor:
sd_codec.rs:226/:239—InsufficientBuffer { needed: requests.len(), available: N }puts element counts into fields the codec documents as byte counts. The path is unreachable (take(N)bounds the loop), but the error would be actively misleading if it ever fired.- CHANGELOG gaps — two breaking changes aren't listed:
Options::write(apubinherent method) became the private trait methodEncode::encode; andServiceEntry/EventGroupEntry::required_size()used to return16whileencodeactually wrote15bytes, so the newencoded_size() == 15is a genuine latent-bug fix. That second one deserves a line — anyone who sized a buffer offrequired_size()gets a different number now.
| /// the fixed option header remains, or if the declared wire size exceeds | ||
| /// the remaining bytes. | ||
| fn decode(buf: &'a [u8]) -> Result<(Self, &'a [u8]), Self::Error> { | ||
| ensure_len(buf, OPTION_HEADER_SIZE)?; |
Contributor
There was a problem hiding this comment.
Blocking — decode admits under-length options, and the accessors then index out of bounds.
ensure_len requires OPTION_HEADER_SIZE (4), but take then consumes length + OPTION_LENGTH_SIZE_DELTA (3). A declared length of 0 or 1 therefore yields a view shorter than the header this function just required.
The doc above says validation is "deferred to the accessors (option_type / as_ipv4 / to_owned)" — but those only validate type/protocol byte values; they index the backing slice unconditionally. SdBody and Decode are both publicly re-exported, so the documented lazy path panics on hostile bytes.
Confirmed against a917c80 with option bytes 00 02 04 00 00 (length=2 -> 5-byte view, type 0x04 = IPv4Endpoint, which needs 12), reached via SdBody::decode -> body.options() -> to_owned():
thread 'repro_short_ipv4_option' panicked at src/protocol/sd/options.rs:396:13:
index out of bounds: the len is 5 but the index is 5
3: OptionView::as_ipv4 at src/protocol/sd/options.rs:396
4: OptionView::to_owned at src/protocol/sd/options.rs:488
A zero-length Configuration option (00 00 01 00) panics the same way in configuration_bytes at :432.
Reachability: the crate's own production paths all go through SdHeaderView::parse, which still runs the eager validate() walk and rejects these — so this is not currently exploitable via parse_someip_sd_datagram. But it is newly-added public API that the docs point users at.
Fix: enforce the per-type minimum length (what validate_option already knows) either inside decode or at the head of each accessor.
| // protocol-level I/O error from inside the socket loop. | ||
| let required = message.required_size(); | ||
| let required = message | ||
| .encoded_size() |
Contributor
There was a problem hiding this comment.
Robustness regression — .expect() on a now-fallible call inside the client socket loop. (Same at :735.)
Message::encoded_size is header.encoded_size()? + payload.encoded_size()?, and payload is a user-supplied PayloadWireFormat impl. The expect message holds for the two in-tree payloads, but a downstream impl that returns Err panics the client's async socket task. That was impossible before this PR — required_size() was infallible.
Note event_publisher.rs:196 handles the identical situation with ?:
let required_size = message.encoded_size()?;so the two sides of the crate now disagree on how to treat the same failure. The server behaviour looks like the right one.
| // Every concrete `SdHeader` type overrides `encoded_size` with a | ||
| // closed-form `Ok(n)` that cannot fail; `unwrap_or(0)` avoids a | ||
| // `Debug` bound on the (generic) associated error type. | ||
| let sd_header_size = sd_header.encoded_size().unwrap_or(0); |
Contributor
There was a problem hiding this comment.
Robustness regression — unwrap_or(0) silently corrupts the SOME/IP length field.
On Err this builds Header::new_sd(request_id, 0) — a header declaring length = 8 — and Message::encode then writes the full payload after it. Receivers truncate at the declared length, so the failure mode is silent wire corruption rather than an error.
The comment justifies it as "every concrete SdHeader type overrides encoded_size with a closed-form Ok(n)". That is true in-tree, but it is not enforced by the bound —
type SdHeader: automotive_wire_codec::Encode + Clone + core::fmt::Debug + Eq + Send + Sync;has no Error = protocol::Error, and PayloadWireFormat is public and implementable downstream. It also cuts against this PR's own CHANGELOG, which gives the reason encoded_size is fallible as "some encodings, e.g. SD configuration strings, can exceed representable bounds".
Either bound the associated type and propagate the error, or pick a fallback that cannot produce a valid-looking short header.
|
|
||
| [dependencies] | ||
| simple-someip = { path = "..", version = "0.12", default-features = false, features = [ | ||
| simple-someip = { path = "..", version = "0.13", default-features = false, features = [ |
Contributor
There was a problem hiding this comment.
did we actually publish .12? Do we just want to keep this with one step up?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Migrate onto shared automotive codec with rest of protocol crates