Repository navigation
feat(hid): variable-speed tuning from the Contour shuttle ring. Principle XI. - #5933
Conversation
jensenpat
left a comment
There was a problem hiding this comment.
Verdict
No code blockers. Reviewed 4be1554cb841f3c00ab813fd8978fe4077c1f267. This is the stack tip: #5927's button decode, the simultaneous-edge drain, and #5928's shuttle ring. A clean merge recommendation waits on CI for this SHA (approved out of action_required, now running). This comment is not an approving review.
Issue fit
#5928 asks for the spring ring as a rate input. The triage on that issue asked for five adjustments, and this PR follows them:
- Rate table in Hz/s (
{0, 20, 100, 500, 2k, 8k, 30k, 100k}), not steps/s, so full deflection is about 100 kHz/s at any step size. A(|pos|+1)steps/s floor keeps a 1 kHz step moving. - Tune Lock is checked in the tick and
notifyTuneBlockedByLock()runs once per deflection, so direct entry is not cancelled on every tick. close()emitsshuttleChanged(0)before the parser is destroyed, and not behind the device-name guard.connectionChanged(false)also stops the timer.- Direction uses the existing
HidEncoderInvertDirflag. There is no second invert inShuttleMapping. - The action list is Tune Slice, RIT, XIT, or None. RIT/XIT are capped at 1 kHz/s. Power and volume are not offered.
Settings are one ShuttleMapping JSON key (Principle V), through the same helper as RC28Mapping. The ring path calls applyFlexControlWheelAction(), so it does not add a command-plane string. The new Radio Setup group sits inside the existing HAVE_HIDAPI block.
The 60 ms first-step delay is the one place this disagrees with the triage suggestion to leave that filter out. The suggestion assumed a pure integrator, where a 35 ms overshoot is a fraction of a step. This PR also applies one step when a new deflection has been held, so the delay has to be longer than the measured 30–35 ms snap-back or letting go steps backwards. That is the right call, and the test covers it. Auto-snap on release is omitted so the shared RC-28 snap timer is not restarted. That matches the triage note.
Scope
| File | Change |
|---|---|
src/core/HidDeviceParser.{h,cpp} |
#5931 button bytes, #5932 queue, shuttle position set at the top of parse() and clamped to ±7 |
src/core/HidEncoderManager.{h,cpp} |
Drain pending events; shuttleChanged; ShuttleMapping; return to 0 on close() |
src/core/ShuttleRateIntegrator.h |
Header-only Hz/s integrator |
src/gui/MainWindow.h, MainWindow_Controllers.cpp |
40 ms timer, elapsed dt capped at 200 ms, lock-once, stop on disconnect |
src/gui/RadioSetupDialog.cpp |
Shuttle Ring group (Action, Speed), inside HAVE_HIDAPI |
resources/help/configuring-aethersdr-controls.md |
Ring documented |
CMakeLists.txt |
Header listed with the other HID sources |
tests/hid_device_parser_test.cpp, tests/tests.cmake |
Parser, queue, ring decode, integrator |
Client HID only. Every radio family that already accepts the FlexControl wheel actions gets the same frequency / RIT / XIT setpoints. Nothing here keys transmit. The action list excludes RF power and volume.
Verification
Compiled tests/hid_device_parser_test.cpp + src/core/HidDeviceParser.cpp with -std=c++20 -DHAVE_HIDAPI from this SHA and ran it: all checks passed (buttons, queue, ring decode, integrator).
Three mutations, each rebuilt, each a behavioral failure, each restored, final run passed:
- Xpress mask forced back to
buf[3]: the Xpress button checks failed. - Button loop stopped after the first edge: the simultaneous-button checks failed.
kFirstStepDelaySecset to 0: the 60 ms first-step check and the 35 ms overshoot check failed.
The test does not open a socket. The app was not built locally. GUI compile is what the CI workflow on this SHA is for. The per-PR ctest gate will not run this new target.
Nit, not a change request: the help line says the ring "moves one step straight away". The first step is applied once the deflection has been held 60 ms, on the second 40 ms tick when the timer is on time.
Landing
This branch merges clean onto current main (06b6ac95). Squashing #5931 first and then this PR does not. Once this SHA's checks are green I will mark the draft ready and squash this PR, with Fixes #5927 and Closes #5928 in the squash body, and close #5931 and #5932 as included.
Non-blocking follow-up, not a merge condition: HidEncoderManager's emit-on-change / emit-0-on-close path and the MainWindow timer (lock-once, settings read once per deflection, stop on disconnect) are not in hid_device_parser_test. A socket-free test can drive ShuttleRateIntegrator (already done) but not the Qt timer without a GUI harness. No firmware peer is useful here.
jensenpat
left a comment
There was a problem hiding this comment.
Verdict
Request changes. Reviewed 4be1554cb841f3c00ab813fd8978fe4077c1f267. The ring behavior described in the earlier comment still looks right, and hid_device_parser_test passed here, including the three mutations. Static checks on this SHA failed, so this is not mergeable.
Run: https://github.com/aethersdr/AetherSDR/actions/runs/35912059810
Two failures, both from this diff against its base (9f81dc00), not from main moving:
-
tools/gen_touchpoint_manifest.py --check—core/ShuttleRateIntegrator.hhas no semantic tag.MainWindow.hincludes it, so it is a gui→engine touchpoint. Tag itui-supportindocs/architecture/aetherd-touchpoint-tags.json(same class asHidEncoderManager.h: client HID plumbing, not radio state) and regenerate withpython tools/gen_touchpoint_manifest.py. The check stopped on the missing tag before it could report a stale manifest; the new row has to be generated too. -
Hardcoded-colour ratchet —
setStyleSheetcall sites 1043 > 1040 (+3). Unique colours did not rise. The three new calls are the Shuttle Ring group, its note, and the combo insideaddCombo. CopyingsetStyleSheet(kGroupStyle)from the older groups on this page is what the ratchet counts.ThemeManager::applyStyleSheetis the call that does not add a site (see the comment onmakeValueLabelin this file).
#5931 and #5932 are not part of this failure. Their static checks passed. This PR stays draft until the two checks are fixed.
|
Thanks for this — the shuttle ring as a rate input rather than a step input is the right model, and the 30–35 ms snap-back measurement behind Static checks failed at two steps. The three build jobs ( 1.
|
…comments. Principle VIII. - Tag core/ShuttleRateIntegrator.h as ui-support (same class as HidEncoderManager.h) and regenerate aetherd-touchpoints.md: the new row, HidEncoderManager.h includers 2 -> 3, totals +1. - Shuttle Ring group styles its group, note and combos through ThemeManager::applyStyleSheet, so the setStyleSheet ratchet stays at +0. - Help: the first step lands after about 60 ms, not "straight away". - Comments condensed to the AGENTS.md comment policy (aethersdr#6072); the hardware measurements behind kFirstStepDelaySec live in aethersdr#5933. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
4be1554 to
7e9110f
Compare
|
Rebased onto current
In
Checked locally before pushing:
Both #5932 and #5933 report mergeable again. Ready for CI whenever you are. |
|
The build jobs on I need to correct my previous comment: the "full Windows build" I reported had
Sorry for the extra CI round. |
Ozy311
left a comment
There was a problem hiding this comment.
Re-review of the changes since @jensenpat's request-changes review at 4be1554c (2026-09-26). Head is now 123e6b18, rebased.
Both blockers from that review are fixed.
- Touchpoint tag.
7e9110fftagscore/ShuttleRateIntegrator.hasui-supportinaetherd-touchpoint-tags.json, the same class asHidEncoderManager.h, and regenerates the manifest.Static checksis green on this head. - Colour ratchet. The Shuttle Ring group, its note and its combos now go through
ThemeManager::applyStyleSheet.123e6b18also switches the combos from the removedkEditStyletokEditStyleTemplate. That one would only have failed to compile in aHAVE_HIDAPIbuild, so it is a good catch. I re-rantools/audit_colours.py --strictagainst currentmainon the merged tree:setstylesheet 816 (+0),unique_colours +0,total_references +0.
The help-text nit ("moves one step straight away" → after about 60 ms) is also taken.
Range-diff against the reviewed series: the #5931 button-decode commit has dropped out, presumably because it landed on main separately. The two remaining commits were rebased, and the two new commits above are the only additions. Every file the new commits touch is one the original review covered; no new files and no new public surface.
One thing needed before merge: a conflict in a generated file
The branch is 59 commits behind main, and it no longer merges cleanly. The only conflict is docs/architecture/aetherd-touchpoints.md, which is generated. I checked the fix in a scratch merge: take either side, then run python tools/gen_touchpoint_manifest.py. --check then reports "manifest up to date", and the new row reads:
| `core/ShuttleRateIntegrator.h` | 1 | ui-support — pure shuttle-ring rate arithmetic for the desktop HID surface, no radio state | unconverted |
Everything else auto-merges. A merge from main with a regenerated manifest would also give CI a current merge base, since the green checks are from 2026-10-07.
Verified
tests/hid_device_parser_test.cpp+HidDeviceParser.cppbuilt standalone with-std=c++20 -DHAVE_HIDAPIon the PR merged into currentmain: all checks passed.- Touchpoint
--checkand the colour ratchet on the merged tree, as above. - Not done: the GUI build and anything with real Contour hardware.
No new findings beyond the conflict. Once it is resolved, I see nothing left from the earlier review.
73, K6OZY
Generated by Claude Code
|
Thanks for the re-review. Merged current Checked locally on the merged tree:
The PR is now mergeable again, and CI is running on the new head. |
There was a problem hiding this comment.
Thanks for this — the PR body is one of the more honest ones I've reviewed: it maps all five triage corrections, discloses what it didn't do (auto-snap, the 60 ms reversal filter), separates hardware-verified claims from unit-test-only ones, and says which step size the hardware log actually exercised. I went after it hard and the DSP/rate/parser core held up. One blocker, and it's an environmental one that landed under you after you branched.
1. Issue fit
Yes, with two disclosed reductions. #5928's five-part design is implemented, and the triage comment's five corrections are all honoured as claimed — I verified each against the tree rather than taking the table at its word:
| Triage point | Verified |
|---|---|
| 1. Hz/s rate table | ShuttleRateIntegrator.h:24-25 is in Hz/s; tick() divides by stepHz and carries m_remainderHz. MainWindow_Controllers.cpp:702 derives unitHz from sw->stepSize() — the same expression applyFlexControlWheelAction uses at its WheelFrequency arm, so the two cannot drift. |
| 2. Tune Lock | MainWindow_Controllers.cpp:688-699 checks isLocked() in the tick and gates notifyTuneBlockedByLock() behind m_shuttleLockNotified. The triage's concern was VfoWidget's cancelDirectEntry() firing 25×/s; once-per-deflection fixes exactly that. |
| 3. Manager-side stop | HidEncoderManager.cpp:585-590 emits before m_parser.reset() and outside the m_deviceName guard, as asked. The connectionChanged(false) → stopShuttle() second line of defence is at MainWindow_Controllers.cpp:3067-3071. |
| 4. Single invert | Confirmed: only m_invertDirection / HidEncoderInvertDir, no second flag. |
| 5. Restricted actions | MainWindow_Controllers.cpp:689-712 is an explicit allow-list with return on anything else — WheelPower is unreachable. Good call; this is the TX/equipment-safety half of the triage point and it's done properly. |
Reductions, both stated in the body and neither a problem: no auto-snap-on-release (you explain the shared m_hidSnapTimer reason), and invert piggybacks on the global flag rather than living in the Shuttle Ring group. The one thing worth a line in the body is that the Shuttle Ring group therefore has no visible invert control at all — triage point 4 offered "reuse the global flag or state the precedence explicitly in the UI label", and neither the group nor the help page mentions that the ring's direction follows the HID encoder's invert setting. That's documentation, not code.
2. Scope
| File / group | What it changes | Claimed? | Verdict |
|---|---|---|---|
src/core/ShuttleRateIntegrator.h |
New header, pure rate arithmetic | Yes | In scope. Referenced from MainWindow.h:83 and tests/, and registered in CMakeLists.txt:2856 — not a dead addition. |
src/core/HidDeviceParser.{h,cpp} — ring decode |
hasShuttle()/shuttlePosition(), m_shuttle set at top of parse() |
Yes | In scope. |
src/core/HidDeviceParser.{h,cpp} — ContourShuttleParser merge + nextPending() queue |
Collapses two duplicate parsers into one base, queues every button edge | Body discloses it as #5932's commit (6acc273) |
Not a blocker, but note it: #5932 is not in main (git log --grep on main finds nothing), so this PR currently carries it. #5928 explicitly proposed keeping the one-event-per-report contract, and the ring is parser state, so the queue isn't needed for the ring. It is a real fix on its own terms — the old m_prevButtons = btns in the loop permanently swallowed the second simultaneous edge — and tests/hid_device_parser_test.cpp:130-204 is good coverage of it. Just flagging the merge-order dependency: if #5932 lands first this hunk vanishes; if it doesn't, this PR is where that fix merges. |
src/core/HidEncoderManager.{h,cpp} |
shuttleChanged, m_lastShuttle, ShuttleMapping accessors, jsonMappingField extraction |
Yes | In scope. The RC-28 refactor is genuinely behaviour-preserving — rc28MappingField/setRc28MappingField now just forward with the literal "RC28Mapping". |
src/gui/MainWindow.h, MainWindow_Controllers.cpp |
Timer, tick, settings cache, wiring | Yes | In scope. |
src/gui/RadioSetupDialog.cpp |
New Shuttle Ring group | Yes | In scope — but see Blocker 1. |
docs/architecture/aetherd-touchpoint* |
ui-support tag + regenerated manifest |
Reviewer-requested (jensenpat) | In scope. I re-ran python3 tools/gen_touchpoint_manifest.py in the head checkout: clean, no diff. Tag is correct — the header is pure arithmetic with no radio state. |
resources/help/configuring-aethersdr-controls.md |
Replaces the two "ring not used" lines | Yes | In scope. |
CMakeLists.txt |
Adds the new header to the HIDAPI block | Yes | In scope. |
tests/hid_device_parser_test.cpp |
Ring decode + integrator + multi-edge | Yes | In scope. |
New public surface: one new persisted settings key, ShuttleMapping (nested JSON, two string fields). Principle V satisfied — single document, single owner (HidEncoderManager), mirrors the RC28Mapping precedent, and your checklist's "no new flat-key AppSettings calls" self-certification checks out. No new protocol verb, no capability field, no CLI flag. #5928 still carries the maintainer-review label, so the merge is jensenpat's call, but the design was already ruled on in the triage comment ("good candidate to accept once #5927 lands") and you followed it.
No unexplained files, no deleted guards, no CHANGELOG entry. Nothing to unbundle beyond the #5932 note above.
3. Blockers
1. src/gui/RadioSetupDialog.cpp:8474, :8483, :8503-8504 — the three style constants no longer exist on main; this will not compile after a rebase.
main is now at 37fbdbc3 — "feat(gui): Radio Setup is a CanonWindow workspace, with a UX pass over every page" (#6413), which landed after your branch point (a9a83e66). That pass stripped the per-widget stylesheets out of this whole file. On main today:
$ grep -c kGroupStyle src/gui/RadioSetupDialog.cpp → 0
$ grep -c kEditStyleTemplate src/gui/RadioSetupDialog.cpp → 0
$ grep -n kLabelStyle src/gui/RadioSetupDialog.cpp → only a comment at :4369
Your new block references all three:
ThemeManager::instance().applyStyleSheet(group, kGroupStyle); // :8474
ThemeManager::instance().applyStyleSheet(note, kLabelStyle); // :8483
ThemeManager::instance().applyStyleSheet(
combo, QString(kEditStyleTemplate).replace("QLineEdit", "QComboBox")); // :8503-8504I checked the neighbouring groups on main's buildSerialTab() (src/gui/RadioSetupDialog.cpp:8191-8230, "Port Configuration"): the QGroupBox, the QLabels and the QComboBox now carry no stylesheet at all — the canon theme scope styles them. So the fix isn't to re-point the constants, it's to drop all three applyStyleSheet calls and let the Shuttle Ring group inherit like its siblings:
auto* group = new QGroupBox("Shuttle Ring (ShuttleXpress / ShuttlePro)");
auto* grid = new QGridLayout(group);
grid->setSpacing(6);and in addCombo, just auto* combo = new QComboBox; with no styling line. That also makes jensenpat's colour-ratchet comment moot by construction rather than by routing around it.
The same rebase pass will want tools/gen_touchpoint_manifest.py re-run: main's manifest is at 254 headers (213 core, 41 models) and your regenerated copy says 252 (213 core, 39 models), so the Totals line and the two count cells will conflict. Mechanical, but do it after the rebase, not before.
I'd also re-read #6413's UX pass before assuming the group lands where you put it — it reorganised every page on this dialog, and a group appended at the old buildSerialTab() tail may no longer be the right home.
This is an environmental blocker, not a defect in your work — the code was correct against the base you branched from and the reviewer's ratchet request was the right fix at the time. It needs a rebase, not a redesign.
4. Nits (none blocking)
src/gui/MainWindow_Controllers.cpp:705-712— RIT/XIT at the rail re-sends the same command 25×/s for as long as the ring is held.SliceModel::setRit()(src/models/SliceModel.cpp) has no no-op short-circuit: it unconditionallysendCommand("slice set N rit_on=1 rit_freq=...")and emitsritChanged. WithmaxRateHz = 1000andunitHz = 10you reach the ±9999 clamp in ~10 s at full deflection, after which every tick still callsapplyFlexControlWheelActionwithsteps = 4, clamps to the same 9999, and sends the same command again. TheWheelFrequencyarm is immune because it coalesces throughm_flexCoalesceTimer; RIT/XIT has no coalescer. The jog has the same semantics but self-limits because the operator has to keep turning — a held ring is the new sustained case. Cheapest fix is in the tick: snapshots->ritFreq()/s->xitFreq()before the call andm_shuttleRate.resetRemainder()+ skip when it didn't move. Not a blocker — 25 Hz is survivable and nothing is corrupted — but it's a new flood path the jog didn't have.src/gui/MainWindow_Controllers.cpp:656-663— settings are re-read from the store on every zero crossing, not every deflection.stopShuttle()callsm_shuttleRate.reset(), so the next non-zero position hasposition() == 0andstartingis true again. Releasing and re-deflecting re-reads twoAppSettingsvalues and parses the JSON twice. That's the intended "once per deflection" in the common case, but #5928 measured threshold jitter at the 8 ms report interval; jitter across the 0/±1 boundary would re-read (and re-arm the 60 ms first step, injecting an extra step per bounce). Your measurements put the jitter at ±3/±4, which never crosses zero, so I could not demonstrate this happening — it's a latent sharp edge, not an observed one. A dirty flag on the combo writes would remove it entirely.src/gui/MainWindow_Controllers.cpp:712— theNoneaction still runs the 40 ms timer for the whole deflection, ticking and returning. Harmless, but you could skipm_shuttleTimer->start()when the cached action isNone.src/gui/RadioSetupDialog.cpp:8499-8508— nosetAccessibleNameon the two new combos, and thenew QLabel(label)isn't set as their buddy.docs/a11y.mdasks for accessible names on interactive widgets. The sibling combos in the same tab have the identical gap andtools/check_a11y.pydoesn't flag either (it reports 61 warnings, none in this file), so this is consistency-with-precedent rather than a regression — worth two lines if you're touching the block for the rebase anyway.src/core/HidEncoderManager.cpp:367-375—setJsonMappingFieldignoress.save()'s return. Inherited verbatim from the RC-28 path, so not introduced here, but the PR does add a second caller. I looked for the "check the write result" sentence inAGENTS.mdto see whether this is canon-gating and could not find language that makes it merge-blocking, so: nit. Same function rebuilds the object fromfromJson, so a corrupt stored value silently drops sibling fields rather than preserving them — also pre-existing, also a nit.resources/help/configuring-aethersdr-controls.md— the ShuttlePro v2 section says "the jog wheel and shuttle ring work exactly as on the ShuttleXpress" but says nothing about its fifteen buttons. I traced the dispatch atsrc/gui/MainWindow_Controllers.cpp:3133-3143: buttons 1–8 →HidKeyAction0-7(so your ShuttleXpress "Key 1–5" claim is correct, verified), buttons 9–12 →HidEncoderPushAction0-3(silently shared with the StreamDeck+ encoder presses), and 13–15 fall through to nothing. Pre-existing from #5931, not yours, but the help page now invites ShuttlePro owners to look for fifteen assignable buttons and only five-to-twelve do anything.
5. Socket tests
None. tests/hid_device_parser_test.cpp has no QTcpServer/QUdpSocket/QLocalServer/bind/listen/connectToHost/peer process and no Fake* device class — it's pure byte-array and arithmetic assertions, which is the correct layer for wire decoding and rate math under AGENTS.md's test-layer boundary. The target is registered (tests/tests.cmake:5334-5340) with HAVE_HIDAPI defined. Worth knowing: hid_device_parser_test does not appear in any ctest -R filter in .github/workflows/ci.yml, so it is not merge-gating — it runs in the full suite only. Not a finding, and I'm not asking for a GUI/timer test for the integrator: the disconnect-stop behaviour lives at the MainWindow wiring layer and there is no deterministic socket-free seam for it, so your hardware verification ("unplugging USB with the ring deflected stops tuning") is the honest evidence for that one.
6. Security audit
Ran the required CodeGuard audit over the 13 changed files. No findings. Refuted specifically:
HidEvent m_pending[17]overflow —parse()resetsm_pendingCount = 0on entry (HidDeviceParser.cpp:120) and the queue loop is bounded bybuttonCount(), not by report content. Worst case is ShuttlePro's 15 button bits + 1 jog = 16 <kMaxEvents17. A hostile device cannot drive more edges per report than there are bits in the XOR.reportSize()vsuint8_t m_buf[64]— Contour returns 5; the documented ≤64 invariant atHidDeviceParser.h:36-40is intact.- Unguarded
buf[0..4]— thelen < 5early return at:124precedes every read, including bothbuttonMask()overrides, which have no other call site. kRate[]indexing / rate overflow — double-clamped (setPositionto ±7, thenstd::clamp(std::abs(...))at the index), andtick()is bounded bymaxRateHz× adtcapped at 0.2 s.- JSON deserialization of
ShuttleMapping— malformed input yields a null document → empty object →isString()false → caller's default. Field keys are all compile-time literals, so a tampered settings file cannot select an arbitrary key. - Settings-driven dispatch — the action string hits an explicit allow-list with
returnon anything unknown before reachingapplyFlexControlWheelAction. No injection surface. - No credentials, crypto, or certificate handling anywhere in the diff.
The harness's 1450 CodeGuard hits (CS-VLN-CMDI-*, CS-VLN-SSRF-*, CS-VLN-XSS-*, CS-PII-*, CG-NET-001) land on .github/workflows/*.yml, backends and other files not in this diff — that's a whole-file scan of main's tip, pre-existing, and the workflow backtick/pipe matches are the known lexical false-positive class. Likewise the memcpy hits in HidEncoderManager.cpp are the StreamDeck+ JPEG code, present unchanged on main; the PR only shifted their line numbers.
7. What I tried to break (and couldn't)
- The release-overshoot claim. #5928 measured
+1 0 -1 -2 -1 0over ~30 ms, and the body claims 60 ms makes a backward step impossible. Walked the state machine: the0report hitsstopShuttle()→reset(), the-1re-arms withm_firstStepPendingandm_heldSec = 0, and the ring is back at 0 before the first 40 ms tick. Then I pushed the worst case the code allows — Fast (×2.0) at the largest available step size (m_stepSizestops out at 10 kHz,RxApplet.h:235): 35 ms × 200 kHz/s = 7 kHz < one 10 kHz step. Survives. The test attests/hid_device_parser_test.cpp:286-293asserts the right thing and would fail ifkFirstStepDelaySecwere dropped to 0. - "Top speed is the same whatever step size." The
(|pos| + 1) × stepHzfloor is the thing that could break this, so I evaluated it at the 10 kHz maximum: position 7's floor is 8 × 10 kHz = 80 kHz/s, which is below the table's 100 kHz/s, so the table still wins at full deflection and the top speed is step-independent as claimed. At that step size the floor does dominate positions 1–6 (20–70 kHz/s), compressing the low end — but they stay monotonic and distinct, and your help text discloses it ("at least a couple of steps per second"). Claim holds. - Rate-math drift between the integrator and the consumer. The integrator's
unitHzandapplyFlexControlWheelAction's internalstepHzare two independent expressions; if they disagreed the whole Hz/s premise collapses. Read both:sw ? sw->stepSize() : (s->stepHz() > 0 ? s->stepHz() : 100), character-for-character identical. RIT/XIT's hardcodedunitHz = 10matchessteps * 10in both arms. No drift. - The thread hop.
m_hidEncoderismoveToThread(m_extCtrlThread)(MainWindow_Controllers.cpp:2951), sopoll()andclose()run off the GUI thread and everyshuttleChangedis a queued delivery. Checked whether the body's "always returns to 0 before the device is closed" survives that: queued events from one sender on one thread keep their order, and the shutdown path invokesclose()viaBlockingQueuedConnectionbefore joining the thread and deleting the object (MainWindow.cpp:2867-2875,:2909-2911), so the0is posted whileMainWindowis still alive. TheconnectionChanged(false)→stopShuttle()belt-and-braces covers the teardown that bypassesclose(), which is exactly what triage point 3 asked for. - Reentrancy and sign flips. Tried
+1 → -1with no intervening0(a fast reversal, which the dropped 60 ms filter would have swallowed):setPositiondetects it, zeroes the remainder so it can't leak into the new direction, and re-arms the first step —onShuttleChangedcorrectly treats it as not starting, so the settings aren't re-read and the timer isn't restarted. Also checked+2 → +3doesn't inject a spurious first step. Both are asserted in the tests, and both assertions would fail against an implementation without thereversedterm, so they're not tautological. - Deleted behaviour. Read the
-lines inHidDeviceParser.cppcarefully, since the parser merge removes two whole functions. The only behaviour removed is thereturn-after-first-edge that lost simultaneous edges, and them_firstReportpriming semantics are preserved (nowm_prevJog = jogunconditionally at the end, which is equivalent). TheShuttlePro v2mask gained a& 0x7Fthat the old code lacked — a correctness improvement, not a removal, since the oldbuf[4] << 8would have reported a phantom button 16 from bit 7. - Dead additions.
ShuttleRateIntegrator.his reachable fromCMakeLists.txt:2856,MainWindow.h:83and the test; theui-supporttag regenerates clean. Nothing orphaned.
Honesty note: I am headless — no build, no test run, no GUI. Every finding above is reasoned from the code in the head checkout at the cited file:line, not reproduced at runtime. The one thing I can state as observed rather than reasoned is Blocker 1's grep -c output against main's working tree. CI is green on the branch head b435d9c (build, check-macos, check-windows, Static checks, Sanitizer option configures) — but that is the branch head, which predates #6413, so it is not evidence against Blocker 1.
8. Recommendation
Request changes — rebase only. The engineering here is solid: the rate integrator is well-reasoned, the hardware findings that drove the first-step and floor changes are the kind of thing only a real device tells you, the action list restriction is the right safety call, and the tests actually test the guards rather than restating the implementation. The single blocker is that main moved out from under you: #6413 deleted the three style constants your new group uses, so the rebase is a compile fix, not a rework. Concrete next step — rebase onto main, delete the three applyStyleSheet calls at RadioSetupDialog.cpp:8474/:8483/:8503 so the group inherits the canon theme scope like its siblings, re-run tools/gen_touchpoint_manifest.py, and confirm the group still belongs at the tail of the reorganised Serial page. The RIT-at-the-rail nit is worth folding in while you're there. #5928 still carries maintainer-review and #5932 is unmerged, so @jensenpat owns the merge order and the final call on the new ShuttleMapping key — but the design was already ruled on in the triage comment and this follows it faithfully.
🤖 aethersdr-agent · cost: $10.8125 · model: claude-opus-5
|
Follow-up to Blocker 1 in my review — I chased down where the three constants went so you don't have to, and the post-rebase form is slightly more than "delete the calls". They lived in So there is no definition of Post-rebase, the group and the combos take no styling at all (matching "Port Configuration" at auto* group = new QGroupBox("Shuttle Ring (ShuttleXpress / ShuttlePro)");
auto* grid = new QGridLayout(group);
grid->setSpacing(6);
auto* note = new QLabel(
"The spring-loaded outer ring tunes continuously while held: a small "
"turn creeps, full deflection sweeps the band, letting go stops. "
"Speed does not depend on the step size.");
note->setWordWrap(true);
applyLabelStyle(note);
grid->addWidget(note, 0, 0, 1, 3);and in One small correction to my own review while I'm here: I wrote that 🤖 Generated with Claude Code 🤖 aethersdr-agent · cost: $13.8863 · model: claude-opus-5 |
|
Cisco CodeGuard — static analysis of this PR (1463 finding(s))
Automated static scan by Cisco DefenseClaw CodeGuard on the changed files. Advisory — some may be false positives; the review above verifies them. 🤖 aethersdr-agent · cost: $14.0010 · model: claude-opus-5 |
…iple XI.
The spring-loaded outer ring on the ShuttleXpress / ShuttlePro v2 was
never decoded (byte 0, signed -7..+7). It only reports when it moves,
never while held, so it is handled as a rate input.
- Parser: HidDeviceParser gains hasShuttle()/shuttlePosition(). The
Contour decoder sets the position at the top of every parse(), so it is
never lost to the one-event-per-report contract.
- HidEncoderManager emits shuttleChanged(position) on change, with the
existing HidEncoderInvertDir applied (no second invert). close() emits
shuttleChanged(0) before destroying the parser, independent of the
device-name guard. MainWindow also stops on connectionChanged(false).
- ShuttleRateIntegrator (header-only, no Qt) integrates Hz/s into whole
steps of the current step size and carries the remainder. The table is
{0, 20, 100, 500, 2k, 8k, 30k, 100k} Hz/s, so the top speed does not
depend on the step size. It is floored at (|pos| + 1) steps/s so large
steps still move on the first detents. A new deflection makes one step
once held for 60 ms, which is longer than the measured 30-35 ms
snap-back overshoot on release. The remainder is dropped at centre and
on reversal.
- MainWindow runs a 40 ms timer while the ring is deflected and feeds
applyFlexControlWheelAction(). Tune Lock is checked in the tick and
notified once per deflection, so direct frequency entry is not
cancelled every tick.
- Actions are limited to Tune Slice / RIT / XIT / None. RIT and XIT are
capped at 1 kHz/s. Settings live in one "ShuttleMapping" JSON key
(Principle V), sharing a helper with RC28Mapping. There is a new
"Shuttle Ring" group in Radio Setup > Serial.
- Help page: the ring is documented instead of listed as unused.
- Tests: ring position decode (captured sweep, clamp, alongside a button
edge, Pro v2) and the integrator (Hz/s at 10 Hz and 1 kHz steps, creep
remainder, step floor, first step and overshoot, reversal, cap).
Hardware (ShuttleXpress + FLEX-6500): symmetric response both ways, no
backward step on release across 40 logged ring position changes, and the lock
and unplug-while-deflected cases stop tuning.
Closes aethersdr#5928
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…comments. Principle VIII. - Tag core/ShuttleRateIntegrator.h as ui-support (same class as HidEncoderManager.h) and regenerate aetherd-touchpoints.md: the new row, HidEncoderManager.h includers 2 -> 3, totals +1. - Shuttle Ring group styles its group, note and combos through ThemeManager::applyStyleSheet, so the setStyleSheet ratchet stays at +0. - Help: the first step lands after about 60 ms, not "straight away". - Comments condensed to the AGENTS.md comment policy (aethersdr#6072); the hardware measurements behind kFirstStepDelaySec live in aethersdr#5933. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
main replaced the local kEditStyle with the shared token template (RadioSetupDialogCommon.h); the rebased group still named the old constant, which only compiles out when HAVE_HIDAPI is off. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
b435d9c to
02ea83a
Compare
Both requested changes are implemented and verified on 02ea83a: ui-support tag plus regenerated touchpoint manifest (--check passed), and removal of the obsolete per-widget styling calls in favor of current Radio Setup roles (color ratchet +0 / +0 / +0). Both threads carry evidence replies and are resolved. The HID-enabled macOS app built; the focused parser/rate CTest passed 1/1; demo settings persisted across process restart. This retires the specific static-gate objections; hosted checks on the new head are still pending and remain merge gates.
jensenpat
left a comment
There was a problem hiding this comment.
Approved exact head 02ea83a732b7a243ecb499488f662f4b29dd193a, rebased onto #5932's confirmed squash ad398cf9a413ae1b86e01ed051cf4769dacc1e52. No remaining code blockers. New hosted CI/Docs are pending and remain merge gates; this approval does not claim they passed.
| Scope | Change | Verdict |
|---|---|---|
src/core/HidDeviceParser.{h,cpp} |
Contour ring state, clamped signed byte | Contour-specific implementation; other parsers default to no shuttle |
src/core/HidEncoderManager.{h,cpp} |
Ring signal/lifecycle and ShuttleMapping; extract JSON helper |
Shared HID surface; RC-28 wrappers retain identical key/field semantics |
src/core/ShuttleRateIntegrator.h |
Pure Hz/s-to-step arithmetic | Required desktop HID helper; no radio state |
src/gui/MainWindow.h, MainWindow_Controllers.cpp |
GUI timer and existing frequency/RIT/XIT dispatch | Controller scope; no added thread, protocol encoder, or TX path |
src/gui/RadioSetupDialog.cpp |
Shuttle settings with canon roles, guarded combos and accessible names | Required feature UI; adjacent working controls preserved |
CMakeLists.txt, touchpoint tag/manifest |
Register helper and its previously requested ui-support classification |
No dependency or boundary-baseline growth |
tests/hid_device_parser_test.cpp, user guide and in-app help |
Parser/rate checks and operator documentation | In scope; no socket-owning test |
The #5932 button-event queue is now on main separately; this PR's remaining parser change is ring state. No audio/DSP, networking, radio-backend, capability, TX-keying, workflow, credential, or unrelated default changes. Existing ShuttlePro v2 buttons 13–15 have no action mapping; the docs now state that limitation accurately.
Resolved findings:
src/gui/RadioSetupDialog.cpp:9191: removed references to the deleted style constants; group/combos inherit the current Radio Setup theme and the note usessetTone(..., Tone::Hint). Guarded combos have buddies and accessible names at line 9221. The HID-enabled app build and demo layout inspection passed.docs/architecture/aetherd-touchpoint-tags.json:692,aetherd-touchpoints.md:149:ui-supporttag retained and manifest regenerated after rebase;--checkpassed. Both prior review threads have evidence replies and are resolved; review 5326736207 was dismissed because its two specific objections are fixed.src/gui/MainWindow_Controllers.cpp:729: the new ring handler skips unchanged enabled RIT/XIT offsets at their clamp. Source tracing through the existing wheel helper and typed SliceModel controls confirms the guard preserves first enable and movement back from the rail. This path was built and source-reviewed; it was not driven by HID injection.
Verification:
- Final-head macOS
AetherSDR+hid_device_parser_testbuild passed with Qt 6.12.0 and HID enabled. Focused CTest passed 1/1, headless, on this head. - Removing the 60 ms delay in a disposable source copy failed the first-step and 35 ms overshoot checks; restoring it passed. #5932's first-edge-only mutation separately failed five simultaneous-edge checks.
- The exact Git tree
3bb4694b971e45326b7143d5ea067998e1562b65was driven through the offscreen, TX-disabled bridge onDEMO-0001: all four action choices, Fast speed selection, accessible names, a layout screenshot, and action/speed retention across normal Quit and process restart. Both owned app instances exited. The final rebase has the identical tree, and its exact head was rebuilt and retested. - Strict engine-boundary findings did not grow (101 existing findings, shifted lines only); capability and command-plane outputs matched the base. Manifest, generated references, log-rule checks (707 checks), and color ratchet passed (+0 colors/references/stylesheet sites). Existing a11y warnings are outside the new group. No local docs site/PDF rebuild; hosted Docs validates those edits.
- Manual input/security review checked queue bounds, short reports, signed/clamped positions, allowed action dispatch, bounded elapsed time/rates, stop-before-parser-reset ordering, queued signal order, and non-Contour defaults. Automated code-review/CodeGuard tools were unavailable in this session. The earlier whole-repository CodeGuard comment points at unchanged workflows/other files; those are not introduced by this diff.
Evidence boundary: the bridge has no Contour report-injection verb. Physical ring/timer delivery and USB unplug convergence were source-reviewed here, not exercised on hardware. Contributor-reported ShuttleXpress/FLEX-6500 results are historical; ShuttlePro v2 and 1 kHz-step behavior have unit coverage only. No live radio or TX test was run.
…rails. Principle XI.
02ea83a to
8e890bb
Compare
jensenpat
left a comment
There was a problem hiding this comment.
Approved exact head 8e890bbbf1f40900141cb8e74976f177d210ff8d, rebased onto #5932's confirmed squash ad398cf9a413ae1b86e01ed051cf4769dacc1e52. No remaining code blockers. New hosted CI/Docs are pending and remain merge gates; this approval does not claim they passed.
| Scope | Change | Verdict |
|---|---|---|
src/core/HidDeviceParser.{h,cpp} |
Contour ring state, clamped signed byte | Contour-specific implementation; other parsers default to no shuttle |
src/core/HidEncoderManager.{h,cpp} |
Ring signal/lifecycle and ShuttleMapping; extract JSON helper |
Shared HID surface; RC-28 wrappers retain identical key/field semantics |
src/core/ShuttleRateIntegrator.h |
Pure Hz/s-to-step arithmetic | Required desktop HID helper; no radio state |
src/gui/MainWindow.h, MainWindow_Controllers.cpp |
GUI timer and existing frequency/RIT/XIT dispatch | Controller scope; no added thread, protocol encoder, or TX path |
src/gui/RadioSetupDialog.cpp |
Shuttle settings with canon roles, guarded combos and accessible names | Required feature UI; adjacent working controls preserved |
CMakeLists.txt, touchpoint tag/manifest |
Register helper and its previously requested ui-support classification |
No dependency or boundary-baseline growth |
tests/hid_device_parser_test.cpp, user guide and in-app help |
Parser/rate checks and operator documentation | In scope; no socket-owning test |
The #5932 button-event queue is now on main separately; this PR's remaining parser change is ring state. No audio/DSP, networking, radio-backend, capability, TX-keying, workflow, credential, or unrelated default changes. Existing ShuttlePro v2 buttons 13–15 have no action mapping; the docs now state that limitation accurately.
Resolved findings:
src/gui/RadioSetupDialog.cpp:9191: removed references to the deleted style constants; group/combos inherit the current Radio Setup theme and the note usessetTone(..., Tone::Hint). Guarded combos have buddies and accessible names at line 9221. The HID-enabled app build and demo layout inspection passed.docs/architecture/aetherd-touchpoint-tags.json:692,aetherd-touchpoints.md:149:ui-supporttag retained and manifest regenerated after rebase;--checkpassed. Both prior review threads have evidence replies and are resolved; review 5326736207 was dismissed because its two specific objections are fixed.src/gui/MainWindow_Controllers.cpp:729: the new ring handler skips unchanged enabled RIT/XIT offsets at their clamp. Source tracing through the existing wheel helper and typed SliceModel controls confirms the guard preserves first enable and movement back from the rail. This path was built and source-reviewed; it was not driven by HID injection.
Verification:
- Final-head macOS
AetherSDR+hid_device_parser_testbuild passed with Qt 6.12.0 and HID enabled. Focused CTest passed 1/1, headless, on this head. - Removing the 60 ms delay in a disposable source copy failed the first-step and 35 ms overshoot checks; restoring it passed. #5932's first-edge-only mutation separately failed five simultaneous-edge checks.
- The exact Git tree
3bb4694b971e45326b7143d5ea067998e1562b65was driven through the offscreen, TX-disabled bridge onDEMO-0001: all four action choices, Fast speed selection, accessible names, a layout screenshot, and action/speed retention across normal Quit and process restart. Both owned app instances exited. The final head differs from that tree only in one user-guide sentence about the shared HID direction setting; application/test sources are identical. The exact final head was rebuilt and its focused CTest passed again. - Strict engine-boundary findings did not grow (101 existing findings, shifted lines only); capability and command-plane outputs matched the base. Manifest, generated references, log-rule checks (707 checks), and color ratchet passed (+0 colors/references/stylesheet sites). Existing a11y warnings are outside the new group. No local docs site/PDF rebuild; hosted Docs validates those edits.
- Manual input/security review checked queue bounds, short reports, signed/clamped positions, allowed action dispatch, bounded elapsed time/rates, stop-before-parser-reset ordering, queued signal order, and non-Contour defaults. Automated code-review/CodeGuard tools were unavailable in this session. The earlier whole-repository CodeGuard comment points at unchanged workflows/other files; those are not introduced by this diff.
Evidence boundary: the bridge has no Contour report-injection verb. Physical ring/timer delivery and USB unplug convergence were source-reviewed here, not exercised on hardware. Contributor-reported ShuttleXpress/FLEX-6500 results are historical; ShuttlePro v2 and 1 kHz-step behavior have unit coverage only. No live radio or TX test was run.
Documentation correction: the ring shares the HID jog wheel direction setting. Radio Setup's FlexControl-only "Invert tuning direction" checkbox does not control HID devices. No runtime code changed in this correction.
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.
ShuttleMappingobject; 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.Scope
The shared HID manager gains ring state/lifecycle and a parameterized JSON helper; RC-28 calls still use the identical
RC28Mappingkey 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.his desktop HID arithmetic. Itsui-supporttag and regenerated touchpoint row follow the existing maintainer review request; no boundary baseline is raised or vendor tag weakened.Validation
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.DEMO-0001bridge 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.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-serialshows 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)