Add expected_response_size to GetReport command#888
Conversation
Add an optional buffer size parameter to GetReport so the I2C device layer can limit the read length to the actual expected payload size instead of always reading the full buffer. This avoids over-reading on devices that are sensitive to read length in write_read transactions.
daniellehadar
requested review from
RobertZ2011,
felipebalbi,
kurtjd and
williampMSFT
jerrysxie
left a comment
jerrysxie
left a comment
There was a problem hiding this comment.
Approved, pending fix for one failed CI on formatting.
| } | ||
| } | ||
|
|
||
| Ok(Some(Response::FeatureReport( |
There was a problem hiding this comment.
if the actual_frame_len is smaller than the response_size, I think we should return the actual_frame_len
if actual_frame_len > response_size { return Err(/* InvalidSize */); }
let returned_len = if constrained { actual_frame_len } else { response_size };
Ok(Some(Response::FeatureReport(
self.buffer.reference().slice(0..returned_len).map_err(Error::Buffer)?,
)))
| [0x05, 0x00, 0x1f, 0x02, EXTENDED_REPORT_ID, 0x06, 0x00] | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
Do we want to add the relevant tests?
(a) constrained read pulls exactly payload + 2 bytes and returns a slice of that length;
(b) device declaring a longer frame (actual_frame_len > response_size) → Err(InvalidSize);
(c) expected_payload_size exceeding the buffer → Err(InvalidSize) from get_mut.
There was a problem hiding this comment.
It will be a bit tough to enable unit test in this module without some refactoring due to the static lifetime and we will need mocks for the i2c bus. Since @williampMSFT is refactoring this module right now, it is probably out of the scope for this PR.
5 participants
Add an optional buffer size parameter to GetReport so the I2C device layer can limit the read length to the actual expected payload size instead of always reading the full buffer. This avoids over-reading response in write_read transactions.