Skip to content

rp2: Fix USB PLL glitch during wake from light sleep. - #15301

Merged
dpgeorge merged 3 commits into
micropython:masterfrom
projectgus:bugfix/rp2_usb_lightsleep
Jun 25, 2024
Merged

dpgeorge merged 3 commits into
micropython:masterfrom
projectgus:bugfix/rp2_usb_lightsleep

Conversation

@projectgus

@projectgus projectgus commented Jun 19, 2024 •

Copy link
Copy Markdown
Contributor

Follow-up to #15111, that change works most of the time but has an intermittent bug where USB doesn't resume as expected after waking from light sleep. Thanks to @GitHubsSilverBullet who reported it here: https://github.com/orgs/micropython/discussions/14401#discussioncomment-9675017

Turns out waking calls clocks_init() which will re-initialise the USB PLL. Most of the time this is OK but occasionally it seems like the clock glitches the USB peripheral and it stops working until the next hard reset.

Adds a machine.lightsleep() test that consistently hangs in the first two dozen iterations of a single test run on rp2 without this fix. Passed over 100 full test runs in a row with this fix.

The test is currently rp2-only as it seems similar lightsleep USB issues exist on other ports (both pyboard and ESP32-S3 native USB don't send any data to the host after waking, until they receive something from the host first.)

EDIT: Added one more small commit to always disable USB if there is no timeout (i.e. entering DORMANT mode where the external oscillator is disabled.)

This work was funded through GitHub Sponsors.

@codecov

codecov Bot commented Jun 19, 2024 •

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.42%. Comparing base (cfa55b4) to head (f60c71d).

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #15301   +/-   ##
=======================================
  Coverage   98.42%   98.42%           
=======================================
  Files         161      161           
  Lines       21248    21248           
=======================================
  Hits        20914    20914           
  Misses        334      334           

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

@github-actions

Copy link
Copy Markdown

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:    +0 +0.000% TEENSY40
        rp2:    +0 +0.000% RPI_PICO_W
       samd:    +0 +0.000% ADAFRUIT_ITSYBITSY_M4_EXPRESS

@dpgeorge

Copy link
Copy Markdown
Member

Maybe we should report this to pico-sdk, and potentially they can add this argument to clocks_init()?

But for now, patching it here is OK.

Can you please split out the addition of our copy of clocks_init() to a separate commit and update the top-level LICENSE? See 8438c87

Adapts pico-sdk clocks_init() into clocks_init_optional_usb() which takes
an argument to initialise USB clocks or not.

To avoid a code size increase the SDK clocks_init() function is linker
wrapped to become clocks_init_optional_usb(true).

This work was funded through GitHub Sponsors.

Signed-off-by: Angus Gratton <[email protected]>
Follow-up to a84c7a0, this commit works most of the time but has an
intermittent bug where USB doesn't resume as expected after waking from
light sleep.

Turns out waking calls clocks_init() which will re-initialise the USB PLL.
Most of the time this is OK but occasionally it seems like the clock
glitches the USB peripheral and it stops working until the next hard reset.

Adds a machine.lightsleep() test that consistently hangs in the first
two dozen iterations on rp2 without this fix. Passed over 100 times in a
row with this fix.

The test is currently rp2-only as it seems similar lightsleep USB issues
exist on other ports (both pyboard and ESP32-S3 native USB don't send any
data to the host after waking, until they receive something from the host
first.)

This work was funded through GitHub Sponsors.

Signed-off-by: Angus Gratton <[email protected]>
@projectgus
projectgus force-pushed the bugfix/rp2_usb_lightsleep branch from c1fa5e3 to 068d9bf Compare June 25, 2024 06:26
@projectgus

Copy link
Copy Markdown
Contributor Author

Can you please split out the addition of our copy of clocks_init() to a separate commit and update the top-level LICENSE? See 8438c87

No worries, done. PTAL!

In this mode, XOSC is stopped so can't really keep
the USB PLL enabled.

Signed-off-by: Angus Gratton <[email protected]>
@projectgus

Copy link
Copy Markdown
Contributor Author

Added one more small commit to always disable USB if going to DORMANT mode with no external oscillator.

Have also raised a feature request with pico-sdk, see link above.

@dpgeorge

Copy link
Copy Markdown
Member

All looks good, thank you!

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants