Skip to content

ports/RTC: Attempt to reduce the inconsistencies between the port's RTC implementation. #10607

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

Merged
merged 3 commits into from
Dec 19, 2024

Conversation

robert-hh
Copy link
Contributor

@robert-hh robert-hh commented Jan 28, 2023

This PR follows the issue #10578 and #5553 and other discussions about the topic. It consists of the commits:

  • docs/quickref: Fix the documentation of rtc.datetime(), which makes the documentation of rtc.datetime() consistent to the implementation for all ports.
  • mimxrt/machine_rtc.c: Drop rtc.now() Keep it at the WiPy port for legacy,
  • ports/RTC: Rework the availability and interface of rtc.init(), which drops rtc.init() from the SAMD port and makes it consistent to the documentation and the WiPy port for the ports ESP32 and MIMXRT. But rtc.init() could as well be dropped for the latter ones and just kept for WiPy.

@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.

@dpgeorge
Copy link
Member

@robert-hh thanks for working on this, sorry I didn't take proper notice of it until now.

I think we should merge this. It fixes obvious inconsistencies and makes the docs match the code behaviour.

Eventually we would bring all ports up to the same level of functionality (eg stm32) but for now this PR fixes things in a good way.

My only comment/question is: does it make sense to remove the init() method from the samd port? Why not fix it like it's done here for the other ports?

@robert-hh
Copy link
Contributor Author

robert-hh commented Dec 17, 2024

Thanks for looking into this. I was patient since you told to defer that until v2.
About SAMD: It's hard to remember my initial thoughts. The way it was, rtc.init() was identical to setting the date&time with rtc.datetime(). The difference for other ports is the position of the hour value. I can make it like that for SAMD as well.

Looking into the documentation for RTC.init(), it allows for a tuple between 3 and 8 items. The code for ESP32, MIMXRT and then SAMD requires ATM 8 items. Should I change the code or the documentation?

Edit: rtc,.init() is missing as well at the ESP8266 port. Could be added there.

@dpgeorge
Copy link
Member

The difference for other ports is the position of the hour value. I can make it like that for SAMD as well.

If it's not much work, would be good to make samd work this way.

Looking into the documentation for RTC.init(), it allows for a tuple between 3 and 8 items. The code for ESP32, MIMXRT and then SAMD requires ATM 8 items. Should I change the code or the documentation?

Supporting a variable number of arguments won't be easy, because the ports need to set all values at once. So I suggest to change the documentation, to make 8 arguments required. A variable number of arguments never worked anyway, so this doesn't break existing code.

@robert-hh
Copy link
Contributor Author

Updated. I noticed that ESP8266 does not have the rtc.init() method. I could add it, if it's worth the code size increase.

Copy link

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:  -128 -0.035% TEENSY40
        rp2:    +0 +0.000% RPI_PICO_W
       samd:   +20 +0.007% ADAFRUIT_ITSYBITSY_M4_EXPRESS
  qemu rv32:    +0 +0.000% VIRT_RV32

@dpgeorge
Copy link
Member

I noticed that ESP8266 does not have the rtc.init() method. I could add it, if it's worth the code size increase.

I don't think it's worth it, so let's leave that out.

Such that it matches the implementation and the documentation of the
`machine.RTC` class.

Signed-off-by: robert-hh <robert@hammelrath.com>
This is not part of the common machine API.  It's dropped on the mimxrt
port and kept only on the cc3200 port for legacy.

Also show the port availability of `RTC.now()` in the documentation.

Signed-off-by: robert-hh <robert@hammelrath.com>
This commit makes the argument ordering of `machine.RTC.init()` the same
for all the ports that implement arguments to this method: cc3200, esp32,
mimxrt and samd.  The cc3200 argument ordering is used, which matches the
documentation.

Also document the availability and the differing semantics for the stm32
and renesas-ra port.

Signed-off-by: robert-hh <robert@hammelrath.com>
@dpgeorge dpgeorge merged commit f4e4599 into micropython:master Dec 19, 2024
12 checks passed
@dpgeorge
Copy link
Member

Thanks for updating, now merged.

@robert-hh
Copy link
Contributor Author

Thank you.

@robert-hh robert-hh deleted the ports_rtc branch December 20, 2024 08:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs ports Relates to multiple ports, or a new/proposed port
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants