Code size report:

   bare-arm:    +0 +0.000% minimal x86:    +0 +0.000%    unix x64:    +0 +0.000% standard      stm32:    +0 +0.000% PYBV10     mimxrt:  +168 +0.046% TEENSY40        rp2:  +204 +0.023% RPI_PICO_W       samd:  +156 +0.059% ADAFRUIT_ITSYBITSY_M4_EXPRESS[incl +4(bss)]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.39%. Comparing base(3c8089d) to head(d126e2a).
Report is 37 commits behind head on master.

❗Current headd126e2a differs from pull request most recent head139fcd1

Pleaseupload reports for the commit139fcd1 to get more accurate results.

@@            Coverage Diff             @@##           master   #15158      +/-   ##==========================================- Coverage   98.42%   98.39%   -0.03%==========================================  Files         161      161                Lines       21204    21204              ==========================================- Hits        20870    20864       -6- Misses        334      340       +6

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report?Share it here.

This PR does not even build for a Arduino BLE 33 Sense or a SEEED XIAO NRF52 or NRF52840_MDK_USB_DONGLE. Compile errors:

rivers/usb/usb_cdc.c: In function 'mp_usbd_port_get_serial_number':
drivers/usb/usb_cdc.c:108:46: error: 'MICROPY_HW_USB_DESC_STR_MAX' undeclared (first use in this function)
108 | MP_STATIC_ASSERT(sizeof(deviceid) * 2 <= MICROPY_HW_USB_DESC_STR_MAX);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~
../../py/misc.h:54:61: note: in definition of macro 'MP_STATIC_ASSERT'
54 | #define MP_STATIC_ASSERT(cond) ((void)sizeof(char[1 - 2 * !(cond)]))
| ^~~~
drivers/usb/usb_cdc.c:108:46: note: each undeclared identifier is reported only once for each function it appears in
108 | MP_STATIC_ASSERT(sizeof(deviceid) * 2 <= MICROPY_HW_USB_DESC_STR_MAX);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~
../../py/misc.h:54:61: note: in definition of macro 'MP_STATIC_ASSERT'
54 | #define MP_STATIC_ASSERT(cond) ((void)sizeof(char[1 - 2 * !(cond)]))
| ^~~~
CC ../../lib/tinyusb/src/portable/nordic/nrf5x/dcd_nrf5x.c
drivers/usb/usb_cdc.c:109:5: error: implicit declaration of function 'mp_usbd_hex_str'; did you mean 'mp_obj_new_str'? [-Werror=implicit-function-declaration]
109 | mp_usbd_hex_str(serial_buf, (uint8_t *)deviceid, sizeof(deviceid));
| ^~~~~~~~~~~~~~~
| mp_obj_new_str
drivers/usb/usb_cdc.c: In function 'usb_cdc_init':
drivers/usb/usb_cdc.c:124:5: error: implicit declaration of function 'mp_usbd_init'; did you mean 'mp_set_init'? [-Werror=implicit-function-declaration]
124 | mp_usbd_init();
| ^~~~~~~~~~~~
| mp_set_init
CC device/startup_nrf52840.c
drivers/usb/usb_cdc.c: In function 'USBD_IRQHandler':
drivers/usb/usb_cdc.c:143:5: error: implicit declaration of function 'tud_int_handler' [-Werror=implicit-function-declaration]
143 | tud_int_handler(0);
| ^~~~~~~~~~~~~~~

Rebasing to the current master branch fails as well due to conflicts in the make files of the other ports. Nothings difficult, just duplicating references to files, but it breaks rebase.

If you look at the CI of this PR, you see that it fails as well in building the firmware.

Ah yes it seems I rushed ahead with this, it was building previously before I went and consolidated some more settings to make UART and ble repl all work the same way.
A little more work to do to get it cleaned up!

The mpconfigboard.h file for the SEEED and Arduino board miss enabling of MICROPY_HW_ENABLE_USBDEV and MICROPY_HW_USB_CDC.
Edit: And in the master branch.tud_sof_cb_enable does not exist at all.

Ah thanks for looking into that. I should make that define enabled by default if USB is enabled.

Adding#define MICROPY_HW_ENABLE_USBDEV (1) makes both the Seeed NRF52 board and the Arduino Nano 33 BLE Sense compile & build properly, given that the old version of lib/tinyusb is used and the branch isnot rebased to master. The boards boot and works fine.
When building with the actual master branch, the Seeed board refuses to work. The USB device does not register.

