Zlp request2

[pigrew]

When deciding if a ZLP needs to be sent, the requested length in the status packet has to be compared to the length being transmitted.
Also, I think there's a possible buffer overrun here in handling HID out control transfers, so I added a TU_VERIFY to make sure the buffer is long enough. I wonder if there are other places where this happens?

Tested on the stm32f070rb.

[cr1901]

I'm not sure the best way to handle this, but I've determined that this patch breaks if data sent down the Control Pipe during the Data Phase is an exact multiple of the packet size.

The culprit is the event handler. Right now, there's no way to tell the difference between a ZLP sent as part of a Data Phase and a ZLP sent as part of a Status Phase, so the event gets eaten and tinyusb never schedules an OUT xfer to receive the ZLP as part of the Status Phase.

@hathach Perhaps it's time to remove that TODO :P?

[cr1901]

Right now, there's no way to tell the difference between a ZLP sent as part of a Data Phase and a ZLP sent as part of a Status Phase, so the event gets eaten and tinyusb never schedules an OUT xfer to receive the ZLP as part of the Status Phase.

Mulling over possible solutions to this problem.

Questions

For @hathach:

1. If a device sends needs to send a ZLP at the end of an xfer (e.g. a BULK or INTERRUPT IN xfer), does the device class code handle this transparently for you? I assume in the case of the vendor device class, you must schedule a ZLP xfer yourself if necessary, as @pigrew has added to porting.md.

   This would happen if e.g. the host requested, say, 128 bytes, you have a packet size of 64, and you only can send 64 bytes. You would send a ZLP to tell the host the xfer is done.

2. Why is DCD_EVENT_XFER_COMPLETE suppressed during a Status Phase xfer?

For @pigrew:

Tested on the stm32f070rb.

If you hooked up your stm32f070rb to a packet analyzer, did you see ZLPs being xferred over the control pipe that were not xferred before your patch?

Easy Way

1. Add a bool to usbd_control_xfer_t that indicates we are in the status phase for the current EP0 xfer.

2. Remove the static qualifier from _control_state.

3. Allow the control pipe check access to _control_state in usbd.c and use that bool to determine whether to suppress DCD_EVENT_XFER_COMPLETE from the event queue or not.

I don't care for sharing globals between modules, like this. But this is the simplest, least invasive way to fix the problem.

Hard Way

1. Add a new event to dcd_eventid_t called something like DCD_EVENT_NEEDED_ZLP. This is only sent by the control pipe, when the control pipe determines it needed to send a ZLP.

2. In the dcd_event_handler switch statement, this case is handled identical to DCD_EVENT_XFER_COMPLETE, except without the control pipe check. That way, a Data Phase ZLP is distinguished from a Status Phase ZLP.

3. In the tud_task switch statement, DCD_EVENT_NEEDED_ZLP and DCD_EVENT_XFER_COMPLETE share the same branch, because the only difference between a Data Phase ZLP and Status Phase ZLP is that

4. Change all the drivers under src/portable to send a DCD_EVENT_NEEDED_ZLP if determines an xfer is about to end on a packet boundary. Yea, this is looking like a not-so-good solution, however...

5. Creating a new event paves the way to making sending ZLPs transparent to all device classes and applications, not just the control pipe.

I would really like some feedback on these possible solutions and how to make them better.

[pigrew]

If you hooked up your stm32f070rb to a packet analyzer, did you see ZLPs being xferred over the control pipe that were not xferred before your patch?

I wrote the patch before I had any sort of USB analyzer, so I don't know. Now, I have a DreamSourceLab DSLogic, which makes it merely inconvenient to answer your question (and I have not traced it...).

