Fix I2C HID command to use write_read for response commands#867
Open
jerrysxie wants to merge 2 commits into
Open
Fix I2C HID command to use write_read for response commands#867jerrysxie wants to merge 2 commits into
jerrysxie wants to merge 2 commits into
Conversation
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
Contributor
There was a problem hiding this comment.
Pull request overview
Updates the HID-over-I2C device-side command handling to comply with the HID over I2C v1.0 requirement that response-producing commands be executed as a single I2C transaction (write + repeated START + read), avoiding an intervening STOP.
Changes:
- Switches response-required HID commands (e.g., GetReport/GetIdle/GetProtocol) to use
I2c::write_readwith a small stack command buffer. - Simplifies the command encoding path by separating response vs non-response command handling.
- Adjusts conditional inclusion of
data_regto remove dead-code branches.
williampMSFT
previously approved these changes
May 27, 2026
- Use device_response_timeout instead of data_read_timeout for the write_read transaction to preserve the original write timeout - Update error log messages to reflect the combined write_read operation instead of just 'read host data' - Fix comment grammar: 'cmds ... has' -> 'Commands ... have' Assisted-by: GitHub Copilot:claude-opus-4.6
wmmc88
approved these changes
May 27, 2026
RobertZ2011
approved these changes
May 27, 2026
williampMSFT
approved these changes
May 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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