Add date shell commands

[nixward]

Currently only supports Unix time.

'get' subcommand returns date in format “Y-m-d H:M:S”
'set' subcommand format is '[Y-m-d] <H:M:S>'

The 'set' subcommand is implemented with basic date validation.

For user convenience of small adjustments to time the time argument will
accept H:M:S, :M:S or ::S where the missing field(s) will be filled in by
the previous time state.

[zephyrbot]

All checks are passing now.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

[jakub-uC]

Most likely I am missing something basic here but why you allow setting minutes and seconds in different ranges?

[nixward]

Most likely I am missing something basic here but why you allow setting minutes and seconds in different ranges?

I feel a little uneasy about this also. I was just allowing for the very unlikely event someone wants to set it at the moment of a leap second. Weighting it up I didn't think there would be any harm to allow it to be a bit more permissive with the small chance someone puts their clock out by a second. It does contradict the fact this PR's current scope is to only support Unix time. If you think this is bad I can definitely change this. There is currently no concise validation of month lengths (which could possibly come in a separate PR) making the day field extra permissive as well. I'll add comments to address this but I'm also happy to restrict it.

[jakub-uC]

Most likely I am missing something basic here but why you allow setting minutes and seconds in different ranges?

I feel a little uneasy about this also. I was just allowing for the very unlikely event someone wants to set it at the moment of a leap second. Weighting it up I didn't think there would be any harm to allow it to be a bit more permissive with the small chance someone puts their clock out by a second. It does contradict the fact this PR's current scope is to only support Unix time. If you think this is bad I can definitely change this. There is currently no concise validation of month lengths (which could possibly come in a separate PR) making the day field extra permissive as well. I'll add comments to address this but I'm also happy to restrict it.

I will appreciate a comment in the code :)
LGTM

[pabigot]

I haven't tested this personally but it looks like a good step towards improving civil time support.

I think it would be worth reconsidering the API for setting the clock to be more helpful. The set format "Y m d H M S" is not consistent with the display format YY-mm-dd HH:MM:SS UTC. Also consider the use case: it may be that most times people set the time they're trying to correct a slight error in an already-close time (e.g. from a battery-backed clock).

Could you accept something like [Y-m-d] H:M:S where the date part is completely optional, and any of the values (but not punctuation) that are missing are filled in by the current time? Then if the system time is 2020-02-10 12:34:56 and the actual time is 2020-02-10 12:43:20 the user can update it with the string ":43:20".

The year validation and use of unsigned conversion specifiers should be changed. The rework of user interface to be more flexible is suggestion, not a requirement; if you don't want it I'll approve anyway.

[nixward]

@pabigot thanks for the review, I'll address what you have raised ASAP. Would you have advice on how to get shippable to pass? There's seems to be an issue with the timespec header for some builds.

[pabigot]

Would you have advice on how to get shippable to pass? There's seems to be an issue with the timespec header for some builds.

Yeah. I don't have a solution for that; AFAICT <sys/timespec.h> is a BSD header, not a POSIX header, so I don't know why the Zephyr POSIX subsystem is including it for native builds. There's some discussion in 0173d86.

#18892 may also be relevant, as it would eliminate the need for Zephyr's <time.h> to try to reconstruct the content of a system <time.h>

[pfalcon]

Yeah. I don't have a solution for that; AFAICT <sys/timespec.h> is a BSD header, not a POSIX header, so I don't know why the Zephyr POSIX subsystem

Probably because "Zephyr POSIX subsystem" is intended to be real-worldly useful, e.g. interoperate with other components (e.g. newlib), and be able to build real-world software?

is including it for native builds.

"native builds" mentioned here are actually non-native builds, from Zephyr PoV. So-called "native_posix" target is a very special, use-only-if-you-know-what-you're-doing target which tries to emulate a subset of Zephyr functionality on top of a foreign system (full-fledged POSIX in this cass). It's not compatible with various Zephyr subsystems. For one, with Zephyr POSIX subsystem.

[nixward]

@pfalcon would you have advice on how to get the shippable build failure to pass?

[pfalcon]

@pabigot: If the date handling functionality implemented here is ok by you, can you approve or at least dismiss your -1?

[pfalcon]

@pfalcon would you have advice on how to get the shippable build failure to pass?

