8000 sleep and wakeup for nrf52 by jun2sak · Pull Request #4236 · adafruit/circuitpython · GitHub
[go: up one dir, main page]
Skip to content

sleep and wakeup for nrf52 #4236

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 56 commits into from
Apr 28, 2021
Merged

sleep and wakeup for nrf52 #4236

merged 56 commits into from
Apr 28, 2021

Conversation

jun2sak
Copy link
@jun2sak jun2sak commented Feb 21, 2021 •
edited by dhalbert
Loading

I have implemented alarm (deep and light sleep) for nrf52.
PinAlarm, TimeAlarm and SleepMemory work on my nRF52840 board. My code is based on esp32s2 alarm implementation, with HW specific parts re-implemented.
Set CIRCUITPY_ALARM = 1 in config.mk file to enable the alarm function (already defined in mpconfigport.mk).
Please refer to README_impl.txt below.

I am not sure whether this works on other typical nRF52 boards,
and the source files contain some debugging codes.
Please let me know how to go on with this PR.

README_impl.txt
[added in place here for easy reading --dhalbert]:

alarm (sleep and wakeup) implementation for nRF52 [Feb 21, 2021]

implemented:

  • PinAlarm (GPIO event), TimeAlarm (RTC)
  • deep sleep (=System OFF, all RAM retention),
    light sleep (=System ON, WFI)
  • SleepMemory (256 bytes on uninitialized RAM area)
  • add CIRCUITPY_ALARM = 1 in config.mk to enable the above.

restriction:

  • only PinAlarm can be used for wakeup from deep sleep.
    (RTC stops while System OFF)
  • up to 2 PinAlarms can be used for light sleep.
    (can be changed by NRFX_GPIOTE_CONFIG_NUM_OF_LOW_POWER_EVENTS
    in nrfx_config.h)
  • "pretending to deep sleep" (deep sleep while USB connected)
    does not work properly.

todo:

  • TimeAlarm for deep sleep (I am not sure if it is possible)
  • TouchAlarm

debugging functions:

  • Implemented dbg_printf() which sends messages to dedicated UART-Tx.
    (ports/nrf/supervisor/port.c)
    It is useful to debug sleep & wakeup while USB connection is off,
    but it is not so smart implementation. Will be removed.
  • There are some debug dump functions as well (showing HW regs, etc).
  • add MY_DEBUGUART = 1 in config.mk to enable the above functions.

@tannewt
Copy link
Member
tannewt commented Feb 22, 2021

@dhalbert Mind doing this review?

@dhalbert dhalbert marked this pull request as draft February 23, 2021 00:23
@dhalbert
Copy link
Collaborator

I converted this to a draft for now.

@jun2sak Should I wait for you to do further work, or are you requesting some feedback on the work so far? Thanks.

@jun2sak
Copy link
Author
jun2sak commented Feb 23, 2021

@dhalbert I have done most of my implementation, but I would like to hear from you to decide further work.

(1)
Checking nRF SDK info and nrf touchin discussion in CY github, I understand nRF processor cannot be woken up by touch (neither light sleep nor deep sleep).
Should we simply ignore TouchAlarm operations (current implementation), or should we raise an exception ?

(2)
I have moved all the debugging codes to a new file debug_uart.c under ports/nrf/supervisor/, and these codes are enabled only when MY_DEBUGUART=1. They would be useful in future debugging.
Is it Ok to leave these code in the repository ?

@tannewt tannewt requested a review from dhalbert February 23, 2021 17:59
dhalbert
dhalbert requested changes Feb 23, 2021
View reviewed changes
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 working on this. I left a bunch of comments, but some would change as you clean up the draft. I will reply to other things in another comment that quotes from your comments.

@dhalbert
Copy link
Collaborator
dhalbert commented Feb 23, 2021 •
edited
Loading

TimeAlarm for deep sleep (I am not sure if it is possible)

This is quite possible if you just use System ON. The power consumption can still be negligible. You don't have to use System OFF if it is inconvenient. See the chart below from the datasheet. System ON with wake from RTC can be about 1.5 - 3uA, which is still wonderfully low. SYSTEM OFF gets it below 1 uA in certain circumstances. You could choose which to do based on which alarms are set. But to start you don't have to differentiate and could just use System ON for now.

image

"pretending to deep sleep" (deep sleep while USB connected)
does not work properly.

This is something we really want to have. What were the stumbling blocks?

Should we simply ignore TouchAlarm operations (current implementation), or should we raise an exception ?

The constructor should raise a NotImplementedError. Alternatively alarm.touch could just not exist, and won't be importable. We'd need to add CIRCUITPY_ALARM_TOUCH as a separate flag to enable/disable this. It would remove the dictionary entry in alarm/__init__.c.

I have moved all the debugging codes to a new file debug_uart.c under ports/nrf/supervisor/, and these codes are enabled only > when MY_DEBUGUART=1. They would be useful in future debugging.
Is it Ok to leave these code in the repository ?

This looks useful and could be quite handy for other debugging. But could you clean it up a bit and rename it to something like NRF_DEBUG_UART?. Also could you remove debugging stuff from the rest of code after it's working?

