Ports/RP2: Allow CLK_SYS and dependants to be left running during LightSleep() #16183
Conversation
This is a re-hash of my previous bug/pull request to the Pico SDK: raspberrypi/pico-sdk#1668 Now that MicroPython has pulled the clock initiation into our own code, we can implement directly. When calling 'lightsleep()' pass the bit-fields for which clocks you want to remain on. If these are dependants of CLK_SYS then that is also kept on. The bit-fields are also use when re-initing the clocks, the ones which are left on are left as they are.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #16183 +/- ##
=======================================
Coverage 98.57% 98.57%
=======================================
Files 164 164
Lines 21345 21345
=======================================
Hits 21041 21041
Misses 304 304 ☔ View full report in Codecov by Sentry. |
|
Code size report: |
|
The first commit is not needed, I guess I screwed up the update to current HEAD... |
This demos the ability to 'lightsleep()' whilst keeping the PIO clocks running and __still__ producing LTC from the data stored in the FIFO. When enabled the power draw (on a bare Pico) drops from 3.8V @ 30mA to 3.8V @ 11mA. See: micropython/micropython#16183
There was a problem hiding this comment.
(Gah, I wrote a long comment here and github seems to have eaten it. I'll try and write it out again...)
My mistake, I messed up strategically after merging the fix in #15301 that solved our USB glitching problem. I should have gone to pico-sdk maintainers right then and asked them to support these use cases, rather than committing MicroPython to a long term fork of this part of the Pico SDK. We're already getting reports of problems with sleep in RP2350, and the new RP chips and SDK code changes will only keep coming.
EDIT: I did actually do this and the request raspberrypi/pico-sdk#1746 is still open, I just have a bad memory! 😆
Bottom line is that it's not viable for us to try and support this kind of low-level platform support code ourselves in the long term, and I should have thought strategically about it back in July.
I'll make a comment on your upstream issue report now and see if the pico-sdk developers are interested to re-engage on this. In the meantime, we can probably keep merging MicroPython-specific changes to clocks_extra.c for now - but I'd sleep better if we had a path to eventually upstreaming them.
| @@ -246,7 +260,9 @@ static void mp_machine_lightsleep(size_t n_args, const mp_obj_t *args) { | |||
| #endif | |||
|
|
|||
| // Go into low-power mode. | |||
| __wfi(); | |||
| if (timer_hw->armed & (1 << 3)) { | |||
There was a problem hiding this comment.
This has the same race described here #16412 (comment)
(but alternative fix for this problem is hopefully incoming)
| @@ -141,6 +143,12 @@ static void mp_machine_lightsleep(size_t n_args, const mp_obj_t *args) { | |||
| } | |||
| } | |||
|
|
|||
| if (n_args >= 2) | |||
| req_sleep_en0 |= mp_obj_get_int(args[1]); | |||
There was a problem hiding this comment.
It's very simple to implement, but unfortunately raw register bits isn't really something we want to expose in the machine API.
machine.lightsleep() is regretfully not well defined across ports now, but the guideline is to have some amount of abstraction that works similarly across ports. Even if we made this as a rp2-port-only addition, the inevitable introduction of new RP series chips means that there could be new or different registers into the future, which might need more special-case API choices.
I note you and @cpottle9 have both come to similar conclusions although you're trying to solve slightly different problems (more context here).
I am wondering if there's a way forward where we can support keeping some peripherals active during lightsleep, without exposing register bits in the Python API.
For PIO, it could mean adding a light_sleep_enable flag for PIO init (similar to proposed for esp32 in #16102). Then I suspect the peripheral driver could manipulate necessary sleep_en bits directly, and keep them set until PIO is deinit-ed again.
The same thing could be added for other peripherals as well. I think reference-counting the initialised peripherals is probably the most complex part. So this shouldn't take particularly much code size to implement, I (naively) think.
The logic for whether to stop CLK_SYS will probably need to look much like it does in this PR already, although I'm hopeful we can eventually upstream some of that into pico-sdk.
How does that sound?
There was a problem hiding this comment.
@mungewell, take a look at my github repo RP2-PowerControl.
It implements code to disable RP2040 and RP2350 hardware blocks using the SLEEP_EN0/1 and WAKE_EN0/1 registers.
This allows you to turn off hardware blocks you don't need ever or turn off hardware blocks when sleeping (EG: time.sleep_ms).
The difference between machine.lightsleep() and time.sleep_ms() is lightsleep turns off one or both PLL blocks depending on whether USB is needed during lightsleep().
The PLL blocks are fairly power hungry.
Using the code in RP2-PowerControl you can turn off all the hardware blocks that lightsleep() turns off except for the PLLs. Or you can choose to keep hardware blocks you need turned on.
Summary
This is a re-hash of my previous bug/pull request to the Pico SDK:
raspberrypi/pico-sdk#1668
Now that MicroPython has pulled the clock initiation into our own code, we can implement directly.
When calling 'lightsleep()' pass the bit-fields for which clocks you want to remain on. If these are dependants of CLK_SYS then that is also kept on. The bit-fields are also use when re-initing the clocks, which are left on are then left as they are.
Testing
I validate this on a RP Pico (RP2040). It is specific to the Clocks on the RP2040 and R2350.
Attached code will demonstrate using the FIFO and PIO to continuously flash the LED, whilst the CPU(s) are placed into 'lightsleep()'. This has the advantage of being MUCH lower power draw; normally 5V @ 19mA, then after ~10s it starts using 'lighsleep()' and the power draw drops to 5V @ 7mA - all whilst still flashing the LED.
The demo code is an extract of my larger project.
blink_fifo_thread.zip