rp2/machine_rtc: add initial support for RTC.

[kadamski]

Initial support for machine.RTC on rp2 port. It only supports datetime()
method and nothing else. The method gets/returns a tuple of 8 items,
just like esp32 port, for example, but the usec parameter is ignored as
the RP2 RTC only works up to seconds precision.

Signed-off-by: Krzysztof Adamski k@japko.eu

[kadamski]

What exception would be best here? maybe mp_raise_OSError(MP_EIO) ?

[kadamski]

BTW, does anyone have any idea how to make the commit message check happy? I did change the commit message already but it seems to be always checking the original one :/

[aivarannamaa]

The build provided by @kadamski worked fine in Thonny -- it was now able to set RTC with the same time syncing code as used for ESP32.

[kevindawson]

@dpgeorge this looks like it conforms to your Git commit conventions but still fails - has something changed?

[tjvr]

@kevindawson The regex used for the commit message check seems to be here. I think the problem in this case is that the message after the colon doesn’t start with a capital letter.

[kevindawson]

@kadamski

any idea how to make the commit message check happy?

error: commit 2bad8b5: Subject line should contain ': ' and end in '.': rp2/machine_rtc: add initial support for RTC.

subject_line_format = r"^[^!]+: [A-Z]+.+ .+\.$" thanks @tjvr

∴"rp2/machine_rtc: Add initial support for RTC."

ps. I'm looking forward to having a hack around with this :))

[kadamski]

@kevindawson rebased the patch and made the first letter of the commit message uppercase.

[kevindawson]

@tjvr who do I request to perform the approve running workflow
thanks

[kevindawson]

@dpgeorge would you please add this to your task list
many thanks in adv.

[dpgeorge]

Thanks for this, definitely something that's needed!

I think it makes sense to have the arguments to datetime() match those of CPython's datetime module. Eg:

>>> import datetime
>>> datetime.datetime.now()
datetime.datetime(2021, 6, 11, 18, 34, 51, 87566)

So that's: (year, month, day, hour, minute, second, microsecond).

See also #7318.

[kadamski]

@dpgeorge I aggree it would be good to fix the datetime to use the same API as CPython. I'm not sure, however, how to do this correctly as, on the other hand, it wouldn't be good to have different implementation on different ports, right?

See also PR 5553 which is related.

[dpgeorge]

I'm not sure, however, how to do this correctly as, on the other hand, it wouldn't be good to have different implementation on different ports, right?

