Repository navigation
fix(hid): report every Contour button edge in a multi-change report. Principle XI. - #5932
Conversation
jensenpat
left a comment
jensenpat
left a comment
There was a problem hiding this comment.
Verdict
No code blockers. Reviewed d63eccbbebf69346af6ecd889140b49d145472ed (the new commit on top of #5931). This comment is not an approving review; CI on this SHA was action_required and is now running.
Issue fit
The follow-up named on #5927: both Contour parsers stored the whole new mask after returning the first changed bit, so a second edge in the same report was dropped, including a release. The PR keeps parse() single-return, adds HidDeviceParser::nextPending() (default none), and drains it from HidEncoderManager::poll(). RC-28, PowerMate, StreamDeck+, and TMate 2 are not given a queue. Re-calling parse() would have been wrong for the parsers that emit Rotate on every non-zero rotation byte.
Scope
| File | Change |
|---|---|
src/core/HidDeviceParser.h |
nextPending(); ContourShuttleParser shared by Xpress and Pro v2 |
src/core/HidDeviceParser.cpp |
Queue every button edge, then the jog delta. Short report clears the queue before returning |
src/core/HidEncoderManager.cpp |
Drain parse() then nextPending() |
tests/hid_device_parser_test.cpp |
Simultaneous press/release, byte 3/4 boundary, release+press, button+jog, short report clears the queue |
tests/tests.cmake |
Same registration as #5931 |
The queue is a fixed 17-slot array (16 button bits + jog). The two subclasses report 5 and 15 buttons, so a full report fits. Pro byte 4 is masked to 7 bits, which matches buttons 9–15 and does not change the scan versus #5931's unmasked shift.
Verification
Same local binary as #5931/#5933: hid_device_parser_test passed, including the new simultaneous-edge checks. Forcing the button loop to stop after the first edge failed those checks (the single-button and jog checks still passed). Restored; the test passed again.
No socket. This PR is still a draft. Squashing it after a squash of #5931 conflicts. It lands inside the #5933 squash once that PR's checks are green.
d63eccb to
6acc273
Compare
…Principle XI. The ShuttleXpress/ShuttlePro v2 parsers returned the first changed button bit but stored the whole new mask, so a second button changing in the same report was dropped. That includes releases, which could leave a button logically held. A jog change arriving with a button change was also deferred to the next report. - HidDeviceParser gains nextPending() (default: none). HidEncoderManager drains it after every parse(), so parse() keeps its single-return contract and the other parsers are untouched. - The two Contour parsers share ContourShuttleParser, which queues every button edge in button order and then the jog delta. The subclasses only supply the button mask and count. - hid_device_parser_test: simultaneous press/release, byte-3/4 boundary (Xpress 4+5, Pro 8+9), release+press in one report, button+jog in one report, and a short report clearing the queue. On the aethersdr#5927 parser 6 of these fail; all 13 checks pass here. Verified on a ShuttleXpress + FLEX-6500: two buttons pressed together both fire, and releasing them together leaves nothing held. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
6acc273 to
2d3a44a
Compare
jensenpat
left a comment
jensenpat
left a comment
There was a problem hiding this comment.
Reviewed exact head 2d3a44aee35fd7c18961359512b8ab942474fba2 after the requested rebase onto upstream main 3c81b3d81. No remaining code blockers.
| Scope | Change | Verdict |
|---|---|---|
src/core/HidDeviceParser.{h,cpp} |
Bounded Contour event queue, shared Xpress/Pro decoder | Required for simultaneous edge delivery |
src/core/HidEncoderManager.cpp |
Drain pending events after parse | Shared HID loop; other parsers default to no pending events |
tests/hid_device_parser_test.cpp |
Simultaneous edges, cross-byte masks, button+jog, short-report reset | Socket-free behavioral coverage |
docs/user/docs/usb-control-surfaces.md |
Explain simultaneous inputs | Required user documentation |
Issue fit: follow-up to #5927; #5931 has already landed. No unexplained files or old-main commits remain. No radio backend, DSP/audio, networking, TX gate, workflow, dependency, or settings changes. The common poll loop is the only non-Contour runtime touchpoint.
Validation on this code: built hid_device_parser_test with HID enabled; headless focused CTest passed 1/1. A first-button-only mutation failed five simultaneous-edge checks; restoring the parser passed, and the final-head CTest passed again. Reviewed the 17-slot queue against the maximum 15 button edges plus jog, the length check before byte access, jog wrapping/priming, and default nextPending() behavior for the other device parsers. Generated references are current; log-rule checks passed 707 checks. Hosted CI/Docs are pending and remain merge gates; this approval does not claim they passed.
Bridge boundary: there is no HID-report injection verb, so parser delivery is covered by the socket-free test rather than a simulated USB device. No hardware or TX was exercised. Contributor-reported ShuttleXpress hardware results are historical; ShuttlePro v2 remains unit coverage only. The automated code-review/CodeGuard tools are unavailable in this session; equivalent correctness and input-boundary review was manual.
…iple XI. (#5933) ## Summary Closes #5928. Use the spring-loaded outer ring on Contour ShuttleXpress and ShuttlePro v2 for continuous, variable-speed tuning. Release stops tuning; the jog wheel remains independent. This follows the button-edge delivery fix in #5932. - Decode the signed ring position and emit it on change. Closing or unplugging the controller emits zero before discarding the parser; the GUI also stops on controller disconnect. - Integrate a held position on a 40 ms timer, using elapsed time capped at 200 ms. Rates are in Hz/s, with a minimum step rate for larger tuning steps and a 60 ms first-step delay for measured spring overshoot. - Offer Tune Slice, RIT, XIT, or None and Slow / Normal / Fast under Radio Setup → Serial & Controllers → Shuttle Ring. Store both fields in one `ShuttleMapping` object; reuse the shared invert flag. Tune Slice respects Tune Lock. RIT/XIT are capped at 1 kHz/s and ±9999 Hz, and a held ring does not repeatedly send an unchanged offset at the limit. - Use current Radio Setup theme roles and guarded, accessible combos. Update the user guide and in-app help, including the existing ShuttlePro button-mapping limits. ## Scope The shared HID manager gains ring state/lifecycle and a parameterized JSON helper; RC-28 calls still use the identical `RC28Mapping` key and field semantics. Other device parsers default to no shuttle. Controller dispatch reuses the existing typed tuning paths; no radio backend, protocol encoder, DSP/audio, network transport, TX-keying path, workflow, or dependency changes. `ShuttleRateIntegrator.h` is desktop HID arithmetic. Its `ui-support` tag and regenerated touchpoint row follow the existing maintainer review request; no boundary baseline is raised or vendor tag weakened. ## Validation - macOS app built with Qt 6.12.0 and HID enabled using `cmake --build <build-dir> -j22`. - `hid_device_parser_test`: 1/1 focused headless CTest passed, covering ring decode/clamps, simultaneous inputs, first-step delay, release overshoot, rate/step-size behavior, reversal, multiplier/cap and invalid step size. - Mutation in a disposable source copy: removing the 60 ms delay failed the first-step and 35 ms overshoot checks; restoring it passed. - Isolated, offscreen, TX-disabled `DEMO-0001` bridge session: all four ring actions and Fast speed selection exercised, action/speed survived normal Quit and process restart, accessible names confirmed, layout screenshot inspected. Both owned app instances exited normally. - Engine-boundary findings unchanged apart from line numbers; capability and command-plane findings identical to the base. Touchpoint manifest, generated docs, log-rule checks, and color ratchet passed. Existing a11y warnings are outside the new group. - Hosted Docs CI validates the site; no local site/PDF rebuild. The bridge has no Contour report-injection verb. Physical ring/timer delivery and USB unplug behavior were source-reviewed here; no real controller or radio was exercised in this landing pass. The contributor previously tested a ShuttleXpress with a FLEX-6500 at a 10 Hz step. ShuttlePro v2 and 1 kHz-step behavior have unit coverage only. No TX test was run. Screenshot note: `radio-setup-serial` shows the top of the scrollable page; the new Shuttle Ring group is further down. A focused bridge screenshot was inspected during this pass. Generated with OpenAI Codex (GPT-6 Astra) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> Co-authored-by: jensenpat <patjensen@gmail.com>
2 participants
Summary
Contour ShuttleXpress and ShuttlePro v2 HID reports can contain several button edges and a jog change together. Decode and deliver all of them, in button order followed by the jog delta, so simultaneous presses or releases are not lost.
Related: #5927. #5931 is already on main; this PR contains only the follow-up event-delivery fix.
Validation
3c81b3d81with contributor attribution preserved and signed commits.hid_device_parser_testbuilt with HID enabled; 1/1 focused CTest passed headless.Scope
Shared HID polling gains queue draining; RC-28, PowerMate, StreamDeck+ and TMate 2 parsers are unchanged. No radio backend, audio/DSP, networking, TX gate, workflow, or dependency changes.
Generated with OpenAI Codex (GPT-6 Astra)