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

Update rcutils_steady_time_now to return the same data as std::chrono #357

Merged
merged 2 commits into from
Apr 27, 2022

Conversation

clalancette
Copy link
Contributor

Based on an investigation several years ago, rcutils_steady_time_now
has slightly different behavior than std::chrono::now . In
particular, on Linux rcutils_steady_time_now was using
CLOCK_MONOTONIC_RAW, while std::chrono is using CLOCK_MONOTONIC.
On macOS, rcutils_steady_time_now was using SYSTEM_CLOCK, while std::chrono
was using CLOCK_MONOTONIC_RAW. Fix both of these so they
match what std::chrono does, and in the case of macOS, significantly
simplify the code by switching to clock_gettime.

Signed-off-by: Chris Lalancette clalancette@openrobotics.org

This should fix #43 . Draft for now, as I haven't even compile tested this on macOS yet.

Based on an investigation several years ago, rcutils_steady_time_now
has slightly different behavior than std::chrono::now .  In
particular, on Linux rcutils_steady_time_now was using
CLOCK_MONOTONIC_RAW, while std::chrono is using CLOCK_MONOTONIC.
On macOS, rcutils_steady_time_now was using SYSTEM_CLOCK, while std::chrono
was using CLOCK_MONOTONIC_RAW.  Fix both of these so they
match what std::chrono does, and in the case of macOS, significantly
simplify the code by switching to clock_gettime.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Copy link
Member

@aprotyas aprotyas left a comment

Choose a reason for hiding this comment

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

LGTM with a minor nit

src/time_unix.c Outdated Show resolved Hide resolved
Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

lgtm, aligned with std::chrono would be better for user application.

With this, all of the tests seem to pass.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette clalancette marked this pull request as ready for review April 26, 2022 18:48
@clalancette
Copy link
Contributor Author

clalancette commented Apr 26, 2022

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@clalancette
Copy link
Contributor Author

The test failing on Windows shows up in the Windows repeated jobs as well: https://ci.ros2.org/view/nightly/job/nightly_win_rep/2596 . So I'm going to go ahead and merge this, thanks for the reviews.

@clalancette clalancette merged commit 91d21c6 into master Apr 27, 2022
@delete-merged-branch delete-merged-branch bot deleted the clalancette/update-steady-time branch April 27, 2022 16:28
alsora pushed a commit to irobot-ros/rcutils that referenced this pull request Jul 28, 2023
…ros2#357)

* Update rcutils_steady_time_now to return the same data as std::chrono

Based on an investigation several years ago, rcutils_steady_time_now
has slightly different behavior than std::chrono::now .  In
particular, on Linux rcutils_steady_time_now was using
CLOCK_MONOTONIC_RAW, while std::chrono is using CLOCK_MONOTONIC.
On macOS, rcutils_steady_time_now was using SYSTEM_CLOCK, while std::chrono
was using CLOCK_MONOTONIC_RAW.  Fix both of these so they
match what std::chrono does, and in the case of macOS, significantly
simplify the code by switching to clock_gettime.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
alsora pushed a commit to irobot-ros/rcutils that referenced this pull request Jul 28, 2023
…ros2#357)

* Update rcutils_steady_time_now to return the same data as std::chrono

Based on an investigation several years ago, rcutils_steady_time_now
has slightly different behavior than std::chrono::now .  In
particular, on Linux rcutils_steady_time_now was using
CLOCK_MONOTONIC_RAW, while std::chrono is using CLOCK_MONOTONIC.
On macOS, rcutils_steady_time_now was using SYSTEM_CLOCK, while std::chrono
was using CLOCK_MONOTONIC_RAW.  Fix both of these so they
match what std::chrono does, and in the case of macOS, significantly
simplify the code by switching to clock_gettime.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

reconsider use of CLOCK_MONOTONIC_RAW
3 participants