Compare commits

..
Author SHA1 Message Date
enricobuehler 8abdd74a62 fix(client/desktop): the Deck keeps its trackpad, and a pad stops buzzing at exit
apple / swift (pull_request) Successful in 1m23s
apple / screenshots (pull_request) Skipped
ci / rust-arm64 (pull_request) Successful in 1m55s
ci / web (pull_request) Successful in 1m14s
ci / docs-site (pull_request) Successful in 1m15s
android / android (pull_request) Successful in 8m52s
ci / rust (pull_request) Successful in 12m50s
windows / build (aarch64-pc-windows-msvc) (pull_request) Successful in 1m7s
windows / build (x86_64-pc-windows-msvc) (pull_request) Successful in 2m2s
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.
2026-08-04 19:10:45 +02:00
5 changed files with 178 additions and 125 deletions
+130 -4
View File
@@ -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<u16> {
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
}
}
@@ -819,77 +819,46 @@ impl DriverAttach {
/// One-shot WARN with everything the host can find out about WHY the driver isn't attached:
/// driver-store presence, the devnode's PnP status/problem code, and where to look next.
///
/// Runs on its own thread and returns immediately. The caller is the session's pad service
/// thread — the one feeding input and rumble — and everything below is slow: the driver-store
/// check waits up to [`INVENTORY_WAIT`] for a `pnputil` enumeration that can take tens of
/// seconds, and the devnode lookup is a synchronous PnP call. Blocking there stalled input for
/// up to two seconds *per unattached pad* (the wait is a deadline, not a one-off: while the
/// enumeration is still outstanding every pad pays it again), at exactly the moment a session
/// is already going wrong. Diagnostics must never be able to hurt the thing they diagnose.
///
/// Off the hot path the wait also stops being a compromise — it can afford to be patient and
/// report what it actually found rather than "still enumerating".
fn diagnose(&self) {
let (driver, inf, driver_log) = (self.driver, self.inf, self.driver_log);
let shm_name = self.shm_name.clone();
let instance_id = self.instance_id.clone();
std::thread::Builder::new()
.name("pf-driver-diagnose".into())
.spawn(move || diagnose_blocking(driver, inf, driver_log, &shm_name, instance_id))
.ok();
let store = match driver_store_has(self.inf) {
Some(true) => "driver package present in the driver store",
Some(false) => {
"driver package NOT in the driver store — run: punktfunk-host.exe driver install --gamepad"
}
None => "driver store could not be queried (pnputil failed or still enumerating)",
};
let devnode = match &self.instance_id {
Some(id) => devnode_status_line(id),
None => {
"no per-session devnode (SwDeviceCreate failed earlier — see the warning above)"
.to_string()
}
};
tracing::warn!(
driver = self.driver,
shm = %self.shm_name,
grace_secs = ATTACH_GRACE.as_secs(),
store,
devnode = %devnode,
driver_log = self.driver_log,
"gamepad driver has not attached to the shared section — the virtual pad exists but no \
driver is serving it (games will not see it); an old (pre-sealed-channel) driver also \
reads as not-attached: update with punktfunk-host.exe driver install --gamepad \
(driver_log is only written by debug driver builds, or with the PFXUSB_DEBUG_LOG / \
PFGAMEPAD_DEBUG_LOG / PFMOUSE_DEBUG_LOG system env var set + the device restarted)"
);
}
}
/// The body of [`DriverAttach::diagnose`], on its own thread. Split out rather than inlined into
/// the closure so the blocking calls stay visible as blocking.
fn diagnose_blocking(
driver: &'static str,
inf: &'static str,
driver_log: &'static str,
shm_name: &str,
instance_id: Option<String>,
) {
let store = match driver_store_has(inf) {
Some(true) => "driver package present in the driver store",
Some(false) => {
"driver package NOT in the driver store — run: punktfunk-host.exe driver install --gamepad"
}
None => "driver store could not be queried (pnputil failed or still enumerating)",
};
let devnode = match &instance_id {
Some(id) => devnode_status_line(id),
None => "no per-session devnode (SwDeviceCreate failed earlier — see the warning above)"
.to_string(),
};
tracing::warn!(
driver,
shm = %shm_name,
grace_secs = ATTACH_GRACE.as_secs(),
store,
devnode = %devnode,
driver_log,
"gamepad driver has not attached to the shared section — the virtual pad exists but no \
driver is serving it (games will not see it); an old (pre-sealed-channel) driver also \
reads as not-attached: update with punktfunk-host.exe driver install --gamepad \
(driver_log is only written by debug driver builds, or with the PFXUSB_DEBUG_LOG / \
PFGAMEPAD_DEBUG_LOG / PFMOUSE_DEBUG_LOG system env var set + the device restarted)"
);
}
/// How long [`driver_store_inventory`] waits for the background pnputil query before reporting
/// without it. Only [`diagnose_blocking`] waits, and that has a thread to itself, so this is
/// generous: pnputil routinely takes longer than a couple of seconds on a busy driver store, and
/// the old two-second budget — chosen to limit the damage while this ran on the pad service thread
/// — meant the diagnosis usually gave up and printed "still enumerating", which is the one answer
/// that helps nobody. Nothing waits on this thread, so patience costs only a late log line.
const INVENTORY_WAIT: Duration = Duration::from_secs(30);
/// How long [`driver_store_inventory`] lets the caller wait for the background pnputil query
/// before reporting without it — [`observe`] runs on the pad service thread, which must keep
/// draining pad slots even when the driver store is wedged.
const INVENTORY_WAIT: Duration = Duration::from_secs(2);
/// Driver-store inventory (`pnputil /enum-drivers`), lower-cased, fetched once per process — only
/// consulted on the failure path, so the subprocess cost never hits a healthy session. The query
/// runs on its OWN thread: pnputil can block for tens of seconds on a busy/wedged driver store,
/// and this keeps one wedged query from being re-run per pad. `None` = not available yet (query
/// still running past [`INVENTORY_WAIT`]) or
/// and the caller is the pad service thread. `None` = not available yet (query still running) or
/// failed; a query that outlives [`INVENTORY_WAIT`] still lands in the cache for later reports.
fn driver_store_inventory() -> Option<&'static str> {
static INV: OnceLock<String> = OnceLock::new();
+14
View File
@@ -466,6 +466,13 @@ fn run_inner(mut opts: SessionOpts, mut mode: ModeCtl) -> Result<Option<Outcome>
#[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<Option<Outcome>
}
};
// 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() {
@@ -360,32 +360,9 @@ fn ring_len(view: &pf_umdf_util::section::MappedView) -> u32 {
/// from being coalesced away by a following LED/trigger report inside one host poll window (the
/// confirmed stuck-rumble path).
fn publish_output(view: &pf_umdf_util::section::MappedView, bytes: &[u8]) {
// Serialized: the whole publish is a read-modify-write (read the cursor, write the slot it
// names, then advance it) and the framework dispatches output callbacks in PARALLEL, so two
// can be inside this at once. Unsynchronized, both read the same `ring_head`, both write the
// SAME slot — tearing one report's bytes across the other's — and both store head+1, so the
// cursor advances once for two reports and the host sees a single torn entry.
//
// An atomic `fetch_add` on the head does not fix it. That hands each writer a distinct slot,
// but it advances the cursor BEFORE the slot bytes exist, so the host can read a slot that is
// still being filled — trading a torn slot for a torn slot the host is invited to read. Making
// the head-advance mean "the slot below is complete" is exactly what the lock buys.
//
// Poison-tolerant on purpose. Poison is sticky, so the repo's usual `if let Ok(g) = lock()`
// would skip the publish for the REST OF THE PROCESS after a single panic elsewhere — silently
// ending game output. Recovering the guard is safe here: the protected state is bytes in a
// shared section, not an invariant a panic could have broken.
let _publish = RING_PUBLISH
.lock()
.unwrap_or_else(std::sync::PoisonError::into_inner);
view.write_bytes(OFF_OUTPUT, bytes);
let seq = view.read_u32(OFF_OUT_SEQ).wrapping_add(1);
// Release, not a plain write: the host loads `out_seq` with Acquire specifically to order its
// copy of the report bytes after it (`dualsense_windows.rs`, "Acquire pairs with the driver's
// publish-then-bump store order"). An Acquire load pairs with a Release store and nothing
// else, so as a plain write this promised the host an ordering it never actually established —
// on a weakly-ordered core (ARM64) the fresh seq could arrive ahead of the bytes it announces.
view.store_u32(OFF_OUT_SEQ, seq, Ordering::Release);
view.write_u32(OFF_OUT_SEQ, seq);
let len = ring_len(view);
if len != 0 {
let head = view.read_u32(OFF_RING_HEAD);
@@ -398,11 +375,6 @@ fn publish_output(view: &pf_umdf_util::section::MappedView, bytes: &[u8]) {
}
}
/// Serializes [`publish_output`] against itself — see the note there for why an atomic cursor is
/// not enough. Uncontended in the common case: one output report at a time is the norm, and the
/// critical section is a few dozen bytes of memcpy into an already-mapped view.
static RING_PUBLISH: std::sync::Mutex<()> = std::sync::Mutex::new(());
/// The sealed-channel client (per-pad: `ProcessSharingDisabled` gives each pad its own WUDFHost, so
/// this static is per-pad). The handshake/adoption/validation state machine lives in `pf_umdf_util`.
static CHANNEL: ChannelClient = ChannelClient::new();
+1 -29
View File
@@ -358,48 +358,20 @@ fn read_state(data: Option<&MappedView>) -> (u32, u16, u8, u8, i16, i16, i16, i1
/// host can tell "driver bound and alive" apart from "driver package missing/failed to bind" and see
/// the game-visible polling path advance.
fn touch_driver_marks(data: &MappedView) {
let _marks = SECTION_PUBLISH
.lock()
.unwrap_or_else(std::sync::PoisonError::into_inner);
data.write_u32(OFF_DRIVER_PROTO, GAMEPAD_PROTO_VERSION);
let hb = data.read_u32(OFF_DRIVER_HEARTBEAT).wrapping_add(1);
data.write_u32(OFF_DRIVER_HEARTBEAT, hb);
}
/// Publish a game's rumble (from SET_STATE) into the DATA section for the host to forward.
///
/// Serialized and Release-published, because IOCTLs arrive concurrently and neither property held
/// before. `seq` was a read-modify-write across the two motor bytes: two `SET_STATE` calls could
/// both read the same value and both write back `seq + 1`, so the host — which treats an unchanged
/// seq as "nothing new" — saw one bump for two writes and skipped a level entirely. A skipped
/// **stop** is the one that hurts: the pad keeps buzzing until the host's ~2.5 s idle force-off
/// notices the game went quiet, which is where the bound on this bug comes from.
///
/// The seq store is Release for the same reason as `pf-gamepad`'s `out_seq`: the host loads it with
/// Acquire and documents that as ordering its read of the motor bytes ("the driver bumps
/// `rumble_seq` AFTER writing the rumble bytes", `gamepad_windows.rs`). A plain write gives that
/// Acquire nothing to pair with, so the guarantee the host's comment claims did not exist in either
/// direction — the host could read a fresh seq against stale motor levels on a weakly-ordered core.
fn publish_rumble(data: Option<&MappedView>, large: u8, small: u8) {
let Some(v) = data else { return };
let _publish = SECTION_PUBLISH
.lock()
.unwrap_or_else(std::sync::PoisonError::into_inner);
v.write_u8(OFF_RUMBLE_LARGE, large);
v.write_u8(OFF_RUMBLE_SMALL, small);
let seq = v.read_u32(OFF_RUMBLE_SEQ).wrapping_add(1);
v.store_u32(OFF_RUMBLE_SEQ, seq, Ordering::Release);
v.write_u32(OFF_RUMBLE_SEQ, seq);
}
/// Serializes the section's read-modify-write publishes ([`publish_rumble`], [`touch_driver_marks`])
/// against each other. One lock rather than one per field: they are all short byte writes into the
/// same mapped view, and the contention is nil compared to the IOCTL round trip that reaches them.
///
/// Poison-tolerant deliberately — poison is sticky, so bailing out on it would silently stop
/// forwarding rumble for the rest of the process. The protected state is bytes in a shared section,
/// not an invariant a panic elsewhere could have violated.
static SECTION_PUBLISH: std::sync::Mutex<()> = std::sync::Mutex::new(());
// Build the 29-byte GET_STATE buffer (the layout xinput1_4 parses).
fn build_get_state(data: Option<&MappedView>) -> [u8; 29] {
let (packet, buttons, lt, rt, lx, ly, rx, ry) = read_state(data);