Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

samd: USB interface locks up when input symbols arrive too fast. #8521

Closed
robert-hh opened this issue Apr 11, 2022 · 11 comments
Closed

samd: USB interface locks up when input symbols arrive too fast. #8521

robert-hh opened this issue Apr 11, 2022 · 11 comments

Comments

@robert-hh
Copy link
Contributor

robert-hh commented Apr 11, 2022

There is a USB interface issue, that the USB interface locks up if input symbols arrive too fast AND are echoed back. In the master branch, this is hard to replicate. The Issue got evident with #8520, where the issues can re reproduced easily.
When the USB interface seems to be locked up, the board continues to work. Analysis in the Debugger shows, that input symbols still arrive and are processed. Output to commands will be created and the function mp_hal_stdout_tx_strn() is called, which in turn calls tud_cdc_write() and tud_cdc_write_flush(). Only the data does not arrive at the connected PC/USB interface.

@robert-hh
Copy link
Contributor Author

robert-hh commented Apr 19, 2022

While the USB interface seems to be locked, REPL output still goes to UART for a while, but locks up later as well, probably because the USB output buffer is filled up.
When REPL input from UART is used, all works well. No lock-up. But maybe only because the symbol input rate is limited by the baud rate.

@robert-hh robert-hh changed the title samd: USB interface locks up when input sybols arrive too fast. samd: USB interface locks up when input symbols arrive too fast. Apr 21, 2022
@robert-hh
Copy link
Contributor Author

robert-hh commented Jun 27, 2022

It looks like I found the culprit in the tinyusb lib. Making the change below the error is gone. It's the right spot for causing the issue. Now I have to make a PR and convince @hathach to change it, and then make the matching PR for the MicroPython repository.

diff --git a/src/portable/microchip/samd/dcd_samd.c b/src/portable/microchip/samd/dcd_samd.c
index 526a6fb63..68aa5ab75 100644
--- a/src/portable/microchip/samd/dcd_samd.c
+++ b/src/portable/microchip/samd/dcd_samd.c
@@ -271,14 +271,14 @@ bool dcd_edpt_xfer (uint8_t rhport, uint8_t ep_addr, uint8_t * buffer, uint16_t
   {
     bank->PCKSIZE.bit.MULTI_PACKET_SIZE = total_bytes;
     bank->PCKSIZE.bit.BYTE_COUNT = 0;
-    ep->EPSTATUSCLR.reg |= USB_DEVICE_EPSTATUSCLR_BK0RDY;
-    ep->EPINTFLAG.reg |= USB_DEVICE_EPINTFLAG_TRFAIL0;
+    ep->EPSTATUSCLR.reg = USB_DEVICE_EPSTATUSCLR_BK0RDY;
+    ep->EPINTFLAG.reg = USB_DEVICE_EPINTFLAG_TRFAIL0;
   } else
   {
     bank->PCKSIZE.bit.MULTI_PACKET_SIZE = 0;
     bank->PCKSIZE.bit.BYTE_COUNT = total_bytes;
-    ep->EPSTATUSSET.reg |= USB_DEVICE_EPSTATUSSET_BK1RDY;
-    ep->EPINTFLAG.reg |= USB_DEVICE_EPINTFLAG_TRFAIL1;
+    ep->EPSTATUSSET.reg = USB_DEVICE_EPSTATUSSET_BK1RDY;
+    ep->EPINTFLAG.reg = USB_DEVICE_EPINTFLAG_TRFAIL1;
   }
 
   return true;

P.S.: I did not search for it all the time. Just here and then.

robert-hh added a commit to robert-hh/tinyusb that referenced this issue Jun 27, 2022
This PR fixes a transmit lock-up, which happens, when data is received
and sent at the sime time at moderate to high speeds, like code
which just echoes incoming data.

In my case, an issue was reported here:
micropython/micropython#8521
robert-hh added a commit to robert-hh/micropython that referenced this issue Jun 27, 2022
This is a problem in TinyUSB. A PR is made. Until that is fixed, the
changed file is kept locally.
robert-hh added a commit to robert-hh/micropython that referenced this issue Jun 27, 2022
This is a problem in TinyUSB. A PR is made. Until that is fixed, the
changed file is kept locally.
@dpgeorge
Copy link
Member

Great work! Let's hope it gets fixed in upstream TinyUSB soon.

@robert-hh
Copy link
Contributor Author

Thanks. I made a PR, but for the current master branch. The SAMD port works with that branch. So either we could work with different commits for the various ports, or we switch them all.

@dpgeorge
Copy link
Member

It's too difficult to use different TinyUSB commits for different ports... would be best to wait for the next release of TinyUSB.

@robert-hh
Copy link
Contributor Author

There is already a new TinyUSB version. MicroPython uses version 0.12.0. The master branch is beyond the version 0.13.0 tag. And the RP2 branch does not build with that version. Two errors:

a) an undefined C Macro: PICO_SHARED_IRQ_HANDLER_HIGHEST_ORDER_PRIORITY
b) a missing function: undefined reference to "tud_task"

