Conversation
Add host-side unit tests for the HID-I2C target service. Coverage includes command-header parsing, SET_REPORT and GET_REPORT handling, the reset/interrupt handshake, and TimeoutBus timeout and recovery behavior, plus HID descriptor construction and oversize-rejection checks. Introduce a shared test_support module with a generic recording HidDevice mock so the descriptor and service tests no longer each define their own near-identical mock. Annotate the command byte arrays with HID-over-I2C wire-format comments. Add dev-dependencies (tokio, embassy-time, critical-section) needed to run the host async tests. Assisted-by: GitHub Copilot:claude-opus-4.8
Follow HID-I2C framing for GET_REPORT and SET_REPORT data. Report IDs are encoded in the command header and are not repeated in the data payload. Assisted-by: GitHub Copilot:GPT-5.6 Sol
There was a problem hiding this comment.
🟡 Changes recommended
Moderate test correctness issues remain in service payloads, timeout setup, and report-type handling.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds unit-test infrastructure and coverage for the HID-I2C target service, including descriptor validation, command handling, reset behavior, and timeout paths.
Changes:
- Adds reusable HID and I2C test doubles.
- Adds descriptor and service behavior tests.
- Adds test dependencies and lockfile updates.
File summaries
| File | Summary |
|---|---|
hidi2c-target-service/src/test_support.rs |
Adds shared test fixtures; the mock must preserve and honor the requested report type. |
hidi2c-target-service/src/service.rs |
Adds service tests; several tests have invalid SetReport payloads, and GetReport tests wait indefinitely before reaching validation. |
hidi2c-target-service/src/lib.rs |
Registers the test-only support module. |
hidi2c-target-service/src/device_descriptor.rs |
Adds descriptor sizing and validation tests. |
hidi2c-target-service/Cargo.toml |
Adds test dependencies. |
Cargo.lock |
Updates locked dependency data. |
Review details
Suppressed comments (3)
hidi2c-target-service/src/service.rs:1036
ScriptedBus::listenis hard-coded to remain pending, butprocess_command'sGetReportpath waits forlisten_for_response()before it callsrespond_to_read(). The configuredread_statusestherefore never get consumed, so this test times out and cannot record the expected reads. Script a queuedRequest::Readinlistenas well, or use a bus mock that models both phases.
let mut bus = scripted_timeout_bus(ScriptedBus {
read_statuses: VecDeque::from([ReadStatus::Complete(2), ReadStatus::Complete(1)]),
..Default::default()
});
hidi2c-target-service/src/service.rs:956
- Because
MOUSE_DESCRIPTORuses explicit report IDs,process_commandtreats the first byte after the length header as the report ID (data_start_index = 1). This command supplies onlyaa bb cc, so the requested report slice is out of bounds and the followingunwrap()fails; include the echoed report-ID byte in the SetReport data.
0x05, // length field, low byte
0x00, // length field, high byte -> 5 total bytes
0xaa, // report payload
0xbb,
0xcc,
hidi2c-target-service/src/service.rs:983
- This descriptor also uses explicit report IDs, so the SetReport payload must contain the echoed
0x21ID after the two-byte length header. With length3and only0x5apresent,data_start_index = 1leaves no payload byte andprocess_command(...).await.unwrap()fails; the length and payload need to account for that ID byte.
0x03, // length field, low byte
0x00, // length field, high byte -> 3 total bytes
0x5a, // report payload
- Files reviewed: 5/6 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Return the matching input or feature report variant from the shared test mock so report-type regressions cannot pass unnoticed. Assisted-by: GitHub Copilot:GPT-5.6 Sol
958b26c to
f964c39
Compare
felipebalbi
left a comment
There was a problem hiding this comment.
Blocking: CI is red, and the cause is a reverted fix
ubuntu / stable / test fails — 18 passed, 4 failed:
set_output_report_forwards_payload— panics at service.rs:961set_feature_report_accepts_extended_report_id— panics at service.rs:988get_feature_report_excludes_explicit_report_id_from_payload— panics at service.rs:1048get_report_rejects_output_report_type— assertion failure at service.rs:1028
Please don't fix these by adjusting the byte arrays. The history matters:
68d69e49added the tests and fixed a real SET_REPORT bug — consumed the echoed report-ID byte, validated it against the command header, subtractedHID_REPORT_HEADER_SIZE_BYTES + report_id_size, and addedset_report_rejects_mismatched_wire_report_id.8a308e42reverted all of that on the premise that report IDs "are not repeated in the data payload", deleted that test, and rewrote the remaining expectations to match — incorrectly, which is why four are red.
Net production diff against main is zero, so the bug is still there. Details in my inline comment on the SET_REPORT length arithmetic; I would like to settle what the spec actually says before any code moves.
Related: the crate now disagrees with itself in three places on GET_REPORT framing. process_command emits a bare 2-byte header; reply_with_input_report (service.rs:510-526) emits [len_lo, len_hi, report_id]; device_descriptor.rs:152 advertises w_max_input_length = input_max + 2 + 1. Whichever is right, all three should agree, and a test should pin it.
Two process asks: a production behaviour change does not belong in a test(...) PR, and please cite the spec section rather than an AI paraphrase of it.
Coverage gaps
No coverage on process_request, process_register_access, or process_input_report_read. These are the same shape as reply_with_input_report, which this PR does test directly via a real Runner — so the harness already exists. Runner::run() and listen_indefinitely I will grant are awkward, and Service::reset() is only meaningful alongside run().
Also untested: invalid opcode byte, invalid register address, I2cPowerState::try_from failure, GET_REPORT with Input type, SET_REPORT with Input type, and the RepeatedStart skip loop in listen_for_response — that last one is unreachable, since ScriptedBus::listen is hardcoded pending().
More broadly, the failure paths are structurally untestable rather than merely untested. Both bus mocks and the pin mock use Infallible, so every Error::Bus(_) arm and all five unwrap_or_else(|_| error!(...)) arms cannot be reached by construction. Inline comments on both.
The harness is heavier than the code under test
9 of the 22 tests are #[tokio::test] async fn with zero .await — all six descriptor tests and all three header tests. Those exercise DeviceDescriptor::new and HidI2cReportCommandHeader::try_from_command_byte, which are already sync and already pure. The small thing was right there; the harness reached past it.
That is the diagnosis for the rest of the file too: roughly 260 lines of mock (test_support.rs plus NoopBus/ScriptedBus/NoopPin) and three new dev-dependencies — tokio, critical-section/std, embassy-time/std — to assert that 0x23 means "Output report, ID 3". The three header tests need none of it, and they are the only cheap tests in the PR. Extracting the frame parsing out of process_command makes the four failing tests sync and mock-free, and opens the door to a proptest no-panic property over arbitrary input. Inline comment on that.
Note the new dev-deps will need to clear the deny and vet-dependencies jobs.
| struct NoopBus; | ||
|
|
||
| impl ErrorType for NoopBus { | ||
| type Error = Infallible; | ||
| } | ||
|
|
||
| impl I2cTargetAsync for NoopBus { | ||
| async fn recover(&mut self) -> Result<(), Self::Error> { | ||
| Ok(()) | ||
| } | ||
|
|
||
| async fn listen(&mut self) -> Result<Request, Self::Error> { | ||
| core::future::pending().await | ||
| } | ||
|
|
||
| async fn respond_to_read(&mut self, _buf: &[u8]) -> Result<ReadStatus, Self::Error> { | ||
| core::future::pending().await | ||
| } | ||
|
|
||
| async fn respond_to_write(&mut self, _buf: &mut [u8]) -> Result<WriteStatus, Self::Error> { | ||
| core::future::pending().await | ||
| } | ||
| } | ||
|
|
||
| struct NoopPin; | ||
|
|
||
| impl embedded_hal::digital::ErrorType for NoopPin { | ||
| type Error = Infallible; | ||
| } | ||
|
|
||
| impl embedded_hal::digital::OutputPin for NoopPin { | ||
| fn set_low(&mut self) -> Result<(), Self::Error> { | ||
| Ok(()) | ||
| } | ||
|
|
||
| fn set_high(&mut self) -> Result<(), Self::Error> { | ||
| Ok(()) | ||
| } | ||
| } |
There was a problem hiding this comment.
embedded-hal-mock is a much better option here.
The concrete problem: NoopPin::Error = Infallible, and both set_low/set_high are no-ops that return Ok(()). That makes two things impossible to test:
- The five
unwrap_or_else(|_| error!(...))arms inservice.rs(315, 492, 506, 536, 681) are unreachable. There is no way to verify this code behaves when the pin errors. reset_asserts_interrupt_and_first_read_acknowledges_completionassertsrunner.attn_pin.asserted(), which only readsAttnPinHandler's own bookkeeping. That test passes even if the GPIO is never driven at all.
embedded_hal_mock::eh1::digital::Mock fixes both: a Transaction list plus .done() proves the pin was actually driven, and it can be made to return Err on demand.
Scoping note so you don't go hunting: embedded-hal-mock only covers embedded_hal::i2c::I2c (controller mode). It has nothing for embedded_mcu_hal::i2c::target::asynch::I2c, so NoopBus/ScriptedBus still have to be hand-rolled — but they should stop using Infallible for the same reason.
Two practical notes: the new dev-dependency will need to clear the deny and vet-dependencies jobs, and it is worth considering whether a reusable target-mode I²C mock belongs in its own crate alongside type-c-interface-test-mocks / power-policy-interface-test-mocks rather than inside this crate's #[cfg(test)].
| // Note: per HID spec, the length field relayed over the wire needs to include its own length (2 bytes) | ||
| let report_size = (u16::from_le_bytes(len_header) | ||
| .checked_sub(device_descriptor::HID_REPORT_HEADER_SIZE_BYTES)) | ||
| .ok_or(Error::Protocol(ProtocolError::InvalidSize))? as usize; | ||
|
|
||
| let data_start_index = if hid_device.report_descriptor().report_ids_implicit() { | ||
| 0 | ||
| } else { | ||
| 1 | ||
| }; | ||
| let report_data = data | ||
| .get(data_start_index..data_start_index + report_size) | ||
| .ok_or(Error::Protocol(ProtocolError::InvalidSize))?; |
There was a problem hiding this comment.
This predates the PR, but it is the root cause of the failing tests, so it needs settling here. @williampMSFT could you confirm?
For a device with explicit report IDs and an N-byte payload, a spec-conformant SET_REPORT frame has len = 2 + 1 + N, and the data-register payload is [report_id, payload…] (N+1 bytes).
This code computes report_size = len - 2 = N + 1, sets data_start_index = 1, and asks for data.get(1..N+2) on an N+1-byte slice → None → Err(InvalidSize).
So SET_REPORT appears to be unconditionally broken for any device that uses report IDs. It simply never had a test. Am I missing something?
Note that the Output-register path at service.rs:438-446 computes header_size_bytes = 2 + (1 if explicit) and gets this right, so the two paths disagree with each other.
History worth flagging, because it explains the four red tests:
68d69e49fixed exactly this — subtractedHID_REPORT_HEADER_SIZE_BYTES + report_id_size, validated the echoed report ID against the command header, and addedset_report_rejects_mismatched_wire_report_id.8a308e42reverted it on the premise that report IDs "are not repeated in the data payload", deleted that test, and rewrote the remaining expectations to match.
Please don't resolve this by adjusting the test byte arrays — that is what produced the current state. Let's settle what the spec says first, then make the code and the tests follow it.
There was a problem hiding this comment.
Good catch, I'll send out a fix shortly. I think I haven't hit this in my time-alarm testing yet because I'm currently working on hooking up the host side of an input report that's one byte smaller than the largest one in the report descriptor, so this is timely :)
There's another unrelated problem in SetReport path that I found while working on the time-alarm stuff that may also impact this PR that I'll put in as part of that too - similar to the fix in 95f81fb. The spec is a bit ambiguous on whether or not you need to repeat the report ID twice in the command:get|set paths, but it turns out you do for both, which strikes me as a bit odd given the lengths they went to in the same packet to dodge a single byte in the case where the report ID is <=15, but oh well.
There was a problem hiding this comment.
I have a couple fixes which I'm almost ready to send. But I was going to send it as a PR to @jerrysxie's PR. I don't mind dropping the fixes, though.
There was a problem hiding this comment.
took a look at your PR and it looks like it incidentally fixes the SetReport thing too, so may as well just go with yours. Thanks!
| /// A HID device mock that records the most recent command it received so tests can assert on it. | ||
| /// | ||
| /// It is generic over the report-size / report-count / descriptor-length parameters so it can serve | ||
| /// both the descriptor-sizing tests (which need small maxima to exercise the oversize-rejection paths) | ||
| /// and the wire-format tests (which need room for multi-byte reports). | ||
| pub struct MockHidDevice<In, Out, Feat, const REPORT_COUNT: u8, const DESC_LEN: usize> | ||
| where | ||
| In: ArrayLength, | ||
| Out: ArrayLength, | ||
| Feat: ArrayLength, | ||
| { | ||
| descriptor: HidReportDescriptor<'static>, | ||
| pub power_state: Option<HidDevicePowerState>, | ||
| pub report_id: Option<ReportId>, | ||
| pub report_data: [u8; 8], | ||
| pub report_len: usize, | ||
| pub feature_report: bool, | ||
| pub reset_count: usize, | ||
| _phantom: PhantomData<(In, Out, Feat)>, | ||
| } |
There was a problem hiding this comment.
The HID mock is shaped so the main path cannot execute.
has_pending_input_report() (line 115) always returns false, so service.rs:510-538 — the entire input-report framing block, including the implicit/explicit report-ID branch — is dead in every test in this PR. wait_for_input_report() (line 111) is pending() forever, so run()'s second select3 arm never fires.
Separately, process_get_report, set_report, set_power_state and reset all return Ok unconditionally, so Error::Device(_) propagation is only ever reachable via the Reset opcode that process_command synthesises itself. Even set_report's own failure branch (ok_or(HidError::TriggerReset), line 105) is unreachable, since report_len is bounded by the caller.
The mock needs to be able to say both "yes, I have a report" and "no, that failed" before any of this becomes testable.
| Ok(Self { | ||
| w_hid_desc_length: core::mem::size_of::<DeviceDescriptor>() as u16, | ||
| bcd_version: HID_I2C_PROTOCOL_VERSION, | ||
| w_report_desc_length: descriptor.as_bytes().len() as u16, |
There was a problem hiding this comment.
Re w_report_desc_length: descriptor.as_bytes().len() as u16: nothing exercises this arithmetic.
as u16 silently truncates, so a report descriptor of 65536+ bytes advertises a length of 0. No test, no guard.
| w_report_desc_length: descriptor.as_bytes().len() as u16, | ||
| w_report_desc_register: crate::HidI2cRegister::ReportDescriptor as u16, | ||
| w_input_register: crate::HidI2cRegister::Input.into(), | ||
| w_max_input_length: input_max as u16 + report_length_header_size, |
There was a problem hiding this comment.
Re w_max_input_length: input_max as u16 + report_length_header_size (and the same shape on line 154): nothing exercises this arithmetic either.
Two distinct failure modes. input_max as u16 truncates, and the u16 + u16 that follows panics on overflow in debug and wraps in release. A HidDevice declaring InputReportMaxSize = U65535 is enough to reach it. Untested in both directions.
| let size_bytes = report.data().len() as u16 | ||
| + device_descriptor::HID_REPORT_HEADER_SIZE_BYTES | ||
| + if report_ids_implicit { | ||
| 0 | ||
| } else { | ||
| device_descriptor::HID_REPORT_ID_SIZE_BYTES | ||
| }; |
There was a problem hiding this comment.
Re report.data().len() as u16 + HID_REPORT_HEADER_SIZE_BYTES + ...: untested, and it has two distinct failure modes.
as u16 truncates, so a report longer than 65535 bytes produces a nonsense length header. And the u16 + u16 chain that follows panics on overflow in debug and wraps in release — report.data().len() == 65535 is enough.
Nothing in the suite can reach it regardless: the mock's has_pending_input_report() returns false, so this entire block is dead code in every test.
| let len_header = (report.data().len() as u16 + device_descriptor::HID_REPORT_HEADER_SIZE_BYTES) | ||
| .to_le_bytes(); |
There was a problem hiding this comment.
Same problem here — report.data().len() as u16 + HID_REPORT_HEADER_SIZE_BYTES truncates and can overflow.
This is also the line 8a308e42 rewrote. It previously added HID_REPORT_ID_SIZE_BYTES and emitted the report ID in the header for explicit-ID devices, which is what reply_with_input_report (510-526) still does and what device_descriptor.rs:152 still advertises. Those three currently disagree; whichever is right, all three should agree and a test should pin it.
| async fn process_command( | ||
| data: &[u8], | ||
| bus: &mut TimeoutBus<Bus>, | ||
| hid_device: &mut HidDevice, | ||
| ) -> Result<(), Error<Bus::Error>> { |
There was a problem hiding this comment.
This function welds parsing to I/O, and that is the root cause of most of what is wrong with the tests.
Concretely: to assert that 0x23 means "Output report, ID 3" you currently need a TimeoutBus, a NoopPin, a RecordingHidDevice, a tokio runtime, and critical-section/std + embassy-time/std with a global timer queue. That is roughly 260 lines of mock and three dev-dependencies to check byte decoding.
Compare HidI2cReportCommandHeader::try_from_command_byte — a pure u8 -> Result<Header, ProtocolError>. Its three tests need no bus, no pin, no device and no runtime, and they are the only cheap tests in this PR.
Extracting the frame parse into something like:
fn parse(frame: &[u8], framing: ReportFraming) -> Result<SetReportCommand<'_>, ProtocolError>would make the four currently-failing tests sync and mock-free, and would let proptest assert "never panics on arbitrary input" — precisely the property nobody can verify today.
Two type-level consequences worth taking at the same time:
- Splitting
HidI2cReportTypeintoGetReportType/SetReportTypedeletes theTryFrom<HidI2cReportType> for GetHidReportTypeimpl (60-70), theInput => return Err(InvalidReportType)arm (660-663), and both tests guarding them. Neither case stays representable. report_ids_implicit: booldrives a header-size calculation at 432, 438, 510 and 650 here, plusdevice_descriptor.rs:140— five hand-written copies of "2, or 3 if explicit", and two of them disagree. That disagreement is the SET_REPORT bug above. AReportFramingtype owningheader_len()makes the arithmetic exist once.
Context for the reasoning: https://balbi.sh/posts/making-smaller-things/
|
FYI, pretty much all of my comments are fixed in a PR I added against this PR. For those interested, here it is: jerrysxie#1 |
## Problem `basic::event_receiver::test::test_recovery_timeout` fails intermittently: **6 failures in 10 consecutive runs** of the same unchanged binary. CI runs `cargo test --locked` from the workspace root and cargo fails fast, so a flake here truncates the rest of the workspace suite on unrelated pull requests. ## Why the old assertion was wrong The test asserted real wall-clock timing with roughly 10% of headroom, capping each successful event with a `with_timeout` ceiling just above the nominal interval: | Constant | Nominal | Ceiling | | --- | --- | --- | | `RECOVERY_ENTRY_MAX_TIMEOUT` | 1000 ms | 1100 ms | | `RECOVERY_TICK_MAX_TIMEOUT` | 100 ms | 110 ms | Under `std`, embassy-time is driven by a detached OS thread using `std::time::Instant` and `Condvar::wait_timeout`. On a loaded host those timers overshoot and trip the ceiling. That ceiling was never a property of the code under test, only of the host's load. Two details worth flagging for review: - The traceback in #962 actually points at the **1100 ms recovery-entry ceiling** (ten chained 100 ms ticks), not the 110 ms tick ceiling the issue text blames. - `RECOVERY_ENTRY_MAX_TIMEOUT` was in use at **two sites with opposite polarity** — once as an idle dwell asserting `Err(TimeoutError)`, and once as a ceiling asserting `Ok(Event::RecoveryTick)`. Raising it in place would have weakened the idle check, so it had to be split rather than simply widened. ## The change Upper bounds become generous liveness guards that still catch a stuck or absent timer: - recovery entry: `5 s` (`RECOVERY_ENTRY_LIVENESS_TIMEOUT`) - individual tick: `1 s` (`RECOVERY_TICK_LIVENESS_TIMEOUT`) Lower bounds are unchanged. They carry the real behavioural guarantee — that the timer must **not** fire early: - recovery entry: `>= 1000 ms` - per tick: `>= 90 ms` (`MINIMUM_RECOVERY_TICK_DELAY`) The idle dwell stays at 1100 ms, renamed `IDLE_NO_EVENT_DWELL`: there the value is a dwell time rather than a ceiling, and raising it would only slow the test. Test-only change: 1 file, +17/-15, entirely inside `#[cfg(test)] mod test` in `cfu-service/src/basic/event_receiver.rs`. No production code is touched, and there are no dependency, `Cargo.toml`, or `Cargo.lock` changes. ## Why coverage survives The test still proves that: - no recovery event is emitted while idle (unchanged 1100 ms dwell asserting `Err(TimeoutError)`); - recovery is not entered before 1000 ms; - an individual tick is not emitted before 90 ms; - the state transitions to `FwUpdateState::Recovery` and stays there; - events actually arrive at all — a hung or missing timer still fails the test, just at 5 s / 1 s instead of 1.1 s / 110 ms. The only property no longer asserted is an upper bound on lateness, which was precisely the non-deterministic part. ## Rejected alternative: mock / paused clock The issue's own first preference was a mock clock. It was investigated and rejected with evidence: - `tokio::time::pause()` does not drive embassy-time's `driver_std`, so pausing tokio's clock has no effect on the timers under test. - embassy's `MockDriver` collides with `embassy-time/std` on the `_embassy_time_now` `no_mangle` symbol. Six other workspace crates force `embassy-time/std` in dev-dependencies, and workspace feature unification makes that collision unavoidable from within `cfu-service`. Adopting a mock clock would therefore require changing dev-dependency features across the workspace, which is well beyond the scope of a flake fix. ## Verification Run on the host at `6853806`, after the change: - `cargo test --locked -p cfu-service` — `5 passed; 0 failed; 0 ignored` - Issue reproduction, ten consecutive runs of `cargo test --locked -p cfu-service --lib test_recovery_timeout` — **10 passed / 0 failed**, each reporting `test result: ok. 1 passed; 0 failed` (the same loop measured 6 failures before the change) - `cargo fmt --check` — clean, exit 0 - `cargo clippy --locked -p cfu-service --all-targets` — clean, exit 0, no warnings Resolves #962 References #951
Add unt test for hidi2c target service
device_descriptor.rsservice.rstest_support.rs