-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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.
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
Implement alarm-module for RP2350 (follow-up for #9965) #10023
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this. I don't want to change the behavior of light sleep though.
//| Otherwise, the behavior is implementation-dependent. WiFi, BLE or other | ||
//| communications or ongoing activities such as audio playback may or may not work during or | ||
//| after light sleep. | ||
//| In some cases there may be no decrease in power consumption. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't want to change this now. If we want to, then we should do it on a major version boundary.
I'd prefer that this PR only turn peripherals off if they aren't in use, which matches the current suggested behavior. If you want to sleep with things off, then you need to turn them off using deinit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would make light-sleep more or less an alias for normal time.sleep()
and therefore useless for anybody running on batteries. The idea of this change is to provide a viable option that is much better. Current light-sleep for the Pico-W is at 60mA - nobody needs this kind of a "sleep", anybody who is using it is tricked by the naming of the api suggesting something different. Anybody who wants everything working while sleeping (which is actually a self-contradiction) should use time.sleep().
Turning things off with deinit() does not change anything for the user. The current implementation for light-sleep uses a __wfi()
that is basically a noop, i.e. the system is spinning instead of sleeping.
And the current implementation is also not up to the docs: try to light-sleep with a display using a backlight pin and you see that it even now it does not work as intended. See issue #7568.
So instead of just keeping an old, buggy and useless implementation I strongly argue to go forward and make light-sleep a useful thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Light sleep can wake up after other events besides time duration. It isn't the same.
CircuitPython should track if anything is in use and vary how it sleeps depending on the state.
I don't want code to resume in a different state than when it entered. If I start audio and then light sleep it shouldn't turn it off, even if that means it doesn't save as much power.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you have audio running, then time.sleep()
does exactly what you need - it does not save much power but everthing including audio keeps running. Perfect and wonderful! You don't loose anything by switching from fake light-sleep to normal sleep. You don't need fake light-sleep in this case. It does not save anything for you.
Aand of course you could also spin-wait on a GPIO if you want, that is basically what fake light-sleep currently does.
So there is a good alternative without drawbacks for all those users who are dreaming about power-saving while everything is working normally. Calling it "time.sleep" instead of "light_sleep_until" just makes it transparent that there is no substantial power-saving involved.
But keeping things as is does not help all the users that need low-power while retaining state. It just makes CircuitPython useless for these use-cases. With the argument: it has always been useless, so why not keep it this way?!
I am going to take the discussion out of the code comment, and probably repeat some stuff from #9521. I will take the blame for the original "light sleep" name, in #3767. I was looking at the Espressif model of light sleep, which did shut down wifi. Eventually we decided we didn't want it to do that, or shut off other peripherals: we just wanted it to sleep as innocuously as possible, without shutting down anything that was in use, but still saving some power. Sometime after that we figured out how to make I think we really do want to implement a third sleep mode that does not preserve all state but does allow pausing and continuing the current program. But it needs to be a new API with a new name. @bablokb -- I know you want a better, less power-consuming sleep now. It is our policy not to change the semantics of APIs during a minor revision. Given that you want this functionality, if you'd like to propose a new API and its semantics, that is great. It could even have a temporary name, as I can't think of a good name right now ("medium_sleep", "continuable_deep_sleep", etc. are not that great). Perhaps this is a now time to branch 9.2.x so we can start 10.x on |
I'm not arguing to keep the same implementation. I'm arguing to keep the API the same. The A separate API that turns things off will be confusing because your code won't resume in the same state it left. Do you disagree? Is your intention that code execution is reset to the start of code.py after light sleep like deep sleep? Is there anything we can't deinit from CircuitPython currently? That is something we may need to add as well. |
I would keep it simple: add an optional It would also be interesting if we actually know what we are talking about. I only know of wifi not being connected after light-sleep. But this is not fundamentally different to what you have today: if you light-sleep long enough, your router will also disconnect you so you already have to be prepared to reconnect with the current implementation. I need to do some more tests on other peripherals to gain some additional insights. |
Let's not add an additional parameter. Instead, let's vary the behavior on if the wifi is active or not since that's what you are aware of. Other peripherals that get deinit can be bugs we fix when it comes up. I know it's tricky to check all of that up front. |
To be honest, this is getting too complicated for me and I still believe it misses the real target use case of low-power systems. From an academic perspective this "keep things running that are running"-mantra might be ok, but from an economic perspective it is not. Somebody has to sit down and implement this logic, either up-front or after bug-reports as you say. And this is beyond my reach and my time. So I am leaving the stage, happy with my Pico2W running at 1.8mA instead of 60mA during light sleep. And once somebody implements the perfect solution, I will switch back to main. |
Totally understandable. Thanks for getting it this far! Hopefully we can find someone to finish this up soon. |
See #9965, which was prematurely closed. Copying notes from there. See the first PR for various measurements.
This PR implements the alarm-module for the RP2350 variant of the pico and fixes #9491.
Notes
light_sleep_until_alarms(maximize_power_save=True)
. But for real life use running on batteries, I cannot imagine that anybody really wants more than the absolute necessary stuff running during sleep.