rp2040: endpoint NACK's after one character

hoihu commented 2 years ago

Operating System

Windows 11

Board

Original Raspberry Pico Board

Firmware

Micropython Build, current master (see e.g. https://micropython.org/download/rp2-pico/)

Host operating system is independanpt (can be windows based or linux / mac)

What happened ?

Pretty detailed here: https://github.com/micropython/micropython/issues/7996

but to summarize it, the USB endpoint does not ACK anymore when after it has received one character. (see also https://github.com/micropython/micropython/issues/7996#issuecomment-972663489 for a detailed view)

This has several consequences. The most annoying is that it does not break the REPL if CTRL+C is sent (on a terminal that sends it as \r\n). That how the bug surfaced. It is also present in other port that use TinyUSB, e.g. Mimixrt (see https://github.com/micropython/micropython/issues/7996#issuecomment-972780367).

I did some modifications and placed https://github.com/micropython/micropython/pull/8040 that implemented the tud_cdc_rx_cband immediately moved the data into a separate ringbuffer. That seemed to solve the issue. But I'm not convinced that this is the right solution. Shouldn't it be that the USB Endpoint only NACK's when the USB rx ringbuffer is full?

From a micropython point of view, it's also not really nice, since the endpoint might have pending data available when the stdin ringbuffer is full. This will not be collected anymore (could be collected via foreground polling). When testing this approach (collecting the remaining bytes) , I discovered that the REPL suddenly goes completly unresponsive if several chunks of say 500 characters are pasted (see https://github.com/micropython/micropython/pull/8040#discussion_r771977107).

At that point I was in doubt if we have overlooked something simple, that's why I'm placing this issue here upstream. It would be very appreciated if the maintainer could have a look at it.

PS: We are not calling tud_task() from an irq, but rely on periodic foreground calling (every milisecond via EVENT_POLL_HOOK). I know that CircuitPython does that differently and I saw that you were also involved there.

Most of my notes are written here: https://github.com/micropython/micropython/pull/8040#discussion_r776079816.

Thanks for feedback.

hoihu commented 2 years ago

any updates or comments?

hathach commented 2 years ago

sorry, I haven't actually got the time to look at this. The bug look easy to reproduce though I am not also too familiar with MP. I will try it out when having time. Meanwhile please try to bump up tinyusb, pico-sdk, mp to see if that helps. If not, don't forget to update your 1st post to latest of testing version.

hoihu commented 2 years ago

sorry, I haven't actually got the time to look at this. The bug look easy to reproduce though I am not also too familiar with MP. I will try it out when having time

No problem. I think the direction at the moment is to merge https://github.com/micropython/micropython/pull/8040 once the reviews are done.

Yet I'm still curious if there is anything that we have overlooked... so if you find some time to look into it would be greatly appreciated. Especially since it doesn't seem to be related the rp2 port only.

tinyusb,

I tried to bump tinyusb app. 4 months ago, no success. I also checked here if there were any related changes since and found not (but did not try out).

pico-sdk

I did not try that, since the mimxrt and samd ports showed a similar problem.

mp

I'm on current master on micropython

robert-hh commented 2 years ago

Implementing the change by @hoihu at the SAMD port revealed another (or the same) problem. If text is pasted to the device and echoed, the USB stack locks up. Data will not be sent any more from the device. Incoming data is for a while accepted. That can easy be replicated by loading the actual MicroPython firmware to a device and then with the REPL prompt either typing very fast or pasting some text, like a piece of code. When characters are not echoed (like in REPL RAW mode), everything is fine. Things get worse when the input character handling is slower, like with the PR which implements the port of @hoihu : https://github.com/micropython/micropython/pull/8520 Do you have any clue where to look at?

hoihu commented 2 years ago

@robert-hh did you try replacing wfi() with EVENT_POLL_HOOK in mp_hal_stdout_tx_strn?

robert-hh commented 2 years ago

No. I tried it now, and as expected, it makes no difference. MICROPY_EVENT_POLL_HOOK consists of a call to mp_handle_pending() and __WFI(). Since the scheduler is not enabled, mp_handle_pending() is practically a NOP.

And anyhow, when the device stops sending on UART, the MP VM still runs, and data received by USB are still processed, until eventually the UBS output buffer the TinyUSB stack is full, and no characters are accepted any more.

So it looks like the other problem here, in that the IRQ processing is halted.

hathach / tinyusb