Skip to content

Backport commit e968767: Fix I2C HID command to use write_read for response commands (#867)#879

Merged
jerrysxie merged 2 commits into
OpenDevicePartnership:stable-v0.1.yfrom
jerrysxie:backport-hid-fix
Jun 5, 2026
Merged

Backport commit e968767: Fix I2C HID command to use write_read for response commands (#867)#879
jerrysxie merged 2 commits into
OpenDevicePartnership:stable-v0.1.yfrom
jerrysxie:backport-hid-fix

Conversation

@jerrysxie
Copy link
Copy Markdown
Contributor

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

…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
@jerrysxie jerrysxie self-assigned this Jun 3, 2026
@jerrysxie jerrysxie marked this pull request as ready for review June 3, 2026 17:16
@jerrysxie jerrysxie requested a review from a team as a code owner June 3, 2026 17:16
Copilot AI review requested due to automatic review settings June 3, 2026 17:16
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

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 write only, with simplified data_reg conditional logic.

Step-by-step review guide

  1. Response commands now use a single I2C transaction (write_read)

    • The opcode.has_response() branch encodes the command into temp_w_buf and performs bus.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).
  2. Non-response commands remain write

    • The else branch continues to encode directly into the shared buffer and uses bus.write(...).
    • The data_reg inclusion is now based solely on opcode.requires_host_data() in this branch, reducing unnecessary branching.
  3. Timeout semantics changed for response commands

    • Previously, the read phase used data_read_timeout; now the combined transaction uses device_response_timeout. This changes behavior and may cause spurious timeouts on large/slow feature reads.

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(...))

Comment thread hid-service/src/i2c/device.rs
@jerrysxie jerrysxie enabled auto-merge (squash) June 3, 2026 19:00
@jerrysxie jerrysxie requested review from felipebalbi and tullom June 4, 2026 19:15
@jerrysxie jerrysxie merged commit 567c360 into OpenDevicePartnership:stable-v0.1.y Jun 5, 2026
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants