Skip to content

feat(hid): variable-speed tuning from the Contour shuttle ring. Principle XI. - #5933

Merged
jensenpat merged 4 commits into
aethersdr:mainfrom
sq9fk:feat/5928-shuttle-ring
Oct 17, 2026
Merged

jensenpat merged 4 commits into
aethersdr:mainfrom
sq9fk:feat/5928-shuttle-ring

Conversation

@sq9fk

@sq9fk sq9fk commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

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)

@jensenpat jensenpat left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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() emits shuttleChanged(0) before the parser is destroyed, and not behind the device-name guard. connectionChanged(false) also stops the timer.
  • Direction uses the existing HidEncoderInvertDir flag. There is no second invert in ShuttleMapping.
  • 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.
  • kFirstStepDelaySec set 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 jensenpat left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. tools/gen_touchpoint_manifest.py --check — core/ShuttleRateIntegrator.h has no semantic tag. MainWindow.h includes it, so it is a gui→engine touchpoint. Tag it ui-support in docs/architecture/aetherd-touchpoint-tags.json (same class as HidEncoderManager.h: client HID plumbing, not radio state) and regenerate with python 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.

  2. Hardcoded-colour ratchet — setStyleSheet call sites 1043 > 1040 (+3). Unique colours did not rise. The three new calls are the Shuttle Ring group, its note, and the combo inside addCombo. Copying setStyleSheet(kGroupStyle) from the older groups on this page is what the ratchet counts. ThemeManager::applyStyleSheet is the call that does not add a site (see the comment on makeValueLabel in 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.

Comment thread src/gui/MainWindow.h
Comment thread src/gui/RadioSetupDialog.cpp Outdated
@aethersdr-agent

Copy link
Copy Markdown
Contributor

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 kFirstStepDelaySec is exactly the kind of evidence that makes a feel-sensitive change reviewable. Nothing is wrong with the feature. Two static gates tripped, both mechanical, and I reproduced both locally on 4be1554c so the fixes below are verified rather than guessed.

Static checks failed at two steps. The three build jobs (build, check-windows, check-macos) were still running when I looked, so they are not part of this. Neither failure is CI infrastructure.


1. aetherd touchpoint manifest

::error::aetherd touchpoints require a valid semantic tag: core/ShuttleRateIntegrator.h

src/gui/MainWindow.h now includes core/ShuttleRateIntegrator.h, which makes it a new gui→engine touchpoint. tools/gen_touchpoint_manifest.py --check refuses any touchpoint header without a semantic tag in the sidecar, so a new engine header reached from src/gui/ always needs one row of JSON. There is a second, quieter staleness too: RadioSetupDialog.cpp's new #include "core/HidEncoderManager.h" takes that header's includer count from 2 to 3, so the generated table is out of date even once the tag exists.

Fix, in order:

  1. Add one entry to docs/architecture/aetherd-touchpoint-tags.json (insert it in place — the file is sorted, so please don't let an editor reformat the whole thing):
  "core/ShuttleRateIntegrator.h": {
    "tag": "ui-support",
    "note": "pure shuttle-ring rate arithmetic for the desktop HID surface, no radio state",
    "split": "",
    "confidence": "high"
  },

ui-support is the right tag by precedent: core/HidEncoderManager.h is already ui-support ("desktop input device plumbing, not radio state"), as are core/FlexControlManager.h and core/ShortcutManager.h. Your header is Qt-free arithmetic over a USB input surface, so it is the same class.

  1. Regenerate rather than hand-edit the table:
python tools/gen_touchpoint_manifest.py

With those two steps the check goes green here; the manifest diff is exactly three lines — totals 230 → 231, the HidEncoderManager.h includer count 2 → 3, and the new ShuttleRateIntegrator.h row.

Generator and tag legend: tools/gen_touchpoint_manifest.py, docs/architecture/aetherd-touchpoints.md.

(Worth a thought, not a request: the integrator is pure <algorithm>/<cmath> with no Qt and no radio state. If it lived in src/gui/ it would not be a touchpoint at all, and tests/tests.cmake would only need its include path changed. Tagging it is entirely acceptable — ui-support exists for headers in exactly this position — so take whichever you prefer.)

2. Hardcoded-colour ratchet

  OK   unique_colours       606  (base 606, +0)
  OK   total_references    2617  (base 2617, +0)
  OVER setstylesheet       1043  (base 1040, +3)

You added no new colours — the counter that moved is setStyleSheet() call sites, and the three are the new Shuttle Ring group in src/gui/RadioSetupDialog.cpp:

  • group->setStyleSheet(kGroupStyle)
  • note->setStyleSheet(kLabelStyle)
  • combo->setStyleSheet(QString(kEditStyle).replace("QLineEdit", "QComboBox"))

I want to be straight about this one: you copied the idiom from the group immediately above yours (StreamDeck+ Encoder Push Actions uses those same three lines, in that order), so this is the gate penalising consistency with its neighbours, not a mistake on your part. The ratchet is a strict delta against the PR's merge base and counts sites, so migrating a colour to a token will not clear it — tools/audit_colours.py says as much in its own failure text.

The cheapest honest fix is to add a small file-local helper next to kGroupStyle and convert one or two existing groups onto it, so the net count falls:

// One place that applies the settings-panel styling, so a new group adds no
// setStyleSheet() call site (colour ratchet, #4569).
static void styleSettingsGroup(QGroupBox* group)
{
    group->setStyleSheet(kGroupStyle);
    for (auto* lbl : group->findChildren<QLabel*>())
        if (lbl->styleSheet().isEmpty()) lbl->setStyleSheet(kLabelStyle);
    for (auto* combo : group->findChildren<QComboBox*>())
        if (combo->styleSheet().isEmpty())
            combo->setStyleSheet(QString(kEditStyle).replace("QLineEdit", "QComboBox"));
}

Call it once after the group is populated (styleSettingsGroup(group); before vbox->addWidget(group)), drop your three lines, and convert the adjacent StreamDeck+ group (RadioSetupDialog.cpp:7804 onward, three sites of its own) the same way. That is 3 sites in the helper, 6 removed, so the ratchet reads negative and you leave the file slightly tidier than you found it. The styleSheet().isEmpty() guard is the existing pattern at RadioSetupDialog.cpp:2020 and :2474, so widgets you style explicitly are left alone.

Reproduce either gate locally without waiting on CI:

python tools/gen_touchpoint_manifest.py --check
git worktree add /tmp/colour-base "$(git merge-base HEAD origin/main)"
python tools/audit_colours.py --src src --compare-src /tmp/colour-base/src --summary-only --strict

No reviewer has flagged anything on the code itself, and nothing in either failure touches the decode, the integrator, or the help-page text. Ping me once it's pushed and I'll look again.


🤖 aethersdr-agent · cost: $4.2691 · model: claude-opus-5

sq9fk added a commit to sq9fk/AetherSDR that referenced this pull request Oct 7, 2026
…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>
@sq9fk
sq9fk force-pushed the feat/5928-shuttle-ring branch from 4be1554 to 7e9110f Compare October 7, 2026 08:46
@sq9fk

sq9fk commented Oct 7, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto current main (25de97c3) and addressed the static-check failures. #5931 landed as fedc434b, so its commit is gone from the stack:

In 7e9110ff:

  1. Touchpoint manifest: tagged core/ShuttleRateIntegrator.h as ui-support next to HidEncoderManager.h and regenerated aetherd-touchpoints.md. The diff is the expected three lines: totals, HidEncoderManager.h includers 2 → 3, and the new row.
  2. Colour ratchet: the group, note and combo now go through ThemeManager::applyStyleSheet. audit_colours.py --strict against the merge base: setstylesheet +0, unique colours +0.
  3. Help nit: the first step is now described as landing after about 60 ms.
  4. Comments: trimmed to the ≤5-line policy (docs: cut review latency — comment policy, AGENTS.md split, condensed comments, Constitution 2.1.0, body-only reviews. Principle VIII. #6072). The measurements behind kFirstStepDelaySec stay in the PR body.

Checked locally before pushing:

  • All 12 strict checks from static-checks.yml pass, including the touchpoint manifest, test registration and the CI test gate.
  • Full Windows build against the pinned Qt 6.12.0 (MSVC 19.44, Ninja).
  • ctest -R hid_device_parser_test passes.

Both #5932 and #5933 report mergeable again. Ready for CI whenever you are.

@sq9fk

sq9fk commented Oct 7, 2026

Copy link
Copy Markdown
Contributor Author

The build jobs on 7e9110ff failed on all three platforms: kEditStyle is undeclared in the Shuttle Ring group. main had moved those combos to the shared kEditStyleTemplate (RadioSetupDialogCommon.h), and my rebase kept the old name. Fixed in 123e6b18, which uses the same call as the neighbouring encoder group.

I need to correct my previous comment: the "full Windows build" I reported had hidapi missing from the build tree after the rebase, so HAVE_HIDAPI was off and none of the HID code compiled. I've now rebuilt with hidapi found — USB HID encoder support enabled (Qt 6.12.0, MSVC 19.44):

  • RadioSetupDialog.cpp, MainWindow_Controllers.cpp, HidEncoderManager.cpp and HidDeviceParser.cpp compile.
  • AetherSDR.exe links.
  • hid_device_parser_test passes.
  • The static checks still pass, with the colour ratchet at +0.

Sorry for the extra CI round.

@sq9fk
sq9fk marked this pull request as ready for review October 7, 2026 11:05
@sq9fk
sq9fk requested review from a team as code owners October 7, 2026 11:05

@Ozy311 Ozy311 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

  1. Touchpoint tag. 7e9110ff tags core/ShuttleRateIntegrator.h as ui-support in aetherd-touchpoint-tags.json, the same class as HidEncoderManager.h, and regenerates the manifest. Static checks is green on this head.
  2. Colour ratchet. The Shuttle Ring group, its note and its combos now go through ThemeManager::applyStyleSheet. 123e6b18 also switches the combos from the removed kEditStyle to kEditStyleTemplate. That one would only have failed to compile in a HAVE_HIDAPI build, so it is a good catch. I re-ran tools/audit_colours.py --strict against current main on 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.cpp built standalone with -std=c++20 -DHAVE_HIDAPI on the PR merged into current main: all checks passed.
  • Touchpoint --check and 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

@sq9fk

sq9fk commented Oct 10, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the re-review. Merged current main (a9a83e66) in b435d9cf. The only conflict was docs/architecture/aetherd-touchpoints.md, which I resolved by regenerating it with tools/gen_touchpoint_manifest.py. --check reports "manifest up to date", and the ShuttleRateIntegrator.h row matches the one you posted.

Checked locally on the merged tree:

  • All 12 strict checks from static-checks.yml pass.
  • Colour ratchet against main: setstylesheet 798 (+0), unique colours +0, references +0.
  • Windows build with hidapi found — USB HID encoder support enabled (Qt 6.12.0, MSVC 19.44) links.
  • hid_device_parser_test passes.

The PR is now mergeable again, and CI is running on the new head.

@aethersdr-agent aethersdr-agent Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-8504

I 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 unconditionally sendCommand("slice set N rit_on=1 rit_freq=...") and emits ritChanged. With maxRateHz = 1000 and unitHz = 10 you reach the ±9999 clamp in ~10 s at full deflection, after which every tick still calls applyFlexControlWheelAction with steps = 4, clamps to the same 9999, and sends the same command again. The WheelFrequency arm is immune because it coalesces through m_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: snapshot s->ritFreq()/s->xitFreq() before the call and m_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() calls m_shuttleRate.reset(), so the next non-zero position has position() == 0 and starting is true again. Releasing and re-deflecting re-reads two AppSettings values 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 — the None action still runs the 40 ms timer for the whole deflection, ticking and returning. Harmless, but you could skip m_shuttleTimer->start() when the cached action is None.
  • src/gui/RadioSetupDialog.cpp:8499-8508 — no setAccessibleName on the two new combos, and the new QLabel(label) isn't set as their buddy. docs/a11y.md asks for accessible names on interactive widgets. The sibling combos in the same tab have the identical gap and tools/check_a11y.py doesn'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 — setJsonMappingField ignores s.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 in AGENTS.md to see whether this is canon-gating and could not find language that makes it merge-blocking, so: nit. Same function rebuilds the object from fromJson, 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 at src/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() resets m_pendingCount = 0 on entry (HidDeviceParser.cpp:120) and the queue loop is bounded by buttonCount(), not by report content. Worst case is ShuttlePro's 15 button bits + 1 jog = 16 < kMaxEvents 17. A hostile device cannot drive more edges per report than there are bits in the XOR.
  • reportSize() vs uint8_t m_buf[64] — Contour returns 5; the documented ≤64 invariant at HidDeviceParser.h:36-40 is intact.
  • Unguarded buf[0..4] — the len < 5 early return at :124 precedes every read, including both buttonMask() overrides, which have no other call site.
  • kRate[] indexing / rate overflow — double-clamped (setPosition to ±7, then std::clamp(std::abs(...)) at the index), and tick() is bounded by maxRateHz × a dt capped 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 return on anything unknown before reaching applyFlexControlWheelAction. 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 0 over ~30 ms, and the body claims 60 ms makes a backward step impossible. Walked the state machine: the 0 report hits stopShuttle() → reset(), the -1 re-arms with m_firstStepPending and m_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_stepSizes tops out at 10 kHz, RxApplet.h:235): 35 ms × 200 kHz/s = 7 kHz < one 10 kHz step. Survives. The test at tests/hid_device_parser_test.cpp:286-293 asserts the right thing and would fail if kFirstStepDelaySec were dropped to 0.
  • "Top speed is the same whatever step size." The (|pos| + 1) × stepHz floor 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 unitHz and applyFlexControlWheelAction's internal stepHz are 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 hardcoded unitHz = 10 matches steps * 10 in both arms. No drift.
  • The thread hop. m_hidEncoder is moveToThread(m_extCtrlThread) (MainWindow_Controllers.cpp:2951), so poll() and close() run off the GUI thread and every shuttleChanged is 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 invokes close() via BlockingQueuedConnection before joining the thread and deleting the object (MainWindow.cpp:2867-2875, :2909-2911), so the 0 is posted while MainWindow is still alive. The connectionChanged(false) → stopShuttle() belt-and-braces covers the teardown that bypasses close(), which is exactly what triage point 3 asked for.
  • Reentrancy and sign flips. Tried +1 → -1 with no intervening 0 (a fast reversal, which the dropped 60 ms filter would have swallowed): setPosition detects it, zeroes the remainder so it can't leak into the new direction, and re-arms the first step — onShuttleChanged correctly treats it as not starting, so the settings aren't re-read and the timer isn't restarted. Also checked +2 → +3 doesn't inject a spurious first step. Both are asserted in the tests, and both assertions would fail against an implementation without the reversed term, so they're not tautological.
  • Deleted behaviour. Read the - lines in HidDeviceParser.cpp carefully, since the parser merge removes two whole functions. The only behaviour removed is the return-after-first-edge that lost simultaneous edges, and the m_firstReport priming semantics are preserved (now m_prevJog = jog unconditionally at the end, which is equivalent). The ShuttlePro v2 mask gained a & 0x7F that the old code lacked — a correctness improvement, not a removal, since the old buf[4] << 8 would have reported a phantom button 16 from bit 7.
  • Dead additions. ShuttleRateIntegrator.h is reachable from CMakeLists.txt:2856, MainWindow.h:83 and the test; the ui-support tag 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

@aethersdr-agent

Copy link
Copy Markdown
Contributor

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 src/gui/RadioSetupDialogCommon.h (:21 kGroupStyle, :36 kEditStyleTemplate on this branch). That header still exists on main, but #6413 replaced the raw stylesheet strings with role-property helpers:

$ grep -nE "^inline" src/gui/RadioSetupDialogCommon.h   # on main
44: inline void setRoleProperty(QWidget*, const char*, const char*)
60: inline void setTone(QWidget*, Tone)
67: inline void setChip(QWidget*, Chip)
72: inline void setButtonRole(QWidget*, ButtonRole)
85: inline void applyLabelStyle(QWidget*)

So there is no definition of kGroupStyle/kEditStyleTemplate/kLabelStyle visible to RadioSetupDialog.cpp anywhere on main — the remaining hits in the tree are file-local statics in other TUs (RC28MappingDialog.cpp, GreenHeronApplet.cpp, …). The build break is confirmed, not just a style mismatch.

Post-rebase, the group and the combos take no styling at all (matching "Port Configuration" at :8191), and the explanatory label uses the new helper:

        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 addCombo, just auto* combo = new QComboBox; with the styling line dropped. Worth double-checking applyLabelStyle's intent against a sibling note label on the rebased page before you commit to it — I read the signature, not its call sites.

One small correction to my own review while I'm here: I wrote that tools/check_a11y.py reports "none in this file". It actually reports 9 warnings in RadioSetupDialog.cpp, all of the a11y-disabled-reason-not-announced kind and all at lines 2057–6222 — none inside your new block (8473–8518). The nit itself stands unchanged: the checker does not flag the new combos, so the missing setAccessibleName is consistency-with-precedent, not a regression.

🤖 Generated with Claude Code


🤖 aethersdr-agent · cost: $13.8863 · model: claude-opus-5

@aethersdr-agent

Copy link
Copy Markdown
Contributor

Cisco CodeGuard — static analysis of this PR (1463 finding(s))

  • [MEDIUM] CG-PATH-001 — Potential path traversal in .github/workflows/appimage.yml /tmp/aetherclaude/pr-5933/.github/workflows/appimage.yml:205
  • [MEDIUM] CG-PATH-001 — Potential path traversal in .github/workflows/appimage.yml /tmp/aetherclaude/pr-5933/.github/workflows/appimage.yml:417
  • [CRITICAL] CS-VLN-CMDI-PIPE — Vulnerability: CS-VLN-CMDI-PIPE in .github/workflows/appimage.yml /tmp/aetherclaude/pr-5933/.github/workflows/appimage.yml:77
  • [CRITICAL] CS-VLN-CMDI-PIPE — Vulnerability: CS-VLN-CMDI-PIPE in .github/workflows/appimage.yml /tmp/aetherclaude/pr-5933/.github/workflows/appimage.yml:387
  • [CRITICAL] CS-VLN-CMDI-SUBSHELL — Vulnerability: CS-VLN-CMDI-SUBSHELL in .github/workflows/appimage.yml /tmp/aetherclaude/pr-5933/.github/workflows/appimage.yml:501
  • [CRITICAL] CS-VLN-CMDI-SUBSHELL — Vulnerability: CS-VLN-CMDI-SUBSHELL in .github/workflows/appimage.yml /tmp/aetherclaude/pr-5933/.github/workflows/appimage.yml:503
  • [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in .github/workflows/appimage.yml /tmp/aetherclaude/pr-5933/.github/workflows/appimage.yml:132
  • [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in .github/workflows/appimage.yml /tmp/aetherclaude/pr-5933/.github/workflows/appimage.yml:257
  • [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in .github/workflows/appimage.yml /tmp/aetherclaude/pr-5933/.github/workflows/appimage.yml:304
  • [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in .github/workflows/appimage.yml /tmp/aetherclaude/pr-5933/.github/workflows/appimage.yml:334
  • [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in .github/workflows/appimage.yml /tmp/aetherclaude/pr-5933/.github/workflows/appimage.yml:366
  • [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in .github/workflows/appimage.yml /tmp/aetherclaude/pr-5933/.github/workflows/appimage.yml:451
  • [MEDIUM] CS-VLN-CMDI-ENVVAR — Vulnerability: CS-VLN-CMDI-ENVVAR in .github/workflows/appimage.yml /tmp/aetherclaude/pr-5933/.github/workflows/appimage.yml:182
  • [MEDIUM] CS-VLN-CMDI-ENVVAR — Vulnerability: CS-VLN-CMDI-ENVVAR in .github/workflows/appimage.yml /tmp/aetherclaude/pr-5933/.github/workflows/appimage.yml:209
  • [MEDIUM] CS-VLN-CMDI-ENVVAR — Vulnerability: CS-VLN-CMDI-ENVVAR in .github/workflows/appimage.yml /tmp/aetherclaude/pr-5933/.github/workflows/appimage.yml:215
  • [MEDIUM] CS-VLN-CMDI-ENVVAR — Vulnerability: CS-VLN-CMDI-ENVVAR in .github/workflows/appimage.yml /tmp/aetherclaude/pr-5933/.github/workflows/appimage.yml:415
  • [MEDIUM] CS-VLN-CMDI-ENVVAR — Vulnerability: CS-VLN-CMDI-ENVVAR in .github/workflows/appimage.yml /tmp/aetherclaude/pr-5933/.github/workflows/appimage.yml:499
  • [MEDIUM] CS-VLN-CMDI-ENVVAR — Vulnerability: CS-VLN-CMDI-ENVVAR in .github/workflows/appimage.yml /tmp/aetherclaude/pr-5933/.github/workflows/appimage.yml:512
  • [MEDIUM] CG-PATH-001 — Potential path traversal in .github/workflows/ci.yml /tmp/aetherclaude/pr-5933/.github/workflows/ci.yml:932
  • [CRITICAL] CS-VLN-CMDI-SUBSHELL — Vulnerability: CS-VLN-CMDI-SUBSHELL in .github/workflows/ci.yml /tmp/aetherclaude/pr-5933/.github/workflows/ci.yml:983
  • [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in .github/workflows/ci.yml /tmp/aetherclaude/pr-5933/.github/workflows/ci.yml:5
  • [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in .github/workflows/ci.yml /tmp/aetherclaude/pr-5933/.github/workflows/ci.yml:17
  • [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in .github/workflows/ci.yml /tmp/aetherclaude/pr-5933/.github/workflows/ci.yml:214
  • [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in .github/workflows/ci.yml /tmp/aetherclaude/pr-5933/.github/workflows/ci.yml:232
  • [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in .github/workflows/ci.yml /tmp/aetherclaude/pr-5933/.github/workflows/ci.yml:251
  • [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in .github/workflows/ci.yml /tmp/aetherclaude/pr-5933/.github/workflows/ci.yml:253
  • [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in .github/workflows/ci.yml /tmp/aetherclaude/pr-5933/.github/workflows/ci.yml:258
  • [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in .github/workflows/ci.yml /tmp/aetherclaude/pr-5933/.github/workflows/ci.yml:322
  • [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in .github/workflows/ci.yml /tmp/aetherclaude/pr-5933/.github/workflows/ci.yml:339
  • [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in .github/workflows/ci.yml /tmp/aetherclaude/pr-5933/.github/workflows/ci.yml:359
  • [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in .github/workflows/ci.yml /tmp/aetherclaude/pr-5933/.github/workflows/ci.yml:403
  • [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in .github/workflows/ci.yml /tmp/aetherclaude/pr-5933/.github/workflows/ci.yml:408
  • [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in .github/workflows/ci.yml /tmp/aetherclaude/pr-5933/.github/workflows/ci.yml:434
  • [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in .github/workflows/ci.yml /tmp/aetherclaude/pr-5933/.github/workflows/ci.yml:459
  • [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in .github/workflows/ci.yml /tmp/aetherclaude/pr-5933/.github/workflows/ci.yml:540
  • [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in .github/workflows/ci.yml /tmp/aetherclaude/pr-5933/.github/workflows/ci.yml:662
  • [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in .github/workflows/ci.yml /tmp/aetherclaude/pr-5933/.github/workflows/ci.yml:709
  • [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in .github/workflows/ci.yml /tmp/aetherclaude/pr-5933/.github/workflows/ci.yml:779
  • [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in .github/workflows/ci.yml /tmp/aetherclaude/pr-5933/.github/workflows/ci.yml:806
  • [CRITICAL] CS-VLN-CMDI-BACKTICK — Vulnerability: CS-VLN-CMDI-BACKTICK in .github/workflows/ci.yml /tmp/aetherclaude/pr-5933/.github/workflows/ci.yml:813

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

sq9fk and others added 3 commits October 17, 2026 11:18
…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>
@jensenpat
jensenpat force-pushed the feat/5928-shuttle-ring branch from b435d9c to 02ea83a Compare October 17, 2026 18:19
@jensenpat
jensenpat dismissed their stale review October 17, 2026 18:20

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
jensenpat previously approved these changes Oct 17, 2026

@jensenpat jensenpat left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses setTone(..., 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-support tag retained and manifest regenerated after rebase; --check passed. 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_test build 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 3bb4694b971e45326b7143d5ea067998e1562b65 was driven through the offscreen, TX-disabled bridge on DEMO-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.

@jensenpat jensenpat left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses setTone(..., 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-support tag retained and manifest regenerated after rebase; --check passed. 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_test build 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 3bb4694b971e45326b7143d5ea067998e1562b65 was driven through the offscreen, TX-disabled bridge on DEMO-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.

@jensenpat
jensenpat merged commit 480a9aa into aethersdr:main Oct 17, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ShuttleXpress / ShuttlePro: support the shuttle ring for variable-speed tuning

3 participants