Skip to content

Commit b95ecc2

Browse files
committed
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.
1 parent f8210e6 commit b95ecc2

2 files changed

Lines changed: 80 additions & 1 deletion

File tree

rmk/src/keyboard.rs

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -440,7 +440,11 @@ impl<'a> Keyboard<'a> {
440440
KeyBehaviorDecision::FlowTap => {
441441
let action = Self::action_from_pattern(self.keymap, key_action, TAP); //tap action
442442
self.process_key_action_normal(action, event).await;
443-
// Push back after triggered press
443+
// Drop any existing held entry at this position (e.g. an EarlyFired entry left by
444+
// a previous quick tap) before inserting the new one. The held buffer assumes one
445+
// entry per position; without this the release handler would find the stale entry
446+
// first via find_pos_mut, skip the release report, and leave the key stuck down.
447+
self.held_buffer.remove_if(|k| k.event.pos == event.pos);
444448
let now = Instant::now();
445449
let time_out = now + Self::morse_timeout(self.keymap, key_action, true);
446450
self.held_buffer.push(HeldKey::new(

rmk/tests/keyboard_morse_tap_dance_test.rs

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
/// Test cases for tap-dance like morses
22
pub mod common;
33

4+
use embassy_time::Duration;
45
use heapless::Vec;
56
use rmk::config::{BehaviorConfig, MorsesConfig, PositionalConfig};
67
use rmk::keyboard::Keyboard;
@@ -575,3 +576,77 @@ fn test_early_fire_then_fire_on_second_tap_with_no_double_tap_config() {
575576
]
576577
};
577578
}
579+
580+
/// Regression test: rapid repeat tapping a FlowTap+EarlyFire key must not jam the key.
581+
///
582+
/// When flow_tap is enabled and a key has early-fire behaviour (tap == hold_after_tap,
583+
/// no double_tap), the first quick tap fires the action immediately and leaves the key
584+
/// in `EarlyFired` state in the held buffer. A second tap that arrives within
585+
/// `prior_idle_time` triggers `FlowTap`, which sends the key-press report and pushes a
586+
/// new `ProcessedButReleaseNotReportedYet` entry; without the fix it would push on
587+
/// top of the stale `EarlyFired` entry. On release `find_pos_mut` would then find the
588+
/// `EarlyFired` entry first and skip the release report, leaving the key held down (jam).
589+
///
590+
/// The fix drops any existing held entry at this position before pushing in the
591+
/// `FlowTap` handler, so the buffer keeps its one-entry-per-position invariant and
592+
/// the release is always reported.
593+
fn create_flow_tap_early_fire_keyboard() -> Keyboard<'static> {
594+
// td!(0): tap=Backspace, hold=RShift, hold_after_tap=Backspace (no double_tap).
595+
// tap == hold_after_tap with no double_tap makes can_fire_early() return true for TAP.
596+
let keymap = [[[td!(0), k!(A)]], [[k!(Kp1), k!(Kp2)]]];
597+
598+
let behavior_config = BehaviorConfig {
599+
morse: MorsesConfig {
600+
enable_flow_tap: true,
601+
prior_idle_time: Duration::from_millis(120),
602+
default_profile: MorseProfile::new(Some(false), Some(MorseMode::HoldOnOtherPress), Some(250), Some(250)),
603+
morses: Vec::from_slice(&[Morse::new_from_vial(
604+
Action::Key(KeyCode::Hid(HidKeyCode::Backspace)),
605+
Action::Modifier(ModifierCombination::RSHIFT),
606+
Action::Key(KeyCode::Hid(HidKeyCode::Backspace)),
607+
Action::No,
608+
MorseProfile::const_default(),
609+
)])
610+
.unwrap(),
611+
..Default::default()
612+
},
613+
..Default::default()
614+
};
615+
616+
let behavior_config: &'static mut BehaviorConfig = Box::leak(Box::new(behavior_config));
617+
let per_key_config: &'static PositionalConfig<1, 2> = Box::leak(Box::new(PositionalConfig::default()));
618+
Keyboard::new(wrap_keymap(keymap, per_key_config, behavior_config))
619+
}
620+
621+
#[test]
622+
fn test_flow_tap_after_early_fire_does_not_jam() {
623+
key_sequence_test! {
624+
keyboard: create_flow_tap_early_fire_keyboard(),
625+
sequence: [
626+
// First tap arrives after >prior_idle_time of idle, so it takes the normal morse
627+
// path: a quick release early-fires Backspace and leaves an EarlyFired entry behind.
628+
[0, 0, true, 150],
629+
[0, 0, false, 30],
630+
// Second tap lands within prior_idle_time of the early-fired Backspace press, so it
631+
// takes the FlowTap path. FlowTap must replace the stale EarlyFired entry, not stack
632+
// a new one on top of it, or the release report below is dropped and Backspace jams.
633+
[0, 0, true, 50],
634+
[0, 0, false, 30],
635+
// Press an unrelated key well after the morse gap timeout to confirm nothing is stuck.
636+
[0, 1, true, 300],
637+
[0, 1, false, 10],
638+
],
639+
expected_reports: [
640+
// First tap: early-fired Backspace press, then release 10ms later (process_key_action_tap).
641+
[0, [kc_to_u8!(Backspace), 0, 0, 0, 0, 0]],
642+
[0, [0, 0, 0, 0, 0, 0]],
643+
// Second tap via FlowTap: Backspace press, then release on key-up.
644+
// The release report was missing before the fix (Backspace stayed held -> jam).
645+
[0, [kc_to_u8!(Backspace), 0, 0, 0, 0, 0]],
646+
[0, [0, 0, 0, 0, 0, 0]],
647+
// Unrelated key A, cleanly pressed and released.
648+
[0, [kc_to_u8!(A), 0, 0, 0, 0, 0]],
649+
[0, [0, 0, 0, 0, 0, 0]],
650+
]
651+
};
652+
}

0 commit comments

Comments
 (0)