Backport commit e968767: Fix I2C HID command to use write_read for response commands (#867)#879
Conversation
…cePartnership#867) Per the HID over I2C Protocol Spec v1.0, commands that require a response (GetReport, GetIdle, GetProtocol) must be performed as a single I2C transaction with a repeated START condition (no STOP between the write and read phases). The previous implementation used separate write and read operations, which inserted a STOP condition between them, violating the spec. Use write_read with a separate stack-allocated command buffer (max 7 bytes for these commands) so the command is sent and the response is read in a single I2C transaction. Also simplify the data_reg conditional logic in both branches to remove dead-code conditions. Assisted-by: GitHub Copilot:claude-opus-4.6
There was a problem hiding this comment.
Pull request overview
Summary of changes
This PR updates the I2C HID device command path to comply with the HID over I2C Protocol v1.0 requirement that response-producing commands (GetReport/GetIdle/GetProtocol) be executed as a single I2C transaction using a repeated START (i.e., no STOP between write and read). It replaces the previous “write then read” sequence with a single write_read call, using a small stack buffer for the encoded command to avoid reusing the shared response buffer for the write phase. It also simplifies the data_reg selection logic by removing conditions that were effectively dead for certain opcode categories.
Changes:
- Switch response commands to
write_read(repeated START) using a stack-allocated command buffer. - Keep non-response commands as
writeonly, with simplifieddata_regconditional logic.
Step-by-step review guide
-
Response commands now use a single I2C transaction (
write_read)- The
opcode.has_response()branch encodes the command intotemp_w_bufand performsbus.write_read(address, cmd_bytes, buf)under a timeout. - The fixed-size
[u8; 7]buffer is consistent with existing command serialization tests (GetReport/GetIdle with registers can reach 7 bytes; GetProtocol with registers is 6).
- The
-
Non-response commands remain
write- The
elsebranch continues to encode directly into the shared buffer and usesbus.write(...). - The
data_reginclusion is now based solely onopcode.requires_host_data()in this branch, reducing unnecessary branching.
- The
-
Timeout semantics changed for response commands
- Previously, the read phase used
data_read_timeout; now the combined transaction usesdevice_response_timeout. This changes behavior and may cause spurious timeouts on large/slow feature reads.
- Previously, the read phase used
Potential issues
| # | Severity | File | Description | Code |
|---|---|---|---|---|
| 1 | 🟡 Medium | hid-service/src/i2c/device.rs:214-216 |
write_read for response commands uses device_response_timeout, but feature-data reads were previously governed by data_read_timeout (and Config docs define it that way). This can cause premature timeouts for legitimate response reads. |
with_timeout(self.timeout_config.device_response_timeout, bus.write_read(...)) |
567c360
into
OpenDevicePartnership:stable-v0.1.y
Per the HID over I2C Protocol Spec v1.0, commands that require a response (GetReport, GetIdle, GetProtocol) must be performed as a single I2C transaction with a repeated START condition (no STOP between the write and read phases).
The previous implementation used separate write and read operations, which inserted a STOP condition between them, violating the spec. Use write_read with a separate stack-allocated command buffer (max 7 bytes for these commands) so the command is sent and the response is read in a single I2C transaction.
Also simplify the data_reg conditional logic in both branches to remove dead-code conditions.
Assisted-by: GitHub Copilot:claude-opus-4.6