Skip to content

Conversation

kfessel
Copy link
Contributor

@kfessel kfessel commented Oct 26, 2023

Contribution description

board/native: set env TZ to UTC by default
also fix the failing minema test - that assumend to be run in Berlin/Paris/Hamburg/Rom/Wien

most (all) embedded libc do not care for TZ and time functions don't do timezone conversion,
making native use UTC for time.h functions does the same

you can still force localtime timezone

  • TZ=":/etc/localtime" <elf_file> or
  • TZ=":/etc/localtime" make term or test or
  • export TZ=":/etc/localtime" or

or another timezone

  • export TZ="LEET+1:33:7" (timezone name and offset from UTC optional DST
  • see man timezone for TZ specifier (not all of it is supported by new/picolib(c)

Testing procedure

run the minema test in Berlin and see that the time stamp is now UTC

Issues/PRs references

#20018 #20005

@github-actions github-actions bot added Platform: native Platform: This PR/issue effects the native platform Area: tests Area: tests and testing framework Area: boards Area: Board ports labels Oct 26, 2023
@kfessel kfessel requested a review from maribu October 26, 2023 14:31
@kfessel
Copy link
Contributor Author

kfessel commented Oct 26, 2023

setting the enviroment is actually the suggested portable solution by gnu libc

https://www.gnu.org/software/libc/manual/html_node/Broken_002ddown-Time.html#index-timegm

@kfessel kfessel requested a review from benpicco October 26, 2023 14:41
@benpicco
Copy link
Contributor

I assume this will also affect the time returned by periph_rtc?

Then this will cause confusion because the time 'inside' native does no longer line up with the system time.
And one might want to correlate an external action (e.g. script) with a log timestamp on native when running a simulation.

Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

> rtc gettime
2023-10-26 14:53:09
$ date
Do 26. Okt 16:53:17 CEST 2023

I think this will lead to confusion.

@@ -79,5 +80,6 @@ VFS_AUTO_MOUNT(native, { .hostpath = FS_NATIVE_DIR }, VFS_DEFAULT_NVM(0), 0);
*/
void board_init(void)
{
setenv("TZ", "UTC", 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's guard this behind a pseudo-module or config option that gets selected by Makefile.tests_common

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how did you set the rtc ? (we should probably start it at 0 where most of our boards start) :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not set it at all, it's synced with the system time by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like sane defaults and the time functions are insane and even more so if timezone things are happening lets ditch then in a more sane direction by default.

if you like your localtime to much TZ=/etc/localtime <elf_file> or TZ=/etc/localtime make term or test
is an option
using mktime on native is a gamble otherwise maybe we should run a Linter that warns and errors about that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO native showing the same time as the host system is a sane default.
Why not make it a config option so we can have predictive behavior for tests that need it without forcing everyone to accept UTC as the one true time?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • cause in networking our timestamps are usually UNIX time - which is kind of UTC.
  • since RIOT is an IoT OS we should probably be as compatible as we can.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kfessel kfessel added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 26, 2023
@riot-ci
Copy link

riot-ci commented Oct 26, 2023

Murdock results

✔️ PASSED

fd11e3e board/native: add RTC at UTC documentation

Success Failures Total Runtime
10295 0 10295 09m:06s

Artifacts

@kfessel kfessel requested a review from bergzand October 27, 2023 10:16
@github-actions github-actions bot removed the Area: tests Area: tests and testing framework label Oct 30, 2023
@maribu
Copy link
Member

maribu commented Oct 30, 2023

Maybe we could have the periph_rtc start at RIOT_EPOCH on a cold boot similar to how it is on real hardware?

Not being slightly off but a lot off may look more intentional to a user and is more consistent to the behavior of real hardware.

Having the timezone of the host not leak into the native board is IMO still a good idea, not only but especially for tests.

@kfessel
Copy link
Contributor Author

kfessel commented Oct 30, 2023

#20031

@mguetschow
Copy link
Contributor

What's the status on this one? Mind looking into it before the upcoming release?

@github-actions github-actions bot removed the Platform: native Platform: This PR/issue effects the native platform label Apr 3, 2025
@kfessel
Copy link
Contributor Author

kfessel commented Apr 3, 2025

rebased to resolve merge confict

@benpicco
Copy link
Contributor

benpicco commented Apr 3, 2025

I blocked that because back then I was doing a lot of tests with native and timestamps in the logs matching the real system time really helped with debugging.

I'm not doing that now, so if you really think running native at UTC time has some kind of advantage that is greater than the confusion it will introduce for native users, go ahead.

@kfessel
Copy link
Contributor Author

kfessel commented Apr 4, 2025

Why not add an

export TZ := UTC

to the two affected test apps?

because it effects the application that use mktime and co and is not isolated to tests
the test are just showing the issue

@kfessel kfessel requested a review from jia200x as a code owner April 4, 2025 11:27
@github-actions github-actions bot added Platform: native Platform: This PR/issue effects the native platform Area: doc Area: Documentation labels Apr 4, 2025
- execute `export TZ=":/etc/localtime"` prior `make term` in the very shell you intend to run make term in
- add `export TZ=":/etc/localtime"` to your `~/.profile`
- run `TZ=":/etc/localtime" make term`
- set `TZ` in riot application picolibc and newlib are able to respect that setting if `+/-hh:mm:ss`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we just add this to pyterm so it's less user hostile?

Copy link
Contributor Author

@kfessel kfessel Apr 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i will not: i think doing things wrong (make embedded things that assume no timezone behave wrong) should not be the default.

i think mktime behaving different on native and on embedded is much more user hostile

if one wants mktime behave like it is in a timezone there are easy ways in application or environment
and i put them in the documentation

@kfessel kfessel force-pushed the p-nativ-TZ-UTC branch 4 times, most recently from ae55ecc to 933a498 Compare April 4, 2025 13:18
Comment on lines 29 to 36
- RTC: host-time but at 'UTC' to behave consistent with embedded systems if TZ is set that timezone will be used.
- set `TZ` environment variable e.g.:
- execute `export TZ=":/etc/localtime"` prior `make term` in the very shell you intend to run make term in
- add `export TZ=":/etc/localtime"` to your `~/.profile`
- run `TZ=":/etc/localtime" make term`
- to set set `TZ` in riot application as picolibc and newlib are able to respect that setting if `NAME+/-hh:mm:ss`
(-time is added, +time is subtracted from UTC (+ pointing WEST England towards America) see `man timezone`
eg.: `IST-5:30` or `TZ="NZST-12NZDT-13,M10.1.0,M3.3.0"` or `TZ="CET-1CEST-2,M3.5.0,M10.5.0"` or `TZ="ACST-9:30ACDT-10:30,M10.1.0,M4.1.0"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the detailed documentation! It's a lot though now, and I'd say it warrants a separate subsection.

@kfessel kfessel force-pushed the p-nativ-TZ-UTC branch 3 times, most recently from 55466d4 to f73e6cc Compare April 8, 2025 08:01
Copy link
Contributor

@mguetschow mguetschow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the documentation, some formulation suggestions below.

Copy link
Contributor

@mguetschow mguetschow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One last round, then I'd be satisfied :)

@kfessel kfessel force-pushed the p-nativ-TZ-UTC branch 2 times, most recently from c1ffdbb to 8f8c136 Compare April 11, 2025 08:38
Copy link
Contributor

@mguetschow mguetschow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM now, thanks!

@kfessel kfessel added this pull request to the merge queue Apr 14, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 14, 2025
@mguetschow mguetschow added this pull request to the merge queue Apr 14, 2025
Merged via the queue into RIOT-OS:master with commit 103ab62 Apr 14, 2025
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: boards Area: Board ports Area: doc Area: Documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: native Platform: This PR/issue effects the native platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants