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

nrf: Implement time.time() and machine.RTC #13340

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

cwalther
Copy link
Contributor

@cwalther cwalther commented Jan 3, 2024

Optionally adds time() and time_ns() to the time module, as well as a machine.RTC class that only implements the datetime() method (following the example of the rp2 port), whose sole purpose is to provide the ability to set the time. Also provides the basis for enabling gmtime(), localtime(), mktime() in the time module.

The nRF52 does not have a dedicated real-time clock peripheral, but timekeeping can be done by the same real-time counter that already powers the time.ticks_* and related functions. For reasonable accuracy, a suitable LFCLK source is required: The internal RC oscillator (BLUETOOTH_LFCLK_RC) by itself is insufficient, but any of the following work fine:

  • external 32kHz crystal (default)
  • synthesis from HFCLK (BLUETOOTH_LFCLK_SYNTH) when HFXO (external 32MHz crystal) is enabled
  • BLUETOOTH_LFCLK_RC + periodic calibration from HFXO (automatically done by the SoftDevice while enabled using ble.enable())

Boards can enable this by defining both configuration options MICROPY_PY_TIME_TICKS and MICROPY_PY_TIME_TIME_TIME_NS. Additionally, they may want to enable MICROPY_PY_TIME_GMTIME_LOCALTIME_MKTIME. I have not enabled it on any of the existing boards, because my main use case is an external one (and I don’t have any of them to test), but can do that if desired.

This includes a generic implementation of mp_time_localtime_get() and mp_time_time_get() in terms of mp_hal_time_ns(), which could also be used by other ports. In particular by the embed port, for which I originally wrote it in #11430 (82b8679), noting the following:

I'm unsure where to put modtime_mphal.h, it could also be in extmod. The important thing is that for MICROPY_PY_TIME_INCLUDEFILE to work it must be at the same path in both the port build (original source tree) and the application build (micropython_embed distribution), therefore not in ports/embed/port.