There're generally 2 ways:

1. Blacklist CONFIG_ARCH_POSIX-based systems (as they're not compatible with Zephyr POSIX subsys).
2. Given that here only clock_gettime/clock_settime is used, try to workaround ARCH_POSIX handling of these calls.

p.2 definitely in scope, there's anticipation and provisions that various software may require just small bits of POSIX, instead of a full coherent subsys, so there're ways to enable individual pieces (at the full responsibility and risk of particular target applications, there's no guarantee it would work in general case).

But "only clock_gettime/clock_settime" is deceptive, because handling happens at the granularity of headers, and the corresponding header(s) define much more stuff.

So, let's specify the mission statement (with special attn: to @pabigot, as you regularly question why sth in POSIX subsys was done in this or that way, please follow thru this case, in all previous cases, similar handling/logic was applied): currently Zephyr POSIX subsys supports 2 libc's: Newlin and minlibc. Here, we're talking about adding bits of support for 3rd libc - Linux' Glibc.

Let me remind how support for Newlib and minlibc was implemented: Newlib (as a more developed libc) was taken as a model, and minlibc was made to be consistent with it. In this case, Newlib defined struct timespec in <sys/timespec.h>. So, minlibc was made to do the same, then the same include covered both.

As can be seen from build error here, Linux doesn't have that header. So, let's now see how Linux Glibc-based system deals with that define. A grep here shows that it's defined in /usr/include/x86_64-linux-gnu/bits/types/struct_timespec.h. It's important to note that the pattern to resolve this issue is the same - each "system-specific" type is defined in a separate header file, or there would be untanglable mess. If only location for these system-specific headers was standardized! But nope, so where we use <sys/timespec.h> for newlib/minlibc, we'll need to #ifdef to use <bits/types/struct_timespec.h>. Oh, and of course, that header belongs not to Glibc, but to Linux kernel external API headers, so our mission statement is adjusted to "... interface with Linux kernel defines".

Repeated a bunch of times for other types, not used in this PR, but pulled from <time.h>, and thus leading to conflicts, I was able to build this PR for BOARD=native_posix. I'll put up my changes into a brunch shortly, and let's keep fingers crossed that fixing one thing didn't broke others (because that's how it typically was in POSIX subsys, until disciplined patterns like "minlibc should follow Newlib as close as possible" were adopted).

[pabigot]

Thanks for the changes; this looks good.

I was going to ask that -ENOEXEC be replaced by -EINVAL for all detected format/input errors, but AFAICT this mis-use is pervasive in Zephyr shell so for consistency I suppose it should stay.

[pfalcon]

Ok, I submitted my changes as #23219 to check that it doesn't cause regression. Build passed. The suggested action is to pick that commit into that PR. The above PR is marked as DNM.

[aescolar]

"native builds" mentioned here are actually non-native builds, from Zephyr PoV. So-called "native_posix" target is a very special, use-only-if-you-know-what-you're-doing target which tries to emulate a subset of Zephyr functionality on top of a foreign system (full-fledged POSIX in this cass). It's not compatible with various Zephyr subsystems. For one, with Zephyr POSIX subsystem.

This is quite misleading: The native_posix port does not "emulate Zephyr". It implements a replacement arch layer (CPU emulation if you wish) and a few peripherals/drivers, so the Zephyr kernel runs on it unknowingly. The only subsystems it is not compatible with are the POSIX API shim*1 and userspace*2, or those requiring drivers which are not yet implemented in native_posix (The full list is actually here #6044).
You can consider it a means that allows for some types of testing and which has a few of the same practical limitation you would face with a unit test framework (as you are compiling your code targeting your workstation OS).

*1 which was supported for a while. But on @pfalcon 's request, to simplify the development of the POSIX API, support was dropped.
*2 there is no MPU emulation.

You can find much more info on native_posix here: https://docs.zephyrproject.org/latest/boards/posix/native_posix/doc/index.html

[nixward]

@pabigot

I was going to ask that -ENOEXEC be replaced by -EINVAL for all detected format/input errors, but AFAICT this mis-use is pervasive in Zephyr shell so for consistency I suppose it should stay.

I see what you are saying. I've switched it. If anyone objects let me know.

[pfalcon]

@nixward : I guess DNM label should be removed now.