rp2: fix usb endpoint handling #8040

hoihu · 2021-11-27T20:36:19Z

dpgeorge

Now that there's a tud_cdc_rx_cb function, the tud_cdc_rx_wanted_cb can be removed and its functionality moved into tud_cdc_rx_cb (otherwise there will be two passes over all incoming data).

Also, the functionality here should be put in samd and mimxrt ports, because they will have the same issue. Would be best to put it all in shared/... and let the ports pull in the same code.

ports/rp2/mphalport.c

dpgeorge · 2021-12-15T01:45:24Z

ports/rp2/mphalport.c

+        if (ringbuf_free(&usb_ringbuf)) {
+            ringbuf_put(&usb_ringbuf, tud_cdc_read_char());
+        } else {
+            break;


If this returns with bytes still available in the CDC buffer, and there's no more USB activity, I think those bytes will never be processed (because there's no further callback here, and mp_hal_stdin_rx_chr will only check in the ringbuf).

I tried to check against this condition and while testing this with a bigger text buffer copy&pasted into the repl, I noticed that the board freezes (may happen after several chunks of say 600 bytes strings pasted into REPL). With freezing I mean that it no longer reacts on the REPL.

I then reverted back before I made the callback changes (current upstream master), and there it seems to work fine. Pasting many blocks of data in the REPL worked until a memoryError was thrown..

So I'm not convinced anymore if handling this callback is actually a good idea.. not until the source of the freeze is found.

The problem could be because there is now some processing for each char in this callback, and that means the USB stack misses some IRQs.

This could be related to the fact that on rp2 the tud_task() is called from MICROPY_VM_HOOK_POLL, whereas on other tinyusb ports like mimxrt and samd, it's called directly from the USB IRQ (calling from MICROPY_VM_HOOK_POLL introduces some latency).

Given that those other ports work well calling tud_task() from IRQ context, it would be good to see if rp2 can do the same.

hmm I just realized that tud_int_handler() does not seem to get called. At least I did not see any references to it. Or am I missing something here? Perhaps that would also explain some issues.

I can try to include the USB IRQ handling (which would then call tud_int_handler() and tud_task(), in line with the Mimxrt and Samd port).

I think the IRQ handler gets registered with the pico-sdk, see lib/tinyusb/src/portable/raspberrypi/rp2040/dcd_rp2040.c:

irq_set_exclusive_handler(USBCTRL_IRQ, dcd_rp2040_irq);

Further digging and thoughts...

hathach/tinyusb#52 suggests that tud_task() should not be called in IRQ context.

The comment here discourages to install a custom USB irq for the rp2040

CircuitPython does seems to deregister the original IRQ and install a separate handler though (https://github.com/adafruit/circuitpython/blob/main/ports/raspberrypi/supervisor/usb.c#L37). Likely due to reducing the latency introduced when a USB irq is fired until tud_task() is called: A usb background task is scheduled in the irq, seems to be a CircuitPython addition there.

I guess we could try to do similar things and improve latency. Perhaps re-installing the IRQ and adding a callback as done by CircuitPython could be beneficial. But how could we signal to call tud_task() from the IRQ? Maybe re-use something from the machinery of micropython.schedule()?

On a general note, it does bother me that this is going to change a lot of things. And from all I've read so far, the existing approach how MicroPython handles the USB stack should be fine. It may not be very good in terms of latency, but it should still satisfy the requirments given by the tinyUSB stack. It's still unclear to me why the endpoint behaves the way it does in #7996... Perhaps it would really be better to report this upstream and get an opinion from there. Maybe this is a real bug and should be solved in the stack, rather than trying to circumfence it in the upper layers.

If this returns with bytes still available in the CDC buffer, and there's no more USB activity, I think those bytes will never be processed (because there's no further callback here, and mp_hal_stdin_rx_chr will only check in the ringbuf).

this is addressed now. There is a separate check in mp_hal_stdio_poll

It looks like the pending_cdc_bytes=0 at the end of this function will overwrite the pending_cdc_bytes = bytes_avail line. Also it seems a waste to use 5 bytes for what is effectively a flag. And also this won't work with multiple CDC interfaces (the cdc_itf_nr will be clobbered by the next interface if all are full).

I suggest the following: have a uint8_t cdc_itf_pending set of bits which are 1 if the corresponding interface needs attention. Then in this function it'll be set like cdc_itf_pending |= 1 << itf, cleared like cdc_itf_pending &= ~(1 << itf) at the top of the function, and then in poll will be something like:

if (cdc_itf_pending) { for (uint8_t itf = 0; itf < MAX; ++itf) { if (cdc_itf_pending & (1 << itf)) { tud_cdc_rx_cb(itf); } } }

