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

esp32/time: make utime conform to CPython #5973

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

tve
Copy link
Contributor

@tve tve commented Apr 26, 2020

This PR attempts to start fixing some of the issues with utime and machine.RTC, per #5969, #5553, #5733. The overall idea is to flesh out utime so it is (a) compatible with CPython and (b) fully-featured to manage the time and (internally) query/set the RTC. The RTC.datetime method would then be deprecated but left in-place for code compatibility and possibly for minimal ports that don't have utime (I have not looked at that aspect).

What this PR currently does is in the esp32 port:

  • change utime.localtime and utime.mktime to conform to the CPython standard 9-tuple (well, actually to the 9 11-tuple elements that have numeric indexes).
  • change the Epoch to be the POSIX 1970 Epoch.
  • add utime.gmtime and time.tzset so one can set a time zone and query UTC as well as local time. The implementation of tzset does not conform to CPython in that it takes a zone specification whereas in CPython it takes no argument and the zone is set via the TZ environment variable.
  • add utime.settime and utime.adjtime as extensions WRT CPython so one can step the time or gradually adjust the time.
  • add a network.SNTP class with methods to start SNTP, stop SNTP, and get the SNTP status (this is only partially implemented in this PR and is intended to leverage the LwIP SNTP implementation).

I believe that this ends up producing a clean interface to time that can be fully implemented on the bigger ports using newlib and LwIP, and that can be stripped down, e.g. without time-zone support, for smaller ports. It somewhat side-steps the incompatibilities between RTC implementations by leaving that class along for legacy use, as well as for minimal ports without utime.

I volunteer to port this PR to the unix and stm32 ports so these are in-sync.

@tve
Copy link
Contributor Author

tve commented Apr 26, 2020

I suspect that the 2000/1/1 Epoch was chosen because it makes seconds-since-epoch fit into 31-bit signed integers...
Interesting conundrum...
Also raises the question whether MP will still be around in 2038 :-)

@dpgeorge
Copy link
Member

Thanks for taking these steps, these are good things to discuss and try to address. But because there are lots of subtle things here, additions to the API and breaking changes, they really need to be split in to separate PRs, at least for the things that are independent like settime and adjtime.

I suspect that the 2000/1/1 Epoch was chosen because it makes seconds-since-epoch fit into 31-bit signed integers...

Yes that's correct. It was engineered around the criteria that time.time() does not allocate memory.

@tve
Copy link
Contributor Author

tve commented Apr 27, 2020

I believe I can switch back to the 2000/1/1 based epoch and make SNTP work. In terms of breaking this up into multiple PRs, please tell me what you'd like broken out. Right now I don't quite see which subsets are usable and testable...

@robert-hh
Copy link
Contributor

Also raises the question whether MP will still be around in 2038 :-)

I hope it will. And I expect, that at this time micros will have an 64 bit architecture.

@tve
Copy link
Contributor Author

tve commented Apr 27, 2020

Alright, back to 2000/1/1! The changes were actually much simpler than I had thought.

I also pulled out all SNTP stuff. I wrote most of a modsntp to use the LwIP SNTP client but then decided that it's too much of a mess in the sense that the code has a slew of #ifdef and so while I can craft something for the esp32 it ends up not being portable and the client is not that awesome to begin with. I'll go with a simplified version of https://github.com/wieck/micropython-ntpclient for my own stuff...

@tve tve changed the title esp32/time: make utime conform to CPython and add SNTP esp32/time: make utime conform to CPython Apr 27, 2020
@dpgeorge
Copy link
Member

The RTC.datetime method would then be deprecated

Can you elaborate on the idea to deprecate this? The RTC is a real peripheral on most MCUs, and you need the ability to set and query it. Some systems will have an external RTC and such things can be exposed as machine.RTC-like objects.

@tve
Copy link
Contributor Author

tve commented Apr 27, 2020

The RTC.datetime method would then be deprecated

Can you elaborate on the idea to deprecate this? The RTC is a real peripheral on most MCUs, and you need the ability to set and query it. Some systems will have an external RTC and such things can be exposed as machine.RTC-like objects.

Good point. So on some systems, like the esp32, the RTC hardware just provides ticks (and moreover is managed by the RTOS). It really isn't particularly useful as machine.RTC. I believe the following 2 options make the most sense:

  1. Leave machine.RTC as-is, document that it's incompatible with other implementations, and declare it as deprecated (use utime instead). Whether it gets removed can be decided in the future.
  2. Fix machine.RTC to be compatible with other implementations causing breakage in the process.