This has been rebased on master (still on top / including the startup buffer PR). I haven't yet re-tested it however.

Both SEEED NRF52 and Arduino Nanon 33 BLE build and run. Only I had to define MICROPY_HW_ENABLE_USBDEV as 1.

Ah, I see now, those boards had justMICROPY_HW_USB_CDC defined, notMICROPY_HW_ENABLE_USBDEV.

I think it makes sense more broadly to haveMICROPY_HW_ENABLE_USBDEV as the "main" define to enable usb, so I've added it to those two boards. All other nrf boards with USB seemed to have both defines already.

For reference, the tinyusb update came in for#14485 and is needed for those features. I don't think it should matter for just this PR though, I really would have thought the previous tinyusb commit would be fine for the changes just made here.

Ok that build failure:

LINK build-PCA10056-s140/firmware.elf/usr/lib/gcc/arm-none-eabi/9.2.1/../../../arm-none-eabi/bin/ld: /tmp/firmware.elf.6R2Ffu.ltrans0.ltrans.o: in function `__wrap_dcd_event_handler':<artificial>:(.text+0x3f3c): undefined reference to `dcd_event_handler'

Is caused by binutils:

Bug 24406 - -Wl,--wrap= incompatible with -flto

Which was apparently fixed in binutils 2.33
The CI build is on ubuntu focal, which hasbinutils-arm-none-eabi amd64 2.34-4ubuntu1+13ubuntu1 so should have had that particular fix, though there's a few linked tickets from that bug report.

Of note I don't have the issue at home with gcc 12.x so it's been fixed since then, but it's hard to tell exactly when.

Also thanks@robert-hh your reports made it clear there were a number of build failures on nrf52840 whenMICROPY_HW_USB_CDC = 1 butMICROPY_HW_ENABLE_USBDEV = 0
I've cleaned them up too, as there's definitely cases where you might want that chip without USB being used.

Building with that PR works fine for the SEEED and Arduino boards, linking is now almost instantly. Both run. However rebasing on top of the current master branch causes conflicts in shared/tinyusb/mp_usbd_cdc.c and differences in lib/tinyusb.

P.S:: I'm using GCC 12.2 as the standard version of the OS.

Thanks I've rebased again onto master now my previous PR has been merged!

I compiled and ran this PR on two NRF52 boards. They build fine and work most of the times. But sometimes the boards do not register as USB devices any more, even when I do a full power cycle. The boards are connected to an external hub. Unplugging the hub from the PC and re-plugging helps most of the time. Connection the boards directly to the PC does not work. There are similar reports in the MIMXRT section on Discord/micropython. So it may be a generic problem of TinyUSB. Note to@projectgus.
Can you suggest something to trace the state more, now that it happens here?

@andrewleech SO I see a strange behavior on the RP2040 port. If in main.py a script is started which enables a Pin IRQ (and disables), then after a hard reset or power up USB is blocked like detailed above. I have to disconnect the USB hub and reconnect it to get USB working again, and then I see at the next boot fragments of the messages created by scripts started from main.py. If I do not add this test script, everything is fine. Once booted, soft reset works without problems.
The trigger does not have to occur. I can tie that input pin to GND and still the problem shows up.
Since I noticed the same behavior at the NRF52 board with this PR, the reason may be identical. People also mentioned problems with the MIMXRT port. I have to check.
Test script:

from machine import Pinfrom time import sleep_ms, sleep_uspin7 = Pin(7, Pin.OUT)def callback(_, p=pin7):    p(1)  # Trigger    p(0)pin3 = Pin(3, Pin.IN)pin3.irq(callback, trigger=Pin.IRQ_FALLING)sleep_ms(100)pin3.irq(handler=None)

The issue starts with the following commit:
8809ae716 shared/tinyusb: Buffer startup CDC data to send to host on connection.

And actually the device is registered, as I shows up in lsusb, but REPL is locked.

Thanks for the report and testing@robert-hh
That's a shame, I thought I'd tested that change pretty thoroughly, looks like I missed some use cases though.
I'll try to make the to replicate your test and hopefully see where the problem occurs, might be related to how the tiny USB tx buffer switches from overwrite to blocking mode

Some information about my test case: The above script is imported in main.py after having imported another python script. Sometimes it locks up, sometimes strange error messages pop up, like the one below, where importing the script should raise an error message. It was iperf3.py, which is not working at a plain RPI Pico. That all looks like having a uninitialized symbol.

