Extract SdStateManager from the server mod file by JustinKovacich · Pull Request #75 · luminartech/simple_someip

JustinKovacich · 2026-04-22T20:40:51Z

Begin the ground work of supporting a fully embedded deployment of simple someip. Start by abstracting the SDStateManager out of the server proper.

This is a chained PR. Prev: None. Next: #76

codecov-commenter · 2026-04-22T20:42:16Z

Codecov Report

❌ Patch coverage is 24.07407% with 246 lines in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
src/server/sd_state.rs	24.81%	200 Missing ⚠️
src/server/mod.rs	20.68%	46 Missing ⚠️

@@            Coverage Diff             @@
##             main      #75      +/-   ##
==========================================
- Coverage   96.55%   94.24%   -2.32%     
==========================================
  Files          30       31       +1     
  Lines        7993     8242     +249     
==========================================
+ Hits         7718     7768      +50     
- Misses        275      474     +199

Files with missing lines	Coverage Δ
src/server/mod.rs	`93.18% <20.68%> (-0.02%)`	⬇️
src/server/sd_state.rs	`24.81% <24.81%> (ø)`

Copilot

Pull request overview

This PR refactors the server’s Service Discovery (SD) session tracking by extracting it from src/server/mod.rs into a dedicated SdStateManager type, making the SD session counter and multicast OfferService emission more self-contained and easier to evolve.

Changes:

Added SdStateManager (src/server/sd_state.rs) to own the SD session-id counter and send multicast OfferService announcements.
Updated Server to hold sd_state: Arc<SdStateManager> and route SD session-id generation / announcements through it.
Moved wraparound tests for SD session-id behavior into the new module.

Reviewed changes

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

File	Description
`src/server/sd_state.rs`	New module implementing `SdStateManager` (session-id counter + multicast `OfferService` sender) with unit tests.
`src/server/mod.rs`	Wires the server to use `SdStateManager` and removes the inlined SD session-id and multicast offer implementation.

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

Copilot

Pull request overview

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

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

Per AUTOSAR SOME/IP-SD, the reboot bit on emitted SD messages must flip from RecentlyRebooted to Continuous once the session counter wraps past 0xFFFF. SdStateManager already owns the counter, so it also tracks wrap (new has_wrapped: AtomicBool latched exactly on the 0xFFFF -> 0x0001 transition) and exposes reboot_flag() as the single source of truth. The four SD emission paths — SdStateManager::send_offer_service, Server::send_unicast_offer, send_subscribe_ack_from_view, and send_subscribe_nack_from_view — all now consume the tracked flag instead of hardcoding Flags::new(true, true). Coverage: four new non-ignored unit tests cover the state machine (fresh, sub-wrap, exactly-on-wrap, monotonic-after-wrap); assert_offer_matches takes an expected RebootFlag; the existing wrap multicast test now asserts first-emit=RecentlyRebooted and second-emit=Continuous across the boundary. Responds to PR #75 feedback on src/server/sd_state.rs:90. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Copilot

Pull request overview

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

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

