From a09c59044c915c8e932481b06f5350185fd20908 Mon Sep 17 00:00:00 2001 From: denevrove Date: Sun, 26 Jul 2026 22:40:10 +0800 Subject: [PATCH 1/4] feat(hid): hold-to-repeat for thumb buttons over HID++ MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Holding a button bound to volume (or scroll) only fired once. Repeating needs the release edge, and the OS hook cannot supply it: the device reports the thumb buttons to the OS as a short press/release pulse — measured 7-22 ms on a Lift regardless of how long the button is physically held, with a multi-second hold emitting no mouse or keyboard event at all. The HID++ 0x1b04 divertedButtonsEvent does carry it: the report holds the complete set of currently-held control IDs, so a CID leaving the set is a release. Diverting is opt-in per button and narrow on purpose. `hold_buttons` selects only buttons whose bound action is repeatable, so a thumb button bound to Back or Forward keeps its native OS-hook path and its crash-safety: a hook remap dies with the process, but a divert outlives it and leaves the button dead to the OS until something restores it. `arm_controls` also checks `is_divertable()` before touching a control rather than assuming the CID exists. The repeat itself accelerates: 400 ms before the second fire, then a gap that shrinks 15% per fire from 220 ms down to a floor of 45 ms (~1.2 s to reach it). A short hold nudges a step or two; a long hold sweeps the range. Timing runs on a worker thread, never in the hook callback, and the repeat state stays off the thread-local HOLD so a synthesized event re-entering the tap cannot double-borrow it. `Action::is_repeatable` is an explicit list, not a `category()` test: `Category::Media` holds both VolumeUp (repeatable) and PlayPause (must not be). Known limitation, pre-existing: the agent has no graceful-shutdown path (takeover uses SIGTERM, IPC has no quit method), so an abnormal exit skips the restore in `ArmedControls::disarm` and leaves a diverted button inactive until the agent runs again — restarting it re-diverts and resumes handling, and a reboot recovers via launch-at-login. Power-cycling the mouse is only needed to get the native OS behavior back with no agent running. BACK_CID / FORWARD_CID were reverse-engineered on one device, not read off a spec, and are marked as such. --- .../openlogi-agent-core/src/hook_runtime.rs | 158 ++++++++++++++++++ .../src/watchers/gesture.rs | 104 +++++++++--- crates/openlogi-core/src/binding.rs | 85 ++++++++++ crates/openlogi-hid/src/gesture.rs | 74 +++++++- crates/openlogi-hid/src/gesture/tests.rs | 119 ++++++++++++- crates/openlogi-hid/src/reprog_controls.rs | 40 +++++ 6 files changed, 549 insertions(+), 31 deletions(-) diff --git a/crates/openlogi-agent-core/src/hook_runtime.rs b/crates/openlogi-agent-core/src/hook_runtime.rs index 27234f1d..689a7c47 100644 --- a/crates/openlogi-agent-core/src/hook_runtime.rs +++ b/crates/openlogi-agent-core/src/hook_runtime.rs @@ -7,7 +7,9 @@ use std::cell::RefCell; use std::collections::BTreeMap; +use std::sync::mpsc::{self, RecvTimeoutError, Sender}; use std::sync::{Arc, RwLock}; +use std::time::Duration; use openlogi_core::binding::{ Action, ButtonId, GestureDirection, SwipeAccumulator, default_binding, @@ -96,6 +98,105 @@ thread_local! { static HOLD: RefCell = RefCell::new(HoldState::default()); } +/// How long a repeatable action's button must stay down before the action +/// starts re-firing. Mirrors a keyboard's typematic delay: long enough that an +/// ordinary click never repeats by accident. +const REPEAT_INITIAL_DELAY: Duration = Duration::from_millis(400); + +/// The gap before the second fire. Deliberately slow: a short hold should nudge +/// the value a step or two, not overshoot it. +const REPEAT_START_INTERVAL: Duration = Duration::from_millis(220); + +/// The floor the gap ramps down to. Fast enough to cross a volume range in one +/// hold, slow enough that the user can still stop on a value. +const REPEAT_MIN_INTERVAL: Duration = Duration::from_millis(45); + +/// The gap shrinks by this fraction (`17/20` = 0.85) after every fire, so a hold +/// accelerates instead of running at one flat rate. Integer maths keeps the ramp +/// exactly reproducible in tests. +const REPEAT_RAMP_NUM: u64 = 17; +/// Denominator of [`REPEAT_RAMP_NUM`]. +const REPEAT_RAMP_DEN: u64 = 20; + +/// The gap to use after the one that just elapsed: 15% shorter, clamped at +/// [`REPEAT_MIN_INTERVAL`]. +/// +/// From [`REPEAT_START_INTERVAL`] this reaches the floor after ~11 fires, about +/// 1.2 s into the hold — gentle enough to land on a single step, quick enough +/// that a long hold sweeps the whole range. +fn ramp(current: Duration) -> Duration { + let next = Duration::from_millis( + u64::try_from(current.as_millis()).unwrap_or(u64::MAX) * REPEAT_RAMP_NUM / REPEAT_RAMP_DEN, + ); + next.max(REPEAT_MIN_INTERVAL) +} + +/// What a capture path tells the repeat worker. +/// +/// Both input paths use this: the OS hook (for buttons it remaps directly) and +/// the HID++ capture session (for buttons diverted to get the release edge). +pub enum RepeatCmd { + /// A repeatable action's button went down — begin the delay-then-repeat + /// cycle. Supersedes any cycle already running. + Start(Action), + /// The button came up, or capture was interrupted — stop re-firing. + Stop, +} + +/// Spawns the worker that re-fires a held button's action, and returns the +/// sender the hook callback signals it with. +/// +/// The delay and the interval are slept here, never in the callback: the +/// callback must not block (see the freeze-hazard note in `macos.rs`). The +/// worker also keeps the repeat state off the thread-local [`HOLD`], so a +/// synthesized event re-entering the tap cannot double-borrow it. +/// +/// [`Sender`] is `Sync` as of Rust 1.72, so the callback can hold it directly +/// without a mutex. +pub fn spawn_repeater( + dpi_cycle: Arc>, + capture: CaptureChannel, +) -> Sender { + let (tx, rx) = mpsc::channel::(); + std::thread::spawn(move || { + // Outer loop: idle until a press arrives. Inner loop: wait out the + // delay, then re-fire on every interval tick until release. + while let Ok(cmd) = rx.recv() { + let mut action = match cmd { + RepeatCmd::Start(action) => action, + RepeatCmd::Stop => continue, + }; + let mut wait = REPEAT_INITIAL_DELAY; + let mut interval = REPEAT_START_INTERVAL; + loop { + match rx.recv_timeout(wait) { + // Nothing interrupted the wait → fire, then wait out the + // current interval and shorten it for the fire after that. + // The first press was already dispatched by the caller, so + // this is strictly the 2nd fire onward. + Err(RecvTimeoutError::Timeout) => { + dispatch_action(&action, &dpi_cycle, &capture); + wait = interval; + interval = ramp(interval); + } + // A fresh press mid-cycle (button swap, or a re-press that + // outran the release): restart the delay *and* the ramp, so + // every hold starts slow again. + Ok(RepeatCmd::Start(next)) => { + action = next; + wait = REPEAT_INITIAL_DELAY; + interval = REPEAT_START_INTERVAL; + } + Ok(RepeatCmd::Stop) => break, + // The agent is shutting down. + Err(RecvTimeoutError::Disconnected) => return, + } + } + } + }); + tx +} + /// Attempt to start the OS hook. Returns `None` if Accessibility is not /// granted or on an unsupported platform — the app continues without crashing. pub fn start( @@ -112,6 +213,9 @@ pub fn start( return None; } + // Hold-to-repeat runs on its own thread so the callback never sleeps. + let repeat = spawn_repeater(Arc::clone(&dpi_cycle), Arc::clone(&capture)); + // The per-hold pointer accumulator lives in the thread-local `HOLD`; the // callback must never block — see the freeze-hazard note in `macos.rs`. let result = Hook::start(move |event| { @@ -181,6 +285,17 @@ pub fn start( if pressed { info!(button = %id, action = %action.label(), "button → executing bound action"); dispatch_action(&action, &dpi_cycle, &capture); + // Increment-style actions keep firing while the button is + // held; the worker owns the timing. A send failure just + // means the worker is gone, which costs only the repeat. + if action.is_repeatable() { + let _ = repeat.send(RepeatCmd::Start(action)); + } + } else { + // Unconditional: a Stop with nothing repeating is a no-op, + // and this way a binding that changed mid-hold still ends + // the cycle its press started. + let _ = repeat.send(RepeatCmd::Stop); } EventDisposition::Suppress } @@ -215,6 +330,9 @@ pub fn start( // The OS dropped events (tap disabled); cancel any hold so a lost // button-up can't later commit a phantom swipe off ordinary motion. HOLD.with_borrow_mut(HoldState::cancel); + // Same reasoning for the repeat cycle: without this, a swallowed + // button-up would leave the action firing forever. + let _ = repeat.send(RepeatCmd::Stop); EventDisposition::PassThrough } MouseEvent::Scroll { .. } => EventDisposition::PassThrough, @@ -319,6 +437,46 @@ mod tests { use super::*; use openlogi_core::binding::GESTURE_SWIPE_THRESHOLD; + #[test] + fn the_repeat_ramp_accelerates_and_then_holds_at_the_floor() { + let mut interval = REPEAT_START_INTERVAL; + for _ in 0..40 { + let next = ramp(interval); + assert!( + next <= interval, + "the gap must never grow: {interval:?} → {next:?}" + ); + assert!(next >= REPEAT_MIN_INTERVAL, "the floor must hold"); + interval = next; + } + assert_eq!( + interval, REPEAT_MIN_INTERVAL, + "a long hold settles at the fastest rate" + ); + } + + #[test] + fn the_repeat_ramp_reaches_the_floor_in_about_a_second() { + // Feel check: too quick and a short hold overshoots, too slow and a long + // hold never gets anywhere. Sum the gaps until the rate stops changing. + let mut interval = REPEAT_START_INTERVAL; + let mut elapsed = Duration::ZERO; + let mut fires = 0; + while interval > REPEAT_MIN_INTERVAL { + elapsed += interval; + interval = ramp(interval); + fires += 1; + } + assert!( + (8..=16).contains(&fires), + "expected ~11 fires to reach the floor, got {fires}" + ); + assert!( + elapsed >= Duration::from_millis(900) && elapsed <= Duration::from_millis(1600), + "expected ~1.2 s of holding to reach the floor, got {elapsed:?}" + ); + } + // The mid-swipe gate itself is unit-tested on `SwipeAccumulator` in // `openlogi-core`; these cover only what `HoldState` adds on top — tagging a // commit with the held button, and matching the button on release. diff --git a/crates/openlogi-agent-core/src/watchers/gesture.rs b/crates/openlogi-agent-core/src/watchers/gesture.rs index 80659682..ad8bee04 100644 --- a/crates/openlogi-agent-core/src/watchers/gesture.rs +++ b/crates/openlogi-agent-core/src/watchers/gesture.rs @@ -20,6 +20,7 @@ use std::collections::BTreeMap; use std::sync::atomic::{AtomicI32, Ordering}; +use std::sync::mpsc::Sender; use std::sync::{Arc, RwLock}; use std::thread; use std::time::{Duration, Instant}; @@ -31,7 +32,7 @@ use tokio::sync::{mpsc, oneshot}; use tracing::{debug, warn}; use crate::DpiCycleState; -use crate::hook_runtime::{self, SharedHookMaps}; +use crate::hook_runtime::{self, RepeatCmd, SharedHookMaps, spawn_repeater}; use crate::receiver_access::ReceiverAccess; /// Shared gesture-direction binding map, mirrored from `AppState` (keyed by @@ -141,6 +142,31 @@ fn should_rearm(done_epoch: u64, live_epoch: u64, has_target: bool) -> bool { done_epoch == live_epoch && has_target } +/// Buttons that need diverting so the capture session can see their release +/// edge: the ones bound to a repeatable action. +/// +/// Deliberately narrow. Diverting a thumb button trades its crash-safety — the +/// OS hook's remap dies with the process, a divert outlives it and leaves the +/// button dead until the mouse is power-cycled — so it is only worth doing for +/// a binding that actually needs the hold duration. +/// +/// Sorted and deduplicated so the result is stable tick to tick and can be +/// compared as part of the session key. +fn hold_buttons(hook_maps: &SharedHookMaps) -> Vec { + let Ok(maps) = hook_maps.read() else { + return Vec::new(); + }; + let mut out: Vec = maps + .bindings + .iter() + .filter(|(_, action)| action.is_repeatable()) + .map(|(button, _)| *button) + .collect(); + out.sort(); + out.dedup(); + out +} + /// Keep one capture session alive for the active device, restarting it when the /// device or the thumb-wheel arming changes, and dispatch incoming inputs. Runs /// for the lifetime of the process. @@ -153,8 +179,11 @@ async fn manage( receiver_access: ReceiverAccess, ) { let (tx, mut rx) = mpsc::unbounded_channel::(); - // (route, capture_thumbwheel, divert_gesture_button) - let mut current: Option<(DeviceRoute, bool, bool)> = None; + // Hold-to-repeat for the buttons this path diverts. Lives for the whole + // manage loop so a session restart mid-hold cannot orphan a running cycle. + let repeat = spawn_repeater(Arc::clone(&dpi_cycle), Arc::clone(&capture_channel)); + // (route, capture_thumbwheel, divert_gesture_button, hold_buttons) + let mut current: Option<(DeviceRoute, bool, bool, Vec)> = None; let mut stop: Option> = None; let mut ticker = tokio::time::interval(TARGET_POLL); let mut accumulators = WheelAccumulators::default(); @@ -175,11 +204,14 @@ async fn manage( dispatch( input, &mut accumulators, - &hook_maps, - &gesture_bindings, - &dpi_cycle, - &capture_channel, - &thumbwheel_sensitivity, + &DispatchCtx { + hook_maps: &hook_maps, + gesture_bindings: &gesture_bindings, + dpi_cycle: &dpi_cycle, + capture: &capture_channel, + thumbwheel_sensitivity: &thumbwheel_sensitivity, + repeat: &repeat, + }, ); } _ = ticker.tick() => { @@ -202,6 +234,7 @@ async fn manage( t, thumbwheel_armed(&hook_maps, sensitivity), divert_gesture, + hold_buttons(&hook_maps), ) }) }; @@ -218,12 +251,17 @@ async fn manage( current = None; continue; } - if let Some((route, capture_thumbwheel, divert_gesture_button)) = want { + if let Some((route, capture_thumbwheel, divert_gesture_button, hold)) = want { let Some(receiver_lease) = receiver_access.try_acquire_for_capture() else { current = None; continue; }; - current = Some((route.clone(), capture_thumbwheel, divert_gesture_button)); + current = Some(( + route.clone(), + capture_thumbwheel, + divert_gesture_button, + hold.clone(), + )); let (stop_tx, stop_rx) = oneshot::channel(); let sink = tx.clone(); let slot = Arc::clone(&capture_channel); @@ -236,6 +274,7 @@ async fn manage( route, capture_thumbwheel, divert_gesture_button, + &hold, sink, stop_rx, slot, @@ -308,15 +347,29 @@ enum WheelOutput { } /// Route one captured input to its bound action (or re-synthesised scroll). -fn dispatch( - input: CapturedInput, - accumulators: &mut WheelAccumulators, - hook_maps: &SharedHookMaps, - gesture_bindings: &GestureBindings, - dpi_cycle: &Arc>, - capture: &CaptureChannel, - thumbwheel_sensitivity: &ThumbwheelSensitivity, -) { +/// The long-lived state [`dispatch`] reads, bundled so the signature stays +/// reviewable as inputs grow. Every field is borrowed for the call — nothing here +/// is owned by the dispatch itself. +#[derive(Clone, Copy)] +struct DispatchCtx<'a> { + hook_maps: &'a SharedHookMaps, + gesture_bindings: &'a GestureBindings, + dpi_cycle: &'a Arc>, + capture: &'a CaptureChannel, + thumbwheel_sensitivity: &'a ThumbwheelSensitivity, + /// Drives hold-to-repeat for the buttons this path diverts. + repeat: &'a Sender, +} + +fn dispatch(input: CapturedInput, accumulators: &mut WheelAccumulators, ctx: &DispatchCtx<'_>) { + let &DispatchCtx { + hook_maps, + gesture_bindings, + dpi_cycle, + capture, + thumbwheel_sensitivity, + repeat, + } = ctx; match input { CapturedInput::Gesture(direction) => { let action = gesture_bindings @@ -338,10 +391,23 @@ fn dispatch( if let Some(action) = action { debug!(?button, action = %action.label(), "HID++ button → action"); hook_runtime::dispatch_action(&action, dpi_cycle, capture); + // Increment-style actions keep firing until the release arrives. + // Only the HID++ path can do this: the OS hook sees these + // buttons as a press/release pulse regardless of hold length. + if action.is_repeatable() { + let _ = repeat.send(RepeatCmd::Start(action)); + } } else { debug!(?button, "HID++ button with no binding — ignored"); } } + CapturedInput::ButtonReleased(button) => { + // Unconditional: a Stop with nothing repeating is a no-op, and this + // way a binding that changed mid-hold still ends the cycle its own + // press started. + debug!(?button, "HID++ button released"); + let _ = repeat.send(RepeatCmd::Stop); + } CapturedInput::Scroll(rotation) => { // Positive rotation is "up"; each direction has its own binding. let up = rotation >= 0; diff --git a/crates/openlogi-core/src/binding.rs b/crates/openlogi-core/src/binding.rs index 97ddea09..26bed91a 100644 --- a/crates/openlogi-core/src/binding.rs +++ b/crates/openlogi-core/src/binding.rs @@ -665,6 +665,31 @@ impl Action { } } + /// Whether holding the bound button should keep re-firing this action. + /// + /// Only actions whose effect is a small increment worth applying many times + /// in a row qualify. Toggles ([`MuteVolume`](Action::MuteVolume), + /// [`PlayPause`](Action::PlayPause)) and one-shot navigation + /// ([`NextTrack`](Action::NextTrack), tab and desktop switches) are excluded + /// on purpose: repeating those on a long press would be surprising rather + /// than useful. + /// + /// Deliberately an explicit list rather than a [`category`](Action::category) + /// test — `Category::Media` holds both `VolumeUp` (repeatable) and + /// `PlayPause` (not), so the category is the wrong granularity. + #[must_use] + pub fn is_repeatable(&self) -> bool { + matches!( + self, + Action::VolumeUp + | Action::VolumeDown + | Action::ScrollUp + | Action::ScrollDown + | Action::HorizontalScrollLeft + | Action::HorizontalScrollRight + ) + } + /// All pickable actions in a deterministic order. /// /// [`Action::CustomShortcut`] is intentionally excluded — it is opened via @@ -1041,6 +1066,66 @@ mod tests { assert_eq!(Action::MuteVolume.category(), Category::Media); } + #[test] + fn repeatable_actions_are_the_increment_style_ones() { + for action in [ + Action::VolumeUp, + Action::VolumeDown, + Action::ScrollUp, + Action::ScrollDown, + Action::HorizontalScrollLeft, + Action::HorizontalScrollRight, + ] { + assert!(action.is_repeatable(), "{action:?} should repeat on hold"); + } + } + + #[test] + fn toggles_and_one_shots_never_repeat() { + // `MuteVolume` and `PlayPause` share `Category::Media` with `VolumeUp`, + // which is exactly why `is_repeatable` cannot be derived from the + // category. + for action in [ + Action::MuteVolume, + Action::PlayPause, + Action::NextTrack, + Action::PrevTrack, + Action::NextTab, + Action::PrevTab, + Action::NextDesktop, + Action::PreviousDesktop, + Action::LeftClick, + Action::Copy, + Action::None, + ] { + assert!( + !action.is_repeatable(), + "{action:?} must not repeat on hold" + ); + } + } + + #[test] + fn every_repeatable_action_is_in_the_catalog() { + // A repeatable action the picker cannot offer would be dead config. + for action in Action::catalog() { + if action.is_repeatable() { + assert!( + matches!( + action, + Action::VolumeUp + | Action::VolumeDown + | Action::ScrollUp + | Action::ScrollDown + | Action::HorizontalScrollLeft + | Action::HorizontalScrollRight + ), + "unexpected repeatable action in catalog: {action:?}" + ); + } + } + } + #[test] fn category_mouse_variants() { assert_eq!(Action::LeftClick.category(), Category::Mouse); diff --git a/crates/openlogi-hid/src/gesture.rs b/crates/openlogi-hid/src/gesture.rs index 1783494b..d1d5913f 100644 --- a/crates/openlogi-hid/src/gesture.rs +++ b/crates/openlogi-hid/src/gesture.rs @@ -40,9 +40,17 @@ pub enum CapturedInput { /// A completed gesture-button swipe. Gesture(GestureDirection), /// A diverted button was pressed — the DPI/ModeShift button - /// ([`ButtonId::DpiToggle`]) or the thumb-wheel single tap - /// ([`ButtonId::Thumbwheel`]). + /// ([`ButtonId::DpiToggle`]), the thumb-wheel single tap + /// ([`ButtonId::Thumbwheel`]), or a thumb-side button diverted for hold + /// tracking (see [`reprog_controls::hold_cid_for_button`]). ButtonPressed(ButtonId), + /// A button diverted for hold tracking was released. + /// + /// Emitted only for the buttons named in `hold_buttons`; the DPI/ModeShift + /// and thumb-wheel captures are rising-edge only and never produce this. + /// Pairs with [`CapturedInput::ButtonPressed`] so a consumer can measure how + /// long the button was held — the whole reason those buttons get diverted. + ButtonReleased(ButtonId), /// Thumb-wheel rotation to re-synthesise as horizontal scroll, in the /// wheel's `diverted_res` increments. Emitted only while the wheel is /// diverted to capture its click. @@ -75,6 +83,9 @@ struct CaptureAccum { /// Whether any DPI/ModeShift control was held in the last event — for /// rising-edge press detection. dpi_down: bool, + /// Which hold-tracked CIDs were held in the last event. Both edges matter + /// here (unlike `dpi_down`), so the consumer can time the hold. + held: Vec, } /// Capture the gesture button, DPI/ModeShift button, and (when @@ -96,6 +107,7 @@ pub async fn run_capture_session( route: DeviceRoute, capture_thumbwheel: bool, divert_gesture_button: bool, + hold_buttons: &[ButtonId], sink: mpsc::UnboundedSender, shutdown: oneshot::Receiver<()>, channel_slot: CaptureChannel, @@ -109,6 +121,7 @@ pub async fn run_capture_session( device_index, capture_thumbwheel, divert_gesture_button, + hold_buttons, ) .await?; @@ -122,6 +135,7 @@ pub async fn run_capture_session( let reprog_index = armed.reprog.as_ref().map(|(_, idx)| *idx); let thumb_index = armed.thumb.as_ref().map(|(_, idx)| *idx); let dpi_set = armed.dpi_cids.clone(); + let hold_set = armed.hold.clone(); let listener = chan.add_msg_listener_guarded({ let accum = Arc::clone(&accum); let sink = sink.clone(); @@ -136,7 +150,7 @@ pub async fn run_capture_session( // Recover the guard even if a prior holder panicked — the // critical section is panic-free, so the data is consistent. let mut acc = accum.lock().unwrap_or_else(PoisonError::into_inner); - handle_reprog(&mut acc, event, &dpi_set, &sink); + handle_reprog(&mut acc, event, &dpi_set, &hold_set, &sink); return; } if let Some(idx) = thumb_index @@ -179,6 +193,8 @@ struct ArmedControls { gesture_diverted: bool, /// DPI/ModeShift CIDs diverted as plain buttons. dpi_cids: Vec, + /// CIDs diverted for hold tracking, paired with the button they represent. + hold: Vec<(u16, ButtonId)>, /// `0x2150` accessor + feature index, present when the thumb wheel is /// diverted. thumb: Option<(Thumbwheel, u8)>, @@ -197,6 +213,15 @@ impl ArmedControls { for &cid in &self.dpi_cids { restore(rc.set_cid_reporting(cid, false, false).await, "DPI button"); } + // Undiverting these matters more than the rest: they are the user's + // ordinary thumb buttons, and leaving them diverted makes them dead + // to the OS until the mouse is power-cycled. + for &(cid, _) in &self.hold { + restore( + rc.set_cid_reporting(cid, false, false).await, + "hold-tracked button", + ); + } } if let Some((tw, _)) = self.thumb.as_ref() { restore(tw.set_reporting(false, false).await, "thumb wheel"); @@ -214,6 +239,7 @@ async fn arm_controls( slot: u8, capture_thumbwheel: bool, divert_gesture_button: bool, + hold_buttons: &[ButtonId], ) -> Result { let device = Device::new(Arc::clone(chan), slot) .await @@ -222,6 +248,7 @@ async fn arm_controls( let mut reprog: Option<(ReprogControlsV4, u8)> = None; let mut gesture_diverted = false; let mut dpi_cids: Vec = Vec::new(); + let mut hold: Vec<(u16, ButtonId)> = Vec::new(); if let Some(info) = device .root() .get_feature(reprog_controls::FEATURE_ID) @@ -251,6 +278,29 @@ async fn arm_controls( dpi_cids.push(cid); } } + // Thumb-side buttons, diverted only when a caller asked for hold + // tracking. A button the firmware does not mark divertable is skipped + // rather than forced: capturing a control the device won't hand over + // would swallow the press with nothing to show for it. + for &button in hold_buttons { + let Some(cid) = reprog_controls::hold_cid_for_button(button) else { + continue; + }; + if hold.iter().any(|(existing, _)| *existing == cid) { + continue; + } + if controls.iter().any(|c| c.cid == cid && c.is_divertable()) { + rc.set_cid_reporting(cid, true, false) + .await + .map_err(|e| GestureError::Hidpp(format!("{e:?}")))?; + hold.push((cid, button)); + } else { + debug!( + ?button, + cid, "button not divertable — hold tracking skipped" + ); + } + } reprog = Some((rc, info.index)); } @@ -290,6 +340,7 @@ async fn arm_controls( reprog, gesture_diverted, dpi_cids, + hold, thumb, }) } @@ -329,6 +380,7 @@ fn handle_reprog( acc: &mut CaptureAccum, event: RawControlEvent, dpi_cids: &[u16], + hold: &[(u16, ButtonId)], sink: &mpsc::UnboundedSender, ) { match event { @@ -349,6 +401,22 @@ fn handle_reprog( let _ = sink.send(CapturedInput::ButtonPressed(ButtonId::DpiToggle)); } acc.dpi_down = dpi_down; + + // Hold-tracked buttons report both edges. The event carries the + // *complete* set of controls currently held, so a CID that was in + // the last set and is missing now is a release — the device sends + // one report per change, not one per poll. + for &(cid, button) in hold { + let now = cids.contains(&cid); + let was = acc.held.contains(&cid); + if now && !was { + acc.held.push(cid); + let _ = sink.send(CapturedInput::ButtonPressed(button)); + } else if !now && was { + acc.held.retain(|held| *held != cid); + let _ = sink.send(CapturedInput::ButtonReleased(button)); + } + } } RawControlEvent::RawXy { dx, dy } => { // Commit the instant a clean direction emerges (mid-swipe, once per diff --git a/crates/openlogi-hid/src/gesture/tests.rs b/crates/openlogi-hid/src/gesture/tests.rs index 035f2cda..ba74b9b8 100644 --- a/crates/openlogi-hid/src/gesture/tests.rs +++ b/crates/openlogi-hid/src/gesture/tests.rs @@ -13,14 +13,15 @@ fn quick_tap_is_a_click_even_while_the_cursor_moves() { let (tx, mut rx) = mpsc::unbounded_channel(); let mut acc = CaptureAccum::default(); - handle_reprog(&mut acc, press(), &[], &tx); + handle_reprog(&mut acc, press(), &[], &[], &tx); handle_reprog( &mut acc, RawControlEvent::RawXy { dx: 120, dy: 5 }, &[], + &[], &tx, ); - handle_reprog(&mut acc, release(), &[], &tx); + handle_reprog(&mut acc, release(), &[], &[], &tx); assert_eq!( rx.try_recv(), @@ -37,13 +38,14 @@ fn a_held_gesture_commits_a_swipe_and_does_not_also_click() { let (tx, mut rx) = mpsc::unbounded_channel(); let mut acc = CaptureAccum::default(); - handle_reprog(&mut acc, press(), &[], &tx); + handle_reprog(&mut acc, press(), &[], &[], &tx); // Pretend the button has been held well past the swipe gate. acc.swipe.backdate_hold_for_test(); handle_reprog( &mut acc, RawControlEvent::RawXy { dx: 120, dy: 5 }, &[], + &[], &tx, ); @@ -52,7 +54,7 @@ fn a_held_gesture_commits_a_swipe_and_does_not_also_click() { Ok(CapturedInput::Gesture(GestureDirection::Right)) ); - handle_reprog(&mut acc, release(), &[], &tx); + handle_reprog(&mut acc, release(), &[], &[], &tx); assert!( rx.try_recv().is_err(), "a committed swipe must not also click on release" @@ -66,8 +68,8 @@ fn a_held_dpi_button_presses_once_on_the_rising_edge() { let dpi = reprog_controls::DPI_MODE_SHIFT_CIDS[0]; let down = RawControlEvent::DivertedButtons([dpi, 0, 0, 0]); - handle_reprog(&mut acc, down, &[dpi], &tx); - handle_reprog(&mut acc, down, &[dpi], &tx); + handle_reprog(&mut acc, down, &[dpi], &[], &tx); + handle_reprog(&mut acc, down, &[dpi], &[], &tx); assert_eq!( rx.try_recv(), @@ -76,6 +78,105 @@ fn a_held_dpi_button_presses_once_on_the_rising_edge() { assert!(rx.try_recv().is_err(), "a held DPI button presses once"); } +#[test] +fn a_hold_tracked_button_emits_both_edges_exactly_once() { + // The whole point of diverting these: a press and a release the consumer can + // time. Repeat frames while the button stays down must not re-emit — the + // device sends one report per change, but a resend must stay harmless. + let (tx, mut rx) = mpsc::unbounded_channel(); + let mut acc = CaptureAccum::default(); + let cid = reprog_controls::FORWARD_CID; + let hold = [(cid, ButtonId::Forward)]; + let down = RawControlEvent::DivertedButtons([cid, 0, 0, 0]); + let up = RawControlEvent::DivertedButtons([0, 0, 0, 0]); + + handle_reprog(&mut acc, down, &[], &hold, &tx); + handle_reprog(&mut acc, down, &[], &hold, &tx); + handle_reprog(&mut acc, up, &[], &hold, &tx); + handle_reprog(&mut acc, up, &[], &hold, &tx); + + assert_eq!( + rx.try_recv(), + Ok(CapturedInput::ButtonPressed(ButtonId::Forward)) + ); + assert_eq!( + rx.try_recv(), + Ok(CapturedInput::ButtonReleased(ButtonId::Forward)) + ); + assert!( + rx.try_recv().is_err(), + "each edge fires once, however many frames repeat it" + ); +} + +#[test] +fn hold_tracking_keeps_the_two_thumb_buttons_independent() { + // Both diverted at once: one going down must not mask the other's edges, + // and the frame that holds both is a single press for each. + let (tx, mut rx) = mpsc::unbounded_channel(); + let mut acc = CaptureAccum::default(); + let back = reprog_controls::BACK_CID; + let fwd = reprog_controls::FORWARD_CID; + let hold = [(back, ButtonId::Back), (fwd, ButtonId::Forward)]; + + handle_reprog( + &mut acc, + RawControlEvent::DivertedButtons([back, 0, 0, 0]), + &[], + &hold, + &tx, + ); + handle_reprog( + &mut acc, + RawControlEvent::DivertedButtons([back, fwd, 0, 0]), + &[], + &hold, + &tx, + ); + handle_reprog( + &mut acc, + RawControlEvent::DivertedButtons([fwd, 0, 0, 0]), + &[], + &hold, + &tx, + ); + + assert_eq!( + rx.try_recv(), + Ok(CapturedInput::ButtonPressed(ButtonId::Back)) + ); + assert_eq!( + rx.try_recv(), + Ok(CapturedInput::ButtonPressed(ButtonId::Forward)) + ); + assert_eq!( + rx.try_recv(), + Ok(CapturedInput::ButtonReleased(ButtonId::Back)), + "back leaving the held set is its release, with forward still down" + ); + assert!(rx.try_recv().is_err(), "forward is still held"); +} + +#[test] +fn an_undiverted_button_emits_nothing_even_if_the_device_reports_it() { + // Nothing is opted in, so a frame naming the thumb button must stay silent — + // otherwise a stray report would fire an action the user never bound to this + // path. + let (tx, mut rx) = mpsc::unbounded_channel(); + let mut acc = CaptureAccum::default(); + let cid = reprog_controls::FORWARD_CID; + + handle_reprog( + &mut acc, + RawControlEvent::DivertedButtons([cid, 0, 0, 0]), + &[], + &[], + &tx, + ); + + assert!(rx.try_recv().is_err(), "hold tracking is opt-in per button"); +} + #[test] fn a_dpi_button_re_presses_after_a_release() { // Rising-edge detection must re-arm: press → release → press is two @@ -87,9 +188,9 @@ fn a_dpi_button_re_presses_after_a_release() { let down = RawControlEvent::DivertedButtons([dpi, 0, 0, 0]); let up = RawControlEvent::DivertedButtons([0, 0, 0, 0]); - handle_reprog(&mut acc, down, &[dpi], &tx); - handle_reprog(&mut acc, up, &[dpi], &tx); - handle_reprog(&mut acc, down, &[dpi], &tx); + handle_reprog(&mut acc, down, &[dpi], &[], &tx); + handle_reprog(&mut acc, up, &[dpi], &[], &tx); + handle_reprog(&mut acc, down, &[dpi], &[], &tx); assert_eq!( rx.try_recv(), diff --git a/crates/openlogi-hid/src/reprog_controls.rs b/crates/openlogi-hid/src/reprog_controls.rs index c25d4e3b..be6f96ff 100644 --- a/crates/openlogi-hid/src/reprog_controls.rs +++ b/crates/openlogi-hid/src/reprog_controls.rs @@ -51,6 +51,46 @@ pub const GESTURE_BUTTON_CID: u16 = 0x00c3; /// cross-checked against Solaar `special_keys.py`. pub const DPI_MODE_SHIFT_CIDS: [u16; 3] = [0x00c4, 0x00ed, 0x00fd]; +/// Control IDs for the thumb-side buttons the OS hook normally remaps — +/// [`ButtonId::Back`](openlogi_core::binding::ButtonId::Back) and +/// [`ButtonId::Forward`](openlogi_core::binding::ButtonId::Forward). +/// +/// These are **not** diverted by default. The OS hook handles them natively, +/// which keeps them working even if the agent dies mid-session — a diverted +/// control stays diverted in the device until something restores it, so an +/// abnormal exit would leave the buttons dead until the mouse is power-cycled. +/// +/// They are diverted only when a bound action needs the release edge, which the +/// OS hook cannot supply: the device reports these buttons to the OS as a short +/// press/release pulse (measured 7–22 ms on a Lift) no matter how long the +/// button is physically held, so hold-to-repeat is impossible from that path. +/// See `gesture::arm_controls`. +/// +/// **Reverse-engineered, not read off a spec.** Both values were identified on a +/// single Logitech Lift (BLE, PID `b031`) by correlating `0x1b04` +/// divertedButtonsEvent reports against OS-level `WM_XBUTTONDOWN` timestamps: +/// `0x0056` fired with button 5 / `Forward`, `0x0053` with button 4 / `Back`, +/// and `getCtrlIdInfo` reports both as divertable. Not yet confirmed against the +/// official control-ID list or a second model — `arm_controls` therefore checks +/// `is_divertable()` before using them instead of assuming they exist. +pub const BACK_CID: u16 = 0x0053; +/// See [`BACK_CID`]. +pub const FORWARD_CID: u16 = 0x0056; + +/// The `0x1b04` control ID for `button`, when OpenLogi knows how to divert it +/// for hold tracking. `None` for buttons that have no divertable control (the +/// primary/secondary clicks) or that are already handled by their own capture +/// path (the gesture button, DPI/ModeShift, thumb wheel). +#[must_use] +pub fn hold_cid_for_button(button: openlogi_core::binding::ButtonId) -> Option { + use openlogi_core::binding::ButtonId; + match button { + ButtonId::Back => Some(BACK_CID), + ButtonId::Forward => Some(FORWARD_CID), + _ => None, + } +} + /// Identity and capabilities of one reprogrammable control, as returned by /// `getCtrlIdInfo`. #[derive(Debug, Clone, Copy, PartialEq, Eq)] From 310528f59bd61c9076f30b3180ff3096183e1d84 Mon Sep 17 00:00:00 2001 From: denevrove Date: Mon, 27 Jul 2026 21:48:55 +0800 Subject: [PATCH 2/4] fix(agent): scope repeat cycles per button and end them with their source MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review found four ways a repeat could outlive the hold that started it. `RepeatCmd` now names a button in every variant. One worker still owns the timing, but it keeps a cycle per button, each with its own due time and its own ramp, so two thumb buttons held at once no longer share a single unkeyed cycle that either release could cancel. `RepeatCycles` is split from the worker thread so the scheduling is testable without dispatching real actions: the worker only sleeps and dispatches, `RepeatCycles` decides what is due and when to wake. A cycle also needs a guaranteed release edge, and only the diverted thumb-side buttons have one — the DPI/ModeShift and thumb-wheel captures are rising-edge only. `reports_hold_edges` gates both the divert set and the repeat start on having a hold CID, so binding a repeatable action to DpiToggle no longer starts something nothing can stop. When capture goes away, no release is coming for anything still held, so the new `StopAll` ends every cycle. It goes out on the deliberate teardown (target or bindings changed, pairing wants the receiver) via `stop_session`, on an unexpected session death before re-arming, and on the hook path when the OS disables the tap. Arming is several independent HID++ writes and any one can fail. `arm_controls` now builds an `ArmedControls` in place and hands it to `disarm` on failure, instead of returning early and leaving an already-diverted thumb button captured with no session alive to release it — which for the user meant a dead button until the mouse was power-cycled. --- .../openlogi-agent-core/src/hook_runtime.rs | 285 +++++++++++++++--- .../src/watchers/gesture.rs | 104 ++++++- crates/openlogi-hid/src/gesture.rs | 81 +++-- 3 files changed, 395 insertions(+), 75 deletions(-) diff --git a/crates/openlogi-agent-core/src/hook_runtime.rs b/crates/openlogi-agent-core/src/hook_runtime.rs index 689a7c47..301961d1 100644 --- a/crates/openlogi-agent-core/src/hook_runtime.rs +++ b/crates/openlogi-agent-core/src/hook_runtime.rs @@ -9,7 +9,7 @@ use std::cell::RefCell; use std::collections::BTreeMap; use std::sync::mpsc::{self, RecvTimeoutError, Sender}; use std::sync::{Arc, RwLock}; -use std::time::Duration; +use std::time::{Duration, Instant}; use openlogi_core::binding::{ Action, ButtonId, GestureDirection, SwipeAccumulator, default_binding, @@ -135,12 +135,89 @@ fn ramp(current: Duration) -> Duration { /// /// Both input paths use this: the OS hook (for buttons it remaps directly) and /// the HID++ capture session (for buttons diverted to get the release edge). +/// +/// Every variant names the button it concerns. Two thumb buttons can be held at +/// once, so an unkeyed stop would let releasing one end the other's cycle while +/// it is still physically down. pub enum RepeatCmd { /// A repeatable action's button went down — begin the delay-then-repeat - /// cycle. Supersedes any cycle already running. - Start(Action), - /// The button came up, or capture was interrupted — stop re-firing. - Stop, + /// cycle for that button, superseding its own previous cycle only. + Start(ButtonId, Action), + /// The button came up — stop re-firing that button. + Stop(ButtonId), + /// Capture was interrupted, so no release edge is coming for anything still + /// held: end every cycle rather than leave an action firing with nothing left + /// to stop it. + StopAll, +} + +/// One button's in-flight repeat cycle. +struct RepeatCycle { + /// The action to re-fire, resolved when the press arrived. + action: Action, + /// When this button's next re-fire is due. + due: Instant, + /// The gap to apply *after* the fire that is currently pending. + interval: Duration, +} + +/// Every hold currently repeating, keyed by button. +/// +/// Split out from the worker thread so the scheduling is testable without +/// dispatching real actions: the worker only sleeps and dispatches, this decides +/// what is due and when to wake. +#[derive(Default)] +struct RepeatCycles { + cycles: BTreeMap, +} + +impl RepeatCycles { + /// Begin (or restart) one button's cycle. A restart resets the delay *and* + /// the ramp, so every hold starts slow again. + fn start(&mut self, button: ButtonId, action: Action, now: Instant) { + self.cycles.insert( + button, + RepeatCycle { + action, + due: now + REPEAT_INITIAL_DELAY, + interval: REPEAT_START_INTERVAL, + }, + ); + } + + /// End one button's cycle, leaving any other held button running. A stop for + /// a button that is not repeating is a no-op, which is what lets the callers + /// send it unconditionally on release. + fn stop(&mut self, button: ButtonId) { + self.cycles.remove(&button); + } + + /// End every cycle — see [`RepeatCmd::StopAll`]. + fn stop_all(&mut self) { + self.cycles.clear(); + } + + /// How long the worker may sleep before the soonest re-fire, or `None` when + /// nothing is held (park until a press arrives). + fn next_wait(&self, now: Instant) -> Option { + self.cycles + .values() + .map(|cycle| cycle.due) + .min() + .map(|due| due.saturating_duration_since(now)) + } + + /// Take every action whose gap has elapsed, advancing each ramp. Buttons ramp + /// independently: one held longer is already firing faster. + fn take_due(&mut self, now: Instant) -> Vec { + let mut due = Vec::new(); + for cycle in self.cycles.values_mut().filter(|cycle| cycle.due <= now) { + due.push(cycle.action.clone()); + cycle.due = now + cycle.interval; + cycle.interval = ramp(cycle.interval); + } + due + } } /// Spawns the worker that re-fires a held button's action, and returns the @@ -159,37 +236,35 @@ pub fn spawn_repeater( ) -> Sender { let (tx, rx) = mpsc::channel::(); std::thread::spawn(move || { - // Outer loop: idle until a press arrives. Inner loop: wait out the - // delay, then re-fire on every interval tick until release. - while let Ok(cmd) = rx.recv() { - let mut action = match cmd { - RepeatCmd::Start(action) => action, - RepeatCmd::Stop => continue, + let mut cycles = RepeatCycles::default(); + loop { + // Park until a press arrives when nothing is held; otherwise wake on + // whichever comes first, the next command or the soonest re-fire. + let cmd = match cycles.next_wait(Instant::now()) { + Some(wait) => match rx.recv_timeout(wait) { + Ok(cmd) => Some(cmd), + Err(RecvTimeoutError::Timeout) => None, + // The agent is shutting down. + Err(RecvTimeoutError::Disconnected) => return, + }, + None => match rx.recv() { + Ok(cmd) => Some(cmd), + Err(_) => return, + }, }; - let mut wait = REPEAT_INITIAL_DELAY; - let mut interval = REPEAT_START_INTERVAL; - loop { - match rx.recv_timeout(wait) { - // Nothing interrupted the wait → fire, then wait out the - // current interval and shorten it for the fire after that. - // The first press was already dispatched by the caller, so - // this is strictly the 2nd fire onward. - Err(RecvTimeoutError::Timeout) => { + match cmd { + Some(RepeatCmd::Start(button, action)) => { + cycles.start(button, action, Instant::now()); + } + Some(RepeatCmd::Stop(button)) => cycles.stop(button), + Some(RepeatCmd::StopAll) => cycles.stop_all(), + // A gap elapsed: fire everything now due. The first press was + // already dispatched by the caller, so this is strictly the 2nd + // fire onward. + None => { + for action in cycles.take_due(Instant::now()) { dispatch_action(&action, &dpi_cycle, &capture); - wait = interval; - interval = ramp(interval); } - // A fresh press mid-cycle (button swap, or a re-press that - // outran the release): restart the delay *and* the ramp, so - // every hold starts slow again. - Ok(RepeatCmd::Start(next)) => { - action = next; - wait = REPEAT_INITIAL_DELAY; - interval = REPEAT_START_INTERVAL; - } - Ok(RepeatCmd::Stop) => break, - // The agent is shutting down. - Err(RecvTimeoutError::Disconnected) => return, } } } @@ -289,13 +364,14 @@ pub fn start( // held; the worker owns the timing. A send failure just // means the worker is gone, which costs only the repeat. if action.is_repeatable() { - let _ = repeat.send(RepeatCmd::Start(action)); + let _ = repeat.send(RepeatCmd::Start(id, action)); } } else { - // Unconditional: a Stop with nothing repeating is a no-op, - // and this way a binding that changed mid-hold still ends - // the cycle its press started. - let _ = repeat.send(RepeatCmd::Stop); + // Unconditional, but keyed to this button: a stop for a + // button that is not repeating is a no-op, so a binding that + // changed mid-hold still ends the cycle its own press + // started — without touching another button's hold. + let _ = repeat.send(RepeatCmd::Stop(id)); } EventDisposition::Suppress } @@ -330,9 +406,9 @@ pub fn start( // The OS dropped events (tap disabled); cancel any hold so a lost // button-up can't later commit a phantom swipe off ordinary motion. HOLD.with_borrow_mut(HoldState::cancel); - // Same reasoning for the repeat cycle: without this, a swallowed + // Same reasoning for the repeat cycles: without this, a swallowed // button-up would leave the action firing forever. - let _ = repeat.send(RepeatCmd::Stop); + let _ = repeat.send(RepeatCmd::StopAll); EventDisposition::PassThrough } MouseEvent::Scroll { .. } => EventDisposition::PassThrough, @@ -437,6 +513,135 @@ mod tests { use super::*; use openlogi_core::binding::GESTURE_SWIPE_THRESHOLD; + /// The gap between fires is long enough that a test can step time by hand: + /// jump past the initial delay, then past one interval, without sleeping. + fn past(now: Instant, gap: Duration) -> Instant { + now + gap + Duration::from_millis(1) + } + + #[test] + fn a_release_stops_only_the_button_that_was_released() { + // Two thumb buttons held at once is the case an unkeyed stop got wrong: + // letting go of one killed the other's repeat while it was still down. + let start = Instant::now(); + let mut cycles = RepeatCycles::default(); + cycles.start(ButtonId::Back, Action::VolumeDown, start); + cycles.start(ButtonId::Forward, Action::VolumeUp, start); + cycles.stop(ButtonId::Back); + + let due = cycles.take_due(past(start, REPEAT_INITIAL_DELAY)); + assert_eq!( + due, + vec![Action::VolumeUp], + "the button still held must keep repeating on its own" + ); + } + + #[test] + fn an_unrelated_buttons_release_leaves_the_hold_running() { + // The OS hook sends a keyed stop on *every* bound button's release, so a + // click on some other button must not disturb a hold in progress. + let start = Instant::now(); + let mut cycles = RepeatCycles::default(); + cycles.start(ButtonId::Back, Action::VolumeDown, start); + cycles.stop(ButtonId::MiddleClick); + + assert_eq!( + cycles.take_due(past(start, REPEAT_INITIAL_DELAY)), + vec![Action::VolumeDown] + ); + } + + #[test] + fn stop_all_ends_every_hold() { + // What a capture session sends as it goes away: no release edge is + // coming for anything still held, so nothing may stay armed. + let start = Instant::now(); + let mut cycles = RepeatCycles::default(); + cycles.start(ButtonId::Back, Action::VolumeDown, start); + cycles.start(ButtonId::Forward, Action::VolumeUp, start); + cycles.stop_all(); + + assert!( + cycles + .take_due(past(start, REPEAT_INITIAL_DELAY)) + .is_empty(), + "an interrupted capture must not leave an action firing" + ); + assert_eq!( + cycles.next_wait(start), + None, + "with nothing held the worker parks instead of spinning" + ); + } + + #[test] + fn nothing_fires_before_the_initial_delay_elapses() { + let start = Instant::now(); + let mut cycles = RepeatCycles::default(); + cycles.start(ButtonId::Back, Action::VolumeDown, start); + + // 90% of the way into the delay — a firmly ordinary click. + let just_short = REPEAT_INITIAL_DELAY.mul_f32(0.9); + assert!( + cycles.take_due(start + just_short).is_empty(), + "an ordinary click must never repeat" + ); + assert_eq!( + cycles.next_wait(start), + Some(REPEAT_INITIAL_DELAY), + "the worker sleeps exactly until the first re-fire is due" + ); + } + + #[test] + fn each_button_ramps_on_its_own_clock() { + // Independent ramps: the button held longer is already firing faster, and + // one button's fire must not reschedule the other's. + let start = Instant::now(); + let mut cycles = RepeatCycles::default(); + cycles.start(ButtonId::Back, Action::VolumeDown, start); + + // Back has been repeating for a while before Forward is even pressed. + let mut now = past(start, REPEAT_INITIAL_DELAY); + for _ in 0..6 { + assert_eq!(cycles.take_due(now), vec![Action::VolumeDown]); + now = past(now, REPEAT_START_INTERVAL); + } + cycles.start(ButtonId::Forward, Action::VolumeUp, now); + + // Forward serves out its own full initial delay, during which Back keeps + // firing alone. + assert_eq!(cycles.take_due(now), vec![Action::VolumeDown]); + assert_eq!( + cycles.take_due(past(now, REPEAT_INITIAL_DELAY)).len(), + 2, + "once its delay is up, both buttons fire" + ); + } + + #[test] + fn a_re_press_restarts_that_buttons_delay_and_ramp() { + let start = Instant::now(); + let mut cycles = RepeatCycles::default(); + cycles.start(ButtonId::Back, Action::VolumeDown, start); + let mut now = past(start, REPEAT_INITIAL_DELAY); + assert_eq!(cycles.take_due(now), vec![Action::VolumeDown]); + + // A fresh press (binding swap, or a re-press that outran the release). + now += Duration::from_millis(10); + cycles.start(ButtonId::Back, Action::VolumeUp, now); + assert!( + cycles.take_due(now + REPEAT_START_INTERVAL).is_empty(), + "the delay starts over, so the hold ramps up slowly again" + ); + assert_eq!( + cycles.take_due(past(now, REPEAT_INITIAL_DELAY)), + vec![Action::VolumeUp], + "and it fires the newly bound action" + ); + } + #[test] fn the_repeat_ramp_accelerates_and_then_holds_at_the_floor() { let mut interval = REPEAT_START_INTERVAL; diff --git a/crates/openlogi-agent-core/src/watchers/gesture.rs b/crates/openlogi-agent-core/src/watchers/gesture.rs index ad8bee04..5b32fc26 100644 --- a/crates/openlogi-agent-core/src/watchers/gesture.rs +++ b/crates/openlogi-agent-core/src/watchers/gesture.rs @@ -27,7 +27,9 @@ use std::time::{Duration, Instant}; use openlogi_core::binding::{Action, ButtonId, GestureDirection, default_binding}; use openlogi_core::config::DEFAULT_THUMBWHEEL_SENSITIVITY; -use openlogi_hid::{CaptureChannel, CapturedInput, DeviceRoute, run_capture_session}; +use openlogi_hid::{ + CaptureChannel, CapturedInput, DeviceRoute, reprog_controls, run_capture_session, +}; use tokio::sync::{mpsc, oneshot}; use tracing::{debug, warn}; @@ -142,13 +144,25 @@ fn should_rearm(done_epoch: u64, live_epoch: u64, has_target: bool) -> bool { done_epoch == live_epoch && has_target } +/// Whether a captured button reports both edges, so a hold started on its press +/// is guaranteed a release to end it. +/// +/// Only the thumb-side buttons this path diverts for hold tracking do. The +/// DPI/ModeShift and thumb-wheel captures are rising-edge only, so starting a +/// repeat on their press would leave it firing with no release to stop it. +fn reports_hold_edges(button: ButtonId) -> bool { + reprog_controls::hold_cid_for_button(button).is_some() +} + /// Buttons that need diverting so the capture session can see their release /// edge: the ones bound to a repeatable action. /// /// Deliberately narrow. Diverting a thumb button trades its crash-safety — the /// OS hook's remap dies with the process, a divert outlives it and leaves the /// button dead until the mouse is power-cycled — so it is only worth doing for -/// a binding that actually needs the hold duration. +/// a binding that actually needs the hold duration. Buttons with no hold CID are +/// dropped here too: they cannot be hold-tracked, so listing them would only +/// churn the session key. /// /// Sorted and deduplicated so the result is stable tick to tick and can be /// compared as part of the session key. @@ -159,7 +173,7 @@ fn hold_buttons(hook_maps: &SharedHookMaps) -> Vec { let mut out: Vec = maps .bindings .iter() - .filter(|(_, action)| action.is_repeatable()) + .filter(|(button, action)| action.is_repeatable() && reports_hold_edges(**button)) .map(|(button, _)| *button) .collect(); out.sort(); @@ -167,6 +181,19 @@ fn hold_buttons(hook_maps: &SharedHookMaps) -> Vec { out } +/// Tear the live capture session down, if there is one. +/// +/// Signalling the oneshot lets the session restore the controls it diverted; the +/// repeat stop goes with it because that session is the only source of release +/// edges — anything held right now would never see its release, and the action +/// would keep firing with nothing left to stop it. +fn stop_session(stop: &mut Option>, repeat: &Sender) { + if let Some(stop) = stop.take() { + let _ = stop.send(()); + let _ = repeat.send(RepeatCmd::StopAll); + } +} + /// Keep one capture session alive for the active device, restarting it when the /// device or the thumb-wheel arming changes, and dispatch incoming inputs. Runs /// for the lifetime of the process. @@ -242,11 +269,8 @@ async fn manage( continue; } // Target or thumb-wheel arming changed (or first tick): stop the - // old session and start one for the new state. Sending on the - // oneshot lets the old session restore the diverted controls. - if let Some(stop) = stop.take() { - let _ = stop.send(()); - } + // old session and start one for the new state. + stop_session(&mut stop, &repeat); if current.is_some() { current = None; continue; @@ -301,6 +325,10 @@ async fn manage( // epoch or a deliberate stop-to-idle is a no-op (see `should_rearm`). if should_rearm(done_epoch, epoch, current.is_some()) { warn!("capture session for the active device ended unexpectedly, re-arming"); + // A session that died mid-hold (device disconnect, HID++ + // error) takes the release edge with it — same reasoning as + // the deliberate stop above. + let _ = repeat.send(RepeatCmd::StopAll); current = None; // Keep the `stop`/`current` invariant: the session already // exited, so its stop receiver is gone and dropping the sender @@ -394,19 +422,22 @@ fn dispatch(input: CapturedInput, accumulators: &mut WheelAccumulators, ctx: &Di // Increment-style actions keep firing until the release arrives. // Only the HID++ path can do this: the OS hook sees these // buttons as a press/release pulse regardless of hold length. - if action.is_repeatable() { - let _ = repeat.send(RepeatCmd::Start(action)); + // Gated on the release edge existing — a rising-edge-only + // capture would start a cycle nothing could stop. + if action.is_repeatable() && reports_hold_edges(button) { + let _ = repeat.send(RepeatCmd::Start(button, action)); } } else { debug!(?button, "HID++ button with no binding — ignored"); } } CapturedInput::ButtonReleased(button) => { - // Unconditional: a Stop with nothing repeating is a no-op, and this - // way a binding that changed mid-hold still ends the cycle its own - // press started. + // Unconditional, but keyed to this button: a stop for a button that + // is not repeating is a no-op, so a binding that changed mid-hold + // still ends the cycle its own press started — while another thumb + // button held at the same time keeps repeating. debug!(?button, "HID++ button released"); - let _ = repeat.send(RepeatCmd::Stop); + let _ = repeat.send(RepeatCmd::Stop(button)); } CapturedInput::Scroll(rotation) => { // Positive rotation is "up"; each direction has its own binding. @@ -513,6 +544,51 @@ fn advance( mod tests { use super::*; + /// Only the thumb-side buttons are diverted for hold tracking, so they are + /// the only ones whose press is guaranteed a matching release. Starting a + /// repeat off any other capture would leave it firing forever. + #[test] + fn only_the_hold_tracked_buttons_report_both_edges() { + assert!(reports_hold_edges(ButtonId::Back)); + assert!(reports_hold_edges(ButtonId::Forward)); + for button in [ + ButtonId::DpiToggle, + ButtonId::Thumbwheel, + ButtonId::ThumbwheelScrollUp, + ButtonId::ThumbwheelScrollDown, + ButtonId::GestureButton, + ] { + assert!( + !reports_hold_edges(button), + "{button:?} is rising-edge only — it must not start a repeat" + ); + } + } + + #[test] + fn hold_buttons_lists_only_repeatable_bindings_that_can_be_tracked() { + let maps = Arc::new(RwLock::new(hook_runtime::HookMaps { + bindings: BTreeMap::from([ + // Repeatable and divertable for hold tracking → wanted. + (ButtonId::Back, Action::VolumeDown), + (ButtonId::Forward, Action::VolumeUp), + // Repeatable but rising-edge only: diverting it would gain + // nothing and starting a repeat off it could never be stopped. + (ButtonId::DpiToggle, Action::VolumeUp), + // Repeatable by binding, but rotation is not a held button. + (ButtonId::ThumbwheelScrollUp, Action::HorizontalScrollRight), + // Hold-trackable button, but a one-shot action. + (ButtonId::MiddleClick, Action::PlayPause), + ]), + gestures: BTreeMap::new(), + })); + assert_eq!( + hold_buttons(&maps), + vec![ButtonId::Back, ButtonId::Forward], + "diverting anything else trades crash-safety for nothing" + ); + } + #[test] fn multiplier_is_unity_at_default_sensitivity() { assert!((scroll_multiplier(DEFAULT_THUMBWHEEL_SENSITIVITY) - 1.0).abs() < f32::EPSILON); diff --git a/crates/openlogi-hid/src/gesture.rs b/crates/openlogi-hid/src/gesture.rs index d1d5913f..729ef561 100644 --- a/crates/openlogi-hid/src/gesture.rs +++ b/crates/openlogi-hid/src/gesture.rs @@ -186,6 +186,11 @@ pub async fn run_capture_session( /// The set of controls a session has diverted, kept so they can be handed back /// to the firmware on teardown. +/// +/// [`Default`] is what makes partial arming recoverable: [`arm_controls`] starts +/// from an empty record and fills it in as each divert succeeds, so a failure +/// half-way still has something to restore. +#[derive(Default)] struct ArmedControls { /// `0x1b04` accessor + feature index, present when the device exposes it. reprog: Option<(ReprogControlsV4, u8)>, @@ -241,22 +246,60 @@ async fn arm_controls( divert_gesture_button: bool, hold_buttons: &[ButtonId], ) -> Result { + // Arming is several independent HID++ writes and any one of them can fail. + // Build the record in place so a failure part-way can hand back whatever was + // already diverted: without this, a control diverted before the failure stays + // captured with no session alive to release it — for a thumb button that + // means it is dead to the OS until the mouse is power-cycled. + let mut armed = ArmedControls::default(); + match arm_into( + &mut armed, + chan, + slot, + capture_thumbwheel, + divert_gesture_button, + hold_buttons, + ) + .await + { + Ok(()) => Ok(armed), + Err(e) => { + warn!(error = %e, "arming failed part-way — restoring the controls already diverted"); + armed.disarm().await; + Err(e) + } + } +} + +/// Body of [`arm_controls`], recording each success on `armed` as it goes. +async fn arm_into( + armed: &mut ArmedControls, + chan: &Arc, + slot: u8, + capture_thumbwheel: bool, + divert_gesture_button: bool, + hold_buttons: &[ButtonId], +) -> Result<(), GestureError> { let device = Device::new(Arc::clone(chan), slot) .await .map_err(|_| GestureError::DeviceUnreachable(slot))?; - let mut reprog: Option<(ReprogControlsV4, u8)> = None; - let mut gesture_diverted = false; - let mut dpi_cids: Vec = Vec::new(); - let mut hold: Vec<(u16, ButtonId)> = Vec::new(); if let Some(info) = device .root() .get_feature(reprog_controls::FEATURE_ID) .await .map_err(|e| GestureError::Hidpp(format!("{e:?}")))? { - let rc = ReprogControlsV4::new(Arc::clone(chan), slot, info.index); - let controls = enumerate_controls(&rc).await?; + // Recorded before the first divert, not after the last: `disarm` walks + // this accessor, so it has to be in place for any restore to happen. + armed.reprog = Some(( + ReprogControlsV4::new(Arc::clone(chan), slot, info.index), + info.index, + )); + let Some((rc, _)) = armed.reprog.as_ref() else { + unreachable!("just assigned"); + }; + let controls = enumerate_controls(rc).await?; // Only divert the gesture button when it owns the gesture role; otherwise // leave it native (a non-owner HID++ control must not be captured-and-dropped). @@ -268,14 +311,14 @@ async fn arm_controls( rc.set_cid_reporting(reprog_controls::GESTURE_BUTTON_CID, true, true) .await .map_err(|e| GestureError::Hidpp(format!("{e:?}")))?; - gesture_diverted = true; + armed.gesture_diverted = true; } for &cid in &reprog_controls::DPI_MODE_SHIFT_CIDS { if controls.iter().any(|c| c.cid == cid && c.is_divertable()) { rc.set_cid_reporting(cid, true, false) .await .map_err(|e| GestureError::Hidpp(format!("{e:?}")))?; - dpi_cids.push(cid); + armed.dpi_cids.push(cid); } } // Thumb-side buttons, diverted only when a caller asked for hold @@ -286,14 +329,14 @@ async fn arm_controls( let Some(cid) = reprog_controls::hold_cid_for_button(button) else { continue; }; - if hold.iter().any(|(existing, _)| *existing == cid) { + if armed.hold.iter().any(|(existing, _)| *existing == cid) { continue; } if controls.iter().any(|c| c.cid == cid && c.is_divertable()) { rc.set_cid_reporting(cid, true, false) .await .map_err(|e| GestureError::Hidpp(format!("{e:?}")))?; - hold.push((cid, button)); + armed.hold.push((cid, button)); } else { debug!( ?button, @@ -301,10 +344,8 @@ async fn arm_controls( ); } } - reprog = Some((rc, info.index)); } - let mut thumb: Option<(Thumbwheel, u8)> = None; if capture_thumbwheel && let Some(info) = device .root() @@ -327,22 +368,20 @@ async fn arm_controls( tw.set_reporting(true, false) .await .map_err(|e| GestureError::Hidpp(format!("{e:?}")))?; - thumb = Some((tw, info.index)); + armed.thumb = Some((tw, info.index)); } else { debug!("thumb wheel reports no single tap — click not capturable"); } } - if !gesture_diverted && dpi_cids.is_empty() && thumb.is_none() { + if !armed.gesture_diverted + && armed.dpi_cids.is_empty() + && armed.hold.is_empty() + && armed.thumb.is_none() + { debug!(slot, "no capturable controls — idle session"); } - Ok(ArmedControls { - reprog, - gesture_diverted, - dpi_cids, - hold, - thumb, - }) + Ok(()) } /// Log (don't propagate) a failure to hand a control back to the firmware. From f6c93965ad12bdcf0f350feb3aa3ac175d277a5c Mon Sep 17 00:00:00 2001 From: denevrove Date: Mon, 27 Jul 2026 21:58:10 +0800 Subject: [PATCH 3/4] fix(hid): record diverted controls before awaiting the divert MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rollback recorded a control only after its set_cid_reporting response came back, but the request reaches the device before the response does: a timed-out or lost response can leave the control diverted with nothing in the record to restore it, which for a thumb button means dead to the OS until the mouse is power-cycled. Record first, then await. The record is now deliberately pessimistic — restoring a control that was never actually diverted just hands back a mapping the firmware already owns, while missing one is the failure above. --- crates/openlogi-hid/src/gesture.rs | 33 ++++++++++++++++++++---------- 1 file changed, 22 insertions(+), 11 deletions(-) diff --git a/crates/openlogi-hid/src/gesture.rs b/crates/openlogi-hid/src/gesture.rs index 729ef561..10b17994 100644 --- a/crates/openlogi-hid/src/gesture.rs +++ b/crates/openlogi-hid/src/gesture.rs @@ -188,8 +188,15 @@ pub async fn run_capture_session( /// to the firmware on teardown. /// /// [`Default`] is what makes partial arming recoverable: [`arm_controls`] starts -/// from an empty record and fills it in as each divert succeeds, so a failure -/// half-way still has something to restore. +/// from an empty record and fills it in as it goes, so a failure half-way still +/// has something to restore. +/// +/// The record is deliberately pessimistic — a control is listed *before* its +/// divert is awaited, because the request leaves for the device before the +/// response comes back, so a timed-out or lost response can still leave the +/// control diverted. Restoring one that was never actually diverted just hands +/// back a mapping the firmware already owns; missing one leaves a thumb button +/// dead to the OS. #[derive(Default)] struct ArmedControls { /// `0x1b04` accessor + feature index, present when the device exposes it. @@ -247,10 +254,10 @@ async fn arm_controls( hold_buttons: &[ButtonId], ) -> Result { // Arming is several independent HID++ writes and any one of them can fail. - // Build the record in place so a failure part-way can hand back whatever was - // already diverted: without this, a control diverted before the failure stays - // captured with no session alive to release it — for a thumb button that - // means it is dead to the OS until the mouse is power-cycled. + // Build the record in place so a failure part-way can hand back whatever may + // already be diverted: without this, a control diverted before the failure + // stays captured with no session alive to release it — for a thumb button + // that means it is dead to the OS until the mouse is power-cycled. let mut armed = ArmedControls::default(); match arm_into( &mut armed, @@ -271,7 +278,8 @@ async fn arm_controls( } } -/// Body of [`arm_controls`], recording each success on `armed` as it goes. +/// Body of [`arm_controls`], recording each control on `armed` before its divert +/// is awaited — see [`ArmedControls`] for why that ordering is the safe one. async fn arm_into( armed: &mut ArmedControls, chan: &Arc, @@ -308,17 +316,17 @@ async fn arm_into( .iter() .any(|c| c.cid == reprog_controls::GESTURE_BUTTON_CID && c.supports_raw_xy()) { + armed.gesture_diverted = true; rc.set_cid_reporting(reprog_controls::GESTURE_BUTTON_CID, true, true) .await .map_err(|e| GestureError::Hidpp(format!("{e:?}")))?; - armed.gesture_diverted = true; } for &cid in &reprog_controls::DPI_MODE_SHIFT_CIDS { if controls.iter().any(|c| c.cid == cid && c.is_divertable()) { + armed.dpi_cids.push(cid); rc.set_cid_reporting(cid, true, false) .await .map_err(|e| GestureError::Hidpp(format!("{e:?}")))?; - armed.dpi_cids.push(cid); } } // Thumb-side buttons, diverted only when a caller asked for hold @@ -333,10 +341,10 @@ async fn arm_into( continue; } if controls.iter().any(|c| c.cid == cid && c.is_divertable()) { + armed.hold.push((cid, button)); rc.set_cid_reporting(cid, true, false) .await .map_err(|e| GestureError::Hidpp(format!("{e:?}")))?; - armed.hold.push((cid, button)); } else { debug!( ?button, @@ -365,10 +373,13 @@ async fn arm_into( } }; if supports_single_tap { + armed.thumb = Some((tw, info.index)); + let Some((tw, _)) = armed.thumb.as_ref() else { + unreachable!("just assigned"); + }; tw.set_reporting(true, false) .await .map_err(|e| GestureError::Hidpp(format!("{e:?}")))?; - armed.thumb = Some((tw, info.index)); } else { debug!("thumb wheel reports no single tap — click not capturable"); } From 87c41dcc17ef3ab31196ec221e27d7c423518bd3 Mon Sep 17 00:00:00 2001 From: denevrove Date: Mon, 27 Jul 2026 22:09:59 +0800 Subject: [PATCH 4/4] fix(agent): give each capture session its own input channel MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The manager receiver outlived individual sessions, so a press captured just before a teardown stayed queued and was dispatched after StopAll had run. That started a repeat cycle whose release could never arrive — the session that would have reported it was already gone — and the action kept firing. Teardown now hands the manager a fresh channel, dropping whatever the dead session had queued along with its receiver, on both the deliberate stop and the unexpected-death path. The correctness no longer rests on the order the select loop happens to poll its branches in. --- .../src/watchers/gesture.rs | 81 ++++++++++++++++--- 1 file changed, 70 insertions(+), 11 deletions(-) diff --git a/crates/openlogi-agent-core/src/watchers/gesture.rs b/crates/openlogi-agent-core/src/watchers/gesture.rs index 5b32fc26..4de5406a 100644 --- a/crates/openlogi-agent-core/src/watchers/gesture.rs +++ b/crates/openlogi-agent-core/src/watchers/gesture.rs @@ -181,16 +181,39 @@ fn hold_buttons(hook_maps: &SharedHookMaps) -> Vec { out } +/// Cut a dead session's input loose and end any repeat it started. +/// +/// A stop tells the repeater that nothing still held will ever see its release — +/// that session was the only source of release edges. But the stop alone is not +/// enough: the manager's receiver outlives individual sessions, so a press +/// captured just before teardown can still be sitting in it and get dispatched +/// *after* the stop, starting a cycle nothing can end. Handing the manager a +/// fresh channel drops those queued events with the old receiver, so the fix +/// does not depend on the order the loop happens to poll its branches in. +fn discard_session_input( + tx: &mut mpsc::UnboundedSender, + rx: &mut mpsc::UnboundedReceiver, + repeat: &Sender, +) { + let (new_tx, new_rx) = mpsc::unbounded_channel(); + *tx = new_tx; + *rx = new_rx; + let _ = repeat.send(RepeatCmd::StopAll); +} + /// Tear the live capture session down, if there is one. /// -/// Signalling the oneshot lets the session restore the controls it diverted; the -/// repeat stop goes with it because that session is the only source of release -/// edges — anything held right now would never see its release, and the action -/// would keep firing with nothing left to stop it. -fn stop_session(stop: &mut Option>, repeat: &Sender) { +/// Signalling the oneshot lets the session restore the controls it diverted; +/// [`discard_session_input`] handles everything it leaves behind. +fn stop_session( + stop: &mut Option>, + tx: &mut mpsc::UnboundedSender, + rx: &mut mpsc::UnboundedReceiver, + repeat: &Sender, +) { if let Some(stop) = stop.take() { let _ = stop.send(()); - let _ = repeat.send(RepeatCmd::StopAll); + discard_session_input(tx, rx, repeat); } } @@ -205,7 +228,10 @@ async fn manage( thumbwheel_sensitivity: ThumbwheelSensitivity, receiver_access: ReceiverAccess, ) { - let (tx, mut rx) = mpsc::unbounded_channel::(); + // Replaced on every teardown (see `discard_session_input`), so one session's + // queued input can never reach the next. The manager keeps its own sender so + // the receiver stays open while no session is running. + let (mut tx, mut rx) = mpsc::unbounded_channel::(); // Hold-to-repeat for the buttons this path diverts. Lives for the whole // manage loop so a session restart mid-hold cannot orphan a running cycle. let repeat = spawn_repeater(Arc::clone(&dpi_cycle), Arc::clone(&capture_channel)); @@ -270,7 +296,7 @@ async fn manage( } // Target or thumb-wheel arming changed (or first tick): stop the // old session and start one for the new state. - stop_session(&mut stop, &repeat); + stop_session(&mut stop, &mut tx, &mut rx, &repeat); if current.is_some() { current = None; continue; @@ -326,9 +352,10 @@ async fn manage( if should_rearm(done_epoch, epoch, current.is_some()) { warn!("capture session for the active device ended unexpectedly, re-arming"); // A session that died mid-hold (device disconnect, HID++ - // error) takes the release edge with it — same reasoning as - // the deliberate stop above. - let _ = repeat.send(RepeatCmd::StopAll); + // error) takes the release edge with it, and whatever it had + // already queued is just as stale — same reasoning as the + // deliberate stop above. + discard_session_input(&mut tx, &mut rx, &repeat); current = None; // Keep the `stop`/`current` invariant: the session already // exited, so its stop receiver is gone and dropping the sender @@ -544,6 +571,38 @@ fn advance( mod tests { use super::*; + #[test] + fn input_queued_before_a_teardown_never_reaches_the_next_session() { + // The press that arrives a moment before the session is stopped: its + // release can never follow, so dispatching it would start a repeat + // nothing could end. + let (mut tx, mut rx) = mpsc::unbounded_channel::(); + let session_sink = tx.clone(); + let (repeat_tx, repeat_rx) = std::sync::mpsc::channel(); + assert!( + session_sink + .send(CapturedInput::ButtonPressed(ButtonId::Back)) + .is_ok() + ); + + discard_session_input(&mut tx, &mut rx, &repeat_tx); + + assert!( + rx.try_recv().is_err(), + "the dead session's queued press must be unreachable, not merely late" + ); + assert!( + matches!(repeat_rx.try_recv(), Ok(RepeatCmd::StopAll)), + "and anything already repeating must be stopped" + ); + assert!( + session_sink + .send(CapturedInput::ButtonPressed(ButtonId::Forward)) + .is_err(), + "a session still running after teardown has nowhere left to send" + ); + } + /// Only the thumb-side buttons are diverted for hold tracking, so they are /// the only ones whose press is guaranteed a matching release. Starting a /// repeat off any other capture would leave it firing forever.