for (uint8_t itf = 0; itf < MAX; ++itf)

Hmm what would MAX be? In the stm32 port, there is MICROPY_HW_USB_CDC_NUM, should that be reused here?

hoihu · 2021-12-19T17:15:51Z

Maybe even mp_hal_set_interrupt_char() could be deleted too and interrupt_char.c included.

hoihu · 2021-12-22T22:42:40Z

@dpgeorge I pushed an update. I could not address all your review points unfortunately. I played around with subscribing the USB IRQ, but I couldn't get it to a reliable working state. The issue was always that when pasting larger amount of text in the REPL, it will freeze.

So I moved back to my original solution. I discovered that when not calling tud_task() in mp_hal_stdout_tx_strn() then pasting worked flawlessly. As soon as I used tud_task() it started to freeze after a few paste's.

The state I now pushed solves the original problem with CTRL+C not working in the REPL and seems to work ok for larger text pasting too. I did not experience any freezing.

But I did not syncronize it with the other ports. I also do not feel comfortable in doing so because I neither have a mimixrt nor a samd board at hand.

hoihu · 2022-01-08T20:27:47Z

I placed hathach/tinyusb#1273

dpgeorge · 2022-03-24T06:09:37Z

I see that there is no response yet from TinyUSB. So maybe we need to solve this our own way, with the ringbuf approach and always taking chars from the USB as soon as they come in, like is done here currently.

hoihu · 2022-03-25T21:50:01Z

So maybe we need to solve this our own way, with the ringbuf approach and always taking chars from the USB as soon as they come in, like is done here currently.

I tested this setup on my development board (raspi pico) pretty extensively. A collegue of mine did the same testing on a linux machine. We did not see any issues (rebased it recently to current master, works also).

But how to proceed for the other ports? I cannot do testing with samd or mimxrt boards.

Suggestion: Start with the change here in the rp2 port, then consolidate the changes later on the other ports.

robert-hh · 2022-03-26T08:06:57Z

@hoihu The test for the mimxrt port can be done by me. I tried to understand the changes that you made. I see one file being changed. And it requires the definition of MICROPY_HW_ENABLE_USBDEV. That's what it seems to be.

robert-hh · 2022-03-26T12:36:54Z

@hoihu I ported the change to mimxrt. Tested with mimxrt1011, mimxrt1015, mimxrt1021, mimxrt1052 and mimxrt1062. You'll find the changed file at https://github.com/robert-hh/micropython/blob/usb_endpoint/ports/mimxrt/mphalport.c So you may include that file into your PR, if you like. Otherwise I will add that to my next service PR.
Pointing the focus on that area, I noticed the need for other cleanup and straightening.

hoihu · 2022-03-26T22:04:55Z

Now that there's a tud_cdc_rx_cb function, the tud_cdc_rx_wanted_cb can be removed and its functionality moved into tud_cdc_rx_cb (otherwise there will be two passes over all incoming data).

done

hoihu · 2022-03-26T22:05:22Z

Maybe even mp_hal_set_interrupt_char() could be deleted too and interrupt_char.c included.

done

hoihu · 2022-03-26T22:17:09Z

@robert-hh :

The test for the mimxrt port can be done by me. I tried to understand the changes that you made. I see one file being changed. And it requires the definition of MICROPY_HW_ENABLE_USBDEV. That's what it seems to be.

yes excactly. It's now a bit more though, since I added the interrupt_char.c too. I also removed the separate "wanted" callback and check for the interrupt char in tud_cdc_rx_cb().

Sorry about those additional changes but I thought it's good to include them and it also addresses the reviews by damien.

So you may include that file into your PR, if you like.

