From 8abdd74a622f073c03a533390687da8054b6dda0 Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Tue, 4 Aug 2026 19:10:45 +0200 Subject: [PATCH] fix(client/desktop): the Deck keeps its trackpad, and a pad stops buzzing at exit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three faults in the desktop session's gamepad path. The Steam Deck lost its built-in trackpad-mouse at the start of every session. SDL's Valve HIDAPI driver clears the pad's digital mappings during *enumeration*, which is part of bringing the gamepad subsystem up — so holding the drivers off from inside GamepadService::pumped could never work: receiving a GamepadSubsystem means the enumeration has already happened. The hint set there detached a driver that had already done the damage, and lizard mode only came back seconds later when the firmware watchdog restored it. The presenter now disables them with its other pre-SDL_Init hints. The threaded worker always had this right; only the caller-pumped path was wrong, and it could not fix itself, hence a separate entry point its callers can place correctly. Player LEDs did nothing at all on any pad that is not a DualSense. The match arm handled the DualSense raw-effects path and let everything else fall through a bare `_`, though SDL exposes set_player_index and owns the per-device pattern. The wire carries a positional bitmask rather than an index, and the bridge is the popcount: every convention that reaches this wire spells "player N" as N lit LEDs — the DualSense patterns 0x04/0x0A/0x15/0x1B/0x1F and the Switch/XInput run 0x01/0x03/0x07/0x0F alike — so counting them works for both, where reading a bit position would only ever suit one. No lit LED means no player, not player 0. The remaining unhandled variants are now named rather than swept up by `_`, so a new one cannot join them silently. A forwarded pad could be left buzzing when the session ended. detach() only posts Ctl::Detach; the close that flushes the pad, tells the host to remove it and explicitly zeroes the motors runs when the pump next drains that message. Single mode broke out of the loop immediately after detaching and Event::Quit never detached at all, so both skipped it entirely. The teardown now sits where every exit converges instead of on the individual breaks. That still leaves the several paths that leave by `?` on a fatal overlay or present error, so the pump also silences its slots on Drop — the explicit call stays, because a pad should go quiet before a long teardown rather than after it. Drop closes the slots directly rather than draining the queue that would have done it: same physical outcome, and it touches no lock, where draining reaches an unwrap on a Mutex that would abort the process if it panicked mid-unwind. --- crates/pf-client-core/src/gamepad.rs | 134 ++++++++++++++++++++++++++- crates/pf-presenter/src/run.rs | 14 +++ 2 files changed, 144 insertions(+), 4 deletions(-) diff --git a/crates/pf-client-core/src/gamepad.rs b/crates/pf-client-core/src/gamepad.rs index 3eb51d7b..37f0d8d3 100644 --- a/crates/pf-client-core/src/gamepad.rs +++ b/crates/pf-client-core/src/gamepad.rs @@ -285,6 +285,21 @@ fn set_valve_hidapi(enabled: bool) { sdl3::hint::set("SDL_JOYSTICK_HIDAPI_STEAM", v); } +/// Disable the Valve HIDAPI drivers **before SDL exists** — call this alongside the other +/// pre-`SDL_Init` hints, not after a subsystem is up. +/// +/// The damage these drivers do happens at *enumeration*, which is part of initialising the +/// joystick/gamepad subsystem. Setting the hint afterwards does detach the driver, but only after +/// it has already sent the Deck its `ID_CLEAR_DIGITAL_MAPPINGS` + `TRACKPAD_NONE` — so the +/// built-in trackpad-mouse dies system-wide and stays dead until the firmware watchdog restores +/// lizard mode seconds later. The threaded worker ([`run`]) has always done this in the right +/// order; the caller-pumped path could not, because by the time it receives a +/// [`sdl3::GamepadSubsystem`] the enumeration has already happened. Hence a separate entry point +/// its callers can put in the right place. +pub fn preinit_disable_valve_hidapi() { + set_valve_hidapi(false); +} + /// Map the SDL-reported controller type to the virtual pad we'd ask the host to create. fn pref_for_type(t: sdl3::gamepad::GamepadType) -> GamepadPref { use sdl3::gamepad::GamepadType as T; @@ -393,9 +408,12 @@ impl GamepadService { /// and calls [`GamepadPump::tick`] once per loop iteration (the threaded worker's /// per-wakeup work: ctl drain, chord-hold check, menu repeat, feedback). /// - /// Like the threaded worker, this disables the Valve HIDAPI drivers up front (their - /// mere enumeration kills the Deck's trackpad-mouse system-wide); they are enabled - /// for the duration of an attached session only. + /// The Valve HIDAPI drivers are held off here too, but this is **too late to be the only + /// place it happens**: the `subsystem` argument means enumeration is already done, and that + /// is when the Deck driver kills the trackpad-mouse. The caller must also call + /// [`preinit_disable_valve_hidapi`] with its other pre-`SDL_Init` hints. This call still + /// earns its place — it re-asserts "off" for a process that ran a session earlier — but on + /// its own it only detaches a driver that has already done the damage. pub fn pumped(subsystem: sdl3::GamepadSubsystem) -> (GamepadService, GamepadPump) { set_valve_hidapi(false); let pads = Arc::new(Mutex::new(Vec::new())); @@ -556,6 +574,38 @@ impl GamepadPump { self.worker.menu_poll(); self.worker.render_feedback(); } + + /// Close every forwarded slot — flush its held wire state, tell the host to remove the pad, + /// and physically silence it. Call once on the way out of the caller's event loop. + /// + /// [`GamepadService::detach`] only *posts* `Ctl::Detach`; the close — the flush, the host-side + /// `GamepadRemove`, and the explicit `set_rumble(0, 0)` backstop in `close_slot_at` — happens + /// when the pump next drains it. An exit path that detached and then left the loop without + /// another [`tick`](Self::tick) therefore skipped all of it, and nothing else would: the slots + /// hold no `Drop` that silences them. A pad left mid-buzz stayed buzzing. + /// + /// This closes the slots directly rather than draining the queued `Ctl::Detach` that would + /// have done it. Same physical outcome by a shorter path, and deliberately so: this also runs + /// from `Drop`, and `drain_ctl` reaches `Mutex::lock().unwrap()`, which on a poisoned lock + /// would panic — during an unwind that aborts the process. Closing a slot touches no lock. + /// + /// Idempotent, and safe with nothing attached. + pub fn shutdown(&mut self) { + self.worker.close_all_slots(); + } +} + +/// The silence backstop of last resort. A caller's loop can also leave by `?` on a fatal overlay +/// or present error — several paths do — and those would skip an explicit +/// [`shutdown`](GamepadPump::shutdown) entirely, leaving a forwarded pad buzzing on the way out. +/// +/// Callers should still call `shutdown` at their normal exit rather than lean on this: the pad +/// wants to go quiet *before* a long teardown (session join, `vkDeviceWaitIdle`), not after it. +/// Doing both is free — `shutdown` is idempotent. +impl Drop for GamepadPump { + fn drop(&mut self) { + self.shutdown(); + } } /// The lowest wire pad index (0..[`MAX_PADS`](punktfunk_core::input::MAX_PADS)) not already held @@ -1626,6 +1676,11 @@ impl Worker { HidOutput::PlayerLeds { bits, .. } if is_ds => { let _ = slot.pad.send_effect(&Ds5Feedback::player_packet(bits)); } + // Every other pad with player LEDs gets them through SDL, which owns the + // per-device pattern. This used to fall through and do nothing at all. + HidOutput::PlayerLeds { bits, .. } => { + let _ = set_player_leds(&slot.pad, bits); + } HidOutput::Trigger { which, ref effect, .. } if is_ds => { @@ -1633,12 +1688,43 @@ impl Worker { .pad .send_effect(&Ds5Feedback::trigger_packet(which, effect)); } - _ => {} + // Deliberately unhandled, listed rather than left to a bare `_` so a new + // variant cannot join them silently: adaptive triggers exist only on a + // DualSense, and the trackpad-haptic / raw-passthrough planes are DS-specific + // and carried by `send_effect` above when the pad is one. + HidOutput::Trigger { .. } + | HidOutput::TrackpadHaptic { .. } + | HidOutput::HidRaw { .. } => {} } } } } +/// The SDL player index for the wire's positional player-LED `bits`, or `None` for "no player". +/// +/// The wire carries a bitmask — one bit per LED, low 5 — while SDL wants a player *index* and owns +/// the per-device pattern. The count bridges them: every convention that reaches this wire spells +/// "player N" as N lit LEDs, both the DualSense patterns (`0x04`, `0x0A`, `0x15`, `0x1B`, `0x1F`) +/// and the Switch/XInput run of low bits (`0x01`, `0x03`, `0x07`, `0x0F`). SDL's index is 0-based, +/// so player 1 is index 0; no lit LED means *no* player rather than player 0. +/// +/// Split out from [`set_player_leds`] so the mapping is testable — an `sdl3::Gamepad` needs a real +/// device, so nothing that takes one can be. +fn player_index_from_bits(bits: u8) -> Option { + match (bits & 0x1F).count_ones() { + 0 => None, + n => Some((n - 1) as u16), + } +} + +/// Drive a non-DualSense pad's player LEDs from the wire's positional `bits`. +fn set_player_leds(pad: &sdl3::gamepad::Gamepad, bits: u8) -> Result<(), sdl3::Error> { + match player_index_from_bits(bits) { + None => pad.unset_player_index(), + Some(i) => pad.set_player_index(i), + } +} + /// The wire pad index a [`HidOutput`] is addressed to (every variant carries `pad`). fn hidout_pad(h: &HidOutput) -> u8 { match h { @@ -2008,3 +2094,43 @@ mod slot_tests { ); } } + +#[cfg(test)] +mod player_led_tests { + use super::*; + + /// Both conventions that reach this wire spell "player N" as N lit LEDs, so the count is the + /// player number regardless of WHICH bits a given pad lights. Pinned because the mapping is + /// otherwise only obvious once you have seen both patterns side by side. + #[test] + fn player_index_counts_lit_leds_for_both_conventions() { + // DualSense / hid-playstation patterns — non-contiguous, symmetric about the centre LED. + assert_eq!(player_index_from_bits(0x04), Some(0)); // player 1 + assert_eq!(player_index_from_bits(0x0A), Some(1)); // player 2 + assert_eq!(player_index_from_bits(0x15), Some(2)); // player 3 + assert_eq!(player_index_from_bits(0x1B), Some(3)); // player 4 + assert_eq!(player_index_from_bits(0x1F), Some(4)); // player 5 + + // Switch/XInput style — a contiguous run of low bits, the same count each time. + assert_eq!(player_index_from_bits(0x01), Some(0)); + assert_eq!(player_index_from_bits(0x03), Some(1)); + assert_eq!(player_index_from_bits(0x07), Some(2)); + assert_eq!(player_index_from_bits(0x0F), Some(3)); + } + + /// No lit LED is "no player", NOT player 0 — the difference between LEDs off and player 1 lit. + #[test] + fn no_lit_led_is_no_player() { + assert_eq!(player_index_from_bits(0x00), None); + // Only the low 5 bits are player LEDs; junk above them must not invent a player. + assert_eq!(player_index_from_bits(0xE0), None); + } + + /// The mask is applied before counting, so out-of-range bits cannot inflate the index past + /// the 5 real LEDs. + #[test] + fn high_bits_are_masked_off_before_counting() { + assert_eq!(player_index_from_bits(0xFF), Some(4)); // 0x1F worth of LEDs, not 8 + assert_eq!(player_index_from_bits(0xE4), Some(0)); // 0x04 with junk on top + } +} diff --git a/crates/pf-presenter/src/run.rs b/crates/pf-presenter/src/run.rs index 1ef0f997..b91127ab 100644 --- a/crates/pf-presenter/src/run.rs +++ b/crates/pf-presenter/src/run.rs @@ -466,6 +466,13 @@ fn run_inner(mut opts: SessionOpts, mut mode: ModeCtl) -> Result #[cfg(windows)] crate::win32::set_app_user_model_id(); sdl3::hint::set("SDL_JOYSTICK_THREAD", "1"); + // Hold SDL's Valve HIDAPI drivers off BEFORE SDL_Init: the Deck driver clears the pad's + // digital mappings at *enumeration*, which is part of bringing the gamepad subsystem up, so a + // hint set after `sdl.gamepad()` — where this used to live, inside GamepadService::pumped — + // only detached a driver that had already killed the built-in trackpad-mouse system-wide. The + // symptom was the Deck losing its trackpad cursor at the start of every session until the + // firmware watchdog restored lizard mode. They are still enabled for an attached session. + pf_client_core::gamepad::preinit_disable_valve_hidapi(); // A touchscreen (the Deck's glass) is forwarded as REAL touch passthrough below — so // suppress SDL's default synthesis of mouse events from touch. Left on, every touch // ALSO warps a synthetic mouse to the touch point, which under the stream's relative @@ -1895,6 +1902,13 @@ fn run_inner(mut opts: SessionOpts, mut mode: ModeCtl) -> Result } }; + // Every exit from the loop above converges here, which is why the gamepad teardown belongs + // here and not on the individual `break`s. `gamepad.detach()` only queues the detach; the + // close — flush, host-side GamepadRemove, and the explicit rumble-stop backstop — runs when + // the pump drains it. Single mode broke out of the loop immediately after detaching and + // Event::Quit never detached at all, so both left forwarded pads unflushed and, if the game + // was rumbling at the time, still buzzing. + pump.shutdown(); // Join the pump BEFORE the device-wide idle: its decode submissions on the shared // device would race vkDeviceWaitIdle otherwise. if let Some(st) = stream.take() {