@jun2sak
Copy link
Author
jun2sak commented Feb 24, 2021

@dhalbert Thank you for your comments. I will make another push in a couple of days.

@hierophect
Copy link
Collaborator

@jun2sak I've pushed fixes to the CI issues, just some simple pre-commit formatting problems. Is this working now, or does it need further fixes? I will try and load it up on my NRF52, if I can find where my dev board is buried...

hierophect
hierophect previously approved these changes Apr 21, 2021
View reviewed changes
Copy link
Collaborator
@hierophect hierophect left a comment

Choose a reason for hiding this comment

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

Tested this on the Feather NRF52840 across real/fake light/deep sleep types, and it works well across all modes. The only functional issue it has is not retaining a system tick across deep sleep, which causes the USB enumeration delay to occur every time it tries to deep sleep, but that's only a minor power loss with longer sleep periods and likely solvable with a follow up issue.

There are also a couple of stylistic things that could still be changed - common_hal_alarm_pretending_deep_sleep can be simplified since it doesn't need to detect non-alarm wakeups, there are blocks of commented-out debugging stuff left over, and the debugging content in serial.h should probably be made generic and more consistent with other ports like STM. But at this point we've got a lot of new Alarm content backed up behind this PR and I think it's probably best if we merge it now and do a bit of sweeping later, potentially when merging #4606.

@hierophect
Copy link
Collaborator

Note that this required a few final edits, including some formatting changes and a couple of module exclusions to make the new code fit on existing small NRF boards like the Simmel. I've also reverted the changes to main - the existing sleep loop is already capable of detecting a fake sleep wakeup, and changing it to an on-the-spot return breaks the ESP32-S2 and STM32 builds of Alarm.

@jun2sak
Copy link
Author
jun2sak commented Apr 21, 2021

@hierophect I have a couple of things to be fixed. I will merge your code and commit with my code in this weekend.

@jun2sak
Copy link
Author
jun2sak commented Apr 25, 2021

@hierophect After the above commit, I got the message saying I dismissed your review. I am sorry if I broke any rule here. But I think I have successfully merged your changes.
I would like to modify around wakeup-cause detection functions to make the code straightforward. Can I go on and commit them ?

@dhalbert
Copy link
Collaborator

@jun2sak No problem, please go ahead and make further changes. Reviews get dismissed automatically when code changes are pushed. It's nothing wrong. We just re-review the new changes.

@jun2sak
Copy link
Author
jun2sak commented Apr 25, 2021

I have committed what I have been working for these days -- merging some latest changes and simplifying system_on_idle_until_alarm(). I found it work on my nRF52840 board, but I understand following items are left undone.

  • deep sleep and USB enumeration delay
  • adjust wake_alarm implementation on nRF to those on esp32s2

@hierophect
Copy link
Collaborator

@jun2sak thanks for cleaning up the internals! Looks good to me. There's just a little bit of formatting stuff to take care of, I can just pull and do it real quick today though. For the future, you can look into using pre-commit, Adafruit has a guide here on how to install and use it (you might also need uncrustify, which you can get with apt, homebrew, etc).

I'll give this another test. Regarding the problems:

  1. The USB enumeration delay has to do with timekeeping. On most ports, there is either a timer or an RTC that will keep incrementing even when the system enters deep sleep. Thus, when the system wakes from deep sleep, it looks at that monotonic time and sees that it is already past the USB enumeration delay period (which should only happen right after startup). Is there a suitable time source we could use for this on NRF?
  2. Since I've reverted the detection changes to Main, the ESP32-S2 should no longer have any incompatibility. Basically, what you're doing on NRF is handling both the idling process and the detection of keyboard and restart interrupts in common_hal_alarm_pretending_deep_sleep. But you don't really need to do this, because main.c will automatically do it for you: the loop in run_code_py uses common_hal_alarm_woken_from_sleep to determine if an interrupt was an alarm or not and sorts them into keyboard interrupts, reload requests and alarms restarts. I removed the custom sorting code in main.c, so now when common_hal_alarm_pretending_deep_sleep exits, it will enter the usual sorting system. So there's no more work to be done, unless you'd like to simplify common_hal_alarm_pretending_deep_sleep even more.

@microdev1
Copy link
Collaborator

I believe some review comments like this one require attention before this PR is merged.
They got hidden due to large number of comments.

@dhalbert dhalbert mentioned this pull request Apr 28, 2021
Closed
@dhalbert dhalbert removed their request for review April 28, 2021 16:10
@dhalbert dhalbert merged commit d4d96bb into adafruit:main Apr 28, 2021
@hierophect
Copy link
Collaborator

@jun2sak we've opted to merge this to unblock the other sleep PRs. Follow up cleanup work is being tracked in #4680

@hierophect hierophect mentioned this pull request Apr 28, 2021
Merged
@tannewt tannewt mentioned this pull request Aug 27, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tannewt tannewt tannewt left review comments

@dhalbert dhalbert dhalbert left review comments

@microdev1 microdev1 microdev1 left review comments

@hierophect hierophect hierophect approved these changes

Assignees
No one assigned
Labels
nordic
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jun2sak @tannewt @dhalbert @nitz @hierophect @microdev1
0