feat(hid): RC-28 button-mapping config dialog (#3323)#3328
Conversation
jensenpat
left a comment
There was a problem hiding this comment.
Thanks for the thorough implementation here. I’m holding merge because I found behavior regressions that CI cannot catch.
Findings:
-
src/gui/MainWindow.cpp:6857usesTransmitModel::isMox()to drive the RC-28 TX LED. In this modelm_moxis only updated from amox=transmit status key, and the radio does not send that key on current firmware; localsetMox()also updatesm_transmitting, notm_mox. As written, bit0 stays high and the RC-28 TX LED will not track local PTT, latched PTT, or hardware TX even though the PR claims it mirrors TX. Use the authoritative transmit state,isTransmitting(), for the LED calculation. -
src/gui/RC28MappingDialog.cpp:216-219changes the default F-key mappings from both the existing RC-28 behavior and #3323’s accepted defaults. Existing users with noRC28Mappingblob will now get F1 short =StepCycleinstead ofStepUp, F2 short =Noneinstead ofStepDown, and F2 hold =Noneinstead of the requestedModeCycle. Default-value changes are user-visible, so please preserve the issue’s defaults unless there is an explicit maintainer decision to change them. -
src/core/HidEncoderManager.cpp:118makes the multi-device warning edge-triggered only. If app startup or hotplug detects two RC-28s beforeRC28MappingDialogexists, later opening the dialog only callsrefreshDeviceInfo(), seesisOpen()==false, and shows “Not connected” whilem_multipleDetectedsuppresses repeatedmultipleDevicesDetectedsignals on hotplug retries. The PR says the dialog surfaces the warning until only one remains, so the blocked state needs to be queryable/replayed when the dialog opens, or otherwise refreshed while still blocked.
CI is green and a synthetic merge into current main is textually clean, so this looks close. Please address the items above and I’ll re-review.
- Fix TX LED to use isTransmitting() not isMox() (isMox reads m_mox which firmware never updates; isTransmitting reads m_transmitting which setTransmitting() sets and emits moxChanged on) - Fix defaults: F1 short→StepUp, F2 short→StepDown, F2 hold→ModeCycle (was StepCycle/None/None — now consistent across dialog combos, hold timer, press dispatch, and LED read in all 4 locations) - Add isBlockedByMultiple()/blockedDeviceName() to HidEncoderManager; refreshDeviceInfo() replays multi-device warning for dialogs opened after the initial edge-triggered emit - Add StepUp/StepDown to kActions and ModeCycle to kHoldActions so the dialog combos can display and persist the new defaults - Remove dead RC28_LEDS_LINK and RC28_LEDS_TX_LINK constants - Fix misleading kHoldActions comment Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Hi @jensenpat — thanks for the detailed review. All three findings have been addressed in the latest push: Finding 1 — TX LED reads wrong state Finding 2 — Dialog defaults don't match expected RC-28 behaviour Finding 3 — Multi-device warning not shown when dialog opened after initial detection All test cases were re-run after these changes and the build is clean (no new warnings from RC-28 code). Ready for re-review when you have a chance. |
There was a problem hiding this comment.
Thanks @w5jwp — the follow-up reads well. Single-JSON-blob storage via rc28MappingField/setRc28MappingField matches Principle V cleanly, the per-key hold timers and the post-release LED write are a nicer model than the prior approach, and the isRC28CompatibleId lift to a static avoids the duplication that the multi-device guard would have invited. AppSettings (not QSettings), QPointers for the dialog/encoder, parented QTimers, queued cross-thread invokes for setInvertDirection/setRC28Leds, and bounded log (setMaximumBlockCount(50)) all match house conventions. The multi-device guard's serial-then-path keying with macOS usage-collection caveat in the comment is well-justified, and the m_rc28PttLatched safety drop on disconnect is the right call.
One substantive observation and a couple of small notes:
1. pttMode is read for any "PTT" action, not only the RC-28 TX bar (src/gui/MainWindow.cpp:4083-4084).
A StreamDeck+ user who maps a generic HidKeyAction* to "PTT" will silently inherit the RC-28's latched mode, because the pttMode lookup runs unconditionally for actionName == "PTT". This contradicts the dialog's framing (TX bar is "hardwired to PTT" and the latched/momentary radios live in the RC-28 section) and would surprise a user who configured latched for their RC-28 and then bound a deck key to PTT.
Suggested gate:
const bool latched = m_hidEncoder->isRC28Compatible()
&& HidEncoderManager::rc28MappingField("pttMode", "Momentary") == "Latched";For symmetry the two m_rc28MappingDialog->appendButtonEvent(...) log lines in this branch probably also want a non-RC-28 fallback label (or to be skipped entirely for non-RC-28 sources), since m_rc28PttLatched and the "TX bar" wording are RC-28 concepts.
2. Related (very minor): the short-press/key-press dispatch logs "key N press" to the RC-28 dialog when the source isn't the RC-28 (dispatchHidAction in MainWindow.cpp:4113 → 6876–6877). If the dialog is open while a StreamDeck+ key fires, the user sees an entry that doesn't correspond to anything on their RC-28. Easy to suppress by checking m_hidEncoder->isRC28Compatible() before the appendButtonEvent inside dispatchHidAction, or by passing a "source is RC-28" flag through the gesture label.
3. Stale tune-mode flags across rebinding (minor). If the user has TuneFast latched on (m_hidFastTune == true) and then rebinds f1Hold to something else via the dialog, m_hidFastTune stays true — direct-tune mode keeps eating encoder steps even though nothing on the device toggles it anymore. A one-liner in setRc28MappingField (or in the dialog's combo handler) to clear m_hidFastTune/m_hidFineTune when their owning slot changes would close that. Not blocking — easy to recover by unplug/replug.
Nothing else jumped out. The cross-thread access of the new diagnostic fields (m_devicePath, m_serialNumber, m_releaseNumber, m_multipleDetected, m_blockedDeviceName) matches the existing const-after-open / benign-stale-read convention you explicitly called out in the header — fine.
🤖 aethersdr-agent · cost: $15.2298 · model: claude-opus-4-7
|
Hi @AetherClaude and @jensenpat — all four findings from the last review have been addressed in the latest push ( Finding 1 — Finding 2 — Finding 3 — Stale tune-mode flags after rebinding (minor) Build is clean on macOS Tahoe (arm64), no new warnings from RC-28 code. |
Hotfix release on top of v26.6.1, bundling three already-merged fixes and nothing else: - fix(ci): unblock macOS DMG build broken by missing background image (aethersdr#3349) - fix(gui): only toast "Step: …" on deliberate step changes (aethersdr#3337) - fix(radio-setup): wrap tab pages in QScrollArea so tall tabs stay reachable (aethersdr#3347) Bumps project version 26.6.1 -> 26.6.1.1 (CalVer hotfix sub-patch). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Closes aethersdr#3323. Extends the initial RC-28 support delivered in aethersdr#3293, which shipped working encoder/LED/PTT handling but deferred the configuration panel to a follow-up — this is that follow-up. ## What this adds - RC28MappingDialog: a dedicated config panel for the Icom RC-28. Assign F1/F2 short-press and long-press (600 ms hold) actions, choose PTT mode (Momentary vs Latched), invert encoder direction, and view live device info (VID/PID, path, serial, firmware) plus a button-event activity log. The TX bar is hardwired to PTT and is not remappable. - New tuning actions: Snap to nearest 100 Hz / 500 Hz / 1 kHz / 100 kHz / 500 kHz, selectable as F1/F2 actions. - Per-key independent hold detection (short press on release, latched long-press toggle), and F1/F2 LEDs that mirror their hold-action state. - Multi-device guard: when more than one RC-28 is enumerated, open() is deferred and the dialog surfaces a warning until only one remains. ## Configuration storage (Principle V / XIV) All RC-28 mapping is persisted as a single nested-JSON blob under one root AppSettings key, "RC28Mapping" (fields: f1Press, f1Hold, f2Press, f2Hold, pttMode), via two shared static helpers (HidEncoderManager::rc28MappingField / setRc28MappingField). Writes regenerate the full object and persist atomically (single setValue + save). No new flat keys are introduced for this feature. The pre- existing HidEncoderInvertDir flat key is grandfathered and left as-is. Per the agreement on aethersdr#3323, no migration shim is included because the namespaced flat keys were never shipped to users. ## Namespacing RC-28 F1/F2 configuration is fully separated from the generic StreamDeck+ HidKeyAction* settings, so the two devices can no longer clobber each other's storage or diverge in displayed labels. ## Protocol verification (Principle I / VIII) The LED output report format and button/encoder report layout were verified against the wfview source (src/usbcontroller.cpp RC28 featureLEDControl path) and the open-source FlexRC-28 Node.js reference driver. Behaviour was additionally exercised on physical Icom RC-28 hardware. ## Test plan (all passing) - Build: links clean on macOS Tahoe (arm64), no new warnings. - F1/F2 short-press fires the assigned action; long-press (>=600 ms) fires the assigned hold action as a latched toggle. - F1/F2 LEDs track their hold-action on/off state; no LED writes occur while a button is held (single post-release write). - TX bar transmits in both Momentary (hold-to-talk) and Latched (press-to-toggle) modes; latched TX drops on radio disconnect. - Snap-to-nearest actions round the active slice to the configured grid. - Settings round-trip: every field persists across app restart; the blob is written atomically and survives the AppSettings corruption guard. - Multi-device: a second RC-28 defers open() and raises the dialog warning; removing it restores normal operation via hotplug. CODEOWNERS note: this PR touches maintainer-owned paths (MainWindow.{h,cpp}, CMakeLists.txt) and requires maintainer review. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
All findings from the post-rebase review against v26.6.1.1 resolved. jensenpat review findings: - TX LED now uses isTransmitting() instead of isMox(). m_mox is only updated by the socket mox= key that current firmware does not send; isTransmitting() (driven by setTransmitting()) is the authoritative local transmit state. - Dialog defaults restored to aethersdr#3323 accepted values: F1 short-press → StepUp, F2 short-press → StepDown, F2 hold → ModeCycle, applied consistently in the combo defaults, hold timer defaults, press dispatch defaults, and LED state read. - Multi-device warning now replayed on dialog open: isBlockedByMultiple() and blockedDeviceName() getters added to HidEncoderManager; the dialog's refreshDeviceInfo() checks the blocked state on entry so the warning appears even when the dialog is opened after the edge- triggered multipleDevicesDetected signal has already fired. aethersdr-agent review findings: - PTT latched mode now gated on isRC28Compatible(): a StreamDeck+ key bound to "PTT" always behaves momentary; appendButtonEvent in the PTT block is gated the same way so "TX bar" labels can't appear for non- RC-28 sources. - dispatchHidAction: appendButtonEvent is now gated on isRC28Compatible() so non-RC-28 button events (e.g. StreamDeck+) do not appear in the RC-28 activity log. - Stale tune-mode flags: RC28MappingDialog emits mappingFieldChanged after each setRc28MappingField write; MainWindow clears m_hidFastTune and m_hidFineTune when f1Hold/f2Hold is rebound and calls updateRC28Leds() immediately to extinguish the F-key LED. Additional findings from post-rebase code review: - Dialog title corrected to "Icom RC-28 Button Mapping" for consistency with all other RC-28 references in the application. - AppSettings race: HidKeyAction0/1 → RC28Mapping one-time migration moved to the main thread (MainWindow constructor, before the ext-ctrl invokeMethod) to avoid touching AppSettings from two threads. AppSettings has no mutex. - Migration bypass: migration block moved outside the HidEncoderEnabled gate so users who had HID disabled at upgrade time still get their old HidKeyAction0/1 mappings migrated when they later enable HID via Preferences. The inner !s.contains("RC28Mapping") guard keeps it a true one-shot. - m_rc28PttLatched safety: latch flag cleared in onConnectionStateChanged(false) so a radio TCP drop doesn't leave a phantom latched-TX state that would re-key the radio on reconnect. - m_multipleDetected: changed from bool to std::atomic<bool> with release store and acquire load; m_blockedDeviceName is written before the release store establishing happens-before with the acquire load in blockedDeviceName(). - updateRC28Leds() guard: isOpen() check added so the LED-update invokeMethod is not queued when the device handle is already closed. - loadSettings() idempotency: isOpen() early-return added so repeated Preferences-dialog OK clicks do not close and reopen a live device. The stale HidEncoderAutoDetect check removed; callers gate on the new HidEncoderEnabled key. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
55837f6 to
5ad10a2
Compare
|
Hi @jensenpat and @AetherClaude — rebased onto v26.6.1.1 and completed a full post-rebase review. The branch now has a clean 2-commit history: the original feature commit and one consolidated fix commit covering everything below. All review findings addressedjensenpat (#1) — TX LED reads wrong state jensenpat (#2) — Dialog defaults don't match accepted #3323 values jensenpat (#3) — Multi-device warning not shown when dialog opened after initial detection aethersdr-agent (#1) — aethersdr-agent (#2) — aethersdr-agent (#3) — Stale tune-mode flags after rebinding Additional findings corrected during post-rebase review
Test coverageAll changes have been tested on macOS Tahoe (arm64) and Linux (x86_64). Build is clean with no new warnings from RC-28 code. Verified:
Requesting re-review from @jensenpat and @AetherClaude. |
jensenpat
left a comment
There was a problem hiding this comment.
Re-reviewed at 5ad10a2. All three CHANGES_REQUESTED findings are addressed:
- RC-28 TX LED now derives from
TransmitModel::isTransmitting()inupdateRC28Leds(), so it tracks local/latched PTT and hardware TX rather than the never-sentmox=key. - Default F-key mappings restored to the #3323 defaults: F1 short
StepUp, F2 shortStepDown, F2 holdModeCycle— consistent across the dialog combos and the dispatch fallbacks. - Multi-device blocked state is now queryable (
isBlockedByMultiple()/blockedDeviceName()) andrefreshDeviceInfo()replays the warning when the dialog opens while two encoders remain.
aethersdr-agent's notes are also handled: latched-PTT mode is gated on isRC28Compatible() so generic HID keys bound to PTT stay momentary; the dialog activity-log lines are RC-28-gated; and stale fast/fine-tune flags are cleared on hold-action rebind and on disconnect.
CI green on all platforms; all commits GPG-verified. Approving.
…rsdr#3328) Closes aethersdr#3323. Extends the initial RC-28 support delivered in aethersdr#3293, which shipped working encoder/LED/PTT handling but deferred the configuration panel to a follow-up — this is that follow-up. ## Summary - **RC28MappingDialog** — a dedicated config panel for the Icom RC-28 (Settings → Icom RC-28 Remote Encoder...): assign F1/F2 short-press and long-press (600 ms hold) actions, choose PTT mode (Momentary vs Latched), invert encoder direction, and view live device info (VID/PID, path, serial, firmware) plus a button-event activity log. The TX bar is hardwired to PTT and is not remappable. - **New tuning actions** — Snap to nearest 100 Hz / 500 Hz / 1 kHz / 100 kHz / 500 kHz, selectable as F1/F2 actions. - **Per-key hold detection** — independent timers so F1 and F2 can be held at once; short press fires on release, long-press is a latched toggle. F1/F2 LEDs mirror their hold-action state. - **Multi-device guard** — when more than one RC-28 is enumerated, `open()` is deferred and the dialog surfaces a warning until only one remains. ## Configuration storage (Principle V / XIV) All RC-28 mapping persists as a single nested-JSON blob under one root AppSettings key, `RC28Mapping` (fields: `f1Press`, `f1Hold`, `f2Press`, `f2Hold`, `pttMode`), via two shared helpers (`HidEncoderManager::rc28MappingField` / `setRc28MappingField`). Writes regenerate the full object and persist atomically (single `setValue` + `save`). No new flat keys are introduced for this feature; the pre-existing `HidEncoderInvertDir` flat key is grandfathered. No migration shim is included because the namespaced flat keys were never shipped to users (agreed on aethersdr#3323). ## Namespacing RC-28 F1/F2 configuration is fully separated from the generic StreamDeck+ `HidKeyAction*` settings, so the two devices can no longer clobber each other's storage or diverge in displayed labels. ## Protocol verification (Principle I / VIII) The LED output report format and button/encoder report layout were verified against the **wfview** source (`src/usbcontroller.cpp` RC28 `featureLEDControl` path) and the open-source **FlexRC-28** Node.js reference driver. Behaviour was additionally exercised on physical Icom RC-28 hardware. ## Test plan - [x] Build links clean on macOS Tahoe (arm64), no new warnings - [x] F1/F2 short-press fires the assigned action; long-press (>=600 ms) fires the assigned hold action as a latched toggle - [x] F1/F2 LEDs track their hold-action on/off state; no LED writes while a button is held (single post-release write) - [x] TX bar transmits in both Momentary (hold-to-talk) and Latched (press-to-toggle) modes; latched TX drops on radio disconnect - [x] Snap-to-nearest actions round the active slice to the configured grid - [x] Settings round-trip: every field persists across app restart; blob written atomically and survives the AppSettings corruption guard - [x] Multi-device: a second RC-28 defers open() and raises the dialog warning; removing it restores normal operation via hotplug ## CODEOWNERS Touches maintainer-owned paths (`MainWindow.{h,cpp}`, `CMakeLists.txt`) and requires maintainer review. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: jensenpat <patjensen@gmail.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Closes #3323. Extends the initial RC-28 support delivered in #3293, which shipped working encoder/LED/PTT handling but deferred the configuration panel to a follow-up — this is that follow-up.
Summary
open()is deferred and the dialog surfaces a warning until only one remains.Configuration storage (Principle V / XIV)
All RC-28 mapping persists as a single nested-JSON blob under one root AppSettings key,
RC28Mapping(fields:f1Press,f1Hold,f2Press,f2Hold,pttMode), via two shared helpers (HidEncoderManager::rc28MappingField/setRc28MappingField). Writes regenerate the full object and persist atomically (singlesetValue+save). No new flat keys are introduced for this feature; the pre-existingHidEncoderInvertDirflat key is grandfathered. No migration shim is included because the namespaced flat keys were never shipped to users (agreed on #3323).Namespacing
RC-28 F1/F2 configuration is fully separated from the generic StreamDeck+
HidKeyAction*settings, so the two devices can no longer clobber each other's storage or diverge in displayed labels.Protocol verification (Principle I / VIII)
The LED output report format and button/encoder report layout were verified against the wfview source (
src/usbcontroller.cppRC28featureLEDControlpath) and the open-source FlexRC-28 Node.js reference driver. Behaviour was additionally exercised on physical Icom RC-28 hardware.Test plan
CODEOWNERS
Touches maintainer-owned paths (
MainWindow.{h,cpp},CMakeLists.txt) and requires maintainer review.🤖 Generated with Claude Code