Skip to content

hid-service: add timeout for I2C device operations #697

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

Draft
jerrysxie wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

hid-service: add timeout for I2C device operations #697

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
87 changes: 62 additions & 25 deletions hid-service/src/i2c/device.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,17 @@
use core::borrow::BorrowMut;

use embassy_sync::mutex::Mutex;
use embassy_time::{Duration, with_timeout};
use embedded_hal_async::i2c::{AddressMode, I2c};
use embedded_services::hid::{DeviceContainer, InvalidSizeError, Opcode, Response};
use embedded_services::{GlobalRawMutex, buffer::*};
use embedded_services::{error, hid, info, trace};

use crate::Error;

const DEVICE_RESPONSE_TIMEOUT_MS: u64 = 200;
Copy link
Contributor Author

@jerrysxie jerrysxie Jan 28, 2026

Choose a reason for hiding this comment

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

@RobertZ2011 I copied these timeouts from the host.rs. Do these timeouts make sense communication with HID passthrough device?

Copy link
Contributor

@RobertZ2011 RobertZ2011 Jan 29, 2026

Choose a reason for hiding this comment

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

200 ms is a good default that should be good for most use cases, but long-term I think these should be configuration options passed into the device. The spec uses 16 bits for length and that would take ~5 seconds at 100 KHz.

const DATA_READ_TIMEOUT_MS: u64 = 50;

Comment on lines 11 to +14
Copy link

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

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

DEVICE_RESPONSE_TIMEOUT_MS and DATA_READ_TIMEOUT_MS are defined here with the same values as in i2c/host.rs, which duplicates configuration and could get out of sync if one side is tuned independently. Consider defining these timeouts in a shared module (e.g. the i2c mod or crate root) and reusing them in both host and device to keep the behavior consistent and easier to adjust.

Suggested change
const DEVICE_RESPONSE_TIMEOUT_MS: u64 = 200;
const DATA_READ_TIMEOUT_MS: u64 = 50;
use crate::{DEVICE_RESPONSE_TIMEOUT_MS, DATA_READ_TIMEOUT_MS};

Copilot uses AI. Check for mistakes.
pub struct Device<A: AddressMode + Copy, B: I2c<A>> {
device: hid::Device,
buffer: OwnedRef<'static, u8>,
Expand Down Expand Up @@ -47,10 +51,19 @@ impl<A: AddressMode + Copy, B: I2c<A>> Device<A, B> {
})))?;

reg.copy_from_slice(&self.device.regs.hid_desc_reg.to_le_bytes());
if let Err(e) = bus.write_read(self.address, &reg, buf).await {
with_timeout(
Duration::from_millis(DEVICE_RESPONSE_TIMEOUT_MS),
bus.write_read(self.address, &reg, buf),
)
.await
.map_err(|_| {
error!("Read HID descriptor timeout");
Error::Hid(hid::Error::Timeout)
})?
.map_err(|e| {
error!("Failed to read HID descriptor");
return Err(Error::Bus(e));
}
Error::Bus(e)
})?;

let res = hid::Descriptor::decode_from_slice(buf);
match res {
Expand Down Expand Up @@ -89,21 +102,27 @@ impl<A: AddressMode + Copy, B: I2c<A>> Device<A, B> {
let len = desc.w_report_desc_length as usize;

let mut bus = self.bus.lock().await;
if let Err(e) = bus
.write_read(
with_timeout(
Duration::from_millis(DEVICE_RESPONSE_TIMEOUT_MS),
bus.write_read(
self.address,
&reg,
buf.get_mut(0..len)
.ok_or(Error::Hid(hid::Error::InvalidSize(InvalidSizeError {
expected: len,
actual: buffer_len,
})))?,
)
.await
{
),
)
.await
.map_err(|_| {
error!("Read report descriptor timeout");
Error::Hid(hid::Error::Timeout)
})?
.map_err(|e| {
error!("Failed to read report descriptor");
return Err(Error::Bus(e));
}
Error::Bus(e)
})?;

self.buffer.reference().slice(0..len).map_err(Error::Buffer)
}
Expand All @@ -123,10 +142,16 @@ impl<A: AddressMode + Copy, B: I2c<A>> Device<A, B> {
})))?;

let mut bus = self.bus.lock().await;
if let Err(e) = bus.read(self.address, buf).await {
error!("Failed to read input report");
return Err(Error::Bus(e));
}
with_timeout(Duration::from_millis(DATA_READ_TIMEOUT_MS), bus.read(self.address, buf))
.await
.map_err(|_| {
error!("Read input report timeout");
Error::Hid(hid::Error::Timeout)
})?
.map_err(|e| {
error!("Failed to read input report");
Error::Bus(e)
})?;

self.buffer
.reference()
Expand Down Expand Up @@ -164,27 +189,39 @@ impl<A: AddressMode + Copy, B: I2c<A>> Device<A, B> {
})?;

let mut bus = self.bus.lock().await;
if let Err(e) = bus
.write(
with_timeout(
Duration::from_millis(DEVICE_RESPONSE_TIMEOUT_MS),
bus.write(
self.address,
buf.get(..len)
.ok_or(Error::Hid(hid::Error::InvalidSize(InvalidSizeError {
expected: len,
actual: buffer_len,
})))?,
)
.await
{
),
)
.await
.map_err(|_| {
error!("Write command timeout");
Error::Hid(hid::Error::Timeout)
})?
.map_err(|e| {
error!("Failed to write command");
return Err(Error::Bus(e));
}
Error::Bus(e)
})?;

if opcode.has_response() {
trace!("Reading host data");
if let Err(e) = bus.read(self.address, buf).await {
error!("Failed to read host data");
return Err(Error::Bus(e));
}
with_timeout(Duration::from_millis(DATA_READ_TIMEOUT_MS), bus.read(self.address, buf))
.await
.map_err(|_| {
error!("Read host data timeout");
Error::Hid(hid::Error::Timeout)
})?
.map_err(|e| {
error!("Failed to read host data");
Error::Bus(e)
})?;

return Ok(Some(Response::FeatureReport(self.buffer.reference())));
}
Expand Down