From 374b5f68566fe7f144639fe31fb25d6f07e6c336 Mon Sep 17 00:00:00 2001 From: Danielle Hollander Date: Mon, 8 Jun 2026 12:06:01 -0400 Subject: [PATCH 1/7] Add expected_response_size to GetReport command Add an optional buffer size parameter to GetReport so the I2C device layer can limit the read length to the actual expected payload size instead of always reading the full buffer. This avoids over-reading on devices that are sensitive to read length in write_read transactions. --- embedded-service/src/hid/command.rs | 23 ++++++++++++++--------- hid-service/src/i2c/device.rs | 21 +++++++++++++++++++-- hid-service/src/i2c/host.rs | 2 +- keyboard-service/src/hid_kb.rs | 4 ++-- 4 files changed, 36 insertions(+), 14 deletions(-) diff --git a/embedded-service/src/hid/command.rs b/embedded-service/src/hid/command.rs index 0798995d6..1cae20a62 100644 --- a/embedded-service/src/hid/command.rs +++ b/embedded-service/src/hid/command.rs @@ -225,7 +225,7 @@ impl Opcode { #[allow(missing_docs)] pub enum Command<'a> { Reset, - GetReport(ReportType, ReportId), + GetReport(ReportType, ReportId, Option), SetReport(ReportType, ReportId, SharedRef<'a, u8>), GetIdle(ReportId), SetIdle(ReportId, ReportFreq), @@ -239,7 +239,7 @@ impl From> for Opcode { fn from(value: Command<'_>) -> Self { match value { Command::Reset => Opcode::Reset, - Command::GetReport(_, _) => Opcode::GetReport, + Command::GetReport(_, _, _) => Opcode::GetReport, Command::SetReport(_, _, _) => Opcode::SetReport, Command::GetIdle(_) => Opcode::GetIdle, Command::SetIdle(_, _) => Opcode::SetIdle, @@ -293,6 +293,7 @@ impl<'a> Command<'a> { report_type: Option, report_id: Option, data: Option>, + expected_response_size: Option, ) -> Result { if opcode.requires_report_id() && report_id.is_none() { return Err(Error::RequiresReportId); @@ -310,7 +311,11 @@ impl<'a> Command<'a> { Opcode::Reset => Command::Reset, Opcode::GetReport => { if report_type? == ReportType::Input || report_type? == ReportType::Feature { - Command::GetReport(report_type?, report_id.ok_or(Error::RequiresReportId)?) + Command::GetReport( + report_type?, + report_id.ok_or(Error::RequiresReportId)?, + expected_response_size, + ) } else { return Err(Error::InvalidReportType); } @@ -476,7 +481,7 @@ impl<'a> Command<'a> { let (command_len, _) = Self::encode_basic_op(buf, Opcode::Reset)?; len += command_len; } - Command::GetReport(report_type, report_id) => { + Command::GetReport(report_type, report_id, _expected_size) => { let (command_len, buf) = Self::encode_common(buf, Opcode::GetReport, Some(*report_type), *report_id)?; len += command_len; @@ -594,35 +599,35 @@ mod test { let mut test_buffer = [0u8; 7]; // Test input report - let len = Command::GetReport(ReportType::Input, REPORT_ID) + let len = Command::GetReport(ReportType::Input, REPORT_ID, None) .encode_into_slice(&mut test_buffer, None, None) .unwrap(); assert_eq!(&test_buffer[0..len], [0x18, 0x02]); // Test feature report test_buffer.fill(0); - let len = Command::GetReport(ReportType::Feature, REPORT_ID) + let len = Command::GetReport(ReportType::Feature, REPORT_ID, None) .encode_into_slice(&mut test_buffer, None, None) .unwrap(); assert_eq!(&test_buffer[0..len], [0x38, 0x02]); // Test extended report test_buffer.fill(0); - let len = Command::GetReport(ReportType::Input, EXT_REPORT_ID) + let len = Command::GetReport(ReportType::Input, EXT_REPORT_ID, None) .encode_into_slice(&mut test_buffer, None, None) .unwrap(); assert_eq!(&test_buffer[0..len], [0x1f, 0x02, EXTENDED_REPORT_ID]); // Test standard report id with registers test_buffer.fill(0); - let len = Command::GetReport(ReportType::Feature, REPORT_ID) + let len = Command::GetReport(ReportType::Feature, REPORT_ID, Some(64)) .encode_into_slice(&mut test_buffer, Some(CMD_REG), Some(DATA_REG)) .unwrap(); assert_eq!(&test_buffer[0..len], [0x05, 0x00, 0x38, 0x02, 0x06, 0x00]); // Test extended report id with registers test_buffer.fill(0); - let len = Command::GetReport(ReportType::Input, EXT_REPORT_ID) + let len = Command::GetReport(ReportType::Input, EXT_REPORT_ID, None) .encode_into_slice(&mut test_buffer, Some(CMD_REG), Some(DATA_REG)) .unwrap(); assert_eq!( diff --git a/hid-service/src/i2c/device.rs b/hid-service/src/i2c/device.rs index 85c7596f0..14c7ff4c8 100644 --- a/hid-service/src/i2c/device.rs +++ b/hid-service/src/i2c/device.rs @@ -9,6 +9,8 @@ use embedded_services::{error, hid, info, trace}; use crate::Error; +const LENGTH_PREFIX_SIZE: usize = 2; + /// Timeout configuration for I2C HID device operations. pub struct Config { /// Timeout for descriptor reads and commands. @@ -209,6 +211,19 @@ impl> Device { Error::Hid(hid::Error::Serialize) })?; + let response_size = match cmd { + hid::Command::GetReport(_, _, Some(expected_payload_size)) => { + *expected_payload_size as usize + LENGTH_PREFIX_SIZE + } + _ => buffer_len, + }; + let read_buf = + buf.get_mut(0..response_size) + .ok_or(Error::Hid(hid::Error::InvalidSize(InvalidSizeError { + expected: response_size, + actual: buffer_len, + })))?; + let mut bus = self.bus.lock().await; with_timeout( @@ -221,7 +236,7 @@ impl> Device { expected: len, actual: temp_w_buf.len(), })))?, - buf, + read_buf, ), ) .await @@ -234,7 +249,9 @@ impl> Device { Error::Bus(e) })?; - Ok(Some(Response::FeatureReport(self.buffer.reference()))) + Ok(Some(Response::FeatureReport( + self.buffer.reference().slice(0..response_size).map_err(Error::Buffer)?, + ))) } else { let len = cmd .encode_into_slice( diff --git a/hid-service/src/i2c/host.rs b/hid-service/src/i2c/host.rs index 973800437..f65b9ff1a 100644 --- a/hid-service/src/i2c/host.rs +++ b/hid-service/src/i2c/host.rs @@ -227,7 +227,7 @@ impl Host { // Create command let report_type = hid::ReportType::try_from(cmd).ok(); - let command = hid::Command::new(cmd, opcode, report_type, report_id, buffer); + let command = hid::Command::new(cmd, opcode, report_type, report_id, buffer, None); match command { Ok(command) => Ok(command), Err(e) => { diff --git a/keyboard-service/src/hid_kb.rs b/keyboard-service/src/hid_kb.rs index f72d9a471..e8a6955d7 100644 --- a/keyboard-service/src/hid_kb.rs +++ b/keyboard-service/src/hid_kb.rs @@ -5,12 +5,12 @@ use embassy_sync::channel::Channel; use embassy_sync::once_lock::OnceLock; use embassy_sync::signal::Signal; use embedded_hal::digital::OutputPin; -use embedded_services::GlobalRawMutex; use embedded_services::buffer::SharedRef; use embedded_services::comms; use embedded_services::error; use embedded_services::hid; use embedded_services::ipc::deferred as ipc; +use embedded_services::GlobalRawMutex; use hid_service::i2c::I2cSlaveAsync; use static_cell::StaticCell; @@ -168,7 +168,7 @@ pub async fn handle_keyboard(mut hid_kb: T) -> Result { + hid::Command::GetReport(report_type, report_id, _expected_size) => { { let report = hid_kb.get_report(report_type, report_id); let report = HidI2cReport::from_report_slice(report, max_input_len).to_bytes(); From bb83c01c0c6fb6121b5c903eea27590acddc7845 Mon Sep 17 00:00:00 2001 From: Danielle Hollander Date: Thu, 11 Jun 2026 14:50:50 -0400 Subject: [PATCH 2/7] validate size --- hid-service/src/i2c/device.rs | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/hid-service/src/i2c/device.rs b/hid-service/src/i2c/device.rs index 14c7ff4c8..f8d266413 100644 --- a/hid-service/src/i2c/device.rs +++ b/hid-service/src/i2c/device.rs @@ -10,6 +10,8 @@ use embedded_services::{error, hid, info, trace}; use crate::Error; const LENGTH_PREFIX_SIZE: usize = 2; +const LENGTH_LO_OFFSET: usize = 0; +const LENGTH_HI_OFFSET: usize = 1; /// Timeout configuration for I2C HID device operations. pub struct Config { @@ -211,11 +213,11 @@ impl> Device { Error::Hid(hid::Error::Serialize) })?; - let response_size = match cmd { + let (response_size, constrained) = match cmd { hid::Command::GetReport(_, _, Some(expected_payload_size)) => { - *expected_payload_size as usize + LENGTH_PREFIX_SIZE + (*expected_payload_size as usize + LENGTH_PREFIX_SIZE, true) } - _ => buffer_len, + _ => (buffer_len, false), }; let read_buf = buf.get_mut(0..response_size) @@ -249,6 +251,21 @@ impl> Device { Error::Bus(e) })?; + if constrained { + let actual_frame_len = + u16::from_le_bytes([read_buf[LENGTH_LO_OFFSET], read_buf[LENGTH_HI_OFFSET]]) as usize; + if actual_frame_len != response_size { + error!( + "Length mismatch: declared={} expected={}", + actual_frame_len, response_size + ); + return Err(Error::Hid(hid::Error::InvalidSize(InvalidSizeError { + expected: response_size, + actual: actual_frame_len, + }))); + } + } + Ok(Some(Response::FeatureReport( self.buffer.reference().slice(0..response_size).map_err(Error::Buffer)?, ))) From d4b09abb326bfa8ee6a6299429e887143b110e8e Mon Sep 17 00:00:00 2001 From: Danielle Hollander Date: Sun, 14 Jun 2026 12:41:10 -0400 Subject: [PATCH 3/7] update to struct --- embedded-service/src/hid/command.rs | 77 ++++++++++++++++++++--------- hid-service/src/i2c/device.rs | 11 +++-- keyboard-service/src/hid_kb.rs | 6 ++- 3 files changed, 65 insertions(+), 29 deletions(-) diff --git a/embedded-service/src/hid/command.rs b/embedded-service/src/hid/command.rs index 1cae20a62..340aed334 100644 --- a/embedded-service/src/hid/command.rs +++ b/embedded-service/src/hid/command.rs @@ -225,7 +225,14 @@ impl Opcode { #[allow(missing_docs)] pub enum Command<'a> { Reset, - GetReport(ReportType, ReportId, Option), + GetReport { + /// Report type (Input or Feature) + report_type: ReportType, + /// Report ID + report_id: ReportId, + /// Expected payload size in bytes, used to constrain the response read length + expected_payload_size: Option, + }, SetReport(ReportType, ReportId, SharedRef<'a, u8>), GetIdle(ReportId), SetIdle(ReportId, ReportFreq), @@ -239,7 +246,7 @@ impl From> for Opcode { fn from(value: Command<'_>) -> Self { match value { Command::Reset => Opcode::Reset, - Command::GetReport(_, _, _) => Opcode::GetReport, + Command::GetReport { .. } => Opcode::GetReport, Command::SetReport(_, _, _) => Opcode::SetReport, Command::GetIdle(_) => Opcode::GetIdle, Command::SetIdle(_, _) => Opcode::SetIdle, @@ -311,11 +318,11 @@ impl<'a> Command<'a> { Opcode::Reset => Command::Reset, Opcode::GetReport => { if report_type? == ReportType::Input || report_type? == ReportType::Feature { - Command::GetReport( - report_type?, - report_id.ok_or(Error::RequiresReportId)?, - expected_response_size, - ) + Command::GetReport { + report_type: report_type?, + report_id: report_id.ok_or(Error::RequiresReportId)?, + expected_payload_size: expected_response_size, + } } else { return Err(Error::InvalidReportType); } @@ -481,7 +488,11 @@ impl<'a> Command<'a> { let (command_len, _) = Self::encode_basic_op(buf, Opcode::Reset)?; len += command_len; } - Command::GetReport(report_type, report_id, _expected_size) => { + Command::GetReport { + report_type, + report_id, + expected_payload_size: _, + } => { let (command_len, buf) = Self::encode_common(buf, Opcode::GetReport, Some(*report_type), *report_id)?; len += command_len; @@ -599,37 +610,57 @@ mod test { let mut test_buffer = [0u8; 7]; // Test input report - let len = Command::GetReport(ReportType::Input, REPORT_ID, None) - .encode_into_slice(&mut test_buffer, None, None) - .unwrap(); + let len = Command::GetReport { + report_type: ReportType::Input, + report_id: REPORT_ID, + expected_payload_size: None, + } + .encode_into_slice(&mut test_buffer, None, None) + .unwrap(); assert_eq!(&test_buffer[0..len], [0x18, 0x02]); // Test feature report test_buffer.fill(0); - let len = Command::GetReport(ReportType::Feature, REPORT_ID, None) - .encode_into_slice(&mut test_buffer, None, None) - .unwrap(); + let len = Command::GetReport { + report_type: ReportType::Feature, + report_id: REPORT_ID, + expected_payload_size: None, + } + .encode_into_slice(&mut test_buffer, None, None) + .unwrap(); assert_eq!(&test_buffer[0..len], [0x38, 0x02]); // Test extended report test_buffer.fill(0); - let len = Command::GetReport(ReportType::Input, EXT_REPORT_ID, None) - .encode_into_slice(&mut test_buffer, None, None) - .unwrap(); + let len = Command::GetReport { + report_type: ReportType::Input, + report_id: EXT_REPORT_ID, + expected_payload_size: None, + } + .encode_into_slice(&mut test_buffer, None, None) + .unwrap(); assert_eq!(&test_buffer[0..len], [0x1f, 0x02, EXTENDED_REPORT_ID]); // Test standard report id with registers test_buffer.fill(0); - let len = Command::GetReport(ReportType::Feature, REPORT_ID, Some(64)) - .encode_into_slice(&mut test_buffer, Some(CMD_REG), Some(DATA_REG)) - .unwrap(); + let len = Command::GetReport { + report_type: ReportType::Feature, + report_id: REPORT_ID, + expected_payload_size: Some(64), + } + .encode_into_slice(&mut test_buffer, Some(CMD_REG), Some(DATA_REG)) + .unwrap(); assert_eq!(&test_buffer[0..len], [0x05, 0x00, 0x38, 0x02, 0x06, 0x00]); // Test extended report id with registers test_buffer.fill(0); - let len = Command::GetReport(ReportType::Input, EXT_REPORT_ID, None) - .encode_into_slice(&mut test_buffer, Some(CMD_REG), Some(DATA_REG)) - .unwrap(); + let len = Command::GetReport { + report_type: ReportType::Input, + report_id: EXT_REPORT_ID, + expected_payload_size: None, + } + .encode_into_slice(&mut test_buffer, Some(CMD_REG), Some(DATA_REG)) + .unwrap(); assert_eq!( &test_buffer[0..len], [0x05, 0x00, 0x1f, 0x02, EXTENDED_REPORT_ID, 0x06, 0x00] diff --git a/hid-service/src/i2c/device.rs b/hid-service/src/i2c/device.rs index f8d266413..c117852f3 100644 --- a/hid-service/src/i2c/device.rs +++ b/hid-service/src/i2c/device.rs @@ -1,10 +1,10 @@ use core::borrow::BorrowMut; use embassy_sync::mutex::Mutex; -use embassy_time::{Duration, with_timeout}; +use embassy_time::{with_timeout, Duration}; use embedded_hal_async::i2c::{AddressMode, I2c}; use embedded_services::hid::{DeviceContainer, InvalidSizeError, Opcode, Response}; -use embedded_services::{GlobalRawMutex, buffer::*}; +use embedded_services::{buffer::*, GlobalRawMutex}; use embedded_services::{error, hid, info, trace}; use crate::Error; @@ -214,9 +214,10 @@ impl> Device { })?; let (response_size, constrained) = match cmd { - hid::Command::GetReport(_, _, Some(expected_payload_size)) => { - (*expected_payload_size as usize + LENGTH_PREFIX_SIZE, true) - } + hid::Command::GetReport { + expected_payload_size: Some(expected_payload_size), + .. + } => (*expected_payload_size as usize + LENGTH_PREFIX_SIZE, true), _ => (buffer_len, false), }; let read_buf = diff --git a/keyboard-service/src/hid_kb.rs b/keyboard-service/src/hid_kb.rs index e8a6955d7..9ec17c003 100644 --- a/keyboard-service/src/hid_kb.rs +++ b/keyboard-service/src/hid_kb.rs @@ -168,7 +168,11 @@ pub async fn handle_keyboard(mut hid_kb: T) -> Result { + hid::Command::GetReport { + report_type, + report_id, + expected_payload_size: _, + } => { { let report = hid_kb.get_report(report_type, report_id); let report = HidI2cReport::from_report_slice(report, max_input_len).to_bytes(); From 2e026a957709cbe0aff3082982ee234e57661a9d Mon Sep 17 00:00:00 2001 From: Danielle Hollander Date: Sun, 14 Jun 2026 12:49:18 -0400 Subject: [PATCH 4/7] apply first_chunk --- hid-service/src/i2c/device.rs | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/hid-service/src/i2c/device.rs b/hid-service/src/i2c/device.rs index c117852f3..4a2e2a3c3 100644 --- a/hid-service/src/i2c/device.rs +++ b/hid-service/src/i2c/device.rs @@ -10,8 +10,6 @@ use embedded_services::{error, hid, info, trace}; use crate::Error; const LENGTH_PREFIX_SIZE: usize = 2; -const LENGTH_LO_OFFSET: usize = 0; -const LENGTH_HI_OFFSET: usize = 1; /// Timeout configuration for I2C HID device operations. pub struct Config { @@ -253,8 +251,13 @@ impl> Device { })?; if constrained { - let actual_frame_len = - u16::from_le_bytes([read_buf[LENGTH_LO_OFFSET], read_buf[LENGTH_HI_OFFSET]]) as usize; + let actual_frame_len = read_buf + .first_chunk::() + .map(|b| u16::from_le_bytes(*b) as usize) + .ok_or(Error::Hid(hid::Error::InvalidSize(InvalidSizeError { + expected: LENGTH_PREFIX_SIZE, + actual: read_buf.len(), + })))?; if actual_frame_len != response_size { error!( "Length mismatch: declared={} expected={}", From c6e242e688d342d01b950c72d549438c8c88101e Mon Sep 17 00:00:00 2001 From: Danielle Hollander Date: Tue, 16 Jun 2026 02:10:29 -0400 Subject: [PATCH 5/7] update length compare --- hid-service/src/i2c/device.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hid-service/src/i2c/device.rs b/hid-service/src/i2c/device.rs index 4a2e2a3c3..4ba5b36d8 100644 --- a/hid-service/src/i2c/device.rs +++ b/hid-service/src/i2c/device.rs @@ -258,7 +258,7 @@ impl> Device { expected: LENGTH_PREFIX_SIZE, actual: read_buf.len(), })))?; - if actual_frame_len != response_size { + if actual_frame_len > response_size { error!( "Length mismatch: declared={} expected={}", actual_frame_len, response_size From eee16dec0581154985c41d7aeb905f49ea7fbd63 Mon Sep 17 00:00:00 2001 From: Danielle Hollander Date: Sun, 21 Jun 2026 07:07:45 -0400 Subject: [PATCH 6/7] update - return actual_frame_len --- hid-service/src/i2c/device.rs | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/hid-service/src/i2c/device.rs b/hid-service/src/i2c/device.rs index 4ba5b36d8..4263a36c5 100644 --- a/hid-service/src/i2c/device.rs +++ b/hid-service/src/i2c/device.rs @@ -1,10 +1,10 @@ use core::borrow::BorrowMut; use embassy_sync::mutex::Mutex; -use embassy_time::{with_timeout, Duration}; +use embassy_time::{Duration, with_timeout}; use embedded_hal_async::i2c::{AddressMode, I2c}; use embedded_services::hid::{DeviceContainer, InvalidSizeError, Opcode, Response}; -use embedded_services::{buffer::*, GlobalRawMutex}; +use embedded_services::{GlobalRawMutex, buffer::*}; use embedded_services::{error, hid, info, trace}; use crate::Error; @@ -250,7 +250,7 @@ impl> Device { Error::Bus(e) })?; - if constrained { + let returned_len = if constrained { let actual_frame_len = read_buf .first_chunk::() .map(|b| u16::from_le_bytes(*b) as usize) @@ -258,20 +258,23 @@ impl> Device { expected: LENGTH_PREFIX_SIZE, actual: read_buf.len(), })))?; - if actual_frame_len > response_size { + if actual_frame_len < LENGTH_PREFIX_SIZE || actual_frame_len > response_size { error!( - "Length mismatch: declared={} expected={}", - actual_frame_len, response_size + "Length mismatch: declared={} expected<={} min={}", + actual_frame_len, response_size, LENGTH_PREFIX_SIZE ); return Err(Error::Hid(hid::Error::InvalidSize(InvalidSizeError { expected: response_size, actual: actual_frame_len, }))); } - } + actual_frame_len + } else { + response_size + }; Ok(Some(Response::FeatureReport( - self.buffer.reference().slice(0..response_size).map_err(Error::Buffer)?, + self.buffer.reference().slice(0..returned_len).map_err(Error::Buffer)?, ))) } else { let len = cmd From 23c79e6736840f087947e83e8eaa3611be2bf2ea Mon Sep 17 00:00:00 2001 From: Danielle Hollander Date: Sun, 21 Jun 2026 07:20:53 -0400 Subject: [PATCH 7/7] format (fmt) --- keyboard-service/src/hid_kb.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/keyboard-service/src/hid_kb.rs b/keyboard-service/src/hid_kb.rs index 9ec17c003..7fb8f0b28 100644 --- a/keyboard-service/src/hid_kb.rs +++ b/keyboard-service/src/hid_kb.rs @@ -5,12 +5,12 @@ use embassy_sync::channel::Channel; use embassy_sync::once_lock::OnceLock; use embassy_sync::signal::Signal; use embedded_hal::digital::OutputPin; +use embedded_services::GlobalRawMutex; use embedded_services::buffer::SharedRef; use embedded_services::comms; use embedded_services::error; use embedded_services::hid; use embedded_services::ipc::deferred as ipc; -use embedded_services::GlobalRawMutex; use hid_service::i2c::I2cSlaveAsync; use static_cell::StaticCell;