It is named .h, mismatching the corresponding ports/*/modtime.c, because it must not be compiled separately, which naming it .c would make harder for users of the embed port - they would need to explicitly exclude it, whereas this way they can continue to just compile all the .c files found in the micropython_embed distribution except those in lib.

Tested on nRF52832 (DS-D6) and nRF52840 (ItsyBitsy).

The first two commits are #13339, included here because separating them would create a text conflict with the third commit. The third first two commits are not strictly required, but helpful. The fourth third commit is the main change.

Copy link

codecov bot commented Jan 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.39%. Comparing base (87d821a) to head (e1a6da0).

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #13340   +/-   ##
=======================================
  Coverage   98.39%   98.39%           
=======================================
  Files         161      161           
  Lines       21200    21200           
=======================================
  Hits        20860    20860           
  Misses        340      340           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@cwalther
Copy link
Contributor Author

cwalther commented Jan 5, 2024

  • Fixed code formatting in machine_rtc.c – I was surprised that codeformat.py had not done that, so I enabled it (inserted first commit).
  • Rebased on master (no changes required).

Copy link

github-actions bot commented Jan 5, 2024

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:    +0 +0.000% standard
      stm32:    +0 +0.000% PYBV10
     mimxrt:    +0 +0.000% TEENSY40
        rp2:    +0 +0.000% RPI_PICO
       samd:    +0 +0.000% ADAFRUIT_ITSYBITSY_M4_EXPRESS

@cwalther
Copy link
Contributor Author

I just see that approximately the same thing has already been proposed in #10400. It looks like it would need some updates for things that have gone on in master in the meantime; other than that, at a first glance, I don’t have a strong opinion on which is better. It has an additional check for whether the epoch offset is valid, but I haven’t spent the thought yet to understand in what scenario that is useful.

@projectgus
Copy link
Contributor

This is an automated heads-up that we've just merged a Pull Request
that removes the STATIC macro from MicroPython's C API.

See #13763

A search suggests this PR might apply the STATIC macro to some C code. If it
does, then next time you rebase the PR (or merge from master) then you should
please replace all the STATIC keywords with static.

Although this is an automated message, feel free to @-reply to me directly if
you have any questions about this.

@cwalther
Copy link
Contributor Author

Rebased on master to deal with the STATIC removal, no other changes required.

@dpgeorge
Copy link
Member

Thanks for the contribution. As you noted above, #10400 also added machine.RTC. I will need to review the differences carefully.

In the meantime can you please rebase on the latest master, to pick up the code formatting and LFCLK fix commits that are now merged.

@cwalther
Copy link
Contributor Author

Thanks, rebased as you suggest. Not tested yet, will do that in a few days.

Board definitions could already define BLUETOOTH_LFCLK_RC or not to choose
whether to use the internal RC oscillator or an external 32kHz crystal as
the source for LFCLK, but that setting was only applied when the softdevice
was enabled by ble.enable(). Apply it from the start, so that the functions
of the time module always have the specified accuracy and run without
interruption when the softdevice is enabled.

Also add an option for the third LFCLK source, synthesized from the HFCLK,
by defining BLUETOOTH_LFCLK_SYNTH.

Signed-off-by: Christian Walther <cwalther@gmx.ch>
A board that uses BLUETOOTH_LFCLK_SYNTH is expected to have a 32MHz crystal
(Do any without one exist at all? I guess not because it's required for
Bluetooth, which nRF microcontrollers are usually chosen for.), otherwise
BLUETOOTH_LFCLK_RC would be more appropriate, so start the crystal
oscillator together with LFCLK to have accurate timekeeping from the start.

Signed-off-by: Christian Walther <cwalther@gmx.ch>
Optionally adds time() and time_ns() to the time module, as well as a
machine.RTC class that only implements the datetime() method (following the
example of the rp2 port), whose sole purpose is to provide the ability to
set the time. Also provides the basis for enabling gmtime(), localtime(),
mktime() in the time module.

The nRF52 does not have a dedicated real-time clock peripheral, but
timekeeping can be done by the same real-time counter that already powers
the time.ticks_* and related functions. For reasonable accuracy, a suitable
LFCLK source is required: The internal RC oscillator (BLUETOOTH_LFCLK_RC)
by itself is insufficient, but any of the following work fine:
- external 32kHz crystal (default)
- synthesis from HFCLK (BLUETOOTH_LFCLK_SYNTH) when HFXO (external 32MHz
  crystal) is enabled
- BLUETOOTH_LFCLK_RC + periodical calibration from HFXO (automatically done
  by the SoftDevice while enabled using ble.enable())

Boards can enable this by defining both configuration options
MICROPY_PY_TIME_TICKS and MICROPY_PY_TIME_TIME_TIME_NS. Additionally, they
may want to enable MICROPY_PY_TIME_GMTIME_LOCALTIME_MKTIME.

This includes a generic implementation of mp_time_localtime_get() and
mp_time_time_get() in terms of mp_hal_time_ns(), which could also be used
by other ports. In particular by the embed port, for which I originally
wrote it, noting the following:

"I'm unsure where to put modtime_mphal.h, it could also be in extmod. The
important thing is that for MICROPY_PY_TIME_INCLUDEFILE to work it must be
at the same path in both the port build (original source tree) and the
application build (micropython_embed distribution), therefore not in
ports/embed/port.

It is named .h, mismatching the corresponding ports/*/modtime.c, because it
must not be compiled separately, which naming it .c would make harder for
users of the embed port - they would need to explicitly exclude it, whereas
this way they can continue to just compile all the .c files found in the
micropython_embed distribution except those in lib."

Signed-off-by: Christian Walther <cwalther@gmx.ch>
@cwalther
Copy link
Contributor Author

I have added another commit in the middle, an extension of the first one (could be squashed into it): On a board that uses BLUETOOTH_LFCLK_SYNTH (none in this repository currently do and I don’t intend to change that, only my external DS-D6), start the crystal oscillator (HFXO) together with the LFCLK. This option only makes sense for boards that have a 32MHz crystal but no 32kHz one (such as the DS-D6), and on such a board one probably wants accurate timekeeping from the start, even at the cost of a bit more power use.

HFXO could be started by the user program later, but there is no Python API for doing only that, only an arcane machine.mem32 incantation. It is also started by ble.enable() because Bluetooth requires it.

I considered doing it in machine.RTC.datetime(tuple) instead, on the grounds that setting the date constitutes an expression of interest in accurate timekeeping by the user, and until then power saving is preferred, but on the other hand it could be surprising to have the accuracy of time.ticks_ms() and the like change depending on whether the RTC is set.

Also rebased on master again for good measure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants