From a03acbe938d813cc3acce6ed42c937ef763a1f8c Mon Sep 17 00:00:00 2001 From: Jerry Xie Date: Wed, 27 May 2026 16:42:59 -0500 Subject: [PATCH 1/2] Fix I2C HID command to use write_read for response commands 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 --- hid-service/src/i2c/device.rs | 117 +++++++++++++++++++++------------- 1 file changed, 71 insertions(+), 46 deletions(-) diff --git a/hid-service/src/i2c/device.rs b/hid-service/src/i2c/device.rs index b64e9203d..28a677a3e 100644 --- a/hid-service/src/i2c/device.rs +++ b/hid-service/src/i2c/device.rs @@ -196,60 +196,85 @@ impl> Device { let buffer_len = buf.len(); let opcode: Opcode = cmd.into(); - let len = cmd - .encode_into_slice( - buf, - Some(command_reg), - if opcode.has_response() || opcode.requires_host_data() { - Some(data_reg) - } else { - None - }, + + if opcode.has_response() { + // cmds that require a response like GetReport, GetIdle, and GetProtocol + // has an upper limit of 7 bytes for the command + let mut temp_w_buf = [0u8; 7]; + + let len = cmd + .encode_into_slice(&mut temp_w_buf, Some(command_reg), Some(data_reg)) + .map_err(|_| { + error!("Failed to serialize command"); + Error::Hid(hid::Error::Serialize) + })?; + + let mut bus = self.bus.lock().await; + + with_timeout( + self.timeout_config.data_read_timeout, + bus.write_read( + self.address, + temp_w_buf + .get(..len) + .ok_or(Error::Hid(hid::Error::InvalidSize(InvalidSizeError { + expected: len, + actual: temp_w_buf.len(), + })))?, + buf, + ), ) + .await .map_err(|_| { - error!("Failed to serialize command"); - Error::Hid(hid::Error::Serialize) + error!("Read host data timeout"); + Error::Hid(hid::Error::Timeout) + })? + .map_err(|e| { + error!("Failed to read host data"); + Error::Bus(e) })?; - let mut bus = self.bus.lock().await; - with_timeout( - self.timeout_config.device_response_timeout, - bus.write( - self.address, - buf.get(..len) - .ok_or(Error::Hid(hid::Error::InvalidSize(InvalidSizeError { - expected: len, - actual: buffer_len, - })))?, - ), - ) - .await - .map_err(|_| { - error!("Write command timeout"); - Error::Hid(hid::Error::Timeout) - })? - .map_err(|e| { - error!("Failed to write command"); - Error::Bus(e) - })?; - - if opcode.has_response() { - trace!("Reading host data"); - with_timeout(self.timeout_config.data_read_timeout, bus.read(self.address, buf)) - .await + Ok(Some(Response::FeatureReport(self.buffer.reference()))) + } else { + let len = cmd + .encode_into_slice( + buf, + Some(command_reg), + if opcode.requires_host_data() { + Some(data_reg) + } else { + None + }, + ) .map_err(|_| { - error!("Read host data timeout"); - Error::Hid(hid::Error::Timeout) - })? - .map_err(|e| { - error!("Failed to read host data"); - Error::Bus(e) + error!("Failed to serialize command"); + Error::Hid(hid::Error::Serialize) })?; - return Ok(Some(Response::FeatureReport(self.buffer.reference()))); - } + let mut bus = self.bus.lock().await; + with_timeout( + self.timeout_config.device_response_timeout, + bus.write( + self.address, + buf.get(..len) + .ok_or(Error::Hid(hid::Error::InvalidSize(InvalidSizeError { + expected: len, + actual: buffer_len, + })))?, + ), + ) + .await + .map_err(|_| { + error!("Write command timeout"); + Error::Hid(hid::Error::Timeout) + })? + .map_err(|e| { + error!("Failed to write command"); + Error::Bus(e) + })?; - Ok(None) + Ok(None) + } } pub async fn process_request(&self) -> Result<(), Error> { From ac6fd95aa8638f78e7504ccb1f1d2522eefb60c1 Mon Sep 17 00:00:00 2001 From: Jerry Xie Date: Wed, 27 May 2026 17:48:47 -0500 Subject: [PATCH 2/2] Address PR review feedback for I2C HID write_read change - 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 --- hid-service/src/i2c/device.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/hid-service/src/i2c/device.rs b/hid-service/src/i2c/device.rs index 28a677a3e..85c7596f0 100644 --- a/hid-service/src/i2c/device.rs +++ b/hid-service/src/i2c/device.rs @@ -198,8 +198,8 @@ impl> Device { let opcode: Opcode = cmd.into(); if opcode.has_response() { - // cmds that require a response like GetReport, GetIdle, and GetProtocol - // has an upper limit of 7 bytes for the command + // Commands that require a response (GetReport, GetIdle, GetProtocol) + // have an upper limit of 7 bytes for the command let mut temp_w_buf = [0u8; 7]; let len = cmd @@ -212,7 +212,7 @@ impl> Device { let mut bus = self.bus.lock().await; with_timeout( - self.timeout_config.data_read_timeout, + self.timeout_config.device_response_timeout, bus.write_read( self.address, temp_w_buf @@ -226,11 +226,11 @@ impl> Device { ) .await .map_err(|_| { - error!("Read host data timeout"); + error!("Command write_read timeout"); Error::Hid(hid::Error::Timeout) })? .map_err(|e| { - error!("Failed to read host data"); + error!("Failed to execute command write_read"); Error::Bus(e) })?;