From 674d52112c2237fc06f0683483e64a8859820c03 Mon Sep 17 00:00:00 2001 From: Jonathan Davies Date: Tue, 12 May 2026 23:00:27 +0000 Subject: [PATCH] fix(morse): drop stale held-buffer entry before flow-tap re-insert When a morse key whose tap action equals its hold-after-tap action is tapped and then pressed again within the flow-tap idle window, the first tap leaves an EarlyFired entry in the held buffer (the early-fire optimisation). The flow-tap branch then pushed a second entry at the same position instead of replacing it, so the buffer held two entries for one key. On release, find_pos_mut returned the stale EarlyFired entry first, hit its no-op match arm, and never emitted the release report, leaving the key stuck down. Remove any existing entry at the position before pushing the new one so the held buffer keeps its one-entry-per-position invariant. Adds a regression test covering the tap then flow-tap-repeat sequence. --- rmk/src/keyboard.rs | 6 +- rmk/tests/keyboard_morse_tap_dance_test.rs | 75 ++++++++++++++++++++++ 2 files changed, 80 insertions(+), 1 deletion(-) diff --git a/rmk/src/keyboard.rs b/rmk/src/keyboard.rs index fbb8ba344..d6d66ad11 100644 --- a/rmk/src/keyboard.rs +++ b/rmk/src/keyboard.rs @@ -440,7 +440,11 @@ impl<'a> Keyboard<'a> { KeyBehaviorDecision::FlowTap => { let action = Self::action_from_pattern(self.keymap, key_action, TAP); //tap action self.process_key_action_normal(action, event).await; - // Push back after triggered press + // Drop any existing held entry at this position (e.g. an EarlyFired entry left by + // a previous quick tap) before inserting the new one. The held buffer assumes one + // entry per position; without this the release handler would find the stale entry + // first via find_pos_mut, skip the release report, and leave the key stuck down. + self.held_buffer.remove_if(|k| k.event.pos == event.pos); let now = Instant::now(); let time_out = now + Self::morse_timeout(self.keymap, key_action, true); self.held_buffer.push(HeldKey::new( diff --git a/rmk/tests/keyboard_morse_tap_dance_test.rs b/rmk/tests/keyboard_morse_tap_dance_test.rs index 117ea3508..198f5ff37 100644 --- a/rmk/tests/keyboard_morse_tap_dance_test.rs +++ b/rmk/tests/keyboard_morse_tap_dance_test.rs @@ -1,6 +1,7 @@ /// Test cases for tap-dance like morses pub mod common; +use embassy_time::Duration; use heapless::Vec; use rmk::config::{BehaviorConfig, MorsesConfig, PositionalConfig}; use rmk::keyboard::Keyboard; @@ -575,3 +576,77 @@ fn test_early_fire_then_fire_on_second_tap_with_no_double_tap_config() { ] }; } + +/// Regression test: rapid repeat tapping a FlowTap+EarlyFire key must not jam the key. +/// +/// When flow_tap is enabled and a key has early-fire behaviour (tap == hold_after_tap, +/// no double_tap), the first quick tap fires the action immediately and leaves the key +/// in `EarlyFired` state in the held buffer. A second tap that arrives within +/// `prior_idle_time` triggers `FlowTap`, which sends the key-press report and pushes a +/// new `ProcessedButReleaseNotReportedYet` entry; without the fix it would push on +/// top of the stale `EarlyFired` entry. On release `find_pos_mut` would then find the +/// `EarlyFired` entry first and skip the release report, leaving the key held down (jam). +/// +/// The fix drops any existing held entry at this position before pushing in the +/// `FlowTap` handler, so the buffer keeps its one-entry-per-position invariant and +/// the release is always reported. +fn create_flow_tap_early_fire_keyboard() -> Keyboard<'static> { + // td!(0): tap=Backspace, hold=RShift, hold_after_tap=Backspace (no double_tap). + // tap == hold_after_tap with no double_tap makes can_fire_early() return true for TAP. + let keymap = [[[td!(0), k!(A)]], [[k!(Kp1), k!(Kp2)]]]; + + let behavior_config = BehaviorConfig { + morse: MorsesConfig { + enable_flow_tap: true, + prior_idle_time: Duration::from_millis(120), + default_profile: MorseProfile::new(Some(false), Some(MorseMode::HoldOnOtherPress), Some(250), Some(250)), + morses: Vec::from_slice(&[Morse::new_from_vial( + Action::Key(KeyCode::Hid(HidKeyCode::Backspace)), + Action::Modifier(ModifierCombination::RSHIFT), + Action::Key(KeyCode::Hid(HidKeyCode::Backspace)), + Action::No, + MorseProfile::const_default(), + )]) + .unwrap(), + ..Default::default() + }, + ..Default::default() + }; + + let behavior_config: &'static mut BehaviorConfig = Box::leak(Box::new(behavior_config)); + let per_key_config: &'static PositionalConfig<1, 2> = Box::leak(Box::new(PositionalConfig::default())); + Keyboard::new(wrap_keymap(keymap, per_key_config, behavior_config)) +} + +#[test] +fn test_flow_tap_after_early_fire_does_not_jam() { + key_sequence_test! { + keyboard: create_flow_tap_early_fire_keyboard(), + sequence: [ + // First tap arrives after >prior_idle_time of idle, so it takes the normal morse + // path: a quick release early-fires Backspace and leaves an EarlyFired entry behind. + [0, 0, true, 150], + [0, 0, false, 30], + // Second tap lands within prior_idle_time of the early-fired Backspace press, so it + // takes the FlowTap path. FlowTap must replace the stale EarlyFired entry, not stack + // a new one on top of it, or the release report below is dropped and Backspace jams. + [0, 0, true, 50], + [0, 0, false, 30], + // Press an unrelated key well after the morse gap timeout to confirm nothing is stuck. + [0, 1, true, 300], + [0, 1, false, 10], + ], + expected_reports: [ + // First tap: early-fired Backspace press, then release 10ms later (process_key_action_tap). + [0, [kc_to_u8!(Backspace), 0, 0, 0, 0, 0]], + [0, [0, 0, 0, 0, 0, 0]], + // Second tap via FlowTap: Backspace press, then release on key-up. + // The release report was missing before the fix (Backspace stayed held -> jam). + [0, [kc_to_u8!(Backspace), 0, 0, 0, 0, 0]], + [0, [0, 0, 0, 0, 0, 0]], + // Unrelated key A, cleanly pressed and released. + [0, [kc_to_u8!(A), 0, 0, 0, 0, 0]], + [0, [0, 0, 0, 0, 0, 0]], + ] + }; +}