So using a new TinyUSB version requires rework of some ports.

@robert-hh
Copy link
Contributor Author

Adaption for rp2 should be easy. a) is just a different #define, which is already present in newer version for the pico lib.
b) tud_task() is now tud_task_ext() with two arguments, of which one is not used. So either a wrapper or a redefine will do.

robert-hh added a commit to robert-hh/tinyusb that referenced this issue Jun 28, 2022
This PR fixes a transmit lock-up, which happens, when data is received
and sent at the sime time at moderate to high speeds, like code
which just echoes incoming data.

In my case, an issue was reported here:
micropython/micropython#8521
@robert-hh
Copy link
Contributor Author

robert-hh commented Jun 28, 2022

So the change is already merged with the master branch of tinyusb. I asked also Ha Thach to have a v0.12.0 branch for a while fro MicroPython, but he is reluctant to do so; understandable. I use a local copy of that changed file at the moment, because I did not expect such a fast response. And I can take care of checking the mimxrt and rp2 branch for an updated tinyusb version. But there are ESP32, nrf and STM32(?) which use that as well. I have no NRF boards for testing, and just one ESP-Cx variant. Of course I can try the updated lib with these. So it's some work to do.

@dpgeorge
Copy link
Member

ESP32 uses it's own version of TinyUSB in the IDF, so no need to worry about that.

There are no stm32 boards using TinyUSB.

I have nRF boards that I can test.

@robert-hh
Copy link
Contributor Author

Thanks. Ha Thach mentioned that he will publish a release (v1.x?) of TinyUSB in the next days.

robert-hh added a commit to robert-hh/micropython that referenced this issue Jun 29, 2022
This is a problem in TinyUSB. A PR is made. Until that is fixed, the
changed file is kept locally.
robert-hh added a commit to robert-hh/micropython that referenced this issue Jun 30, 2022
This is a problem in TinyUSB. A PR is made. Until that is fixed, the
changed file is kept locally.
robert-hh added a commit to robert-hh/micropython that referenced this issue Jun 30, 2022
This was a problem in TinyUSB. It is fixed in the master branch of TinyUSB.
Until this version is used by MicroPython, the changed file is kept locally.
robert-hh added a commit to robert-hh/micropython that referenced this issue Jul 3, 2022
This was a problem in TinyUSB. It is fixed in the master branch of TinyUSB.
Until this version is used by MicroPython, the changed file is kept locally.
robert-hh added a commit to robert-hh/micropython that referenced this issue Jul 4, 2022
This is a problem in TinyUSB. A PR is made. Until that is fixed, the
changed file is kept locally.
robert-hh added a commit to robert-hh/micropython that referenced this issue Jul 5, 2022
This is a problem in TinyUSB. A PR is made. Until that is fixed, the
changed file is kept locally.
robert-hh added a commit to robert-hh/micropython that referenced this issue Jul 11, 2022
This is a problem in TinyUSB. A PR is made. Until that is fixed, the
changed file is kept locally.
robert-hh added a commit to robert-hh/micropython that referenced this issue Jul 16, 2022
This is a problem in TinyUSB. A PR is made. Until that is fixed, the
changed file is kept locally.
robert-hh added a commit to robert-hh/micropython that referenced this issue Jul 18, 2022
This is a problem in TinyUSB. A PR is made. Until that is fixed, the
changed file is kept locally.
@robert-hh
Copy link
Contributor Author

Fixed with the tinyusb update.

tannewt added a commit to tannewt/circuitpython that referenced this issue Oct 25, 2023
…n-main

Translations update from Hosted Weblate
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants