fix(hid): hotplugCheck full scan + HAVE_HIDAPI guard fixes#3298
Conversation
hotplugCheck() never found a device connected after startup when no device was present at loadSettings() time — m_openVid/m_openPid stay zero, so the early-return branch was never taken and the scan loop was missing entirely. Add the missing `return` after the reconnect attempt and a full VID/PID scan for the cold-start case. hidEncoderDefaultAction() / hidEncoderDefaultPushAction() were defined outside #ifdef HAVE_HIDAPI despite being declared inside it; clang flagged the mismatch on non-HIDAPI builds. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Remove hotplugCheck scan loop and #ifdef HAVE_HIDAPI guard fixes; extracted to standalone PR aethersdr#3298 per reviewer request. - Revert migration default (HidEncoderAutoDetect fallback) to "False" (opt-in baseline); the opt-in vs opt-out product decision does not need to block this PR — if opt-out is chosen it can land as a one-line follow-up after merge. - Add source-citation comment to setRC28Leds(): format verified against FlexRC-28 Node.js driver (_sendLED) and wfview usbcontroller.cpp. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Thanks for turning this around so quickly, @w5jwp — the extraction was clean and the commit message captures the root cause well. One note for future PRs: the description template sections were left as placeholder text. Filling them in takes about two minutes and makes the review cycle smoother: Summary — The opening two paragraphs of your description are exactly what belongs here; just move them into the template section rather than above it. Constitution principle honored — Both fixes fit Principle VIII (Evidence Over Assertion): the root causes are demonstrable from the code and verifiable by a no-HIDAPI build. A one-liner like Test plan checkboxes — Even for small fixes, checking the boxes you've actually run ( Checklist — In particular the signing item: branch protection enforces signed commits, so if the push went through it's fine, but leaving the checkbox unchecked makes reviewers pause to verify separately. If you're using Claude to assist with development on this repo, point it at these three files before opening a PR — they're the ones the project expects every AI-assisted contribution to have read:
The AGENTS.md preamble says explicitly: "read this file end-to-end before writing code or recommending merges" — if you drop that file into your Claude context at the start of a session it will handle the template and commit message format automatically. None of this blocks this PR — just flagging for next time. |
NF0T
left a comment
There was a problem hiding this comment.
Thorough pass on both changed files. Both fixes are correct and can merge.
HidEncoderManager.cpp — hotplugCheck() cold-start scan
Root cause confirmed: when no device is present at loadSettings() time, m_openVid and m_openPid stay zero. The if (m_openVid && m_openPid) branch is never entered, the function silently returns, and any device plugged in after launch is never detected regardless of how long the hotplug timer keeps firing.
The fix is correct and internally consistent:
- The
returnadded at the end of theif (m_openVid && m_openPid)block is right — when we have a recorded device we should keep retrying that specific device, not fall into a general scan. - The cold-start scan loop uses
const auto* devices/int count— identical pattern todetectDevice()(line 36) andopen()(line 77). No new idioms introduced. - The new
ifinside the loop has braces, consistent with AGENTS.md style. ✅ - The
m_hotplugTimer->stop()inside the loop is redundant (open()already stops it at line 74), but this matches the existing double-stop pattern in the warm-start path and is benign.
The pre-existing no-brace if (open(m_openVid, m_openPid)) m_hotplugTimer->stop(); (lines 147–148) violates the braces-on-all-control-flow rule from AGENTS.md. That predates this PR so I'm not putting it on you here, but it's a candidate for a separate cleanup.
MainWindow.cpp — #ifdef HAVE_HIDAPI guard fix
Confirmed the full extent of the problem: three methods are declared inside #ifdef HAVE_HIDAPI in MainWindow.h (lines 551–558) — hidEncoderDefaultAction, hidEncoderDefaultPushAction, and refreshStreamDeckLabels. refreshStreamDeckLabels() is already inside the large #ifdef HAVE_HIDAPI block at lines 6753–6953. The two hidEncoderDefault* functions at lines 6730 and 6742 were the only unguarded definitions. The PR wraps exactly those two. The guard is complete — nothing else left unguarded. ✅
The resulting structure (two consecutive #ifdef HAVE_HIDAPI blocks after the patch) is syntactically clean and a common pattern throughout the file.
handleVirtualFlexControlWheel() immediately above is correctly unguarded — its declaration in the header (line 362) is outside any guard. ✅
Constitution & project compliance
- Principle VIII — both root causes are directly verifiable: the cold-start bug is a straightforward missing code path; the guard mismatch is a hard compiler error on no-HIDAPI builds (clang: out-of-line definition does not match any declaration). ✅
- No new
AppSettingscalls, noMeterSmootherviolations, no visual or UX changes. ✅
CI
All five checks green (build, check-windows, check-macos, analyze(cpp), CodeQL). ✅
CODEOWNERS
Both changed files (src/core/HidEncoderManager.cpp, src/gui/MainWindow.cpp) fall under the * Tier 3 catchall — @aethersdr/reviewers. This review constitutes a valid approving vote for merge from that tier.
Approved. Clean extraction, correct fixes, no regressions.
Closes #616 ## Summary - **Fixes silent no-op**: `IcomRC28Parser` had wrong byte offsets — direction was being read from `buf[2]` (always `0x00`) instead of `buf[3]`, and buttons from `buf[4]` instead of `buf[5]`. All encoder events were silently discarded. Layout verified against hardware captures and cross-referenced with the open-source FlexRC-28 reference driver. - **Counter-based deduplication**: the RC-28 sends bursts of identical reports per detent; dedup on `buf[1]` emits exactly one step per physical click. - **LED control**: LINK LED lights when AetherSDR opens the device and extinguishes on close/disconnect. TX LED mirrors MOX state. Both sync correctly on hot-plug. Output report format (`[0x00, 0x01, ledByte, zeros…]`, 33 bytes, active-low bitmask) verified against the FlexRC-28 Node.js driver (`_sendLED`) and wfview `src/usbcontroller.cpp` (RC28 `featureLEDControl` path). - **Button defaults for RC-28**: F1=StepUp, F2=StepDown, TX bar=PTT (hold-to-talk — fires `setTransmit` on both press and release). - **Step-change toast**: status bar briefly shows the new step size when F1/F2 is pressed. - **`isRC28Compatible()`**: covers both Icom VID/PID (`0x0C26:0x001E`) and the AetherPad emulator (`0x2341:0x0266`), which runs the same wire protocol. > **Note:** The `hotplugCheck()` scan fix and `#ifdef HAVE_HIDAPI` guard fix have been extracted to PR #3298 per reviewer request — both were bugs in `main` independent of this work. > **Migration default (open question):** The one-time migration fallback for `HidEncoderAutoDetect` has been left at `"False"` (opt-in) — matching the pre-PR baseline. This means clean installs will not have `HidEncoderEnabled` set automatically. Whether RC-28 support should be opt-in or opt-out for new users is a product decision left open for the merge review. ## Limitations / follow-up work - **Button customization UI is not included in this PR.** F1, F2, and TX bar actions can be overridden via the `HidKeyAction{0-2}` settings keys, but there is no UI for this yet. A follow-up PR will add a configuration panel. - **Direct USB connection only.** This implementation supports the RC-28 connected via USB directly to the computer running AetherSDR. Connecting the RC-28 directly to the FlexRadio hardware is not addressed by this change. ## Test plan - [ ] RC-28 LINK LED lights on app launch; goes out when AetherSDR quits - [ ] Rotating encoder CW/CCW moves VFO frequency - [ ] F1 steps tuning rate up; status bar shows new step size - [ ] F2 steps tuning rate down - [ ] Holding TX bar activates MOX and lights TX LED; releasing drops both - [ ] Hot-plug: unplug and re-plug RC-28 while app is running — LINK LED returns and encoder/buttons resume without restart ## Platform testing | Platform | Result | |---|---| | macOS Tahoe (arm64) | ✅ Tested | | Debian 13 (arm64) | ✅ Tested | 🤖 Generated with [Claude Code](https://claude.ai/claude-code) --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
…#3298) hotplugCheck() never found a device connected after startup when no device was present at loadSettings() time — m_openVid/m_openPid stay zero, so the early-return branch was never taken and the scan loop was missing entirely. Add the missing `return` after the reconnect attempt and a full VID/PID scan for the cold-start case. hidEncoderDefaultAction() / hidEncoderDefaultPushAction() were defined outside #ifdef HAVE_HIDAPI despite being declared inside it; clang flagged the mismatch on non-HIDAPI builds. <!-- Thanks for contributing to AetherSDR! Before opening the PR, please: - Read AGENTS.md if this is your first contribution (or your AI tool's first contribution) — it has the conventions every contributor agent is expected to follow. - Read CONTRIBUTING.md for the commit-signing and branch-protection rules. - If you're an AI agent: claim the originating issue first by assigning yourself via `gh issue edit <N> --add-assignee <handle>`. Double- assigning alongside the @aethersdr-agent triage bot is fine. --> ## Summary <!-- One-paragraph description of what changes and why. If this fixes an issue, lead with "Fixes #N" or "Closes #N". --> ## Constitution principle honored <!-- Cite the principle by Roman numeral when relevant. E.g. "Principle V — new feature uses nested-JSON persistence." "Principle II — MeterSmoother used for the new meter widget." See CONSTITUTION.md for the full list. --> ## Test plan - [ ] Local build passes (`cmake --build build`) - [ ] Behavior verified on a real radio if applicable - [ ] Existing tests pass (CI) - [ ] Reproduction steps documented if user-reported bug ## Checklist - [ ] Commits are signed (`docs/COMMIT-SIGNING.md`) - [ ] No new flat-key `AppSettings` calls — use nested-JSON-under-one-key (Principle V) - [ ] All meter UI uses `MeterSmoother` (Principle II) - [ ] Documentation updated if user-visible behavior changed - [ ] Security-sensitive changes reference a GHSA if applicable Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Closes aethersdr#616 ## Summary - **Fixes silent no-op**: `IcomRC28Parser` had wrong byte offsets — direction was being read from `buf[2]` (always `0x00`) instead of `buf[3]`, and buttons from `buf[4]` instead of `buf[5]`. All encoder events were silently discarded. Layout verified against hardware captures and cross-referenced with the open-source FlexRC-28 reference driver. - **Counter-based deduplication**: the RC-28 sends bursts of identical reports per detent; dedup on `buf[1]` emits exactly one step per physical click. - **LED control**: LINK LED lights when AetherSDR opens the device and extinguishes on close/disconnect. TX LED mirrors MOX state. Both sync correctly on hot-plug. Output report format (`[0x00, 0x01, ledByte, zeros…]`, 33 bytes, active-low bitmask) verified against the FlexRC-28 Node.js driver (`_sendLED`) and wfview `src/usbcontroller.cpp` (RC28 `featureLEDControl` path). - **Button defaults for RC-28**: F1=StepUp, F2=StepDown, TX bar=PTT (hold-to-talk — fires `setTransmit` on both press and release). - **Step-change toast**: status bar briefly shows the new step size when F1/F2 is pressed. - **`isRC28Compatible()`**: covers both Icom VID/PID (`0x0C26:0x001E`) and the AetherPad emulator (`0x2341:0x0266`), which runs the same wire protocol. > **Note:** The `hotplugCheck()` scan fix and `#ifdef HAVE_HIDAPI` guard fix have been extracted to PR aethersdr#3298 per reviewer request — both were bugs in `main` independent of this work. > **Migration default (open question):** The one-time migration fallback for `HidEncoderAutoDetect` has been left at `"False"` (opt-in) — matching the pre-PR baseline. This means clean installs will not have `HidEncoderEnabled` set automatically. Whether RC-28 support should be opt-in or opt-out for new users is a product decision left open for the merge review. ## Limitations / follow-up work - **Button customization UI is not included in this PR.** F1, F2, and TX bar actions can be overridden via the `HidKeyAction{0-2}` settings keys, but there is no UI for this yet. A follow-up PR will add a configuration panel. - **Direct USB connection only.** This implementation supports the RC-28 connected via USB directly to the computer running AetherSDR. Connecting the RC-28 directly to the FlexRadio hardware is not addressed by this change. ## Test plan - [ ] RC-28 LINK LED lights on app launch; goes out when AetherSDR quits - [ ] Rotating encoder CW/CCW moves VFO frequency - [ ] F1 steps tuning rate up; status bar shows new step size - [ ] F2 steps tuning rate down - [ ] Holding TX bar activates MOX and lights TX LED; releasing drops both - [ ] Hot-plug: unplug and re-plug RC-28 while app is running — LINK LED returns and encoder/buttons resume without restart ## Platform testing | Platform | Result | |---|---| | macOS Tahoe (arm64) | ✅ Tested | | Debian 13 (arm64) | ✅ Tested | 🤖 Generated with [Claude Code](https://claude.ai/claude-code) --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
hotplugCheck() never found a device connected after startup when no device was present at loadSettings() time — m_openVid/m_openPid stay zero, so the early-return branch was never taken and the scan loop was missing entirely. Add the missing
returnafter the reconnect attempt and a full VID/PID scan for the cold-start case.hidEncoderDefaultAction() / hidEncoderDefaultPushAction() were defined outside #ifdef HAVE_HIDAPI despite being declared inside it; clang flagged the mismatch on non-HIDAPI builds.
Summary
Constitution principle honored
Test plan
cmake --build build)Checklist
docs/COMMIT-SIGNING.md)AppSettingscalls — use nested-JSON-under-one-key(Principle V)
MeterSmoother(Principle II)