Per AUTOSAR SOME/IP-SD, the reboot bit on emitted SD messages must flip from RecentlyRebooted to Continuous once the session counter wraps past 0xFFFF. SdStateManager already owns the counter, so it also tracks wrap (new has_wrapped: AtomicBool latched exactly on the 0xFFFF -> 0x0001 transition) and exposes reboot_flag() as the single source of truth. The four SD emission paths — SdStateManager::send_offer_service, Server::send_unicast_offer, send_subscribe_ack_from_view, and send_subscribe_nack_from_view — all now consume the tracked flag instead of hardcoding Flags::new(true, true). Coverage: four new non-ignored unit tests cover the state machine (fresh, sub-wrap, exactly-on-wrap, monotonic-after-wrap); assert_offer_matches takes an expected RebootFlag; the existing wrap multicast test now asserts first-emit=RecentlyRebooted and second-emit=Continuous across the boundary. Responds to PR #75 feedback on src/server/sd_state.rs:90. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Round-2 Copilot feedback on PR #76 caught 5 resolvable issues (6th rejected — see below): - src/client/inner.rs: the three `push_front(...).ok()` re-enqueue sites (SetInterface x2, SendSD, Subscribe) silently dropped the returned Err. Swapped each to `if let Err(rejected) = ... { ... rejected.reject_with_capacity("request_queue"); }`, matching the primary `push_back` overflow arm. These branches are defensive — by construction the slot we just popped is free — but a future refactor that changes queue usage would otherwise reintroduce the silent-drop-of-oneshot-senders regression cb1d0d1 was specifically written to prevent. - src/server/subscription_manager.rs: `list.push(...).ok()` on a freshly-allocated `SubscribersList` (first subscriber in a new event group) swapped to `.expect(...)` with a message naming the `SUBSCRIBERS_PER_GROUP >= 1` invariant. Tripwires a future cap-0 regression at test time instead of silently losing the only subscriber. - src/client/session.rs: reword the `SessionTracker` doc comment's reference to non-existent "module docs" — point directly at the `SESSION_CAP` constant instead. Rejected: src/client/error.rs `Capacity` variant as a breaking exhaustive-match break. This is consistent with the earlier decision recorded on #75/#80 to accept the breaking change rather than carry deprecated shims or wrap everything in `Error::Io` — the current release is a breaking version bump by design, and hiding the variant in `Io` would lose the lowercase-snake_case tag semantics the new error was specifically designed to carry. No production behavior changes — just defensive tripwires, docs. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Copilot

Pull request overview

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

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

- src/lib.rs: UDP_BUFFER_SIZE doc now enumerates exactly which send paths honor this cap (SocketManager::send, publish_event, publish_raw_event) and explicitly calls out the SD announcement / SubscribeAck / SubscribeNack paths that still use heap Vec buffers as a known gap planned for the bare-metal no_alloc refactor. - src/server/event_publisher.rs: reworded "stack buffer sized to MTU" comments at the two buffer-allocation sites — the buffer lives in the async future's state, not literally on the stack, and the cap is a UDP payload limit, not an Ethernet MTU. New wording points at the UDP_BUFFER_SIZE docs for the distinction. The two `use std::vec` comments (event_publisher.rs:335, socket_manager.rs:362) were verified to be false positives: removing the imports breaks the lib-test build with 4 errors about `vec!` macro not in scope. Same no_std mechanics as the prior #75-1 resolution — reply posted on the comment threads. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Copilot

Pull request overview

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

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

