Skip to content
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

[Bug] Use CLOCK_MONOTONIC where available instead of CLOCK_REALTIME #270

Closed
bryceschober opened this issue Oct 25, 2023 · 4 comments
Closed
Labels
bug Something isn't working

Comments

@bryceschober
Copy link
Contributor

Describe the bug

The PR #269 brought my attention to the fact that z_clock_now() uses clock_gettime(CLOCK_REALTIME...) on most posix-like platforms. But POSIX specifies that CLOCK_MONOTONIC is the only one appropriate for usage as a relative timing reference, because it will never go backwards as a result of calling clock_settime(). In practice, this is definitely true on Linux systems, and is also true in the Zephyr implementation. The esp-idf implementation looks like it has the same properties.

So in practice, relative timing (the most common usage of time in embedded systems at the library / protocol level) should only ever use CLOCK_MONOTONIC, and CLOCK_REALTIME should only ever be used when the use case is actually user-level wall-clock time.

To reproduce

Sorry, but I haven't tried this out in actual practice... but it's pretty widely-known best practices for embedded posix-like systems.

  1. Start a long-running example.
  2. Use some means of setting the system wall clock backwards that ultimately uses clock_settime(), in simulation of an NTP update, etc.
  3. What happens? 🤷 Depending on the system implementation, protocol timing could be very broken...

System info

Any posix-like system using CLOCK_REALTIME.

@bryceschober bryceschober added the bug Something isn't working label Oct 25, 2023
@cguimaraes
Copy link
Member

Indeed.

In fact, we have both z_clock_* and z_time_* primitives which original purpose was to include support for both time computations.

But I am now wondering if for this case, we want the CLOCK_MONOTONIC or the CLOCK_MONOTONIC_RAW, since the former might still have its clock oscillator disciplined by NTP.

@cguimaraes
Copy link
Member

Being addressed by @jean-roland in #269

@Mallets
Copy link
Member

Mallets commented Oct 26, 2023

Indeed.

In fact, we have both z_clock_* and z_time_* primitives which original purpose was to include support for both time computations.

But I am now wondering if for this case, we want the CLOCK_MONOTONIC or the CLOCK_MONOTONIC_RAW, since the former might still have its clock oscillator disciplined by NTP.

I've just checked what Rust does: https://doc.rust-lang.org/std/time/struct.Instant.html#underlying-system-calls
CLOCK_MONOTONIC is used for Unix platforms. For compatibility reasons, I'd then prefer to keep CLOCK_MONOTONIC. By doing so, we can have the same behaviour in Zenoh-Pico and Zenoh-Rust-based API for what concerns time counting.

@Mallets
Copy link
Member

Mallets commented Oct 26, 2023

Closed by #269

@Mallets Mallets closed this as completed Oct 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants