Skip to content
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

Internal API revisions to sleep #4606

Merged
merged 5 commits into from
May 20, 2021
Merged

Internal API revisions to sleep #4606

merged 5 commits into from
May 20, 2021

Conversation

hierophect
Copy link
Collaborator

This PR suggests some changes to the internal structure of the Alarm module to fix some bugs with light sleep and hopefully make the sleep program flow easier to follow. Starting as draft so it isn't accidentally merged.

  • Changed shared_alarm_save_wake_alarm to take an alarm parameter. This means it can be called directly in Light Sleep to set the current alarm without an intermediary static variable, and it doesn't impact deep sleep in main.c since common_hal_alarm_get_wake_alarm/common_hal_alarm_create_wake_alarm (see below) can be called as the parameter. Setting the global variable to an alarm out of the current VM memory is now contained entirely within the light sleep function.
  • As per the above, common_hal_alarm_get_wake_alarm is renamed common_hal_alarm_create_wake_alarm. Now that light sleep "real" alarm management is entirely self contained, this function can be upfront about only returning generated "copy" alarms for deep sleep.
  • All x_woke_us_up functions are renamed x_woke_this_cycle, making it clear they apply only to wakeup detection within the current RAM cycle, and won't work for deep sleep.
  • _get_wake_alarm has been completely merged into common_hal_alarm_create_wake_alarm, and now has no parameters, since all current VM alarm objects are handled by light sleep. The internal functions used are of x_create_wakeup_alarm type, see below.
  • All x_get_wakeup_alarm functions have been separated into two: x_create_wakeup_alarm and x_find_triggered_alarm. The creation of alarms is used for deep sleep (real and fake), whereas finding triggered alarms is for light sleep.
  • common_hal_alarm_light_sleep_until_alarms now detects the wakeup cause itself, searches through the alarm list itself, and saves the real alarm object directly to the global array using the shared_alarm_save_wake_alarm, rather than saving an incorrect fake alarm to the global array (old ESP case) or using a static variable to hack around the existing system (old STM case). The alarm in the global array now matches the alarm returned by the function.
  • _idle_until_alarm has been factored into light sleep as it was not used anywhere else.
  • Various comments added describing program flow.

Considered but unchanged:

  • _get_wakeup_cause still handles both light and deep sleep, as deep sleep calls to common_hal_alarm_create_wake_alarm may need either fake or true deep sleep wakeup detection.
  • _setup_sleep_alarms still handles all kinds of sleep, since platforms like the STM32 have overlap in light/deep sleep setup, especially for the RTC.

@hierophect
Copy link
Collaborator Author

Old structure:
sleep diagram

New structure:
DeepSleepRev2

@hierophect hierophect mentioned this pull request Apr 15, 2021
dhalbert
dhalbert previously approved these changes May 10, 2021
Copy link
Collaborator

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

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

These changes clairfy the flow nicely - thanks! I did not test but you did. The build failures are due to GitHub cache failures. You could merge from upstream and re-run, because the cache key has already been changed.

@hierophect
Copy link
Collaborator Author

@dhalbert Thanks for the review! But let's get STM32 sleep merged in first. That way I can merge the API changes to all existing Alarm modules at once, plus the STM32 port has a minor API adjustment (makes PinAlarms use const for pin objects, like other pin-related modules) that is better to put in before this.

@hierophect hierophect marked this pull request as ready for review May 17, 2021 15:31
@hierophect hierophect requested a review from dhalbert May 17, 2021 15:32
@hierophect
Copy link
Collaborator Author

NRF isn't returning the correct global object in testing, will resolve tomorrow morning.

@hierophect
Copy link
Collaborator Author

@dhalbert nevermind, object problems were just a testing mistake. I've tested this and it works across all 4 possible sleep modes on all platforms, NRF/STM/ESP. Should be good to go.

Copy link
Collaborator

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

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

Thanks for taking this on!

@dhalbert dhalbert merged commit f5aa55c into adafruit:main May 20, 2021
@hierophect hierophect deleted the sleep-revamp branch May 20, 2021 20:01
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.

None yet

2 participants