drivers: usb: udc_stm32: Handle HAL callbacks on separate thread #80820
Conversation
|
A huge thanks. We'll for sure help testing on extended sample of series. |
|
Is it safe to enqueue a stack-allocated msg pointer? Perhaps the event approach is safer. Given that the combined number of IN and OUT endpoints is typically less than 32 (15 IN + 15 OUT), a Additionally, this approach helps prevent potential message queue overflows that might result in missed event/message handling and supports ordered processing. |
Yes, it is copied into the ringbuffer.
Isnt it technically up to 16+16 endpoints including the control endpoint?
While I agree about the message queue overflows, I don't see how k_event would support ordered processing? Thats mostly why I chose the msgq based approach, to make sure the events were processed in the order they arrived |
|
Not trying to block, just discussing out of curiosity.
Apologies, I just noticed the message is passed not a pointer; all good.
That fits.
Not in a fifo way, but In the processing. I.e. process |
Ah, I dont think this is what we would want though. All of these callbacks are triggered from the same IRQ, i.e. have the same priority, and should be processed in the order they arrived. It should maybe be considered how the ordering interdependency with the @erwango I believe it is ready for testing. Is there anything else I can do to help the testing effort along? |
|
@topisani It's failing on After: |
|
@erwango Alright, i believe this is my attempt at detecting USB HS support that fails. I have an l432 board i can test on, if that is the case, it should fail there as well |
|
I could reproduce the issue on a nucleo_l432kc, and it appears to have been the HS detection. It now follows the maximum-speed dts prop, like in most of the other udc drivers, which fixed the issue for me. |
|
Alright, i did some testing using
I have also done a test on an arduino_portenta_h7 based custom board, that also failed. The common factor is that both boards use HS USB. Checking with |
topisani
force-pushed
the
udc-stm32
branch
2 times, most recently
from
2275c22 to
2a056c0
Compare
zephyrbot
requested review from
erwango,
FRASTM,
gautierg-st,
GeorgeCGV,
jfischer-no,
marwaiehm-st,
mathieuchopstm and
tmon-nordic
|
Hi @topisani I've started to test on my side, here my results using
I will continue tests and report here. |
|
Addressed most review comments from @jfischer-no. For the remaining ones, I still need some clarification on how to proceed. |
|
@jfischer-no PTAL |
|
@topisani Please rebase. |
topisani
dismissed stale reviews from henrikbrixandersen and erwango
via
e980fee
topisani
force-pushed
the
udc-stm32
branch
2 times, most recently
from
e980fee to
974c2bb
Compare
All HAL callback handling is offloaded to a separate thread, as they involve non isr-compatible operations such as mutexes. Fixes zephyrproject-rtos#61464 Signed-off-by: Tobias Pisani <mail@topisani.dev>
This fixes usbd_caps_speed(), which is used in sample_usbd_init, and the documentation. Without it, no high speed configuration would be added. Signed-off-by: Tobias Pisani <mail@topisani.dev>
On USB HS, the previous lower limit of 64 is too small, the value 160 has been chosen apparently through trial and error. See 1204aa2 for the original implementation in the old device driver. Signed-off-by: Tobias Pisani <mail@topisani.dev>
|
A big thank you for this PR! |
All HAL callback handling is offloaded to a separate thread.
Tested on STM32H747 (arduino portenta) with the high speed controller.
This is an attempt at fixing #61464.
Marked as a draft, as i would at least like to do some minor stylistic cleanup before submitting, but if anyone could help test, I would appreciate it. As far as i can tell, no additional locking should be required, and no timing invariants are broken, but I could use some extra eyes on that as well.
Fixes: #61464 (UDC STM32 part)