However, I can say that USBTMC, with the interrupt EP disabled (leaving a 32-byte config descriptor (cfg + interface + EP IN+ EP OUT)), and 8 byte EP0 packets, does not work in the master branch. (there's a small typo where I forgot to rename some macros in the USBTMC descriptors which makes it less simple to run this test, I'll post a PR for that in the next few days. It would be nice to have Travis CI try building a few different variations of USBMTC, though perhaps only on a single platform so that there's less building required...).

When I use the same code with this ZLP patch, it does work.

I would appreciate if this PR is merged, as it fixes the CONTROL IN case (AFAIK). If I'm reading it correctly, the "TODO" if statement only applies to EP 0x00 and not 0x80, so only CONTROL OUT.

I would really like some feedback on these possible solutions and how to make them better.

The class module has extra information which is required to decide if a ZLP is needed. In the case of control EP, if a short packet is sent depends on the value of wLength, which as of now, the DCD does not itself know. It's an library architecture question about how much of the EP0 control/parsing the DCD should do. Up to now, I've been leaning towards minimizing the DCD, and putting everything centrally in the library.

Control are parsed/handled by src/device/usbd_control.c, so that feels like the appropriate place to decide if a ZLP is needed, and also handle status bytes.

I don't have time to debug this ZLP patch at the moment, but my quick thinking is similar to your "simple" case, but I would make all of the logic go into usbd_control.c. Route all xfer packets on EP0 to usbd_control (get rid of the TODO if(...) break;), and have usbd_controldecide what to do with them. control transfers with no data phase need to also be handled. We might need to add another function to usbd_control to inform it that a SETUP packet has been received. The only downside to this is that things will be slower since less of the processing will be done in the interrupt context.

I think of fixing CONTROL IN and CONTROL OUT as separate (but related) issues. Fixing CONTROL OUT can be a separate patch?

[cr1901]

When I use the same code with this ZLP patch, it does work.

Unless you have local patches, I'm not sure how your code can work, even with this ZLP patch as-is. Right now, even with this patch, tinyusb will eat/discard XFER_COMPLETE events for ZLPs sent down the control pipe, and I'm unaware of how this logic can be bypassed unless you have local patches you haven't committed yet.

[pigrew]

This patch only works for control data IN. My reading is that only ZLP sent OUT are ignored, and ZLP IN are handled properly with this patch.

I have some uncommitted code in usbtmc, but it's unrelated to the logic here.

[pigrew]

New thought on test code:

We don't have a good example for checking if ZLP and such is correct. We could create a new "vendor class" for debugging/verifying the core. Do we want to do this? I believe that libcm0

I propose that we add some debug code to help us with this. I could hack some debug tests into the USBTMC class, or we can create a new testing class.

libopencm3 has a debug class for testing, though its license is not compatible with this project, so we can't use their code directly. Perhaps we can implement the same interface, though.

USBTMC's VISA driver is pretty flexible and allows the host application to send/receive arbitrary control requests, however it doesn't give full control of the device to the host application (like libusb would). The specification also allows for vendor-defined messages. Perhaps adding this, but disabled by default, would be a good interim solution.

I'm going to add a simple interface to USBTMC to allow it to respond to control transfers with specified data lengths, to make debugging easier. However, I'll leave the extension disabled by default.

[cr1901]

This patch only works for control data IN.

Oh, that's what you mean by:

I think of fixing CONTROL IN and CONTROL OUT as separate (but related) issues. Fixing CONTROL OUT can be a separate patch?

Anyways, can you and @hathach works out the details here for getting this merged as well as a CONTROL OUT fix? My msp430 patch is blocked on fixing CONTROL OUT, and as much as I want to work on this, I have to move on to paid work for the time being.

[pigrew]

@cr1901 I apologize for the confusion. When first writing the patch, I hadn't considered CONTROL OUT requests.

Do you have a suggested method for testing CONTROL OUT (on Windows 10)? USBTMC's driver seems to have very buggy CONTROL OUT support.

@hathach I'd appreciate if you could review this patch, and then we can worry about CONTROL OUT as a separate issue? What are your thoughts?

[cr1901]

@pigrew Sorry for the delay, I had to move on to other things for now (I was gone 9-13, and sick this week).

Do you have a suggested method for testing CONTROL OUT (on Windows 10)? USBTMC's driver seems to have very buggy CONTROL OUT support.

I'm afraid I don't, unless libusb's WinUSB wrapper gives you easy access to the CONTROL OUT (it should AIUI).

@hathach I'd appreciate if you could review this patch, and then we can worry about CONTROL OUT as a separate issue? What are your thoughts?

I second this, is there any possibility this patch could be reviewed/submitted soon to keep momentum going? This patch is already merged locally in #194; I will need a corresponding fix for CONTROL OUT as well however, which could be a separate PR.

[hathach]

@pigrew @cr1901 Sorry, I am late to the party, somehow this falls off my radar. I see we need to send ZLP as you both pointed out when

• the responded length is less than requested from host
• the responded length is multiple of EP0 packet size

Yes, currently usbd discard the ZLP transfer with EP0, we will probably need to address that in order for this patch to work. It is hard to do it by simply reviewing and asking, @pigrew already did a great work. I will check out this pr to its own branch and do some dirty work. Will also find a way to test it out as well. Thank you very much :)

[pigrew]

@hathach Thanks for looking at this. I've made this thread way too long already, but here is a brief summary for you:

