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

reconsider use of CLOCK_MONOTONIC_RAW #43

Closed
clalancette opened this issue Aug 4, 2017 · 9 comments · Fixed by #357
Closed

reconsider use of CLOCK_MONOTONIC_RAW #43

clalancette opened this issue Aug 4, 2017 · 9 comments · Fixed by #357
Labels
enhancement New feature or request

Comments

@clalancette
Copy link
Contributor

This is a duplicate of ros2/rcl#110, and originally came from a discussion at ros/roscpp_core#55 (comment)

@clalancette
Copy link
Contributor Author

From my understanding of the clocks, CLOCK_MONOTONIC start from some unspecified time, and is always monotonic (i.e. never goes backwards). However, it can be slewed based on adjtime() (basically NTP). CLOCK_MONOTONIC_RAW is the same, except it will never be slewed. The question we have to answer is what we want rcutils_steady_time_now() to represent. If we want it to represent "system time", plus or minus an offset, then we should use CLOCK_MONOTONIC. If we want it to always march forward at the same rate, we should use CLOCK_MONOTONIC_RAW. This article also has a decent explanation.

@wjwwood
Copy link
Member

wjwwood commented Aug 7, 2017

Hmm, it would be interesting to see what std::chrono and our Windows / macOS code are doing. Given that neither monotonic nor monotonic raw jump out at me as the obvious right behavior for steady (I would suppose it depends on the user's preference), I would lean towards consistency with std::chrono and/or the other platforms.

I will guess that Window's is more like monotonic raw as we have it implemented since it uses the performance ticks to calculate it (I think). It would be great if someone could summarize what the similar functions out there do for "steady".

@clalancette
Copy link
Contributor Author

OK, here's a summary of what is going on (as far as I can tell):

rcutils_steady_time_now():

std::chrono:

There is one other thing to consider, which is time spent during suspend. For instance, on Linux, CLOCK_MONOTONIC does not "tick" during suspend (https://stackoverflow.com/questions/6360210/androidlinux-uptime-using-clock-monotonic), but I believe on Windows, QueryPerformanceCounter does (https://stackoverflow.com/questions/24330496/how-do-i-create-monotonic-clock-on-windows-which-doesnt-tick-during-suspend). Again, MacOS documentation is lacking, so it is hard to tell, but it seems like mach_absolute_time() does not "tick" during suspend. (I just found this PEP, which lays all of this out nicely: https://www.python.org/dev/peps/pep-0418/#monotonic-clocks)

So, with all of the background above, it is worth looking into the expectations of the current users of rcutils_steady_time_now().

  1. rcl_get_steady_time()
    • Thin callback wrapper around rcutils_steady_time_now()
    • As far as I can tell, never called in current implementation (excepting tests)
  2. rcl_timer_*()
    • Called by several functions in here
    • Timer is wrapped:
      • rclcpp (TimerBase)
        • In turn, used to implemente GenericTimer and WallTimer
      • rclpy (WallTimer)
  3. logging_macros.h.em
    • In this case, the logging macros are looking to see if enough time has passed since the last print

So, given all the above, here are my conclusions/recommendations:

  1. Using something like CLOCK_MONOTONIC (instead of CLOCK_MONOTONIC_RAW) is probably the correct thing. The reason for it is that local oscillators are not perfect, and do not always line up with the amount of time that has actually passed, and thus having a bit of an adjustment from the outside is probably a good thing.
  2. Further, I think it behooves us to follow in the footsteps of std::chrono as @wjwwood points out, to keep expectations the same.
  3. Thus, we should change the implementation on Linux to use CLOCK_MONOTONIC, and change the implementation on MacOS to use mach_absolute_time().

This is really long, so I'll quit there. Thoughts?

@wjwwood
Copy link
Member

wjwwood commented Aug 8, 2017

Thus, we should change the implementation on Linux to use CLOCK_MONOTONIC, and change the implementation on MacOS to use mach_absolute_time().

Sounds good to me, though it's a shame that Window's behavior and Linux's behavior might differ. Also I think it is a bit academic because we're unlikely to have many timers which run long enough to be significantly affected by the slewing or not slewing, e.g. a slew of a few microseconds is unlikely to affect a timer for many seconds or even minutes.

@mikaelarguedas
Copy link
Member

@clalancette have all the action items of this issue been addressed ? if so should we close this ?

@wjwwood
Copy link
Member

wjwwood commented Feb 7, 2018

No, because we're still using monotonic raw in our linux implementation. Also, we've not updated our macOS code to look like libc++'s std::chrono implementation.

@mikaelarguedas
Copy link
Member

Thanks for the summary 👍

@alsora
Copy link

alsora commented Jun 29, 2020

Hi, any updates on this?
Is it still on the roadmap?

@clalancette
Copy link
Contributor Author

It's not something I've had time to work on. Though I would definitely be willing to do a review if someone else can fix the Linux and macOS implementations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants