feat(battery): support legacy 0x1000 BatteryStatus and its charging quirk

[laofun]

Summary

Older mice like a Bluetooth-direct MX Master 2S only expose the legacy BatteryStatus feature (0x1000), never the unified 0x1004, so they showed no battery at all. This adds 0x1000 support and handles its quirk: the firmware reports 0% while charging.

Changes

• Implement 0x1000, mirroring the SmartShift 0x2110/0x2111 fallback. The inventory probe prefers 0x1004 and falls back to 0x1000. Coarse BatteryLevel from fixed buckets (0x1000 has no level bitmask).
• Hold the last-known % through a charge session so the reading does not flip to "Charging · 0%" once one discharge read exists.
• Cold start (charger plugged before the app opens, no cached %): show "Charging" instead of a bogus 0% across the card, summary, and tray menu.
• Add openlogi diag battery to print the raw 0x1004/0x1000 report.

Testing

• cargo fmt --all -- --check, cargo clippy --workspace, cargo test (bucket mapping, hold-% across all four branches, GUI label).
• On hardware: MX2S shows 50% discharging over BT-direct, and 0% status=Recharging while charging.

[greptile-apps]

Greptile Summary

This PR adds support for the legacy 0x1000 BatteryStatus HID++ feature so older Bluetooth-direct devices (e.g. MX Master 2S) show battery readings instead of nothing, and handles the firmware quirk where discharge_level is always 0 while charging. All four concerns raised in previous review threads are addressed: the hold_percentage_while_charging guard is properly scoped to BatteryProbe::Legacy, the ?-early-exit in the feature-table loop is replaced with break, LegacyBatteryStatus::Other = 7 prevents an undocumented status value from erasing the battery widget, and the fallthrough error in read_battery_raw now reports 0x1004 rather than 0x1000.

• Protocol layer (battery_status/mod.rs, features.rs, mappings.rs): new BatteryStatusFeature and BatteryProbe enum; battery_feature_index prefers 0x1004 and falls back to 0x1000, mirroring the SmartShift 0x2111/0x2110 pattern.
• Cache layer (cache.rs): hold_percentage_while_charging carries the last-known percentage forward through a charge session for Legacy devices only; both the full-probe and cache-hit paths thread it correctly, with unit tests covering all branches.
• GUI layer (widgets.rs, home.rs, app_menu.rs): cold-start charging state (no prior %) is rendered as "Charging" without a bogus 0%, applied consistently across the gallery card, detail summary, progress bar, and tray menu.

Confidence Score: 5/5

• This PR is safe to merge. The changes are well-scoped, address a real hardware gap, and all previously-raised review concerns are resolved with correct guards and matching tests.
• The core hold logic, feature-table lookup, and GUI display paths are all correctly implemented and covered by unit tests. The only non-trivial observation — that battery_charging_no_reading applies universally rather than just to Legacy probes — has no practical impact on current Logitech hardware, and the hold layer already handles the critical data-correctness side of the same distinction.
• No files require special attention. cache.rs has the most complex new logic but it is well-tested and the probe-type guard is correctly placed.

Important Files Changed

Filename	Overview
crates/openlogi-hidpp/src/feature/battery_status/mod.rs	New BatteryStatusFeature (0x1000) implementation. Enum covers all known status values 0–7, including the previously-raised Other = 7 variant that prevents a parse failure from making the battery indicator vanish. #[non_exhaustive] is set and the upstream mapper has a wildcard arm, so any undocumented future value ≥ 8 will propagate as Unknown rather than crashing.
crates/openlogi-hid/src/inventory/features.rs	Introduces BatteryProbe enum and refactors battery_feature_index to prefer 0x1004 and fall back to 0x1000. The previously-raised ?-early-exit concern is addressed with break. New tests cover all four cases (unified found, legacy-only, unified preferred over legacy, neither).
crates/openlogi-hid/src/inventory/cache.rs	Adds hold_percentage_while_charging with an explicit BatteryProbe::Legacy guard — the previously-raised issue of the hold applying unconditionally to Unified probes is now fixed. Correctly threaded through both the full-probe and cache-hit paths, with unit tests covering all four branches.
crates/openlogi-hid/src/mappings.rs	Adds map_legacy_battery_status and legacy_battery_level_from_percentage. Status mapping is complete and matches the Solaar reference. The value-7 (Other) test confirms TryFromPrimitive + map_legacy_battery_status round-trips to Unknown rather than silently erasing the battery widget.
crates/openlogi-gui/src/app/widgets.rs	Adds battery_charging_no_reading for cold-start charging display and the associated UI logic in battery_summary. The function correctly suppresses the progress bar and percentage text when showing a bogus 0% during charging.
crates/openlogi-hid/src/write/diagnostics.rs	Adds read_battery_raw for openlogi diag battery. Probes 0x1004 first, falls back to 0x1000, and reports the corrected feature_hex: 0x1004 error when neither is present — addressing the previously-raised hardcoded-0x1000 issue.
crates/openlogi-gui/src/app_menu.rs	Tray menu correctly handles the cold-start charging case by testing battery_charging_no_reading before formatting the percentage string, keeping the menu item label accurate.
crates/openlogi-gui/src/app/home.rs	Gallery card battery view uses battery_charging_no_reading to choose between "Charging" text and the percentage string. Battery icon is still rendered in both branches so the charging glyph remains visible during cold start.
crates/openlogi-cli/src/cmd/diag/battery.rs	New diagnostic command that selects a device exposing 0x1000 or 0x1004 and prints the raw firmware report via read_battery_raw.

