shared/tinyusb: Buffer startup stdout (banner) to send once usb cdc is connected. #14485

andrewleech · 2024-05-15T01:34:17Z

At startup, buffer stdout / microypthon banner so that it can be printed on initial connection of mpremote, just like stm port does.

Requires: hathach/tinyusb#2629 (merged) and hathach/tinyusb#2647 (merged)

Extra details: hathach/tinyusb#2595

Sitting on top of #14462

functional change

This change is most obvious when you've first plugged in a micropython device (or hit reset) when it's a board that uses USB (CDC) serial in the chip itself for the repl interface. This doesn't apply to UART going via a separate usb-serial chip.

On all ports other than stm32 currently, nothing happens when you first run a terminal like mpremote on a freshly booted board.
A user might typically first hit enter to look for a repl prompt >>> to ensure they are connected to a micropython board, then maybe hit ctrl-d to soft reboot and see the banner to check they're connected to the right board.

On stm however, on a freshly started board when you connect mpremote the banner is immediately shown!.

This PR extends this behaviour to rp2, mimxrt, samd and renesas. It's also extended to esp32s2 / s3 in the follow up PR.

github-actions · 2024-05-15T01:43:05Z

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:  +260 +0.072% TEENSY40[incl -8(bss)]
        rp2:  +820 +0.244% RPI_PICO[incl +516(bss)]
       samd:  +792 +0.298% ADAFRUIT_ITSYBITSY_M4_EXPRESS[incl +520(bss)]

codecov · 2024-05-15T04:40:40Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.39%. Comparing base (025d10a) to head (9b8d07d).
Report is 36 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #14485   +/-   ##
=======================================
  Coverage   98.39%   98.39%           
=======================================
  Files         161      161           
  Lines       21204    21204           
=======================================
  Hits        20864    20864           
  Misses        340      340

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

andrewleech · 2024-05-15T04:42:18Z

Initial tests working on rp2!

anl@STEP: ~ $ mpremote a1
Connected to MicroPython at /dev/ttyACM1
Use Ctrl-] or Ctrl-x to exit this shell
MicroPython v1.23.0-preview.377.g754ae8b76d.dirty on 2024-05-15; Raspberry Pi Pico with RP2040
Type "help()" for more information.
>>>

andrewleech · 2024-05-16T04:17:14Z

anl@STEP: ~ $ mpremote
Connected to MicroPython at /dev/ttyACM0
Use Ctrl-] or Ctrl-x to exit this shell
MicroPython v1.23.0-preview.380.g7ab4754386.dirty on 2024-05-16; Seeed Xiao with SAMD21G18A
Type "help()" for more information.
>>>

andrewleech · 2024-05-16T04:17:33Z

anl@STEP: ~ $ mpremote
Connected to MicroPython at /dev/ttyACM0
Use Ctrl-] or Ctrl-x to exit this shell
MicroPython v1.23.0-preview.378.g9958203b3d.dirty on 2024-05-15; i.MX RT1010 EVK with MIMXRT1011DAE5A
Type "help()" for more information.
>>>

andrewleech · 2024-05-16T06:18:27Z

anl@STEP: ~ $ mpremote
Connected to MicroPython at /dev/ttyACM0
Use Ctrl-] or Ctrl-x to exit this shell
MicroPython v1.23.0-preview.381.gd3a95507b6.dirty on 2024-05-16; PCA10059 with NRF52840
Type "help()" for more information.
>>>

andrewleech · 2024-05-16T08:43:46Z

@mattytrentini

anl@STEP: ~ $ mpremote
Connected to MicroPython at /dev/ttyACM0
Use Ctrl-] or Ctrl-x to exit this shell
MicroPython v1.23.0-preview.378.g526af512b4.dirty on 2024-05-16; ItsyBitsy M4 Express with SAMD51G19A
Type "help()" for more information.
>>>

projectgus

This will be a really nice addition, thanks @andrewleech 😁

projectgus · 2024-05-22T00:14:24Z

shared/tinyusb/mp_usbd_cdc.c