ror: unexpected indent>>> TrTraceback (most recent call last):  File "<stdin>", line 1, in <module>NameError: name 'Tr' isn't defined>>> >>> TrTraceback (most recent call last):  File "<stdin>", line 1, in <module>NameError: name 'Tr' isn't defined>>> >>>  >>> Trac  acrTracTraceback (most recent call last):  File "<stdin>", line 1SyntaxError: invalid syntax

It seems that it does not have to be that specific script. It often happens if the imported script raises an exception. Then, the output it want to create plus the usual boot banner get lengthy, like 674 bytes.
Edit: But just creating a lengthy output in main.py does not cause the lock-up.

Another strange observation:
I nuked the board which showed most of the phenomenon and copied back a small main,py and the sample file. Then, the problem was gone!
Then, I copied back like 40 more files to the board, and the issue as back again.

With mprmote I get a REPL prompt, but with a part of the main.py being in the REPL input buffer, like:

Connected to MicroPython at /dev/ttyACM0Use Ctrl-] or Ctrl-x to exit this shellysh import *Traceback (most recent call last):  File "<stdin>", line 1SyntaxError: invalid syntax

The main.py I'm using now for tests is:

from upysh import *from pye import pyefrom machine import Pinimport cs1237power=Pin(13, Pin.OUT, value=1)cs=cs1237.CS1237(Pin(15), Pin(14))

With upysh.py being from mircopython-lib, pye.py is my on-board editor, both being frozen in the firmware, and cs1237.py being the driver for a cs1237 ADC, at which I noticed the problem. attached. When starting it you must NOT attach the sensor. Without the sensor an error is raised. With the sensor, all is fine. Maybe a timing issue. In some tests, instead of importing that file a simpletime.sleep_ms(500) was sufficient to cause that misbehavior. The cs1237 driver waits for ~500ms to get a response from the ADC on instantiation.

cs1237.zip

So it''s still kind of spooky. The problem may as well be somewhere in the file system or timing dependent.

The "correct" behaviour of that new buffering changeis to to keep the most recent "full stdout" buffer worth of data from before a terminal is connected for display on-connection.

So a failure in main that drops to repl should show the tail of the error message along with the repl prompt when you open a terminal.

So yeah I'd expect that snippet of traceback, though probably not the content from main itself.... It would be interesting to increase the size of the buffer and see what else is shown then. This is defined in micropython/shared/tinyusb/tusb_config.h

The lockups seen on picocom are definitely an issue that warrants deeper investigation also.

I see indeed the tail of output made during execution of main.py. But text from main.py should not be there, and the REPL history buffer must be empty. With Picocom, the REPL history buffer contains e.g.:

from upysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fiberoupysh impo Fibe

That looks very strange!

Really strange! Was that after a soft reset or a hard boot? I'm wondering if there's some static buffers / memory pointers being reused without appropriate clearing on soft reset? I'm really not sure how the buffer change could do this either unless it's a buffer (mis) allocation issue that the explicit clear in tinyusb was hiding previously.
Though maybe my disabling the buffer clear in tinyusb is also relating to initial reboot, it might be missing a reset on the tx ringbuffer head and tail pointers on a cold boot now too.

Repl history buffer though seems like a separate issue to me, though might be a similar root cause. I don't think there should be any direct relation between repl history buffer and tinyusb tx buffer though.

That all happens only after hard reset resp power cycle. Once it's working, soft reset behaves fine. It seems to be the initial set-up, which fails. And only if the start-up is delayed and/or an exception is raised by the code executed in main.py. May it be that some initialization only happens when input is requested from REPL? Or the opposite? It could be a timing effect.

I think the root cause is in the linked issue above (found by debugging@robert-hh's case above and then refining it.)

@andrewleech I see you made changes. Is this ready for re-review?

Not quite sorry@dpgeorge I need to test them still.

@dpgeorge I've addressed your review feedback thanks and re-tested on the nrf52840 dongle and dev board.

I also tested USB without the poll after adding theLDFLAGS += -Wl,--wrap=dcd_event_handler and it appears to work correctly with just the interrupt (like on other ports) which is great!

Thanks for updating, the changes now look good.

I tested this on anARDUINO_NANO_33_BLE_SENSE and it works well. Tested:

• CDC upload and download speed and integrity
• mpremote cp files to and from the device over USB CDC
• run the test suite