Skip to content

fix(osm): keep OSM/OSL timeout alive across non-consuming events #866

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
yekingyan wants to merge 3 commits into
base: main
Choose a base branch
from
Open

fix(osm): keep OSM/OSL timeout alive across non-consuming events #866

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d22415f
fix(osm): keep OSM/OSL timeout alive across non-consuming events
yekingyan Jun 12, 2026
055fd99
test(osm): add regression test for timeout cancelled by non-consuming…
yekingyan Jun 12, 2026
0610cb4
fix(osl): use the layer bound in the match arm instead of the outer p…
yekingyan Jul 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 65 additions & 23 deletions rmk/src/keyboard/oneshot.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use embassy_futures::select::{Either, select};
use embassy_time::Timer;
use embassy_time::{Instant, Timer};
use rmk_types::modifier::ModifierCombination;

use crate::event::KeyboardEvent;
Expand Down Expand Up @@ -72,22 +72,48 @@ impl<'a> Keyboard<'a> {
match self.osm_state {
OneShotState::Initial(cur_modifiers) | OneShotState::Single(cur_modifiers) => {
self.osm_state = OneShotState::Single(cur_modifiers);
let timeout = Timer::after(self.keymap.one_shot_timeout());
match select(timeout, self.keyboard_event_subscriber.next_message_pure()).await {
Either::First(_) => {
// Timeout, release modifiers
self.update_osl(event);
self.osm_state = OneShotState::None;

// Send release report because modifiers were held
if activate_on_keypress {
self.send_keyboard_report_with_resolved_modifiers(false).await;
let quick_release = self.keymap.one_shot_modifiers_config().quick_release;

// If unprocessed_events already contains a consuming event, skip the
// await loop — waiting on the subscriber would miss it because events
// already dequeued from the channel live only in unprocessed_events.
let already_has_consumer = self
.unprocessed_events
.iter()
.any(|e| (quick_release && e.pressed) || (!quick_release && !e.pressed));

if !already_has_consumer {
let deadline = Instant::now() + self.keymap.one_shot_timeout();
loop {
let now = Instant::now();
if now >= deadline {
self.update_osl(event);
self.osm_state = OneShotState::None;
if activate_on_keypress {
self.send_keyboard_report_with_resolved_modifiers(false).await;
}
break;
}
}
Either::Second(e) => {
// New event, send it to queue
if self.unprocessed_events.push(e).is_err() {
warn!("Unprocessed event queue is full, dropping event");
let timeout = Timer::after(deadline - now);
match select(timeout, self.keyboard_event_subscriber.next_message_pure()).await {
Either::First(_) => {
self.update_osl(event);
self.osm_state = OneShotState::None;
if activate_on_keypress {
self.send_keyboard_report_with_resolved_modifiers(false).await;
}
break;
}
Either::Second(e) => {
if self.unprocessed_events.push(e).is_err() {
warn!("Unprocessed event queue is full, dropping event");
}
// If this event would consume the OSM, stop waiting
if (quick_release && e.pressed) || (!quick_release && !e.pressed) {
break;
}
// Non-consuming event (e.g. layer key release), keep waiting
}
}
}
}
Expand Down Expand Up @@ -136,17 +162,33 @@ impl<'a> Keyboard<'a> {
OneShotState::Initial(l) | OneShotState::Single(l) => {
self.osl_state = OneShotState::Single(l);

let timeout = embassy_time::Timer::after(self.keymap.one_shot_timeout());
match select(timeout, self.keyboard_event_subscriber.next_message_pure()).await {
Either::First(_) => {
let deadline = Instant::now() + self.keymap.one_shot_timeout();
loop {
let now = Instant::now();
if now >= deadline {
// Timeout, deactivate layer
self.keymap.deactivate_layer(layer_num);
self.osl_state = OneShotState::None;
break;
}
Either::Second(e) => {
// New event, send it to queue
if self.unprocessed_events.push(e).is_err() {
warn!("Unprocessed event queue is full, dropping event");
let timeout = Timer::after(deadline - now);
match select(timeout, self.keyboard_event_subscriber.next_message_pure()).await {
Either::First(_) => {
// Timeout, deactivate layer
self.keymap.deactivate_layer(layer_num);

@HaoboGu HaoboGu Jun 13, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should layer_num be l in OneShotState::Initial(l) | OneShotState::Single(l)?

@yekingyan yekingyan Jul 3, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should layer_num be l in OneShotState::Initial(l) | OneShotState::Single(l)?

Good catch — should be l.

Just pushed a fix (0610cb4c) that uses the match-bound l instead of the outer layer_num parameter.

They're numerically equal on every current call path, so there's no behavior change today, but l is the semantically correct value and it's more robust if that invariant ever changes.

self.osl_state = OneShotState::None;
break;
}
Either::Second(e) => {
// New event, send it to queue
if self.unprocessed_events.push(e).is_err() {
warn!("Unprocessed event queue is full, dropping event");
}
// A key press consumes the one-shot layer.
if e.pressed {
break;
}
// Release events (e.g. layer key release) don't consume, keep waiting
}
}
}
Expand Down
35 changes: 33 additions & 2 deletions rmk/tests/keyboard_one_shot_test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -658,8 +658,39 @@ mod one_shot_test {
};
}

// TODO: test_osm_quick_release_rolling removed — OSM + morse/tap-hold interaction
// has a known bug where the OSM deadline loop times out before the tap resolves.
// NOTE: test_osm_quick_release_rolling is intentionally omitted. With a
// tap-hold key the OSM modifier is cleared before the buffered morse tap
// resolves, so the tapped key is emitted without the modifier. That is a
// separate OSM + morse/tap-hold timing issue (not a timeout bug).

/// Regression test: a non-consuming event arriving during the OSM release
/// timeout must NOT cancel that timeout. Previously the single
/// `select(timeout, next_event)` gave up the timeout on the first event,
/// leaving the OSM stuck in `Single` forever. Here an OSL press interrupts
/// the wait; after the timeout elapses the OSM must have auto-released, so
/// the later layer-1 key is sent WITHOUT the one-shot modifier.
#[test]
fn test_osm_timeout_not_cancelled_by_non_consuming_event() {
key_sequence_test! {
keyboard: create_test_keyboard_with_behavior_config(BehaviorConfig {
one_shot: OneShotConfig {
timeout: Duration::from_millis(100),
},
..BehaviorConfig::default()
}),
sequence: [
[0, 0, true, 10], // Press OSM LShift
[0, 0, false, 10], // Release OSM LShift -> Single, start timeout
[0, 1, true, 10], // Press OSL(1): non-consuming event during timeout
[0, 2, true, 200], // After timeout elapses, press layer-1 key C
[0, 2, false, 10], // Release C
],
expected_reports: [
[0, [kc_to_u8!(C), 0, 0, 0, 0, 0]], // C WITHOUT LShift (OSM timed out)
[0, [0, 0, 0, 0, 0, 0]], // All released
]
};
}

#[test]
fn test_osm_quick_release_combined_modifiers() {
Expand Down