From 3212ca7854a5c9c93d805d1f9ef924e1448de4b9 Mon Sep 17 00:00:00 2001 From: KJH Date: Mon, 27 Jul 2026 14:51:32 +0800 Subject: [PATCH 1/2] fix(hidpp): retry lost feature-table reads during enumeration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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. --- crates/openlogi-hidpp/src/channel.rs | 12 ++-- crates/openlogi-hidpp/src/device.rs | 100 ++++++++++++++++++++++++++- 2 files changed, 104 insertions(+), 8 deletions(-) diff --git a/crates/openlogi-hidpp/src/channel.rs b/crates/openlogi-hidpp/src/channel.rs index 5175bb32..15be4ca9 100755 --- a/crates/openlogi-hidpp/src/channel.rs +++ b/crates/openlogi-hidpp/src/channel.rs @@ -688,7 +688,7 @@ fn short_payload_as_long(payload: &[u8; SHORT_REPORT_LENGTH - 1]) -> [u8; LONG_R } #[cfg(test)] -mod tests { +pub(crate) mod tests { use super::*; use std::{ io, @@ -1107,14 +1107,14 @@ mod tests { } #[derive(Clone)] - struct MockRawHidHandle { + pub(crate) struct MockRawHidHandle { incoming_tx: async_channel::Sender>, written_reports: Arc>>>, responses_on_write: Arc>>>, } impl MockRawHidHandle { - fn queue_response(&self, msg: HidppMessage) { + pub(crate) fn queue_response(&self, msg: HidppMessage) { self.responses_on_write .lock() .unwrap() @@ -1125,12 +1125,12 @@ mod tests { self.incoming_tx.send(raw_report(msg)).await.unwrap(); } - fn written_reports(&self) -> Vec> { + pub(crate) fn written_reports(&self) -> Vec> { self.written_reports.lock().unwrap().clone() } } - struct MockRawHidChannel { + pub(crate) struct MockRawHidChannel { incoming_tx: async_channel::Sender>, incoming_rx: async_channel::Receiver>, written_reports: Arc>>>, @@ -1138,7 +1138,7 @@ mod tests { } impl MockRawHidChannel { - fn new() -> (Self, MockRawHidHandle) { + pub(crate) fn new() -> (Self, MockRawHidHandle) { let (incoming_tx, incoming_rx) = async_channel::unbounded(); let written_reports = Arc::new(Mutex::new(Vec::new())); let responses_on_write = Arc::new(Mutex::new(VecDeque::new())); diff --git a/crates/openlogi-hidpp/src/device.rs b/crates/openlogi-hidpp/src/device.rs index 1de9fd6f..eec5fdcd 100755 --- a/crates/openlogi-hidpp/src/device.rs +++ b/crates/openlogi-hidpp/src/device.rs @@ -1,7 +1,8 @@ //! Implements peripheral devices connected to HID++ channels. -use std::{any::TypeId, collections::HashMap, sync::Arc}; +use std::{any::TypeId, collections::HashMap, sync::Arc, time::Duration}; +use futures::{FutureExt, select}; use thiserror::Error; use tracing::trace; @@ -145,7 +146,7 @@ impl Device { ); let mut features = Vec::with_capacity(count as usize); for i in 1..=count { - let info = feature_set_feature.get_feature(i).await?; + let info = read_feature_entry(&feature_set_feature, self.device_index, i).await?; trace!( index = self.device_index, slot = i, @@ -175,6 +176,101 @@ impl Device { } } +/// Per-attempt deadline for one feature-table read during enumeration. +/// +/// The channel's default [`crate::channel::SEND_RESPONSE_TIMEOUT`] (5s) is +/// longer than the budget most callers give the whole walk, so one dropped +/// report used to consume the caller's entire probe budget and abort +/// enumeration. A HID++ round trip that is going to answer answers in tens of +/// milliseconds; past this the report is lost, and re-asking beats waiting. +const FEATURE_READ_ATTEMPT: Duration = Duration::from_millis(700); + +/// Attempts per feature-table entry before enumeration gives up on it. +/// +/// Bluetooth-direct links drop individual reports while the table itself stays +/// stable, so a lost entry is worth re-asking for rather than discarding a walk +/// that may already be thirty entries deep. +const FEATURE_READ_ATTEMPTS: u8 = 4; + +/// Pause between attempts, letting the link drain before re-asking. +const FEATURE_READ_BACKOFF: Duration = Duration::from_millis(120); + +/// Reads one feature-table entry, re-asking under a short per-attempt deadline +/// when the link drops the report. +/// +/// A feature-level refusal ([`Hidpp20Error::Feature`]) or an unsupported +/// response returns immediately: the device answered, so re-asking cannot +/// change the answer. Only transport failures are retried. +async fn read_feature_entry( + feature_set: &FeatureSetFeature, + device_index: u8, + index: u8, +) -> Result { + let mut last_error = None; + for attempt in 1..=FEATURE_READ_ATTEMPTS { + let mut read = std::pin::pin!(feature_set.get_feature(index).fuse()); + let outcome = select! { + result = read => Some(result), + _ = futures_timer::Delay::new(FEATURE_READ_ATTEMPT).fuse() => None, + }; + match outcome { + Some(Ok(info)) => return Ok(info), + Some(Err(e @ (Hidpp20Error::Feature(_) | Hidpp20Error::UnsupportedResponse))) => { + return Err(e); + } + Some(Err(e)) => last_error = Some(e), + None => trace!( + index = device_index, + slot = index, + attempt, + "feature-table read timed out — re-asking" + ), + } + if attempt < FEATURE_READ_ATTEMPTS { + futures_timer::Delay::new(FEATURE_READ_BACKOFF).await; + } + } + Err(last_error.unwrap_or(Hidpp20Error::Channel(ChannelError::Timeout))) +} + +#[cfg(test)] +mod tests { + use std::sync::Arc; + + use crate::{ + channel::{HidppChannel, tests::MockRawHidChannel}, + feature::{CreatableFeature as _, feature_set::FeatureSetFeature}, + protocol::v20::Hidpp20Error, + }; + + use super::{FEATURE_READ_ATTEMPTS, read_feature_entry}; + + /// An entry whose report is lost is re-asked rather than abandoned. Aborting + /// on the first lost report is what made Bluetooth-direct enumeration give + /// up mid-table, which callers then misread as "not a peripheral". + #[test] + fn lost_feature_entry_is_retried_before_giving_up() { + futures::executor::block_on(async { + let (raw, handle) = MockRawHidChannel::new(); + let channel = Arc::new(HidppChannel::from_raw_channel(raw).await.unwrap()); + // The mock answers nothing, so every attempt runs to its deadline. + let feature_set = FeatureSetFeature::new(Arc::clone(&channel), 0xff, 0x01); + + let err = read_feature_entry(&feature_set, 0xff, 1).await.unwrap_err(); + + assert!( + matches!(err, Hidpp20Error::Channel(_)), + "an unanswered entry surfaces as a transport failure, got {err:?}" + ); + assert_eq!( + handle.written_reports().len(), + usize::from(FEATURE_READ_ATTEMPTS), + "every attempt should reach the wire" + ); + }); + } +} + /// Represents a device-specific error. #[derive(Debug, Error)] #[non_exhaustive] From 3fd6230684ab335b576386efaaacb9f3b459659e Mon Sep 17 00:00:00 2001 From: KJH Date: Mon, 27 Jul 2026 14:51:32 +0800 Subject: [PATCH 2/2] fix(hid): keep a direct node's identity when its feature walk fails MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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. --- crates/openlogi-hid/src/inventory.rs | 19 ++++++++++++------- crates/openlogi-hid/src/inventory/probe.rs | 18 ++++++++++++++++++ 2 files changed, 30 insertions(+), 7 deletions(-) diff --git a/crates/openlogi-hid/src/inventory.rs b/crates/openlogi-hid/src/inventory.rs index a95294e4..4f3c4fd4 100644 --- a/crates/openlogi-hid/src/inventory.rs +++ b/crates/openlogi-hid/src/inventory.rs @@ -38,13 +38,18 @@ const MAX_BOLT_SLOTS: u8 = 6; /// device wedges the whole enumeration — and the GUI runs `enumerate` on a /// polling watcher, so a permanent hang would stall every later refresh. /// -/// Kept short so a snapshot settles quickly: a timed-out node is skipped and -/// re-probed on the next watcher tick (~2 s), and the first probe usually wakes -/// the device so the retry succeeds fast. Slots are probed concurrently on both -/// receiver paths, so a healthy receiver's worst case is the 1.5 s arrival drain -/// plus a single slot's [`BOLT_SLOT_PROBE`] / [`UNIFYING_SLOT_PROBE`] — not their -/// sum — which this stays comfortably above, so awake devices never trip it. -const PROBE_BUDGET: Duration = Duration::from_secs(6); +/// A timed-out node is skipped and re-probed on the next watcher tick (~2 s), +/// and the first probe usually wakes the device so the retry succeeds fast. +/// Slots are probed concurrently on both receiver paths, so a healthy +/// receiver's worst case is the 1.5 s arrival drain plus a single slot's +/// [`BOLT_SLOT_PROBE`] / [`UNIFYING_SLOT_PROBE`] — not their sum — which this +/// stays comfortably above, so awake devices never trip it. +/// +/// Sized for the Bluetooth-direct feature walk, the long pole: a ~35-entry +/// 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); /// Per-slot budget for the HID++ 2.0 feature walk on a Unifying paired device. /// diff --git a/crates/openlogi-hid/src/inventory/probe.rs b/crates/openlogi-hid/src/inventory/probe.rs index 8e2b717d..8e73b714 100644 --- a/crates/openlogi-hid/src/inventory/probe.rs +++ b/crates/openlogi-hid/src/inventory/probe.rs @@ -437,6 +437,24 @@ async fn probe_direct( let walk_succeeded = capabilities.is_some(); let caps = capabilities.unwrap_or_default(); let is_peripheral = probe.battery.is_some() || caps.buttons || caps.pointer || caps.lighting; + // A walk that never completed says nothing about what this node is: the + // discriminator below would read "no battery, no config feature" off an + // empty probe and reject a real mouse as a receiver's secondary interface. + // Settle it as a transient failure and keep the node's cache entry, so the + // last-good inventory is replayed while the link recovers. + if !walk_succeeded { + debug!( + vid = format_args!("{:04x}", info.vendor_id), + pid = format_args!("{:04x}", info.product_id), + "feature walk did not complete — transient probe failure, keeping last-known identity" + ); + return NodeProbe { + inventory: None, + healthy: false, + complete: false, + outcomes: vec![seen(Some(CacheKey::Direct(info.id.clone())))], + }; + } if !is_peripheral { debug!( vid = format_args!("{:04x}", info.vendor_id),