PR 1: #124 follow-ups — parsed union NonSdRequestCallback + doc trues-up (#125 stack)#129
Closed
JustinKovacich wants to merge 13 commits into
Closed
PR 1: #124 follow-ups — parsed union NonSdRequestCallback + doc trues-up (#125 stack)#129JustinKovacich wants to merge 13 commits into
JustinKovacich wants to merge 13 commits into
Conversation
ctx:usize callback shape, eager-Ready documentation, announce_only rationale, live-witness negative tests, UDP_BUFFER_SIZE doc fix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Stored as (callback, ctx) on ServerDeps/ServerHandles/Server and passed back verbatim from recv_loop. usize over *mut c_void keeps Server: Send (run's declared + Send bound survives) with no unsafe in this crate; the pointer cast lives in the consumer's callback. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Removes the shared-queue nondeterminism (from_unicast depended on select_biased bias order) and enables deterministic SD-socket injection for the negative observer tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The old None-case test could not fail (its witness was never registered). Replace with: registered observer must not fire for SD messages on the unicast socket nor for non-SD datagrams on the SD socket; the None case shrinks to its real guarantee (no panic). Refutation-checked by inverting the from_unicast branch. Also folds in review riders: stale section comment, inline pipe construction, factory doc restructure with the passive-path caveat. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…bscribe Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Also aligns the Server.non_sd_observer field doc with the (callback, ctx) pair shape (review rider). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The 'announcement builders / SubscribeAck/Nack still use heap Vec — known gap' claim went stale when the phase-21 per-event-allocation cleanup (7c58649) converted those paths to [0u8; UDP_BUFFER_SIZE] stack buffers. Every production outbound path is now capped by the constant. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
loopback.rs never compiled after the field landed (the main-crate CI matrix doesn't build this crate's tests). None preserves behavior. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Cross-branch reconciliation (2026-06-11): feat/embassy-mem-channel-cap
independently reshaped the callback to a library-parsed
(service_id, method_id, payload, e2e_status) contract. The decided
shape is the union of both branches:
fn(ctx: usize, source: SocketAddrV4, service_id: u16,
method_id: u16, payload: &[u8], e2e_status: u8)
recv_loop parses the SOME/IP header so consumers receive decoded
fields — parse and (eventually) E2E stay in the audited crate per
MISRA/ASIL rather than being hand-rolled in N C consumers.
e2e_status is 0 (unchecked) on the server path today; source is
future-proofing (unused by halo and dft).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR is the second in the #125 / phase-22 stack and primarily updates the server-side non-SD observer contract to be library-parsed and ctx-carrying, while tightening related tests and truing-up documentation (including SD buffer sizing claims).
Changes:
- Breaking:
NonSdRequestCallbackis now(callback, ctx)-registered and invoked with parsed SOME/IP fields (service_id,method_id,payload,e2e_status) plussource. - Test harness improvements: per-socket mock pipes make
from_unicastdeterministic, and negative observer tests become “live-witness” (able to actually fail on regressions). - Documentation updates: subscription timing caveat for
Ready-based impls,announce_only_futurerationale,UDP_BUFFER_SIZEdoc correction, plus changelog migration notes.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/bare_metal_server.rs | Deterministic mock transport routing + updated positive/negative observer tests for new parsed callback contract |
| src/server/subscription_manager.rs | Documents eager-at-construction side effects for synchronous Ready-based subscription futures |
| src/server/runtime.rs | Threads (callback, ctx) into recv_loop and invokes observer with parsed header fields + payload |
| src/server/mod.rs | Updates public API types/fields to use parsed, ctx-carrying NonSdRequestCallback and documents the new contract |
| src/lib.rs | Fixes UDP_BUFFER_SIZE rustdoc to reflect stack-buffered outbound SD serialization paths |
| simple-someip-embassy-net/tests/loopback.rs | Fixes compilation by providing the new non_sd_observer field in ServerDeps initializers |
| docs/simple_someip/plans/2026-06-10-pr1-124-followups.md | Adds implementation plan document for the PR’s follow-up work (now part of repo docs) |
| docs/simple_someip/plans/2026-06-10-pr1-124-followups-design.md | Adds design doc describing the reconciled callback contract and rationale |
| CHANGELOG.md | Records the breaking callback signature change and migration guidance |
Comments suppressed due to low confidence (1)
tests/bare_metal_server.rs:454
- The
build_method_requesthelper’s doc comment says it builds a request with “no payload”, but the function now accepts and appends apayloadslice (and tests use a non-empty payload). This can mislead future edits to the test vector.
/// Build a minimal SOME/IP method-request datagram (16-byte header,
/// no payload, message_type = Request). The exact byte layout matches
/// the on-wire SOME/IP header format documented in
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+289
to
+293
| /// fire-and-forget calls to offered services). `None` reproduces the | ||
| /// historical "non-SD ignored" behavior. The callback receives the | ||
| /// opaque `ctx` word back verbatim, plus the full raw datagram bytes | ||
| /// and the source `SocketAddrV4`; the consumer is responsible for | ||
| /// re-parsing the SOME/IP header and any E2E check. |
|
|
||
| **Goal:** Land the four #124 review follow-ups — ctx-carrying `NonSdRequestCallback` (breaking), eager-`Ready` timing docs, `announce_only_future` rationale, live-witness negative tests — plus the stale `UDP_BUFFER_SIZE` rustdoc fix. | ||
|
|
||
| **Architecture:** One breaking signature change (`fn(data, src)` → `fn(ctx: usize, data, src)`, stored as `Option<(NonSdRequestCallback, usize)>` on `ServerDeps`/`ServerHandles`/`Server` and threaded through `recv_loop`); a mock-transport split in `tests/bare_metal_server.rs` (per-socket pipes routed by `multicast_if_v4`, making `from_unicast` deterministic); the rest is documentation and CHANGELOG. |
Comment on lines
+155
to
+158
| `non_sd_observer: Some(my_cb)` → `non_sd_observer: Some((my_cb, 0))`, | ||
| and replace the leading `ctx: usize` parameter plus manual header parsing | ||
| with the decoded arguments `(ctx, source, service_id, method_id, payload, | ||
| e2e_status)`. Registration becomes `Some((my_cb, 0))`. |
…tion Grounded in an executed dry run (preview/embassy_union_rebase): full per-commit conflict inventory, 2-error union-adoption surface, DISPATCH_CTX design, fresh-baseline capture as PR 2's before. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This was referenced Jun 17, 2026
Contributor
Author
|
Folded into |
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
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.
Second PR of the #125 / phase-22 stack
(docs/simple_someip/plans/2026-06-10-pr1-124-followups-design.md).
Stacked on PR 0 (#127); retargets when that merges.
NonSdRequestCallbackis now a parsed, ctx-carryingcallback:
fn(ctx: usize, source: SocketAddrV4, service_id: u16, method_id: u16, payload: &[u8], e2e_status: u8),registered as a
(callback, ctx)pair. This is the reconciledunion of this stack's ctx shape and
feat/embassy-mem-channel-cap's library-parsed shape (decision2026-06-11): recv_loop parses the SOME/IP header so C consumers
never hand-roll parse/E2E (MISRA/ASIL);
ctx: usizekeepsServer: Sendwith zerounsafein-crate;e2e_statusis 0(unchecked) on the server path today. Migration in CHANGELOG.
test could not fail) — refutation-checked by inverting the routing
branch; mock transport gets per-socket pipes so
from_unicastisdeterministic.
Ready-basedsubscribe/unsubscribe;
announce_only_futuresplit-shape rationale;UDP_BUFFER_SIZEoutbound-SD claim (stack-buffered since phase 21's7c58649).
simple-someip-embassy-net/tests/loopback.rs(missingnon_sd_observerfield — never compiled by the main-crate CI matrix).Merge gates: (1) Feliciano gets the callback-signature heads-up
first so no further halo FFI builds on the old shape; (2)
feat/embassy-mem-channel-capadopts the same contract for itsDispatchFnwhen it rebases.Known follow-ups (pre-existing, not from this PR): the
src/lib.rs - transportdoctest fails under --all-features (outsideCI's gate); bare_metal_server.rs's file header overclaims its
compile-witness scope (new_with_deps is _alloc-gated).
🤖 Generated with Claude Code