Repository navigation
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.
| #endif | ||
| #ifdef HAVE_HIDAPI | ||
| #include "core/HidEncoderManager.h" | ||
| #include "core/ShuttleRateIntegrator.h" |
There was a problem hiding this comment.
This include makes core/ShuttleRateIntegrator.h a gui→engine touchpoint, and Static checks failed on it: aetherd touchpoints require a valid semantic tag: core/ShuttleRateIntegrator.h (run 35912059810).
Add a ui-support entry next to core/HidEncoderManager.h in docs/architecture/aetherd-touchpoint-tags.json — this header is client-side rate math, not radio state — then run python tools/gen_touchpoint_manifest.py and commit the regenerated docs/architecture/aetherd-touchpoints.md. The checker returns on the missing tag before it reports a stale manifest, so both writes are required.
| // ── Contour shuttle ring (#5928) ───────────────────────────────────────── | ||
| { | ||
| auto* group = new QGroupBox("Shuttle Ring (ShuttleXpress / ShuttlePro)"); | ||
| group->setStyleSheet(kGroupStyle); |
There was a problem hiding this comment.
Static checks: the colour ratchet counted setStyleSheet 1043 > 1040 (+3) against this PR's base. These are the three new sites: this kGroupStyle call, note->setStyleSheet(kLabelStyle) just below, and combo->setStyleSheet(...) in addCombo. No new colour literal (unique colours stayed 606).
The older groups on this page already call setStyleSheet, but a new call still raises the count. Use ThemeManager::instance().applyStyleSheet(...) for these three widgets, the same way makeValueLabel does at the top of this file. That styles the widget without adding a counted setStyleSheet site.
|
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.
|
…Principle XI. The ShuttleXpress/ShuttlePro v2 parsers returned the first changed button bit but stored the whole new mask, so a second button changing in the same report was dropped. That includes releases, which could leave a button logically held. A jog change arriving with a button change was also deferred to the next report. - HidDeviceParser gains nextPending() (default: none). HidEncoderManager drains it after every parse(), so parse() keeps its single-return contract and the other parsers are untouched. - The two Contour parsers share ContourShuttleParser, which queues every button edge in button order and then the jog delta. The subclasses only supply the button mask and count. - hid_device_parser_test: simultaneous press/release, byte-3/4 boundary (Xpress 4+5, Pro 8+9), release+press in one report, button+jog in one report, and a short report clearing the queue. On the aethersdr#5927 parser 6 of these fail; all 13 checks pass here. Verified on a ShuttleXpress + FLEX-6500: two buttons pressed together both fire, and releasing them together leaves nothing held. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…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>
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. |
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>
|
The build jobs on I need to correct my previous comment: the "full Windows build" I reported had
Sorry for the extra CI round. |
Summary
Closes #5928.
Stacked on #5931 → #5932. Until those merge, the diff here also shows their commits. Only
4be1554cis new. I'll rebase ontomainas each one lands. #5932's per-report event queue is what lets the ring position ride along without touching the one-event contract.This implements the design from #5928 with all five corrections from the triage review:
ShuttleRateIntegratorintegrates{0, 20, 100, 500, 2k, 8k, 30k, 100k}Hz/s and converts to whole steps of the current step size at tick time, carrying the remainder. Full deflection is ~100 kHz/s at a 10 Hz step and at a 1 kHz step alike (tested).notifyTuneBlockedByLock()fires once per deflection, so direct frequency entry is not cancelled every 40 ms.HidEncoderManagertracksm_lastShuttle.close()emitsshuttleChanged(0)beforem_parser.reset()and independently of them_deviceNameguard.MainWindowalso stops the timer onconnectionChanged(false).HidEncoderInvertDir. There is no second flag.The 60 ms sign-reversal filter is left out, as suggested. The accumulator is reset at centre and on sign change.
Two changes driven by hardware testing
A temporary debug log on a real ShuttleXpress, not included in this PR, showed the software response is symmetric (first step ~0.51 s after reaching ±1 in both directions). This unit's +1 zone is several times wider than −1, though (5.7 s vs 0.7 s at +1/−1 before +2/−2 registered, and 0.88 s vs 0.19 s in an earlier raw capture). At 20 Hz/s, that made the first detent feel dead. It would get far worse with larger steps: at a 1 kHz step, position 1 moved one step every 50 s. So:
max(table, (|pos| + 1) steps/s). At a 10 Hz step nothing changes. At a 1 kHz step, positions 1–4 now move and are distinct (2, 3, 4, 5 kHz/s).Other details
hasShuttle()/shuttlePosition()as state, set at the top ofparse()before any return, so it survives a same-report button edge (tested).ShuttleMappingis a single nested JSON key (Principle V).rc28MappingFieldandshuttleMappingFieldnow share one file-local helper, with no behaviour change for RC-28.QElapsedTimer, capped at 200 ms), because Qt's coarse timer on Windows fires every 33–52 ms rather than every 40 ms.m_hidSnapTimer. It can be a follow-up if wanted.Constitution principle honored
hid_device_parser_test(ring decode plus integrator cases) and on hardware.ShuttleMappingkey.Test plan
ctest -R hid_device_parser_testpasses.Checklist
AppSettingscalls (one nestedShuttleMappingkey)MeterSmoother(no meter UI touched)resources/help/configuring-aethersdr-controls.md)dtcapped)🤖 Generated with Claude Code