Add Clock::sleep_until method by emersonknapp · Pull Request #1748 · ros2/rclcpp

emersonknapp · 2021-08-12T02:42:34Z

Part of #1730
Related to ros2/rosbag2#694

Add a Clock::sleep_until method. Handle all 3 clock types, and edge cases for shutdown and enabling/disabling ROS time.

Adds test cases for enable/disable ROStime, and rclcpp::shutdown - which needed special interrupts. Tests ROS time via /clock messages, and has basic testing for steady/system times, though those are much simpler cases.

emersonknapp · 2021-08-13T01:20:02Z

For curiosity while awaiting feedback:

Linux
Linux-aarch64
macOS
Windows

fujitatomoya

overall, i think looks good. some minor comments.

rclcpp/src/rclcpp/clock.cpp

rclcpp/test/rclcpp/test_time.cpp

fujitatomoya · 2021-08-27T04:26:31Z

@emersonknapp

i think it looks okay.

could you also address linter warning?
https://build.ros2.org/job/Rpr__rclcpp__ubuntu_focal_amd64/1355/testReport/junit/rclcpp/uncrustify/src_rclcpp_clock_cpp/

emersonknapp · 2021-09-13T20:45:55Z

@fujitatomoya do you have merge access? Or do we also need reviews from one of the maintainers @mabelzhang @wjwwood @ivanpauno

emersonknapp · 2021-09-13T20:47:31Z

Linux
Linux-aarch64
macOS
Windows

fujitatomoya · 2021-09-13T20:56:28Z

do we also need reviews from one of the maintainers

👍 we could use the other review. @ivanpauno @clalancette @wjwwood

clalancette

A few minor things that I think we should fix, but otherwise looks pretty good to me.

rclcpp/test/rclcpp/test_time.cpp

rclcpp/src/rclcpp/clock.cpp

clalancette

I've left one nit inline, but it just changing a documentation string.

More concerning is the yellow builds on aarch64; it looks like one of these tests is probably crashing there. Once that is fixed, I'm happy to approve.

rclcpp/include/rclcpp/clock.hpp

Handle all 3 clock types, and edge cases for shutdown and enabling/disabling ROS time Signed-off-by: Emerson Knapp <eknapp@amazon.com>

…ond resolution on clang(osx/linux) and windows, unlike nanosecond on Linux+GCC) Signed-off-by: Emerson Knapp <eknapp@amazon.com>

…est environment on Jenkins Signed-off-by: Emerson Knapp <eknapp@amazon.com>

…ess now() Signed-off-by: Emerson Knapp <eknapp@amazon.com>

…nd the test environment on Jenkins" This reverts commit 4fb7660. Signed-off-by: Emerson Knapp <eknapp@amazon.com>

Signed-off-by: Emerson Knapp <eknapp@amazon.com>

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>

… input Signed-off-by: Emerson Knapp <eknapp@amazon.com>

emersonknapp · 2021-09-23T22:39:13Z

little sanity check aarch64

emersonknapp · 2021-09-23T23:19:37Z

Linux
Linux-aarch64
macOS
Windows

emersonknapp · 2021-09-24T19:12:47Z

@clalancette are you able to merge this, based on that approval?

clalancette · 2021-09-24T19:15:08Z

Yeah, I'll go ahead and merge.

clalancette · 2021-09-24T22:02:19Z

This is flakey on aarch64: https://ci.ros2.org/job/ci_linux-aarch64/10018/

emersonknapp · 2021-09-27T17:46:07Z

Yeah that's weird. I'm looking into it, can spin up an ARM instance on EC2

This reverts commit 81df584.

This reverts commit 81df584. Signed-off-by: Jorge Perez <jjperez@ekumenlabs.com>

This reverts commit d04319a.

