From 14502769e0d97ee666af6df513c540976e978677 Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Mon, 3 Aug 2026 16:57:11 +0200 Subject: [PATCH 1/2] fix(host/input): rumble comes back when a controller does MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Unplug a pad mid-session and plug it back in, and roughly half the time it never rumbles again for the rest of the session. The removal arm restarted the pad's rumble sequence counter. The client's reorder gate does not restart: `rumble_last_seq` lives for the whole QUIC connection and has no reset path, so it still holds whatever the pad reached before the unplug. Restarting the host counter therefore hands the client a seq it has already seen, and its wrapping half-space compare drops every envelope until the counter climbs back past the stored value — up to 128 sends. Since the counter only advances on a level change or a ~120 ms renewal while a level is non-zero, that spans many separate rumble events, so it reads as a flaky controller rather than a clean outage. Whether it bites is decided by how much the pad rumbled beforehand, which is why it looks intermittent: a pad that never rumbled before the re-plug has `None` on the client side and always heals. The counter now survives, matching the sibling pad-state gate — whose comment eleven lines above already explains that a re-plug must arrive with a still- newer seq to be accepted. The three clears that actually end the stale lease move into `clear_pad_feedback`, whose signature deliberately has no seq parameter so the arm cannot regress by editing. Covered by a regression test that drives the real wire encoder and the real client gate, and asserts the pre-fix behaviour is genuinely rejected across the whole forward window, so it cannot pass vacuously. Found by the 2026-08-03 force-feedback sweep (B1/T5 — see the backlog in punktfunk-planning design/haptics-sweep-2026-08-03.md). --- crates/punktfunk-host/src/native/input.rs | 101 ++++++++++++++++++++-- 1 file changed, 96 insertions(+), 5 deletions(-) diff --git a/crates/punktfunk-host/src/native/input.rs b/crates/punktfunk-host/src/native/input.rs index a71cde1c..29e773b5 100644 --- a/crates/punktfunk-host/src/native/input.rs +++ b/crates/punktfunk-host/src/native/input.rs @@ -629,6 +629,20 @@ const RUMBLE_RENEW_FLOOR_MS: u64 = 60; /// own expiry. `3` total zero sends = the immediate one + this many renewal re-sends. const RUMBLE_STOP_BURST: u8 = 2; +/// Clear a removed pad's rumble bookkeeping — the level, the "we have seen a level" flag, and any +/// stop re-sends still owed. Together these end the pad's lease, so a re-plug on the same wire +/// index inherits nothing that could buzz the new device. +/// +/// The per-pad rumble **sequence is deliberately not a parameter**: it must stay monotonic for the +/// life of the connection because the client gates on it with a wrapping half-space compare and +/// never resets its side (`punktfunk-core/src/client/pump/datagram_task.rs`). Resetting it here is +/// the bug pinned by [`tests::rumble_seq_survives_a_removal_so_the_client_gate_accepts`]. +fn clear_pad_feedback(state: &mut (u16, u16), seen: &mut bool, stop_burst: &mut u8) { + *state = (0, 0); + *seen = false; + *stop_burst = 0; +} + /// Send one rumble datagram on the universal 0xCA plane. `envelope_on` picks the self-terminating /// v2 form (`[level][seq][ttl_ms]`, the default) or the legacy v1 level datagram (the /// `PUNKTFUNK_RUMBLE_ENVELOPE=0` bisect hatch). Best-effort like every side-plane datagram. @@ -824,11 +838,22 @@ pub(super) fn input_thread( tracing::info!(pad = idx, "gamepad unplugged (native detach)"); } // Fresh feedback bookkeeping so a later re-plug on this index inherits no - // stale rumble lease/seq (a lease still ticking would buzz the new pad). - rumble_state[idx] = (0, 0); - rumble_seen[idx] = false; - rumble_seq[idx] = 0; - rumble_stop_burst[idx] = 0; + // stale rumble lease (a lease still ticking would buzz the new pad). + // + // `rumble_seq` deliberately SURVIVES — do not reset it here. The client's + // rumble reorder gate (`client/pump/datagram_task.rs`) is per-CONNECTION + // and has no reset path, so restarting this counter strands every later + // envelope for the re-plugged pad until the host climbs back past the + // value the client already stored (up to 128 sends ≈ 15 s of continuous + // rumble, or dozens of separate rumble events). The three clears below are + // what actually kill a stale lease; the sibling `pad_seq` gate keeps its + // value across a removal for exactly the same reason (see the comment at + // the top of this arm). + clear_pad_feedback( + &mut rumble_state[idx], + &mut rumble_seen[idx], + &mut rumble_stop_burst[idx], + ); } } InputKind::GamepadArrival => { @@ -1071,6 +1096,72 @@ mod tests { } } + /// A pad re-plug must not strand the client's rumble reorder gate. + /// + /// The client's `rumble_last_seq` lives for the whole QUIC connection and has no reset path + /// (`punktfunk-core/src/client/pump/datagram_task.rs`), so this host's per-pad rumble counter + /// has to stay monotonic across a `GamepadRemove`. Regression: the removal arm used to do + /// `rumble_seq[idx] = 0`, which made every envelope after a re-plug fail `seq_newer` until the + /// counter climbed back past the value the client had already stored — up to 128 sends. + /// + /// Drives the real wire encoder and the real gate, so it fails if either side's rule moves. + #[test] + fn rumble_seq_survives_a_removal_so_the_client_gate_accepts() { + use punktfunk_core::input::GamepadSnapshot; + use punktfunk_core::quic::{decode_rumble_envelope, encode_rumble_datagram_v2}; + + // The client half: one per-pad slot, per connection, never reset. + let deliver = |seq: u8, gate: &mut Option| { + let d = encode_rumble_datagram_v2(0, 0x4000, 0x8000, seq, 400); + let env = decode_rumble_envelope(&d) + .expect("v2 envelope decodes") + .envelope + .expect("v2 tail present"); + if GamepadSnapshot::seq_newer(env.seq, *gate) { + *gate = Some(env.seq); + true + } else { + false + } + }; + + // The host half: one wrapping counter, bumped on every change and every renewal. + let mut gate: Option = None; + let mut seq = 0u8; + + // A long rumble before the unplug pushes the client's stored seq well past zero. + for _ in 0..100 { + seq = seq.wrapping_add(1); + assert!(deliver(seq, &mut gate)); + } + assert_eq!(gate, Some(100)); + + // The pad is unplugged mid-buzz: the lease is cleared, the counter is not. + let (mut state, mut seen, mut burst) = ((0x1234u16, 0x5678u16), true, RUMBLE_STOP_BURST); + clear_pad_feedback(&mut state, &mut seen, &mut burst); + assert_eq!( + (state, seen, burst), + ((0, 0), false, 0), + "lease not cleared" + ); + + // It returns on the same wire index and the game rumbles again: the very first envelope + // has to reach the actuator. + seq = seq.wrapping_add(1); + assert!( + deliver(seq, &mut gate), + "first envelope after a re-plug was dropped by the client's reorder gate" + ); + + // Non-vacuity: the pre-fix behaviour (counter restarted at 0) really is rejected, and + // stays rejected for the whole forward window — this is the bug, reproduced. + let mut stranded = Some(100u8); + assert!( + (1..=100).all(|s| !deliver(s, &mut stranded)), + "test is vacuous — a restarted counter should have been gated out" + ); + } + /// Incremental wire events accumulate into the full pad frame the virtual xpad applies. #[test] fn gamepad_accumulator() { From 9979489b56dac72cca1f119bae0b9e7359b298ab Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Mon, 3 Aug 2026 17:34:47 +0200 Subject: [PATCH 2/2] fix(host/pads): an unplugged controller actually disappears MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Unplug a controller mid-session and the virtual pad it was driving outlives it: the game keeps seeing a connected, permanently idle device for the rest of the session. The single-controller session — the common case — hits this every time. `PadSlots::sweep` needs two passes to retire a pad. The first pass to see the mask bit clear only ARMS the 300 ms devnode-churn grace; the drop lands on a later pass. But sweep runs only from a state frame, and the producer emits exactly one frame per detach — `native/input.rs` guards the emit on the bit still being set — so for a pad with no still-changing sibling in the same manager, the second pass never comes. Nothing periodic reaches sweep: `heartbeat` and `pump` walk the slots without it. Split the two halves. `sweep` still folds a frame's mask into the grace clocks, and `reap` — new — drops whatever has run out, with no frame needed. Every manager now reaps on the periodic pump it already runs, so the teardown completes ~300 ms after the detach instead of never. `reap` deliberately cannot arm a clock: it only reads `inactive_since` and clears it, so a pad whose bit never went clear has nothing to run out and no amount of reaping can drop it. That is what makes it safe on a hot loop, and it keeps the anti-flap guarantee intact — a mask that blips clear and returns still never churns a devnode. The two existing tests hand-fed a SECOND removal frame, which production never sends; they passed while the real path leaked. Both now drive the unplug through a pump tick, and PadSlots gains three tests pinning the new invariants. Verified non-vacuous: with the reap neutered, both manager tests fail with "the pump tick never completed the unplug". Behaviour notes: this puts UI_DEV_DESTROY on the GameStream control thread's budget for the first time, and a mask glitch longer than the grace now really does flap — which is SWEEP_GRACE working as documented, so the constant stays. Found by the 2026-08-03 force-feedback sweep (B2 — see the backlog in punktfunk-planning design/haptics-sweep-2026-08-03.md). --- crates/pf-inject/src/inject/linux/gamepad.rs | 5 + crates/pf-inject/src/inject/pad_slots.rs | 116 +++++++++++++++--- crates/pf-inject/src/inject/uhid_manager.rs | 66 +++++++--- .../src/inject/windows/gamepad_windows.rs | 25 ++-- 4 files changed, 171 insertions(+), 41 deletions(-) diff --git a/crates/pf-inject/src/inject/linux/gamepad.rs b/crates/pf-inject/src/inject/linux/gamepad.rs index cac39a69..435cf87d 100644 --- a/crates/pf-inject/src/inject/linux/gamepad.rs +++ b/crates/pf-inject/src/inject/linux/gamepad.rs @@ -669,6 +669,11 @@ impl GamepadManager { /// Service every pad's FF protocol; `send(index, low, high)` is invoked for each pad whose /// mixed rumble level changed. Call frequently (games block in `EVIOCSFF` until answered). pub fn pump_rumble(&mut self, mut send: impl FnMut(u16, u16, u16)) { + // Finish any unplug whose removal frame only armed the grace — the producer sends that + // frame once, so without this the uinput node would outlive the controller. The swept + // mask is discarded because this manager keeps no per-index sibling state (the pads mix + // rumble internally); if that ever changes, consume it like the other two backends do. + self.slots.reap(); for (i, pad) in self.slots.iter_mut() { if let Some((low, high)) = pad.pump_ff() { send(i as u16, low, high); diff --git a/crates/pf-inject/src/inject/pad_slots.rs b/crates/pf-inject/src/inject/pad_slots.rs index 6f595ff6..8538fab7 100644 --- a/crates/pf-inject/src/inject/pad_slots.rs +++ b/crates/pf-inject/src/inject/pad_slots.rs @@ -62,15 +62,30 @@ impl

