Repository navigation
feat(uds_c): a UDS server over DoIP for C firmware, served from tables of handlers - #96
Merged
Merged
Conversation
FelicianoAngulo2
marked this pull request as ready for review
October 27, 2026 16:37
This was referenced Oct 27, 2026
JustinKovacich
requested changes
Oct 27, 2026
JustinKovacich
left a comment
JustinKovacich
left a comment
Contributor
There was a problem hiding this comment.
Stack review of #92–#96, this PR's part: 8 Required, 3 Question, 2 Optional.
Gate (local): fmt clean; clippy pedantic 0 hits, and clean for uds_c on thumbv7em; uds_c's 62 tests pass, including the header layout test; powerset clean (workspace, and uds_c on thumbv7em with time-driver); cargo doc with -D missing_docs clean; size unchanged. I read every unsafe block in Firmware, the record lending and Layers; each relies on a stated single-context contract carried by an unsafe fn or constructor, Miri passes, and I found no hole.
CI is red on this PR's own checks (see inline). Security Audit is red on every PR in the stack and on main's nightly runs (RUSTSEC-2026-0110, bare-metal unmaintained), so that one is not this PR's doing.
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.
FelicianoAngulo2
force-pushed
the
feat/uds-c-server
branch
from
October 28, 2026 01:09
af21403 to
7aefc49
Compare
JustinKovacich
requested changes
Oct 28, 2026
JustinKovacich
left a comment
JustinKovacich
left a comment
Contributor
There was a problem hiding this comment.
All thirteen points from the first round are fixed, and each fix checks out against the code:
abandoninUdsTransitionHooks, with the order documented.- The all-zero seed is refused, and the unlocked answer has the level's length.
- 0x04 and 0x05 continue.
Firmwareis the one public way to drive the server.Validatedis required to arm.- The reset's execute runs before the default session's.
DTC_AVAILABILITYis onPolicy.uds_tcp_write's contract says what a confirmation means.- The prose lint and line widths are clean.
The C binding's package job failed because cargo package -p uds_c resolves uds_on_ip from crates.io, where 0.7.0 has no connection feature. 03de923 packages uds_c with simple_doip, uds_services and uds_on_ip, so cargo takes them from the workspace, and the package is now verified as well. The PR body's cargo package -p uds_c --no-verify line is out of date.
This round found two new problems. The re-arm finding is reproduced with a host test over the bench harness, and its output is in the comment.
2 Required, 1 Question, 2 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.
FelicianoAngulo2
force-pushed
the
feat/uds-c-server
branch
from
October 28, 2026 03:06
03de923 to
4ac8825
Compare
FelicianoAngulo2
force-pushed
the
feat/uds-c-server
branch
from
October 28, 2026 12:48
4ac8825 to
179a770
Compare
FelicianoAngulo2
enabled stacked auto-merge (squash)
November 5, 2026 00:46
…unsafe_code alone check_governance accepts uds_c, the C binding, declaring its own lint tables instead of inheriting the workspace's, and only where those tables are the workspace's entry for entry with unsafe_code at deny rather than forbid. The C binding declares and calls C, which forbid rules out and no attribute can relax; at deny, each module that does so allows it with a reason. Own tables in any other crate, and any other difference, a dropped or relaxed entry or a table identical to the workspace's, are rejected, and the self-tests show each. The tables are read line by line, so the check still needs no TOML parser and runs on any Python 3.
An entity may confirm a diagnostic message once its socket reports the message's last byte transmitted, not once the socket has taken it. Entity gains a last type parameter, a sealed ConfirmOn: OnWrite, the default and the behavior so far, or OnTransmit, chosen with confirm_on_transmit or by naming it in the type new_discovery_later builds, and which bounds the socket by the new TransmitProgress trait, so no other socket need implement it. Each slot counts the bytes written to its socket from its first, and a request's last byte is placed on that stream when the write that takes it completes. While a written response awaits transmission the entity reads the progress again each millisecond, holding back no other connection, and a connection that closes first confirms what it wrote, as before. DoIpTransport::start_discovery takes either kind of entity. On the embedded probe data grows 80 bytes, a stream position for each of the entity's four confirms and a byte count for each of its two slots, and text 364 bytes; with the run-time feature bss grows the same 80 bytes, text 532 and main's frame 16. The baselines record it.
…s of handlers The new crate holds everything a C firmware needs to serve UDS over DoIP from the stack, against the fixed C contract it ships as include/uds_c.h. UdsLayer and its tables mirror the header's structures, with the layouts asserted for 32-bit targets on both sides and compared field by field with the host's C compiler by a test. TableApp<P: Policy> implements every service uds_services stages over an ordered list of layers, the earlier entry winning, declines through serves those no layer has a handler for, so they are serviceNotSupported, and is built in a const context; the Policy states the layers, the sessions and their timing, the transition rule and the limits, and TableApp::validate refuses tables the server cannot serve safely. A handler answers through uds_record_copy into the record the server lends it while it runs, pends and is canceled through the operation-status protocol, and a reset or session change runs every layer's check and prepare before its response and every layer's execute, the last layer's first, from execute_deferred once the response is out. nal adapts the header's socket calls to edge-nal, cancel-safe; doip names the entity and transport over them, starts discovery and polls the server; the time-driver feature gives embassy-time's clock over uds_now_ms. The crate restates the lint standard with unsafe_code at deny and is not a default member, since it enables simple_doip's connection feature, so CI lints, tests, documents and packages it in its own job, packaging unverified until the APIs it calls are on crates.io, runs its tests under Miri, and builds it for both bare-metal targets at the MSRV. It depends on uds_on_ip, so it is published last and carries the release tag. The embedded probe is unchanged.
uds_record_copy_16_be, uds_record_copy_32_be and uds_record_copy_64_be copy a u16, u32 or u64 into the lent record at an offset, in big-endian order, the byte order of UDS records, bounded as uds_record_copy bounds a copy, so a C handler no longer packs the bytes itself; a float goes as its IEEE 754 bits. The test firmware's seed handler answers through uds_record_copy_16_be.
dtc_status gives the ISO 14229-1:2020 Annex D.2 transitions a DTC memory makes, after a matured test result, at an operation cycle's start and on a clear, as pure const fns over DtcStatusMask, so a memory kept by the application, in C behind uds_c say, applies the annex rather than restating it, from any task. UDSSVC_ARCH_0047 draws the boundary: the memory is the application's, the transitions this crate's. They maintain every bit but warningIndicatorRequested, which MAINTAINED leaves out as the server's DTCStatusAvailabilityMask, and confirm a DTC at its first failure.
A layer's UdsDtcHooks now describe the DTC memory it keeps, each DTC's number and status and its snapshot record's identifiers and data, in place of a handler that built the ReadDTCInformation response bytes, so the server encodes the reports itself from uds_protocol's types (0x01, 0x02, 0x03, 0x04 and 0x0A, against the availability mask the transitions maintain) and the firmware restates none of the standard. ClearDiagnosticInformation matches its group to a DTC the memory holds, or all of them, and calls the clear handler with that DTC's index, requestOutOfRange for any other group. The header exports uds_services' Annex D status transitions as pure functions the memory may call from any task, and names the status bits they maintain. UdsDtcReportFn is gone, UdsDtcClearFn takes an index, and UdsDtcHooks and UdsLayer grow.
…ir sessions ISO 14229-1:2020 10.2 Figure 7 has ControlDTCSetting and CommunicationControl state resume on entering a session where the service is not supported, which uds_services leaves to the application (UDSSVC_ARCH_0038). execute_deferred now does it for the C layers: entering any session but the extended and programming ones calls the setting handler with on and the communication-control handler with enableRxAndTx for every communication type, once each, a response-pending answer cancelled at once, so firmware no longer restates the rule in a session hook.
Every firmware built on uds_c wrote the same plumbing around doip::poll_run and execute_deferred: a cell for a static only its polling context reaches, the run future kept across polls and built again after a reset, and a guard against arming it twice. Firmware holds the server and its run future in one static, const-constructed so the server is initialised data. Its arm and poll are unsafe, their contract confining every call to one execution context, with the server built there or constant and each run future the only holder of its server reference, which is what its Sync rests on; arm rearms a server a transport error stopped. The firmware still names the run future's type, which only type_alias_impl_trait can do for a static. Tested on the host and under Miri.
…emory's layer DtcStatusMask::intersects is true for an empty mask, so a status mask the availability mask emptied (0x19 01/02 with 0x80, say) reported every DTC; the reports now match bit by bit. ClearDiagnosticInformation calls the clear of the layer that keeps the memory, whose indices it passes, rather than the first layer with one. A snapshot record whose count overstates the items the memory gives is generalReject instead of a malformed response, a count past 255 is sent as 0x00, the DTC number's top byte is ignored on every report and lookup alike, and the DTCs are read below UDS_DTC_ALL. validate now refuses a second DTC memory, and a snapshot or clear without its memory's get, which the server would never call; a reset whose execute returns resumes DTC setting and communication as the default session does.
…m tables of handlers
…m tables of handlers
…m tables of handlers
…m tables of handlers
…ide their sessions
…m tables of handlers
…m tables of handlers
…m tables of handlers
…m tables of handlers
…m tables of handlers
…m tables of handlers
…ide their sessions
…m tables of handlers
…m tables of handlers
…m tables of handlers
…m tables of handlers The C binding's package job packages uds_c with simple_doip, uds_services and uds_on_ip, so cargo resolves those from the packages it builds rather than from crates.io, where uds_on_ip 0.7.0 has no `connection` feature. The package is now verified as well: the build compiles against the APIs uds_c calls before they are released. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…static The server is stepped again only where uds_services says it may be. Firmware::poll builds the run future again over the same server after a recoverable transport error, returning the new RunError::Interrupted, so a response awaiting its confirmation keeps its change. After a terminal one it returns RunError::Stopped and leaves the server unarmed for good: Firmware::arm refuses it from then on, and the server serves again once the firmware restarts. start_discovery's, arm's, poll's, the README's and the changelog's accounts follow. The reviewer's reproduction is a test: with writes capped at 4 bytes, a 10 03 prepared by both layers, then an accept failing for one poll, the tester receives the whole 50 03, both layers execute the extended session and nothing is abandoned. A failed accept mid-request is tested to stop the server for good.
at_operation_cycle_start keeps testFailed: ISO 14229-1:2020 Table D.3 changes it only on a test result and on a clear, so a DTC that failed in the last cycle still reads as failing until its test reports again. A failed DTC (0x2F) starts the next cycle as 0x6D, not 0x6C; the test and the doc say so.
uds_dtc_status_at_operation_cycle_start keeps testFailed, as uds_services' transition now does: a failed DTC (0x2F) starts the next cycle as 0x6D. The header and the test say so.
…m tables of handlers The server has one network and one DTC group, so the narrower scopes a tester may name are refused rather than widened: a CommunicationControl subnet other than all networks (0x0) and the receiving one (0xF) is requestOutOfRange (0x31), and ControlDTCSetting's MAX_OPTION_RECORD_LEN is 0, so a request with an option record is incorrectMessageLengthOrInvalidFormat (0x13). Neither reaches its C handler, whose signature is unchanged. The header, the changelog and the handlers' test say so.
…m tables of handlers Downloads have two session gates, Policy::TRANSFER_SESSIONS and the table's transfer access, and the server serves only where both admit. TableApp::validate now refuses a download access naming a session outside TRANSFER_SESSIONS with the new TableError::TransferSessions, so the mismatch is caught at start-up rather than as every download refused in that session. The header, the changelog and a validation test say so.
…m tables of handlers The crate's .gitignore drops .omniscient and .superpowers/, local tool directories that belong in a global excludes file.
…static A terminal transport error leaves the server dead until the firmware restarts, and RunError::Stopped, the README, the changelog and uds_c.h now state the firmware's duty: restart, by resetting the ECU say, to serve again. The changelog's Firmware entry is rewrapped.
The changelog's DTC entry is rewrapped to the line width.
…m tables of handlers The transfer-session check holds only a layer that serves downloads, one with begin, to TRANSFER_SESSIONS: a layer without begin may leave any access mask, UDS_SESSION_MASK_ALL included, and still start. Tested both ways; the header and the changelog say so.
FelicianoAngulo2
force-pushed
the
feat/uds-c-server
branch
from
November 5, 2026 02:15
179a770 to
ac0e794
Compare
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.
The new
uds_ccrate embeds the uds_stack server in C firmware: the firmware lists its identifiers and handlers in C tables (uds_c.h), supplies non-blocking sockets and a clock, and polls the server from one task. Handlers answer throughuds_record_copy, bounded by each identifier's table length, so C never writes server memory.uds_record_copy_16_be,_32_beand_64_becopy an integer big-endian, the byte order of UDS records. For the DTC services a layer keeps a DTC memory the server reads (each DTC's number, status and snapshot record), and the server encodes the ReadDTCInformation reports itself; the memory updates each status with the ISO 14229-1 Annex D transitionsuds_services::dtc_statusadds (UDSSVC_ARCH_0047), whichuds_c.hexports as pure functions;validaterefuses a second DTC memory, or a snapshot or clear without its memory. After a confirmed response the server resumes DTC setting and communication on entering a session where ControlDTCSetting and CommunicationControl are not served, or after a reset whose execute returns (ISO 14229-1 10.2), so firmware does not restate the rule. A response is confirmed once the firmware'suds_tcp_transmittedreports its bytes transmitted, through simple_doip's opt-inOnTransmitconfirmation, so a reset's execute runs only after its response has left the ECU. Each layer's transition hooks (check, prepare, execute, abandon, with the active session) let a layer undo a prepare another layer refused.Firmwareis the one way to drive the server: it keeps the server and its run future in onestatic,armtakes the proofvalidatereturns andpollreads that server's layers, both unsafe on the one-context contract. A transport error between requests is recoverable andpollruns the same server on (RunError::Interrupted); one mid-request stops it for good (RunError::Stopped), and the firmware restarts to serve again; naming the run future's type for thestatictakes the nightlytype_alias_impl_traitin the firmware's crate, while this crate stays on stable. The governance tool admits the restated lint table that relaxesunsafe_codefor this crate alone.Depends on #95.
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.ymland the package job (cargo package -p simple_doip -p uds_services -p uds_on_ip -p uds_c)rust_diag_tc4builds and serves over this branch's tip (luminartech/halo#5928 and its children)