fix(hid-rp): gamepad input report data offset for byte-wide axes - #829
Merged
finger563 merged 1 commit intoSep 30, 2026
Merged
Conversation
GamepadInputReport::get_report() and set_data() skipped a fixed two bytes at the front of the object. That is right for the default uint16_t axes (the one-byte report id plus one byte of alignment padding), but with uint8_t axes there is no padding, so the copy started one byte late: X was dropped, every later field moved down a slot, RZ read 0 and the last byte came from past the end of the object. With REPORT_ID 0 there is no id byte to skip at all. Derive the offset from the layout instead: alignof(JOYSTICK_TYPE) after the report id, 0 without one. Add a host test covering the default, byte-wide and no-report-id layouts and the set_data() round trip.
|
✅Static analysis result - no issues found! ✅ |
Contributor
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The offset correction matches the supported object layouts and is covered by focused regression tests.
Review effort: Balanced
Findings: None
What changed in this PR
Fixes gamepad report serialization offsets for byte-wide axes and reports without IDs.
Changes:
- Derives payload offset from report ID presence and joystick alignment.
- Adds host tests for default, byte-wide, no-ID, neutral, and round-trip cases.
| File | Description |
|---|---|
components/hid-rp/include/hid-rp-gamepad.hpp |
Corrects payload offsets in serialization and deserialization. |
components/hid-rp/test/hid_rp_gamepad_host_test.cpp |
Adds regression coverage for affected layouts. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
finger563
approved these changes
Sep 30, 2026
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.
Problem
GamepadInputReport::get_report()andset_data()skip a fixed two bytes at the front of the object. That is correct for the defaultuint16_taxes (the one-byte report id plus one byte of alignment padding beforejoystick_axes), but a report withuint8_taxes has no padding, so the copy starts one byte late:set_data()writes one byte past itWith
REPORT_ID == 0there is no id byte at all, so the offset is wrong there too.Object dump of
GamepadInputReport<12, uint8_t, uint8_t, 0, 255, 0, 255, 1>with sentinel values (01is the id, then X Y Z RZ brake accelerator hat buttons consumer):We hit this building an 8-bit joystick for the Xbox Adaptive Controller: the stick sat at one edge and only moved on one axis.
Fix
Derive the offset from the layout instead of hard-coding it:
data_offset = REPORT_ID != 0 ? alignof(JOYSTICK_TYPE) : 0, used by bothget_report()andset_data(). The defaultuint16_treport is unchanged (its offset is still 2).Test
components/hid-rp/test/hid_rp_gamepad_host_test.cpp, in the same standalone style as the other hid-rp host tests. It checks the byte layout of the default report, a byte-wide report, a byte-wide report with no id, the neutral (centered) report, and theset_data()round trip. Against the current header the byte-wide cases fail on every field; with this change everything passes.hid_rp_3dconnexion_host_testandhid_rp_report_map_host_teststill pass.Not touched here, since no current report uses it: mixed widths (say
uint8_taxes withuint16_ttriggers) would also get padding betweenjoystick_axesandtrigger_axes, whichnum_data_bytesdoes not account for.