On systems where the RTC provides YMDHMS, like stm32, I believe the best option is to leave machine.RTC as-is and document its datetime interface correctly. This means RTC.datetime and utime have overlapping functionality but with different input/output formats. A good question is what to do about daylight savings.

Setting aside the issue that there are incompatible implementations and documentation right now, it seems to me that there are two options for machine.RTC:

  1. split the module into two different interfaces: one that provides ticks and one that provides YMDHMS, which one is available on a port is determined by the HW and applications that want to be portable should use utime
  2. keep a single interface that supports ticks as well as YMDHMS; each implementation will be "native" for one side and will use conversion routines for the other.

I personally would vote for option 1.

@tve
Copy link
Contributor Author

tve commented Apr 27, 2020

As I'm implementing an sntp daemon I'm realizing there's an additional issue, which is that currently utime.time() returns an int. There is no way to get a subsecond real-time timestamp from utime. CPython has time.time() return a float. I'm going to add a utime.time_us() call for now. To be honest, I would prefer to change utime.time to return a float and I really question why utime.time has to be engineered not to allocate. For critical timing code one of the utime.ticks_xx functions should be used anyway. I'm sure I'm missing something here...

@robert-hh
Copy link
Contributor

The mantissa size of float is not sufficient in single precision ports.

@tve
Copy link
Contributor Author

tve commented Apr 27, 2020

The mantissa size of float is not sufficient in single precision ports.

Good point. So maybe adding a time_us returning a big int is a decent solution.

@tve tve mentioned this pull request May 1, 2020
@tve
Copy link
Contributor Author

tve commented May 2, 2020

I added a time.time_us() call that returns the time since the epoch in microseconds. This parallels the time.ticks_us() extension. I also modified settime to take a second microsecond parameter. I would have liked to pass in the time in microseconds but there is no way to turn that into seconds & microseconds using the public mp_obj api, this means the caller gets to do that.

The time_us() and settime with microseconds calls are necessary to implement an SNTP client, which I've done (in python) and it works well. Overall, this PR is pretty much ready from my POV, I just want to give it another round of testing. Given the request to break this up into multiple PRs I'll wait for further instructions...

@pumelo
Copy link

pumelo commented May 2, 2020

I would like to see the rtc beeing monotonic. however if it's bound to unix timestamp it's going to be non monotonic in the presence of leap seconds ... I don't think newlib is handling that correct.
See also this:
http://sourceware-org.1504.n7.nabble.com/PATCH-Change-time-t-to-64-bit-by-default-td468131.html

@tve
Copy link
Contributor Author

tve commented May 2, 2020

I would like to see the rtc beeing monotonic. however if it's bound to unix timestamp it's going to be non monotonic in the presence of leap seconds ... I don't think newlib is handling that correct.
See also this:
http://sourceware-org.1504.n7.nabble.com/PATCH-Change-time-t-to-64-bit-by-default-td468131.html

If you use adjtime it is monotonic as far as I can tell: https://github.com/espressif/esp-idf/blob/v4.0/components/newlib/time.c
The link you posted doesn't say anything about monotonicity.

@mirko
Copy link
Sponsor

mirko commented Jan 19, 2021

Just tested and providing feedback: merging this PR into latest master and/or 1.13 results in - when adjusting/setting time using e.g. NTP (ports/esp8266/modules/ntptime.py) - ending up with a negative timestamps.

@tve
Copy link
Contributor Author

tve commented Mar 3, 2021

Just tested and providing feedback: merging this PR into latest master and/or 1.13 results in - when adjusting/setting time using e.g. NTP (ports/esp8266/modules/ntptime.py) - ending up with a negative timestamps.

That's expected and happens because ntptime sets the RTC directly. With this PR the RTC is managed by esp-idf, so you need to use time.settime() to set the time, not machine.RTC.

tannewt added a commit to tannewt/circuitpython that referenced this pull request Mar 11, 2022
@projectgus
Copy link
Contributor

Heads-up that I've mentioned this PR in a discussion trying to pull together all of the different approaches to local time and timezone management, here:
https://github.com/orgs/micropython/discussions/12378

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

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

6 participants