Repository navigation
fix(ctr2): scan only the known CTR2 VID:PIDs, not the whole HID bus (#6580) - #6583
Conversation
…6580) — Principle VIII. Ctr2ProxyModel rescans every 2 s on the GUI thread (#6487), and Ctr2HidapiPort::enumerate() called hid_enumerate(0, 0). On Windows that makes hidapi request strings and descriptors from every HID device on the machine, freezing the spectrum and waterfall about 100 ms each rescan. Enumerate each distinct VID:PID in knownCtr2Devices() instead, the same change #6256 made for the Ulanzi dial. Every known entry, the self-named Aether* firmware included, already requires one of these pairs. Verified on a FLEX-6700 / Windows 11: freeze present on 86e90af, gone with only this change applied; main-thread minidumps showed the stall in QTimer -> refreshDevices -> enumerate -> hid_enumerate. Co-authored-by: Don @ cloaked.agency <don@cloaked.agency>
|
Tested on Windows [10/11 build], FLEX-6600, no CTR2 or dial attached. Built this Before (26.10.3): the whole GUI, waterfall included, hitched every ~2 s. Procmon After (this PR): the hitching is gone and the GUI stays smooth. [Optional: Procmon LGTM from a Windows user's side. Since every Windows user on 26.10.3 runs the |
NF0T
left a comment
There was a problem hiding this comment.
Thanks, Ozy. The diagnosis is right and the fix is the smallest one that addresses it. Ctr2ProxyModel's 2 s rescan (#6487) runs on the GUI thread on every session, whether or not a CTR2 is attached, and hid_enumerate(0, 0) on Windows asks every HID device on the machine for its strings and descriptor. Filtering by the table's VID:PID pairs is the same change #6256 made for the Ulanzi dial. With it, every hid_enumerate call in src/ is filtered (HidEncoderManager.cpp:424/450/503 and UlanziDialWindowsManager.cpp:107 already were). There's no sibling call left on the old path.
Scope
| File | Claimed by title/body | Verdict |
|---|---|---|
src/core/Ctr2HidapiPort.cpp (+25/−13) |
yes, enumerate() only |
in scope |
One commit (a766088e, authored 2026-10-18, signature verified), and its merge base is current main. The diff adds no new public surface and removes no guard. The usage-page filter, fromWide mapping and recognised-first stable_sort are unchanged; the old loop body just moved inside a per-ID loop.
Preference vs fix: this is a fix. It points at a defect (#6580) with a measured cause and leaves every default and the rescan interval alone.
Technical
- Correct as written. Duplicate pairs are removed before the calls (five table rows give two pairs, so two calls).
hid_free_enumerationis called once per list, and is also safe whenhid_enumeratereturns null for an absent ID.<utility>is already included forstd::pair/std::as_const. - The firmware updater is not narrowed. I checked this first, because an updater that loses sight of a device in bootloader mode would be a real regression.
FirmwareInstallModel::poll()finds bootloader-mode devices throughEspFlasher::listDevices()(serial), not HID. It only accepts HID entries whosectr2Model()is non-empty, and every one of those is at a table VID:PID. - The stated narrowing is slightly wider than the body says. The body says an off-table vendor-page device "could not be recognised as a CTR2 anyway". That is true for recognition. But
Ctr2ProxyModel::start()(:511-525) does not check recognition: it opens whatever the user selected. OnlytryAutoStart()requiresusbLink(). So before this change a user could pick an unrecognised vendor-page device at an off-table VID:PID and press Start, and now that device no longer appears in the list. An unrecognised device at a table VID:PID (the test'sSome_S3_Gadgetat303a:1001) still appears. No documented path depends on the off-table case:docs/user/docs/ctr2-proxy.mdnames only CTR2-Max/AetherCTR2 and AetherKnob, and both are in the table. So this is not a blocker. It is worth knowing if someone ships link firmware under its own PID. - Default pick, a small overclaim. "Which device is picked by default" is unchanged except in one case: two recognised devices at different VID:PIDs (e.g. a CTR2-Max and a CTR2-MIDI) with no saved setup. The first-recognised pick then follows table order (
303Agroup first) instead of hidapi's bus order. Bus order was arbitrary anyway, and a saved serial still wins. Not worth a change.
CI
All eight checks pass (build, check-macos, check-windows, Static checks, sanitizer configure, CodeQL ×3). CI can't exercise this change: no runner has HID devices, and the unit tests inject the device source. That's why the evidence has to come from running the app, as the body says. I agree that no new test is reasonable here. A test would only pin which arguments get passed to hidapi, not the stall.
Process / Constitution
Fixes #6580 is present, and the PR covers that one issue. Principle VIII is a fair citation: the cause was located with main-thread minidumps and confirmed by two A/Bs on the same machine. The body is honest about the residual cost (each scan still opens every HID interface), the narrowing, and the hotfix question. There is no CHANGELOG.md entry, which is correct. CODEOWNERS: the file falls to the * → @aethersdr/reviewers rule (Tier 3).
What I tried to break
- The updater path in bootloader mode, as described above: it holds, because it uses serial.
- An absent VID:PID returning null: handled.
- Duplicate table rows causing double enumeration and duplicate list entries: the de-duplication prevents both.
- An unrecognised device at a table VID:PID: still listed, so
testDeviceLabels' dev-board case is still reachable at runtime. - Sibling
hid_enumerate(0, 0)callers: none remain. - Not reproduced by me. This Windows machine has no hidapi (
third_party/hidapiabsent), so its build doesn't compileCtr2HidapiPort, and the stall can't be reproduced in demo mode. The runtime evidence I'm relying on is yours (minidump stack plus two A/Bs) and @nreed97's independent before/after on a FLEX-6600 with 9 HID devices and no CTR2. Procmon showed a scan every 2.0 s at about 0.2 s each before the change, and smooth afterwards. One caveat for the record: that comment still has its template placeholders ([10/11 build],[Optional: …]), so the "after" figure isn't stated. The before/after observation stands on its own, though, and it matches yours.
Verdict
Approve. Nothing blocks this. I'd support the hotfix suggestion: every Windows user on v26.10.3 runs this scan.
Squash subject: fix(ctr2): scan only the known CTR2 VID:PIDs, not the whole HID bus. Principle VIII. (#6583)
Forward guidance (non-blocking): if a future firmware ships the CTR2 link under a new VID:PID, it now has to be added to knownCtr2Devices() (and the udev rule) before it even appears in the list. That was already required for it to be recognised, and it's now required for it to be selectable at all. A one-line comment on the table saying so might save someone a confusing afternoon.
Nice work. A minidump stack and an A/B that isolates the cause are what made this easy to review.
73, Ryan NF0T
👨🏼💻 Co-authored by Claude Opus 5.5
Summary
Fixes #6580.
Since v26.10.3 (#6487),
Ctr2ProxyModelrescans for devices every 2 s on the GUI thread, on every session, with or without a CTR2. Each rescan callsCtr2HidapiPort::enumerate(), which calledhid_enumerate(0, 0). On Windows that makes hidapi request the serial, manufacturer and product strings and the report descriptor from every HID device on the machine (the hidapi 0.14.0windows/hid.cpath that #6256 walks through). While that runs the event loop is blocked, so the spectrum and waterfall freeze for about 100 ms every 2 s. How long the freeze lasts depends on how many HID devices the machine has and how slowly they answer.enumerate()now callshid_enumerate(vid, pid)once for each distinct VID:PID inCtr2HidPort::knownCtr2Devices(), which today is0x303A:0x1001and0x2886:0x0056. hidapi then makes the expensive per-device requests only for devices at those IDs. The usage-page filter, the sort and everything downstream are unchanged. This is the same change #6256 made for the Ulanzi dial.What does not change: the rescan timer and its interval, hotplug detection, which device is picked by default, the firmware updater's use of
enumerate(), threads, and defaults.One narrowing, stated: a HID interface on the CTR2 vendor usage page (
0xFF00/0x01) at a VID:PID outside the table no longer shows up in the CTR2 device list. It could not be recognised as a CTR2 anyway, becausematchKnown()requires a table VID:PID for every entry, the self-namedAether*firmware included. On Linux,packaging/linux/70-aethersdr-ctr2.rulesalready grants access to exactly this set.Residual, as #6256 states it: on Windows a filtered
hid_enumeratestill opens every HID interface with no access rights to read its cached attributes. Only the per-device string and descriptor requests go away. The A/B below shows that is enough.Release note for the maintainer: every Windows user on v26.10.3 runs this scan, so this may be worth a hotfix. That is your call.
Constitution principle honored
Principle VIII: the cause and the fix were each established by measurement on the running app, not by reading the code.
Test plan
86e90afc;Ctr2HidapiPort.cppis byte-identical between86e90afcand this base.86e90afc: spectrum/waterfall freeze about 100 ms every 1-2 s. Main-thread minidumps (cdb .dump /m, symbols from the build's PDB) caught the GUI thread inQTimer::timerEvent -> Ctr2ProxyModel::refreshDevices -> Ctr2HidapiPort::enumerate -> hid_enumerate -> HidD_GetManufacturerString.m_deviceRescan->start()commented out: freeze gone.ctr2_proxy_model_testandctr2_usb_closed_retention_testand the other nine CTR2 tests pass (11/11). Two of them,ctr2_usb_relay_testandctr2_usb_closed_retention_test, are socket-based; they were run unchanged and are not touched here.No new test: the change is in which
hid_enumeratecall is made against the real HID bus, which the unit tests replace with injected device sources. The A/B above is the evidence.Checklist
docs/COMMIT-SIGNING.md)AppSettingscalls — use nested-JSON-under-one-key(Principle V)
reverse-engineered from a proprietary binary (Principle IV)
MeterSmoother(AGENTS.md convention). Not applicable: no meter UI touched.docs/and theaffected READMEs. Not
CHANGELOG.md, which is a release-prep file aPR must not add to (AGENTS.md); describe it in the Summary above instead. Not applicable: no user-visible behaviour change beyond the fix itself.
73, Ozy K6OZY · model: claude-opus-5-5