Repository navigation
feat(uds_services)!: a session change prepares, or is refused, before its response - #94
Conversation
JustinKovacich
left a comment
There was a problem hiding this comment.
Stack review of #92–#96, this PR's part: 3 Required, 1 Question, 1 Optional.
Gate: fmt clean; clippy pedantic 0 hits; all tests pass; workspace powerset clean; cargo doc with -D missing_docs clean; the size check matches the baseline (text 54340, main frame 3320, which the commit message accounts for).
The new driver tests are good coverage: pending, refusal, a close mid-prepare, busy, and a suppressed response that pended.
Skills run for this review: rust-quality-gate, rust-principles, rust-api-design, rust-embedded, rust-design-review (change-scoped), /code-review, writing-for-customers.
519fe38 to
d425ddb
Compare
JustinKovacich
left a comment
There was a problem hiding this comment.
Every Required from the first round is fixed:
abandonon both traits, called fromapply, from the step guard and on a failed confirmation, with tests for each path.prepareandresetare told the active session.- The changelog snippet spells
core::future::Future. - The transport-error drop path is listed.
On MAY_RESPOND_PENDING and prepare: keep them required. I've answered on that thread.
1 Optional.
Gates at this head, run locally: cargo fmt --check, clippy pedantic on the workspace (0 hits), cargo test --workspace --all-features, the cargo hack powerset (depth 2), cargo doc with -D missing_docs and broken links denied, and just embedded with both size baselines. All clean.
Skills: rust-quality-gate, rust-principles, rust-api-design, rust-embedded, rust-design-review (change-scoped) and writing-for-customers. /code-review ran on this PR.
Security Audit is red on main too (RUSTSEC-2026-0110 and RUSTSEC-2026-0386), so its failure here is not this PR's.
JustinKovacich
left a comment
There was a problem hiding this comment.
The round-2 Optional is fixed, and the fix goes further than it asked:
- A drained failed confirmation now stays on the server (
Unconfirmed::failed), and the step guard abandons it once when the step ends.a_failed_confirmation_drained_before_a_transport_error_is_abandoned_oncecovers it. - A
tS3_Serverexpiry the drains recorded but the step never applied is applied by the guard, and the confirmed session it overtook is abandoned.a_confirmed_session_a_timeout_overtakes_is_abandoned_when_the_step_failscovers it. stepandrunnow fail withStepError. An error at the between-requestsnext_eventisRecoverable:idlestays set there, so the change awaiting its confirmation is kept and the next step takes the confirmation.a_transport_error_between_requests_keeps_the_change_awaiting_confirmationcovers it. Every other error isTerminal, as before. This is the second remedy suggested for #96's re-arm finding, andhandleawaits nothing beforebetween_requests, soRecoverablecannot be returned with anything half done.- The
step/rundocs, bothabandondocs, the changelog's breaking entry,service-traits.rstand the milestone-1 limits instack-position.rstall describe the split.
No comments.
Gates at 237785e, run locally: cargo fmt --check, clippy pedantic on the workspace (0 hits), cargo test --workspace --all-features, the cargo hack powerset (depth 2), cargo doc with -D missing_docs and broken links denied, and just embedded with both size baselines. All clean.
Skills: rust-quality-gate, rust-principles, rust-api-design, rust-embedded, rust-design-review (change-scoped) and writing-for-customers. /code-review not run: the four fixups are small enough that I traced each changed path by hand.
Security Audit is red on main too (RUSTSEC-2026-0110 and RUSTSEC-2026-0386), so its failure here is not this PR's.
… its response DiagnosticSessionControl gains a required async prepare, awaited once a change is admitted and before its positive response, and a required MAY_RESPOND_PENDING saying whether 0x78 answers while it runs. A refusal is the request's negative response, conditionsNotCorrect say. The session is still entered only on the positive response's confirmation, and a link closed while prepare runs drops it with nothing sent. UDSSVC_ARCH_0046 places the stage, and UDSSVC_ARCH_0033 says why the service carries the constant. Breaking: every implementation adds both, as the changelog shows. On the embedded probe text falls 1348 bytes and main's frame grows from 1552 to 3320 bytes. No future changes size (-Zprint-type-sizes), but LLVM now inlines the poll of Server::handle, a separate 2232-byte frame before, into main, and keeps serve's poll, 832 bytes, apart; the deepest call chain from main, summed over the image's call graph, falls from 5180 to 5084 bytes. The baseline records it.
… before its response The probe's run-time configuration moves with this change as its default one does: main's frame grows from 9200 to 10080 bytes and text falls from 56564 to 56244. size-baseline-run-time.json records it.
… before its response
… before its response
… before its response
… before its response The embedded probe grows with what abandoning a change keeps: two server fields add 8 bytes of data (of bss in the run-time configuration), and the abandoned change each drain may record adds text to serve's poll and to main, into which handle is inlined. Text rises from 54420 to 56816 bytes and main's frame from 3320 to 3568; in the run-time configuration text rises from 56264 to 59396 and main's frame from 10080 to 10320. Deferred::merge is kept out of line, which saves 1436 bytes of the default configuration's text. Both baselines record it.
… before its response The probe's size baselines are re-recorded for this change: conclude now reports whether it handed the request's own change over, which costs main's frame 16 bytes (3568 to 3584; run-time 10320 to 10336) and saves text (56816 to 56512; run-time 59396 to 59300).
… before its response
… before its response A failed confirmation a drain takes is kept on the server, beside the response awaiting its confirmation, rather than in the drain's record, and the step's guard abandons it as the step ends, so a transport error before the record is applied no longer drops it. Tested with a failed confirmation drained while a second change prepares, then a transport error: abandon is called once. The probe's main frame grows 8 bytes (3584 to 3592) and its text falls from 56512 to 56264; the run-time configuration is unchanged. The baseline records it.
… before its response A transport failing at the between-requests await is recoverable: Server::step and Server::run fail with the new StepError, Recoverable there and Terminal anywhere else. After a recoverable error the step's guard keeps the change awaiting its confirmation and the instance may be stepped again; after a terminal one it abandons it, as before. Breaking, as the changelog says; the step and run docs, UDSSVC_ARCH_0040's limit and UDSSVC_ARCH_0046 say which error abandons. The test transport gains a failing step, and a test steps on after a recoverable error and enters the session on the confirmation. The probe's text moves from 56264 to 56280 bytes (run-time 59300 to 59284) and main's frame falls back to 3584; the baseline records it.
… before its response What the drains record is kept on the server, merged in arrival order, until apply runs, so a terminal error no longer drops a tS3_Server expiry or the confirmed session it overtook: the step's guard applies the expiry and abandons the session. Reached with a second tester's request in progress, which does not stop the controlling tester's tS3_Server, and tested so. A confirmation not yet applied is still not made, as the step doc and UDSSVC_ARCH_0040's first limit now say; that limit also names Server::into_parts instead of saying the parts cannot be taken back out. Events between requests are handled in one method, which keeps handle within clippy's length. The probe's text grows from 56280 to 56640 bytes and data by 8 to 9576, and main's frame falls from 3584 to 3528; run-time: text 59284 to 59504, bss 9724 to 9732, main's frame 10336 to 10296. Both baselines record it.
… before its response UDSSVC_ARCH_0040's cancel-safety limit names the one safe await as the transport's next_event between requests, since next_event is awaited mid-request too, and says what a cancel elsewhere leaves as the terminal error now does.
237785e to
6b269b3
Compare
…n time (#95) An application can decline a listed service at run time through one `serves = f` key on `uds_server!` (`fn(&App, UdsServiceType) -> bool`, constant for the server's life): the server answers serviceNotSupported, in the order ISO 14229-1 Figure 5 gives, instead of a later NRC. A C binding whose tables leave a service unserved needs this. Depends on #94. ## Issue URL Towards luminartech/halo#5100 ## Testing Run on this branch with the commits of the PRs below it, each commit alone and on the stack's top: - tool self-tests, `validate`, `governance`, the Sphinx needs build, and prose (plain and CI's `--strict crates/uds_c`) - `cargo test --workspace --all-features`; clippy on the host and for `thumbv7em-none-eabihf` - `just embedded` builds on 1.91 for both targets and the size check (matches the baseline) - `cargo doc` with `-D warnings`; `pre-commit run --all-files` - On the stack's top also the uds_c Miri job from `ci.yml` and `cargo package -p uds_c --no-verify`
A session change asks the application to prepare it before the positive response, with the active and target sessions, and the application can refuse it or answer pending;
MAY_RESPOND_PENDINGbecomes required, so this is a breaking change for implementers ofDiagnosticSessionControl. A change that was prepared but not made (refused later, unconfirmed, link closed, tS3 first, mid-request transport error, step dropped) is reported throughabandon, onDiagnosticSessionControlandEcuReset, empty by default;EcuReset::resetalso gets the active session.Server::stepandrunreturn aStepError:Recoverablewhere the transport failed between requests, keeping a change that awaits its confirmation and letting the server be stepped again,Terminalotherwise.Depends on #93.
Issue URL
Towards https://github.com/luminartech/halo/issues/5100
Testing
Run on this branch with the commits of the PRs below it, each commit alone and on the stack's top:
validate,governance, the Sphinx needs build, and prose (plain and CI's--strict crates/uds_c)cargo test --workspace --all-features; clippy on the host and forthumbv7em-none-eabihfjust embeddedbuilds on 1.91 for both targets and the size check (matches the baseline)cargo docwith-D warnings;pre-commit run --all-filesci.ymlandcargo package -p uds_c --no-verify