PadSlots

{ self.label } - /// Drop every allocated pad whose `active_mask` bit has stayed clear for [`SWEEP_GRACE`] (the - /// unplug sweep run on each state frame), logging each. Returns the swept indices as a bitmask - /// so the caller resets its per-index sibling state; an index another manager owns is `None` - /// here, so it is never swept. The grace is the devnode-churn debounce: a mask that glitches - /// clear for a few frames and returns re-arms nothing. + /// Fold one state frame's `active_mask` into the grace clocks, then drop whatever has run out + /// (see [`Self::reap`]). Returns the dropped indices as a bitmask so the caller resets its + /// per-index sibling state; an index another manager owns is `None` here, so it is never + /// touched. The grace is the devnode-churn debounce: a mask that glitches clear for a few + /// frames and returns re-arms nothing. + /// + /// A frame can only ARM the grace, never complete it — no time has passed at the instant the + /// clock starts. Since the producer emits exactly ONE frame per detach, [`Self::reap`] on the + /// manager's periodic pump is what actually finishes the unplug; a backend that only ever + /// called `sweep` would keep the detached pad alive for the rest of the session. pub fn sweep(&mut self, active_mask: u16) -> u16 { self.sweep_at(active_mask, Instant::now()) } + /// Drop every allocated pad whose grace has run out, logging each — the half of the unplug + /// that needs no state frame. Returns the dropped indices as a bitmask, same as [`Self::sweep`]. + /// + /// This can only ever *complete* an unplug some frame already started: it never arms a clock, + /// so however often it runs it cannot drop a pad whose `active_mask` bit never went clear. + /// That is what makes it safe to call from a hot pump loop. + pub fn reap(&mut self) -> u16 { + self.reap_at(Instant::now()) + } + /// Backdate every armed grace clock by [`SWEEP_GRACE`], so the NEXT sweep drops the pads /// whose bits are still clear — consumer tests (the managers') drive the debounce without /// wall-clock sleeps. Test-only: production code has no business expiring the grace. @@ -81,26 +96,37 @@ impl

