Skip to content

libc/minimal: add support for gmtime and its reverse "timegm" #17468

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Jul 17, 2019
Merged

libc/minimal: add support for gmtime and its reverse "timegm" #17468

merged 4 commits into from
Jul 17, 2019

Conversation

@pabigot
Copy link
Contributor

@pabigot pabigot commented Jul 10, 2019 •
edited
Loading

This PR replaces #17003, using the standard name gmtime for conversion between struct tm and time_t. The gmtime function is added to minimal libc, and a sys library supporting time-related operations is added for the reverse function compatible with the GLIBC timegm extension.

This adds the POSIX gmtime_r instead of the C gmtime_s because the former is more common and, in fact, is currently used in stm32 drivers.

@pabigot pabigot added area: C Library C Standard Library area: API Changes to public APIs labels Jul 10, 2019
@pabigot pabigot added this to the 2.0 milestone Jul 10, 2019
@pabigot pabigot requested review from nashif and pfalcon July 10, 2019 16:13
@pabigot pabigot changed the title add support for gmtime and its reverse "timegm" libc/minimal: add support for gmtime and its reverse "timegm" Jul 10, 2019
@zephyrbot zephyrbot added the area: Tests Issues related to a particular existing or missing test label Jul 10, 2019
@zephyrbot
Copy link

zephyrbot commented Jul 10, 2019

Found the following issues, please fix and resubmit:

License issues

In most cases you do not need to do anything here, especially if the files
reported below are going into ext/ and if license was approved for inclusion
into ext/ already. Fix any missing license/copyright issues. The license
exception if a JFYI for the maintainers and can be overriden when merging the
pull request.

  • lib/libc/minimal/source/time/gmtime.c is not apache-2.0 licensed: public-domain
  • lib/libc/minimal/source/time/gmtime.c has non-permissive license: public-domain
  • lib/os/timeutil.c is not apache-2.0 licensed: public-domain
  • lib/os/timeutil.c has non-permissive license: public-domain

@pabigot
Copy link
Contributor Author

pabigot commented Jul 10, 2019 •
edited
Loading

I've updated the code so the words "public domain" are in a separate comment from the one that provides the proper SPDX-License-Identifier. Apparently that wasn't good enough for CI, but this seems to be the best we do in this situation since SPDX doesn't support structured acknowledgement of public domain origins without a registered dedication, and that's way too much work.

Found the following issues, please fix and resubmit:

License issues

In most cases you do not need to do anything here, especially if the files
reported below are going into ext/ and if license was approved for inclusion
into ext/ already. Fix any missing license/copyright issues. The license
exception if a JFYI for the maintainers and can be overriden when merging the
pull request.

  • lib/libc/minimal/source/time/gmtime.c is not apache-2.0 licensed: public-domain
  • lib/libc/minimal/source/time/gmtime.c has non-permissive license: public-domain
  • lib/os/timeutil.c is not apache-2.0 licensed: public-domain
  • lib/os/timeutil.c has non-permissive license: public-domain

@MaureenHelm MaureenHelm modified the milestones: 2.0, v2.0.0 Jul 10, 2019
pabigot added 4 commits July 11, 2019 10:31
@pabigot
libc: minimal: provide types in time.h
66f0ebe 
Provide definitions for a subset of the standard 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
libc: minimal: provide gmtime implementation
515fe56 
Implement the conversion from UNIX time to broken-down civil time per
the gmtime() and gmtime_r() functions.

Signed-off-by: Peter A. Bigot <[email protected]>
@pabigot
sys: timeutil: add module
866e7d1 
Add a generic API to provide the inverse operation for gmtime and as a
home for future generic time-related functions that are not in POSIX.

Signed-off-by: Peter A. Bigot <[email protected]>
@pabigot
tests/lib/timeutil: add tests for time conversions
4e53c36 
This verifies gmtime and timeutil_timegm against each other and
reference data for a wide range of instances.

Signed-off-by: Peter A. Bigot <[email protected]>
@pabigot
Copy link
Contributor Author

pabigot commented Jul 11, 2019

I've decoupled this from the POSIX subsystem, hopefully reducing any impediments to it being processed towards a merge.

There's nothing I can do about the license check failure, which seems to be based on the words "public domain" appearing in the file outside the license block.

@pabigot pabigot mentioned this pull request Jul 15, 2019
Closed
pfalcon
pfalcon approved these changes Jul 16, 2019
View reviewed changes
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.

Looks good, thanks for refactoring. (Can't vouch that there's nothing to improve here or something won't need a change later, but definitely looks good for the initial in, and surely useful).

carlescufi
carlescufi approved these changes Jul 17, 2019
View reviewed changes
Copy link
Member

@carlescufi carlescufi left a comment

Choose a reason for hiding this comment

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

This looks good and addresses all the discussion points that we have raised priorly

@carlescufi carlescufi merged commit 7251d8c into zephyrproject-rtos:master Jul 17, 2019
@pabigot pabigot deleted the pr/20190710a branch July 17, 2019 12:49
@PiotrZierhoffer PiotrZierhoffer mentioned this pull request Jul 25, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@carlescufi carlescufi carlescufi approved these changes

@nashif nashif Awaiting requested review from nashif

@andrewboie andrewboie Awaiting requested review from andrewboie

@andyross andyross Awaiting requested review from andyross

@galak galak Awaiting requested review from galak

@MaureenHelm MaureenHelm Awaiting requested review from MaureenHelm

+1 more reviewer

@pfalcon pfalcon pfalcon approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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

Milestone

v2.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@pabigot @zephyrbot @pfalcon @carlescufi @MaureenHelm