+        }
+        n = MIN(n, tud_cdc_write_available());
+        if (n == 0) {
+            break;


IIUC, if disconnected then after the tinyusb FIFO is full then further output is dropped until USB-CDC connects.

Could this lead to some misleading situations where USB host connects and receives:

banner
early program output
[unknown number of dropped bytes here]
contemporary output that was sent after USB host connected

Is that right? Does it seem like a problem?

The best solution I can see from this perspective would be to rotate bytes out of the overflowing FIFO as needed, but (a) this is quite slow if USB never connects and (b) not supported by current TinyUSB API.

However there is tud_cdc_n_write_clear() which empties the write FIFO. What do you think about doing this each time the FIFO fills up while disconnected? It means maybe some buffered bytes are lost, but the "new after old with missing middle" situation shown above can't happen.

(Note this isn't a request necessarily, I'm not fully convinced this is worth worrying about.)

Related request I do have is that it'd be good to have a short comment or so in here please, outlining the intended execution flow for this "disconnected, buffering" case as it may not be obvious to future readers why it's structured like this.

Yeah you raise a good question. My thoughts were that keeping the banner (and maybe a little after it) is a good thing to display on connection.
But yes it could be somewhat confusing if there's a running application with more information, the disconnect in log could seem quite random. I'm not too sure how STM handles this currently, whether it's the initial data persisted or the latest data. @dpgeorge do you have a preference here?

A comment or two is definitely warranted regardless!

In stm32 it's a rolling ring buffer, so new data is always placed into the pending CDC output buffer, and if the ring buffer is full then the oldest data is lost. That means on connection to a USB host, the host sees a consistent history of the most recent data, possibly truncated at the beginning of that data.

Thanks @dpgeorge, the consistent view of "latest data" does make sense really. I'll have a closer look to see if I can make it automatically overwrite, I actually think this might be possible.

If not, the suggestion of "clear on full" might be worth doing instead.

Hah, this is convenient: https://github.com/hathach/tinyusb/blob/1f259b3ab0f1b9587dee9c0bfb5574d70d021640/src/class/cdc/cdc_device.c#L250

void cdcd_init(void) { ... // Config TX fifo as overwritable at initialization and will be changed to non-overwritable // if terminal supports DTR bit. Without DTR we do not know if data is actually polled by terminal. // In this way, the most current data is prioritized. tu_fifo_config(&p_cdc->tx_ff, p_cdc->tx_ff_buf, TU_ARRAY_SIZE(p_cdc->tx_ff_buf), 1, true);

So yeah if I basically remove the "space in buffer" check when not connected, it'll pretty much "just work". Though I'll need to confirm behavior when you try to write more than a buffer's worth in one go, whether it keeps the start or end of the data.

That is certainly convenient!

It may actually be impossible to debug this with mpremote, given the pyserial hard-coded flush of the input buffer on open. 🤦 But it should be visible with other serial programs, I'd expect.

I made the change and it seemed to work fine! Even with mpremote.... I added a main.py on the samd51 board:
print(list(range(512)))

Now when I start mpremote I see:

$ mpremote Connected to MicroPython at /dev/ttyACM0 Use Ctrl-] or Ctrl-x to exit this shell 486, 487, 488, 489, 490, 491, 492, 493, 494, 495, 496, 497, 498, 499, 500, 501, 502, 503, 504, 505, 506, 507, 508, 509, 510, 511] MicroPython v1.23.0-preview.379.g130e763226.dirty on 2024-05-25; ItsyBitsy M4 ExpreType "help()" for more information. >>>

shared/tinyusb/mp_usbd_cdc.c

andrewleech · 2024-05-24T03:55:09Z

Note this PR currently has the TinyUSB submodule pinned to my PR with required SOF updates, so it won't currently build easily.

This upstream PR will need to be merged first, it's blocked by the need to test on renesas.

projectgus · 2024-05-24T04:14:22Z

Note this PR currently has the TinyUSB submodule pinned to my PR with required SOF updates, so it won't currently build easily.

FWIW I think we could emulate something similar by adding a callback in the mp_usbd_task, or maybe even soft timer. However I think better to stick with the approach you've already used, if it's going to be available soon.

andrewleech · 2024-05-24T04:36:42Z

Yes I'd originally thought of using a soft timer however it does have some overheads, and I'm not sure if all needed ports have it enabled.

I think the upstream change should be achievable, I've had very productive discussions on it already.

I'm just blocked by renesas port testing, I can't figure out the auto generated code being used in the board profiles there enough to enable USB on the evk board I've loaned.

andrewleech · 2024-05-27T12:42:12Z

I've now got a renesas board working finally and have finished the implementation there, it's all working too.

I just need to clean up some of the commented code in the nrf stuff and it's all good to go!

Signed-off-by: Andrew Leech <andrew@alelec.net>

andrewleech · 2024-05-29T02:11:25Z

This PR has tested working on rp2, mimxrt, samd and renesas.

esp32 support is in follow-up PR #15108.
nrf support is in follow-up PR #15158.

The required updates to TinyUSB have been merged upstream and the submodule here is now pinned to the matching master commit.

As such I consider this PR complete and ready for review.

andrewleech force-pushed the tinyusb_startup_banner_buffer branch from 7fddddc to 4d29eec Compare May 15, 2024 04:40

andrewleech changed the title ~~DRAFT: shared/tinyusb: Buffer startup stdout (banner) to send once usb cdc is connected.~~ shared/tinyusb: Buffer startup stdout (banner) to send once usb cdc is connected. May 15, 2024

andrewleech force-pushed the tinyusb_startup_banner_buffer branch from 4d29eec to 3b8e785 Compare May 15, 2024 11:21

andrewleech mentioned this pull request May 16, 2024

Add support for dcd_sof_enable() to some additional ports. hathach/tinyusb#2647

Merged