• This patch only addresses CONTROL IN transfers
• I have tested this patch using USBTMC, and it properly handles CONTROL IN of arbitrary lengths.
• CONTROL IN can be tested by configuring 8 byte packets and creating a descriptor with a multiple of 8 bytes (either string or USBTMC INTF are convenient).
• I have a private version of the USBTMC test script can loop transfers lengths to ensure that everything works. I added a non-standard control transfer to USBTMC to the class, which generates repeating pattern which is verified by the Python test script. I don't know if we want to add this to the master branch or not. I can add this patch to this PR if requested, but it shouldn't be enabled by default since it requires a large statically allocated buffer (130 bytes) for storing the control transfer data.
• I propose that CONTROL OUT be fixed with a separate PR. Is there a USB class which can accept arbitrary length CONTROL OUT transactions or has a defined control transfer with a length which is a multiple of 8 bytes? USBTMC should be able to, but can't due to a Windows driver bug. If not, we may need to create a test class which used from libusb (from Python?) to perform many unit tests. This is the strategy that libopencm3 uses.

[hathach]

@hathach Thanks for looking at this. I've made this thread way too long already, but here is a brief summary for you:

• This patch only addresses CONTROL IN transfers
• I have tested this patch using USBTMC, and it properly handles CONTROL IN of arbitrary lengths.
• CONTROL IN can be tested by configuring 8 byte packets and creating a descriptor with a multiple of 8 bytes (either string or USBTMC INTF are convenient).
• I have a private version of the USBTMC test script can loop transfers lengths to ensure that everything works. I added a non-standard control transfer to USBTMC to the class, which generates repeating pattern which is verified by the Python test script. I don't know if we want to add this to the master branch or not. I can add this patch to this PR if requested, but it shouldn't be enabled by default since it requires a large statically allocated buffer (130 bytes) for storing the control transfer data.
• I propose that CONTROL OUT be fixed with a separate PR. Is there a USB class which can accept arbitrary length CONTROL OUT transactions or has a defined control transfer with a length which is a multiple of 8 bytes? USBTMC should be able to, but can't due to a Windows driver bug. If not, we may need to create a test class which used from libusb (from Python?) to perform many unit tests. This is the strategy that libopencm3 uses.

Thanks @pigrew for lots of useful tips and suggestions, I have added couple of unit tests with Ceedling/CMock/Unity #206. This will help to simulate/mock the situation we are to tests. I have been wanting to add more tests with CMock/Unity, this is a good chance to do so :).

PS: I am still in the middle of other works, but I will try to fix this. It is not as fast as we would like :)

[hathach]

@pigrew You need to add uart.c file to the board.mk as well, or maybe you forgot to check that in.

[pigrew]

@hathach
I've merged your PR to this PR. I'm still looking at the cause of the stm32f0 issue (so probably don't merge this PR yet)...

It looks like it's related to the SET CONFIGURATION request (which doesn't have a data stage).

[hathach]

@pigrew I think fsdev somehow miss a setup packet, it doesn't report the setup packet that it doesn't response to. Post on the other PR, but I merge here as well for reference.

sometime miss a setup packet, then completely non-response to that packet as following screenshot nd log. As shown in the log, the setup packet is as follows string index 0, len = 255, then string index 2 len =2. It completely skip the string index = 2, len = 255

I have no idea how removing above line could cause this issue, it is possibly something related to the fsdev timing thing ? Not sure though hmm. I was thinking may be event queue is overflow and increased CFG_TUD_TASK_QUEUE_SZ but it doesn't help

You can get the similar log with new LOG=, LOG value will set the CFG_TUSB_DEBUG option.

make BOARD=stm32f072disco LOG=1 clean all flash

000:  80 06 00 02 00 00 62 00                         | ......b.
  EP Addr = 0x80, len = 64                                                        
  EP Addr = 0x80, len = 34                                                        
  EP Addr = 0x00, len = 0                                                         

000:  80 06 00 03 00 00 FF 00                         | ........
  EP Addr = 0x80, len = 4                                                         
  EP Addr = 0x00, len = 0                                                         
000:  80 06 02 03 09 04 02 00                         | ........
  EP Addr = 0x80, len = 2                                                         
  EP Addr = 0x00, len = 0                                                         
000:  80 06 02 03 09 04 1E 00                         | ........
  EP Addr = 0x80, len = 30                                                        
  EP Addr = 0x00, len = 0                                                         

000:  80 06 01 03 09 04 FF 00                         | ........
  EP Addr = 0x80, len = 16                                                        
  EP Addr = 0x00, len = 0                                                         

[pigrew]

