fix: writing data via usb on windows #89
Open
Conversation
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
Reviewer's GuideIntroduce Windows-specific chunked USB HID write logic by looping writes with report ID prepended, add a helper to split data into 64-byte packets, and correct a typo in the payload generation function name. Class diagram for updated USB HID write logicclassDiagram
class HidDevice {
+write(&self, data: &[u8]) -> Result<usize>
}
class usb_hid {
+write_raw(device: &HidDevice, data: &[u8]) -> Result<()>
+prepend_byte_and_offset(data: &[u8], offset: usize) -> [u8; 65]
}
HidDevice <.. usb_hid : uses
Class diagram for corrected payload generation functionclassDiagram
class Args {
+config: Option<String>
}
class main {
+generate_payload(args: &mut Args) -> Result<PayloadBuffer>
+write_payload(transport: &TransportProtocol, payload: PayloadBuffer) -> Result<()>
}
Args <.. main : uses
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @RedCommander735 - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Potential buffer overrun in chunked write loop. (link)
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/usb_hid.rs:107` </location>
<code_context>
+ {
+ written = 0;
+
+ while written < data.len() {
+ let new_data: &[u8] = &prepend_byte_and_offset(data, written);
+ let n = device.write(new_data).context("write payload")?;
+ written = written + n - 1;
</code_context>
<issue_to_address>
Potential buffer overrun in chunked write loop.
If less than 64 bytes remain, slicing 'data[offset..offset + 64]' will panic. Please handle the final chunk to prevent out-of-bounds access.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @RedCommander735 - I've reviewed your changes - here's some feedback:
- The write loop on Windows subtracts 1 from each write count, so if device.write ever returns 0 or 1 this could spin indefinitely—consider erroring or breaking out when n <= 1 to avoid infinite loops.
- The hardcoded report ID (0x0) in prepend_byte_and_offset limits flexibility—consider parameterizing this value or documenting its assumptions for different HID devices.
- You might extract the chunking and packet-preparation into a reusable iterator or helper struct to make the write logic more declarative and testable, reducing manual loop complexity.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The write loop on Windows subtracts 1 from each write count, so if device.write ever returns 0 or 1 this could spin indefinitely—consider erroring or breaking out when n <= 1 to avoid infinite loops.
- The hardcoded report ID (0x0) in prepend_byte_and_offset limits flexibility—consider parameterizing this value or documenting its assumptions for different HID devices.
- You might extract the chunking and packet-preparation into a reusable iterator or helper struct to make the write logic more declarative and testable, reducing manual loop complexity.
## Individual Comments
### Comment 1
<location> `src/usb_hid.rs:107` </location>
<code_context>
+ {
+ written = 0;
+
+ while written < data.len() {
+ let new_data: &[u8] = &prepend_byte_and_offset(data, written);
+ let n = device.write(new_data).context("write payload")?;
</code_context>
<issue_to_address>
Potential for infinite loop if device.write returns 1.
Since written does not increase when device.write returns 1, the loop may never terminate. Please add a check or safeguard for this scenario.
</issue_to_address>
### Comment 2
<location> `src/usb_hid.rs:128` </location>
<code_context>
Ok(())
}
+
+fn prepend_byte_and_offset(data: &[u8], offset: usize) -> [u8; 65] {
+ let mut result: [u8; 65] = [0u8; 65];
+ result[1] = 0x0;
</code_context>
<issue_to_address>
Possible out-of-bounds panic if offset > data.len().
Add a debug assertion or explicit check to ensure offset does not exceed data.len().
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| data.len() | ||
| ); | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| fn prepend_byte_and_offset(data: &[u8], offset: usize) -> [u8; 65] { |
There was a problem hiding this comment.
issue (bug_risk): Possible out-of-bounds panic if offset > data.len().
Add a debug assertion or explicit check to ensure offset does not exceed data.len().
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @RedCommander735 - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/usb_hid.rs:107` </location>
<code_context>
+ {
+ written = 0;
+
+ while written < data.len() {
+ let new_data: &[u8] = &prepend_byte_and_offset(data, written);
+ let n = device.write(new_data).context("write payload")?;
+ written = written + n - 1;
+ }
+ }
</code_context>
<issue_to_address>
Potential off-by-one error in written byte calculation.
Subtracting 1 from 'written + n' may cause incorrect loop termination or skipped bytes, particularly when 'n' is 0 or 1. Please verify if this adjustment is needed.
</issue_to_address>
### Comment 2
<location> `src/usb_hid.rs:131` </location>
<code_context>
+fn prepend_byte_and_offset(data: &[u8], offset: usize) -> [u8; 65] {
+ let mut result: [u8; 65] = [0u8; 65];
+ result[1] = 0x0;
+ if offset > data.len() {
+ return result;
+ }
+ if data.len() - offset < 64 {
</code_context>
<issue_to_address>
Returning a zeroed buffer when offset exceeds data length may mask errors.
Returning a zeroed buffer in this case may hide upstream logic errors. Instead, consider returning an error or panicking to make the issue explicit.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+107
to
+110
| while written < data.len() { | ||
| let new_data: &[u8] = &prepend_byte_and_offset(data, written); | ||
| let n = device.write(new_data).context("write payload")?; | ||
| written = written + n - 1; |
There was a problem hiding this comment.
issue (bug_risk): Potential off-by-one error in written byte calculation.
Subtracting 1 from 'written + n' may cause incorrect loop termination or skipped bytes, particularly when 'n' is 0 or 1. Please verify if this adjustment is needed.
Comment on lines
+131
to
+132
| if offset > data.len() { | ||
| return result; |
There was a problem hiding this comment.
suggestion (bug_risk): Returning a zeroed buffer when offset exceeds data length may mask errors.
Returning a zeroed buffer in this case may hide upstream logic errors. Instead, consider returning an error or panicking to make the issue explicit.
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.
ref #25
Seems to have been a combination of both. This works on my machine
Summary by Sourcery
Improve USB HID write functionality on Windows to handle data larger than single reports and correct a function naming typo.
Bug Fixes:
Enhancements: