Skip to content

Add expected_response_size to GetReport command #888

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
daniellehadar wants to merge 2 commits into
base: main
Choose a base branch
from
+53 −14
Open

Add expected_response_size to GetReport command #888

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
374b5f6
Add expected_response_size to GetReport command
Jun 8, 2026
bb83c01
validate size
Jun 11, 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
23 changes: 14 additions & 9 deletions embedded-service/src/hid/command.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ impl Opcode {
#[allow(missing_docs)]
pub enum Command<'a> {
Reset,
GetReport(ReportType, ReportId),
GetReport(ReportType, ReportId, Option<u16>),

@jerrysxie jerrysxie Jun 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

non-blocking: not loving this unamed tuple element that is u16? Maybe instead of tuple, we can use struct?

    GetReport {
        report_type: ReportType,
        report_id: ReportId,
        ExpectedPayloadSize: Option<u16>,
    },

SetReport(ReportType, ReportId, SharedRef<'a, u8>),

@jerrysxie jerrysxie Jun 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This additional size constraint is very specific to a I2C transport, so it feels a bit weird to thread it into the general HID command layer. Is this expected size expected to change on the fly dynamically? It feels like this should belong to the I2C device layer instead, but currently there is isno good separation between I2C and HID in the current code. What do you think? @williampMSFT Leave this here for now, since it will be refactored soon anyway. Will the refactored code take this case into account?

GetIdle(ReportId),
SetIdle(ReportId, ReportFreq),
Expand All @@ -239,7 +239,7 @@ impl From<Command<'_>> 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,
Expand Down Expand Up @@ -293,6 +293,7 @@ impl<'a> Command<'a> {
report_type: Option<ReportType>,
report_id: Option<ReportId>,
data: Option<SharedRef<'a, u8>>,
expected_response_size: Option<u16>,
) -> Result<Self, Error> {
if opcode.requires_report_id() && report_id.is_none() {
return Err(Error::RequiresReportId);
Expand All @@ -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);
}
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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!(
Expand Down
38 changes: 36 additions & 2 deletions hid-service/src/i2c/device.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@ 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 {
/// Timeout for descriptor reads and commands.
Expand Down Expand Up @@ -209,6 +213,19 @@ impl<A: AddressMode + Copy, B: I2c<A>> Device<A, B> {
Error::Hid(hid::Error::Serialize)
})?;

let (response_size, constrained) = match cmd {
hid::Command::GetReport(_, _, Some(expected_payload_size)) => {
(*expected_payload_size as usize + LENGTH_PREFIX_SIZE, true)
}
_ => (buffer_len, false),
};
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(
Expand All @@ -221,7 +238,7 @@ impl<A: AddressMode + Copy, B: I2c<A>> Device<A, B> {
expected: len,
actual: temp_w_buf.len(),
})))?,
buf,
read_buf,
),
)
.await
Expand All @@ -234,7 +251,24 @@ impl<A: AddressMode + Copy, B: I2c<A>> Device<A, B> {
Error::Bus(e)
})?;

Ok(Some(Response::FeatureReport(self.buffer.reference())))
if constrained {
let actual_frame_len =
u16::from_le_bytes([read_buf[LENGTH_LO_OFFSET], read_buf[LENGTH_HI_OFFSET]]) as usize;

@jerrysxie jerrysxie Jun 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This will fail CI as no panic clippy lints are enabled. We can use at() to return a result.

@jerrysxie jerrysxie Jun 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Instead of indexing twice, since we know the length is always in the front of the buffer, can we do something like this instead?

                let actual_frame_len = read_buf
                    .first_chunk::<LENGTH_PREFIX_SIZE>()
                    .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 {

@jerrysxie jerrysxie Jun 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is a strict comparison check so the device has to return an exact size. But according to the hidi2c spec, the device may return fewer bytes than the max size for a report. Would this work for other passthrough cases? @wmmc88 @RobertZ2011

@RobertZ2011 RobertZ2011 Jun 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We should support fewer bytes if the spec says that it's valid.

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)?,
)))
} else {
let len = cmd
.encode_into_slice(
Expand Down
2 changes: 1 addition & 1 deletion hid-service/src/i2c/host.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ impl<B: I2cSlaveAsync> Host<B> {

// 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) => {
Expand Down
4 changes: 2 additions & 2 deletions keyboard-service/src/hid_kb.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -168,7 +168,7 @@ pub async fn handle_keyboard<T: HidKeyboard>(mut hid_kb: T) -> Result<embedded_s
}

// Instructs the keyboard to immediately return the latest input/feature report
hid::Command::GetReport(report_type, report_id) => {
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();
Expand Down
Loading