Right. But at the moment the existing ports are not consistent among themselves (as #5553 explains). And I think it's fair enough to add the method here on the rp2 port with the new desired API, because there is no existing code written specifically for rp2 that uses datetime, so it's a fresh start.

I know it's not the best situation, but to make progress towards a consistent API that matches CPython's datetime, I think making this step on rp2 is good progress. Put differently, I think it'd just make things worse to not match CPython's datetime here.

[kadamski]

here is no existing code written specifically for rp2 that uses datetime, so it's a fresh start

Well, that is not entirely true. Since this is part of machine API, basically any code that uses machine.RTC from some other port, will have to adapt, if we change it for rp2. The constructor of the datetime and the actual value represented by this object, was, so far, consistent between ports as far as I know and that would break this consistency and the users would have to handle that situation on their own.

The elegant solution would be to create a new API that would be consistent while keeping the old one for legacy reasons, at least for some time. Something like a machine.RTC2. Or maybe it is time to consider something more general and better than the machine itself? Something like a device module that would have unified API that is not reconstructed by each port individually but would just call some callbacks internally that requests the data in a machine specific way but the actual API itself has just one implementation? This is something a circuitpython did well - the ports do not provide full python objects, like machine.RTC() but they provide some functions called common_hal_XXX instead to do the actual operations (like reading the datetime, setting the datetime in case of RTC). This is more limiting than each port implementing the full object but on the other hand it is much more userfriendly and micropython did grow large enough for this to pose large problem already.

Note that implementing things this way (with HAL), not only makes the API consistent between ports but also reduces the boilerplate code that has to be written for each and every port and makes maintaining easier thanks to less duplicated code.

So, what is your end verdict? Do we merge this PR as is to keep the API more consistent between ports and fix the incompatibility with CPython as a separate topic or should I change my PR?

[dpgeorge]

Well, that is not entirely true. Since this is part of machine API, basically any code that uses machine.RTC from some other port, will have to adapt, if we change it for rp2.

Yes that is true.

The constructor of the datetime and the actual value represented by this object, was, so far, consistent between ports as far as I know and that would break this consistency and the users would have to handle that situation on their own.

I had a look and stm32, esp32, esp8266 are all (almost) consistent with the datetime() method: it's (year, month, day, day-of-week, hour, minute, second, subsecond). The only inconsistency is that esp8266 uses millisecond for subsecond, while the others use microsecond.

cc3200 is different, it matches CPython, but it doesn't have a datetime() method, only the constructor and init().

mimxrt has datetime() and it matches CPython (but it was added very recently so it's not too late to change it...).

The elegant solution would be to create a new API that would be consistent while keeping the old one for legacy reasons, at least for some time.

That's a big step. It might be possible to just make datetime() take either a 7-tuple or 8-tuple, and the 7-tuple case would match CPython (but the return value would always be an 8-tuple...).

Something like a device module that would have unified API that is not reconstructed by each port individually but would just call some callbacks internally that requests the data in a machine specific way but the actual API itself has just one implementation? This is something a circuitpython did well

Yes I agree CircuitPython did this well. And we are slowly moving in that direction, eg extmod/machine_spi.c and extmod/machine_i2c.c do this. And I don't think it needs a new API to do this, it just needs a lot of refactoring of the implementation (the API can stay the same, for the most part).

So, what is your end verdict? Do we merge this PR as is to keep the API more consistent between ports and fix the incompatibility with CPython as a separate topic or should I change my PR?

Ok, the options would be:

1. Merge this as-is, and change mimxrt so it matches. Then all ports are almost consistent (cc3200 doesn't have datetime but it can be added, and esp8266 would have milliseconds instead of microseconds). Also merge docs/library/machine.RTC.rst Fix example code, document datetime method. #5553 to fix the docs.
2. Change this to match CPython datetime. Then any library that wants to be compatible with rp2 (and mimxrt) will need to detect which version of the tuple is needed. Eventually other ports will follow with a change.

It feels like option (1) is the most sensible, to not break existing code.

[kadamski]

That's a big step. It might be possible to just make datetime() take either a 7-tuple or 8-tuple, and the 7-tuple case would match CPython (but the return value would always be an 8-tuple...).

That is true, the constructor could work in both cases but the return value is the problem here.

Yes I agree CircuitPython did this well. And we are slowly moving in that direction, eg extmod/machine_spi.c and extmod/machine_i2c.c do this.

Indeed, the way machine_spi.c is done is (I think) what I was talking about! If I would volunteer to do a PR for creating something like extmod/machine_rtc.c (based on the techniques used in extmod/machine_spi.c) and implement this for the ports I could test it on, would you be interested and find time to review this? I would prefer to do this in a backwards compatible way, however, by providing this unified module alongside the existing machine_rtc implementations, to be honest. But if you think it is ok to break backwards compatibility, I can do it this way too.

And I don't think it needs a new API to do this, it just needs a lot of refactoring of the implementation (the API can stay the same, for the most part).

Well, the new API would only be needed if we wanted to keep backwards compatibility which, you know, is a nice thing to have, even if we're talking Python here :)

It feels like option (1) is the most sensible, to not break existing code.

I agree.

[dpgeorge]

Ok, let's go for option (1).

If I would volunteer to do a PR for creating something like extmod/machine_rtc.c (based on the techniques used in extmod/machine_spi.c) and implement this for the ports I could test it on, would you be interested and find time to review this?

Yes, that would be great. Note that SPI/I2C have a dynamic protocol table because they are used for both software and hardware implementations. That's probably not going to be needed for RTC because it only has one implementation. See also extmod/utime_mphal.c for a pattern of using static mp_hal_xxx functions provided by a port.

I would prefer to do this in a backwards compatible way, however

Yes that would be necessary (backwards compatible). What I was thinking would be to provide a generic hook macro in the locals dict for a class/module so the port can add arbitrary method/functions (in an ideal situation that would not contain anything, so all ports are the same, but to start with it's a way to unify implementations).

[dpgeorge]

See #7383 for a PR to change the mimxrt port to the same datetime tuple as is used here.

[dpgeorge]

Rebased and merged in 37d01d4

Thanks @kadamski for a very clean PR/commit, with not too much new functionality at once, and a good commit message.

[aivarannamaa]

I'm confused now... I installed rp2-pico-20210613-unstable-v1.15-211-g3ab8806c0.uf2 and experimented according to https://github.com/micropython/micropython/pull/5553/files:

MicroPython v1.15 on 2021-06-13; Raspberry Pi Pico with RP2040
Type "help()" for more information.
>>> import machine
>>> rtc = machine.RTC()
>>> rtc.datetime((2021, 6, 13, 7, 10, 13, 0, 0))
>>> rtc.datetime()
(2021, 1, 1, 5, 0, 7, 42, 0)
>>> import time
>>> time.localtime()
(2021, 1, 1, 0, 7, 49, 4, 1)
>>> 

I expected at least year, month and day to match the assigned values.

[aivarannamaa]

It looks like there is a confusion regarding numbering the weekday. When I'm giving 6 for Sunday, then Pico's RTC.datetime kind of works, but the weekday is decremented in time.localtime():

>>> import machine
>>> rtc = machine.RTC()
>>> rtc.datetime((2021, 6, 13, 6, 10, 40, 0,0))
>>> rtc.datetime()
(2021, 6, 13, 6, 10, 40, 4, 0)
>>> import time
>>> time.localtime()
(2021, 6, 13, 10, 40, 20, 5, 164)

Previously I thought that weekday argument to RTC.datetime had to be 1..7 in ESP32. Now I see that it's even more confusing -- 7 and 6 give the same result:

Specifying sunday as 7 for ESP32 (v1.14 on 2021-02-02)

>>> import machine
>>> rtc = machine.RTC()
>>> rtc.datetime((2021, 6, 13, 7, 10, 44, 0, 0))
>>> rtc.datetime()
(2021, 6, 13, 6, 10, 44, 4, 158853)
>>> import time
>>> time.localtime()
(2021, 6, 13, 10, 44, 19, 6, 164)
>>> 

specifying Sunday as 6:

>>> import machine
>>> rtc = machine.RTC()
>>> rtc.datetime((2021, 6, 13, 6, 10, 46, 0, 0))
>>> rtc.datetime()
(2021, 6, 13, 6, 10, 46, 3, 358834)
>>> import time
>>> time.localtime()
(2021, 6, 13, 10, 46, 10, 6, 164)
>>> 

[kadamski]

rtc.datetime((2021, 6, 13, 7, 10, 13, 0, 0))
rtc.datetime()
(2021, 1, 1, 5, 0, 7, 42, 0)

According to pico-sdk docs (https://raspberrypi.github.io/pico-sdk-doxygen/structdatetime__t.html), the weekday is 0-6 where 0 is Sunday. That is quite confusing, as you have to start counting weekdays at 0, while the months and days of a month are counted starting from 1. Then, there is this problem of the first day in week - according to ISO 8601, it is Monday, but US, Canada, Australia (and probably some others) recognize Sunday as the first day of a week, so does Pico.

All in all, I think this weekday is nothing but confusing and we will benefit from getting rid of it, as was explained here. What I can do as a quick fix for your problem is this:

>>> rtc.datetime((2021, 6, 13, 7, 10, 13, 0, 0))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
OSError: [Errno 22] EINVAL
>>>

[aivarannamaa]

Giving error for weekday 7 is good idea! In addition to this, it should be documented with RTC.

Also please don't forget the discrepancy between RTC.datetime() and time.localtime()!

[kadamski]

@aivarannamaa thanks for your raport, it reveals more problems with RTC than I originally seen. We have focused on making sure the order of arguments in the tuple we give to RTC.datetime() is the same on all machines but we didn't make sure the meaning of those fields is also the same! I will try to check how that works on other ports this evening and may provide some more patches, maybe that is only rp2 specific problem and other implementations are all the same..

As for the RTC.datetime() vs time.localtime() discrepancy it is a good idea to open a new issue for that. I am not sure we we should always sync the RTC and localtime or not so lets discuss this there. Will open the issue or should I?

Note that, according to my initial check, the ESP32 implementation simply ignores the dayw argument when the datetime() is called with a tuple, which would explain why using 6 and 7 gives the same results.

[aivarannamaa]

As for the RTC.datetime() vs time.localtime() discrepancy it is a good idea to open a new issue for that. I am not sure we we should always sync the RTC and localtime or not so lets discuss this there. Will open the issue or should I?

Please open it yourself, then you can already propose your solution ideas!

[robert-hh]

Note that, according to my initial check, the ESP32 implementation simply ignores the dayw argument when the datetime() is called with a tuple, which would explain why using 6 and 7 gives the same results.

The implementation for mimxrt also ignores the weekday value on input. It is calculated on output from the date.