@hathach , Yep, I saw your message, and am still confused about the cause. On stm32, the reference manual says that it will always ACK a setup packet, even if it were to discard it (due to its buffer being full, etc)... It also stated that the USB spec only allows the device to ACK it. not sure if that's what's happening.

I traced the logic a bit, and the difference with vs without the if statement is that without it, OUT control transfers (SET CONFIGURATION) are handled differently (they now get a status packet sent within control_xfer_cb...). I've lost grasp of how the code is built, so the reasons are not obvious to me...

A smaller patch to get stm32 working is to add an if statement to usbd_control_xfer_cb:

bool usbd_control_xfer_cb (uint8_t rhport, uint8_t ep_addr, xfer_result_t result, uint32_t xferred_bytes)
{
  (void) result;

  // Endpoint Address is opposite to direction bit, this is Status Stage complete event
  if ( tu_edpt_dir(ep_addr) != _ctrl_xfer.request.bmRequestType_bit.direction )
  {
    TU_ASSERT(0 == xferred_bytes);
    return true;
  }
  if(xferred_bytes == 0) { // <----------------This is added to get back to the old "working" behaviour
    return true;
  }

[hathach]

@pigrew Yeah, device does ACK the setup packet, as shown in the trace screenshot, but it didn't call dcd_setup_received(), I guess it is discarded somehow.

[pigrew]

One step closer.... It's calling tud_control_status() twice... When handling TUSB_REQ_SET_CONFIGURATION, it "manually" calls tud_control_status (usbd.c:513), and then later in response to the xfer complete packet. Maybe the fsdev driver is mixing up directions:

USBD: event SETUP_RECEIVED
  000:  00 09 01 00 00 00 00 00                         | ........
  Set Configuration
  USBTMC open
Send CTRL Status //<------- sent in TUSB_REQ_SET_CONFIGURATION
USBD: event XFER_COMPLETE
  Endpoint: 0x80, Bytes: 0
  EP Addr = 0x80, len = 0
xxx
Send CTRL Status //<-------- sent  in usbd_control_xfer_cb
USBD: event SETUP_RECEIVED
  000:  80 06 00 01 00 00 12 00                         | ........
  Get Descriptor
USBD: event XFER_COMPLETE
  Endpoint: 0x80, Bytes: 18
  EP Addr = 0x80, len = 18
Send CTRL Status
USBD: event XFER_COMPLETE
  Endpoint: 0x00, Bytes: 0
  EP Addr = 0x00, len = 0
USBD: event SETUP_RECEIVED
  000:  80 06 00 02 00 00 13 00                         | ........
  Get Descriptor

[pigrew]

@hathach, I'm heading to sleep for the night, and shouldn't spend much time on this during the next few days.... Perhaps you can craft a good fix?

My analysis:

1. _ctrl_xfer.buffer is only set in tud_control_xfer()
2. With no data phase, tud_control_xfer() is never set
3. When the usbd_control_xfer_cb is called, the buffer still points to the buffer of the previous event item, so the preceding IN transaction.
4. usbd_control_xfer_cb() responding to the status of SET_CONFIGURATION thinks it was a data packet since it's comparing to the buffer of the previous control transfer, not the active one.

Somewhere we need to tell usb_control that there is a new control transfer, in the event that there is no data phase.

[hathach]

@hathach, I'm heading to sleep for the night, and shouldn't spend much time on this during the next few days.... Perhaps you can craft a good fix?

My analysis:

1. _ctrl_xfer.buffer is only set in tud_control_xfer()
2. With no data phase, tud_control_xfer() is never set
3. When the usbd_control_xfer_cb is called, the buffer still points to the buffer of the previous event item, so the preceding IN transaction.
4. usbd_control_xfer_cb() responding to the status of SET_CONFIGURATION thinks it was a data packet since it's comparing to the buffer of the previous control transfer, not the active one.

Somewhere we need to tell usb_control that there is a new control transfer, in the event that there is no data phase.

Yeah, your analysis is very helpful. Please take the bed, I will review the whole usbd control again. Thank you very much for your work again :)

[hathach]

this should fix your mentioned issue with status (no data) didn't update _ctrl_xfer request and buffer pigrew#3

It certainly make the stack more stable but it still doesn't help with f0. I still think the dcd discard the setup packet somehow (but still ACK it to the host).

[pigrew]

@hathach The patch helps my F070.... I'm able to enumerate my device now, and ZLP for control IN transfers which are multiples of the packet size are working. So, I'm happy with the patch now, and unable to reproduce the issues you found...

[pigrew]

