Skip to content

hid: fix bug in report parsing when reports are not declared contiguously - #936

Merged
williampMSFT merged 1 commit into
OpenDevicePartnership:mainfrom
williampMSFT:user/williamp/hid-aggregate-multireport-fix
Aug 5, 2026
Merged

hid: fix bug in report parsing when reports are not declared contiguously#936
williampMSFT merged 1 commit into
OpenDevicePartnership:mainfrom
williampMSFT:user/williamp/hid-aggregate-multireport-fix

Conversation

@williampMSFT

Copy link
Copy Markdown
Collaborator

#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.

@williampMSFT
williampMSFT marked this pull request as ready for review August 5, 2026 20:49
@williampMSFT
williampMSFT requested a review from a team as a code owner August 5, 2026 20:49
@williampMSFT
williampMSFT requested review from RobertZ2011, felipebalbi, jerrysxie and tullom and a lite review from Copilot August 5, 2026 20:49

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Summary of changes

This PR fixes a correctness bug in HID report-descriptor parsing where report fields for the same Report ID can appear in multiple non-contiguous regions of the descriptor. Instead of treating “switching report IDs” as ending the current report and only tracking a running max, the parser now accumulates bit sizes per Report ID across the entire descriptor. This aligns the implementation with legal HID descriptor patterns where a report ID can be revisited and extended later. New unit tests cover both non-contiguous Report ID re-entry and Push/Pop restoring a prior report ID.

Changes:

  • Track accumulated Input/Output/Feature bit counts per Report ID while parsing, then compute the max at the end.
  • Remove the previous “flush on report-id switch / pop-to-different-id” logic that incorrectly split reports.
  • Add tests for non-contiguous Report ID declarations and for Report ID restoration via Pop.

Step-by-step review guide

  1. Parsing logic change: accumulate per Report ID

    • The constructor now maintains a reports table keyed by state.report_id, and adds each Main item’s computed bits to the appropriate entry.
    • This matters because HID descriptors can legally interleave report IDs; the final report size must reflect the sum of all fields for that ID, regardless of contiguity.
  2. Push/Pop semantics

    • Pop now simply restores the prior GlobalItemState without forcing a “report boundary.”
    • This is important because Push/Pop can restore a prior report ID and continue adding fields to the same report, which should be accumulated rather than treated as a new report.
  3. Final max computation

    • After parsing, the code iterates across all tracked reports and computes the maximum input/output/feature payload sizes.
    • This preserves the intended API contract: max_report_sizes() returns the maximum payload size across all report IDs (excluding the report ID byte), now with correct aggregation.

Potential issues

No issues found.

# Severity File Description Code
No issues found.

Comment thread embedded-service/src/relay/hid.rs
Comment thread embedded-service/src/relay/hid.rs
Comment thread embedded-service/src/relay/hid.rs
@williampMSFT
williampMSFT merged commit 34d85e3 into OpenDevicePartnership:main Aug 5, 2026
16 checks passed
@williampMSFT
williampMSFT deleted the user/williamp/hid-aggregate-multireport-fix branch August 5, 2026 22:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants