Skip to content

Conversation

@pabigot
Copy link
Contributor

@pabigot pabigot commented Jun 22, 2019

Commits in this PR are a first step towards providing support in Zephyr to register system time (e.g. milliseconds since startup) against civil time (UTC, TAI, etc.), which is necessary for things like Bluetooth mesh scheduled operations.

The first commit provides the minimal libc implementation in Zephyr with the same time-related types that are present in POSIX and newlib in all other toolchains.

The second commit uses conversion algorithms to translate between struct tm and time_t representations.

Once this functionality is in place it should be possible to use external time sources such as NTP or Bluetooth Mesh Time Service to get current time onto a Zephyr device, then translate between system time and real time, including (in the future) dealing with leap seconds, local time offsets, and daylight saving time.

I anticipate adding new types to provide TAI-UTC offset, local time offset, and DST change information in a future PR.

A proactive comment about two style issues that may raise questions:

  • The type definitions in the minimal libc use C standard sized types (int64_t) instead of the Zephyr alias (s64_t). Here I am following existing practice in libc/minimal.
  • The clock implementation code closely follows the original algorithms, which were written for C++, including by declaring the variables at the point where their initial value is available. This feature was added to C twenty years ago, but I understand mid-block definitions to be undesirable in Zephyr. If the declarations need to be bunched at the top of the block I'll rework this.

Also I'm not entirely happy about the creating of a new lib subdirectory, but I tried to use misc and failed to convince CMake to pay attention to me. I don't see any other location to put cross-platform utility code; if this belongs somewhere else please let me know.

Finally, I would very much like to explicitly identify the algorithm implementation as being a translation of the public-domain material by Howard Hinnant, but I don't know how to express that without breaking the format of the license block. I've at least been able to put the link to the original in comments near the functions.

Provide POSIX-compatible definitions for a subset of the time types that
must be provided by this file, in anticipation of supporting civil time
in Zephyr.

Signed-off-by: Peter A. Bigot <[email protected]>
@pabigot pabigot added the area: API Changes to public APIs label Jun 22, 2019
@zephyrbot zephyrbot added area: C Library C Standard Library area: Tests Issues related to a particular existing or missing test labels Jun 23, 2019
@zephyrbot
Copy link

zephyrbot commented Jun 23, 2019

All checks are passing now.

Review history of this comment for details about previous failed status.
Note that some checks might have not completed yet.

In anticipation of the need to support civil time (year/month/day) and
to align system time with civil time add functions that convert between
time_t and struct tm representations.

Signed-off-by: Peter A. Bigot <[email protected]>
@carlescufi
Copy link
Member

The proposed solution seems to be much more simple than the standard POSIX combination:

gmtime(): UNIX time to broken-down time
mktime() or localtime(): broken-down time to UNIX time, requires dealing with timezones

Instead, what if we implemented:

gmtime(): POSIX compliant, instead of time_civil_from_unix()
timegm(): GNU extensions to POSIX, instead of time_unix_from_civil()

https://pubs.opengroup.org/onlinepubs/009695399/functions/gmtime.html
http://man7.org/linux/man-pages/man3/timegm.3.html

@pabigot
Copy link
Contributor Author

pabigot commented Jun 25, 2019

I've verified that gmtime() produces the same answer as time_civil_from_unix for the test values, so that can be a substitute.

timegm() may be a problem as a GNU extension. Will all development environments support it (clang, commercial compilers)? On host Linux you have to specifically enable it by defining one of _DEFAULT_SOURCE, _BSD_SOURCE, or _SVID_SOURCE. It's also not available with the flags currently provided in Zephyr builds (using SDK-0.10.1 CONFIG_NEWLIB_LIBC=y for particle_xenon).

counter_cmos implements the same algorithm as time_days_from_civil in an internal hinnant function, so that API might be independently desired. However, if timegm() worked it should just be a matter of dividing the result by 86400.

If a way to use timegm() can be found I don't object to withdrawing this and using those APIs instead.

A motivation to stick with custom ones would be the fact that time_t is generally s64_t, and it might be worth providing a solution based on u32_t for Zephyr, allowing dates from 1970-01-01T00:00:00 to 2106-02-07T06:28:15 without pulling in support for 64-bit arithmetic.

@carlescufi
Copy link
Member

Actually @pabigot my proposal was that you simply rename those but keep the implementation for minimal libc. I wasn't really suggesting you withdraw the PR, just to use more standard names.

@pabigot
Copy link
Contributor Author

pabigot commented Jun 26, 2019

@carlescufi I can certainly provide gmtime() for minimal libc with this PR.

I'm less happy about the reverse function. timegm() is not a POSIX API, and it's not available in Zephyr builds without changing the flags in a way that would probably expose other GNU-specific API that really shouldn't be exposed.

Zephyr doesn't seem to have a place for completely generic utility code that requires an implementation file (not just a header) and isn't associated with any subsystem. Until there's a solution to that, I think the best path forward is adding gmtime() to minimal libc, and making the inverse function local to the driver.

That'd be two drivers with the hinnant capability. When there are three we might want to revisit the problem.

Copy link
Contributor

@pfalcon pfalcon left a comment

Choose a reason for hiding this comment

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

This conflicts with existing PR on elaborating POSIX compatibility with both minlibc and newlib.

typedef int64_t time_t;
typedef int32_t suseconds_t;

struct timespec {
Copy link
Contributor

Choose a reason for hiding this comment

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

This conflicts with #16626 , which takes care about minlibc vs newlib compatibility.

* @return the signed number of days between 1970-01-01 and the
* specified day. Negative days are before 1970-01-01.
*/
s64_t time_days_from_civil(s64_t y,
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use more descriptive names than "y", etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The variable names match the reference implementation from which this was translated.

Copy link
Contributor

Choose a reason for hiding this comment

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

reference implementation from which this was translated.

Then it should have licensing and copyright of it, in addition to yours? But even then, as long as you modify it at all, you can use more descriptive names.

z += 719468;

s64_t era = ((z >= 0) ? z : (z - 146096)) / 146097;
unsigned int doe = (z - era * (s64_t)146097);
Copy link
Contributor

Choose a reason for hiding this comment

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

Even for local vars, more descriptive names are prefererrable.

@pfalcon
Copy link
Contributor

pfalcon commented Jun 26, 2019

I'm with @carlescufi here: at all times we should prefer standard, well-known APIs instead of NIH "original designs". Here, the POSIX timing APIs, possibly with well-known extensions. Sometimes, it may make sense to provide some "intermediate" level APIs, e.g. when POSIX APIs are too bloaty for low-resource devices we target. But IMHO, that should be in addition to standard APIs, e.g. implementation stack would be low-level APIs, e.g. drivers/kernel -> intermediate API -> POSIX API.

@pabigot
Copy link
Contributor Author

pabigot commented Jun 27, 2019

This will be reworked based on adding gmtime() to minimal libc once #16626 goes in. No point spending more time on this PR.

@pabigot pabigot closed this Jun 27, 2019
@pfalcon
Copy link
Contributor

pfalcon commented Jun 27, 2019

No point spending more time on this PR.

Fairly speaking, that's not the outcome expected. The idea is to pool together people interested in adding standard APIs to Zephyr (and POSIX offers quite a good functionality coverage for systems APIs), and motivate them to work together towards good solutions, instead of on-spot workarounds (like "putting functions in drivers").

But good to hear a new version will be submitted, thanks!

@pabigot
Copy link
Contributor Author

pabigot commented Jun 27, 2019

@pfalcon Yes to coordinating changes, but the approach and details in this specific PR are clearly no longer the right path forward. We'll argue about whether the hinnant function should be put in a shareable location, and whether its variables should be renamed from the reference implementation, when there's a PR that introduces a second copy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: API Changes to public APIs area: C Library C Standard Library area: Tests Issues related to a particular existing or missing test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants