Fix handling of control transfers where size is a multiple of bMaxPacket...

[nagasakihacky]

...Size0

End of transfer is indicated by a "short" packet where packet size is less than MaxPacketSize. If the transfer has a length that is a multiple of MaxPacketSize, a zero length packet must be sent to indicate the end of the transfer.

By calling the "normal transmission" section rather than the "end of transmission" section for the last packet of data, the state is not prematurely set to LAST_DATA_IN, so usb_control_send_chunk() gets called again and sends a ZLP

[mrnuke]

Do you have a test case for this, or is this conjecture?

[nagasakihacky]

I have a test case using a composite device with MSC and HID. The firmware was just sending NAKs after the first data packet of the configuration descriptor. I had both MSC and HID working fine independently, and also a composite with HID and CDC, but this was the only example that resulted in a 64 byte descriptor. From Jan Axelson's USB Complete:

"For some control read transfers, the amount of data returned by the device can
vary. If the amount is less than the requested number of bytes and is an even
multiple of the endpoint’s maximum packet size, the device should indicate
when it has no more data to send by returning a ZLP (USB 2.0) or a
zero-length Data Payload (SuperSpeed) in response to a request for data after
the device has sent all of its data."

[kuldeepdhaka]

cases:

1. device return the equal amount of data that the host is expecting.

   1.1. the length of data is multiple of bMaxEndpointSize0
   you want to send a ZLP (at the end)
   (afaif host right after getting the expected data, send a STATUS-OUT)

   1.2. the length of data is not multiple of bMaxEndpointSize0
   no need to send ZLP

2. device return less data than the host is expecting

   2.1. the length of data is returned multiple of bMaxEndpointSize0
   you want to send a ZLP (at the end)
   (afaif need to send ZLP to tell host that i[device] have no more data)

   2.2. the length of data is not multiple of bMaxEndpointSize0
   no need to send a ZLP

   2.3. wLength > 0, but device return 0 bytes, (similar to case 2.1)
   (afaif ZLP need to be send)
   (library is sending, https://github.com/libopencm3/libopencm3/blob/master/lib/usb/usb_control.c#L160)

3. wLength = 0, no data stage

disclaimer: just my analysis. i could be completly wrong!

[kuldeepdhaka]

current code:
1.1. since their is no ZLP sending anywhere, doing in general way. no problem
1.2. no problem
2.1. [https://github.com/libopencm3/libopencm3/blob/master/lib/usb/usb_control.c#L156] problem!
2.2. no problem
2.3 https://github.com/libopencm3/libopencm3/blob/master/lib/usb/usb_control.c#L160 no problem

you changes:
1.1. [https://github.com/nagasakihacky/libopencm3/blob/patch-1/lib/usb/usb_control.c#L76] problem! because your changes force to send a ZLP (ie stack is expecting to send a ZLP [LAST_DATA_IN], host send a STATUS-OUT boom!) [you broke this]
1.2. no problem
2.1. [https://github.com/nagasakihacky/libopencm3/blob/patch-1/lib/usb/usb_control.c#L156] since your change force to send a ZLP, you are solving this
2.2. no problem
2.3 https://github.com/libopencm3/libopencm3/blob/master/lib/usb/usb_control.c#L160 no problem

disclaimer: just my analysis. i could be completly wrong!

[karlp]

If someone wants to "fix" this, it should be pretty easy to provide test device firmware and some test host code. @nagasakihacky do you have that available at all?

[nagasakihacky]

After finding this and a few other quite fundamental bugs along the way I ended up reverting to ST's libraries. At some point I may see if I can switch back to this library, but I didn't have the luxury of time and so had to abandon

[karlp]

no problem, thanks for the report at least.

[kuldeepdhaka]

please try out https://github.com/kuldeepdhaka/libopencm3-examples/tree/usb-stack-tester
it contain a example to test the above analysis on usb stack

[kuldeepdhaka]

as per my finding, this commit will break the usb stack.

[karlp]

@kuldeepdhaka So, this commit as is is no good, but the issue it attempts to fix is valid, is that correct? I was hoping to look at your stack tester control transfer sizing tests, that's the problem here right?

[kuldeepdhaka]

@karlp yes, the issue is valid but commit has problems.

[karlp]

fixed via merge of 3ed12b6