I would prefer if you could add the changes to the mixmrt port (since I don't have a board.. perhaps I should buy one...)

robert-hh · 2022-03-27T07:08:37Z

I would prefer if you could add the changes to the mixmrt port (since I don't have a board.. perhaps I should buy one...)

Done. And it works fine. The fact, that Ctrl-C did not work anymore after a single character was indeed annoying.

dpgeorge · 2022-03-29T02:28:11Z

ports/rp2/mphalport.c

-    if ((poll_flags & MP_STREAM_POLL_RD) && ringbuf_peek(&stdin_ringbuf) != -1) {
-        ret |= MP_STREAM_POLL_RD;
+    #if MICROPY_HW_ENABLE_USBDEV
+    if (pending_cdc_bytes) {


it might make sense to check if ringbuf_free() before calling tud_cdc_rx_cb

dpgeorge · 2022-03-29T02:28:45Z

ports/rp2/mphalport.c

@@ -84,21 +89,10 @@ uintptr_t mp_hal_stdio_poll(uintptr_t poll_flags) {
 // Receive single character
 int mp_hal_stdin_rx_chr(void) {
    for (;;) {
-        #if MICROPY_HW_ENABLE_UART_REPL
        int c = ringbuf_get(&stdin_ringbuf);


will probably need to check cdc pending here and call tud_cdc_rx_cb, because the poll function may never be called

@hoihu I still think this comment needs attention. If cdc_itf_pending is set then this function may run forever and never check that flag.

@dpgeorge I pushed an update. Since mp_hal_stdio_poll() and mp_hal_stdin_rx_chr() share the same logic, I moved the common code to a separate function poll_cdc_interfaces().

dpgeorge · 2022-03-29T02:30:48Z

We will also need to test if the throughput of USB changes with this change.

robert-hh · 2022-03-29T06:59:46Z

Tested with the actual state of the PR on a Teensy 4.0, Test set-up:
Send 100000 blocks of 256 Bytes from the PC to the T40,m confirmed each by the T40.

Time actual master: 20.819 seconds
Time actual PR: 19.007 seconds

So the new code is slightly faster.

Send script:

import serial

buffer=256 * b'*'

ser = serial.Serial("/dev/ttyACM0")

for i in range(100000):
    ser.write(buffer)
    ser.read(1)
    # print(i)

ser.write(b"x"*256)
ser.close()

Receive script:

import sys
n = 0
amt = 0
try:
    while True:
        r=sys.stdin.buffer.read(256)
        sys.stdout.buffer.write("+")
        if r[0] == 120:
            break
        n += 1
        amt += len(r)
except:
    pass

Once the receive script has finished, one can get into REPL again and get the values of amt and n.

dpgeorge · 2022-03-29T07:16:35Z

Thanks for the test. IMO would be good to add that (or something like it) to mpremote, eg mpremote test-link.

robert-hh · 2022-03-29T08:53:25Z

Identical results on an rp2 Pico. About mpremote: Is is possible to start a script with mpremote and then release the /dev/ttyACM0, such that another PC-local script can use it?
Edit: Found the --no-follow option.

hoihu · 2022-03-30T20:34:45Z

It looks like the pending_cdc_bytes=0 at the end of this function will overwrite the pending_cdc_bytes = bytes_avail line. Also it seems a waste to use 5 bytes for what is effectively a flag.

Oh dear, my bad. I meant to return, not break. Sorry about that.

And also this won't work with multiple CDC interfaces (the cdc_itf_nr will be clobbered by the next interface if all are full).

I was thinking about this also, but then I thought that with only one ringbuffer, multiple CDC interfaces are not really supported? At least not concurrently, since the received characters are mixed in the ringbuffer?

Otherwise, we should think about using a separate ringbuffer per CDC interface IMO. I recall that the stm32 port does a similar thing, not sure though.

I'm happy to add the changes as suggested though.

dpgeorge · 2022-04-04T06:42:01Z

I was thinking about this also, but then I thought that with only one ringbuffer, multiple CDC interfaces are not really supported? At least not concurrently, since the received characters are mixed in the ringbuffer?

Yes the chars from multiple CDCs would be mixed. That's OK for the REPL (and is how it works when UART and CDC both feed the REPL), but not OK for when one CDC is the REPL and other CDCs are used for other things. For now it doesn't matter because there's no support to configure multiple CDCs. In the future different CDCs could feed into different ringbufs, but that would still require a cdc_itf_pending bitmask. So I think leave it as you currently have it (with that bitmask).

The CDC USB endpoint got NACK'd if a character was received but not consumed by the application (e.g. via sys.stdin.read()). See also issue micropython#7996. This approach uses a callback `tud_cdc_rx_cb` which is called by the tinyUSB stack on reception of new cdc data. By consuming the data immediately, the endpoint does not stall anymore. The previous handler `tud_cdc_rx_wanted_cb` was obsolete and removed. In addition some cleanup was done along the way: by adding `interrupt_char.c` and removing the existing code `mp_hal_set_interrupt_char()`. Also, there is now only one (stdin) ringbuffer.

hoihu · 2022-04-04T20:28:12Z

In the future different CDCs could feed into different ringbufs, but that would still require a cdc_itf_pending bitmask.

Supporting multiple CDC interfaces properly would a cool addition indeed!

dpgeorge · 2022-04-05T01:00:48Z

Merged in 5873390

I tested the patch and the USB CDC throughput is unchanged. I also tested copying a 600k file to/from a Pico using mpremote, and it worked correctly.

@hoihu