Covers the send path end-to-end (SOME/IP envelope, SD flags, OfferService entry fields, and the IPv4 endpoint option) plus session-id advancement and wrap-through-zero exercised via send_offer_service itself, and a smoke test for Server::start_announcing. All `#[ignore]`d pending the loopback MULTICAST-flag fix on this branch; without that fix, hosts drop the multicast packet silently and the tests time out on recv. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Rustfmt on stable wraps the let-else continue blocks and the assert_offer_matches signature differently than I hand-wrote. Let cargo fmt normalize the style. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Per AUTOSAR SOME/IP-SD, the reboot bit on emitted SD messages must flip from RecentlyRebooted to Continuous once the session counter wraps past 0xFFFF. SdStateManager already owns the counter, so it also tracks wrap (new has_wrapped: AtomicBool latched exactly on the 0xFFFF -> 0x0001 transition) and exposes reboot_flag() as the single source of truth. The four SD emission paths — SdStateManager::send_offer_service, Server::send_unicast_offer, send_subscribe_ack_from_view, and send_subscribe_nack_from_view — all now consume the tracked flag instead of hardcoding Flags::new(true, true). Coverage: four new non-ignored unit tests cover the state machine (fresh, sub-wrap, exactly-on-wrap, monotonic-after-wrap); assert_offer_matches takes an expected RebootFlag; the existing wrap multicast test now asserts first-emit=RecentlyRebooted and second-emit=Continuous across the boundary. Responds to PR #75 feedback on src/server/sd_state.rs:90. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Round-2 Copilot review caught a real correctness bug in the prior reboot-flag refactor: the four SD emission paths read `reboot_flag()` BEFORE advancing the session counter, so the message whose session_id crosses 0xFFFF -> 0x0001 (where `has_wrapped` actually latches) still advertises `RebootFlag::RecentlyRebooted`. The flip only lands on the NEXT emission — violating AUTOSAR SOME/IP-SD semantics that say the wrap message itself should carry `Continuous`. Reordered in all four sites: call `next_session_id()` first so `has_wrapped` latches, then read `reboot_flag()` for this specific message. Sites: - `SdStateManager::send_offer_service` (sd_state.rs) - `Server::send_unicast_offer` (mod.rs) - `Server::send_subscribe_ack_from_view` (mod.rs) - `Server::send_subscribe_nack_from_view` (mod.rs) Added short comments at each site pointing at the canonical ordering note on `send_offer_service`. Also reworded the multicast-loopback `#[ignore]` comment block and per-test message to remove the stale branch-name reference (`feature/firmware_someip_conversion`) — the underlying dependency is the `lo` MULTICAST flag, not a branch-specific fix. New wording says "skipped on hosts whose `lo` lacks the MULTICAST flag" with the `ip link show lo` diagnostic pointer. Coverage: the existing ignore-gated wrap test `send_offer_service_wraps_session_id_through_zero_on_send` already asserts the pre-wrap/post-wrap flag transition on-the-wire; with the ordering fix it now passes in environments that run ignored tests (would have FAILED before this commit — which is why the bug slipped past the first round). The non-ignored state-machine tests (`reboot_flag_flips_to_continuous_exactly_on_wrap` et al.) are unaffected and still green. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Copilot round-3 flagged the `#[ignore]` reason on start_announcing_emits_first_offer_within_timeout for still carrying a branch-specific phrase ("re-enable after lo fix on this branch"), which becomes stale once merged. Replaced with a durable prerequisite description: requires loopback multicast support (MULTICAST on lo) Matches the companion rewording in a638a4b on the sd_state.rs multicast-loopback harness comment block. Addresses Copilot comment 3132878961. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Round-2 Copilot feedback on PR #76 caught 5 resolvable issues (6th rejected — see below): - src/client/inner.rs: the three `push_front(...).ok()` re-enqueue sites (SetInterface x2, SendSD, Subscribe) silently dropped the returned Err. Swapped each to `if let Err(rejected) = ... { ... rejected.reject_with_capacity("request_queue"); }`, matching the primary `push_back` overflow arm. These branches are defensive — by construction the slot we just popped is free — but a future refactor that changes queue usage would otherwise reintroduce the silent-drop-of-oneshot-senders regression cb1d0d1 was specifically written to prevent. - src/server/subscription_manager.rs: `list.push(...).ok()` on a freshly-allocated `SubscribersList` (first subscriber in a new event group) swapped to `.expect(...)` with a message naming the `SUBSCRIBERS_PER_GROUP >= 1` invariant. Tripwires a future cap-0 regression at test time instead of silently losing the only subscriber. - src/client/session.rs: reword the `SessionTracker` doc comment's reference to non-existent "module docs" — point directly at the `SESSION_CAP` constant instead. Rejected: src/client/error.rs `Capacity` variant as a breaking exhaustive-match break. This is consistent with the earlier decision recorded on #75/#80 to accept the breaking change rather than carry deprecated shims or wrap everything in `Error::Io` — the current release is a breaking version bump by design, and hiding the variant in `Io` would lose the lowercase-snake_case tag semantics the new error was specifically designed to carry. No production behavior changes — just defensive tripwires, docs. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

- src/lib.rs: UDP_BUFFER_SIZE doc now enumerates exactly which send paths honor this cap (SocketManager::send, publish_event, publish_raw_event) and explicitly calls out the SD announcement / SubscribeAck / SubscribeNack paths that still use heap Vec buffers as a known gap planned for the bare-metal no_alloc refactor. - src/server/event_publisher.rs: reworded "stack buffer sized to MTU" comments at the two buffer-allocation sites — the buffer lives in the async future's state, not literally on the stack, and the cap is a UDP payload limit, not an Ethernet MTU. New wording points at the UDP_BUFFER_SIZE docs for the distinction. The two `use std::vec` comments (event_publisher.rs:335, socket_manager.rs:362) were verified to be false positives: removing the imports breaks the lib-test build with 4 errors about `vec!` macro not in scope. Same no_std mechanics as the prior #75-1 resolution — reply posted on the comment threads. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

