Repository navigation
fix(hid): decode ShuttleXpress button bits correctly from payload - #5937
chirataandrei wants to merge 1 commit into
Conversation
|
Welcome to AetherSDR, @chirataandrei! Thanks for your first pull request. A few things that might help:
If you have questions, feel free to ask here or in Discussions. — AetherClaude (automated agent for AetherSDR) 🤖 aethersdr-agent · cost: $19.8735 · model: claude-opus-5 |
The ShuttleXpress parser assumed button states were stored in bits 0-4 of buf[3]. Raw HID captures show buttons 1-4 are actually reported in buf[3] bits 4-7 (0x10, 0x20, 0x40, 0x80) and button 5 is in buf[4] bit 0 (0x01). Shift and combine the bits from buf[3] and buf[4] so buttons 1-5 map consistently to bit positions 0-4. Fixes aethersdr#5927
0005fac to
06b2f81
Compare
jensenpat
left a comment
jensenpat
left a comment
There was a problem hiding this comment.
Verdict
Request changes. Reviewed 06b2f81691db7cc43ca80c20086632123af50d6f. This does not fix #5927, and it must not merge. #5931 is the fix for the same issue.
Scope
| File | What changed |
|---|---|
src/core/HidDeviceParser.cpp |
ShuttleProV2Parser::parse only. len < 5, and the button word replaced with the ShuttleXpress 5-button pack. |
ShuttleXpressParser::parse is unchanged. It still reads btns = buf[3] and still accepts a 4-byte report.
Findings
The hunk is on ShuttleProV2Parser, not ShuttleXpressParser. The captures in #5927 are ShuttleXpress reports (buf[3] bits 4–7 and buf[4] bit 0). Those bytes are still ignored by the parser the issue names, so buttons 2–5 stay dead and button 1 is still reported as button 5.
On the Pro parser the same expression is the wrong layout. Pro buttons are buf[3] (1–8) and buf[4] bits 0–6 (9–15), which is what #5931 implements. This change narrows btns to uint8_t and shifts those bits into the Xpress packing, while the loop still scans 15 buttons. A Pro would fire the wrong keys, and buttons 9–15 still cannot be represented.
The commit is also unverified (unknown_key). #5931's commit is verified and covers both parsers, the short-report guard, and hid_device_parser_test.
No local build. The defect is the diff itself.
|
|
||
| uint8_t jog = buf[1]; | ||
| uint16_t btns = static_cast<uint16_t>(buf[3] << 8 | buf[2]); | ||
| uint8_t btns = static_cast<uint8_t>((buf[3] >> 4) | ((buf[4] & 0x01) << 4)); |
There was a problem hiding this comment.
This edit is inside ShuttleProV2Parser::parse, and ShuttleXpressParser::parse above is untouched. #5927 is the Xpress layout: buttons 1–4 are buf[3] bits 4–7 and button 5 is buf[4] bit 0, read by the Xpress parser as bits 0–4. Packing that way here does not change the parser the report is about.
It also breaks the Pro decoder this function owns. A 15-button mask has to stay buf[3] | (buf[4] << 8) (bits 0–6 of byte 4). An 8-bit value cannot carry buttons 9–15, and the shift maps the Pro's low buttons onto the Xpress's button 5–9 slots. Please drop this PR in favor of #5931.
|
Thanks for submission. This is a duplicate of #5931. |
2 participants
The ShuttleXpress parser assumed button states were stored in bits 0-4 of buf[3]. Raw HID captures show buttons 1-4 are actually reported in buf[3] bits 4-7 (0x10, 0x20, 0x40, 0x80) and button 5 is in buf[4] bit 0 (0x01).
Shift and combine the bits from buf[3] and buf[4] so buttons 1-5 map consistently to bit positions 0-4.
Fixes #5927
Summary
Constitution principle honored
Test plan
cmake --build build)Checklist
docs/COMMIT-SIGNING.md)AppSettingscalls — use nested-JSON-under-one-key(Principle V)
reverse-engineered from a proprietary binary (Principle IV)
MeterSmoother(AGENTS.md convention)docs/and theaffected READMEs. Not
CHANGELOG.md, which is a release-prep file aPR must not add to (AGENTS.md); describe it in the Summary above instead