Repository navigation
fix(hid): decode Contour ShuttleXpress/ShuttlePro v2 buttons from bytes 3-4. Principle XI. - #5931
Conversation
…es 3-4. Principle XI. Both parsers assumed the button bitmask lived in bytes 2-3. A raw capture from a ShuttleXpress shows buttons 1-4 in byte 3 bits 4-7 and button 5 in byte 4 bit 0 (the ShuttlePro v2 button 5-9 positions), so only physical button 1 was seen, reported as button 5. ShuttlePro v2 read byte 2 (always 0) as the low byte, shifting buttons 1-8 to 9-16 and never reading 9-15. - ShuttleXpress: pack (buf[3] >> 4) | (buf[4] & 1) << 4 into bits 0-4. - ShuttlePro v2: buf[3] | buf[4] << 8. - Require len >= 5 for both: m_buf is reused across reads, so a short report would otherwise decode a stale buf[4]. - Fix the layout comments. - Add hid_device_parser_test with the captured reports; it fails 3 checks on the old parser and passes on the fix. Fixes aethersdr#5927 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Welcome to AetherSDR, @sq9fk! 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) |
jensenpat
left a comment
There was a problem hiding this comment.
Verdict
No code blockers. Reviewed 4c908a50825bef7e3c1e89336bbfb191c9a2bde5. This is the right fix for #5927. A clean merge recommendation waits on the CI run that was sitting in action_required and is now in progress. This comment is not an approving review.
Issue fit
#5927 asks for all five ShuttleXpress buttons to report as 1–5. The suggested pack is (buf[3] >> 4) | ((buf[4] & 0x01) << 4), with len < 5. The triage note on the issue also asked for the same guard on ShuttlePro v2 (buf[3] | (buf[4] << 8)), corrected layout comments, and a parser test from the captured reports. This PR does those and nothing else.
Scope
| File | Change |
|---|---|
src/core/HidDeviceParser.cpp |
Xpress and Pro v2 button bytes, len < 5, comments |
tests/hid_device_parser_test.cpp |
Captured Xpress reports, short report, jog/wrap, inferred Pro 1–15 |
tests/tests.cmake |
hid_device_parser_test registered. No socket, no AETHER_SETTINGS_CONSUMERS entry (the test does not touch settings) |
Shared HID parsing only. No radio command, no family branch, no settings key.
Verification
hid_device_parser_test was compiled and run from #5933 (4be1554c), which contains this parser change. All of this PR's checks passed there. Replacing the Xpress mask with the old buf[3] read failed the button checks and left the jog and Pro checks passing. The tree was restored and the test passed again.
The test does not open a socket. CI had not run (first-contributor action_required); those workflows are approved and running on this SHA. The per-PR gate does not execute a new ctest, so the local run is the coverage for this target.
Stack
#5932 and #5933 are stacked on this commit. Squashing this PR onto current main and then squashing #5932 conflicts in HidDeviceParser.cpp and tests/hid_device_parser_test.cpp. The three changes land cleanly as one squash of #5933. I will do that once #5933's checks are green, and close this PR as included, rather than merge it out from under the stack.
Non-blocking: ShuttlePro v2 is still inferred from the Xpress capture, as the issue says. No hardware here.
jensenpat
left a comment
There was a problem hiding this comment.
Approving 4c908a50825bef7e3c1e89336bbfb191c9a2bde5. CI is green on this SHA: build, check-macos, check-windows, Static checks, and Sanitizer option configures. The earlier comment stands for the parser fix and the local hid_device_parser_test run.
Landing this PR on its own. #5933 is blocked on static checks, so the stack tip is not the merge. #5932 will need a rebase onto main after this squash; squashing it as-is conflicts in the parser and the new test.
Summary
Fixes #5927.
Both Contour parsers read the button bitmask from the wrong bytes. A raw
hid_read()capture from a ShuttleXpress (VID 0B33 / PID 0020) shows buttons 1–4 inbuf[3]bits 4–7 and button 5 inbuf[4]bit 0. These are the ShuttlePro v2 button 5–9 positions. Because the parser only scanned bits 0–4 ofbuf[3], only physical button 1 was seen, and it was reported as button 5.ShuttleProV2Parserbuilt its mask frombuf[2], which is always 0, andbuf[3], which shifted buttons 1–8 to 9–16 and never read 9–15.btns = (buf[3] >> 4) | ((buf[4] & 0x01) << 4), so the existing scan loop and 1-based numbering stay unchanged.btns = buf[3] | (buf[4] << 8).len >= 5.HidEncoderManager::m_bufis reused across reads, so a short report would otherwise decode a stalebuf[4]as a phantom press.tests/hid_device_parser_test.cpp: registered intests/tests.cmakeand built unconditionally, because the parsers have no hidapi dependency and only needHAVE_HIDAPIfor their#ifdef. It uses the captured reports from the issue:Out of scope, and to be done in a follow-up PR as noted on the issue: when two buttons change in the same report, only the first edge is reported.
Constitution principle honored
Principle XI: the fix is demonstrated.
hid_device_parser_testfails 3 checks when built against the parser frommain(Xpress buttons, short-report guard, Pro v2 buttons) and passes all checks with this change.Test plan
cmake --build build, Windows 11, MSVC 19.44, Qt 6.8.3, Ninja)ctest -R hid_device_parser_testpasses; the same test fails 3 checks on the old parser.Checklist
docs/COMMIT-SIGNING.md)AppSettingscalls (no settings touched)MeterSmoother(no meter UI touched)🤖 Generated with Claude Code