JustinKovacich · 2026-06-09T20:55:57Z

Closing without merge to declutter the stack: this phase's changes are carried in full by the consolidated lineage under PR #114 (phase 21), which the next development stack builds on. Branch is retained.

Copilot AI review requested due to automatic review settings April 22, 2026 20:40

JustinKovacich self-assigned this Apr 22, 2026

Copilot started reviewing on behalf of JustinKovacich April 22, 2026 20:41 View session

JustinKovacich added documentation Improvements or additions to documentation enhancement New feature or request labels Apr 22, 2026

Copilot AI reviewed Apr 22, 2026

View reviewed changes

JustinKovacich mentioned this pull request Apr 22, 2026

Switch to existing collections to heapless, hide other non-embedded f… #76

Closed

Copilot AI review requested due to automatic review settings April 23, 2026 02:18

Copilot started reviewing on behalf of JustinKovacich April 23, 2026 02:18 View session

Copilot AI reviewed Apr 23, 2026

View reviewed changes

Comment thread src/server/mod.rs

Comment thread src/server/sd_state.rs Outdated

JustinKovacich requested a review from Copilot April 23, 2026 17:41

Copilot started reviewing on behalf of JustinKovacich April 23, 2026 17:42 View session

Copilot AI reviewed Apr 23, 2026

View reviewed changes

Comment thread src/server/mod.rs Outdated

Comment thread src/server/mod.rs Outdated

Comment thread src/server/sd_state.rs Outdated

Comment thread src/server/sd_state.rs

Comment thread src/server/mod.rs

JustinKovacich force-pushed the feature/firmware_someip_conversion branch from fe47e7a to c278a83 Compare April 23, 2026 17:56

Copilot AI review requested due to automatic review settings April 23, 2026 18:12

Copilot started reviewing on behalf of JustinKovacich April 23, 2026 18:12 View session

Copilot AI reviewed Apr 23, 2026

View reviewed changes

Comment thread src/server/sd_state.rs

Comment thread src/server/mod.rs Outdated

JustinKovacich requested a review from Copilot April 24, 2026 20:34

Copilot started reviewing on behalf of JustinKovacich April 24, 2026 20:34 View session

Copilot AI reviewed Apr 24, 2026

View reviewed changes

JustinKovacich marked this pull request as ready for review April 24, 2026 20:47

JustinKovacich requested a review from zheylmun April 24, 2026 20:47

JustinKovacich and others added 3 commits April 24, 2026 18:40

Extract SdStateManager from server mod

370d3ac

Shifted tests around, added a test, removed dead impl Default

9c73ab8

JustinKovacich and others added 4 commits April 24, 2026 18:40

fmt: apply rustfmt style to the new multicast tests

d3315e8

Rustfmt on stable wraps the let-else continue blocks and the assert_offer_matches signature differently than I hand-wrote. Let cargo fmt normalize the style. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

JustinKovacich force-pushed the feature/firmware_someip_conversion branch from b5927b7 to 7f975cc Compare April 24, 2026 22:41

JustinKovacich closed this Jun 9, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract SdStateManager from the server mod file#75

Extract SdStateManager from the server mod file#75
JustinKovacich wants to merge 7 commits into
mainfrom
feature/firmware_someip_conversion

JustinKovacich commented Apr 22, 2026 •

edited

Loading

Uh oh!

codecov-commenter commented Apr 22, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

JustinKovacich commented Jun 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

JustinKovacich commented Apr 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Apr 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

JustinKovacich commented Jun 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

JustinKovacich commented Apr 22, 2026 •

edited

Loading

codecov-commenter commented Apr 22, 2026 •

edited

Loading