fix(unifying): enable wireless notifications so paired devices enumerate#309
fix(unifying): enable wireless notifications so paired devices enumerate#309laofun wants to merge 1 commit into
Conversation
58d6a8e to
f68b17d
Compare
Greptile SummaryThis PR fixes Unifying receiver enumeration by enabling wireless notifications (register
Confidence Score: 4/5Safe to merge for the core enumeration fix; one logic gap in error handling could transiently clear the device list on a first-cycle HID read failure. The wireless-notification enable, name-register fix, and online-flag correction are all well-motivated and wire-verified. The one concern is drain_device_arrival_unifying: when set_wireless_notifications fails but trigger_device_arrival succeeds, the function returns Some([]) rather than None, telling the ledger 'no devices found' instead of 'probe unreliable — use last snapshot.' On a fresh attach combined with a transient HID read error, this would show all paired devices as offline for that cycle. crates/openlogi-hid/src/inventory.rs — specifically the error handling in drain_device_arrival_unifying Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Inv as inventory.rs
participant Uni as UnifyingReceiver
participant HW as Receiver HW
Inv->>Uni: drain_device_arrival_unifying()
Uni->>HW: read_register(0x00) [Notifications]
HW-->>Uni: current flags
Uni->>HW: "write_register(0x00, flags | WIRELESS)"
HW-->>Uni: ACK
Note over Inv,HW: WIRELESS bit now set — 0x41 events will flow
Uni->>HW: trigger_device_arrival (write 0x02)
HW-->>Uni: ACK
HW-)Uni: 0x41 DeviceConnection (slot 1)
HW-)Uni: 0x41 DeviceConnection (slot 1, duplicate)
Uni-->>Inv: Some([conn_slot1, conn_slot1])
Note over Inv: Dedup by slot index (HashMap), then sort
Inv->>Uni: read_long_register(0xFF, 0xB5, [0x40+slot-1, …])
HW-->>Uni: [sub, len, name bytes, padding]
Note over Inv: parse_codename_unifying clamps len
Inv->>HW: "probe_or_reuse(slot, online=true)"
HW-->>Inv: capabilities (Some → online, None → offline)
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Inv as inventory.rs
participant Uni as UnifyingReceiver
participant HW as Receiver HW
Inv->>Uni: drain_device_arrival_unifying()
Uni->>HW: read_register(0x00) [Notifications]
HW-->>Uni: current flags
Uni->>HW: "write_register(0x00, flags | WIRELESS)"
HW-->>Uni: ACK
Note over Inv,HW: WIRELESS bit now set — 0x41 events will flow
Uni->>HW: trigger_device_arrival (write 0x02)
HW-->>Uni: ACK
HW-)Uni: 0x41 DeviceConnection (slot 1)
HW-)Uni: 0x41 DeviceConnection (slot 1, duplicate)
Uni-->>Inv: Some([conn_slot1, conn_slot1])
Note over Inv: Dedup by slot index (HashMap), then sort
Inv->>Uni: read_long_register(0xFF, 0xB5, [0x40+slot-1, …])
HW-->>Uni: [sub, len, name bytes, padding]
Note over Inv: parse_codename_unifying clamps len
Inv->>HW: "probe_or_reuse(slot, online=true)"
HW-->>Inv: capabilities (Some → online, None → offline)
Reviews (4): Last reviewed commit: "fix(unifying): enable wireless notificat..." | Re-trigger Greptile |
f68b17d to
9aff9b7
Compare
|
Addressed both review notes: the deduped connection list is now sorted by slot so device order is stable across probe cycles, and added a note on |
AprilNEA
left a comment
There was a problem hiding this comment.
This looks like the right direction overall — enabling the Unifying notification stream before trigger_device_arrival, correcting the 0x40 name base, and deduping repeated arrivals are all well-scoped, and CI is green.
One edge I’d tighten: set_wireless_notifications(true) currently writes the whole notifications register as [0, 1, 0]. That sets the wireless bit, but it also clears any other notification flags already enabled on register 0x00. In this repo, the pairing flow writes [0x00, 0x09, 0x00] (WIRELESS | SOFTWARE_PRESENT) before streaming pairing events, so an inventory poll during/near pairing could accidentally drop the software-present bit or any future notification flag.
Could we make this read-modify-write, or at least use the same named notification flag constants as pairing.rs so the intended bitmask is explicit? The rest of the PR looks good to me.
The receiver only re-broadcasts 0x41 device-arrival events while wireless notifications (register 0x00) are on; without it an actively-connected device showed "no paired devices". Add set_wireless_notifications() and enable it before draining arrivals (Solaar does the same before listing). Derive online from the HID++ 2.0 feature walk succeeding — the crate's event.online reads the wrong notification byte (always "offline" here) and trigger_device_arrival re-broadcasts 0x41 for offline slots too. Read device names at the Unifying 0x40 base (not Bolt's 0x60), clamp the device-reported name length so a bogus byte can't over-read the register, and dedup repeated arrivals per slot.
9aff9b7 to
f2b7ef5
Compare
|
Good catch — switched |
Summary
A device on a Unifying receiver wouldn't show up even while connected. The
receiver only re-broadcasts arrival events while wireless notifications
(register 0x00) are on, and we never enabled it — so the trigger got ACK'd
but emitted nothing. Solaar does this before listing.
Changes
"offline" here).
clamp the reported length.
Testing
Tested on an MX Master 2S over a Unifying receiver. Added unit tests for the
name parser (normal, over-long length, short response).