8000 shared/tinyusb: Use device event hook to schedule USB task. by projectgus · Pull Request #17338 · micropython/micropython · GitHub
[go: up one dir, main page]
Skip to content

shared/tinyusb: Use device event hook to schedule USB task. #17338

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
Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

shared/tinyusb: Use device event hook to schedule USB task. #17338

wants to merge 2 commits into from
+2 −37

Conversation

projectgus
Copy link
Contributor
@projectgus projectgus commented May 22, 2025 •
edited
Loading

Summary

Previously MicroPython ports would linker-wrap dcd_event_handler in order to schedule the USB task callback to run when needed.

TinyUSB 0.16 added proper support for an event hook to do the same thing without the hacky linker wrapping. (See hathach/tinyusb#2303)

(The vendored TinyUSB is 0.17.0 since 09fa90e, the version in ESP-IDF is 0.18.0 for both ESP-IDF V5.2 and V5.4.1.)

As a bonus:

This work was funded through GitHub Sponsors.

Testing

  • Built RPI_PICO and ESP32_GENERIC_S3 boards, interacted with the default REPL, and also ran the usb/device/mouse example on both boards.

@projectgus projectgus added the shared Relates to shared/ directory in source label May 22, 2025
@projectgus projectgus requested a review from andrewleech May 22, 2025 07:51
@codecov Codecov
Copy link
codecov bot commented May 22, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.54%. Comparing base (dc1af38) to head (da7017f).
Report is 1 commits behind head on master.

Additional details and impacted files 
@@           Coverage Diff           @@
##           master   #17338   +/-   ##
=======================================
  Coverage   98.54%   98.54%           
=======================================
  Files         169      169           
  Lines       21898    21898           
=======================================
  Hits        21579    21579           
  Misses        319      319

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@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:   +16 +0.004% TEENSY40
        rp2:   -24 -0.003% RPI_PICO_W
       samd:   -16 -0.006% ADAFRUIT_ITSYBITSY_M4_EXPRESS
  qemu rv32:    +0 +0.000% VIRT_RV32

andrewleech
andrewleech approved these changes May 22, 2025
View reviewed changes
Copy link
Contributor
@andrewleech andrewleech left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice find @projectgus ! That's much cleaner.

I do see that queue_event()/tud_event_hook_cb() is called from a number of places in the ISR for different types of events, my initial thought was maybe we only want to trigger on some events or prevent repeat calls if there are multiple events in one ISR.

But now I've confirmed that's not an issue, this new hook is queing up the usb task in the c-scheduler which has built in protection against re-scheduling of the same node, subsequent schedules are just ignored if it's already queued to be run (py/scheduler.c -> mp_sched_schedule_node())

@dpgeorge
Copy link
Member

Tested on ARDUINO_NANO_33_BLE_SENSE with the following:

>>> import time
>>> print('test');time.sleep(1)
test
>>>

Prior to this PR, the "test" output would not appear until after the 1 second elapsed. With this PR it now works properly, "test" is printed immediately, and then it waits 1 second before the prompt appears.

Great work @projectgus getting this hook merged to TinyUSB!

@dpgeorge
Copy link
Member

Could we now get rid of MICROPY_LINK_TINYUSB in ports/esp32/esp32_common.cmake?

@projectgus projectgus force-pushed the refactor/tinyusb_event_hook branch from 71b180b to 6de46d1 Compare May 23, 2025 04:05
@projectgus
Copy link
Contributor Author
projectgus commented May 23, 2025 •
edited
Loading

Could we now get rid of MICROPY_LINK_TINYUSB in ports/esp32/esp32_common.cmake?

Oh, good catch! Have taken it out as well.

Prior to this PR, the "test" output would not appear until after the 1 second elapsed. With this PR it now works properly,

Huh, that's good! I see the nrf port of TinyUSB sometimes calls usb_defer_func() to defer an ISR callback, and this invokes the new hook function as well (previously it would have bypassed dcd_event_handler)

projectgus added 2 commits May 23, 2025 14:11
@projectgus
shared/tinyusb: Use device event hook to schedule USB task.
4691435 
Previously MicroPython ports would linker-wrap dcd_event_handler
in order to schedule the USB task callback to run when needed.

TinyUSB 0.16 added proper support for an event hook to do the
same thing without the hacky linker wrapping.

This work was funded through GitHub Sponsors.

Signed-off-by: Angus Gratton <angus@redyak.com.au>
@projectgus
nrf: Revert "nrf/Makefile: Enable LTO by default only on newer gcc.".
da7017f 
This reverts commit 62e0fa0.

Reverting as the only linker wrap needed for nrf port was removed
in the parent commit.

This work was funded through GitHub Sponsors.

Signed-off-by: Angus Gratton <angus@redyak.com.au>
@projectgus projectgus force-pushed the refactor/tinyusb_event_hook branch from 6de46d1 to da7017f Compare May 23, 2025 04:11
@projectgus
Copy link
Contributor Author

Pushed one more small tweak, can remove the dcd.h include line from mp_usbd.c

@dpgeorge
Copy link
Member

can remove the dcd.h include line from mp_usbd.c

Very good!

@robert-hh
Copy link
Contributor

@dpgeorge I tried to replicate your test with print('test');time.sleep(1) by fetching the actual PR and building it. It still prints "test" after one second. Did you update the tinyusb lib?

@dpgeorge
Copy link
Member

Did you update the tinyusb lib?

No, I didn't change anything else. Just this PR on latest master. Maybe you need to make clean first? And did you verify that it flashed the device correctly, eg check the date of the build on the device?

@robert-hh
Copy link
Contributor

I always start with make clean. But I had to update the tinyusb lib to 0.18.0. That did not happen with the PR. So maybe the PR misses the version change of tinyUSB.

@dpgeorge
Copy link
Member

But I had to update the tinyusb lib to 0.18.0.

TinyUSB should be at 0.17.0 on current master, and that's all that is needed for this PR to work.

Can you please try with a fresh clone of this repo? It really should work without needing to update TinyUSB.

@robert-hh
Copy link
Contributor
robert-hh commented May 23, 2025 •
edited
Loading

If I make a fresh clone of MicroPython, run make submodules in the nrf directory, checkout the PR 17338 and create a firmware, it works. But I do not understand why.

  • the git hashes of the PR17338 MicroPython branch and the tinyusb and nrfx submodules are the same.
  • the files of both ports/nrf subdirectories are identical.
  • the compiler used for building is the same.

So the output ought to be the same. But the firmware files differ in size and content. Do you have a hint for a reason?

@dpgeorge
Copy link
Member

So the output ought to be the same. But the firmware files differ in size and content. Do you have a hint for a reason?

Do you have a makefile or GNUmakefile in ports/nrf that is affecting the build? Or maybe some other "hidden" files? If you want to dig deeper, you can start diff'ing the object files (eg see which ones changed size) and even diff'ing the assembly dump of firmware.elf. But that's a fair bit of work. If it works with a fresh clone then I'd say it works.

@robert-hh
Copy link
Contributor

Thank you for your answer that late in the night.
I had a GNUmakefile, but removing it did not change anything. I compared with diff the tinyusb & nrf libs.
I compared the two map files of good vs bad build. There are some differences mostly in the order of the various files in created firmware. Comparing object files just tells, that all of them are different. So I went the simply way by re-cloning MP and setting up the environment as I had it before. Doing that I rebased & pushed all my branches, causing some noise with github messages.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewleech andrewleech andrewleech approved these changes

@dpgeorge dpgeorge dpgeorge approved these changes

Assignees
No one assigned
Labels
shared Relates to shared/ directory in source
Projects
None yet
Milestone
No milestone
Development

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