hid-service: add timeout for I2C device operations#697
hid-service: add timeout for I2C device operations#697jerrysxie wants to merge 1 commit intoOpenDevicePartnership:mainfrom
Conversation
Wrap all I2C master bus operations with timeouts to prevent indefinite blocking when communicating with HID devices. - Add DEVICE_RESPONSE_TIMEOUT_MS (200ms) for descriptor reads and commands - Add DATA_READ_TIMEOUT_MS (50ms) for input reports and feature data - Return Error::Hid(hid::Error::Timeout) on timeout expiry
|
|
||
| use crate::Error; | ||
|
|
||
| const DEVICE_RESPONSE_TIMEOUT_MS: u64 = 200; |
There was a problem hiding this comment.
@RobertZ2011 I copied these timeouts from the host.rs. Do these timeouts make sense for communication with HID passthrough device?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Pull request overview
This PR adds bounded timeouts around all I2C master operations performed by the HID I2C device side, preventing indefinite blocking when devices misbehave or stop responding.
Changes:
- Introduced
DEVICE_RESPONSE_TIMEOUT_MS(200 ms) andDATA_READ_TIMEOUT_MS(50 ms) in the I2C device implementation. - Wrapped all
bus.write_read,bus.read, andbus.writecalls inhid-service/src/i2c/device.rswithembassy_time::with_timeout, mapping timeout expiries toError::Hid(hid::Error::Timeout). - Added per-call logging for timeout vs. underlying bus errors for descriptor reads, report descriptor reads, input reports, and command/host-data transfers.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| const DEVICE_RESPONSE_TIMEOUT_MS: u64 = 200; | ||
| const DATA_READ_TIMEOUT_MS: u64 = 50; | ||
|
|
There was a problem hiding this comment.
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.
| const DEVICE_RESPONSE_TIMEOUT_MS: u64 = 200; | |
| const DATA_READ_TIMEOUT_MS: u64 = 50; | |
| use crate::{DEVICE_RESPONSE_TIMEOUT_MS, DATA_READ_TIMEOUT_MS}; |
Wrap all I2C master bus operations with timeouts to prevent indefinite blocking when communicating with HID devices.