diff --git a/crates/openlogi-agent-core/src/orchestrator.rs b/crates/openlogi-agent-core/src/orchestrator.rs index 019d08bb..f1644141 100644 --- a/crates/openlogi-agent-core/src/orchestrator.rs +++ b/crates/openlogi-agent-core/src/orchestrator.rs @@ -267,9 +267,12 @@ impl Orchestrator { } /// Push the saved native scroll-inversion bit to every currently online - /// device. This is separated from [`Self::rebuild`] because rebuilding also - /// happens for foreground-app changes, while the HID++ write is only needed - /// when config or device presence changes. + /// device. Separated from [`Self::rebuild`] (which also runs on + /// foreground-app changes) because the HID++ write is only needed when + /// config or device presence changes. The write short-circuits at the + /// `0x2121` layer when the wheel already holds the desired state, so calling + /// it on every reload costs at most one wheel-mode read per device — and + /// still recovers a device whose earlier write timed out while it was waking. fn apply_native_scroll_inversions(&self) { for dev in self .devices diff --git a/crates/openlogi-core/src/config.rs b/crates/openlogi-core/src/config.rs index 87702587..9346e090 100644 --- a/crates/openlogi-core/src/config.rs +++ b/crates/openlogi-core/src/config.rs @@ -271,6 +271,35 @@ pub enum WheelMode { Ratchet, } +/// SmartShift auto-disengage out-of-box default (`16` ≈ 4 turn/s, per the +/// x2110 / x2111 spec). The sensitivity slider's default and the heal target +/// for a corrupt persisted threshold. +pub const SMARTSHIFT_AUTO_DISENGAGE_DEFAULT: u8 = 16; + +/// Smallest auto-disengage threshold OpenLogi will store or apply (`8` ≈ +/// 2 turn/s). Below this the ratchet releases into free-spin at everyday scroll +/// speeds, leaving the wheel "stuck" spinning (#317); `0` is also the firmware +/// "do not change" sentinel that must never be stored as a real value. A +/// persisted threshold below this floor is a corrupt artifact and is healed to +/// [`SMARTSHIFT_AUTO_DISENGAGE_DEFAULT`] on load. +pub const SMARTSHIFT_MIN_AUTO_DISENGAGE: u8 = 8; + +/// Heal a persisted auto-disengage threshold on load: anything below +/// [`SMARTSHIFT_MIN_AUTO_DISENGAGE`] (including the `0` sentinel) becomes the +/// default. `0xFF` (permanent ratchet) and every real threshold at or above the +/// floor pass through unchanged. +fn deserialize_auto_disengage<'de, D>(deserializer: D) -> Result +where + D: serde::Deserializer<'de>, +{ + let value = u8::deserialize(deserializer)?; + Ok(if value < SMARTSHIFT_MIN_AUTO_DISENGAGE { + SMARTSHIFT_AUTO_DISENGAGE_DEFAULT + } else { + value + }) +} + /// Per-device SmartShift wheel configuration, persisted so the agent can /// re-apply it when the device reconnects: the values are written to device /// RAM and do not survive a power cycle (#189), despite earlier assumptions @@ -282,8 +311,10 @@ pub enum WheelMode { #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] pub struct SmartShift { pub mode: WheelMode, - /// SmartShift auto-disengage threshold (`0x01`–`0xFE`, in 0.25 turn/s - /// steps), or `0xFF` for a permanently engaged ratchet. + /// SmartShift auto-disengage threshold (`0x08`–`0xFE`, in 0.25 turn/s + /// steps), or `0xFF` for a permanently engaged ratchet. A persisted value + /// below [`SMARTSHIFT_MIN_AUTO_DISENGAGE`] is healed to the default on load. + #[serde(deserialize_with = "deserialize_auto_disengage")] pub auto_disengage: u8, /// Tunable-torque force percentage (`1`–`100`), `0` when the device /// doesn't support tunable torque. @@ -1101,6 +1132,32 @@ mod tests { ); } + #[test] + fn low_auto_disengage_heals_to_default_on_load() { + // A pre-#317 config could persist a runaway-low threshold (or the `0` + // sentinel); loading it must heal to the default so reapply doesn't + // re-program free-spin-on-any-scroll into the device — while a real + // threshold and the `0xFF` permanent-ratchet value pass through. + let heal = |v: u8| { + let body = format!("mode = \"ratchet\"\nauto_disengage = {v}\ntunable_torque = 50\n"); + toml::from_str::(&body) + .expect("parse") + .auto_disengage + }; + assert_eq!(heal(0), SMARTSHIFT_AUTO_DISENGAGE_DEFAULT); + assert_eq!(heal(1), SMARTSHIFT_AUTO_DISENGAGE_DEFAULT); + assert_eq!( + heal(SMARTSHIFT_MIN_AUTO_DISENGAGE - 1), + SMARTSHIFT_AUTO_DISENGAGE_DEFAULT + ); + assert_eq!( + heal(SMARTSHIFT_MIN_AUTO_DISENGAGE), + SMARTSHIFT_MIN_AUTO_DISENGAGE + ); + assert_eq!(heal(16), 16); + assert_eq!(heal(0xff), 0xff); + } + #[test] fn bindings_roundtrip_per_device() { let mut cfg = Config::default(); diff --git a/crates/openlogi-gui/src/components/smartshift_panel.rs b/crates/openlogi-gui/src/components/smartshift_panel.rs index 70ab748a..b2cd4dae 100644 --- a/crates/openlogi-gui/src/components/smartshift_panel.rs +++ b/crates/openlogi-gui/src/components/smartshift_panel.rs @@ -5,11 +5,12 @@ //! and a **permanent ratchet** toggle. The latter two only apply in ratchet //! mode, so they grey out under free-spin. //! -//! Unlike DPI presets, nothing here is written to `config.toml`: the device -//! persists wheel mode / threshold / torque in its own non-volatile memory, so -//! the panel only reads and writes the device (via -//! [`AppState::commit_smartshift`]). The current state is read lazily on the -//! same background-thread pattern as [`crate::components::dpi_panel`]. +//! Each change is written to the device *and* persisted to `config.toml` (via +//! [`AppState::commit_smartshift`]): the device holds wheel mode / threshold / +//! torque in volatile RAM that resets on a power cycle (#189), so the agent +//! re-applies the saved config when the device reconnects. The current state is +//! read lazily on the same background-thread pattern as +//! [`crate::components::dpi_panel`]. use gpui::{ AnyElement, AppContext as _, BorrowAppContext as _, Context, Entity, InteractiveElement, @@ -21,6 +22,7 @@ use gpui_component::{ slider::{Slider, SliderEvent, SliderState}, v_flex, }; +use openlogi_core::config::{SMARTSHIFT_AUTO_DISENGAGE_DEFAULT, SMARTSHIFT_MIN_AUTO_DISENGAGE}; use openlogi_hid::{AUTO_DISENGAGE_PERMANENT, DeviceRoute, SmartShiftMode, SmartShiftStatus}; use crate::components::device_read::issue_device_read; @@ -29,13 +31,15 @@ use crate::state::{AppState, SmartShiftLoad}; use crate::theme::{self, ACCENT_BLUE, Palette, SelectableStyle}; /// Friendly slider range for the `autoDisengage` threshold. The wire field is -/// `0x01`–`0xFE` (0.25 turn/s steps), but realistic scroll speeds sit well -/// inside 1–50 (≈12.5 turn/s) — Logitech's own default is ~16. A device -/// reporting a value above this is clamped for display; it is only rewritten -/// once the user actually drags the slider. -const THRESHOLD_MIN: u8 = 1; +/// `0x01`–`0xFE` (0.25 turn/s steps); the slider exposes the usable band +/// [`SMARTSHIFT_MIN_AUTO_DISENGAGE`]–`50` (≈2–12.5 turn/s, default ~16). +/// Thresholds below the floor free-spin on everyday scrolling (#317), so the +/// floor and default are shared with the `openlogi-core` config heal. A device +/// reporting a value outside the band is normalised for display by +/// [`clamp_threshold`]; it is only rewritten once the user drags the slider. +const THRESHOLD_MIN: u8 = SMARTSHIFT_MIN_AUTO_DISENGAGE; const THRESHOLD_MAX: u8 = 50; -const DEFAULT_THRESHOLD: u8 = 16; +const DEFAULT_THRESHOLD: u8 = SMARTSHIFT_AUTO_DISENGAGE_DEFAULT; pub struct SmartShiftPanel { /// The auto-disengage threshold slider. Always constructed (range is @@ -420,7 +424,41 @@ fn raw_to_threshold(raw: f32) -> u8 { .clamp(f32::from(THRESHOLD_MIN), f32::from(THRESHOLD_MAX)) as u8 } -/// Clamp a device-reported threshold into the slider's friendly range. +/// Map a device-reported threshold into the slider's friendly band for display. +/// +/// A non-permanent auto-disengage below [`THRESHOLD_MIN`] — including the `0` +/// "do not change"/unset sentinel — releases the wheel into free-spin on the +/// gentlest scroll (#317), so it must never seed the slider or the +/// permanent-ratchet restore at that runaway value. Such values are normalised +/// to the default (matching the `openlogi-core` config heal); values above the +/// band clamp down to [`THRESHOLD_MAX`]. (`0xFF` permanent ratchet never reaches +/// here — the caller handles it before clamping.) fn clamp_threshold(value: u8) -> u8 { - value.clamp(THRESHOLD_MIN, THRESHOLD_MAX) + if value < THRESHOLD_MIN { + DEFAULT_THRESHOLD + } else { + value.min(THRESHOLD_MAX) + } +} + +#[cfg(test)] +mod tests { + use super::{DEFAULT_THRESHOLD, THRESHOLD_MAX, THRESHOLD_MIN, clamp_threshold}; + + #[test] + fn clamp_threshold_heals_sub_floor_to_default() { + // 0 (the firmware "do not change" sentinel) and any sub-floor value + // used to seed the slider / permanent-ratchet restore with a runaway + // free-spin threshold (#317); they normalise to the default instead. + assert_eq!(clamp_threshold(0), DEFAULT_THRESHOLD); + assert_eq!(clamp_threshold(1), DEFAULT_THRESHOLD); + assert_eq!(clamp_threshold(THRESHOLD_MIN - 1), DEFAULT_THRESHOLD); + } + + #[test] + fn clamp_threshold_keeps_in_band_values_and_clamps_high() { + assert_eq!(clamp_threshold(THRESHOLD_MIN), THRESHOLD_MIN); + assert_eq!(clamp_threshold(16), 16); + assert_eq!(clamp_threshold(200), THRESHOLD_MAX); + } } diff --git a/crates/openlogi-hid/src/hires_wheel.rs b/crates/openlogi-hid/src/hires_wheel.rs index 38f7e46f..2ac87307 100644 --- a/crates/openlogi-hid/src/hires_wheel.rs +++ b/crates/openlogi-hid/src/hires_wheel.rs @@ -52,6 +52,17 @@ async fn set_scroll_inversion_on_channel( .get_wheel_mode() .await .map_err(|e| WriteError::Hidpp(format!("{e:?}")))?; + // Idempotent: the desired state is "native HID reporting with this invert + // flag". When the wheel already holds it, skip the write — config reloads + // fire on every DPI / SmartShift / binding change, and re-writing the wheel + // mode each time is needless HID++ traffic that can race other writes. + if mode.inverted == inverted && mode.target == WheelEventTarget::Native { + debug!( + index, + inverted, "native scroll inversion already set; skipping" + ); + return Ok(()); + } let written = feature .set_wheel_mode(WheelEventTarget::Native, mode.resolution, inverted) .await diff --git a/crates/openlogi-hid/src/write/smartshift.rs b/crates/openlogi-hid/src/write/smartshift.rs index 4ac41bae..e7b519ab 100644 --- a/crates/openlogi-hid/src/write/smartshift.rs +++ b/crates/openlogi-hid/src/write/smartshift.rs @@ -9,7 +9,6 @@ use hidpp::{ smartshift::{SmartShiftFeature, WheelMode}, smartshift_enhanced::{SmartShiftEnhancedFeature, SmartShiftEnhancedStatusChange}, }, - protocol::v20::{ErrorType, Hidpp20Error}, }; use tracing::debug; @@ -111,67 +110,28 @@ impl SmartShift { } } - /// Write a full desired status. Enhanced (`0x2111`) takes mode + - /// auto-disengage + tunable torque directly. Legacy (`0x2110`) has no - /// tunable-torque function, so that field is ignored; the wheel mode and - /// auto-disengage threshold are written explicitly — never relying on the - /// device treating a `None`/`0` field as "keep current". + /// Write a full desired status — wheel mode plus the auto-disengage + /// threshold and (Enhanced only) tunable torque. + /// + /// Per the `0x2110` / `0x2111` `setRatchetControlMode` spec, `0` is the + /// firmware's "do not change" sentinel for `autoDisengage` and + /// `currentTunableTorque` (real values are `0x01..=0xFF`). So a zero field + /// is sent as "preserve" rather than rejected — this is the only way to + /// write a mode change on a device that reports `tunable_torque == 0` + /// (e.g. one without tunable-torque hardware), which otherwise silently + /// failed the whole write. async fn set_status(&self, status: SmartShiftStatus) -> Result<(), WriteError> { let SmartShiftStatus { mode, auto_disengage, tunable_torque, } = status; - match self { - Self::Enhanced(feature) => { - let auto_disengage = nonzero_smartshift_value(auto_disengage)?; - let tunable_torque = nonzero_smartshift_value(tunable_torque)?; - feature - .set_ratchet_control_mode(SmartShiftEnhancedStatusChange { - wheel_mode: Some(smartshift_to_wheel(mode)), - auto_disengage: Some(auto_disengage), - tunable_torque: Some(tunable_torque), - }) - .await - .map(|_| ()) - .map_err(|e| { - classify_hidpp_error( - e, - HidppOperation::WriteSmartShift, - SmartShiftEnhancedFeature::ID, - ) - }) - } - Self::Legacy(feature) => feature - .set_ratchet_control_mode( - Some(smartshift_to_wheel(mode)), - Some(auto_disengage), - None, - ) - .await - .map_err(|e| { - classify_hidpp_error(e, HidppOperation::WriteSmartShift, SmartShiftFeature::ID) - }), - } - } - - /// Write a new wheel `mode`, preserving the fields read from the device. - /// - /// Enhanced SmartShift uses `0` as the “do not change” sentinel, so a zero - /// readback is preserved by sending `None` rather than rejected as a target - /// value. This is for read-modify-write flows like toggle; explicit user - /// writes still go through [`Self::set_status`] and reject zero targets. - async fn set_mode_preserving_status( - &self, - mode: SmartShiftMode, - current: SmartShiftStatus, - ) -> Result<(), WriteError> { match self { Self::Enhanced(feature) => feature .set_ratchet_control_mode(SmartShiftEnhancedStatusChange { wheel_mode: Some(smartshift_to_wheel(mode)), - auto_disengage: NonZeroU8::new(current.auto_disengage), - tunable_torque: NonZeroU8::new(current.tunable_torque), + auto_disengage: NonZeroU8::new(auto_disengage), + tunable_torque: NonZeroU8::new(tunable_torque), }) .await .map(|_| ()) @@ -182,7 +142,19 @@ impl SmartShift { SmartShiftEnhancedFeature::ID, ) }), - Self::Legacy(_) => self.set_status(SmartShiftStatus { mode, ..current }).await, + // `Some(0)` encodes as `0x00` = "do not change" per the x2110 spec + // and `SmartShiftFeature::set_ratchet_control_mode`, so this matches + // the Enhanced branch's `NonZeroU8::new` preserve-on-zero semantics. + Self::Legacy(feature) => feature + .set_ratchet_control_mode( + Some(smartshift_to_wheel(mode)), + Some(auto_disengage), + None, + ) + .await + .map_err(|e| { + classify_hidpp_error(e, HidppOperation::WriteSmartShift, SmartShiftFeature::ID) + }), } } @@ -222,16 +194,6 @@ impl SmartShift { } } -fn nonzero_smartshift_value(value: u8) -> Result { - NonZeroU8::new(value).ok_or_else(|| { - classify_hidpp_error( - Hidpp20Error::Feature(ErrorType::InvalidArgument), - HidppOperation::WriteSmartShift, - SmartShiftEnhancedFeature::ID, - ) - }) -} - /// Read the device's current SmartShift mode + sensitivity — companion to /// [`toggle_smartshift`]. pub async fn get_smartshift_status(route: &DeviceRoute) -> Result { @@ -300,7 +262,12 @@ pub(super) async fn toggle_smartshift_on_channel( let smartshift = SmartShift::open(&mut device).await?; let status = smartshift.status().await?; let next = status.mode.flipped(); - smartshift.set_mode_preserving_status(next, status).await?; + smartshift + .set_status(SmartShiftStatus { + mode: next, + ..status + }) + .await?; debug!(index, ?next, "wrote SmartShift mode"); Ok(next) }