From 35079d1bb9b12ab020c7c35f9926a52bcd039246 Mon Sep 17 00:00:00 2001 From: wav1e <134903554+SLYAPA@users.noreply.github.com> Date: Sun, 25 Oct 2026 17:07:53 +1000 Subject: [PATCH 1/2] Deliver gamepad buttons as held values, not Android actions nativeGamepadButtonEvent's third argument is the button's value, not KeyEvent's action: 1 is held, 0 is released. The engine stores it as the event's value and its input pipeline reads a non-zero value as held. Cordial sent the action, so every button registered on its release and stayed held until its next press. A game that reads a held jump button -- a mobile jump control, which is what the Android client draws -- repeats the jump for as long as the button is held, so one A press left the character jumping without end. That is #113's "the buttons start pressing on their own", and the 8BitDo report behind it. Measured on engine 2.741.0.1061 (the flag at 0x7334cb0 reads 0, so the legacy path runs): with the Roblox in-game menu open, a lone action=0 left the menu untouched and a lone action=1 toggled it, twice over, for B and for Start. On the patched build the menu opens on the press and the in-game Token shop opens on the press, and a real 8BitDo in Bee Swarm produced 24 balanced events, every press action=0 value=1 and every release action=1 value=0. The conversion lives in deliver_gamepad_button, the single delivery point, so callers keep speaking Android's action and devctl padbutton keeps taking down|up. The legacy path also derives a 2*isDown flag from the argument, which this inverts; nothing observable follows that flag, and path A (flag on) uses only the value, so the value is the contract in both. --- crates/cordial-runtime/src/android/input.rs | 49 ++++++++++++++++++++- 1 file changed, 47 insertions(+), 2 deletions(-) diff --git a/crates/cordial-runtime/src/android/input.rs b/crates/cordial-runtime/src/android/input.rs index c06f185e..a7a23bda 100644 --- a/crates/cordial-runtime/src/android/input.rs +++ b/crates/cordial-runtime/src/android/input.rs @@ -1189,16 +1189,51 @@ pub fn deliver_gamepad_disconnect(id: i32) { } } +/// What `nativeGamepadButtonEvent` wants in its third argument: 1 while the +/// button is held, 0 once it is released. +/// +/// **Not** Android's `KeyEvent` action, although the JNI parameter is called +/// `action`. Measured on 2026-10-25 against the Roblox in-game menu on build +/// 2.741.0.1061 (the engine flag at `0x7334cb0` reads 0, so the legacy path +/// runs): a lone `action=0` left the menu untouched, and a lone `action=1` +/// toggled it, twice over, for `B` and for `Start`. The engine stores the +/// argument as the event's `value` and its input pipeline reads a non-zero +/// value as held; the legacy path also derives a `2*isDown` flag from it, but +/// nothing observable follows that flag. +/// +/// Sending Android's convention instead holds every button from its release +/// until its next press. Games read that as a stuck button: #113's "the +/// buttons start pressing on their own", and a character that jumps on release +/// and never stops, which is what a mobile jump button does while it is held. +const GAMEPAD_VALUE_HELD: i32 = 1; +const GAMEPAD_VALUE_RELEASED: i32 = 0; + +/// Translate one Android `KeyEvent` action into the value the engine expects. +pub fn gamepad_button_value(action: i32) -> i32 { + if action == ACTION_DOWN { + GAMEPAD_VALUE_HELD + } else { + GAMEPAD_VALUE_RELEASED + } +} + +/// `nativeGamepadButtonEvent`. `action` is Android's `KeyEvent` action, the +/// convention every caller here uses; the value the engine actually wants is +/// derived by [`gamepad_button_value`]. pub fn deliver_gamepad_button(id: i32, key_code: i32, action: i32) { let f = GAMEPAD_BUTTON.load(std::sync::atomic::Ordering::Relaxed); if f.is_null() { report_unregistered("nativeGamepadButtonEvent"); return; } + let value = gamepad_button_value(action); // SAFETY: `f` is a native resolved via a symbol lookup against the loaded libroblox.so, which is never unloaded. - let r = unsafe { cordial_linker_sys::game_activity::gamepad_button(f, id, key_code, action) }; + let r = unsafe { cordial_linker_sys::game_activity::gamepad_button(f, id, key_code, value) }; if trace_gamepad() { - eprintln!("[cordial] nativeGamepadButton(id={id}, key={key_code}, action={action}) -> {r:?}"); + eprintln!( + "[cordial] nativeGamepadButton(id={id}, key={key_code}, action={action}, \ + value={value}) -> {r:?}" + ); } } @@ -3720,6 +3755,16 @@ mod tests { use super::*; + /// `nativeGamepadButtonEvent` takes a held value, not Android's action: a + /// press is 1 and a release is 0. Delivering the action instead leaves + /// every button held from its release until its next press, which games + /// read as a stuck button (#113). + #[test] + fn a_gamepad_button_is_delivered_as_a_held_value_not_an_android_action() { + assert_eq!(gamepad_button_value(ACTION_DOWN), 1, "a press is held"); + assert_eq!(gamepad_button_value(ACTION_UP), 0, "a release is not held"); + } + /// `devctl text` with no box focused must still release every key it /// pressed. It used to skip the release, leaving the key in `KEYS_HELD`. #[test] From 7ffa4d7da67584f8b60a3a3f521a4cd21b4e59c2 Mon Sep 17 00:00:00 2001 From: wav1e <134903554+SLYAPA@users.noreply.github.com> Date: Sun, 25 Oct 2026 17:07:53 +1000 Subject: [PATCH 2/2] Release the buttons a gamepad was holding when it goes away A pad that is unplugged, or a joydev node that disappears, left the engine believing every button it had down was still down. An unplug gets its releases from the kernel, which emits one per held key when a device is destroyed (measured: A held on a destroyed uinput pad arrives as key=96, action=1, before ENODEV). Switching controllers off in Settings only closes the file, and then nothing sends the release at all: the engine keeps the button pressed for the rest of the run, which in a game reads as the button pressing itself long after it was let go. Track the buttons each pad is holding in a bitmask (Pad.held) and, on withdraw, send one release per named button before the disconnect. The mask follows the event that was actually sent, so a button the layout cannot name is never released later either. Indices past 31 are not tracked; no pad here declares more than sixteen buttons. A pad opened under a finger is also out of step, in the other direction: its init burst is the pad's current state, and the pressed records in it were dropped with the rest of the burst. Replay the ones that are down after the announcement, and only those -- every pad opens with most buttons up, and eleven no-op releases per plug-in are noise in the trace, not information. Axes are left out on purpose: a stick that is already off centre corrects itself on its next event, and a trigger at rest reads full-scale on some pads. Four unit tests: the mask follows press and release, release_held sends one up per named button and clears, a button held at withdraw is released before the disconnect, and an init burst replays only the buttons that are down. --- crates/cordial-runtime/src/android/gamepad.rs | 272 +++++++++++++++++- 1 file changed, 258 insertions(+), 14 deletions(-) diff --git a/crates/cordial-runtime/src/android/gamepad.rs b/crates/cordial-runtime/src/android/gamepad.rs index 9b6cbbd0..a3c3117e 100644 --- a/crates/cordial-runtime/src/android/gamepad.rs +++ b/crates/cordial-runtime/src/android/gamepad.rs @@ -207,11 +207,27 @@ pub fn current_enabled() -> bool { /// and disconnecting an id it has not heard of is the kind of call that has no /// reason to be safe. /// -/// `disconnect` is a parameter only so the test does not need an engine. -fn withdraw(pads: &mut Vec, mut disconnect: impl FnMut(i32)) -> usize { +/// `button` and `disconnect` are parameters only so the test does not need an +/// engine; `button` is the release that has to go out before the disconnect. +fn withdraw( + pads: &mut Vec, + mut button: impl FnMut(i32, i32, i32), + mut disconnect: impl FnMut(i32), +) -> usize { let mut sent = 0; - for pad in pads.drain(..) { + for mut pad in pads.drain(..) { if pad.announced { + // **A release the pad never got to send has to be sent for it.** + // This is the path where the device is still there and Cordial + // simply stops reading it, so the kernel does not help: it emits + // releases for held keys when a device is *destroyed* (an unplug or + // a uinput teardown), and closing the file is not that. The engine + // is left with the button down, and the game acts on it -- a + // character that keeps jumping with the controllers switched off is + // what that looks like. INFERRED: that the engine keeps button + // state across a disconnect. Sending the release first is correct + // either way, and it costs one call per held button. + release_held(&mut pad, &mut button); disconnect(pad.id); sent += 1; } @@ -219,6 +235,25 @@ fn withdraw(pads: &mut Vec, mut disconnect: impl FnMut(i32)) -> usize { sent } +/// Tell the engine that every button this pad still has down has come up. +/// +/// Called on every path that drops a pad, *before* the disconnect. On an unplug +/// the kernel has already emitted these releases itself (measured: a uinput pad +/// destroyed with A held arrives as `key=96, action=1` before `ENODEV`), so this +/// is usually a no-op. It is the paths that close the file without destroying +/// the device -- the controllers switch in Settings -- where the release would +/// otherwise never be sent. +fn release_held(pad: &mut Pad, button: &mut impl FnMut(i32, i32, i32)) { + let mut held = std::mem::take(&mut pad.held); + while held != 0 { + let index = held.trailing_zeros() as u8; + held &= held - 1; + if let Some(code) = pad.layout.button(index) { + button(pad.id, code, ACTION_UP); + } + } +} + /// The gate's decision, separated from where it reads it. /// /// Split out only so it can be tested: `enabled` caches into a `OnceLock`, so a @@ -347,6 +382,20 @@ struct Pad { /// What the axes last said, so that one stick's two axes can go to the engine /// together. See [`AxisState`]. sticks: AxisState, + /// The joydev button indices this pad has told the engine are down, one bit + /// each. + /// + /// **Kept so the release can be sent when Cordial lets go of the pad.** The + /// engine holds a button until it is told the button came up. An unplug gets + /// that release from the kernel, which emits one for every held key when a + /// device is destroyed -- measured here, and it arrives before `ENODEV`. But + /// switching controllers off in Settings only closes the file, and then + /// nothing sends the release: the engine keeps the button pressed for the + /// rest of the run, which in a game reads as the button "pressing itself" + /// long after it was let go. A bitmask rather than a list because the release + /// is then a loop over set bits, and `u32` because joydev indices are small: + /// no pad here declares more than sixteen buttons. + held: u32, } static PADS: std::sync::Mutex> = std::sync::Mutex::new(Vec::new()); @@ -1166,11 +1215,44 @@ fn drain(pad: &mut Pad) -> bool { // input pump. No real pad has 256 buttons, but "no real // device does that" is not a reason to let a byte off a // character device decide whether the client stays up. - (true, JS_EVENT_BUTTON) => init_buttons = init_buttons.max(index.saturating_add(1)), - (true, JS_EVENT_AXIS) => init_axes = init_axes.max(index.saturating_add(1)), - (false, k) if pad.announced => { - dispatch(pad.id, k, index, value, &pad.layout, &mut pad.sticks) + (true, JS_EVENT_BUTTON) => { + init_buttons = init_buttons.max(index.saturating_add(1)); + // **A button held when the device opens is really + // held, and the engine has to be told so.** The init + // burst is the pad's current state, not a press: it is + // read here as the starting state of a pad that has + // just been opened, and a pad can be opened under a + // finger. Forwarding the pressed ones after the + // announcement is what stops a re-opened pad from + // being out of step with the engine -- a button the + // engine has down with nothing left to release it, or + // (with the release on the way out) one the engine has + // up while the pad has it down. Zero values are + // dropped: every pad opens with most buttons up, and + // eleven no-op releases per plug-in are noise in the + // trace, not information. + if let Some(record) = replay_init(JS_EVENT_BUTTON, index, value) { + deferred.push(record); + } } + (true, JS_EVENT_AXIS) => { + init_axes = init_axes.max(index.saturating_add(1)); + // Axes are left out on purpose. A stick that is + // already off centre when the pad opens corrects + // itself on its next event, and the init value of a + // trigger at rest is a full-scale number on some pads + // -- sending that as a trigger pull would be a worse + // guess than waiting. + } + (false, k) if pad.announced => dispatch( + pad.id, + k, + index, + value, + &pad.layout, + &mut pad.sticks, + &mut pad.held, + ), (false, k) => deferred.push((k, index, value)), _ => {} } @@ -1188,7 +1270,15 @@ fn drain(pad: &mut Pad) -> bool { announce(pad.id, pad.node, init_buttons, init_axes, &pad.layout); pad.announced = true; for (k, index, value) in deferred { - dispatch(pad.id, k, index, value, &pad.layout, &mut pad.sticks); + dispatch( + pad.id, + k, + index, + value, + &pad.layout, + &mut pad.sticks, + &mut pad.held, + ); } } else if !pad.announced && !deferred.is_empty() { // Events arriving from a device that never sent an init burst, which @@ -1210,14 +1300,24 @@ fn drain(pad: &mut Pad) -> bool { true } -fn dispatch(id: i32, kind: u8, index: u8, value: i16, layout: &Layout, sticks: &mut AxisState) { +fn dispatch( + id: i32, + kind: u8, + index: u8, + value: i16, + layout: &Layout, + sticks: &mut AxisState, + held: &mut u32, +) { match kind { JS_EVENT_BUTTON => { if let Some(code) = layout.button(index) { + let down = value != 0; + track_button(held, index, down); input::deliver_gamepad_button( id, code, - if value != 0 { ACTION_DOWN } else { ACTION_UP }, + if down { ACTION_DOWN } else { ACTION_UP }, ); } } @@ -1226,6 +1326,45 @@ fn dispatch(id: i32, kind: u8, index: u8, value: i16, layout: &Layout, sticks: & } } +/// Record what the engine has just been told about one button. +/// +/// The bitmask is this side's record of the engine's state, so it follows the +/// event that was actually sent rather than the device: a button whose code the +/// layout cannot name is never sent, and must not be released later either. +/// Indices past 31 are not tracked -- joydev's init burst can in principle +/// declare that many, no pad here does, and dropping the bit only means that +/// button would not be released when the pad goes away. +fn track_button(held: &mut u32, index: u8, down: bool) { + let Some(bit) = 1u32.checked_shl(index.into()) else { + return; + }; + if down { + *held |= bit; + } else { + *held &= !bit; + } +} + +/// What an init-burst record is still worth sending once the pad is announced. +/// +/// **A button held when the device opens is really held**, and the engine has to +/// be told: the pad can be opened under a finger, or re-opened under a new node +/// after a re-enumeration, and a pad that is out of step with the engine has a +/// button down that nothing will release (or, after a release was sent on the +/// way out, a button up that the pad has down). The init burst is the pad's +/// current state, so the pressed buttons are replayed as ordinary events after +/// the declaration. Zero values are dropped: every pad opens with most buttons +/// up, and eleven no-op releases per plug-in are noise in the trace, not +/// information. +/// +/// Axes are left out on purpose. A stick that is already off centre when the pad +/// opens corrects itself on its next event, and a trigger at rest reads as a +/// full-scale value on some pads -- sending that as a trigger pull would be a +/// worse guess than waiting. +fn replay_init(kind: u8, index: u8, value: i16) -> Option<(u8, u8, i16)> { + (kind == JS_EVENT_BUTTON && value != 0).then_some((kind, index, value)) +} + /// One tick's worth of gamepad, called from [`super::looper::pump`] beside /// `input::idle_keepalive`. /// @@ -1244,7 +1383,11 @@ pub fn poll() { if !enabled() { if WITHDRAW.swap(false, Ordering::Relaxed) { let mut pads = PADS.lock().unwrap_or_else(|e| e.into_inner()); - let n = withdraw(&mut pads, input::deliver_gamepad_disconnect); + let n = withdraw( + &mut pads, + input::deliver_gamepad_button, + input::deliver_gamepad_disconnect, + ); if n > 0 { eprintln!( "[cordial] gamepad: switched off; {n} controller(s) disconnected from the engine" @@ -1298,6 +1441,7 @@ pub fn poll() { announced: false, layout: read_layout(node), sticks: AxisState::default(), + held: 0, }); } } @@ -1311,6 +1455,12 @@ pub fn poll() { } for id in gone { let node = pads.iter().find(|p| p.id == id).map(|p| p.node); + // The device is gone, so the release events it owes the engine will + // never arrive. Sending them now is the only thing that keeps a button + // held at unplug time from staying pressed for the rest of the run. + if let Some(pad) = pads.iter_mut().find(|p| p.id == id) { + release_held(pad, &mut input::deliver_gamepad_button); + } pads.retain(|p| p.id != id); input::deliver_gamepad_disconnect(id); if input::trace_gamepad() { @@ -1375,7 +1525,7 @@ pub fn synthetic_axis(id: i32, index: u8, value: i16) { #[cfg(test)] mod tests { - use super::{enabled_for, free_slot, js_node, withdraw, Pad}; + use super::{enabled_for, free_slot, js_node, release_held, withdraw, Pad}; fn pad(id: i32, announced: bool) -> Pad { // A real file descriptor that never produces an event; `withdraw` only @@ -1387,6 +1537,7 @@ mod tests { announced, layout: super::Layout::standard(), sticks: super::AxisState::default(), + held: 0, } } @@ -1417,7 +1568,7 @@ mod tests { fn switching_off_disconnects_each_announced_pad_once_and_lets_go_of_all() { let mut pads = vec![pad(0, true), pad(1, false), pad(2, true)]; let mut told = Vec::new(); - let n = withdraw(&mut pads, |id| told.push(id)); + let n = withdraw(&mut pads, |_, _, _| {}, |id| told.push(id)); assert_eq!(told, vec![0, 2], "pad 1 was never announced, so the engine has never heard of it"); assert_eq!(n, 2); assert!(pads.is_empty(), "the device files are released, not kept open while off"); @@ -1425,10 +1576,103 @@ mod tests { // Control: nothing held, nothing sent. A second tick while still off is // a no-op rather than a second disconnect for the same ids. let mut again = Vec::new(); - assert_eq!(withdraw(&mut pads, |id| again.push(id)), 0); + assert_eq!(withdraw(&mut pads, |_, _, _| {}, |id| again.push(id)), 0); assert!(again.is_empty()); } + /// **A button held when Cordial lets go of the pad has to be released.** + /// + /// The engine holds a button until it is told the button came up. An unplug + /// gets that from the kernel; switching controllers off in Settings does not, + /// because the device is not destroyed, only closed. Order matters too: the + /// release has to reach the engine before the disconnect, and the pad's id + /// may be reused by the next pad. INFERRED: that the engine keeps button + /// state across a disconnect. + #[test] + fn a_button_held_when_the_pad_goes_away_is_released_before_the_disconnect() { + let mut held = pad(3, true); + // Indices 0 and 2 down, 1 up: only the two down ones may be released. + held.held = 0b101; + let mut pads = vec![held]; + // One log behind a `RefCell` because `withdraw` takes both callbacks at + // once and the ordering between them is the point. + let log = std::cell::RefCell::new(Vec::::new()); + withdraw( + &mut pads, + |id, key, action| log.borrow_mut().push(format!("button {id} {key} {action}")), + |id| log.borrow_mut().push(format!("disconnect {id}")), + ); + assert_eq!( + log.into_inner(), + vec![ + // The standard layout's buttons 0 and 2 are KEYCODE_BUTTON_A + // and _C; ACTION_UP is 1. + "button 3 96 1".to_string(), + "button 3 98 1".to_string(), + "disconnect 3".to_string(), + ], + "the held buttons are released, once each, before the disconnect" + ); + assert!(pads.is_empty()); + } + + /// The release loop is driven by the bitmask, not by the layout, so an + /// index the layout cannot name is skipped rather than sent as a wrong key. + #[test] + fn release_held_sends_one_up_per_named_button_and_clears_the_mask() { + let mut p = pad(1, true); + p.held = (1 << 20) | 1; // index 0, and one past the standard table's 15 + let mut calls: Vec<(i32, i32, i32)> = Vec::new(); + release_held(&mut p, &mut |id, key, action| calls.push((id, key, action))); + assert_eq!(calls, vec![(1, 96, super::ACTION_UP)], "only the named button is sent"); + assert_eq!(p.held, 0, "cleared, so a second path cannot release the same button twice"); + + // Control: nothing held, nothing sent. + let mut again = Vec::new(); + release_held(&mut p, &mut |id, key, action| again.push((id, key, action))); + assert!(again.is_empty()); + } + + /// The bitmask has to follow the events the engine is sent, in both + /// directions, or the release on the way out names the wrong buttons. + #[test] + fn a_button_bit_follows_the_press_and_the_release() { + use super::track_button; + let mut held = 0u32; + track_button(&mut held, 0, true); + assert_eq!(held, 1, "button 0 down is bit 0"); + track_button(&mut held, 3, true); + assert_eq!(held, 0b1001); + track_button(&mut held, 0, false); + assert_eq!(held, 0b1000, "only the button that came up is cleared"); + track_button(&mut held, 3, false); + assert_eq!(held, 0); + // Control: an index past the mask cannot set a bit, and must not wrap + // into one that a release would then send for the wrong button. + track_button(&mut held, 32, true); + track_button(&mut held, 255, true); + assert_eq!(held, 0); + } + + /// The init burst is the pad's state at open, and the pressed buttons are + /// the part of it that has to reach the engine -- a pad re-opened under a + /// finger must not be out of step with it. Zero values and axes are dropped. + #[test] + fn an_init_burst_replays_only_the_buttons_that_are_down() { + use super::{replay_init, JS_EVENT_AXIS, JS_EVENT_BUTTON}; + assert_eq!(replay_init(JS_EVENT_BUTTON, 2, 1), Some((JS_EVENT_BUTTON, 2, 1))); + assert_eq!( + replay_init(JS_EVENT_BUTTON, 2, 0), + None, + "a button that is up at open needs no release sent" + ); + assert_eq!( + replay_init(JS_EVENT_AXIS, 1, -32767), + None, + "an axis at open is left to its next event" + ); + } + /// The setting is a real switch now, in both directions, and the launch /// environment only seeds it. #[test]