fix: gate device config panels on HID++ capabilities (closes #127)#147
Conversation
A Bluetooth-direct / wired device probed through `probe_direct` had its kind hard-coded to `DeviceKind::Unknown` — the receiver pairing register that supplies a kind on the Bolt path doesn't exist there. The GUI's wired-keyboard lighting heuristic (`supports_lighting`, added in #29) then treats any `Unknown` + `Direct` device as a lighting-only keyboard, so a Bluetooth-direct MX Anywhere 3S / MX Master 3 lost its Buttons and Pointer tabs and showed only an irrelevant color panel (#127) — leaving no way to remap back/forward. Read the device's marketing type from HID++ feature `0x0005` (DeviceTypeAndName), folded into the existing `probe_features` session so it costs one extra short round-trip and no new device handshake. The direct path now reports the real kind (Mouse, …); the Bolt path keeps its pairing-register kind and falls back to `0x0005` only when that register reads `Unknown`. Fixes #127
Greptile SummaryThis PR fixes #127 by replacing kind-based UI panel gating with capability-based gating — panels now show when the device's HID++ feature table announces the relevant feature, not when
Confidence Score: 5/5The change is safe to merge; the capability-gating logic is deterministic and well-tested, the probe cache is correctly isolated per-device, and the removal of exposes_peripheral_feature is sound because enumerate_features already returns all feature IDs including ReprogControls and AdjustableDpi. The fix is narrow and well-contained: the UI path now reads from a Capabilities struct derived entirely from the device's own HID++ feature table, so panel display is provably correct for any device that answers enumerate_features. Fallbacks (presumed_from_kind for offline devices, asset-registry kind overriding the Bolt register) are documented and tested. The concurrent probe dispatch and cache eviction logic have unit tests covering edge cases like grace-period resets and stale-tick boundaries. The only nit is the futures-concurrency version being pinned directly rather than through the workspace. No files require special attention; the most complex file (inventory.rs) has thorough inline documentation and tests. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[HID++ device detected] --> B{Bolt receiver?}
B -- Yes --> C[probe_bolt_slot\npairing register → bolt_kind]
B -- No --> D[probe_direct\nregister = Unknown]
C --> E[probe_or_reuse\ncached? stale?]
D --> E
E -- fresh probe --> F[probe_features\nDevice::new + enumerate_features]
E -- cache hit --> G[reuse Cached probe]
F --> H[Capabilities::from_feature_ids]
F --> I[0x0005 → map_device_type → probed_kind]
F --> J[Battery + ModelInfo]
H --> K[ProbedFeatures]
I --> K
J --> K
K -- capabilities.is_some --> L[Cache probe\nCacheOutcome::Fresh]
K -- capabilities.is_none --> M[Don't cache\nfallback to prior or default]
L --> N[resolve_device_kind\nprobed wins unless Unknown]
G --> N
N --> O[PairedDevice.kind + capabilities]
O --> P[build_device_list\neffective_kind: asset > HID++]
P --> Q[DeviceRecord.capabilities]
Q -- Some --> R[tabs_for: use measured caps]
Q -- None --> S[tabs_for: presumed_from_kind]
R --> T{caps.buttons?}
R --> U{caps.pointer?}
R --> V{caps.lighting?}
T -- Yes --> W[Buttons tab]
U -- Yes --> X[Pointer tab]
V -- Yes --> Y[Lighting tab]
Reviews (7): Last reviewed commit: "fix(hid): don't let a failed re-probe co..." | Re-trigger Greptile |
`probe_features` queried HID++ 0x0005 for every online device, but the Bolt path only uses the result when the pairing register returned `Unknown`. For a well-behaved Bolt device that round-trip was discarded, eating into the shared 5s PROBE_BUDGET (worse with many slots / a slow-waking device). Gate the read behind a `read_device_type` flag: the direct path always asks, the Bolt path only when its register kind is `Unknown`.
…gister The reporter on #127 connects over a Logi Bolt receiver, not Bluetooth- direct — so the fix has to hold on the Bolt path too. A Bolt-routed device only shows the lighting-only UI when it is classified as a keyboard, which means the receiver's pairing register is misreporting an MX Anywhere 3S as `Keyboard`. The previous precedence (register wins, 0x0005 only fills an `Unknown`) could not correct that. Flip it: the device's own `0x0005` (DeviceTypeAndName) report is the authoritative kind for any online device; the pairing-register kind is the fallback for offline devices (no probe) or a `0x0005` type we don't model. This corrects a register that names the wrong concrete kind, not just an `Unknown` one — covering both the Bolt and the Bluetooth-direct paths in #127. The 0x0005 read is no longer conditional, but it is no longer discarded either: it is the primary source and runs only for online devices that just answered two other reads, so it is one cheap short round-trip with a real purpose (addresses the earlier review note about a wasted probe).
|
Pushed a follow-up that strengthens the fix and addresses the review note. Why the precedence flipped. #127's reporter is on a Logi Bolt receiver, not Bluetooth-direct. On the Bolt path a device only renders the lighting-only UI when it's classified as a The device's own HID++ Re: the discarded-round-trip note. The The |
Layered guards so a misclassified device can't silently lose its config panels again (the #127 failure mode), and so the same trap doesn't recur when keyboard configuration lands: - Asset-registry cross-check: surface the registry's curated per-model `type` into `ResolvedAsset.kind` and prefer it over the runtime HID++ kind when a device matched a known depot. `DeviceKind::from_registry_type` case-folds the field (the registry ships both `"mouse"` and `"MOUSE"`). - Observability: log at debug when device-kind sources disagree — the `0x0005` probe vs the Bolt pairing register, and the resolved kind vs the asset registry. Turns a silent misclassification into a line a `RUST_LOG=…=debug` bug report will show. - Regression tests pinning the `tabs_for` contract: a pointer device always gets Buttons + Pointer and never collapses to lighting-only; a keyboard gets Lighting, not pointer tabs; the wired-keyboard `Unknown`+direct lighting fallback is kept intentionally. Documents the four incompatible "device type" vocabularies (Bolt register, `0x0005`, asset string, our enum) and flags that `tabs_for` should move to HID++ capability-driven gating before keyboard support.
#127's root cause was gating UI panels on a coarse, fallible `DeviceKind` proxy. Fixing the proxy (0x0005, asset cross-check, precedence) only made the guess more accurate; the disease was using identity to decide capability at all. A panel's presence has nothing to do with "is this a mouse" — it depends on whether the device exposes the HID++ feature that drives it. Pivot to capability-driven gating: - openlogi-core: `Capabilities { buttons, pointer, lighting }`, derived from a device's feature-ID set (`from_feature_ids`) — buttons=0x1b0x, pointer=0x2201/0x2202, lighting=0x8040/0x8070/0x8071/0x8080/0x8081/ 0x198x/0x1990. `presumed_from_kind` is the fallback for devices we can't probe. - openlogi-hid: `probe_features` captures the feature-ID list that `enumerate_features` already returns (zero extra round-trips) and folds every read into a `ProbedFeatures` struct. The Bluetooth-direct peripheral discriminator now reuses these capabilities, deleting the redundant `exposes_peripheral_feature` probe (and its extra session). - openlogi-gui: `DeviceRecord` carries `Option<Capabilities>`; the snapshot merge carries the last-known set forward so a sleeping device keeps its panels. `tabs_for` shows Buttons iff `caps.buttons`, Pointer iff `caps.pointer`, Lighting iff `caps.lighting` — `is_configurable_pointer` and the `supports_lighting` Unknown+Direct heuristic are deleted. `kind` is now identity only (icon/label) — a wrong kind is a cosmetic glitch, never a lost panel. This closes #127 at the root and removes the trap that would have hidden a keyboard's future button config behind a `kind == Keyboard` check.
Reframed: capability-driven gating (the root fix)The earlier commits made the This PR now pivots to the essential line:
What this resolves from the review:
No #29 regression: a wired G-series keyboard exposes Verified: |
`enumerate` awaited each candidate's probe serially, so a single asleep/unresponsive node that burns the full 5s PROBE_BUDGET stalled the whole snapshot behind it — and the watcher re-runs every ~2s, so a device dual-paired over both a Bolt dongle and Bluetooth (one path dormant) produced a recurring 5s hitch and a repeating timeout warning. Probe all candidates concurrently via futures-concurrency's `Vec::join` (concurrent on one task — no Send bound, fits the current-thread watcher runtime). The tick is now bounded by the slowest probe, not their sum; a dead node no longer delays the live devices. Each candidate is an independent HID interface, so there's no shared state to contend on.
The polling watcher re-handshook every device every ~2s: a full `enumerate_features` feature-table walk (the dominant cost, ~20+ HID++ round-trips) plus battery/model/0x0005 reads — even though a device's model, feature table, and marketing type never change while it's connected. Only the battery is volatile. Add a stateful `Enumerator` that holds a per-device probe cache keyed by the device's own identity (Bolt unit id from the cheap pairing register, or direct vendor+product). A known device reuses its cached probe and skips the feature-table walk, re-probing only on a cache miss or every REFRESH_TICKS (~30s) to refresh the battery. The one-shot `enumerate` free function (used by the CLI) runs against a fresh empty cache, so its behavior is unchanged. Keying on the device's true identity (never its slot) means a re-paired device can't inherit another's cached capabilities. Cache reads are shared `&` across the concurrent probes; updates are collected and folded in after the join, so no `RefCell` is needed. As a bonus, an offline device now reuses its cached model + capabilities, so a sleeping mouse keeps its identity and config panels instead of dropping out of the list.
The direct (Bluetooth/USB) probe cache keyed on vendor+product, which identifies a *model*, not a unit — so two same-model devices on one interface would have the second inherit the first's cached serial/model. Key on `async_hid::DeviceInfo::id` instead — the OS-assigned HID node id (macOS registry-entry id, Linux dev path, Windows interface path). It's unique per node, so distinct units never collide, and stable while connected, so the cache still hits across ticks. Available without probing, like the vendor/product it replaces.
Code review of the probe cache found that a fresh re-probe (on cache miss or staleness) that *failed* — e.g. a transient enumerate_features timeout — returned ProbedFeatures::default() and cached it, overwriting the device's last-known data. The device then showed battery=None (and could vanish, since model_info=None makes build_device_list skip it) and stayed that way for up to REFRESH_TICKS. Route every probe decision through `probe_or_reuse`: - A re-probe is cached only when it succeeds (capabilities is Some, i.e. the feature-table walk completed). - A failed re-probe of a known device falls back to its last-known cached data instead of overwriting it — immutable data stays valid; a transient glitch no longer drops the device. - probe_direct no longer caches a rejected non-peripheral (receiver secondary interface), so it can't leave a permanent dead entry. Also add grace-period eviction (`evict_unseen`): entries for devices not seen for CACHE_MISS_GRACE consecutive ticks are dropped, bounding the cache instead of letting re-paired / re-plugged identities accumulate forever. probe_one now returns a CacheOutcome per device (Fresh / Seen / Unkeyed) to drive both insertion and eviction.
Problem
On
master, a Bluetooth-direct / wired MX Anywhere 3S / MX Master 3 shows only a Lighting (color) tab — the Buttons and Pointer tabs vanish, so there's no way to remap back/forward or anything else (#127).Root cause
Two pieces interact:
inventory.rs::probe_direct(the receiver-less path: Bluetooth-direct, USB-C) hard-codeskind: DeviceKind::Unknown. There's no Bolt pairing register on this path to supply a real kind, and nothing else filled it in.app.rs::supports_lighting(added in feat: add wired G-series keyboard RGB control #29 for wired G-series keyboards) treats anyUnknown+DeviceRoute::Directdevice as a lighting-capable keyboard.So a Bluetooth-direct mouse is indistinguishable from a wired keyboard:
is_configurable_pointer(Unknown)isfalse(no Buttons/Pointer) whilesupports_lightingistrue(Lighting only). A Bolt-connected unit reportsMouse(0x02)from the pairing register and is unaffected — which is why the same model works over the Bolt receiver but breaks over Bluetooth.Fix
Read the device's marketing type from HID++ feature
0x0005(DeviceTypeAndName) and use it for the kind:probe_featuressession, so it's one extra short round-trip on an already-open device — no newDevice::new/enumerate_featureshandshake.0x0005type directly → a Bluetooth-direct mouse is nowMouse, restoring the Buttons + Pointer tabs and dropping the spurious Lighting tab.0x0005only when that register readsUnknown— so currently-correct devices are untouched.map_device_typemaps the0x0005enum to ourDeviceKind;resolve_device_kindencodes the precedence (primary wins unlessUnknown, then probe) and is unit-tested.Verification
cargo test -p openlogi-hid— 3 new precedence tests pass.cargo clippy -p openlogi-hid --all-targetsclean under-D warnings.kind == Mouse; this just needs to confirm the device answers0x0005at index0xffon the direct path.Notes / out of scope
CGEventTap(it's outside button range 0–4; seehook_runtime.rs), plus pointer drift and a scroll-direction request. If that user is on a post-feat: add wired G-series keyboard RGB control #29 build over Bluetooth this fix restores their tabs, but the DPI-button limitation is separate and not addressed here.Keyboard(0x01)rather thanUnknown— would not be covered, but there's no evidence of it.Fixes #127