USBTMC triggers assertion soon after configuration, due to extra CLEAR_FEATURE(ENDPT_HALT)

[pigrew]

Operating System

Windows 10

Board

stm32f070rb

Firmware

master branch (ae8b8f0)
Driver is NI VISA for windows v21.5

What happened ?

Summary: Soon after device configuration, the host sends a CLEAR_FEATURE(ENDPOINT_HALT) on the USBTMC bulk-out endpoint, triggering an assertion that a transfer is being started on a busy endpoint (usbd_edpt_xfer:1243).

At configuration, the class driver initiates a bulk-in transfer, so the endpoint is correctly being marked as busy. Then, the host sends CLEAR_FEATURE(ENDPOINT_HALT), and the class driver tries to initiate a second transfer on the already busy EP.

I'm not sure what the device should be doing:

1. Should the device initiate a bulk-out transfer after configuration, or leave the EP in a STARLL state?
2. What should happen when CLEAR_FEATURE(ENDPOINT_HALT) when the EP is already listening?

I'm proposing the following, though additional input is appreciated:

1. Do start a bulk-outtransfer upon configuration. (as is currently implemented)
2. During a redundant clear-feature(halt), stall then unstall the endpoint, so the data toggle is reset to DATA0, followed by the start of a new transfer.

I'm going to continue trying to figure out what the correct behavior should be, and propose a patch.... Triggering an assert is bad.

How to reproduce ?

Flash with usbtmc example, and connect to host...

Debug Log as txt file

log_clearFeature.txt

Screenshots

No response

[pigrew]

Ok, USB 2.0 spec confirms that it's OK to send extra ClearFeature(ENDPOINT_HALT), and in that case, the data toggle should be reset, so that's what I'll do (implement by stalling and then unstalling endpoint):

USB 2.0 spec 9.4.5:

For endpoints using data toggle, regardless of whether an endpoint has the Halt feature set, a
ClearFeature(ENDPOINT_HALT) request always results in the data toggle being reinitialized to DATA0.

It also states that the DTOG's should be reset if set_configuration is sent a second time, but I'll worry about that later....

The Halt feature is reset to zero after either a SetConfiguration() or SetInterface() request even if the
requested configuration or interface is the same as the current configuration or interface.

[pigrew]

@hathach This is starting to look more like a device stack issue. Would you mind looking into the stack, and then I can fix the USBTMC code in particular?

When clear_feature(endpoint_halt) is requested on an endpoint, usbd_edpt_clear_stall is then called:

tinyusb/src/device/usbd.c

Lines 822 to 825 in 4639cac

	if ( TUSB_REQ_CLEAR_FEATURE == p_request->bRequest )
	{
	usbd_edpt_clear_stall(rhport, ep_addr);
	}else

But, endpt_clear_stall does nothing when the endpoint is not stalled:

tinyusb/src/device/usbd.c

Lines 1325 to 1341 in 4639cac

	void usbd_edpt_clear_stall(uint8_t rhport, uint8_t ep_addr)
	{
	rhport = _usbd_rhport;
	uint8_t const epnum = tu_edpt_number(ep_addr);
	uint8_t const dir = tu_edpt_dir(ep_addr);
	// only clear if currently stalled
	if ( _usbd_dev.ep_status[epnum][dir].stalled )
	{
	TU_LOG(USBD_DBG, " Clear Stall EP %02X\r\n", ep_addr);
	dcd_edpt_clear_stall(rhport, ep_addr);
	_usbd_dev.ep_status[epnum][dir].stalled = false;
	_usbd_dev.ep_status[epnum][dir].busy = false;
	}
	}

--

Based on the USB spec (previous comment), the DTOG should be reset in all cases. It may also make sense to abort any pending transfers (but I don't know how that fits into the current USBD API).

USB 3.2 seems to specify that the streams be reset (so aborting/discarding pending transfers is consistant):

Regardless of whether an endpoint has the Halt feature set, a ClearFeature(ENDPOINT_HALT) request always results in the data sequence being reinitialized to zero, and if Streams are enabled, the Stream State Machine shall be reinitialized to the Disabled state.

[hathach]

Driver is NI VISA for windows v21.5

Thanks @pigrew for the analysis, since I am not familiar with usbtmc, can you provide the link to download/install the driver. I will try to reproduce this to have a closer look/fix on the issue.

[pigrew]

https://www.ni.com/en-us/support/downloads/drivers/download.ni-visa.html is the driver I'm using (on windows 10).

Also, I'm working on some USBTMC changes including the high-speed patch, in my branch https://github.com/pigrew/tinyusb/tree/usbtmc_highspeed

[pigrew]

I discovered there was an intermittent failure also due to this same issue with the USBTMC interrupt-in endpoint, so I added that to my patch.

[pigrew]

One solution would be to modify usbd.c starting at:

tinyusb/src/device/usbd.c

Line 817 in 4639cac

case TUSB_REQ_CLEAR_FEATURE:

to be:

          case TUSB_REQ_CLEAR_FEATURE:
          case TUSB_REQ_SET_FEATURE:
          {
            if ( TUSB_REQ_FEATURE_EDPT_HALT == p_request->wValue )
            {
              // USB spec requires DTOG to be reset upon clear(EP_HALT),
              // and transfer buffers to be cleared.

              usbd_edpt_stall(rhport, ep_addr); // This is a NO-OP if the EP is already stalled
              usbd_edpt_buffers_clear(rhport, ep_addr); // Clear pending transfers in event queue
              if ( TUSB_REQ_CLEAR_FEATURE ==  p_request->bRequest )
              {
                 // Allow class to override clearing of halt
                if(driver->notify_pending_clear_ep_halt() == CLEAR_EP_HALT_ALLOWED) {
                  usbd_edpt_clear_stall(rhport, ep_addr);
                }
              }
            }

            if (driver)
            {
              // Some classes such as USBTMC needs to clear/re-init its buffer when receiving 
              // CLEAR_FEATURE request. We will also forward std request targeted endpoint to
             // class drivers as well

              // STD request must always be ACKed regardless of driver returned value
              // Also clear complete callback if driver set since it can also stall the request.
              (void) invoke_class_control(rhport, driver, p_request);
              usbd_control_set_complete_callback(NULL);

              // skip ZLP status if driver already did that
              if ( !_usbd_dev.ep_status[0][TUSB_DIR_IN].busy ) tud_control_status(rhport, p_request);
            }
          }
          break;

This may break existing classes, since they will need to re-issue any pending transfers on the endpoints. A class driver may want to choose to maintain an endpoint in a stalled state (if the class decides it is inappropriate for the halt to be cleared, as mentioned in the USBTMC spec....). However, I don't plan on immediately using this feature to leave an EP in the stalled state.