This reverts commit d04319a. Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* Revert "Revert "Add Clock::sleep_until method (#1748)" (#1793)" This reverts commit d04319a. Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Context, Shutdown Callback, Condition Var per call The `Clock` doesn't have enough information to know which Context should wake it on shutdown, so this adds a Context as an argument to sleep_until(). Since the context is per call, the shutdown callback is also registered per call and cannot be stored on impl_. The condition_variable is also unique per call to reduce spurious wakeups when multiple threads sleep on the same clock. Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Throw if until has wrong clock type Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * unnecessary newline Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Shorten line Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Fix time jump thresholds and add ROS time test Use -1 and 1 thresholds because 0 and 0 is supposed to disable the callbacks Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Shorten line Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * rclcpp::ok() -> context->is_valid() Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * No pre-jump handler instead of noop handler Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * If ros_time_is_active errors, let it throw Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Get time source change from callback to avoid race if ROS time toggled quickly Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Fix threshold and no pre-jump callback Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Use RCUTILS_MS_TO_NS macro Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Explicit cast for duration to system time Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Throws at the end of docblock Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Add tests for invalid and non-global contexts Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

This reverts commit 81df584. Signed-off-by: Jorge Perez <jjperez@ekumenlabs.com>

* Revert "Revert "Add Clock::sleep_until method (ros2#1748)" (ros2#1793)" This reverts commit d04319a. Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Context, Shutdown Callback, Condition Var per call The `Clock` doesn't have enough information to know which Context should wake it on shutdown, so this adds a Context as an argument to sleep_until(). Since the context is per call, the shutdown callback is also registered per call and cannot be stored on impl_. The condition_variable is also unique per call to reduce spurious wakeups when multiple threads sleep on the same clock. Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Throw if until has wrong clock type Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * unnecessary newline Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Shorten line Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Fix time jump thresholds and add ROS time test Use -1 and 1 thresholds because 0 and 0 is supposed to disable the callbacks Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Shorten line Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * rclcpp::ok() -> context->is_valid() Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * No pre-jump handler instead of noop handler Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * If ros_time_is_active errors, let it throw Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Get time source change from callback to avoid race if ROS time toggled quickly Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Fix threshold and no pre-jump callback Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Use RCUTILS_MS_TO_NS macro Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Explicit cast for duration to system time Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Throws at the end of docblock Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Add tests for invalid and non-global contexts Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

emersonknapp mentioned this pull request Aug 12, 2021

[Epic] Rosbag2 playback time control and simulation time ros2/rosbag2#696

Closed

50 tasks

emersonknapp force-pushed the emersonknapp/clock-sleep-until branch from 7fffce5 to 3842e17 Compare August 13, 2021 01:08

fujitatomoya reviewed Aug 16, 2021

View reviewed changes

emersonknapp force-pushed the emersonknapp/clock-sleep-until branch from e980175 to b3b53f2 Compare August 26, 2021 21:22

emersonknapp requested a review from fujitatomoya August 26, 2021 21:22

fujitatomoya approved these changes Sep 13, 2021

View reviewed changes

emersonknapp force-pushed the emersonknapp/clock-sleep-until branch from 02beba9 to f0cd10e Compare September 13, 2021 21:33

clalancette requested changes Sep 16, 2021

View reviewed changes

rclcpp/test/rclcpp/test_time.cpp Outdated Show resolved Hide resolved

rclcpp/src/rclcpp/clock.cpp Show resolved Hide resolved

rclcpp/src/rclcpp/clock.cpp Show resolved Hide resolved

emersonknapp force-pushed the emersonknapp/clock-sleep-until branch from f0cd10e to 489e51d Compare September 16, 2021 23:57

emersonknapp requested a review from clalancette September 17, 2021 00:00

clalancette requested changes Sep 17, 2021

View reviewed changes

rclcpp/include/rclcpp/clock.hpp Outdated Show resolved Hide resolved

Emerson Knapp and others added 8 commits September 23, 2021 15:27

Add Clock::sleep_until method

a8745e2

Handle all 3 clock types, and edge cases for shutdown and enabling/disabling ROS time Signed-off-by: Emerson Knapp <eknapp@amazon.com>

Cross-platform compatibility for system time_point (which is microsec…

c68b817

…ond resolution on clang(osx/linux) and windows, unlike nanosecond on Linux+GCC) Signed-off-by: Emerson Knapp <eknapp@amazon.com>

WIP commit to be reverted - adding logging output to understand the t…

72bc231

…est environment on Jenkins Signed-off-by: Emerson Knapp <eknapp@amazon.com>

Make sure that if time source changes, we're not comparing a meaningl…

206ab8f

…ess now() Signed-off-by: Emerson Knapp <eknapp@amazon.com>

Revert "WIP commit to be reverted - adding logging output to understa…

018c75c

…nd the test environment on Jenkins" This reverts commit 4fb7660. Signed-off-by: Emerson Knapp <eknapp@amazon.com>

Address review comments

b58c37e

Signed-off-by: Emerson Knapp <eknapp@amazon.com>

Fix uncrustify warning

54585b7

Signed-off-by: Emerson Knapp <eknapp@amazon.com>

Add a few edge case test cases based on reviewer feedback

5b61edc

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>

emersonknapp force-pushed the emersonknapp/clock-sleep-until branch from 489e51d to 9529ef8 Compare September 23, 2021 22:32

Update documentation to mention what happens if a time in the past is…

c6ab0a8

… input Signed-off-by: Emerson Knapp <eknapp@amazon.com>

emersonknapp force-pushed the emersonknapp/clock-sleep-until branch from 9529ef8 to c6ab0a8 Compare September 23, 2021 22:35

emersonknapp requested a review from clalancette September 23, 2021 23:19

clalancette approved these changes Sep 24, 2021

View reviewed changes

clalancette merged commit 81df584 into ros2:master Sep 24, 2021

Blast545 mentioned this pull request Sep 27, 2021

👨‍🌾 5 Test regressions in nightly_osx_debug #1787

Closed

Blast545 added a commit that referenced this pull request Oct 1, 2021

Revert "Add Clock::sleep_until method (#1748)"

0cf60cd

This reverts commit 81df584.

Blast545 added a commit that referenced this pull request Oct 1, 2021

Revert "Add Clock::sleep_until method (#1748)"

fb643a0

This reverts commit 81df584. Signed-off-by: Jorge Perez <jjperez@ekumenlabs.com>

Blast545 added a commit that referenced this pull request Oct 5, 2021

Revert "Add Clock::sleep_until method (#1748)" (#1793)

d04319a

This reverts commit 81df584. Signed-off-by: Jorge Perez <jjperez@ekumenlabs.com>

sloretz added a commit that referenced this pull request Nov 3, 2021

Revert "Revert "Add Clock::sleep_until method (#1748)" (#1793)"

01b86ec

This reverts commit d04319a.

sloretz mentioned this pull request Nov 3, 2021

Re-add Clock::sleep_until #1814

Merged

sloretz added a commit that referenced this pull request Nov 3, 2021

Revert "Revert "Add Clock::sleep_until method (#1748)" (#1793)"

4f49e11

This reverts commit d04319a. Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Bi0T1N pushed a commit to Bi0T1N/rclcpp that referenced this pull request Dec 15, 2021

Revert "Add Clock::sleep_until method (ros2#1748)" (ros2#1793)

823a0d1

This reverts commit 81df584. Signed-off-by: Jorge Perez <jjperez@ekumenlabs.com>

Conversation

emersonknapp commented Aug 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

emersonknapp commented Aug 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fujitatomoya left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fujitatomoya commented Aug 27, 2021

Uh oh!

emersonknapp commented Sep 13, 2021

Uh oh!

emersonknapp commented Sep 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fujitatomoya commented Sep 13, 2021

Uh oh!

clalancette left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

clalancette left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

emersonknapp commented Sep 23, 2021

Uh oh!

emersonknapp commented Sep 23, 2021

Uh oh!

emersonknapp commented Sep 24, 2021

Uh oh!

clalancette commented Sep 24, 2021

Uh oh!

clalancette commented Sep 24, 2021

Uh oh!

emersonknapp commented Sep 27, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

emersonknapp commented Aug 12, 2021 •

edited

Loading

emersonknapp commented Aug 13, 2021 •

edited

Loading

emersonknapp commented Sep 13, 2021 •

edited

Loading