fix(hid): support 0x1001 BatteryVoltage in diag battery (#688)

crates/openlogi-cli/src/cmd/diag.rs

@@ -24,7 +24,7 @@ pub enum DiagCmd {
     Features(features::FeaturesArgs),
     /// Dump HID++ 0x1b04 reprogrammable controls and capability flags.
     Controls(controls::ControlsArgs),
-    /// Read the raw battery report (0x1004 or 0x1000 fields).
+    /// Read the raw battery report (0x1004, 0x1000, or 0x1001 fields).
     Battery(battery::BatteryArgs),
     /// Read DPI → write a small delta → read back → restore → report.
     Dpi(dpi::DpiArgs),

crates/openlogi-cli/src/cmd/diag/battery.rs

@@ -1,7 +1,8 @@
 //! `openlogi diag battery` — dump the device's raw battery report.
 //!
-//! Prints exactly what the firmware returns (unified `0x1004` fields, or legacy
-//! `0x1000` `discharge_level`/`next_level`/`status`). Run it once on battery and
+//! Prints exactly what the firmware returns (unified `0x1004` fields, legacy
+//! `0x1000` `discharge_level`/`next_level`/`status`, or `0x1001`
+//! `voltage_mv`/`status`/`critical`). Run it once on battery and
 //! once with the charger plugged in to see how the device reports while charging
 //! — e.g. an MX2S returns `discharge_level=0` mid-charge, which is the device's
 //! own limitation, not a bug in the read path.
@@ -20,8 +21,8 @@ pub struct BatteryArgs {
 }
 
 pub async fn run(args: BatteryArgs) -> Result<()> {
-    // 0x1004 UnifiedBattery / 0x1000 BatteryStatus — pick a device with either.
-    let (route, name) = select_device(args.device.as_deref(), &[0x1000, 0x1004]).await?;
+    // 0x1004 UnifiedBattery / 0x1000 BatteryStatus / 0x1001 BatteryVoltage — pick a device with any.
+    let (route, name) = select_device(args.device.as_deref(), &[0x1000, 0x1001, 0x1004]).await?;
     println!("device: {name} ({route})");
 
     let line = openlogi_hid::read_battery_raw(&route)

crates/openlogi-hid/src/write/diagnostics.rs

@@ -2,7 +2,8 @@ use std::sync::Arc;
 
 use hidpp::{
     device::Device, feature::CreatableFeature, feature::battery_status::BatteryStatusFeature,
-    feature::feature_set::FeatureSetFeature, feature::unified_battery::UnifiedBatteryFeature,
+    feature::battery_voltage::BatteryVoltageFeature, feature::feature_set::FeatureSetFeature,
+    feature::unified_battery::UnifiedBatteryFeature,
 };
 
 use crate::channel::route::DeviceRoute;
@@ -125,52 +126,72 @@ pub async fn dump_reprog_controls(
 }
 
 /// Diagnostic read of the device's raw battery report — the unified `0x1004`
-/// fields, or the legacy `0x1000` `discharge_level`/`next_level`/`status`. For
-/// `openlogi diag battery`: surfaces exactly what the firmware reports so a
-/// claim like "MX2S shows 0% while charging" can be confirmed against the wire
-/// instead of guessed (the GUI only ever shows the mapped value).
+/// fields, legacy `0x1000` `discharge_level`/`next_level`/`status`, or `0x1001`
+/// `voltage_mv`/`status`/`critical`. For `openlogi diag battery`: surfaces
+/// exactly what the firmware reports so a claim like "MX2S shows 0% while
+/// charging" can be confirmed against the wire instead of guessed (the GUI only
+/// ever shows the mapped value).
 pub async fn read_battery_raw(route: &DeviceRoute) -> Result<String, WriteError> {
     let index = route.device_index();
     with_route(route, move |channel| async move {
         let mut device = Device::new(Arc::clone(&channel), index)
             .await
             .map_err(|_| WriteError::DeviceUnreachable { index })?;
 
-        match open_feature::<UnifiedBatteryFeature>(&mut device).await {
-            Ok(feature) => {
-                let info = feature
-                    .get_battery_info()
-                    .await
-                    .map_err(|e| WriteError::Hidpp(format!("{e:?}")))?;
-                return Ok(format!(
-                    "0x1004 UnifiedBattery: percentage={} level={:?} status={:?}",
-                    info.charging_percentage, info.level, info.status
-                ));
-            }
-            Err(WriteError::FeatureUnsupported { .. }) => {}
-            Err(e) => return Err(e),
+        read_battery_raw_device(&mut device).await
+    })
+    .await
+}
+
+pub(crate) async fn read_battery_raw_device(device: &mut Device) -> Result<String, WriteError> {
+    match open_feature::<UnifiedBatteryFeature>(device).await {
+        Ok(feature) => {
+            let info = feature
+                .get_battery_info()
+                .await
+                .map_err(|e| WriteError::Hidpp(format!("{e:?}")))?;
+            return Ok(format!(
+                "0x1004 UnifiedBattery: percentage={} level={:?} status={:?}",
+                info.charging_percentage, info.level, info.status
+            ));
+        }
+        Err(WriteError::FeatureUnsupported { .. }) => {}
+        Err(e) => return Err(e),
+    }
+
+    match open_feature::<BatteryStatusFeature>(device).await {
+        Ok(feature) => {
+            let info = feature
+                .get_battery_level_status()
+                .await
+                .map_err(|e| WriteError::Hidpp(format!("{e:?}")))?;
+            return Ok(format!(
+                "0x1000 BatteryStatus: discharge_level={} next_level={} status={:?}",
+                info.discharge_level, info.next_level, info.status
+            ));
         }
+        Err(WriteError::FeatureUnsupported { .. }) => {}
+        Err(e) => return Err(e),
+    }
 
-        match open_feature::<BatteryStatusFeature>(&mut device).await {
-            Ok(feature) => {
-                let info = feature
-                    .get_battery_level_status()
-                    .await
-                    .map_err(|e| WriteError::Hidpp(format!("{e:?}")))?;
-                return Ok(format!(
-                    "0x1000 BatteryStatus: discharge_level={} next_level={} status={:?}",
-                    info.discharge_level, info.next_level, info.status
-                ));
-            }
-            Err(WriteError::FeatureUnsupported { .. }) => {}
-            Err(e) => return Err(e),
+    match open_feature::<BatteryVoltageFeature>(device).await {
+        Ok(feature) => {
+            let info = feature
+                .get_battery_info()
+                .await
+                .map_err(|e| WriteError::Hidpp(format!("{e:?}")))?;
+            return Ok(format!(
+                "0x1001 BatteryVoltage: voltage_mv={} status={:?} critical={}",
+                info.voltage_mv, info.status, info.critical
+            ));
         }
+        Err(WriteError::FeatureUnsupported { .. }) => {}
+        Err(e) => return Err(e),
+    }
 
-        // Reached only when neither 0x1004 nor 0x1000 is present; report the
-        // preferred feature rather than implying 0x1000 was specifically absent.
-        Err(WriteError::FeatureUnsupported {
-            feature_hex: 0x1004,
-        })
+    // Reached only when neither 0x1004, 0x1000, nor 0x1001 is present; report the
+    // preferred feature rather than implying 0x1000 or 0x1001 was specifically absent.
+    Err(WriteError::FeatureUnsupported {
+        feature_hex: UnifiedBatteryFeature::ID,
     })
-    .await
 }

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

@@ -613,3 +613,126 @@ fn per_key_v2_scripted_response(request: &[u8]) -> Option<Vec<u8>> {
     response[4..].copy_from_slice(&payload[..payload_len]);
     Some(response)
 }
+
+#[tokio::test]
+async fn read_battery_raw_handles_unified_legacy_and_voltage_features() -> Result<(), WriteError> {
+    // 1. Unified 0x1004 device
+    let (raw, _) =
+        ScriptedRawHidChannel::with_responder(|req| battery_scripted_response(req, Some(0x1004)));
+    let chan = Arc::new(
+        HidppChannel::from_raw_channel(raw)
+            .await
+            .expect("scripted channel"),
+    );
+    let mut dev = Device::new(chan, 0xff).await.expect("device");
+    let report = diagnostics::read_battery_raw_device(&mut dev).await?;
+    assert!(report.starts_with("0x1004 UnifiedBattery:"));
+    assert!(report.contains("percentage=85"));
+
+    // 2. Legacy 0x1000 device
+    let (raw, _) =
+        ScriptedRawHidChannel::with_responder(|req| battery_scripted_response(req, Some(0x1000)));
+    let chan = Arc::new(
+        HidppChannel::from_raw_channel(raw)
+            .await
+            .expect("scripted channel"),
+    );
+    let mut dev = Device::new(chan, 0xff).await.expect("device");
+    let report = diagnostics::read_battery_raw_device(&mut dev).await?;
+    assert!(report.starts_with("0x1000 BatteryStatus:"));
+    assert!(report.contains("discharge_level=70"));
+
+    // 3. Voltage 0x1001 device (e.g. G502 / G915)
+    let (raw, _) =
+        ScriptedRawHidChannel::with_responder(|req| battery_scripted_response(req, Some(0x1001)));
+    let chan = Arc::new(
+        HidppChannel::from_raw_channel(raw)
+            .await
+            .expect("scripted channel"),
+    );
+    let mut dev = Device::new(chan, 0xff).await.expect("device");
+    let report = diagnostics::read_battery_raw_device(&mut dev).await?;
+    assert!(report.starts_with("0x1001 BatteryVoltage:"));
+    assert!(report.contains("voltage_mv=3950"));
+
+    // 4. Device without any battery feature
+    let (raw, _) =
+        ScriptedRawHidChannel::with_responder(|req| battery_scripted_response(req, None));
+    let chan = Arc::new(
+        HidppChannel::from_raw_channel(raw)
+            .await
+            .expect("scripted channel"),
+    );
+    let mut dev = Device::new(chan, 0xff).await.expect("device");
+    let err = diagnostics::read_battery_raw_device(&mut dev)
+        .await
+        .unwrap_err();
+    assert!(matches!(
+        err,
+        WriteError::FeatureUnsupported {
+            feature_hex: 0x1004
+        }
+    ));
+
+    Ok(())
+}
+
+fn battery_scripted_response(request: &[u8], feature_id: Option<u16>) -> Option<Vec<u8>> {
+    if request.len() < 7 || !matches!(request[0], 0x10 | 0x11) {
+        return None;
+    }
+    let feature_index = request[2];
+    let function = request[3] >> 4;
+    let mut payload = [0u8; 16];
+    let long = match (feature_index, function) {
+        // Root ping used by Device::new.
+        (0x00, 0x01) => {
+            payload[0] = 4;
+            false
+        }
+        // Root feature lookup.
+        (0x00, 0x00) => {
+            let req_fid = u16::from_be_bytes([request[4], request[5]]);
+            payload[0] = if Some(req_fid) == feature_id {
+                0x05
+            } else {
+                0x00
+            };
+            false
+        }
+        // Battery reading function (feature index 0x05, function 0 or 1)
+        (0x05, 0x00 | 0x01) => {
+            match feature_id {
+                Some(0x1004) if function == 1 => {
+                    // UnifiedBattery: percentage = 85, level = Good (4), status = Discharging (0)
+                    payload[0] = 85;
+                    payload[1] = 4;
+                    payload[2] = 0;
+                    true
+                }
+                Some(0x1000) if function == 0 => {
+                    // BatteryStatus: discharge_level = 70, next_level = 50, status = Discharging (0)
+                    payload[0] = 70;
+                    payload[1] = 50;
+                    payload[2] = 0;
+                    true
+                }
+                Some(0x1001) if function == 0 => {
+                    // BatteryVoltage: 3950 mV (0x0f6e), status flags = 0 (discharging)
+                    payload[..2].copy_from_slice(&3950u16.to_be_bytes());
+                    payload[2] = 0;
+                    true
+                }
+                _ => return None,
+            }
+        }
+        _ => return None,
+    };
+
+    let mut response = vec![0u8; if long { 20 } else { 7 }];
+    response[0] = if long { 0x11 } else { 0x10 };
+    response[1..4].copy_from_slice(&request[1..4]);
+    let payload_len = response.len() - 4;
+    response[4..].copy_from_slice(&payload[..payload_len]);
+    Some(response)
+}