Implement HID report aggregation - #933
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a HID “aggregation” facility that can combine multiple HidDevice implementations into a single composite HidDevice, including runtime report-descriptor concatenation and report-ID renumbering/remapping. To support that, it extends the HidDevice trait with a compile-time upper bound on descriptor size, and enhances report-descriptor parsing so the system can compute declared max report payload sizes. It also updates HID-I2C target service initialization to validate that a device’s declared report sizes fit within its type-level maximums, and provides a new RT685s EVK example that composes the mock mouse + keyboard into one HID-I2C endpoint.
Changes:
- Introduce
impl_hid_aggregate_device!plus supporting descriptor parsing/combining + report-id mapping utilities inembedded-service. - Extend
HidDevicewithMAX_DESCRIPTOR_LENand update mock HID relays accordingly. - Make HID-I2C
DeviceDescriptor::newfallible and propagate a structuredDeviceDescriptorErrorwhen descriptor-declared report sizes exceed type-level bounds.
Step-by-step review guide
-
New descriptor parsing and sizing (
HidReportDescriptor::new,max_report_sizes)- The report descriptor is now parsed into items (short + long) to compute max payload sizes for Input/Output/Feature reports (excluding the report ID byte).
- This enables later validation in the HID-I2C device descriptor path and supports safe buffer sizing decisions.
-
Descriptor combination + report-ID remapping (
ReportIdMap::combine)- Multiple child descriptors are combined by wrapping each input in a Push/Pop pair to prevent global-item state leakage, and by rewriting/inserting Report ID items so the combined device always has explicit, globally unique report IDs.
- A
ReportIdMapcaptures host-ID ⇄ (owner, native-ID) routing soget_report,set_report, and unsolicited input reports can be forwarded/relabelled correctly.
-
Aggregate device macro (
impl_hid_aggregate_device!)- Generates an aggregate
HidDevicetype, resources (descriptor buffer), descriptor construction logic, and routing glue that relabels report IDs on the way in/out. - Uses
select_arrayforwait_for_input_reportwith an explicit drop-safety rationale, and avoids drop-safety pitfalls forprocess_next_input_reportby first selecting a concrete child index.
- Generates an aggregate
-
HID-I2C integration: validating report sizes (
DeviceDescriptorError)hidi2c-target-servicenow checks that the descriptor-declared max payload sizes do not exceedHidDevice::{Input,Output,Feature}ReportMaxSize.- Service construction now propagates failure instead of assuming the device descriptor is always valid.
Potential issues
| # | Severity | File | Description | Code |
|---|---|---|---|---|
| 1 | Medium | embedded-service/src/relay/hid.rs:768-805 |
For implicit descriptors, a Report ID is only inserted upon encountering a Collection item. If an implicit descriptor has no Collection items (currently permitted by the parser), the combine step can succeed without inserting/recording any ID for that input, yielding an unusable routing map for that child. | if implicit && !report_id_inserted && item.header.is_collection() { ... } |
| 2 | Low | hidi2c-target-service/src/device_descriptor.rs:93-104 |
DeviceDescriptorError docs refer to “actual” bytes, but the field is named declared, which is confusing for callers and for log/debug output. |
InputReportTooLarge { declared: usize, max: usize } |
Reviewed changes
Copilot reviewed 11 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| hidi2c-target-service/src/service.rs | Propagates fallible device-descriptor construction during service init. |
| hidi2c-target-service/src/lib.rs | Re-exports the new DeviceDescriptorError to make the new failure mode consumable. |
| hidi2c-target-service/src/device_descriptor.rs | Adds report-size validation and a structured error type for invalid descriptors. |
| embedded-service/src/relay/hid.rs | Implements descriptor parsing, max-size computation, report-ID combining/remapping, and impl_hid_aggregate_device!. |
| embedded-service/src/lib.rs | Re-exports macro internals (embassy_futures, typenum) needed by the new macro. |
| embedded-service/Cargo.toml | Adds heapless/typenum dependencies used by descriptor parsing and macro type-level max. |
| Cargo.lock | Updates lockfile for new workspace dependency usage. |
| examples/rt685s-evk/src/mocks/mouse/relay.rs | Updates mock mouse relay to provide MAX_DESCRIPTOR_LEN (and adjusts report count). |
| examples/rt685s-evk/src/mocks/keyboard/relay.rs | Updates mock keyboard relay to provide MAX_DESCRIPTOR_LEN. |
| examples/rt685s-evk/src/bin/mock_i2c_mouse.rs | Removes now-obsolete “missing macro” comments. |
| examples/rt685s-evk/src/bin/mock_i2c_keyboard.rs | Removes now-obsolete “missing macro” comments. |
| examples/rt685s-evk/src/bin/mock_i2c_mouse_keyboard.rs | Adds a new combined mouse+keyboard HID-I2C example using impl_hid_aggregate_device!. |
| examples/std/Cargo.lock | Lockfile update for dependency additions used by the HID aggregation facility. |
| examples/rt685s-evk/Cargo.lock | Lockfile update for dependency additions used by the HID aggregation facility. |
| examples/pico-de-gallo/Cargo.lock | Lockfile update for dependency additions used by the HID aggregation facility. |
Suppressed comments (2)
hidi2c-target-service/src/device_descriptor.rs:100
- The doc comment says the largest report is measured in
actualbytes, but the error payload field is nameddeclared. Either rename the field toactual, or update the docs to match the current field name to avoid confusion for callers.
/// The HID device returned an output report descriptor whose largest report (`actual` bytes)
/// is larger than the device's `OutputReportMaxSize` (`max` bytes).
OutputReportTooLarge { declared: usize, max: usize },
hidi2c-target-service/src/device_descriptor.rs:104
- The doc comment says the largest report is measured in
actualbytes, but the error payload field is nameddeclared. Either rename the field toactual, or update the docs to match the current field name to avoid confusion for callers.
/// The HID device returned a feature report descriptor whose largest report (`actual` bytes)
/// is larger than the device's `FeatureReportMaxSize` (`max` bytes).
FeatureReportTooLarge { declared: usize, max: usize },
}
| // pending report — the winning child's report stays queued until it is drained by | ||
| // `process_next_input_report`. | ||
| let _ = $crate::_macro_internal::embassy_futures::select::select_array( | ||
| self.children.each_mut().map(|child| child.wait_for_input_report()), |
There was a problem hiding this comment.
If we do have an unaccounted report ID from a child, passing it through might cause a malformed HID report. If we don't want to panic, a malformed HID report might cause the OS to kill this HID driver. I guess that is acceptable.
…usly (#936) #933 had a bug: it was not correctly handling the case where a report ID gets 'revisited', but it turns out it's legal to go back to an earlier report ID and keep adding to it, e.g. - Report ID = 1 - declare 2 bytes - Report ID = 2 - declare 3 bytes - Report ID = 1 - declare 2 bytes means that report ID 1 is a 4-byte report. This changes our parsing logic to account for this; we have to track each individual report instead of just the largest observed so far.
Introduced a new facility for aggregating a collection of HidDevice implementations into a single HidDevice implementation, including introducing and/or renumbering report IDs as necessary:
impl_hid_aggregate_device!. This is the HID analogue to theimpl_odp_mctp_relay_handler!macro.This required extending the HidDevice trait slightly so that subdevices can express the size of their report descriptor as part of their type, which allows the aggregation macro to correctly size the combined report descriptor buffer.
Additionally, added an example that combines the mock mouse and keyboard, and removed comments in the single-device macros indicating that this functionality was not yet implemented.
Resolves #840.
Resolves #838.