STMicroelectronics / STM32CubeU5

Full Firmware Package for the STM32U5 series: HAL+LL drivers, CMSIS, BSP, MW, plus a set of Projects (examples and demos) running on all boards provided by ST (Nucleo, Evaluation and Discovery Kits).
Other
124 stars 67 forks source link

Polling USB interrupt endpoints at 1 ms intervals doesn't work #40

Open wooyay opened 10 months ago

wooyay commented 10 months ago

Describe the set-up

Describe the bug (skip if none)

USB endpoints that declare a 1 ms polling interval are not correctly handled by the USBX STM32 host controller driver.

How to reproduce the bug (skip if none)

  1. Program the Ux_Host_HID example to the board and configure the board jumpers.
  2. Attach a Low Speed mouse to the host port and observe expected behaviour - interrupt endpoint is polled every 8 ms with data being returned when available.
  3. Attach a Full Speed mouse with an endpoint that has a 1 ms pollinig interval to the host port. The interrupt endpoint stops being polled after the first NAK.

Note that this is not limited to Ux_Host_HID or HID devices. It affects any USB endpoint that sets a 1 ms periodic interval.

Additional context

Investigation shows that when the endpoint is created the interval mask is set as 0 as expected to force the scheduler to queue the host channel every frame. In _ux_hcd_stm32_perioidic_schedule() the scheduler mode is set:

/* If it's scheduled each SOF/uSOF, the request should be submitted
* immediately after packet is done. This is performed in callback.  */
if (ed -> ux_stm32_ed_interval_mask == 0)
  ed -> ux_stm32_ed_sch_mode = 0;

However, HAL_HCD_HC_NotifyURBChange_Callback() does not appear to handle the case where urb_state == URB_NOTREADY and ed -> ux_stm32_ed_type == EP_TYPE_INTR (and possibly EP_TYPE_ISOC) so the transmit request is never resubmitted.

ALABSTM commented 10 months ago

Hi @wooyay,

The HAL_HCD_HC_NotifyURBChange_Callback() is supposed to be customized by users depending on the needs of their applications. In your case, are you referring to the __weak callback at the driver level or to a redefined one in one of the of the USBx applications provided within this firmware?

Regarding the handling of the case urb_state == URB_NOTREADY, it seems handled at the middleware library level, as shown below:

https://github.com/STMicroelectronics/STM32CubeU5/blob/c8fcb26ff629cf7c2a3b2c60e6121625eaa5ca2d/Middlewares/ST/usbx/common/usbx_stm32_host_controllers/ux_hcd_stm32_callback.c#L421-L435

Regarding endpoint types EP_TYPE_INTR and EP_TYPE_ISOC handling in case urb_state == URB_NOTREADY, they are handled at driver level in function HAL_HCD_HC_SubmitRequest() as you can see from this code snippet.

Once again, the HAL_HCD_HC_NotifyURBChange_Callback() is supposed to be customized by users depending on the needs of their applications, by adding required processing.

With regards,

wooyay commented 9 months ago

Hi @ALABSTM,

I realise HAL_HCD_HC_NotifyURBChange_Callback() is a function to be implemented, but in the case of the USBx applications this has already been done by the USBx STM32 host controller driver. As previously said, if you run the Ux_Host_HID example with no modifications and attach a mouse device with a 1 ms polling interval you will see the host poll the endpoint until the first NAK is returned from the device and then no further attempts to poll. This does not affect slower devices with polling intervals of 2 ms or more.

When interrupt endpoints are created the polling interval mask is calculated based on the interval given in the endpoint descriptor: https://github.com/STMicroelectronics/STM32CubeU5/blob/c8fcb26ff629cf7c2a3b2c60e6121625eaa5ca2d/Middlewares/ST/usbx/common/usbx_stm32_host_controllers/ux_hcd_stm32_endpoint_create.c#L199-L202 In the case of a typical Low Speed mouse with a endpoint polling interval of 10 ms ux_stm32_ed_interval_mask is calculated as 7, so the endpoint is polled every 8 frames. A Full Speed mouse with a endpoint polling interval of 1 ms sets ux_stm32_ed_interval_mask to 0 so it must be polled every frame.

Interrupt endpoints are typically submitted by the periodic scheduler which is triggered by the SOF interrupt. The periodic scheduler compares the current frame number against the interrupt mask to decide whether HAL_HCD_HC_SubmitRequest() should be called but only if ux_stm32_ed_sch_mode != 0. If the interval mask is 0, as set for 1 ms endpoints, ux_stm32_ed_sch_mode is never set: https://github.com/STMicroelectronics/STM32CubeU5/blob/c8fcb26ff629cf7c2a3b2c60e6121625eaa5ca2d/Middlewares/ST/usbx/common/usbx_stm32_host_controllers/ux_hcd_stm32_periodic_schedule.c#L201-L208 1 ms endpoints must be scheduled by the callback as stated in the comment.

If the transfer completes with a NAK, the callback function is called with urb_state == URB_NOTREADY. For all interrupt endpoints the callback does not reschedule the transfer by calling HAL_HCD_HC_SubmitRequest(). This is expected for endpoints with a polling interval of more than 1 ms as ux_stm32_ed_sch_mode == 1 and so the periodic scheduler will handle this on a later SOF interrupt. For 1 ms endpoints they are never handled as HAL_HCD_HC_NotifyURBChange_Callback() doesn't call HAL_HCD_HC_SubmitRequest()and the periodic scheduler will ignore them because ux_stm32_ed_sch_mode == 0.

In the code from the callback handling urb_state == URB_NOTREADY another case needs to be added to handle EP_TYPE_INTR where ux_stm32_ed_sch_mode == 0 to resubmit the transfer either by calling HAL_HCD_HC_SubmitRequest() again to by forcing the periodic scheduler to submit the transfer by setting ux_stm32_ed_sch_mode = 1.

ALABSTM commented 9 months ago

Hi @wooyay,

Many thanks for all these details. A report has been logged internally at the attention of our development teams. I will keep you updated.

With regards,

ALABSTM commented 9 months ago

ST Internal Reference: 173341