andrewleech force-pushed the tinyusb_startup_banner_buffer branch from 3b8e785 to 526af51 Compare May 16, 2024 06:13

projectgus reviewed May 22, 2024

View reviewed changes

andrewleech force-pushed the tinyusb_startup_banner_buffer branch from 526af51 to aff686c Compare May 24, 2024 03:26

andrewleech mentioned this pull request May 24, 2024

ports/esp32: Use shared/tinyusb integration. #15108

Open

andrewleech mentioned this pull request May 24, 2024

shared/tinyusb: Add common cdc tx/rx functions. #14462

Merged

andrewleech force-pushed the tinyusb_startup_banner_buffer branch from 130e763 to 54e0abf Compare May 27, 2024 11:57

andrewleech and others added 8 commits May 29, 2024 11:48

shared/tinyusb: Add common cdc tx/rx functions.

7bfd123

Signed-off-by: Andrew Leech <andrew@alelec.net>

rp2/mphalport: Use shared/tinyusb cdc functions.

16a5b29

Signed-off-by: Andrew Leech <andrew@alelec.net>

mimxrt/mphalport: Use shared/tinyusb cdc functions.

d558b35

Signed-off-by: Andrew Leech <andrew@alelec.net>

renesas-ra/mphalport: Use shared/tinyusb cdc functions.

5a6874c

Signed-off-by: Andrew Leech <andrew@alelec.net>

samd/mphalport: Use shared/tinyusb cdc functions.

7dff5ba

Signed-off-by: Andrew Leech <andrew@alelec.net>

esp32/ARDUINO_NANO_ESP32: Update shared/tinyusb/mp_usbd_cdc.c filename.

2313625

Signed-off-by: Andrew Leech <andrew@alelec.net>

nrf/Makefile: Update shared/tinyusb/mp_usbd_cdc.c filename.

e7b4332

Signed-off-by: Andrew Leech <andrew@alelec.net>

shared/tinyusb: Buffer startup stdout/banner to send on cdc connection.

9b8d07d

Signed-off-by: Andrew Leech <andrew@alelec.net>

andrewleech force-pushed the tinyusb_startup_banner_buffer branch from 54e0abf to 9b8d07d Compare May 29, 2024 01:59

andrewleech mentioned this pull request May 29, 2024

ports/nrf: Consolidate stdio functions. #15158

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

shared/tinyusb: Buffer startup stdout (banner) to send once usb cdc is connected. #14485

shared/tinyusb: Buffer startup stdout (banner) to send once usb cdc is connected. #14485

andrewleech commented May 15, 2024 •

edited

github-actions bot commented May 15, 2024 •

edited

codecov bot commented May 15, 2024 •

edited

andrewleech commented May 15, 2024 •

edited

andrewleech commented May 16, 2024

andrewleech commented May 16, 2024

andrewleech commented May 16, 2024

andrewleech commented May 16, 2024

projectgus left a comment

projectgus May 22, 2024

andrewleech May 22, 2024 •

edited

dpgeorge May 24, 2024

andrewleech May 24, 2024

andrewleech May 24, 2024 •

edited

projectgus May 27, 2024

andrewleech May 28, 2024

andrewleech commented May 24, 2024

projectgus commented May 24, 2024

andrewleech commented May 24, 2024

andrewleech commented May 27, 2024

andrewleech commented May 29, 2024

shared/tinyusb: Buffer startup stdout (banner) to send once usb cdc is connected. #14485

Are you sure you want to change the base?

shared/tinyusb: Buffer startup stdout (banner) to send once usb cdc is connected. #14485

Conversation

andrewleech commented May 15, 2024 • edited

functional change

github-actions bot commented May 15, 2024 • edited

codecov bot commented May 15, 2024 • edited

Codecov Report

andrewleech commented May 15, 2024 • edited

andrewleech commented May 16, 2024

andrewleech commented May 16, 2024

andrewleech commented May 16, 2024

andrewleech commented May 16, 2024

projectgus left a comment

Choose a reason for hiding this comment

projectgus May 22, 2024

Choose a reason for hiding this comment

andrewleech May 22, 2024 • edited

Choose a reason for hiding this comment

dpgeorge May 24, 2024

Choose a reason for hiding this comment

andrewleech May 24, 2024

Choose a reason for hiding this comment

andrewleech May 24, 2024 • edited

Choose a reason for hiding this comment

projectgus May 27, 2024

Choose a reason for hiding this comment

andrewleech May 28, 2024

Choose a reason for hiding this comment

andrewleech commented May 24, 2024

projectgus commented May 24, 2024

andrewleech commented May 24, 2024

andrewleech commented May 27, 2024

andrewleech commented May 29, 2024

andrewleech commented May 15, 2024 •

edited

github-actions bot commented May 15, 2024 •

edited

codecov bot commented May 15, 2024 •

edited

andrewleech commented May 15, 2024 •

edited

andrewleech May 22, 2024 •

edited

andrewleech May 24, 2024 •

edited