Sequence Diagram

sequenceDiagram
    participant GUI
    participant Cache as probe_or_reuse (cache.rs)
    participant Features as features.rs
    participant HID as HID++ wire

    GUI->>Cache: enumerate tick
    alt cache miss / stale
        Cache->>Features: probe_features(channel, slot)
        Features->>HID: Device::new + enumerate_features()
        HID-->>Features: feature-ID table
        Features->>Features: "battery_feature_index(ids)<br/>prefers 0x1004, falls back 0x1000"
        Features->>HID: read_battery(probe) → Unified or Legacy path
        HID-->>Features: BatteryInfo (percentage, level, status)
        Features-->>Cache: "(ProbedFeatures, Option<BatteryProbe>)"
        Cache->>Cache: "hold_percentage_while_charging()<br/>Only for BatteryProbe::Legacy:<br/>carry prior % when charging+0%"
        Cache-->>GUI: ProbedFeatures (battery held if Legacy quirk)
    else cache hit
        Cache->>HID: read_battery(c.battery probe) — one round-trip
        HID-->>Cache: BatteryInfo
        Cache->>Cache: "hold_percentage_while_charging()<br/>(Legacy guard same as above)"
        Cache-->>GUI: updated ProbedFeatures
    end

    GUI->>GUI: "battery_charging_no_reading(b)<br/>true when Charging+0% (cold-start)<br/>→ show "Charging" without bogus 0%"
    GUI->>GUI: "battery_view / battery_summary / tray menu<br/>render suppressed or held percentage"

Loading

_{Reviews (7): Last reviewed commit: "style(hid): rustfmt write re-exports" | Re-trigger Greptile}

[laofun]

Thanks @greptile-apps — applied all three:

1. write.rs fallthrough now reports feature_hex: 0x1004 (the preferred feature) instead of implying 0x1000 was specifically absent.
2. battery_feature_index now breaks past u8::MAX instead of ?-returning None and discarding an already-found legacy probe.
3. Added LegacyBatteryStatus::Other = 7 so a device reporting Solaar's value 7 surfaces as BatteryStatus::Unknown rather than failing the parse and making the battery vanish — covered by a new unit test.

[greptile-apps]

All three changes look correct and well-executed. A few observations:

1. write.rs fallthrough (feature_hex: 0x1004) — correct. The comment makes the intent clear, and 0x1004 is the right "preferred" feature to report when neither is present.

2. battery_feature_index break past u8::MAX — the loop structure is clean. The comment even acknowledges the unreachability honestly. The legacy accumulation across the loop means an early 0x1000 entry won't be lost if the table somehow exceeds 255 entries before a 0x1004 hit.

3. LegacyBatteryStatus::Other = 7 — the test in mappings.rs is a good regression anchor:

let other = HidppLegacyBatteryStatus::try_from(7u8).expect("7 is a known variant");
assert_eq!(map_legacy_battery_status(other), BatteryStatus::Unknown);

This validates both that the TryFromPrimitive derive accepts 7 and that the wildcard arm in map_legacy_battery_status routes it to Unknown rather than failing.

One minor nit on get_battery_level_status in battery_status/mod.rs: the LegacyBatteryStatus::try_from failure path still maps to Hidpp20Error::UnsupportedResponse. With Other = 7 added, the only way to hit that arm now is a truly undocumented value (8+). The mapping is reasonable, but worth confirming that TryFromPrimitive on a #[non_exhaustive] enum still derives TryFrom<u8> for only the declared variants — it does, since #[non_exhaustive] is a stability guard for downstream crates, not a change to the derive semantics. No action needed.

The PR is in good shape. LGTM.

Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.

[AprilNEA]

Thanks for the careful follow-up here — the 0x1000 fallback, diagnostic command, and regression tests all look well-scoped, and CI is green.

I found two edge cases worth tightening before merge:

1. hold_percentage_while_charging should probably be scoped to BatteryProbe::Legacy. The helper and comments describe the MX2S / 0x1000 charging quirk, but it is currently applied after probe_features and on cache refresh regardless of whether the selected probe is Unified or Legacy (crates/openlogi-hid/src/inventory.rs, around the calls after fresh reads). On a 0x1004 device that legitimately reports charging_percentage = 0 while charging, or reports no percentage support, a previous non-zero cached value could be carried forward even though the legacy quirk is not involved. Matching on BatteryProbe::Legacy at the call site would keep the workaround tied to the feature that needs it.

2. The cold-start GUI suppression hides the 0% text, but the detail summary progress bar still renders from battery.percentage (relative_percent(0) and battery_color(0)). That means the summary can still show an empty/critical-looking bar for the same “Charging, no reliable reading” case. It may be worth hiding the bar, using a neutral/charging style, or otherwise routing battery_charging_no_reading(battery) through the progress-bar rendering too.

Otherwise this is a nice, focused addition — especially the raw openlogi diag battery path for verifying the firmware behavior on real devices.

[laofun]

Both addressed:

1. hold_percentage_while_charging now takes the BatteryProbe and early-returns unless it's Legacy, so the hold is tied to the 0x1000 quirk — a 0x1004 device reporting a genuine 0% while charging surfaces it. Added a unified_charging_zero_is_not_held test.
2. The summary progress bar now routes through battery_charging_no_reading() too: in the "charging, no reliable reading" case it leaves the track empty instead of drawing the 1%-wide critical sliver.

[greptile-apps]

Want your agent to iterate on Greptile's feedback? Try greploops.