From d537bca6f180cd5049a9ef76ad7cc932418b24c7 Mon Sep 17 00:00:00 2001 From: Toby Yan Date: Wed, 1 Jul 2026 15:02:58 +0800 Subject: [PATCH] fix(openlogi): stabilize bluetooth direct devices Filter non-identifying direct HID++ model info before it becomes a persistent device identity, and keep Bluetooth-direct offline placeholders on the HID++ self slot instead of slot 0. Treat no-evidence direct refreshes for cached devices as transient misses so a slow or sleeping BLE device replays last-good inventory instead of being cleared. Required macOS privacy grants for packaged installs: - Input Monitoring: OpenLogi GUI app bundle at /Applications/OpenLogi.app (binary: /Applications/OpenLogi.app/Contents/MacOS/openlogi-gui). - Input Monitoring: OpenLogi Agent helper bundle at /Applications/OpenLogi.app/Contents/Library/LoginItems/OpenLogiAgent.app (binary: /Applications/OpenLogi.app/Contents/Library/LoginItems/OpenLogiAgent.app/Contents/MacOS/openlogi-agent). - Accessibility: OpenLogi Agent helper bundle at /Applications/OpenLogi.app/Contents/Library/LoginItems/OpenLogiAgent.app (binary: /Applications/OpenLogi.app/Contents/Library/LoginItems/OpenLogiAgent.app/Contents/MacOS/openlogi-agent), because the agent owns the CGEventTap mouse hook. Remove the temporary async-hid path patch/vendor tree and raw HID transport trace from the final change; the fix now stays inside OpenLogi's probe and GUI state logic. --- CHANGELOG.md | 5 + crates/openlogi-core/src/device.rs | 33 ++ crates/openlogi-gui/src/state.rs | 21 + crates/openlogi-gui/src/state/devices.rs | 148 +++++- crates/openlogi-hid/src/inventory/features.rs | 490 +++++++++++++++--- crates/openlogi-hid/src/inventory/probe.rs | 147 +++++- 6 files changed, 769 insertions(+), 75 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8e696e11..3b8455c7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- *(hid)* keep macOS Bluetooth-direct Logitech devices discoverable when HID++ optional reads are slow. +- *(gui)* avoid stale Bluetooth-direct `unit:*` device cards and render direct offline placeholders as slot 255. + ## [0.6.18](https://github.com/AprilNEA/OpenLogi/compare/openlogi-core-v0.6.17...openlogi-core-v0.6.18) - 2026-06-29 ### Added diff --git a/crates/openlogi-core/src/device.rs b/crates/openlogi-core/src/device.rs index 583257d1..c97131a6 100644 --- a/crates/openlogi-core/src/device.rs +++ b/crates/openlogi-core/src/device.rs @@ -203,6 +203,18 @@ impl DeviceModelInfo { pub fn config_key(&self) -> String { format!("{:x}{:04x}", self.extended_model_id, self.model_ids[0]) } + + /// Whether this `DeviceInformation` payload names a real Logitech model. + /// + /// Some direct macOS HID interfaces answer the `0x0003` function with a + /// structurally valid but semantically empty payload: unit id set, every + /// transport bit clear, and all model IDs zero. Unit id alone can + /// disambiguate two devices, but it cannot identify the model, resolve an + /// asset, or produce a trustworthy direct config key. + #[must_use] + pub fn has_model_identity(&self) -> bool { + self.model_ids.iter().any(|id| *id != 0) + } } /// Mirror of hidpp's `DeviceTransport` bitfield — one bool per protocol the @@ -325,4 +337,25 @@ mod tests { Capabilities::default() ); } + + #[test] + fn model_info_identity_requires_a_real_model_id() { + use super::{DeviceModelInfo, DeviceTransports}; + + let mut info = DeviceModelInfo { + entity_count: 15, + serial_number: None, + unit_id: [0x1c, 0x18, 0x18, 0x00], + transports: DeviceTransports::default(), + model_ids: [0, 0, 0], + extended_model_id: 0, + }; + assert!( + !info.has_model_identity(), + "a HID interface that reports no model id must not create a direct config key" + ); + + info.model_ids[0] = 0xb034; + assert!(info.has_model_identity()); + } } diff --git a/crates/openlogi-gui/src/state.rs b/crates/openlogi-gui/src/state.rs index f35705e5..d0f4f552 100644 --- a/crates/openlogi-gui/src/state.rs +++ b/crates/openlogi-gui/src/state.rs @@ -1336,6 +1336,14 @@ fn persist_identities(config: &mut Config, list: &[DeviceRecord]) { if !record.online { continue; } + if !has_persistable_device_identity(record) { + debug!( + key = %record.config_key, + display = %record.display_name, + "online device lacks a persistable identity — not recording offline placeholder" + ); + continue; + } let Some(capabilities) = record.capabilities else { continue; }; @@ -1360,6 +1368,19 @@ fn persist_identities(config: &mut Config, list: &[DeviceRecord]) { } } +fn has_persistable_device_identity(record: &DeviceRecord) -> bool { + match record.route { + Some(DeviceRoute::Direct { .. }) => { + record + .model_info + .as_ref() + .is_some_and(openlogi_core::device::DeviceModelInfo::has_model_identity) + && (record.serial_number.is_some() || record.unit_id != [0; 4]) + } + Some(DeviceRoute::Bolt { .. } | DeviceRoute::Unifying { .. }) | None => true, + } +} + /// Whether a DPI discovery error is permanent (the device genuinely lacks the /// feature or reports nothing usable) versus transient (a timeout or busy /// device worth retrying). diff --git a/crates/openlogi-gui/src/state/devices.rs b/crates/openlogi-gui/src/state/devices.rs index bad673f9..c61e1ae0 100644 --- a/crates/openlogi-gui/src/state/devices.rs +++ b/crates/openlogi-gui/src/state/devices.rs @@ -150,6 +150,14 @@ fn append_offline_known<'a>( .model_info .as_ref() .map_or_else(|| key.to_string(), DeviceModelInfo::config_key); + if stale_direct_identity_shadowed_by_live(list, key, identity) { + debug!( + key, + display = %identity.display_name, + "suppressing stale direct unit identity shadowed by a live direct device" + ); + continue; + } if blocked_keys.contains(key) || blocked_keys.contains(&model_key) { continue; } @@ -160,6 +168,56 @@ fn append_offline_known<'a>( } } +fn stale_direct_identity_shadowed_by_live( + list: &[DeviceRecord], + key: &str, + identity: &DeviceIdentity, +) -> bool { + let Some((vendor_id, product_id, key_identity)) = direct_key_parts(key) else { + return false; + }; + if key_identity != "unit" { + return false; + } + if identity + .model_info + .as_ref() + .is_some_and(DeviceModelInfo::has_model_identity) + { + return false; + } + list.iter().any(|record| { + record.online + && record.display_name == identity.display_name + && record.kind == identity.kind + && matches!( + record.route, + Some(DeviceRoute::Direct { + vendor_id: live_vid, + product_id: live_pid, + }) if live_vid == vendor_id && live_pid == product_id + ) + }) +} + +fn direct_key_parts(key: &str) -> Option<(u16, u16, &str)> { + let mut parts = key.split(':'); + match ( + parts.next(), + parts.next(), + parts.next(), + parts.next(), + parts.next(), + ) { + (Some("direct"), Some(vid), Some(pid), Some(identity), Some(_)) => Some(( + u16::from_str_radix(vid, 16).ok()?, + u16::from_str_radix(pid, 16).ok()?, + identity, + )), + _ => None, + } +} + /// Synthesize an offline placeholder from a persisted [`DeviceIdentity`]. /// /// `route: None` keeps every hardware write a no-op until the live inventory @@ -195,12 +253,20 @@ fn offline_record( route: None, kind: identity.kind, capabilities: Some(identity.capabilities), - slot: 0, + slot: offline_slot_placeholder(config_key), online: false, battery: None, } } +fn offline_slot_placeholder(config_key: &str) -> u8 { + if config_key.starts_with("direct:") && direct_key_parts(config_key).is_some() { + 0xff + } else { + 0 + } +} + fn model_info_from_legacy_model_key(key: &str) -> Option { if key.len() <= 4 || !key.chars().all(|c| c.is_ascii_hexdigit()) { return None; @@ -334,7 +400,10 @@ fn prettify_codename(raw: &str) -> String { #[cfg(test)] mod tests { use openlogi_core::config::Config; - use openlogi_core::device::{DeviceInventory, PairedDevice, ReceiverInfo}; + use openlogi_core::device::{ + DeviceInventory, DeviceModelInfo, DeviceTransports, PairedDevice, ReceiverInfo, + }; + use openlogi_hid::DeviceRoute; use crate::asset::AssetResolver; @@ -387,6 +456,43 @@ mod tests { } } + fn online_direct_record( + key: &str, + display_name: &str, + vendor_id: u16, + product_id: u16, + ) -> DeviceRecord { + DeviceRecord { + config_key: key.to_string(), + model_key: "1b034".to_string(), + display_name: display_name.to_string(), + asset: None, + model_info: Some(DeviceModelInfo { + entity_count: 3, + serial_number: Some("2331LZ520ZQ8".to_string()), + unit_id: [0; 4], + transports: DeviceTransports { + btle: true, + ..DeviceTransports::default() + }, + model_ids: [0xb034, 0, 0], + extended_model_id: 1, + }), + codename: Some(display_name.to_string()), + serial_number: Some("2331LZ520ZQ8".to_string()), + unit_id: [0; 4], + route: Some(DeviceRoute::Direct { + vendor_id, + product_id, + }), + kind: DeviceKind::Mouse, + capabilities: Some(Capabilities::presumed_from_kind(DeviceKind::Mouse)), + slot: 0xff, + online: true, + battery: None, + } + } + fn mouse_identity(name: &str) -> DeviceIdentity { DeviceIdentity { display_name: name.to_string(), @@ -447,6 +553,15 @@ mod tests { assert_eq!(rec.capabilities, Some(id.capabilities)); } + #[test] + fn direct_offline_record_uses_direct_slot_placeholder() { + let id = mouse_identity("MX Master 3S"); + let cache = AssetResolver::new(); + let rec = offline_record("direct:046d:b034:serial:2331lz520zq8", &id, &cache); + + assert_eq!(rec.slot, 0xff); + } + #[test] fn known_devices_are_appended_only_when_absent_from_live() { // "A" is live; "B" is known-but-asleep. The union keeps the live "A" @@ -469,6 +584,35 @@ mod tests { ); } + #[test] + fn stale_direct_unit_identity_is_suppressed_when_live_direct_serial_exists() { + let mut list = vec![online_direct_record( + "direct:046d:b034:serial:2331lz520zq8", + "MX Master 3S", + 0x046d, + 0xb034, + )]; + let mut stale = mouse_identity("MX Master 3S"); + stale.model_info = Some(DeviceModelInfo { + entity_count: 15, + serial_number: None, + unit_id: [0; 4], + transports: DeviceTransports::default(), + model_ids: [0, 0, 0], + extended_model_id: 0, + }); + let cache = AssetResolver::new(); + + append_offline_known( + &mut list, + [("direct:046d:b034:unit:1c181800", &stale)].into_iter(), + &cache, + ); + + assert_eq!(list.len(), 1); + assert_eq!(list[0].config_key, "direct:046d:b034:serial:2331lz520zq8"); + } + #[test] fn asset_kind_overrides_a_misreporting_hid_kind() { // #127: the registry knows this depot is a mouse, so a HID++ source that diff --git a/crates/openlogi-hid/src/inventory/features.rs b/crates/openlogi-hid/src/inventory/features.rs index 199fb37f..31a6c6e7 100644 --- a/crates/openlogi-hid/src/inventory/features.rs +++ b/crates/openlogi-hid/src/inventory/features.rs @@ -1,22 +1,46 @@ -use std::sync::Arc; +use std::{fmt::Write as _, future::Future, sync::Arc, time::Duration}; use hidpp::{ channel::HidppChannel, device::Device, - feature::hires_wheel::HiResWheelFeature, feature::{ CreatableFeature, device_information::DeviceInformationFeature, device_type_and_name::DeviceTypeAndNameFeature, unified_battery::UnifiedBatteryFeature, }, + feature::{hires_wheel::HiResWheelFeature, root::FeatureInformation}, }; use openlogi_core::device::{ BatteryInfo, Capabilities, DeviceKind, DeviceModelInfo, DeviceTransports, }; +use tokio::time::timeout; use tracing::debug; use crate::mappings::{ map_battery_level, map_battery_status, map_device_type, normalize_serial_number, }; +use crate::route::DIRECT_DEVICE_INDEX; + +/// Optional per-feature reads must not consume the whole node probe budget. +/// +/// BLE-direct devices can occasionally drop or delay one HID++ response while +/// still being otherwise usable. The feature table is the gate for "this is a +/// configurable device"; battery, wheel-inversion, serial and marketing type are +/// useful adornments and should degrade independently. +const OPTIONAL_FEATURE_TIMEOUT: Duration = Duration::from_millis(1500); +const DIRECT_CAPABILITY_TIMEOUT: Duration = Duration::from_millis(300); + +const DIRECT_CAPABILITY_FEATURE_IDS: &[u16] = &[ + 0x1b00, 0x1b01, 0x1b02, 0x1b03, 0x1b04, // reprogrammable controls + 0x2201, 0x2202, // adjustable DPI / pointer resolution + 0x8070, 0x8080, // keyboard lighting families OpenLogi can drive +]; +const DIRECT_POINTER_CAPABILITY_FEATURE_IDS: &[u16] = &[ + 0x1b00, 0x1b01, 0x1b02, 0x1b03, 0x1b04, // reprogrammable controls + 0x2201, 0x2202, // adjustable DPI / pointer resolution +]; +const DIRECT_KEYBOARD_CAPABILITY_FEATURE_IDS: &[u16] = &[ + 0x8070, 0x8080, // keyboard lighting families OpenLogi can drive +]; /// Everything a single device probe yields. Any field is `None` when the /// device doesn't expose that feature or the read failed. @@ -28,6 +52,21 @@ pub(super) struct ProbedFeatures { pub(super) kind: Option, /// Configuration capabilities derived from the device's feature table. pub(super) capabilities: Option, + /// Direct-device discriminator evidence from measured facts only. Kind-based + /// fallback capabilities keep an accepted mouse configurable, but they are + /// not enough to prove that a macOS HID interface is the primary peripheral. + pub(super) direct_peripheral_evidence: bool, +} + +fn format_feature_ids(ids: &[u16]) -> String { + let mut out = String::with_capacity(ids.len().saturating_mul(7).saturating_sub(2)); + for (index, id) in ids.iter().enumerate() { + if index > 0 { + out.push_str(", "); + } + let _ = write!(&mut out, "{id:#06x}"); + } + out } /// Read just the battery by addressing the `UnifiedBattery` feature at its @@ -41,15 +80,17 @@ pub(super) async fn read_battery( feature_index: u8, ) -> Option { let feature = UnifiedBatteryFeature::new(Arc::clone(channel), slot, feature_index); - feature - .get_battery_info() - .await - .ok() - .map(|info| BatteryInfo { - percentage: info.charging_percentage, - level: map_battery_level(info.level), - status: map_battery_status(info.status), - }) + optional_read( + slot, + "UnifiedBattery getBatteryInfo", + feature.get_battery_info(), + ) + .await + .map(|info| BatteryInfo { + percentage: info.charging_percentage, + level: map_battery_level(info.level), + status: map_battery_status(info.status), + }) } /// Runtime index of the `UnifiedBattery` feature in an enumerated feature-ID @@ -85,6 +126,10 @@ pub(super) async fn probe_features( return (ProbedFeatures::default(), None); } }; + debug!(slot, "Device::new succeeded"); + if slot == DIRECT_DEVICE_INDEX { + return probe_direct_features(channel, &device, slot).await; + } // The enumeration response IS the device's feature-ID table — capture it // for capability derivation instead of discarding it. let mut battery_index = None; @@ -92,21 +137,41 @@ pub(super) async fn probe_features( Ok(Some(features)) => { let ids: Vec = features.iter().map(|f| f.id).collect(); battery_index = battery_feature_index(ids.iter().copied()); - Some(Capabilities::from_feature_ids(&ids)) + let caps = Capabilities::from_feature_ids(&ids); + debug!( + slot, + feature_count = ids.len(), + feature_ids = %format_feature_ids(&ids), + battery_index, + capabilities = ?caps, + "feature table enumerated" + ); + Some(caps) + } + Ok(None) => { + debug!(slot, "feature enumeration returned no table"); + None } - Ok(None) => None, Err(e) => { debug!(slot, error = ?e, "enumerate_features failed"); return (ProbedFeatures::default(), None); } }; + // Identity is essential for the GUI/agent to treat this as the live device. + // Read it before volatile adornments such as battery; a flaky battery query + // must not leave a BLE-direct mouse looking offline. + let model_info = read_model_info(&device, slot).await; + if let Some(caps) = capabilities.as_mut() && let Some(feature) = device.get_feature::() { - caps.scroll_inversion = feature - .get_wheel_capabilities() - .await - .is_ok_and(|wheel| wheel.has_invert); + caps.scroll_inversion = optional_read( + slot, + "HiResWheel getWheelCapabilities", + feature.get_wheel_capabilities(), + ) + .await + .is_some_and(|wheel| wheel.has_invert); } let battery = match battery_index { @@ -114,54 +179,18 @@ pub(super) async fn probe_features( None => None, }; - let model_info = match device.get_feature::() { - Some(feature) => match feature.get_device_info().await { - Ok(info) => { - let serial_number = if info.capabilities.serial_number { - match feature.get_serial_number().await { - Ok(serial) => normalize_serial_number(&serial), - Err(e) => { - debug!(slot, error = ?e, "DeviceInformation serial read failed"); - None - } - } - } else { - None - }; - Some(DeviceModelInfo { - entity_count: info.entity_count, - serial_number, - unit_id: info.unit_id, - transports: DeviceTransports { - usb: info.transport.usb, - equad: info.transport.e_quad, - btle: info.transport.btle, - bluetooth: info.transport.bluetooth, - }, - model_ids: info.model_id, - extended_model_id: info.extended_model_id, - }) - } - Err(e) => { - debug!(slot, error = ?e, "DeviceInformation read failed"); - None - } - }, - None => None, - }; - // `0x0005` reports the device's own marketing type (mouse, keyboard, …) — // the authoritative kind signal. On the direct path it's the only one; on // the Bolt path it corrects a pairing register that reported the wrong (or // `Unknown`) kind. let kind = match device.get_feature::() { - Some(feature) => match feature.get_device_type().await { - Ok(ty) => Some(map_device_type(ty)), - Err(e) => { - debug!(slot, error = ?e, "DeviceType read failed"); - None - } - }, + Some(feature) => optional_read( + slot, + "DeviceTypeAndName getDeviceType", + feature.get_device_type(), + ) + .await + .map(map_device_type), None => None, }; @@ -171,16 +200,346 @@ pub(super) async fn probe_features( model_info, kind, capabilities, + direct_peripheral_evidence: false, }, battery_index, ) } +async fn read_model_info(device: &Device, slot: u8) -> Option { + let feature = device.get_feature::()?; + read_model_info_from_feature(&feature, slot).await +} + +async fn read_model_info_from_feature( + feature: &DeviceInformationFeature, + slot: u8, +) -> Option { + let info = optional_read( + slot, + "DeviceInformation getDeviceInfo", + feature.get_device_info(), + ) + .await?; + let serial_number = if info.capabilities.serial_number { + optional_read( + slot, + "DeviceInformation getSerialNumber", + feature.get_serial_number(), + ) + .await + .and_then(|serial| normalize_serial_number(&serial)) + } else { + None + }; + let model = DeviceModelInfo { + entity_count: info.entity_count, + serial_number, + unit_id: info.unit_id, + transports: DeviceTransports { + usb: info.transport.usb, + equad: info.transport.e_quad, + btle: info.transport.btle, + bluetooth: info.transport.bluetooth, + }, + model_ids: info.model_id, + extended_model_id: info.extended_model_id, + }; + if !model.has_model_identity() { + debug!( + slot, + entity_count = model.entity_count, + unit_id = format_args!( + "{:02x}{:02x}{:02x}{:02x}", + model.unit_id[0], + model.unit_id[1], + model.unit_id[2], + model.unit_id[3] + ), + model_ids = ?model.model_ids, + extended_model_id = model.extended_model_id, + "DeviceInformation payload has no model id — ignoring as non-identifying" + ); + return None; + } + Some(model) +} + +async fn probe_direct_features( + channel: &Arc, + device: &Device, + slot: u8, +) -> (ProbedFeatures, Option) { + let mut ids = Vec::new(); + + let model_info = match direct_feature_info( + device, + slot, + DeviceInformationFeature::ID, + "Root getFeature DeviceInformation", + ) + .await + { + Some(info) => { + ids.push(DeviceInformationFeature::ID); + let feature = DeviceInformationFeature::new(Arc::clone(channel), slot, info.index); + read_model_info_from_feature(&feature, slot).await + } + None => None, + }; + + let mut battery = None; + let mut battery_index = None; + if let Some(info) = direct_feature_info( + device, + slot, + UnifiedBatteryFeature::ID, + "Root getFeature UnifiedBattery", + ) + .await + { + ids.push(UnifiedBatteryFeature::ID); + battery_index = Some(info.index); + battery = read_battery(channel, slot, info.index).await; + } + + let mut kind = None; + if let Some(info) = direct_feature_info( + device, + slot, + DeviceTypeAndNameFeature::ID, + "Root getFeature DeviceTypeAndName", + ) + .await + { + ids.push(DeviceTypeAndNameFeature::ID); + let feature = DeviceTypeAndNameFeature::new(Arc::clone(channel), slot, info.index); + kind = optional_read( + slot, + "DeviceTypeAndName getDeviceType", + feature.get_device_type(), + ) + .await + .map(map_device_type); + } + + for &id in direct_capability_probe_ids(kind) { + if direct_feature_info_with_timeout( + device, + slot, + id, + "Root getFeature capability", + DIRECT_CAPABILITY_TIMEOUT, + ) + .await + .is_some() + { + ids.push(id); + } + } + + let mut capabilities = direct_capabilities_from_ids(&ids, kind); + if let Some(info) = direct_feature_info_with_timeout( + device, + slot, + HiResWheelFeature::ID, + "Root getFeature HiResWheel", + DIRECT_CAPABILITY_TIMEOUT, + ) + .await + { + ids.push(HiResWheelFeature::ID); + let feature = HiResWheelFeature::new(Arc::clone(channel), slot, info.index); + capabilities.scroll_inversion = optional_read_with_timeout( + slot, + "HiResWheel getWheelCapabilities", + feature.get_wheel_capabilities(), + DIRECT_CAPABILITY_TIMEOUT, + ) + .await + .is_some_and(|wheel| wheel.has_invert); + } + let measured_capabilities = Capabilities::from_feature_ids(&ids); + let direct_peripheral_evidence = battery.is_some() + || model_info + .as_ref() + .is_some_and(DeviceModelInfo::has_model_identity) + || measured_capabilities.buttons + || measured_capabilities.pointer + || measured_capabilities.lighting; + + debug!( + slot, + feature_count = ids.len(), + feature_ids = %format_feature_ids(&ids), + battery_index, + capabilities = ?capabilities, + direct_peripheral_evidence, + "direct root probe completed" + ); + + ( + ProbedFeatures { + battery, + model_info, + kind, + capabilities: Some(capabilities), + direct_peripheral_evidence, + }, + battery_index, + ) +} + +async fn direct_feature_info( + device: &Device, + slot: u8, + id: u16, + what: &'static str, +) -> Option { + direct_feature_info_with_timeout(device, slot, id, what, OPTIONAL_FEATURE_TIMEOUT).await +} + +async fn direct_feature_info_with_timeout( + device: &Device, + slot: u8, + id: u16, + what: &'static str, + budget: Duration, +) -> Option { + let root = device.root(); + optional_read_with_timeout(slot, what, root.get_feature(id), budget) + .await + .flatten() +} + +fn direct_capability_probe_ids(kind: Option) -> &'static [u16] { + match kind { + Some(DeviceKind::Mouse | DeviceKind::Trackball | DeviceKind::Touchpad) => { + DIRECT_POINTER_CAPABILITY_FEATURE_IDS + } + Some(DeviceKind::Keyboard | DeviceKind::Numpad) => DIRECT_KEYBOARD_CAPABILITY_FEATURE_IDS, + _ => DIRECT_CAPABILITY_FEATURE_IDS, + } +} + +fn direct_capabilities_from_ids(ids: &[u16], kind: Option) -> Capabilities { + let mut capabilities = Capabilities::from_feature_ids(ids); + if let Some(kind) = kind { + let presumed = Capabilities::presumed_from_kind(kind); + capabilities.buttons |= presumed.buttons; + capabilities.pointer |= presumed.pointer; + capabilities.lighting |= presumed.lighting; + } + capabilities +} + +async fn optional_read( + slot: u8, + what: &'static str, + read: impl Future>, +) -> Option +where + E: std::fmt::Debug, +{ + optional_read_with_timeout(slot, what, read, OPTIONAL_FEATURE_TIMEOUT).await +} + +async fn optional_read_with_timeout( + slot: u8, + what: &'static str, + read: impl Future>, + budget: Duration, +) -> Option +where + E: std::fmt::Debug, +{ + match timeout(budget, read).await { + Ok(Ok(value)) => Some(value), + Ok(Err(e)) => { + debug!(slot, feature = what, error = ?e, "optional HID++ feature read failed"); + None + } + Err(_) => { + debug!( + slot, + feature = what, + budget = ?budget, + "optional HID++ feature read timed out" + ); + None + } + } +} + #[cfg(test)] mod tests { + use std::time::Duration; + use hidpp::feature::{CreatableFeature as _, unified_battery::UnifiedBatteryFeature}; + use openlogi_core::device::DeviceKind; + + use super::{ + DIRECT_CAPABILITY_FEATURE_IDS, DIRECT_CAPABILITY_TIMEOUT, + DIRECT_KEYBOARD_CAPABILITY_FEATURE_IDS, DIRECT_POINTER_CAPABILITY_FEATURE_IDS, + OPTIONAL_FEATURE_TIMEOUT, battery_feature_index, direct_capabilities_from_ids, + direct_capability_probe_ids, format_feature_ids, + }; + + #[test] + fn optional_feature_timeout_is_bounded_inside_probe_budget() { + assert!( + OPTIONAL_FEATURE_TIMEOUT >= Duration::from_secs(1), + "BLE direct optional reads need enough room for a slow macOS report callback" + ); + assert!( + OPTIONAL_FEATURE_TIMEOUT < super::super::PROBE_BUDGET, + "optional reads must not consume the whole node probe budget" + ); + assert!( + DIRECT_CAPABILITY_TIMEOUT * (DIRECT_CAPABILITY_FEATURE_IDS.len() as u32) + < super::super::PROBE_BUDGET, + "direct capability probes are best-effort and must not exhaust the whole probe budget" + ); + } - use super::battery_feature_index; + #[test] + fn direct_probe_capability_ids_cover_ui_panels() { + let caps = + openlogi_core::device::Capabilities::from_feature_ids(DIRECT_CAPABILITY_FEATURE_IDS); + assert!(caps.buttons, "direct root probe must check button features"); + assert!( + caps.pointer, + "direct root probe must check pointer features" + ); + assert!( + caps.lighting, + "direct root probe must check lighting features" + ); + } + + #[test] + fn direct_probe_limits_capability_queries_by_device_kind() { + assert_eq!( + direct_capability_probe_ids(Some(DeviceKind::Mouse)), + DIRECT_POINTER_CAPABILITY_FEATURE_IDS + ); + assert_eq!( + direct_capability_probe_ids(Some(DeviceKind::Keyboard)), + DIRECT_KEYBOARD_CAPABILITY_FEATURE_IDS + ); + assert_eq!( + direct_capability_probe_ids(None), + DIRECT_CAPABILITY_FEATURE_IDS + ); + } + + #[test] + fn direct_probe_presumes_mouse_panels_when_capability_reads_time_out() { + let caps = direct_capabilities_from_ids(&[], Some(DeviceKind::Mouse)); + assert!(caps.buttons); + assert!(caps.pointer); + assert!(!caps.lighting); + } #[test] fn battery_index_is_one_based_in_the_enumerated_table() { @@ -200,4 +559,13 @@ mod tests { assert_eq!(battery_feature_index([0x0001, 0x2201, 0x1b04]), None); assert_eq!(battery_feature_index([]), None); } + + #[test] + fn feature_trace_ids_are_hex_formatted() { + assert_eq!( + format_feature_ids(&[0x0001, 0x1004, 0x2202]), + "0x0001, 0x1004, 0x2202" + ); + assert_eq!(format_feature_ids(&[]), ""); + } } diff --git a/crates/openlogi-hid/src/inventory/probe.rs b/crates/openlogi-hid/src/inventory/probe.rs index e1d970ad..72fe01af 100644 --- a/crates/openlogi-hid/src/inventory/probe.rs +++ b/crates/openlogi-hid/src/inventory/probe.rs @@ -301,6 +301,14 @@ async fn probe_direct( ) -> NodeProbe { let id = CacheKey::Direct(info.id.clone()); let cached = cache.get(&id); + debug!( + name = %info.name, + vid = format_args!("{:04x}", info.vendor_id), + pid = format_args!("{:04x}", info.product_id), + cached = cached.is_some(), + tick, + "probing direct HID++ self slot" + ); // A direct device is always "present" (its HID node is the candidate), so // treat it as online: reuse the cached probe while fresh, otherwise probe. let (probe, outcome) = @@ -315,29 +323,50 @@ async fn probe_direct( // at the receiver, which sits at index 0 and steals every DPI / SmartShift // write attempt. We reuse the capabilities the probe already derived from // the feature table — no extra round-trip. - // A completed feature-table walk is what makes this probe's verdict - // trustworthy: without it (the device never answered) a rejection below - // would be indistinguishable from a transient glitch, so the node is - // settled as a failed probe and its last inventory replayed. + // A completed direct probe with no previous cache can reject a receiver + // secondary interface. Once this node has a last-good direct device, though, + // a no-evidence refresh is just another transient miss: replay the cached + // inventory instead of clearing it. let capabilities = probe.capabilities; 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; + let is_peripheral = probe.direct_peripheral_evidence; + let outcome_name = match &outcome { + CacheOutcome::Fresh(..) => "fresh", + CacheOutcome::Update(..) => "update", + CacheOutcome::Seen(..) => "seen", + CacheOutcome::Unkeyed => "unkeyed", + }; + debug!( + name = %info.name, + vid = format_args!("{:04x}", info.vendor_id), + pid = format_args!("{:04x}", info.product_id), + walk_succeeded, + is_peripheral, + has_battery = probe.battery.is_some(), + has_model = probe.model_info.is_some(), + kind = ?probe.kind, + caps_buttons = caps.buttons, + caps_pointer = caps.pointer, + caps_lighting = caps.lighting, + caps_scroll_inversion = caps.scroll_inversion, + direct_peripheral_evidence = probe.direct_peripheral_evidence, + cache_outcome = outcome_name, + "direct HID++ probe verdict" + ); if !is_peripheral { debug!( vid = format_args!("{:04x}", info.vendor_id), pid = format_args!("{:04x}", info.product_id), has_model = probe.model_info.is_some(), + walk_succeeded, + caps_buttons = caps.buttons, + caps_pointer = caps.pointer, + caps_lighting = caps.lighting, "slot 0xff exposes no battery or config feature — likely a receiver \ secondary interface; skipping" ); - // Don't cache or keep a rejected non-peripheral — `Unkeyed` lets any - // prior entry for this node be evicted. - return NodeProbe { - inventory: None, - healthy: walk_succeeded, - outcomes: vec![CacheOutcome::Unkeyed], - }; + return direct_non_peripheral_result(cached.is_some(), walk_succeeded, outcome); } // Without a Bolt receiver we don't have a wpid, codename, or pairing @@ -372,6 +401,40 @@ async fn probe_direct( } } +fn direct_non_peripheral_result( + cached: bool, + walk_succeeded: bool, + outcome: CacheOutcome, +) -> NodeProbe { + if walk_succeeded && !cached { + // A fresh, uncached direct node that answered but exposes no peripheral + // evidence is most likely a receiver's secondary HID++ interface. + return NodeProbe { + inventory: None, + healthy: true, + outcomes: vec![CacheOutcome::Unkeyed], + }; + } + + // For a known direct device, "no evidence this tick" is not an + // authoritative absence. Keep the cache entry seen, but never replace it + // with the empty/no-evidence probe that just failed to confirm the device. + NodeProbe { + inventory: None, + healthy: false, + outcomes: vec![seen_without_cache_update(outcome)], + } +} + +fn seen_without_cache_update(outcome: CacheOutcome) -> CacheOutcome { + match outcome { + CacheOutcome::Fresh(key, _) | CacheOutcome::Update(key, _) | CacheOutcome::Seen(key) => { + CacheOutcome::Seen(key) + } + CacheOutcome::Unkeyed => CacheOutcome::Unkeyed, + } +} + async fn drain_device_arrival(bolt: &BoltReceiver) -> Vec { let rx = bolt.listen(); if let Err(e) = bolt.trigger_device_arrival().await { @@ -495,3 +558,63 @@ async fn read_codename(channel: &HidppChannel, slot: u8) -> Option { .ok() .map(str::to_string) } + +#[cfg(test)] +mod tests { + use super::super::cache::{CacheKey, Cached}; + use super::super::features::ProbedFeatures; + use super::*; + + fn cached_probe() -> Cached { + Cached { + probe: ProbedFeatures::default(), + battery_index: None, + probed_tick: 7, + } + } + + #[test] + fn cached_direct_device_without_fresh_evidence_is_not_cleared() { + let result = direct_non_peripheral_result( + true, + true, + CacheOutcome::Fresh( + CacheKey::Bolt { + unit_id: [1, 2, 3, 4], + }, + cached_probe(), + ), + ); + + assert!(!result.healthy); + assert!(result.inventory.is_none()); + assert!( + matches!( + result.outcomes.as_slice(), + [CacheOutcome::Seen(CacheKey::Bolt { .. })] + ), + "a no-evidence refresh must keep the existing cache alive without replacing it" + ); + } + + #[test] + fn uncached_direct_non_peripheral_rejection_stays_authoritative() { + let result = direct_non_peripheral_result( + false, + true, + CacheOutcome::Fresh( + CacheKey::Bolt { + unit_id: [1, 2, 3, 4], + }, + cached_probe(), + ), + ); + + assert!(result.healthy); + assert!(result.inventory.is_none()); + assert!( + matches!(result.outcomes.as_slice(), [CacheOutcome::Unkeyed]), + "a receiver secondary interface must not be cached as a device" + ); + } +}