fix(hid): confirm Unifying reachability after battery errors

Author: Phecda

crates/openlogi-hid/src/inventory/probe.rs

@@ -22,7 +22,7 @@ use tracing::{debug, warn};
 use crate::mappings::{map_kind, map_unifying_kind, resolve_device_kind};
 use crate::route::DIRECT_DEVICE_INDEX;
 
-use super::cache::{CacheKey, CacheOutcome, Cached, is_stale, probe_or_reuse, seen};
+use super::cache::{CacheKey, CacheOutcome, Cached, probe_or_reuse, seen};
 use super::features::ProbedFeatures;
 use super::{ARRIVAL_DRAIN, BOLT_SLOT_PROBE, MAX_BOLT_SLOTS, UNIFYING_SLOT_PROBE};
 
@@ -602,25 +602,22 @@ async fn probe_unifying_slot(
 /// A successful full probe ([`CacheOutcome::Fresh`]) confirms liveness on a
 /// cache miss/stale entry. A fresh cached entry normally refreshes its battery,
 /// whose successful response ([`CacheOutcome::Update`]) is the liveness check.
-/// Devices without that feature get a root ping instead.
-async fn probe_unifying_features(
+/// A failed battery refresh, or a device without that feature, gets a root ping
+/// before being treated as offline.
+pub(super) async fn probe_unifying_features(
     channel: &Arc<HidppChannel>,
     slot: u8,
     id: &CacheKey,
     cached: Option<&Cached>,
     tick: u64,
 ) -> (ProbedFeatures, CacheOutcome, bool) {
-    if let Some(cached) = cached
-        && !is_stale(cached, tick)
-        && cached.battery_index.is_none()
-    {
-        let online = Device::new(Arc::clone(channel), slot).await.is_ok();
-        return (cached.probe.clone(), CacheOutcome::Seen(id.clone()), online);
-    }
-
     let (probe, outcome) =
         probe_or_reuse(channel, slot, Some(id.clone()), cached, true, tick).await;
-    let online = matches!(outcome, CacheOutcome::Fresh(..) | CacheOutcome::Update(..));
+    let online = if matches!(outcome, CacheOutcome::Fresh(..) | CacheOutcome::Update(..)) {
+        true
+    } else {
+        Device::new(Arc::clone(channel), slot).await.is_ok()
+    };
     (probe, outcome, online)
 }
 

crates/openlogi-hid/src/inventory/tests.rs

@@ -1,12 +1,15 @@
-use std::collections::HashSet;
+use std::{collections::HashSet, error::Error, io, sync::Arc};
 
+use hidpp::channel::{HidppChannel, RawHidChannel};
 use openlogi_core::device::{
     Capabilities, DeviceInventory, DeviceKind, PairedDevice, ReceiverInfo,
 };
+use tokio::sync::{Mutex, mpsc};
 
 use super::cache::{CACHE_MISS_GRACE, CacheKey, CacheOutcome, Cached, REFRESH_TICKS, is_stale};
 use super::probe::{
     NodeProbe, assemble_bolt_probe, assemble_unifying_device, parse_codename_unifying,
+    probe_unifying_features,
 };
 use super::{Enumerator, ONESHOT_ATTEMPTS, one_shot_should_stop};
 use crate::inventory::features::ProbedFeatures;
@@ -105,6 +108,101 @@ fn unifying_cached_features_do_not_override_current_liveness() {
     );
 }
 
+struct BatteryErrorPingChannel {
+    incoming_tx: mpsc::UnboundedSender<Vec<u8>>,
+    incoming_rx: Mutex<mpsc::UnboundedReceiver<Vec<u8>>>,
+}
+
+impl BatteryErrorPingChannel {
+    fn new() -> Self {
+        let (incoming_tx, incoming_rx) = mpsc::unbounded_channel();
+        Self {
+            incoming_tx,
+            incoming_rx: Mutex::new(incoming_rx),
+        }
+    }
+}
+
+#[hidpp::async_trait]
+impl RawHidChannel for BatteryErrorPingChannel {
+    fn vendor_id(&self) -> u16 {
+        0x046d
+    }
+
+    fn product_id(&self) -> u16 {
+        0xc52b
+    }
+
+    async fn write_report(&self, src: &[u8]) -> Result<usize, Box<dyn Error + Send + Sync>> {
+        let mut response = src.to_vec();
+        if src.get(2).copied() != Some(0) {
+            if response.len() < 7 {
+                return Err(Box::new(io::Error::other("short mock HID++ report")));
+            }
+            response[2] = 0xff;
+            response[3] = src[2];
+            response[4] = src[3];
+            response[5] = 0x08;
+            response[6] = 0;
+        }
+        self.incoming_tx.send(response).map_err(|_| {
+            Box::new(io::Error::other("mock HID++ response receiver closed"))
+                as Box<dyn Error + Send + Sync>
+        })?;
+        Ok(src.len())
+    }
+
+    async fn read_report(&self, buf: &mut [u8]) -> Result<usize, Box<dyn Error + Send + Sync>> {
+        let Some(report) = self.incoming_rx.lock().await.recv().await else {
+            return Err(Box::new(io::Error::other(
+                "mock HID++ response sender closed",
+            )));
+        };
+        let len = report.len().min(buf.len());
+        buf[..len].copy_from_slice(&report[..len]);
+        Ok(len)
+    }
+
+    fn supports_short_long_hidpp(&self) -> Option<(bool, bool)> {
+        Some((true, true))
+    }
+
+    async fn get_report_descriptor(
+        &self,
+        _buf: &mut [u8],
+    ) -> Result<usize, Box<dyn Error + Send + Sync>> {
+        unreachable!("mock declares HID++ support")
+    }
+}
+
+#[tokio::test]
+async fn unifying_battery_failure_uses_root_ping_before_marking_offline() {
+    let channel = Arc::new(
+        HidppChannel::from_raw_channel(BatteryErrorPingChannel::new())
+            .await
+            .unwrap_or_else(|e| panic!("mock HID++ channel should open: {e}")),
+    );
+    let id = CacheKey::UnifyingSlot {
+        receiver_uid: "receiver".to_string(),
+        slot: 1,
+    };
+    let cached = Cached {
+        probe: ProbedFeatures {
+            capabilities: Some(Capabilities::default()),
+            ..ProbedFeatures::default()
+        },
+        battery_index: Some(4),
+        probed_tick: 10,
+    };
+
+    let (_, _, online) = probe_unifying_features(&channel, 1, &id, Some(&cached), 11).await;
+
+    assert!(
+        online,
+        "a failed battery refresh is inconclusive when the root ping still answers"
+    );
+}
+
 fn inventory(slots: &[u8]) -> Vec<DeviceInventory> {
     vec![DeviceInventory {
         receiver: ReceiverInfo {