fix(hid): correct IcomRC28Parser byte offsets and report size (#1896)#2870
Conversation
There was a problem hiding this comment.
Thanks @NF0T — solid bit of detective work. The byte-layout table in the description matches the diff exactly and the failure modes you describe (buf[0]==0x01 constant → no tuning, buf[1] alternating → spurious ToggleMox) drop out cleanly from the old code, so the root-cause story is convincing. Scope is clean: only the two files claimed in the title.
One substantive concern, plus a couple of smaller ones.
The buf[0] == 0x01 heuristic collides with macOS seq=0x01 reports
const uint8_t* data = (buf[0] == 0x01) ? buf + 1 : buf;On Windows/Linux, buf[0] is the report ID (always 0x01) — strip is correct.
On macOS (report ID stripped by hidapi), buf[0] is the seq counter, which is 0x01 on the first report of every detent. So on macOS:
- seq=0x01 report: parser sees
buf[0]==0x01, incorrectly strips a byte.data[0]becomes the unused byte (0x00),data[2]becomes the byte after direction (0x00). Theseq == 0x01rotation guard fails, no rotate event fires. - seq=0x02 report: parser correctly doesn't strip.
data[0]==0x02, but the guard requiresseq == 0x01, so no rotate event fires.
Net effect: rotation never fires on macOS with this fix — the exact platform the PR description calls out as broken. Buttons would only work half the time (on seq=0x02 reports), since the button state byte position shifts on the falsely-stripped seq=0x01 reports.
Robust disambiguation can use buf[1]: on Windows/Linux buf[1] is the seq counter (0x01 or 0x02); on macOS buf[1] is the unused byte (0x00). So:
const bool hasReportId = (buf[0] == 0x01) && (buf[1] == 0x01 || buf[1] == 0x02);
const uint8_t* data = hasReportId ? buf + 1 : buf;That cleanly distinguishes "report ID 0x01 followed by seq 0x01/0x02" from "seq 0x01 followed by unused 0x00" and works without platform #ifdefs. Worth confirming against a raw macOS capture from @bobbyrmanson before merging.
Minor — reportSize() = 32 truncates the report ID byte on Windows/Linux
HidEncoderManager calls hid_read(device, buf, parser->reportSize()). On Windows/Linux the wire report is 33 bytes (1 report ID + 32 data), so capping at 32 means we lose the last data byte. The parser only reads bytes 0–4 so this doesn't matter functionally today, but if anyone ever adds a check on a later byte it will silently fail on Windows/Linux but not macOS. Consider reportSize() = 33 — it lets you use len to detect the platform path unambiguously (33 = report-ID prefixed, 32 = stripped) and dodges the buf[0]/buf[1] heuristic entirely.
Small — m_prevButtonState{0x07} first-report behaviour
Initialising to 0x07 (idle) is correct for the common case. If AetherSDR starts with a button physically held, the first non-0x07 report will fire a press event — which is the right behaviour, just worth noting in case anyone wonders why a stuck button generates an event on startup.
Looks good
- Removing the encoder-wrap math and the
m_firstReportgate is correct given the new format isn't a counter. len < 6minimum coversdata[4]access in both strip / no-strip cases — defensive and right.qCDebug/qCWarningaren't relevant here since the parser is silent by design; the manager logs disconnect, which is the right boundary.
Happy to re-review once the macOS path is sorted. The Windows/Linux side of this fix looks right and the description's analysis is convincing — please get @bobbyrmanson to confirm CW/CCW rotation works on macOS before merging.
…sdr#1896) The RC-28 parser had completely wrong byte assumptions: it treated the constant report-ID byte as an encoder counter, and the sequence counter as a button bitmask, causing the knob to do nothing while spuriously toggling ToggleMox on every tick. Rewrite parse() to match the actual 32-byte RC-28 report layout: - Strip report ID (0x01) when present (Windows/Linux hidapi behaviour) - Rotation from direction byte at data[2] (0x01=CW, 0x02=CCW) - Emit only on seq==0x01 to produce one step per detent (device sends 2 reports per click) - Button state from absolute enum at data[4]: F1=0x05, F2=0x03, TX=0x06, idle=0x07 - Expand from 2 to 3 buttons (F1, F2, TX bar) - Fix reportSize() 64→32 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ffafae7 to
68cc479
Compare
|
Thanks for the thorough review — the macOS collision was a real bug. Amended with a
Minimum length guard lowered from 6 → 5 (we need 5 data bytes; macOS reports are 32 so both paths are covered with headroom). @bobbyrmanson — rotation and buttons should now work correctly on macOS. Would appreciate a test before this merges. |
…lias Temporary diagnostic patch to trace whether the AetherPad alias path fires at runtime. Replaces qCDebug(lcDevices) with qWarning() so the output bypasses the aether.devices category filter (which defaults to warning-level). Will be reverted/removed once we have confidence in the alias + PR aethersdr#2870 cherry-pick. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ation Adds a per-call qWarning showing len + first 8 hex bytes of each incoming HID report. Used to verify the on-wire byte layout produced by the AetherPad emulator against PR aethersdr#2870's corrected byte offsets. Sample output (10 taps of Button A on AetherPad in TEST mode): IcomRC28Parser::parse[DIAG] len=33 data="01 01 00 01 00 07 00 00 " IcomRC28Parser::parse[DIAG] len=33 data="01 02 00 01 00 07 00 00 " (× 10 detents) This is the hardware-in-the-loop confirmation that: - Windows hidapi delivers 33-byte reports (Report ID 0x01 + 32 data) - seq counter alternates 0x01 / 0x02 per detent (parser acts on 0x01 only) - direction byte (offset 2 in data, offset 3 on wire) is 0x01 for CW - button state (offset 4 in data, offset 5 on wire) is 0x07 (idle) when no button is pressed - With PR aethersdr#2870 applied, this wire data correctly produces a single Rotate(+1) event per detent (visible as a frequency step on screen) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Built a hardware-in-the-loop bench against this PR while developing a USB control surface that needed RC-28 wire compatibility — sharing results in case it's useful for closing out the review. Bench: Arduino Giga R1 running aether-pad RC-28 emulator firmware. The Giga's mbed PluggableUSB stack hard-codes the device VID/PID at the Arduino default Windows 11 + AetherSDR with this PR applied — 10 taps of a "CW detent" stimulus on the emulator: One macOS 26.5 (Apple Silicon) + same AetherSDR build + this PR — same firmware, same stimulus: Frequency moved correctly on this platform too. One unexpected finding worth flagging for @aethersdr-agent's macOS-strip concern: in this macOS / hidapi build, the Report ID is NOT being stripped — `hid_read` returns 33 bytes including the `0x01` prefix, same as Windows. The `(len >= 33) ? buf + 1 : buf` heuristic still does the right thing (both platforms take the strip-prefix branch and land on correct alignment), but the platform divergence the bot flagged may be hidapi-version-dependent rather than universal. If anyone has a macOS environment where hidapi does strip the ID, that'd be the case worth re-validating against this fix. Net: the Windows/Linux path of this PR is observably correct against real wire bytes that match the documented layout, and macOS is also working here (just not via the strip path). Happy to provide additional captures (raw hidapi reads, button-combo sequences, malformed-length tests) on request. 73 de Nigel G0JKN |
|
@nigelfenton — thank you for the HIL bench, that's a genuinely valuable confirmation. Independent wire-byte verification against an emulator that mirrors the documented layout is exactly the validation this PR needed, and the cross-platform symmetry in your captures (same 33-byte frames, same On your macOS-no-strip finding: that's a useful data point and doesn't undermine the fix. The
So the heuristic costs one branch and one pointer arithmetic to be robust against hidapi version drift on either platform. Worth keeping even though your environment doesn't exercise the strip path. The platform divergence I originally flagged came from older hidapi documentation and a 2022-era macOS report from a different driver; your evidence that modern hidapi on Apple Silicon includes the ID matches the upstream hidapi 0.14+ behavior change. If anyone reading this is on an older macOS hidapi (pre-0.13, or a Homebrew formula that pins lower), the strip-path branch is what catches you. Net: I read this as sufficient evidence to land. The wire format is now documented in code, both observed platform behaviors are handled, and your captures show one Rotate per detent with no spurious MOX — which were the two user-visible failure modes from #1896. 73 de KK7AC 73, Jeremy KK7GWY & Claude (AI dev partner) |
jensenpat
left a comment
There was a problem hiding this comment.
Code looks correct and the rewrite is justified by real captures. Style-guide nits (named constants) are minor; the more substantive followups (button-3 UI, unit tests) can be separate issues.
Thanks Ryan
#3171) ## Summary Adds the Arduino composite VID/PID `0x2341:0x0266` to `HidDeviceParser`'s supported-devices table, routing it to `IcomRC28Parser`. Twelve lines, no behaviour change for any existing device. ## Motivation The aether-pad reference implementation ([github.com/nigelfenton/aether-pad](https://github.com/nigelfenton/aether-pad)) is a small Arduino Giga R1-based RC-28 emulator I've been using as a hardware-in-the-loop test bench for the RC-28 wire protocol. End-to-end validation against PR #2870's report-layout fix was done with this bench on 2026-05-20 (parse-side DIAG trace + frequency display tracking each detent). The bench has been carrying this alias as a local-only patch since then. Filing it upstream so the bench works against unmodified `main` going forward, and so other folks who want to build community RC-28-compatible hardware on Arduino have a path in. ## Why an alias rather than spoofing Icom's VID/PID The Giga R1's mbed PluggableUSB stack hardcodes the device-level VID/PID at the core level — the emulator firmware can't claim `0x0C26:0x001E` even if it wanted to. And spoofing FlexRadio-class hardware identifiers in community firmware is something I'd rather not do for the obvious driver-INF / ethical reasons. Routing the Arduino-default composite VID/PID to the same parser is the clean way in. The wire protocol is identical to the real RC-28 — same 32-byte report layout, same seq/dir/button encoding documented above `IcomRC28Parser`. The alias just gives community hardware a supported route into the existing parser. ## Test plan - [x] Compiles clean (incremental Windows build, 698/698 targets) - [x] Binary launches + stays alive, closes cleanly - [x] Live wire validation already on file from the 2026-05-20 bench (with this exact patch on a feature branch); aether-pad firmware on [nigelfenton/aether-pad@feat/rc28-test-mode](https://github.com/nigelfenton/aether-pad/tree/feat/rc28-test-mode) drives the protocol bytes ## Out of scope The aether-pad also has an AetherControl-style FlexControl-protocol persona in flight — that needs separate plumbing on the AE side (`FlexControlManager::detectPort()`'s VID/PID filter is the blocker there). I'll file that as its own PR once the firmware-side persona work lands. This PR is just the HID alias. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…sdr#1896) (aethersdr#2870) ## Summary The Icom RC-28 parser has never worked correctly. The knob did not tune the radio, and turning it caused spurious transmit toggles. This fixes the root cause: completely wrong byte-offset assumptions in `IcomRC28Parser::parse()`. ## Root cause The parser comment claimed "64-byte reports, byte 0 = encoder counter, byte 1 = button bitmask." The RC-28 actually sends **32-byte reports** with this layout (0-indexed, after the hidapi report ID byte): | Offset | Meaning | |--------|---------| | 0 | Sequence counter: 0x01 = first report of step, 0x02 = second | | 1 | Unused (0x00) | | 2 | Direction: 0x01 = CW, 0x02 = CCW, 0x00 = none | | 3 | Unused (0x00) | | 4 | Button state: 0x07 = idle, 0x05 = F1, 0x03 = F2, 0x06 = TX bar | On **Windows/Linux**, hidapi prepends the report ID (0x01) as `buf[0]`, so the old parser read: - `buf[0]` = 0x01 (constant) → encoder position never changed → **no tuning** - `buf[1]` = sequence counter alternating 0x01/0x02 per knob tick → interpreted as button bitmask bit-0 toggling → **ToggleMox fired on every knob movement** On **macOS**, hidapi strips the report ID, producing different but equally broken behaviour. The user confirmed failure on both platforms. ## Fix - Strip report ID byte when `buf[0] == 0x01` (Windows/Linux) before parsing; otherwise use `buf` directly (macOS). - Read direction from `data[2]`; emit `HidEvent::Rotate` only on `seq == 0x01` (first report of each pair) — the RC-28 sends two reports per detent, so without this guard each click tunes two steps. - Read button state from `data[4]` as an absolute enum, not a bitmask. - Expand from 2 to **3 buttons**: F1 (button 1), F2 (button 2), TX bar (button 3). - Fix `reportSize()`: 64 → 32. Raw HID captures and platform confirmation provided by @bobbyrmanson in the issue thread. Root cause analysis from AetherClaude (2026-05-01) validated against source and independently confirmed here. ## Testing - [ ] Knob CW tunes up by one step per detent (no double-stepping) - [ ] Knob CCW tunes down by one step per detent - [ ] F1 press/release fires button 1 action - [ ] F2 press/release fires button 2 action - [ ] TX bar press/release fires button 3 action - [ ] No spurious transmit toggles on knob movement - [ ] Works on macOS (report ID stripped path) - [ ] Works on Windows/Linux (report ID included path) @bobbyrmanson has offered to test a patched build on macOS and Windows 11. Fixes aethersdr#1896 Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
aethersdr#3171) ## Summary Adds the Arduino composite VID/PID `0x2341:0x0266` to `HidDeviceParser`'s supported-devices table, routing it to `IcomRC28Parser`. Twelve lines, no behaviour change for any existing device. ## Motivation The aether-pad reference implementation ([github.com/nigelfenton/aether-pad](https://github.com/nigelfenton/aether-pad)) is a small Arduino Giga R1-based RC-28 emulator I've been using as a hardware-in-the-loop test bench for the RC-28 wire protocol. End-to-end validation against PR aethersdr#2870's report-layout fix was done with this bench on 2026-05-20 (parse-side DIAG trace + frequency display tracking each detent). The bench has been carrying this alias as a local-only patch since then. Filing it upstream so the bench works against unmodified `main` going forward, and so other folks who want to build community RC-28-compatible hardware on Arduino have a path in. ## Why an alias rather than spoofing Icom's VID/PID The Giga R1's mbed PluggableUSB stack hardcodes the device-level VID/PID at the core level — the emulator firmware can't claim `0x0C26:0x001E` even if it wanted to. And spoofing FlexRadio-class hardware identifiers in community firmware is something I'd rather not do for the obvious driver-INF / ethical reasons. Routing the Arduino-default composite VID/PID to the same parser is the clean way in. The wire protocol is identical to the real RC-28 — same 32-byte report layout, same seq/dir/button encoding documented above `IcomRC28Parser`. The alias just gives community hardware a supported route into the existing parser. ## Test plan - [x] Compiles clean (incremental Windows build, 698/698 targets) - [x] Binary launches + stays alive, closes cleanly - [x] Live wire validation already on file from the 2026-05-20 bench (with this exact patch on a feature branch); aether-pad firmware on [nigelfenton/aether-pad@feat/rc28-test-mode](https://github.com/nigelfenton/aether-pad/tree/feat/rc28-test-mode) drives the protocol bytes ## Out of scope The aether-pad also has an AetherControl-style FlexControl-protocol persona in flight — that needs separate plumbing on the AE side (`FlexControlManager::detectPort()`'s VID/PID filter is the blocker there). I'll file that as its own PR once the firmware-side persona work lands. This PR is just the HID alias. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
The Icom RC-28 parser has never worked correctly. The knob did not tune
the radio, and turning it caused spurious transmit toggles. This fixes
the root cause: completely wrong byte-offset assumptions in
IcomRC28Parser::parse().Root cause
The parser comment claimed "64-byte reports, byte 0 = encoder counter,
byte 1 = button bitmask." The RC-28 actually sends 32-byte reports
with this layout (0-indexed, after the hidapi report ID byte):
On Windows/Linux, hidapi prepends the report ID (0x01) as
buf[0],so the old parser read:
buf[0]= 0x01 (constant) → encoder position never changed → no tuningbuf[1]= sequence counter alternating 0x01/0x02 per knob tick →interpreted as button bitmask bit-0 toggling → ToggleMox fired on
every knob movement
On macOS, hidapi strips the report ID, producing different but
equally broken behaviour. The user confirmed failure on both platforms.
Fix
buf[0] == 0x01(Windows/Linux) beforeparsing; otherwise use
bufdirectly (macOS).data[2]; emitHidEvent::Rotateonly onseq == 0x01(first report of each pair) — the RC-28 sends tworeports per detent, so without this guard each click tunes two steps.
data[4]as an absolute enum, not a bitmask.TX bar (button 3).
reportSize(): 64 → 32.Raw HID captures and platform confirmation provided by @bobbyrmanson in
the issue thread. Root cause analysis from AetherClaude (2026-05-01)
validated against source and independently confirmed here.
Testing
@bobbyrmanson has offered to test a patched build on macOS and Windows 11.
Fixes #1896