PadSlots

{ } } - /// [`Self::sweep`] with an injectable clock (unit tests drive the grace window). + /// [`Self::sweep`] with an injectable clock (unit tests drive the grace window): arm or disarm + /// each slot's clock from the mask, then reap whatever has already run out. fn sweep_at(&mut self, active_mask: u16, now: Instant) -> u16 { - let mut swept = 0u16; - for (i, slot) in self.pads.iter_mut().enumerate() { + for i in 0..MAX_PADS { if active_mask & (1 << i) != 0 { self.inactive_since[i] = None; // active (again): a glitch never reaches the drop + } else if self.pads[i].is_some() && self.inactive_since[i].is_none() { + self.inactive_since[i] = Some(now); // newly inactive — start the grace + } + } + self.reap_at(now) + } + + /// [`Self::reap`] with an injectable clock. Deliberately arms nothing — it only ever reads + /// `inactive_since` and clears it, so a pad whose bit never went clear has no clock to run out + /// and cannot be dropped here. + fn reap_at(&mut self, now: Instant) -> u16 { + let mut swept = 0u16; + for i in 0..MAX_PADS { + let Some(since) = self.inactive_since[i] else { + continue; // active, or never went clear — nothing to complete + }; + if self.pads[i].is_none() { + self.inactive_since[i] = None; // the slot went away by some other route continue; } - if slot.is_none() { - continue; - } - match self.inactive_since[i] { - None => self.inactive_since[i] = Some(now), // newly inactive — start the grace - Some(since) if now.duration_since(since) >= SWEEP_GRACE => { - tracing::info!(index = i, "controller unplugged ({})", self.label); - *slot = None; - self.inactive_since[i] = None; - swept |= 1 << i; - } - Some(_) => {} // inside the grace — hold + if now.duration_since(since) >= SWEEP_GRACE { + tracing::info!(index = i, "controller unplugged ({})", self.label); + self.pads[i] = None; + self.inactive_since[i] = None; + swept |= 1 << i; } } swept @@ -161,6 +187,56 @@ mod tests { PadSlots::new("Test", "test pad", "") } + #[test] + fn a_single_frame_plus_a_reap_completes_the_unplug() { + // The shape production actually produces: ONE cleared-mask frame, then time, then a reap + // with no further frame. Before the arm/reap split the pad survived here forever. + let mut s = slots(); + assert!(s.ensure(2, |i| Ok(i as u32))); + assert_eq!( + s.sweep(0b0), + 0, + "a frame arms the grace but cannot itself drop" + ); + assert!(s.get(2).is_some()); + s.expire_grace(); + assert_eq!(s.reap(), 1 << 2, "the reap did not complete the unplug"); + assert!(s.get(2).is_none()); + assert_eq!(s.reap(), 0, "nothing left to reap"); + } + + #[test] + fn reap_never_drops_a_pad_no_frame_ever_deactivated() { + // Reaping COMPLETES an unplug; it must never invent one. A pad whose bit never went clear + // has no armed clock, so any number of reaps — even with the clock backdated — leaves it. + let mut s = slots(); + assert!(s.ensure(0, |i| Ok(i as u32))); + for _ in 0..10 { + assert_eq!(s.reap(), 0); + s.expire_grace(); + } + assert!( + s.get(0).is_some(), + "reap dropped a pad that never went inactive" + ); + } + + #[test] + fn a_glitch_that_returns_inside_the_grace_never_drops_the_pad() { + // The anti-flap guarantee, now that reaps are frequent: a client mask that blips clear and + // comes back must not churn a PnP devnode. + let mut s = slots(); + assert!(s.ensure(0, |i| Ok(i as u32))); + assert_eq!(s.sweep(0b0), 0); // bit clears — arms only + for _ in 0..5 { + assert_eq!(s.reap(), 0, "dropped a pad inside its grace"); + } + assert_eq!(s.sweep(0b1), 0); // the bit returns — disarms + s.expire_grace(); + assert_eq!(s.reap(), 0, "a returned bit must leave nothing armed"); + assert!(s.get(0).is_some()); + } + #[test] fn ensure_creates_once_and_reports_freshness() { let mut s = slots(); diff --git a/crates/pf-inject/src/inject/uhid_manager.rs b/crates/pf-inject/src/inject/uhid_manager.rs index cdb95fd4..8f91f0a9 100644 --- a/crates/pf-inject/src/inject/uhid_manager.rs +++ b/crates/pf-inject/src/inject/uhid_manager.rs @@ -217,13 +217,10 @@ impl UhidManager { if idx >= MAX_PADS { return; } - // Unplugs: drop any allocated pad whose mask bit cleared, resetting its state. + // Unplugs: arm the grace for any pad whose mask bit cleared (the drop itself lands + // on a later `pump` tick — this frame is the only one the producer sends). let swept = self.slots.sweep(f.active_mask); - for i in 0..MAX_PADS { - if swept & (1 << i) != 0 { - self.reset_pad(i); - } - } + self.reset_swept(swept); if f.active_mask & (1 << idx) == 0 { return; // this event WAS the unplug } @@ -282,6 +279,12 @@ impl UhidManager { mut hidout: impl FnMut(HidOutput), ) { let now = Instant::now(); + // Finish any unplug whose removal frame only armed the grace. The producer emits that + // frame exactly once, so without this a detached pad — the single-pad session being the + // common case — would never be destroyed. Runs BEFORE the loop so a reaped index is + // already gone for `get_mut` here and for `heartbeat`'s `get` later in the same tick. + let swept = self.slots.reap(); + self.reset_swept(swept); for i in 0..MAX_PADS { let Some(pad) = self.slots.get_mut(i) else { continue; @@ -360,6 +363,18 @@ impl UhidManager { } } + /// Reset the sibling state of every index a sweep or reap just dropped. Both halves of the + /// unplug land here, so a pad torn down on the pump tick clears exactly what one torn down on + /// a state frame would — in particular `hidout_dedup`, which has no watchdog to re-arm it and + /// would otherwise swallow an identical lightbar/trigger re-assert after a re-plug. + fn reset_swept(&mut self, swept: u16) { + for i in 0..MAX_PADS { + if swept & (1 << i) != 0 { + self.reset_pad(i); + } + } + } + /// Reset one pad's sibling state (on create and unplug) so the first frame/feedback after a /// (re)connect starts from scratch and is always forwarded. fn reset_pad(&mut self, idx: usize) { @@ -494,18 +509,36 @@ mod tests { } #[test] - fn removal_frame_never_recreates_the_pad_it_swept() { + fn one_removal_frame_plus_a_pump_tick_completes_the_unplug() { + // The producer emits the cleared-mask frame exactly ONCE — `native/input.rs` guards it on + // the bit still being set — so the teardown has to finish on the periodic pump. The + // previous version of this test hand-fed a SECOND removal frame, which is what let the + // never-reaped pad hide: with one frame and no pump, the device outlived the session. let mut m = mgr(); m.handle(&frame(1, 0b10, 0)); assert!(m.slots.get(1).is_some()); - // Bit 1 cleared: the first sweep only ARMS the devnode-churn grace — the pad holds (a - // mask glitch must not flap PnP devices; see pad_slots::SWEEP_GRACE). + // The one removal frame: arms the devnode-churn grace, drops nothing. m.handle(&frame(1, 0b00, 0)); assert!(m.slots.get(1).is_some(), "inside the grace — not yet swept"); - // Grace elapsed: the frame IS pad 1's removal — sweep, then early-return (no ensure). + // A tick inside the grace must NOT flap the devnode (pad_slots::SWEEP_GRACE). + m.pump(|_, _, _| {}, |_| {}); + assert!( + m.slots.get(1).is_some(), + "a tick inside the grace dropped it" + ); + // Grace elapsed: the next tick completes the unplug, with no further frame. m.slots.expire_grace(); + m.pump(|_, _, _| {}, |_| {}); + assert!( + m.slots.get(1).is_none(), + "the pump tick never completed the unplug" + ); + // …and a further cleared-mask frame must not resurrect it (the arm branch early-returns). m.handle(&frame(1, 0b00, 0)); - assert!(m.slots.get(1).is_none()); + assert!( + m.slots.get(1).is_none(), + "a cleared-mask frame recreated the pad" + ); } #[test] @@ -551,10 +584,15 @@ mod tests { assert_eq!(collect(&mut m), vec![(0, 100, 0)]); // first value forwards assert_eq!(collect(&mut m), vec![]); // exact repeat deduped assert_eq!(collect(&mut m), vec![(0, 7, 7)]); // change forwards - // Unplug + recreate re-arms the dedup: the same level forwards again. - m.handle(&frame(0, 0b0, 0)); // arms the sweep grace + // Unplug + recreate re-arms the dedup: the same level forwards again. The unplug completes + // on a PUMP tick, not on a second frame — that is all production ever sends. + m.handle(&frame(0, 0b0, 0)); // the one removal frame — arms the grace m.slots.expire_grace(); - m.handle(&frame(0, 0b0, 0)); // grace elapsed — actually swept + assert_eq!(collect(&mut m), vec![]); // this tick reaps; nothing queued to forward + assert!( + m.slots.get(0).is_none(), + "the pump tick completed the unplug" + ); m.handle(&frame(0, 0b1, 0)); *m.backend.feedback.borrow_mut() = vec![rumble((7, 7))]; assert_eq!(collect(&mut m), vec![(0, 7, 7)]); diff --git a/crates/pf-inject/src/inject/windows/gamepad_windows.rs b/crates/pf-inject/src/inject/windows/gamepad_windows.rs index 3cfe7803..d0191c72 100644 --- a/crates/pf-inject/src/inject/windows/gamepad_windows.rs +++ b/crates/pf-inject/src/inject/windows/gamepad_windows.rs @@ -318,14 +318,10 @@ impl GamepadManager { if idx >= MAX_PADS { return; } - // Unplugs: drop any allocated pad whose mask bit cleared. + // Unplugs: arm the grace for any pad whose mask bit cleared (the drop itself lands + // on a later `pump_rumble` tick — this frame is the only one the producer sends). let swept = self.slots.sweep(f.active_mask); - for i in 0..MAX_PADS { - if swept & (1 << i) != 0 { - self.last_rumble[i] = (0, 0); - self.last_active[i] = Instant::now(); - } - } + self.reset_swept(swept); if f.active_mask & (1 << idx) == 0 { return; } @@ -345,10 +341,25 @@ impl GamepadManager { } } + /// Reset the sibling state of every index a sweep or reap just dropped, so both halves of the + /// unplug clear the same things. + fn reset_swept(&mut self, swept: u16) { + for i in 0..MAX_PADS { + if swept & (1 << i) != 0 { + self.last_rumble[i] = (0, 0); + self.last_active[i] = Instant::now(); + } + } + } + /// Relay any changed rumble level to the client. XUSB motors are 0..255; the wire carries /// 0..65535, so scale by 257. `large` (low-frequency) → the datagram's `low`, `small` /// (high-frequency) → `high` — matching the other backends. pub fn pump_rumble(&mut self, mut send: impl FnMut(u16, u16, u16)) { + // Finish any unplug whose removal frame only armed the grace — the producer sends that + // frame once, so without this the XUSB devnode would outlive the controller. + let swept = self.slots.reap(); + self.reset_swept(swept); for (i, pad) in self.slots.iter_mut() { if let Some((large, small)) = pad.service() { // The game drove the pad this poll (SET_STATE bumped the seq) — refresh the