From 42e607fa895e710d813835a6091a21248179da43 Mon Sep 17 00:00:00 2001 From: John Schock Date: Wed, 29 Apr 2026 12:01:35 -0700 Subject: [PATCH 1/3] UefiHidDxeV2: Add keystroke repeat functionality Implement keyboard repeat for held keys, matching the behavior of the legacy C HidKeyboardDxe driver: - 500ms initial delay before repeat begins - 20ms repeat rate (~50 keys/sec) while key is held - Modifier keys, toggle keys, and non-spacing (dead) keys are excluded - Last newly pressed repeatable key wins in multi-key reports - Timer cancelled on key release or keyboard reset Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- HidPkg/UefiHidDxeV2/src/boot_services.rs | 11 + HidPkg/UefiHidDxeV2/src/keyboard.rs | 384 +++++++++++++++++- HidPkg/UefiHidDxeV2/src/keyboard/key_queue.rs | 29 +- .../src/keyboard/simple_text_in.rs | 2 + .../src/keyboard/simple_text_in_ex.rs | 4 + 5 files changed, 425 insertions(+), 5 deletions(-) diff --git a/HidPkg/UefiHidDxeV2/src/boot_services.rs b/HidPkg/UefiHidDxeV2/src/boot_services.rs index 6eccb9062c..cf54a9e01f 100644 --- a/HidPkg/UefiHidDxeV2/src/boot_services.rs +++ b/HidPkg/UefiHidDxeV2/src/boot_services.rs @@ -79,6 +79,8 @@ pub trait UefiBootServices { controller_handle: efi::Handle, ) -> efi::Status; + fn set_timer(&self, event: efi::Event, r#type: efi::TimerDelay, trigger_time: u64) -> efi::Status; + fn locate_protocol( &self, protocol: *mut efi::Guid, @@ -195,6 +197,9 @@ impl UefiBootServices for StandardUefiBootServices { ) -> efi::Status { (self.boot_services().locate_protocol)(protocol, registration, interface) } + fn set_timer(&self, event: efi::Event, r#type: efi::TimerDelay, trigger_time: u64) -> efi::Status { + (self.boot_services().set_timer)(event, r#type, trigger_time) + } } #[cfg(test)] @@ -284,6 +289,10 @@ mod test { efi::Status::SUCCESS } + extern "efiapi" fn mock_set_timer(_event: efi::Event, _type: efi::TimerDelay, _trigger_time: u64) -> efi::Status { + efi::Status::SUCCESS + } + #[test] fn standard_uefi_boot_services_should_wrap_boot_services() { let boot_services = MaybeUninit::::zeroed(); @@ -299,6 +308,7 @@ mod test { boot_services.open_protocol = mock_open_protocol; boot_services.close_protocol = mock_close_protocol; boot_services.locate_protocol = mock_locate_protocol; + boot_services.set_timer = mock_set_timer; const TEST_GUID: efi::Guid = efi::Guid::from_fields(0, 0, 0, 0, 0, &[0, 0, 0, 0, 0, 0]); let mut event = 1 as efi::Event; @@ -373,5 +383,6 @@ mod test { ), efi::Status::SUCCESS ); + assert_eq!(test_boot_services.set_timer(event, efi::TIMER_CANCEL, 0), efi::Status::SUCCESS); } } diff --git a/HidPkg/UefiHidDxeV2/src/keyboard.rs b/HidPkg/UefiHidDxeV2/src/keyboard.rs index b7cb3bbb83..b8b7932b49 100644 --- a/HidPkg/UefiHidDxeV2/src/keyboard.rs +++ b/HidPkg/UefiHidDxeV2/src/keyboard.rs @@ -46,6 +46,11 @@ use crate::{ keyboard::key_queue::OrdKeyData, }; +// Repeat key delay: 500ms in 100ns units. +const REPEAT_KEY_DELAY: u64 = 5_000_000; +// Repeat key rate: 20ms in 100ns units (~50 keys/sec). +const REPEAT_KEY_RATE: u64 = 200_000; + // usages supported by this module const KEYBOARD_MODIFIER_USAGE_MIN: u32 = 0x000700E0; const KEYBOARD_MODIFIER_USAGE_MAX: u32 = 0x000700E7; @@ -91,6 +96,11 @@ struct LayoutChangeContext { keyboard_handler: *mut KeyboardHidHandler, } +#[repr(C)] +struct RepeatTimerContext { + keyboard_handler: *mut KeyboardHidHandler, +} + /// Keyboard HID Handler pub struct KeyboardHidHandler { boot_services: &'static dyn UefiBootServices, @@ -108,6 +118,9 @@ pub struct KeyboardHidHandler { key_notify_event: efi::Event, layout_change_event: efi::Event, layout_context: *mut LayoutChangeContext, + repeat_timer_event: efi::Event, + repeat_context: *mut RepeatTimerContext, + repeat_key: Option, } impl KeyboardHidHandler { @@ -129,6 +142,9 @@ impl KeyboardHidHandler { key_notify_event: core::ptr::null_mut(), layout_change_event: core::ptr::null_mut(), layout_context: core::ptr::null_mut(), + repeat_timer_event: core::ptr::null_mut(), + repeat_context: core::ptr::null_mut(), + repeat_key: None, } } @@ -325,6 +341,7 @@ impl KeyboardHidHandler { //Mark the instance invalid by setting the keyboard_handler raw pointer to null, but leak the LayoutContext //instance. Leaking context allows context usage in the callback should it fire. debugln!(DEBUG_ERROR, "Failed to close layout_change_event event, status: {:x?}", status); + // Event still exists in firmware; null out handler to prevent stale access and leak context. unsafe { (*self.layout_context).keyboard_handler = ptr::null_mut(); } @@ -338,6 +355,59 @@ impl KeyboardHidHandler { Ok(()) } + // Creates the repeat key timer event used for keystroke repeat when a key is held. + fn install_repeat_timer(&mut self) -> Result<(), efi::Status> { + let context = RepeatTimerContext { keyboard_handler: self as *mut Self }; + let context_ptr = Box::into_raw(Box::new(context)); + + let mut repeat_timer: efi::Event = ptr::null_mut(); + let status = self.boot_services.create_event( + efi::EVT_TIMER | efi::EVT_NOTIFY_SIGNAL, + efi::TPL_NOTIFY, + Some(on_repeat_timer), + context_ptr as *mut c_void, + ptr::addr_of_mut!(repeat_timer), + ); + if status.is_error() { + drop(unsafe { Box::from_raw(context_ptr) }); + Err(status)?; + } + + self.repeat_timer_event = repeat_timer; + self.repeat_context = context_ptr; + Ok(()) + } + + // Closes the repeat timer event and frees the context. + fn uninstall_repeat_timer(&mut self) -> Result<(), efi::Status> { + if !self.repeat_timer_event.is_null() { + let status = self.boot_services.set_timer(self.repeat_timer_event, efi::TIMER_CANCEL, 0); + if status.is_error() { + debugln!(DEBUG_ERROR, "Failed to cancel repeat_timer, status: {:x?}", status); + // Timer may still fire; null out handler to prevent stale access and leak context. + unsafe { + (*self.repeat_context).keyboard_handler = ptr::null_mut(); + } + return Err(status); + } + self.repeat_key = None; + + let status = self.boot_services.close_event(self.repeat_timer_event); + if status.is_error() { + debugln!(DEBUG_ERROR, "Failed to close repeat_timer event, status: {:x?}", status); + // Event still exists in firmware; null out handler to prevent stale access and leak context. + unsafe { + (*self.repeat_context).keyboard_handler = ptr::null_mut(); + } + return Err(status); + } + drop(unsafe { Box::from_raw(self.repeat_context) }); + self.repeat_context = ptr::null_mut(); + self.repeat_timer_event = ptr::null_mut(); + } + Ok(()) + } + // Installs a default keyboard layout. fn install_default_layout(&mut self) -> Result<(), efi::Status> { let mut hii_database_protocol_ptr: *mut protocols::hii_database::Protocol = ptr::null_mut(); @@ -432,6 +502,11 @@ impl KeyboardHidHandler { if extended_verification { self.led_state.clear(); } + // Cancel any active repeat timer. + if !self.repeat_timer_event.is_null() { + self.boot_services.set_timer(self.repeat_timer_event, efi::TIMER_CANCEL, 0); + } + self.repeat_key = None; Ok(()) } @@ -592,6 +667,7 @@ impl HidReportReceiver for KeyboardHidHandler { self.reset(true)?; self.install_protocol_interfaces(controller)?; self.initialize_keyboard_layout()?; + self.install_repeat_timer()?; // Register a Ctrl-Alt-Delete handler to reset the system. Register only for DEL scan code; CTRL-ALT // shift state will be qualified in the callback. @@ -671,11 +747,31 @@ impl HidReportReceiver for KeyboardHidHandler { pressed_keys.push(changed_key); } } - for key in released_keys { - self.key_queue.keystroke(key, key_queue::KeyAction::KeyUp); + for key in &released_keys { + self.key_queue.keystroke(*key, key_queue::KeyAction::KeyUp); } - for key in pressed_keys { - self.key_queue.keystroke(key, key_queue::KeyAction::KeyDown); + + // If the released key was the repeat key, cancel repeat. + if let Some(repeat_usage) = self.repeat_key + && released_keys.contains(&repeat_usage) + { + self.boot_services.set_timer(self.repeat_timer_event, efi::TIMER_CANCEL, 0); + self.repeat_key = None; + } + + // Track the last newly pressed repeatable key to set as the repeat candidate. + let mut new_repeat_key: Option = None; + for key in &pressed_keys { + self.key_queue.keystroke(*key, key_queue::KeyAction::KeyDown); + if self.key_queue.is_repeatable_key(*key) { + new_repeat_key = Some(*key); + } + } + + // If a new repeatable key was pressed, arm the repeat delay timer. + if let Some(usage) = new_repeat_key { + self.repeat_key = Some(usage); + self.boot_services.set_timer(self.repeat_timer_event, efi::TIMER_RELATIVE, REPEAT_KEY_DELAY); } //after processing all the key strokes, check if any keys were pressed that should trigger the notifier callback @@ -703,6 +799,10 @@ impl HidReportReceiver for KeyboardHidHandler { impl Drop for KeyboardHidHandler { fn drop(&mut self) { + // Close repeat timer first — its callback may reference protocol events. + if let Err(status) = self.uninstall_repeat_timer() { + debugln!(DEBUG_ERROR, "KeyboardHidHandler::drop: Failed to close repeat_timer: {:?}", status); + } if let Some(controller) = self.controller { if let Err(status) = simple_text_in::SimpleTextInFfi::uninstall(self.boot_services, self.agent, controller) { @@ -720,6 +820,35 @@ impl Drop for KeyboardHidHandler { } } +// Handles the repeat timer event. When a repeatable key is held, this fires after the initial delay (and then at the +// repeat rate) to re-enqueue the held key into the key queue. Runs at TPL_NOTIFY for mutual exclusion with other +// keyboard handler access. +extern "efiapi" fn on_repeat_timer(_event: efi::Event, context: *mut c_void) { + let context = unsafe { (context as *mut RepeatTimerContext).as_mut() }.expect("bad repeat timer context pointer"); + + if context.keyboard_handler.is_null() { + debugln!(DEBUG_ERROR, "on_repeat_timer invoked with invalid handler"); + return; + } + + let keyboard_handler = unsafe { context.keyboard_handler.as_mut() }.expect("bad keyboard handler"); + + let Some(repeat_usage) = keyboard_handler.repeat_key else { + return; + }; + + // Re-process the held key as a new KeyDown event. This picks up the current modifier/toggle state. + keyboard_handler.key_queue.keystroke(repeat_usage, key_queue::KeyAction::KeyDown); + + // Signal key notify event if the repeated keystroke matched a registered callback. + if keyboard_handler.key_queue.peek_notify_key().is_some() { + keyboard_handler.boot_services.signal_event(keyboard_handler.key_notify_event); + } + + // Re-arm the timer at the repeat rate for the next repeat. + keyboard_handler.boot_services.set_timer(keyboard_handler.repeat_timer_event, efi::TIMER_RELATIVE, REPEAT_KEY_RATE); +} + // handles keyboard layout change event that occurs when a new keyboard layout is set. extern "efiapi" fn on_layout_update(_event: efi::Event, context: *mut c_void) { let context = unsafe { (context as *mut LayoutChangeContext).as_mut() }.expect("bad context pointer"); @@ -907,6 +1036,7 @@ mod test { let boot_services = create_fake_static_boot_service(); boot_services.expect_create_event().returning(|_, _, _, _, _| efi::Status::SUCCESS); boot_services.expect_create_event_ex().returning(|_, _, _, _, _, _| efi::Status::SUCCESS); + boot_services.expect_set_timer().returning(|_, _, _| efi::Status::SUCCESS); boot_services.expect_install_protocol_interface().returning(|_, _, _, _| efi::Status::SUCCESS); boot_services.expect_locate_protocol().returning(|_, _, _| efi::Status::NOT_FOUND); boot_services.expect_signal_event().returning(|_| efi::Status::SUCCESS); @@ -931,6 +1061,7 @@ mod test { let boot_services = create_fake_static_boot_service(); boot_services.expect_create_event().returning(|_, _, _, _, _| efi::Status::SUCCESS); boot_services.expect_create_event_ex().returning(|_, _, _, _, _, _| efi::Status::SUCCESS); + boot_services.expect_set_timer().returning(|_, _, _| efi::Status::SUCCESS); boot_services.expect_install_protocol_interface().returning(|_, _, _, _| efi::Status::SUCCESS); boot_services.expect_locate_protocol().returning(|_, _, _| efi::Status::NOT_FOUND); boot_services.expect_signal_event().returning(|_| efi::Status::SUCCESS); @@ -1055,6 +1186,7 @@ mod test { boot_services.expect_raise_tpl().returning(|_| efi::TPL_APPLICATION); boot_services.expect_restore_tpl().returning(|_| ()); boot_services.expect_close_event().returning(|_| efi::Status::SUCCESS); + boot_services.expect_set_timer().returning(|_, _, _| efi::Status::SUCCESS); static mut HANDLER: *mut KeyboardHidHandler = core::ptr::null_mut(); @@ -1184,6 +1316,7 @@ mod test { let boot_services = create_fake_static_boot_service(); boot_services.expect_create_event().returning(|_, _, _, _, _| efi::Status::SUCCESS); boot_services.expect_create_event_ex().returning(|_, _, _, _, _, _| efi::Status::SUCCESS); + boot_services.expect_set_timer().returning(|_, _, _| efi::Status::SUCCESS); boot_services.expect_install_protocol_interface().returning(|_, _, _, _| efi::Status::SUCCESS); boot_services.expect_locate_protocol().returning(|_, _, _| efi::Status::NOT_FOUND); boot_services.expect_signal_event().returning(|_| efi::Status::SUCCESS); @@ -1231,6 +1364,7 @@ mod test { let boot_services = create_fake_static_boot_service(); boot_services.expect_create_event().returning(|_, _, _, _, _| efi::Status::SUCCESS); boot_services.expect_create_event_ex().returning(|_, _, _, _, _, _| efi::Status::SUCCESS); + boot_services.expect_set_timer().returning(|_, _, _| efi::Status::SUCCESS); boot_services.expect_install_protocol_interface().returning(|_, _, _, _| efi::Status::SUCCESS); boot_services.expect_locate_protocol().returning(|_, _, _| efi::Status::NOT_FOUND); boot_services.expect_signal_event().returning(|_| efi::Status::SUCCESS); @@ -1296,6 +1430,7 @@ mod test { let boot_services = create_fake_static_boot_service(); boot_services.expect_create_event().returning(|_, _, _, _, _| efi::Status::SUCCESS); boot_services.expect_create_event_ex().returning(|_, _, _, _, _, _| efi::Status::SUCCESS); + boot_services.expect_set_timer().returning(|_, _, _| efi::Status::SUCCESS); boot_services.expect_install_protocol_interface().returning(|_, _, _, _| efi::Status::SUCCESS); boot_services.expect_locate_protocol().returning(|_, _, _| efi::Status::NOT_FOUND); boot_services.expect_signal_event().returning(|_| efi::Status::SUCCESS); @@ -1459,4 +1594,245 @@ mod test { assert!(callback_key_data.is_none()); assert!(callbacks.is_empty()); } + + #[test] + fn key_press_should_arm_repeat_timer() { + use std::sync::atomic::{AtomicU32, AtomicU64}; + + static SET_TIMER_TYPE: AtomicU32 = AtomicU32::new(0); + static SET_TIMER_TIME: AtomicU64 = AtomicU64::new(0); + + let boot_services = create_fake_static_boot_service(); + boot_services.expect_create_event().returning(|_, _, _, _, _| efi::Status::SUCCESS); + boot_services.expect_create_event_ex().returning(|_, _, _, _, _, _| efi::Status::SUCCESS); + boot_services.expect_set_timer().returning(|_, timer_type, trigger_time| { + SET_TIMER_TYPE.store(timer_type, std::sync::atomic::Ordering::SeqCst); + SET_TIMER_TIME.store(trigger_time, std::sync::atomic::Ordering::SeqCst); + efi::Status::SUCCESS + }); + boot_services.expect_install_protocol_interface().returning(|_, _, _, _| efi::Status::SUCCESS); + boot_services.expect_locate_protocol().returning(|_, _, _| efi::Status::NOT_FOUND); + boot_services.expect_signal_event().returning(|_| efi::Status::SUCCESS); + boot_services.expect_open_protocol().returning(|_, _, _, _, _, _| efi::Status::NOT_FOUND); + boot_services.expect_raise_tpl().returning(|_| efi::TPL_APPLICATION); + boot_services.expect_restore_tpl().returning(|_| ()); + + let mut keyboard_handler = KeyboardHidHandler::new(boot_services, 1 as efi::Handle); + let mut hid_io = MockHidIo::new(); + hid_io.expect_set_output_report().returning(|_, _| Ok(())); + hid_io + .expect_get_report_descriptor() + .returning(|| Ok(hidparser::parse_report_descriptor(BOOT_KEYBOARD_REPORT_DESCRIPTOR).unwrap())); + + keyboard_handler.key_queue.set_layout(Some(hii_keyboard_layout::get_default_keyboard_layout())); + keyboard_handler.initialize(2 as efi::Handle, &hid_io).unwrap(); + + // press the 'a' key + let report: &[u8] = &[0x00, 0x00, 0x04, 0x00, 0x00, 0x00, 0x00, 0x00]; + keyboard_handler.receive_report(report, &hid_io); + + // Repeat timer should have been armed with TIMER_RELATIVE and delay + assert_eq!(SET_TIMER_TYPE.load(std::sync::atomic::Ordering::SeqCst), efi::TIMER_RELATIVE); + assert_eq!(SET_TIMER_TIME.load(std::sync::atomic::Ordering::SeqCst), super::REPEAT_KEY_DELAY); + assert!(keyboard_handler.repeat_key.is_some()); + } + + #[test] + fn key_release_should_cancel_repeat_timer() { + use std::sync::atomic::AtomicU32; + + static LAST_TIMER_TYPE: AtomicU32 = AtomicU32::new(0xFF); + + let boot_services = create_fake_static_boot_service(); + boot_services.expect_create_event().returning(|_, _, _, _, _| efi::Status::SUCCESS); + boot_services.expect_create_event_ex().returning(|_, _, _, _, _, _| efi::Status::SUCCESS); + boot_services.expect_set_timer().returning(|_, timer_type, _| { + LAST_TIMER_TYPE.store(timer_type, std::sync::atomic::Ordering::SeqCst); + efi::Status::SUCCESS + }); + boot_services.expect_install_protocol_interface().returning(|_, _, _, _| efi::Status::SUCCESS); + boot_services.expect_locate_protocol().returning(|_, _, _| efi::Status::NOT_FOUND); + boot_services.expect_signal_event().returning(|_| efi::Status::SUCCESS); + boot_services.expect_open_protocol().returning(|_, _, _, _, _, _| efi::Status::NOT_FOUND); + boot_services.expect_raise_tpl().returning(|_| efi::TPL_APPLICATION); + boot_services.expect_restore_tpl().returning(|_| ()); + + let mut keyboard_handler = KeyboardHidHandler::new(boot_services, 1 as efi::Handle); + let mut hid_io = MockHidIo::new(); + hid_io.expect_set_output_report().returning(|_, _| Ok(())); + hid_io + .expect_get_report_descriptor() + .returning(|| Ok(hidparser::parse_report_descriptor(BOOT_KEYBOARD_REPORT_DESCRIPTOR).unwrap())); + + keyboard_handler.key_queue.set_layout(Some(hii_keyboard_layout::get_default_keyboard_layout())); + keyboard_handler.initialize(2 as efi::Handle, &hid_io).unwrap(); + + // press the 'a' key + let report: &[u8] = &[0x00, 0x00, 0x04, 0x00, 0x00, 0x00, 0x00, 0x00]; + keyboard_handler.receive_report(report, &hid_io); + assert!(keyboard_handler.repeat_key.is_some()); + + // release the 'a' key + let report: &[u8] = &[0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00]; + keyboard_handler.receive_report(report, &hid_io); + + // Timer should have been cancelled + assert_eq!(LAST_TIMER_TYPE.load(std::sync::atomic::Ordering::SeqCst), efi::TIMER_CANCEL); + assert!(keyboard_handler.repeat_key.is_none()); + } + + #[test] + fn modifier_keys_should_not_trigger_repeat() { + let boot_services = create_fake_static_boot_service(); + boot_services.expect_create_event().returning(|_, _, _, _, _| efi::Status::SUCCESS); + boot_services.expect_create_event_ex().returning(|_, _, _, _, _, _| efi::Status::SUCCESS); + boot_services.expect_set_timer().returning(|_, _, _| efi::Status::SUCCESS); + boot_services.expect_install_protocol_interface().returning(|_, _, _, _| efi::Status::SUCCESS); + boot_services.expect_locate_protocol().returning(|_, _, _| efi::Status::NOT_FOUND); + boot_services.expect_signal_event().returning(|_| efi::Status::SUCCESS); + boot_services.expect_open_protocol().returning(|_, _, _, _, _, _| efi::Status::NOT_FOUND); + boot_services.expect_raise_tpl().returning(|_| efi::TPL_APPLICATION); + boot_services.expect_restore_tpl().returning(|_| ()); + + let mut keyboard_handler = KeyboardHidHandler::new(boot_services, 1 as efi::Handle); + let mut hid_io = MockHidIo::new(); + hid_io.expect_set_output_report().returning(|_, _| Ok(())); + hid_io + .expect_get_report_descriptor() + .returning(|| Ok(hidparser::parse_report_descriptor(BOOT_KEYBOARD_REPORT_DESCRIPTOR).unwrap())); + + keyboard_handler.key_queue.set_layout(Some(hii_keyboard_layout::get_default_keyboard_layout())); + keyboard_handler.initialize(2 as efi::Handle, &hid_io).unwrap(); + + // press left shift only (modifier byte bit 1) + let report: &[u8] = &[0x02, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00]; + keyboard_handler.receive_report(report, &hid_io); + + // Modifier keys should not trigger repeat + assert!(keyboard_handler.repeat_key.is_none()); + } + + #[test] + fn new_key_press_should_replace_repeat_key() { + let boot_services = create_fake_static_boot_service(); + boot_services.expect_create_event().returning(|_, _, _, _, _| efi::Status::SUCCESS); + boot_services.expect_create_event_ex().returning(|_, _, _, _, _, _| efi::Status::SUCCESS); + boot_services.expect_set_timer().returning(|_, _, _| efi::Status::SUCCESS); + boot_services.expect_install_protocol_interface().returning(|_, _, _, _| efi::Status::SUCCESS); + boot_services.expect_locate_protocol().returning(|_, _, _| efi::Status::NOT_FOUND); + boot_services.expect_signal_event().returning(|_| efi::Status::SUCCESS); + boot_services.expect_open_protocol().returning(|_, _, _, _, _, _| efi::Status::NOT_FOUND); + boot_services.expect_raise_tpl().returning(|_| efi::TPL_APPLICATION); + boot_services.expect_restore_tpl().returning(|_| ()); + + let mut keyboard_handler = KeyboardHidHandler::new(boot_services, 1 as efi::Handle); + let mut hid_io = MockHidIo::new(); + hid_io.expect_set_output_report().returning(|_, _| Ok(())); + hid_io + .expect_get_report_descriptor() + .returning(|| Ok(hidparser::parse_report_descriptor(BOOT_KEYBOARD_REPORT_DESCRIPTOR).unwrap())); + + keyboard_handler.key_queue.set_layout(Some(hii_keyboard_layout::get_default_keyboard_layout())); + keyboard_handler.initialize(2 as efi::Handle, &hid_io).unwrap(); + + // press 'a' key + let report: &[u8] = &[0x00, 0x00, 0x04, 0x00, 0x00, 0x00, 0x00, 0x00]; + keyboard_handler.receive_report(report, &hid_io); + let first_repeat = keyboard_handler.repeat_key; + assert!(first_repeat.is_some()); + + // press 'a' + 'b' (add 'b' while holding 'a') + let report: &[u8] = &[0x00, 0x00, 0x04, 0x05, 0x00, 0x00, 0x00, 0x00]; + keyboard_handler.receive_report(report, &hid_io); + + // repeat key should now be 'b' (the newly pressed key), not 'a' + assert!(keyboard_handler.repeat_key.is_some()); + assert_ne!(keyboard_handler.repeat_key, first_repeat); + } + + #[test] + fn repeat_timer_callback_should_enqueue_key() { + let boot_services = create_fake_static_boot_service(); + boot_services.expect_create_event().returning(|_, _, _, _, _| efi::Status::SUCCESS); + boot_services.expect_create_event_ex().returning(|_, _, _, _, _, _| efi::Status::SUCCESS); + boot_services.expect_set_timer().returning(|_, _, _| efi::Status::SUCCESS); + boot_services.expect_install_protocol_interface().returning(|_, _, _, _| efi::Status::SUCCESS); + boot_services.expect_locate_protocol().returning(|_, _, _| efi::Status::NOT_FOUND); + boot_services.expect_signal_event().returning(|_| efi::Status::SUCCESS); + boot_services.expect_open_protocol().returning(|_, _, _, _, _, _| efi::Status::NOT_FOUND); + boot_services.expect_raise_tpl().returning(|_| efi::TPL_APPLICATION); + boot_services.expect_restore_tpl().returning(|_| ()); + + let mut keyboard_handler = KeyboardHidHandler::new(boot_services, 1 as efi::Handle); + let mut hid_io = MockHidIo::new(); + hid_io.expect_set_output_report().returning(|_, _| Ok(())); + hid_io + .expect_get_report_descriptor() + .returning(|| Ok(hidparser::parse_report_descriptor(BOOT_KEYBOARD_REPORT_DESCRIPTOR).unwrap())); + + keyboard_handler.key_queue.set_layout(Some(hii_keyboard_layout::get_default_keyboard_layout())); + keyboard_handler.initialize(2 as efi::Handle, &hid_io).unwrap(); + + // press 'a' key + let report: &[u8] = &[0x00, 0x00, 0x04, 0x00, 0x00, 0x00, 0x00, 0x00]; + keyboard_handler.receive_report(report, &hid_io); + + // consume the initial keystroke + let initial_key = keyboard_handler.pop_key().unwrap(); + assert_eq!(initial_key.key.unicode_char, 'a' as u16); + assert!(keyboard_handler.pop_key().is_none()); + + // Simulate the repeat timer callback firing by calling it directly + let context = keyboard_handler.repeat_context; + super::on_repeat_timer(ptr::null_mut(), context as *mut c_void); + + // Should have enqueued a repeat keystroke + let repeat_key = keyboard_handler.pop_key().unwrap(); + assert_eq!(repeat_key.key.unicode_char, 'a' as u16); + } + + #[test] + fn reset_should_cancel_repeat_timer() { + use std::sync::atomic::AtomicU32; + + static LAST_TIMER_TYPE: AtomicU32 = AtomicU32::new(0xFF); + + let boot_services = create_fake_static_boot_service(); + boot_services.expect_create_event().returning(|_, _, _, _, event| unsafe { + event.write(3 as efi::Event); + efi::Status::SUCCESS + }); + boot_services.expect_create_event_ex().returning(|_, _, _, _, _, _| efi::Status::SUCCESS); + boot_services.expect_set_timer().returning(|_, timer_type, _| { + LAST_TIMER_TYPE.store(timer_type, std::sync::atomic::Ordering::SeqCst); + efi::Status::SUCCESS + }); + boot_services.expect_install_protocol_interface().returning(|_, _, _, _| efi::Status::SUCCESS); + boot_services.expect_locate_protocol().returning(|_, _, _| efi::Status::NOT_FOUND); + boot_services.expect_signal_event().returning(|_| efi::Status::SUCCESS); + boot_services.expect_open_protocol().returning(|_, _, _, _, _, _| efi::Status::NOT_FOUND); + boot_services.expect_raise_tpl().returning(|_| efi::TPL_APPLICATION); + boot_services.expect_restore_tpl().returning(|_| ()); + boot_services.expect_close_event().returning(|_| efi::Status::SUCCESS); + + let mut keyboard_handler = KeyboardHidHandler::new(boot_services, 1 as efi::Handle); + let mut hid_io = MockHidIo::new(); + hid_io.expect_set_output_report().returning(|_, _| Ok(())); + hid_io + .expect_get_report_descriptor() + .returning(|| Ok(hidparser::parse_report_descriptor(BOOT_KEYBOARD_REPORT_DESCRIPTOR).unwrap())); + + keyboard_handler.key_queue.set_layout(Some(hii_keyboard_layout::get_default_keyboard_layout())); + keyboard_handler.initialize(2 as efi::Handle, &hid_io).unwrap(); + + // press 'a' to set up repeat + let report: &[u8] = &[0x00, 0x00, 0x04, 0x00, 0x00, 0x00, 0x00, 0x00]; + keyboard_handler.receive_report(report, &hid_io); + assert!(keyboard_handler.repeat_key.is_some()); + + // reset should cancel repeat + keyboard_handler.reset(false).unwrap(); + assert_eq!(LAST_TIMER_TYPE.load(std::sync::atomic::Ordering::SeqCst), efi::TIMER_CANCEL); + assert!(keyboard_handler.repeat_key.is_none()); + } } diff --git a/HidPkg/UefiHidDxeV2/src/keyboard/key_queue.rs b/HidPkg/UefiHidDxeV2/src/keyboard/key_queue.rs index 09a01d4927..6f8a9ac40f 100644 --- a/HidPkg/UefiHidDxeV2/src/keyboard/key_queue.rs +++ b/HidPkg/UefiHidDxeV2/src/keyboard/key_queue.rs @@ -148,7 +148,7 @@ impl KeyQueue { // Processes the given keystroke and updates the KeyQueue accordingly. pub(crate) fn keystroke(&mut self, key: Usage, action: KeyAction) { - let Some(ref active_layout) = self.layout else { + let Some(active_layout) = &self.layout else { //nothing to do if no layout. This is unexpected: layout should be initialized with default if not present. debugln!(DEBUG_WARN, "key_queue::keystroke: Received keystroke without layout."); return; @@ -410,6 +410,33 @@ impl KeyQueue { self.registered_keys.insert(key_data); } + // Returns whether the given usage represents a key that should support key repeat. + // Modifier keys (Shift, Ctrl, Alt, etc.), toggle keys (CapsLock, NumLock, ScrollLock), and + // non-spacing (dead) keys are excluded. + pub(crate) fn is_repeatable_key(&self, usage: Usage) -> bool { + let Some(active_layout) = &self.layout else { + return false; + }; + + let Some(efi_key) = usage_to_efi_key(usage) else { + return false; + }; + + for key in &active_layout.keys { + match key { + HiiKey::Key(descriptor) if descriptor.key == efi_key => { + return !KEYBOARD_MODIFIERS.contains(&descriptor.modifier) + && !TOGGLE_MODIFIERS.contains(&descriptor.modifier); + } + HiiKey::NsKey(_ns_descriptor) if _ns_descriptor.descriptor.key == efi_key => { + return false; + } + _ => continue, + } + } + false + } + // Remove a previously added notify key; keystrokes matching this key data will no longer be added to the notify // queue. pub(crate) fn remove_notify_key(&mut self, key_data: &OrdKeyData) { diff --git a/HidPkg/UefiHidDxeV2/src/keyboard/simple_text_in.rs b/HidPkg/UefiHidDxeV2/src/keyboard/simple_text_in.rs index bb18c9d682..dea1b2fc5f 100644 --- a/HidPkg/UefiHidDxeV2/src/keyboard/simple_text_in.rs +++ b/HidPkg/UefiHidDxeV2/src/keyboard/simple_text_in.rs @@ -476,6 +476,7 @@ mod test { // used in keyboard init and uninstall boot_services.expect_create_event_ex().returning(|_, _, _, _, _, _| efi::Status::SUCCESS); + boot_services.expect_set_timer().returning(|_, _, _| efi::Status::SUCCESS); boot_services.expect_signal_event().returning(|_| efi::Status::SUCCESS); boot_services.expect_open_protocol().returning(|_, _, _, _, _, _| efi::Status::NOT_FOUND); boot_services.expect_locate_protocol().returning(|_, _, _| efi::Status::NOT_FOUND); @@ -613,6 +614,7 @@ mod test { efi::Status::SUCCESS }); boot_services.expect_create_event_ex().returning(|_, _, _, _, _, _| efi::Status::SUCCESS); + boot_services.expect_set_timer().returning(|_, _, _| efi::Status::SUCCESS); boot_services.expect_raise_tpl().returning(|_| efi::TPL_APPLICATION); boot_services.expect_restore_tpl().returning(|_| ()); diff --git a/HidPkg/UefiHidDxeV2/src/keyboard/simple_text_in_ex.rs b/HidPkg/UefiHidDxeV2/src/keyboard/simple_text_in_ex.rs index fe66ba2d63..95d5de1c05 100644 --- a/HidPkg/UefiHidDxeV2/src/keyboard/simple_text_in_ex.rs +++ b/HidPkg/UefiHidDxeV2/src/keyboard/simple_text_in_ex.rs @@ -626,6 +626,7 @@ mod test { // used in keyboard init and uninstall boot_services.expect_create_event_ex().returning(|_, _, _, _, _, _| efi::Status::SUCCESS); + boot_services.expect_set_timer().returning(|_, _, _| efi::Status::SUCCESS); boot_services.expect_signal_event().returning(|_| efi::Status::SUCCESS); boot_services.expect_open_protocol().returning(|_, _, _, _, _, _| efi::Status::NOT_FOUND); boot_services.expect_locate_protocol().returning(|_, _, _| efi::Status::NOT_FOUND); @@ -781,6 +782,7 @@ mod test { // used in keyboard init and uninstall boot_services.expect_create_event_ex().returning(|_, _, _, _, _, _| efi::Status::SUCCESS); + boot_services.expect_set_timer().returning(|_, _, _| efi::Status::SUCCESS); boot_services.expect_signal_event().returning(|_| efi::Status::SUCCESS); extern "efiapi" fn mock_set_report( _this: *const hid_io::protocol::Protocol, @@ -868,6 +870,7 @@ mod test { // used in keyboard init and uninstall boot_services.expect_create_event_ex().returning(|_, _, _, _, _, _| efi::Status::SUCCESS); + boot_services.expect_set_timer().returning(|_, _, _| efi::Status::SUCCESS); boot_services.expect_signal_event().returning(|event| { if event == NOTIFY_EVENT { SimpleTextInExFfi::process_key_notifies(event, CONTEXT_PTR.load(Ordering::SeqCst)); @@ -1015,6 +1018,7 @@ mod test { boot_services.expect_locate_protocol().returning(|_, _, _| efi::Status::NOT_FOUND); boot_services.expect_create_event_ex().returning(|_, _, _, _, _, _| efi::Status::SUCCESS); + boot_services.expect_set_timer().returning(|_, _, _| efi::Status::SUCCESS); boot_services.expect_raise_tpl().returning(|_| efi::TPL_APPLICATION); boot_services.expect_restore_tpl().returning(|_| ()); From 5bf442c7e061a24f74d635f1e5d3136f0eb4067d Mon Sep 17 00:00:00 2001 From: John Schock Date: Wed, 29 Apr 2026 12:42:53 -0700 Subject: [PATCH 2/3] UefiHidDxeV2: Hand off repeat to remaining held key on release When the repeating key is released while other keys are still held, select a new repeat candidate from the remaining held keys and arm the delay timer. Previously, releasing the repeat key would stop all repeat even if other repeatable keys were still pressed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- HidPkg/UefiHidDxeV2/src/keyboard.rs | 51 ++++++++++++++++++++++++++++- 1 file changed, 50 insertions(+), 1 deletion(-) diff --git a/HidPkg/UefiHidDxeV2/src/keyboard.rs b/HidPkg/UefiHidDxeV2/src/keyboard.rs index b8b7932b49..b63ea193b2 100644 --- a/HidPkg/UefiHidDxeV2/src/keyboard.rs +++ b/HidPkg/UefiHidDxeV2/src/keyboard.rs @@ -751,12 +751,25 @@ impl HidReportReceiver for KeyboardHidHandler { self.key_queue.keystroke(*key, key_queue::KeyAction::KeyUp); } - // If the released key was the repeat key, cancel repeat. + // If the released key was the repeat key, cancel repeat and pick a new + // candidate from the remaining held keys. if let Some(repeat_usage) = self.repeat_key && released_keys.contains(&repeat_usage) { self.boot_services.set_timer(self.repeat_timer_event, efi::TIMER_CANCEL, 0); self.repeat_key = None; + + // Find a repeatable key among the still-held keys. + let new_candidate = + self.current_keys.iter().rev().find(|k| self.key_queue.is_repeatable_key(**k)).copied(); + if let Some(usage) = new_candidate { + self.repeat_key = Some(usage); + self.boot_services.set_timer( + self.repeat_timer_event, + efi::TIMER_RELATIVE, + REPEAT_KEY_DELAY, + ); + } } // Track the last newly pressed repeatable key to set as the repeat candidate. @@ -1681,6 +1694,42 @@ mod test { assert!(keyboard_handler.repeat_key.is_none()); } + #[test] + fn release_repeat_key_should_handoff_to_remaining_held_key() { + let boot_services = create_fake_static_boot_service(); + boot_services.expect_create_event().returning(|_, _, _, _, _| efi::Status::SUCCESS); + boot_services.expect_create_event_ex().returning(|_, _, _, _, _, _| efi::Status::SUCCESS); + boot_services.expect_set_timer().returning(|_, _, _| efi::Status::SUCCESS); + boot_services.expect_install_protocol_interface().returning(|_, _, _, _| efi::Status::SUCCESS); + boot_services.expect_locate_protocol().returning(|_, _, _| efi::Status::NOT_FOUND); + boot_services.expect_signal_event().returning(|_| efi::Status::SUCCESS); + boot_services.expect_open_protocol().returning(|_, _, _, _, _, _| efi::Status::NOT_FOUND); + boot_services.expect_raise_tpl().returning(|_| efi::TPL_APPLICATION); + boot_services.expect_restore_tpl().returning(|_| ()); + + let mut keyboard_handler = KeyboardHidHandler::new(boot_services, 1 as efi::Handle); + let mut hid_io = MockHidIo::new(); + hid_io.expect_set_output_report().returning(|_, _| Ok(())); + hid_io + .expect_get_report_descriptor() + .returning(|| Ok(hidparser::parse_report_descriptor(BOOT_KEYBOARD_REPORT_DESCRIPTOR).unwrap())); + + keyboard_handler.key_queue.set_layout(Some(hii_keyboard_layout::get_default_keyboard_layout())); + keyboard_handler.initialize(2 as efi::Handle, &hid_io).unwrap(); + + // press 'a' + 'b' simultaneously + let report: &[u8] = &[0x00, 0x00, 0x04, 0x05, 0x00, 0x00, 0x00, 0x00]; + keyboard_handler.receive_report(report, &hid_io); + assert!(keyboard_handler.repeat_key.is_some()); + + // release 'b' (the repeat key), 'a' still held + let report: &[u8] = &[0x00, 0x00, 0x04, 0x00, 0x00, 0x00, 0x00, 0x00]; + keyboard_handler.receive_report(report, &hid_io); + + // 'a' should now be the repeat key + assert_eq!(keyboard_handler.repeat_key, Some(hidparser::report_data_types::Usage::from(0x00070004u32))); + } + #[test] fn modifier_keys_should_not_trigger_repeat() { let boot_services = create_fake_static_boot_service(); From cf474d9dfd90819b803de681cd02fdc087dbaf31 Mon Sep 17 00:00:00 2001 From: John Schock Date: Fri, 1 May 2026 13:50:52 -0700 Subject: [PATCH 3/3] Address review feedback. --- HidPkg/UefiHidDxeV2/src/keyboard.rs | 18 +++++++++++------- HidPkg/UefiHidDxeV2/src/keyboard/key_queue.rs | 2 +- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/HidPkg/UefiHidDxeV2/src/keyboard.rs b/HidPkg/UefiHidDxeV2/src/keyboard.rs index b63ea193b2..e7818de45a 100644 --- a/HidPkg/UefiHidDxeV2/src/keyboard.rs +++ b/HidPkg/UefiHidDxeV2/src/keyboard.rs @@ -370,7 +370,7 @@ impl KeyboardHidHandler { ); if status.is_error() { drop(unsafe { Box::from_raw(context_ptr) }); - Err(status)?; + return Err(status); } self.repeat_timer_event = repeat_timer; @@ -504,7 +504,10 @@ impl KeyboardHidHandler { } // Cancel any active repeat timer. if !self.repeat_timer_event.is_null() { - self.boot_services.set_timer(self.repeat_timer_event, efi::TIMER_CANCEL, 0); + let status = self.boot_services.set_timer(self.repeat_timer_event, efi::TIMER_CANCEL, 0); + if status.is_error() { + debugln!(DEBUG_ERROR, "Failed to cancel repeat_timer during reset, status: {:x?}", status); + } } self.repeat_key = None; Ok(()) @@ -837,14 +840,15 @@ impl Drop for KeyboardHidHandler { // repeat rate) to re-enqueue the held key into the key queue. Runs at TPL_NOTIFY for mutual exclusion with other // keyboard handler access. extern "efiapi" fn on_repeat_timer(_event: efi::Event, context: *mut c_void) { - let context = unsafe { (context as *mut RepeatTimerContext).as_mut() }.expect("bad repeat timer context pointer"); + let Some(context) = (unsafe { (context as *mut RepeatTimerContext).as_mut() }) else { + debugln!(DEBUG_ERROR, "on_repeat_timer invoked with null context pointer"); + return; + }; - if context.keyboard_handler.is_null() { + let Some(keyboard_handler) = (unsafe { context.keyboard_handler.as_mut() }) else { debugln!(DEBUG_ERROR, "on_repeat_timer invoked with invalid handler"); return; - } - - let keyboard_handler = unsafe { context.keyboard_handler.as_mut() }.expect("bad keyboard handler"); + }; let Some(repeat_usage) = keyboard_handler.repeat_key else { return; diff --git a/HidPkg/UefiHidDxeV2/src/keyboard/key_queue.rs b/HidPkg/UefiHidDxeV2/src/keyboard/key_queue.rs index 6f8a9ac40f..8b4a3079f7 100644 --- a/HidPkg/UefiHidDxeV2/src/keyboard/key_queue.rs +++ b/HidPkg/UefiHidDxeV2/src/keyboard/key_queue.rs @@ -428,7 +428,7 @@ impl KeyQueue { return !KEYBOARD_MODIFIERS.contains(&descriptor.modifier) && !TOGGLE_MODIFIERS.contains(&descriptor.modifier); } - HiiKey::NsKey(_ns_descriptor) if _ns_descriptor.descriptor.key == efi_key => { + HiiKey::NsKey(ns_descriptor) if ns_descriptor.descriptor.key == efi_key => { return false; } _ => continue,