fix(rc28): replace counter dedup with velocity-proportional tuning#3467
Conversation
…ethersdr#3395) buf[1] is encoder pulse count (rotation speed), not a monotonic per-detent counter. The previous dedup on counter change suppressed all rotation events when buf[1] held a constant value — which is exactly what the device sends during slow continuous rotation (buf[1]=1 throughout), making precise sub-kHz adjustments impossible on all platforms, not just Windows. Fix: emit one Rotate event per HID report with magnitude = buf[1], so slow turns yield small single steps and fast turns yield proportionally larger steps (natural velocity-proportional behaviour). Also add a button-held guard (btns == 0x07) so accidental knob movement while a button is pressed does not retune. Root cause confirmed via raw HID probe on physical RC-28 hardware (Debian 13 Trixie). Fix cross-referenced against two independent open-source RC-28 implementations that both treat buf[1] as speed magnitude with no counter-based dedup: - FlexRC-28 (CerberusSolutions/FlexRC-28): names buf[1] "speed", emits speed directly as step magnitude - wfview (eliggett/wfview): does value += data[1] / value -= data[1], also gates rotation behind data[5]==0x07 (no buttons held) Tested on Debian 13 Trixie with a physical Icom RC-28 (VID 0x0C26, PID 0x001E). Slow continuous rotation now tracks correctly at 1 step per report; fast rotation scales proportionally. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@aethersdr/reviewers — tagging for review. This fixes the RC-28 slow-turn tracking issue (#3395) that affects all platforms. Root cause, cross-reference against FlexRC-28 and wfview, and test results are detailed in the PR description and in the issue comment. |
|
The bot is undergoing a move, I'll review these hopefully later tonight. |
NF0T
left a comment
There was a problem hiding this comment.
Approving. Root cause is correctly identified, the fix is cross-validated against two independent open-source RC-28 implementations (FlexRC-28 and wfview), and you've got hardware confirmation on a physical device. The button-held guard and the m_prevCounter removal are both improvements over the prior state. Clear to merge.
A few notes for the record:
Constitution principle citations — Future PRs from you should end the commit subject line with the load-bearing principle per CONTRIBUTING.md. This fix earns two: Principle VIII (Evidence Over Assertion) — the raw HID probe and dual reference-implementation cross-check overrode the previous assumption about buf[1]'s semantics rather than asserting a fix from first principles — and Principle XI (Fixes Are Demonstrated) — slow-rotation tracking confirmed on physical hardware, fast-rotation velocity scaling confirmed proportional. Neither is a blocker here, just a heads-up for next time.
Branch base — The branch sits a few commits behind main due to a batch of release-prep commits that landed after this was cut. None of those commits touch HidDeviceParser.*, so there's no conflict risk and we're proceeding without requiring a rebase. Not your fault; the timing just worked out that way.
AetherPad emulator — The physical Icom RC-28 is the authoritative source for its own HID wire protocol. Per Principle I (the real device, like FlexLib for the radio protocol, is the authority — emulators and secondary implementations must conform to it), IcomRC28Parser should accurately reflect actual RC-28 hardware behavior, which your HID probe has now confirmed. If the AetherPad Arduino firmware sends buf[1] as a monotonic per-detent counter rather than a pulse-count speed field, the emulator's firmware needs to be updated to match real device behavior — that's an aether-pad repo concern, not a reason to hold this fix.
…ethersdr#3467) Fixes aethersdr#3395 ## Root cause `buf[1]` in the RC-28 HID report is **not** a monotonic per-detent counter — it is an encoder pulse count (rotation speed) since the last report. The previous dedup condition `counter != m_prevCounter` suppressed all rotation events as soon as `buf[1]` stabilised at a constant value, which is exactly what the device does during slow continuous rotation (buf[1]=1 for every report). Result: slow continuous rotation emitted exactly **one** step total regardless of how long the knob was turned, making precise sub-kHz tuning impossible on all platforms. The bot's issue analysis correctly identified the `dir==0x00` edge case on Windows, but the counter-dedup logic is the primary bug and affects Linux and macOS equally. ## Changes - `buf[1]` renamed from `counter` to `speed` throughout - Rotation condition changed from `counter != m_prevCounter` to `btns == 0x07 && speed > 0 && (dir == 0x01 || dir == 0x02)` - Step magnitude changed from fixed `±1` to `sign * static_cast<int>(speed)` — velocity-proportional: slow turn = 1 step/report, fast turn = 10–35 steps/report - `m_prevCounter` member variable removed from `IcomRC28Parser` - Added button-held guard (`btns == 0x07`): accidental knob movement while pressing F1/F2/PTT no longer retunes the radio - Comment blocks in both `.cpp` and `.h` updated to correctly document the wire protocol ## Cross-reference This fix aligns with two independent open-source RC-28 implementations: - **[CerberusSolutions/FlexRC-28](https://github.com/CerberusSolutions/FlexRC-28)** (`src/rc28.js`): explicitly names `data[1]` as `speed` and emits it directly as step magnitude with no counter-based dedup - **[eliggett/wfview](https://gitlab.com/eliggett/wfview)** (`usbcontroller.cpp`): does `value += data[1]` / `value -= data[1]` per report, and gates rotation behind `data[5] == 0x07` (the button-held guard we adopted) ## Testing - Tested on **Debian 13 Trixie** with a physical **Icom RC-28** (VID `0x0C26`, PID `0x001E`) - Slow continuous rotation: `buf[1]=1` constant, 1 step emitted per report — tracks correctly at every step size - Fast rotation: `buf[1]` reaches 20–35, proportionally larger frequency jumps - Button press/release detection unchanged and verified - Button-held guard verified: knob movement during F1/F2 hold does not change frequency - Build: clean on Debian 13 Trixie, Qt 6.x, GCC, `HAVE_HIDAPI` enabled Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Fixes #3395
Root cause
buf[1]in the RC-28 HID report is not a monotonic per-detent counter — it is an encoder pulse count (rotation speed) since the last report. The previous dedup conditioncounter != m_prevCountersuppressed all rotation events as soon asbuf[1]stabilised at a constant value, which is exactly what the device does during slow continuous rotation (buf[1]=1 for every report). Result: slow continuous rotation emitted exactly one step total regardless of how long the knob was turned, making precise sub-kHz tuning impossible on all platforms.The bot's issue analysis correctly identified the
dir==0x00edge case on Windows, but the counter-dedup logic is the primary bug and affects Linux and macOS equally.Changes
buf[1]renamed fromcountertospeedthroughoutcounter != m_prevCountertobtns == 0x07 && speed > 0 && (dir == 0x01 || dir == 0x02)±1tosign * static_cast<int>(speed)— velocity-proportional: slow turn = 1 step/report, fast turn = 10–35 steps/reportm_prevCountermember variable removed fromIcomRC28Parserbtns == 0x07): accidental knob movement while pressing F1/F2/PTT no longer retunes the radio.cppand.hupdated to correctly document the wire protocolCross-reference
This fix aligns with two independent open-source RC-28 implementations:
src/rc28.js): explicitly namesdata[1]asspeedand emits it directly as step magnitude with no counter-based dedupusbcontroller.cpp): doesvalue += data[1]/value -= data[1]per report, and gates rotation behinddata[5] == 0x07(the button-held guard we adopted)Testing
0x0C26, PID0x001E)buf[1]=1constant, 1 step emitted per report — tracks correctly at every step sizebuf[1]reaches 20–35, proportionally larger frequency jumpsHAVE_HIDAPIenabled