Or maybe it doesn't work well.... I'm now running into issues with recovering from a stall condition (I sent a CONTROL IN request which wasn't defined).
Oh.... stupid me. It's just getting stuck on the breakpoint at a TU_ASSERT.... I'm going to change it to a TU_VERIFY so that it doesn't automatically trigger a breakpoint.

The code now passes my USBTMC test script on STM32F070.

[hathach]

@pigrew I still see the discard setup packet as above, but it is recoverable. Though it is kind of random. If you notice a couple seconds of delay in enumeration, that is host has to issue a bus reset and re-enumerate due to lack of response. I don't remember if this happens before but it is easy to reproduce on my Linux machine.

However the purpose of this PR (ZLP) has been fully complete, we can merge this now, great works :D. Thank you very much. I will file an separated issue for missing setup packet on F0, we will look at it later on when having time.

[hathach]

@pigrew I am ready to merge this long-waited PR, let's me know if you want to do anything else :)

[pigrew]

@pigrew I still see the discard setup packet as above, but it is recoverable. Though it is kind of random. If you notice a couple seconds of delay in enumeration, that is host has to issue a bus reset and re-enumerate due to lack of response. I don't remember if this happens before but it is easy to reproduce on my Linux machine.

However the purpose of this PR (ZLP) has been fully complete, we can merge this now, great works :D. Thank you very much. I will file an separated issue for missing setup packet on F0, we will look at it later on when having time.

Great. I just changed the other TU_ASSERT, and hopefully we can merge this PR and I'll look at the other patches I have queued up.

I have a likely-controversial patch which speeds up enumeration on Windows when EP0 has a max packet size of 8. Perhaps this is a separate issue, but Windows will always perform a bus-reset during enumeration. Windows always waits a few seconds for a timeout..... My solution is for that is to pre-transmit the status packet along with the first data packet when in an unconfigurated state. I have sometimes noticed the slow enumeration, I'll see if I can debug that too (as it's probably a separate issue)....

[pigrew]

@pigrew I am ready to merge this long-waited PR, let's me know if you want to do anything else :)

Ship it. It's ready for merging, AFAIK.

[hathach]

@pigrew I still see the discard setup packet as above, but it is recoverable. Though it is kind of random. If you notice a couple seconds of delay in enumeration, that is host has to issue a bus reset and re-enumerate due to lack of response. I don't remember if this happens before but it is easy to reproduce on my Linux machine.
However the purpose of this PR (ZLP) has been fully complete, we can merge this now, great works :D. Thank you very much. I will file an separated issue for missing setup packet on F0, we will look at it later on when having time.

Great. I just changed the other TU_ASSERT, and hopefully we can merge this PR and I'll look at the other patches I have queued up.

I have a likely-controversial patch which speeds up enumeration on Windows when EP0 has a max packet size of 8. Perhaps this is a separate issue, but Windows will always perform a bus-reset during enumeration. Windows always waits a few seconds for a timeout..... My solution is for that is to pre-transmit the status packet along with the first data packet when in an unconfigurated state. I have sometimes noticed the slow enumeration, I'll see if I can debug that too (as it's probably a separate issue)....

windows behavior is standard, I will try to investigate on how dcd discard the setup packet from dcd, by trying to add some TU_ASSERT() to pause CPU for error condition. But it is workable now, so we can live with it :)

[pigrew]

windows behavior is standard, I will try to investigate on how dcd discard the setup packet from dcd, by trying to add some TU_ASSERT() to pause CPU for error condition. But it is workable now, so we can live with it :)

I completely agree. It's working well enough for me. Thanks for looking at it, I'll look also in the next few days if you don't solve it before then.

Be sure to read the STM32 reference manual where it says:

Since the USB specification states that a SETUP packet cannot be answered with a
handshake different from ACK, eventually aborting a previously issued command to start
the new one, the USB logic doesn’t allow a control endpoint to answer with a NAK or STALL
packet to a SETUP token received from the host.

When the STAT_RX bits are set to ‘01 (STALL) or ‘10 (NAK) and a SETUP token is
received, the USB accepts the data, performing the required data transfers and sends back
an ACK handshake. If that endpoint has a previously issued CTR_RX request not yet
acknowledged by the application (i.e. CTR_RX bit is still set from a previously completed
reception), the USB discards the SETUP transaction and does not answer with any
handshake packet regardless of its state, simulating a reception error and forcing the host to
send the SETUP token again. This is done to avoid losing the notification of a SETUP
transaction addressed to the same endpoint immediately following the transaction, which
triggered the CTR_RX interrupt

[hathach]

thanks for the tip, I probably won't be able to do much with this port this week. This is not urgent, so no hurry :)