Add pico_low_power library#2852
Conversation
Allows for much simpler custom linker scripts
…ript_var variables Means that CMake doesn't need to know the default memory addresses for different platforms
…l files easier Restructured so that it includes the platform-specific files before common ones, so common ones can be overridden
Use new include_linker_script_dir and use_linker_script_file functions to add the linker arguments
Breaking change for Bazel builds using different binary types, instead of setting PICO_DEFAULT_LINKER_SCRIPT to eg `//src/rp2_common/pico_crt0:no_flash_linker_script` it is now `//src/rp2_common/pico_standard_link:no_flash_linker_script`
Treat rp2040 layout (boot2 instead of embedded blocks) as the outlier
|
|
||
| static inline generic_bitset_t *bitset_write_word(generic_bitset_t *bitset, uint word_num, uint32_t value) { | ||
| check_bitset(bitset); | ||
| if (word_num < bitset_word_size(bitset)) { |
There was a problem hiding this comment.
Should this raise an error if word_num >= bitset_word_size(bitset) ?
There was a problem hiding this comment.
This is a question for @kilograham, but given that the other functions don't throw any errors when you attempt to pass a value that is outside the range and just return 0/do nothing, I think this one should also not throw an error?
Oops can you add the failing case to the test and I’ll go fix I guess - or you can |
Add new powman_timer_pause function which sets the timer to restart from the current time, rather than the previously set time Use _pause instead of _stop in the powman functions that aren't supposed to change the time Add PICO_POWMAN_CALIBRATE_LPOSC_FROM_OTP (default 1) to use OTP frequency value for the LPOSC frequency, otherwise (or if OTP ECC read is 0) uses the reset value of the registers Only pause & start the timer when it's required due to changing the frequency in _set_1khz_tick_source_xxx_with_hz functions
pico_runtime_init now depends on hardware_rtc
Replace assertions with invalid_params_if or invalid_params_if_and_return, and fix invalid assertion that event happened when leaving dormant Prevent races on entry to dormant & pstate by defining a minimum time allowed Switch powman_timer back to XOSC after low_power_dormant_until_aon_timer
Add 4 and 77 sized bitsets
Fixed, and added tests of more bitset sizes |
| * \param exclusive Whether to only listen for the timer interrupt, or other interrupts. | ||
| * \return 0 on success, non-zero on error. | ||
| */ | ||
| int low_power_sleep_until_timer(timer_hw_t *timer, absolute_time_t until, const clock_dest_bitset_t *keep_enabled, bool exclusive); |
There was a problem hiding this comment.
I'm wondering whether we should make this use aon and do the conversion internally? It would avoid confusion with the API. We could then also add a helper, something like...
inline absolute_time_t low_power_delay_ms(uinnt32_t ms) {
return delayed_by_ms(aon_timer_get_absolute_time(), ms);
}
There was a problem hiding this comment.
I think keeping a low_power_sleep_until_timer is good, so you can do sleeps with microsecond resolution - but could add a new low_power_sleep_until_aon_timer to sleep with millisecond resolution?
That helper sounds good, and maybe also a
inline bool low_power_start_aon_timer(uint64_t ms)to handle the timespec stuff for you
There was a problem hiding this comment.
Ah and you definitely don't want to only have low_power_sleep_until_aon_timer on RP2040, as the resolution there is 1s
There was a problem hiding this comment.
ah - yes, of course. I just think mixing the types of absolute delay is going to cause confusion. Maybe the sleep function should take an absolute delay
There was a problem hiding this comment.
I could add a check whether the microseconds are 0 or not? For any times from AON timer they must be 0, and for system timers then will almost certainly be non-zero, so that check would guard the AON timer functions reasonably well.
I think I will also add low_power_sleep_for_ms (maybe low_power_sleep_for_us too), low_power_dormant_for_ms, and low_power_pstate_for_ms, so those variants will take a consistent delay
There was a problem hiding this comment.
I could add a check whether the microseconds are 0 or not?
Ah yes - good idea!
Add low_power_xx_for_ms functions, which handle starting AON timer if necessary Add low_power_set_external_clock_source function to avoid the need to have src_hz and gpio_pin in the arguments passed to low_power_dormant_until_aon_timer, and for RP2040 you just need to call low_power_set_external_clock_source beforehand (no-op on RP2350) Add DORMANT_CLOCK_SOURCE_DEFAULT and DORMANT_CLOCK_HZ_DEFAULT, so you don't need `#if PICO_RP2040` in user code
|
Latest commit changes the signature for I think this is cleaner than having unused Also adds |
|
Are you happy this is correct now? I can retest if you are. |
Also increase powman_set_power_state timeout when waiting for POWMAN_STATE_WAITING_BITS, as it was sometimes timing out
This writes the calibrated frequency for the LPOSC to improve the accuracy, and switches it back to the default XOSC when waking up
It is unreliable, see RP2040-E10
Yes, I think I'm happy it's ready for retesting now |
| * \param ms The time in milliseconds to start the AON timer at. | ||
| * \return true on success, false on failure. | ||
| */ | ||
| static inline bool low_power_start_aon_timer_at_time_ms(uint64_t ms) { |
There was a problem hiding this comment.
This resets the time if the aon timer is already running. So you'd always want to do this when using powman...
if (!aon_timer_is_running()) {
low_power_start_aon_timer_at_time_ms(1776858754000);
}
Maybe the API should have a flag - bool reset_time_if_already_running (or something) which makes it do nothing if the aon timer is already running? or maybe just make it do nothing if already running..
There was a problem hiding this comment.
I think this one should reset the time, because you're explicitly specifying a time to start at - you might want to use it once the timer is already running to synchronise it with another clock source
But maybe low_power_start_aon_timer should check if aon_timer_is_running before resetting the time
| */ | ||
| static inline bool low_power_start_aon_timer(void) { | ||
| struct timespec ts; | ||
| ms_to_timespec(to_ms_64_since_boot(get_absolute_time()), &ts); |
There was a problem hiding this comment.
Is this a sensible value to set the epoch to? It would be better to set it to 1st Jan 1970 perhaps?
| * | ||
| * \return true on success, false on failure. | ||
| */ | ||
| static inline bool low_power_start_aon_timer(void) { |
There was a problem hiding this comment.
ditto about this resetting to clock if aon is already started
| #if PICO_RP2040 | ||
| int low_power_set_external_clock_source(uint src_hz, uint gpio_pin); | ||
| #else | ||
| static inline int low_power_set_external_clock_source(__unused uint src_hz, __unused uint gpio_pin) { |
There was a problem hiding this comment.
I wonder whether we should just not have this function in RP2350
There was a problem hiding this comment.
I think this is an @kilograham question - I'm happy to make it RP2040-only, but that would mean users have to put #if RP2040 in their code
There was a problem hiding this comment.
I would call it like this...
low_power_set_external_clock_source(RTC_CLOCK_FREQ_HZ, RTC_CLOCK_SRC_GPIO_IN);
I don't think RTC_CLOCK_FREQ_HZ exists on RP2350 so it would have to be wrapped in an ifdef anyway.
There was a problem hiding this comment.
I added a DORMANT_CLOCK_HZ_DEFAULT define which you can use instead, that is defined as 0 on non-rp2040
6 participants
This adds a pico_low_power library, which allows going to sleep, dormant, and powman pstates.
Also adds bitsets and hardware_rosc, to support the low power stuff.
Includes a possible fix for #2420 to get the bazel rp2040 clang build working
Also requires raspberrypi/picotool#298 when putting persistent data in XIP SRAM
See low_power.h for documentation