Skip to content

shell: date_service: make clear that leap seconds are not supported #94310

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

diegoherranz
Copy link
Contributor

SYS_CLOCK_REALTIME is following Unix time so, even if tm_sec == 60 is accepted, make very clear that leap seconds are not supported.

jakub-uC
jakub-uC previously approved these changes Aug 11, 2025
@nixward
Copy link
Member

nixward commented Aug 11, 2025

Hi @diegoherranz, very valid point. To limit flash usage for a very narrow and erroneous use case it's likely better to limit range to 0-59 and remove additional string warning.

tm_sec == 60 was accepted but it would only result in the time being
wrongly set (off by 1 second) since SYS_CLOCK_REALTIME is following
Unix time which doesn't support them.

Better to not accept them in the first place.

Signed-off-by: Diego Herranz <[email protected]>
@diegoherranz
Copy link
Contributor Author

Hi @diegoherranz, very valid point. To limit flash usage for a very narrow and erroneous use case it's likely better to limit range to 0-59 and remove additional string warning.

Yes, I agree. Done.

Copy link
Member

@nixward nixward left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

@jakub-uC
Copy link
Contributor

jakub-uC commented Aug 12, 2025

I believe the previous implementation was also fine. The date command is optional and only included when CONFIG_DATE_SHELL is enabled. If someone needs to save flash space, they can simply exclude it entirely.
The current change is also fine, but personally, I wouldn’t go too far with it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Shell Shell subsystem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants