Skip to content

Fix missing SETUP DataOut packets #94

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Fix missing SETUP DataOut packets #94

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 15 additions & 8 deletions src/control_pipe.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,10 +89,18 @@ impl<B: UsbBus> ControlPipe<'_, B> {
// a stalled state.
self.ep_out.unstall();

/*sprintln!("SETUP {:?} {:?} {:?} req:{} val:{} idx:{} len:{} {:?}",
req.direction, req.request_type, req.recipient,
req.request, req.value, req.index, req.length,
self.state);*/
#[cfg(feature = "defmt")]
defmt::trace!(
"SETUP {:?} {:?} {:?} req:{} val:{} idx:{} len:{} {:?}",
req.direction,
req.request_type,
req.recipient,
req.request,
req.value,
req.index,
req.length,
self.state
);

if req.direction == UsbDirection::Out {
// OUT transfer
Expand All @@ -109,21 +117,20 @@ impl<B: UsbBus> ControlPipe<'_, B> {
self.i = 0;
self.len = req.length as usize;
self.state = ControlState::DataOut(req);
Some(req)
} else {
// No data stage

self.len = 0;
self.state = ControlState::CompleteOut;
return Some(req);
Some(req)
}
} else {
// IN transfer

self.state = ControlState::CompleteIn(req);
return Some(req);
Some(req)
}

None
Copy link
Member

Choose a reason for hiding this comment

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

Based on these changes, there's actually no code path that returns None, so there's not much point in using an Option, is there? Why not just return a Result<Request, Error> instead?

Was there originally intent for returning None from this function for valid inbound Requests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll dig through the previous commits to see what the author was intending. But yeah, I'd prefer to return a result too.

From what I've seen so far it's mostly been to mask errors. In general you don't want errors to block USB communication if possible (and just keep going). However, there are situations (like this one) where an error is basically fatal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another thing to add, this situation isn't very common. Most USB devices don't use the Control endpoint for data communication. In the USB HID spec (I believe) if an OUTPUT endpoint isn't defined for the interface the USB HID lock leds are transmitted through the Control endpoint.
In my experience with USB devices so far, this only applies to keyboards.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Going through the commits, I don't think this case has ever been handled correctly. This part of the code has been refactored a lot and I suspect has never been tested. After enough digging, even the original commit has the same issue.
Generally for USB you can ignore something (i.e. STALL) if you don't support it. However, you can't do this for parts of the spec that are required.

989c10d <- Refactor
503ecf7 <- More refactoring
963cc6b <- More refactoring
5db5a89 <- Initial commit

}

pub fn handle_out(&mut self) -> Option<Request> {
Expand Down