shared/timeutils: Standardize supported date range on all platforms. #17366
Conversation
|
Code size report: |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #17366 +/- ##
=======================================
Coverage 98.56% 98.56%
=======================================
Files 169 169
Lines 21948 21968 +20
=======================================
+ Hits 21633 21653 +20
Misses 315 315 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
(Force-pushed today after rebase to head revision) |
|
Good to have the tests, and the improvements and consistency across ports. Does that now also make the tuple order and sizes across all ports the same? |
|
I wasn't even aware that some ports did not follow the documentation for the tuple content. Given how the test code is written, my guess is that if the order is not the expected one for year, month and day the test will certainly fail. The code does not check the content of time, this could be added, at least to ensure some level of consistency (tuple order). As for the extra numbers and tuple size 8/9, this was not in the scope of this PR. I was considering submitting a separate PR to provide an optional support for a simple TZ strings with DST support, so that devices can provide proper localtime support for logs for instance. But I first have to implement it :-) |
| return (mp_uint_t)(ns / 1000000000ULL); | ||
| } | ||
|
|
||
| static inline uint64_t timeutils_nanoseconds_since_epoch_to_nanoseconds_since_1970(uint64_t ns) { | ||
| static inline int64_t timeutils_nanoseconds_since_epoch_to_nanoseconds_since_1970(int64_t ns) { |
There was a problem hiding this comment.
Should the arg and return type stay as unsigned here? (I guess there's a good reason you changed it to signed.)
Any bare-metal port with an epoch at 2000 would actually need a 64 signed timestamp to properly encode time on the whole range. Internally, timeutils can still use the unsigned 32 bit representation to reduce code footprint, but the port should be using 64 bit signed timestamps when interfacing it. I will work on checking this on all platforms using |
9766d35 to
df0f466
Compare
|
I have pushed the new version, which should bring proper datetime support for most platforms. |
|
With which port did you test the PR? |
I tested on ports/unix (using int64_t timestamps) and on our bare-metal port (an ARM platform using 1970 epoch, using 32 bit timestamps), but I don't have MicroPython classic bare metals available here. This is the reason I was asking if there was any possibility of running CI test remotely on bare metal hardware. |
|
Anyone could suggest an idea to explein why the qemu_mips would be failing ? // convert a large timestamp value (stored in a mpz) to ll
mp_printf(&mp_plat_print, "%lx\n", mp_obj_get_ll(obj_timestamp));
// convert a small timestamp value (as a smallint) to a ll;
obj_timestamp = timeutils_obj_from_timestamp(0xc0ffee);
mp_printf(&mp_plat_print, "%lx\n", mp_obj_get_ll(obj_timestamp));On qemu_mips, this is returning 0 instead of the proper values. The problem is likely the implementation of int64_t res;
mp_obj_int_to_bytes_impl((mp_obj_t)arg, MP_ENDIANNESS_BIG, sizeof(res), (byte *)&res);
return res;It is very similar to edit 1 : ffi is apparently skipped on mips. But I believe I found the problem, pushing a new version... |
|
I compiled this PR for a few boards, installed them on the board and ran the test script The test passes for STM32, ESP32 and MIMXRT The fail does not necessary tell that the change is wrong. Chances are better that it is a problem of the test script. Can you provide a few test cases and the expected result, which I can feed manually to the boards at REPL? |
Thank you very much, that helps a lot. If you could try to run the script in If this test pass completely, maybe the problem is with another function. import time
print(time.time())to check if I broke something there. |
|
As suspected the problem is with running the test e.g. on RP2 with: When I run the test with Exceptions: Runtime of the test between 1.7s (Teensy 4.1) and 26.9s (Renesas-RA EK6M2). |
Excellent, thank you very much for running these all these tests. So it looks like the objective of having an uniform supported date range is met. The NRF error makes perfect sense: NRF port has very limited support for module Thank you again for you help testing this PR. |
|
It is still strange that the test looks fine when executed directly via
Edit: If I modify |
|
I did not anticipate that the test would take that long to run on bare metals. It might be a problem if it slows down the whole testing process. I have just pushed a new version that skips faster over first days of the month until the 27th. This should run significantly faster. Can you give it a try ? |
|
All boards except cc3200 pass the test now when performed using run-tests.py. The script time_mktime.py itself runs fine at the CC3200 board. So it's a test run issue. The problem seems to be in the code transport mechanism with pyboard.py. Even running the code with |
Excellent, thank you very much for your assistance ! While you have all the platforms available at hand, would you ave a chance to try to the other PR that I pushed yesterday, #17444 ? Same scenario as here: there is a new test file |
|
I deleted my comments. You have to delete yours. |
This is code makes sure that time functions work properly on a reasonable date range, on all platforms, regardless of the epoch. The suggested minimum range is 1970 to 2099. In order to reduce code footprint, code to support far away dates is only enabled when it makes sense. New types are defined to identify timestamps. Helper functions are provided to properly convert to/from mp_obj_t without risking overflows. Signed-off-by: Yoctopuce dev <[email protected]>
|
Excellent, thank you robert-hh for your help in testing this PR.
|
| @@ -314,6 +314,36 @@ mp_int_t mp_obj_get_int(mp_const_obj_t arg) { | |||
| return val; | |||
| } | |||
|
|
|||
| #if MICROPY_LONGINT_IMPL != MICROPY_LONGINT_IMPL_NONE | |||
| mp_uint_t mp_obj_get_uint(mp_const_obj_t arg) { | |||
There was a problem hiding this comment.
Please can you put this change (adding these two methods) into a separate commit? It's really quite independent to the change to timeutils.
Please also put the new coverage test for these functions into the same commit.
Also, I don't think this will build correctly when longlong is enabled, because mp_obj_int_get_uint_checked() is not implemented for longlong...
There was a problem hiding this comment.
OK, will do. Should that be a separate commit be in a separate PR as well, or a separate commit in the same PR ?
There was a problem hiding this comment.
A separate commit in this PR is fine.
| @@ -27,6 +27,46 @@ | |||
| #ifndef MICROPY_INCLUDED_LIB_TIMEUTILS_TIMEUTILS_H | |||
| #define MICROPY_INCLUDED_LIB_TIMEUTILS_TIMEUTILS_H | |||
|
|
|||
| #if MICROPY_PY_BUILTINS_FLOAT && MICROPY_FLOAT_IMPL == MICROPY_FLOAT_IMPL_DOUBLE | |||
| #include <math.h> // required for trunc() | |||
There was a problem hiding this comment.
I think this if-guard and include should come after the include of py/obj.h below, so the MICROPY_xxx configurations are available.
| // | ||
| // By default this is enabled for machines using 64 bit pointers only, | ||
| // but it can be enabled by specific ports | ||
| #ifndef MP_SUPPORT_Y2100_AND_BEYOND |
There was a problem hiding this comment.
This is a configuration setting, so should start with MICROPY_.
I think it should be called MICROPY_EPOCH_SUPPORT_Y2100_AND_BEYOND and put in py/mpconfig.h, along with MICROPY_EPOCH_IS_1970 and MICROPY_EPOCH_IS_2000.
There was a problem hiding this comment.
Makes sense. I wasn't sure how much visibility you wanted to give to that option, but I am happy to put in mpconfig.h
| #if MP_SUPPORT_Y2100_AND_BEYOND || MICROPY_EPOCH_IS_2000 | ||
| typedef int64_t platform_timestamp_t; | ||
| #else | ||
| typedef mp_uint_t platform_timestamp_t; |
There was a problem hiding this comment.
This type is used in extmod code so (due to code layering) I think it's better to define it in the core in py/mpconfig.h.
And also call it mp_platform_timestamp_t or even just mp_timestamp_t (that matches eg mp_off_t).
| #if MP_SUPPORT_Y2100_AND_BEYOND | ||
| typedef int64_t timeutils_timestamp_t; | ||
| #else | ||
| typedef mp_uint_t timeutils_timestamp_t; |
There was a problem hiding this comment.
This type is only used in timeutils so can stay named as is and defined here 👍
| return mp_obj_int_get_uint_checked(arg); | ||
| } | ||
|
|
||
| int64_t mp_obj_get_ll(mp_const_obj_t arg) { |
There was a problem hiding this comment.
I would call this mp_obj_get_int64() because that's what it is.
Or, this function should return long long type.
There was a problem hiding this comment.
I used int64_t because this is what other functions in timeutils were already using, and because I find it clear. However, from another perspective, there is an existing mpz function which is using the type long long to do the reverse operation (mp_obj_new_int_from_ll()), so it might make more sense to use long long as well.
Which option do you prefer ?
There was a problem hiding this comment.
I suggest changing he type to long long and keeping the function name.
|
Running the tests on PYBD_SF6 everything passes except time.localtime( -1 ) returned (1999, 12, 31, 23, 59, 59, 4, 365) (pass)
time.localtime( 447549467 ) returned (2014, 3, 7, 23, 17, 47, 4, 66) (pass)
time.localtime( -940984933 ) returned (1970, 3, 7, 23, 17, 47, 5, 66) (pass)
-time.localtime( -1072915199 ) returned (1966, 1, 1, 0, 0, 1, 5, 1) (pass)
-time.localtime( -1072915200 ) returned (1966, 1, 1, 0, 0, 0, 5, 1) (pass)
-time.localtime( -1072915201 ) returned (1965, 12, 31, 23, 59, 59, 4, 365) (pass)
+time.localtime( -1072915199 ) returned (2102, 2, 6, 6, 28, 17, 1, 37) expecting (1966, 1, 1, 0, 0, 1, 5, 1)
+time.localtime( -1072915200 ) returned (2102, 2, 6, 6, 28, 16, 1, 37) expecting (1966, 1, 1, 0, 0, 0, 5, 1)
+time.localtime( -1072915201 ) returned (2102, 2, 6, 6, 28, 15, 1, 37) expecting (1965, 12, 31, 23, 59, 59, 4, 365)I'm not sure what to do here? The new behaviour is probably more correct, but it's a breaking change. |
The proper solution is probably to add an extra setting |
OK, that sounds reasonable. If you can keep that test passing, that would make things simpler. |
This is pull request is to ensure that time functions (
mktime,localtimeandgmtime) work properly on a reasonable date range, on all platforms, regardless of the epoch.It does fix issue #17340
Summary
The most important point is to have a test case that checks conversion from timestamp to tuple and back for every day in the supported time range. This was currently missing. The new test code starts from 2001, first iterates backward on tuple to find the earliest supported date and then goes forward by adding 86400 to the timestamp, to verify that the date computations are correct, including leap years, and do not raise an overflow.
The second part of the pull request is to ensure that this test does actually pass on all platforms.
The most severely affected platforms were 32 bit platforms using a 1970 epoch, as
mp_int_tOn some of these platforms, the resulting supported date range was only 2001 to 2037. As 2038 is now only 13 years away, this clearly has to be fixed quickly.
Small code footprint beeing a key value for MicroPython, the suggested minimal supported time range for all platforms is 1970 to 2099. This range can be covered using 32 bit timestamps on ressource-limited platforms, fixes the 1970 epoch problems and requires less code for handling of leap years.
Support for years 2100 and beyond is only enabled for platforms using 64 bit pointers, which are typically less ressource-constrained.
Testing
The PR includes new test cases that verify the supported time range using architecture-independent Python code.
Trade-offs and Alternatives
The new
timeutilscode has a smaller footprint than the original for 32 bit machines, as it does not have to handle signed inputs and does not have to handle century-based leap years.We could have opted to use signed timestamps centered on 2000, as was probably the original intent of the timeutils code. We felt however that it was more useful to fully support the 21st century rather going supporting dates back to 1940's but breaking in 2068. Handling timestamps as unsigned also simplifies the code by removing some checks for negative values.
We could have opted to upgrade all platforms to use 64-bit signed integers for time functions, which would have offered the broadest possible range for abstract date operations, spanning from the year 1600 to the year 3000 and beyond. However, this would have had a code size impact on 32-bit platforms, with little added value. We guessed that, if a specific application on a 32-bit platform required handling exotic dates, a Python-based implementation would remain a good option.