-
Notifications
You must be signed in to change notification settings - Fork 2k
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
sys/phydat: add functions for Unix time conversion to phydat #19080
Conversation
sys/include/phydat.h
Outdated
int64_t phydat_unix(int16_t year, int16_t month, int16_t day, | ||
int16_t hour, int16_t minute, int16_t second, | ||
int32_t offset); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this just a re-implementation of mktime()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, with a few differences:
- The time offset is explicit instead of implicit.
- It does not have a DST flag.
- It uses
int64_t
instead oftime_t
(int
) to avoid Y2K38.
I couldn't find any 64-bit mktime
implementation available in RIOT-OS, but please let me know if there is one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It uses
int64_t
instead oftime_t
(int) to avoid Y2K38.
Are you sure time_t
is only 32 bit? For
printf("time_t is %u bit\n", sizeof(time_t) * 8);
I get
time_t is 64 bit
on both newlib and picolibc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure time_t is only 32 bit?
No, this depends on the platform. The ARM-based boards I tested use 64-bit time. ESP32, ATmega328 and native, however, give me time_t is 32 bit
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well fine then, if it's useful to you then let's go ahead.
Just two small nits, squash directly.
bors merge |
19080: sys/phydat: add functions for Unix time conversion to phydat r=benpicco a=silkeh Co-authored-by: Silke Hofstra <[email protected]>
Build failed: |
tests/phydat_unix/Makefile.ci
Outdated
@@ -0,0 +1 @@ | |||
BOARD_INSUFFICIENT_MEMORY := |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can create this list by running
make create-Makefile.ci
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there documentation for this somewhere? It didn't work for me in the form you suggested (I had to add a BOARD=
) or using BUILD_IN_DOCKER=1
. It seemed to work by manually running things in the Docker image, however, so I hope it's correct.
Add functionality for calculating Unix timestamps to phydat. This allows one to convert dates received from RTCs and the like to Unix timestamps.
20282eb
to
26653dd
Compare
bors merge |
Build succeeded: |
Contribution description
Add functionality for calculating Unix timestamps to phydat. This allows one to convert dates received from RTCs and the like to Unix timestamps.
This was originally included in #16384, in which it was decided to move it to a separate PR. See the discussion here.
Testing procedure
Tests are included.
Issues/PRs references
None.