fix(hidpp): retry lost feature-table reads during enumeration - #469
fix(hidpp): retry lost feature-table reads during enumeration#469kohjunhao wants to merge 2 commits into
Conversation
`enumerate_features` walked the feature table with `get_feature(i).await?`, so a single lost report aborted the whole walk — discarding a table that may already have been thirty entries deep. Over Bluetooth-LE individual reports do get dropped, and each one cost the channel's full `SEND_RESPONSE_TIMEOUT` (5s), which on its own exceeded the budget most callers give the entire walk. Read each entry through `read_feature_entry`, which bounds an attempt at 700ms and re-asks up to four times with a short backoff. A HID++ round trip that is going to answer answers in tens of milliseconds, so the shorter deadline costs nothing on a healthy link and converts a 5s stall into a fast retry. Feature-level refusals and unsupported responses return immediately: the device answered, so re-asking cannot change the answer. Only transport failures retry. The channel test mock becomes `pub(crate)` so device-level tests can drive a link that never answers.
`probe_direct` applied the receiver-secondary discriminator to every probe, including ones whose feature walk never completed. An empty probe reports no battery and no configuration features, which is indistinguishable from a Bolt receiver's secondary interface — so a real Bluetooth-direct mouse was rejected as a phantom and dropped from the inventory. Check `walk_succeeded` first and settle an incomplete walk as a transient probe failure carrying `seen(...)` rather than `Unkeyed`, so the node keeps its cache entry and its last-good inventory is replayed while the link recovers. Raise `PROBE_BUDGET` to 25s so the outer `timeout` cannot cancel the per-entry retries before they help; at 6s one lost report consumed the whole budget.
Greptile SummaryThis PR makes HID++ feature enumeration resilient to dropped reports and preserves direct-device inventory across incomplete walks.
Confidence Score: 4/5The PR appears safe to merge, with a non-blocking concern that the BLE-specific timeout increase also lengthens stalled receiver, watcher, and one-shot enumeration paths. The retry and transient-failure handling preserve feature-enumeration correctness, but using a single 25-second budget across transports can delay watcher updates and compound across one-shot retries. Files Needing Attention: crates/openlogi-hid/src/inventory.rs
|
| Filename | Overview |
|---|---|
| crates/openlogi-hidpp/src/device.rs | Adds per-entry feature-table deadlines, retries, backoff, and a no-response retry test. |
| crates/openlogi-hid/src/inventory/probe.rs | Prevents an incomplete direct feature walk from being mistaken for a receiver secondary interface. |
| crates/openlogi-hid/src/inventory.rs | Expands the shared probe timeout to accommodate BLE retries, also lengthening stalled receiver and one-shot paths. |
| crates/openlogi-hidpp/src/channel.rs | Makes existing channel test mocks crate-visible for device-level tests. |
Sequence Diagram
sequenceDiagram
participant Inventory
participant Device
participant Channel
participant Peripheral
Inventory->>Device: enumerate_features()
loop Each feature-table entry
Device->>Channel: get_feature(index)
Channel->>Peripheral: HID++ request
alt Response within 700 ms
Peripheral-->>Channel: Feature information
Channel-->>Device: Entry
else Missing response
Device->>Device: Wait 120 ms and retry (up to 4)
end
end
alt Walk completes
Device-->>Inventory: Capabilities
else Walk fails
Device-->>Inventory: Transient failure
Inventory->>Inventory: Replay last-good inventory
end
Reviews (1): Last reviewed commit: "fix(hid): keep a direct node's identity ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| /// table over a link that drops individual reports, which `hidpp::device` | ||
| /// re-asks for per entry. At 6 s one lost report consumed the whole budget and | ||
| /// the walk was abandoned mid-table, surfacing as a mouse that never appeared. | ||
| const PROBE_BUDGET: Duration = Duration::from_secs(25); |
There was a problem hiding this comment.
The 25-second budget wraps complete node probes across every transport, so a stalled receiver or direct node delays watcher refresh and hotplug handling for up to 25 seconds, while four incomplete one-shot attempts can take roughly 101 seconds. Consider applying the longer allowance specifically to Bluetooth-direct feature walks.
Knowledge Base Used: openlogi-hid: device inventory, pairing, and HID++ feature control
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
2 participants
Summary
On a Bluetooth-direct MX Master 3S the GUI showed "No device connected" while the device was plainly present and answering. Every tick logged the same sequence:
Two independent causes:
Device::enumerate_featureswalked the feature table withfeature_set_feature.get_feature(i).await?, so a single lost report aborted the entire walk — discarding a table that may already have been thirty entries deep. Over Bluetooth-LE individual reports do get dropped, and each one cost the channel's fullSEND_RESPONSE_TIMEOUT(5s), which alone exceeds the 6sPROBE_BUDGETthe caller gave the whole walk. One dropped packet was fatal by construction.probe_directthen applied the receiver-secondary discriminator to that empty probe. "No battery and no configuration features" is exactly what a Bolt receiver's secondary interface looks like, so a real mouse was rejected as a phantom and dropped from the inventory.This matches #468, which reports the same partial-walk-then-timeout on an MX Anywhere 3S.
Changes
crates/openlogi-hidppread_feature_entry: bounds a single feature-table read at 700ms and re-asks up to four times with a 120ms backoff. A HID++ round trip that is going to answer answers in tens of milliseconds, so the shorter deadline costs nothing on a healthy link while converting a 5s stall into a fast retry.Hidpp20Error::Feature) andUnsupportedResponsereturn immediately — the device answered, so re-asking cannot change the answer. Only transport failures retry.channel.rs's test mock becomespub(crate)so device-level tests can drive a link that never answers.crates/openlogi-hidprobe_directcheckswalk_succeededbefore the peripheral discriminator, and settles an incomplete walk as a transient probe failure carryingseen(...)rather thanUnkeyed. The node keeps its cache entry and its last-good inventory is replayed while the link recovers, preserving the documented "replay last-good inventory through transient failures" behaviour instead of evicting on a glitch.PROBE_BUDGET6s → 25s, so the outertimeoutcannot cancel the per-entry retries before they help.Testing
cargo fmt --check cargo clippy -p openlogi-hidpp -p openlogi-hid --all-targets -- -D warnings cargo test -p openlogi-hidpp -p openlogi-hidAll clean. 237 tests pass, including a new
device::tests::lost_feature_entry_is_retried_before_giving_up, which asserts an unanswered entry reaches the wireFEATURE_READ_ATTEMPTStimes rather than once.Hardware verification — MX Master 3S, Bluetooth-LE direct, macOS 26.5.2 (25F84), Apple silicon, measured with
openlogi list:Successful probes also settle faster as the link warms: 14s, 5s, 2s, 1s, 1s. After the change the GUI lists the device, reads battery, and writes DPI and SmartShift normally.
Not verified. Bolt, Unifying, wired/USB, Linux and Windows were not tested — I only have a BLE-direct mouse on macOS.
One shape question worth your judgement:
PROBE_BUDGETis shared across all transports, so raising it does lengthen the worst case before a genuinely absent device settles as absent. A per-transport budget may be the better structure if that regression matters on the receiver paths; happy to rework it that way.Fixes #468