search

rust-embedded-community / usb-device

Experimental device-side USB framework for microcontrollers in Rust.
MIT License
451 stars 76 forks source link

Fix missing SETUP DataOut packets #94

Closed haata closed 2 years ago

haata commented 2 years ago
mvirkkunen commented 2 years ago

handle_setup returns None for control OUT transfers because the entire transfer hasn't been received yet and it's waiting for data packets, the SETUP request is stored in self.state and will eventually be returned by handle_out. The built-in test class tests for control OUT transfers as well and they do generally work.

Do you have a testcase that fails without these changes and works with them?

haata commented 2 years ago

handle_setup returns None for control OUT transfers because the entire transfer hasn't been received yet and it's waiting for data packets, the SETUP request is stored in self.state and will eventually be returned by handle_out. The built-in test class tests for control OUT transfers as well and they do generally work.

Do you have a testcase that fails without these changes and works with them?

The problem I have here is during enumeration on Linux (I'm testing with a real device).

The process goes as such (I can provide hardware traces later if that helps)

  1. USB device is enumerated
  2. USB interface is enumerated (a HID keyboard is found)
  3. The Linux HID stack sends SET_REPORT over the control interface for the enumerated interface
  4. handle_setup is called with a 1 byte buffer that indicates the current state of the lock LEDs (e.g. NumLock + CapsLock + ScrollLock)
    • This is the important part. The USB stacks I've worked with so far will optimize this to auto-reply for zero length control data packets.
    • Since this is a 1 byte buffer, we need to give an actual response. With the current API the only way to check again is to check poll, but this won't work in my case as poll is only triggered on USB interrupts (and there are no pending interrupts as we've already read the ControlOut packet and the host is waiting for the reply).
    • https://github.com/twitchyliquid64/usbd-hid/blob/master/src/hid_class.rs#L651 is where the ControlOut packet is handled in my case.

Basically, if there is a real error in response to the ControlOut data packet (e.g. buffer tool small) we should return an error and not hide it and assume all packets are good. We're also required to respond to Setup packets within a certain amount of time (https://www.beyondlogic.org/usbnutshell/usb6.shtml#SetupPacket) within 500 ms (the host won't send anything while it's waiting).

Looking more carefully at the Option usage. It's ok, but there isn't a good way to indicate buffer overflows (and other internal errors). You just have to know that's why transmissions stop working. I spent a few hours tracing code to figure this (https://github.com/rust-embedded-community/usb-device/pull/95) was my issue (tracing live code isn't possible without defmt due to the timing requirements and how slow RTT is for normal messages). But yeah, that's mostly an annoyance.

stappersg commented 2 years ago

Do you have a testcase that fails without these changes and works with them?

The problem I have here is during enumeration on Linux (I'm testing with a real device).

The process goes as such (I can provide hardware traces later if that helps)

....

Please provide a testcase that fails without these changes and works with them.

mvirkkunen commented 2 years ago

What does "handle_setup is called with a 1 byte buffer" mean exactly? The SETUP data packet is always 8 bytes long. Possible data will come later, and will be handled by handle_out. Do you have a trace log where handle_setup is reading a packet with a length that's not 8? And the status stage of the control transfer (assuming that what you mean by "we need to give an actual response") is the same whether there is a data stage or not.

Which driver are you using? For what it's worth, this could be due to packets being read in the wrong order because the way it works currently was designed badly (by me).

haata commented 2 years ago

I'm sorry for the noise, I think something else must be going on. It was 100% reproducible yesterday. I have been working on remote wakeup and dealing with the control buffer size (so something else may have shuffled around).

Will close this for now and come back with a lot more info and traces if I can repro it.