fix(smartshift): stop runaway free-spin scroll and SmartShift control snap-back

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

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<u8, D::Error>
+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::<SmartShift>(&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();

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);
+    }
 }

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

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, WriteError> {
-    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<SmartShiftStatus, WriteError> {
@@ -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)
 }