10000 lib/tinyusb: Update TinyUSB to release 0.17.0 by dpgeorge · Pull Request #15902 · micropython/micropython · GitHub
[go: up one dir, main page]
Skip to content

lib/tinyusb: Update TinyUSB to release 0.17.0 #15902

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Sep 26, 2024
Merged

lib/tinyusb: Update TinyUSB to release 0.17.0 #15902

merged 2 commits into from
Sep 26, 2024

Conversation

dpgeorge
Copy link
Member
@dpgeorge dpgeorge commented Sep 24, 2024 •
edited
Loading

Summary

TinyUSB (finally!) has a new release, 0.17.0. Usually we track latest master but now there's a release it would be good to update to that.

More importantly, an updated TinyUSB is needed for both RP2350 support #15619 and ESP32 shared TinyUSB integration #15108. Both those PRs update TinyUSB to a commit prior to the 0.17.0 release, and it would be good to consolidate those updates into a single update of TinyUSB, hence this PR.

Testing

Testing is needed for all existing TinyUSB ports:

  • mimxrt
  • nrf
  • renesas-ra FS
  • renesas-ra HS
  • rp2
  • samd

@dpgeorge dpgeorge added the lib Relates to lib/ directory in source label Sep 24, 2024
@dpgeorge dpgeorge added this to the release-1.24.0 milestone Sep 24, 2024
This was referenced Sep 24, 2024
Merged
Merged
@codecov Codecov
Copy link
codecov bot commented Sep 24, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.57%. Comparing base (40048f0) to head (b0ba151).
Report is 2 commits behind head on master.

Additional details and impacted files 
@@           Coverage Diff           @@
##           master   #15902   +/-   ##
=======================================
  Coverage   98.57%   98.57%           
=======================================
  Files         164      164           
  Lines       21336    21336           
=======================================
  Hits        21031    21031           
  Misses        305      305

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

@github-actions GitHub Actions
Copy link

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:   +56 +0.015% TEENSY40
        rp2:   +40 +0.004% RPI_PICO_W
       samd:   +68 +0.025% ADAFRUIT_ITSYBITSY_M4_EXPRESS[incl +4(bss)]
  qemu rv32:    +0 +0.000% VIRT_RV32

@robert-hh
Copy link
Contributor
robert-hh commented Sep 24, 2024 •
edited
Loading

Tested with

  • MIMXRT 1062 (Teensy 4.0), MIMXRT1011, MIMXRT1015, MIMXRT1021, MIMXRT1052, MIMXRT1172
  • SAMD21 with and without external flash ,
  • SAMD51,
  • RP2040, RP2350 ARM, RP2350 RISCV
  • NRF52840

all build and work. NRF seems better than before. Code size increase for SAMD21 is 48bytes. Lacking a ARDUINO_PORTENTA_C33 board I could not test it with Renesas-RA. But at least the firmware builds.

@andrewleech
Copy link
Contributor

Thanks @robert-hh I've got a renesas FS USB board I'll be able to test with.

@iabdalkader no pressure but if you get a chance to test on the portenta renesas or would be good to confirm it hasn't been broken by this change.

@dpgeorge
Copy link
Member Author

@robert-hh thanks for testing! That's very helpful.

I also tested with the following:

  • rp2 / Pico
  • samd / Itsy Bitsy M0 Express
  • mimxrt / Teensy 4.0

I didn't see any regressions. USB CDC throughput is unchanged.

@andrewleech
Copy link
Contributor

USB CDC throughput is unchanged.

Hi @dpgeorge do you have a script/pattern you could share that you use to test CDC throughout? I'd like to check that before/after on renesas and esp boards.

@dpgeorge
Copy link
Member Author

do you have a script/pattern you could share that you use to test CDC throughout?

Yes! See #15909.

@andrewleech
Copy link
Contributor

Thanks for that @dpgeorge !
Renesas FS USB is slightly slower (right is with this PR applied), but not dramatically:
image

@dpgeorge
Copy link
Member Author

You probably need to do a few runs before you can compare code changes. Eg the load of your PC can change the throughput.

@andrewleech
Copy link
Contributor
andrewleech commented Sep 25, 2024 •
edited
Loading

Yeah I did a few repeats and got similar results, though to be fair I didn't go back to original to re-check that.
I did also try applying [renesas-ra/usb: Use interrupt rather than polling for usb task.(]#15550 ) but didn't see any change to the speed - I guess the polling will be working just fine with this test code where it calls sleep in loops etc.

edit: Ok I did rerun both MR code then went back to master and got similar results on the large block sizes: 0.53 MBits/sec vs 0.56 MBits/sec. Not a deal breaker I don't think, but curious.

@dpgeorge
Copy link
Member Author

@andrewleech so you were able to test the renesas-ra port? If it worked then I think we are good to go with this PR.

@andrewleech
Copy link
Contributor
andrewleech commented Sep 26, 2024 •
edited
Loading

@dpgeorge sorry I wasn't entirely clear - yes renesas-ra port is tested for FS connections and it does work. I don't have hardware to test HS myself though. FS is slightly slower but not enough to prevent moving forward I don't think.

@dpgeorge
Copy link
Member Author

@andrewleech OK, thanks. So the remaining test would be for renesas-ra port in HS mode.

@iabdalkader are you able to test this PR on an Arduino Portenta C33?

@iabdalkader
Copy link
Contributor

@iabdalkader are you able to test this PR on an Arduino Portenta C33?

@andrewleech @dpgeorge Yes I can test it today. I tested this too #15550 and it works fine, if you merge it first it will be easier for me to test both.

@dpgeorge
Copy link
Member Author

I tested this too #15550 and it works fine, if you merge it first it will be easier for me to test both.

OK, that's now merged.

@iabdalkader
Copy link
Contributor

I pulled this PR locally, and rebased on master and tested C33, and it seems to work fine.

@dpgeorge
lib/tinyusb: Update to version 0.17.0.
09fa90e 
Includes support for RP2350, and improvements for ESP32.

Signed-off-by: Damien George <damien@micropython.org>
@dpgeorge
shared/tinyusb: Use new persistent-tx-fifo configure interface.
b0ba151 
The old configuration option has been removed from TinyUSB.

Signed-off-by: Damien George <damien@micropython.org>
@dpgeorge dpgeorge force-pushed the lib-tinyusb-update-to-0170 branch from 46e6ee8 to b0ba151 Compare September 26, 2024 13:15
@dpgeorge
Copy link
Member Author

Thanks @iabdalkader for testing.

@dpgeorge dpgeorge merged commit b0ba151 into micropython:master Sep 26, 2024
63 checks passed
@dpgeorge dpgeorge deleted the lib-tinyusb-update-to-0170 branch September 26, 2024 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
lib Relates to lib/ directory in source
Projects
None yet
Milestone
release-1.24.0
Development

Successfully merging this pull request may close these issues.
4 participants
@dpgeorge @robert-hh @andrewleech @iabdalkader
0