Add Clock.sleep_until by sloretz · Pull Request #858 · ros2/rclpy

sloretz · 2021-12-09T01:30:07Z

Fixes #617

This works, and adds a method Clock.sleep_until() to sleep until a particular time on a clock is reached, respecting ROS time.

I chose to implement the time jump callbacks in Python because it's easier and reuses code, but it does mean the internal API Event::wait_until_ros() requires the caller to setup the required time jump callback or it will wait forever. Since this is an internal API, I think it's fine to document it and leave it as is. I couldn't think of a cleaner solution and user's won't be affected one way or the other.

Related to ros2/rclcpp#1730

Signed-off-by: Shane Loretz <sloretz@openrobotics.org>

sloretz · 2021-12-09T18:39:40Z

PR job requires ros2/rcl#955 released to pass

Signed-off-by: Shane Loretz <sloretz@openrobotics.org>

sloretz · 2021-12-13T20:10:41Z

CI (build: --packages-up-to rclpy test: --packages-select rclpy)

Linux
Linux-aarch64
macOS
Windows

Signed-off-by: Shane Loretz <sloretz@openrobotics.org>

sloretz · 2021-12-13T22:04:25Z

CI (Re-run)

Linux
Linux-aarch64
macOS
Windows

Signed-off-by: Shane Loretz <sloretz@openrobotics.org>

sloretz · 2021-12-13T22:44:26Z

CI Re-run

Linux
Linux-aarch64
macOS
Windows

clalancette · 2021-12-14T18:47:49Z

PR job requires ros2/rcl#955 released to pass

I've merged ros/rosdistro#31477, which should allow the Rpr job to pass (once the downstream packages are done compiling. which will take a while).

clalancette

I chose to implement the time jump callbacks in Python because it's easier and reuses code

Just as a question: what is the alternative? To implement it on the C++ side?

Besides that, I've left some thoughts and comments inline. Nothing major, really, other than the possible problems of flaky tests.

rclpy/src/rclpy/event.hpp

rclpy/src/rclpy/event.cpp

rclpy/test/test_clock.py

sloretz · 2021-12-14T19:36:44Z

I chose to implement the time jump callbacks in Python because it's easier and reuses code

Just as a question: what is the alternative? To implement it on the C++ side?

Yeah, the alternative would be to implement it in C++ by calling rcl_clock_add_jump_callback() directly. The advantage is the internal rclpy::Event::wait_until_ros() API would encapsulate all the ROS time stuff; the disadvantage is Python is easier to write. Since it's an internal API I went with the easier option.

Signed-off-by: Shane Loretz <sloretz@openrobotics.org>

clalancette · 2021-12-14T22:08:06Z

rclpy/test/test_clock.py

+    clock._set_ros_time_is_active(ros_time_enabled)
+
+    # wait for thread to exit
+    time.sleep(0.2)


If we don't daemonize the thread, then we could do a join on the thread here instead, which at least gets rid of one sleep in the tests. What do you think?

join()ing the threads to avoid the second sleep in a8ddedf

clalancette · 2021-12-14T22:08:32Z

Yeah, the alternative would be to implement it in C++ by calling rcl_clock_add_jump_callback() directly. The advantage is the internal rclpy::Event::wait_until_ros() API would encapsulate all the ROS time stuff; the disadvantage is Python is easier to write. Since it's an internal API I went with the easier option.

Sounds reasonable to me. I think the solution you've chosen is fine.

clalancette · 2021-12-15T14:26:27Z

@ros-pull-request-builder retest this please

j-rivero

I can not find any relevant issue or discrepancy in the code. Just a Nit about doxygen. I re-run CI trying to trigger the possible race condition in tests.

Linux
Linux-aarch64
macOS
Windows

j-rivero · 2021-12-15T14:39:08Z

rclpy/src/rclpy/event.hpp

+public:
+  /// Wait until a time specified by a system or steady clock.
+  /// \param clock the clock to use for time synchronization with until
+  template<typename ClockType>


Nit: Do we need to document the until parameter here and in the next method?

Documenting the 'until' parameter in de582e1

Signed-off-by: Shane Loretz <sloretz@openrobotics.org>

ivanpauno

I have some minor comments, otherwise lgtm!

rclpy/rclpy/clock.py

rclpy/src/rclpy/event.cpp

ivanpauno · 2021-12-15T17:41:33Z

rclpy/test/test_clock.py

 # See the License for the specific language governing permissions and
 # limitations under the License.

+import threading


Some of my comments in the tests cases of #864 also apply here

Of my comments in the other PR, I think that the only one relevant for the moment is using thread.join() instead of a sleep() call to wait the thread to exit.
The other comments can be ignored.

Signed-off-by: Shane Loretz <sloretz@openrobotics.org>

clalancette

I agree with @ivanpauno's comment about renaming Event -> ClockEvent. I also have expressed concern about the possibility of flaky tests here, but I actually don't see a reasonable way to fix it given what this is trying to test. So once the rename is done, I'm happy to approve.

Signed-off-by: Shane Loretz <sloretz@openrobotics.org>

fujitatomoya

lgtm

sloretz · 2021-12-15T19:03:17Z

CI (build: --packages-up-to rclpy test: --packages-select rclpy)

Linux
Linux-aarch64
macOS
Windows

sleep_until works, but need to split other fixes out

63b735d

Signed-off-by: Shane Loretz <sloretz@openrobotics.org>

sloretz changed the title ~~sleep_until works, but need to split other fixes out~~ Add Clock.sleep_until Dec 9, 2021

sloretz mentioned this pull request Dec 9, 2021

Make context.on_shutdown allow free functions as shutdown callbacks #859

Merged

This was referenced Dec 9, 2021

Fix time.py and clock.py circular import #860

Merged

Improve JumpThreshold documentation and forbid zero durations #861

Merged

Add __enter__ and __exit__ to JumpHandle #862

Merged

sloretz added 7 commits December 13, 2021 09:23

Merge branch 'master' into rclpy__sleep_until

e3ceee4

Reduce differences with Master

112e856

Signed-off-by: Shane Loretz <sloretz@openrobotics.org>

flake8 whitespace fix

d90902a

Signed-off-by: Shane Loretz <sloretz@openrobotics.org>

Add docstring to sleep_until

54a7dc2

Signed-off-by: Shane Loretz <sloretz@openrobotics.org>

Check ROS Time enabled in C++ to avoid TOCTTOU bug

c90a41e

Signed-off-by: Shane Loretz <sloretz@openrobotics.org>

Ditch unused bool return

8418310

Signed-off-by: Shane Loretz <sloretz@openrobotics.org>

Add documentation to Event

0a0c5a3

Signed-off-by: Shane Loretz <sloretz@openrobotics.org>

sloretz marked this pull request as ready for review December 13, 2021 20:09

Duration cast for clocks with poor resolutions

4fe3c8d

Signed-off-by: Shane Loretz <sloretz@openrobotics.org>

Satisfy uncrustify

7391967

Signed-off-by: Shane Loretz <sloretz@openrobotics.org>

sloretz mentioned this pull request Dec 13, 2021

Add Clock.sleep_for using sleep_until #864

Merged

clalancette reviewed Dec 14, 2021

View reviewed changes

sloretz added 4 commits December 14, 2021 11:47

Remove 'be'

9c93f34

Signed-off-by: Shane Loretz <sloretz@openrobotics.org>

Add missing chrono and mutex includes

a07942d

Signed-off-by: Shane Loretz <sloretz@openrobotics.org>

Shorten really long timeouts

0530656

Signed-off-by: Shane Loretz <sloretz@openrobotics.org>

Shorten on_shutdown callback call

e6fe67a

Signed-off-by: Shane Loretz <sloretz@openrobotics.org>

clalancette reviewed Dec 14, 2021

View reviewed changes

j-rivero reviewed Dec 15, 2021

View reviewed changes

sloretz added 2 commits December 15, 2021 09:29

Join threads to avoid sleeping

a8ddedf

Signed-off-by: Shane Loretz <sloretz@openrobotics.org>

Document until parameter

de582e1

Signed-off-by: Shane Loretz <sloretz@openrobotics.org>

ivanpauno reviewed Dec 15, 2021

View reviewed changes

sloretz added 2 commits December 15, 2021 10:00

Larger small amount of time and make sure 10 second tests are quicker

8615b7b

Signed-off-by: Shane Loretz <sloretz@openrobotics.org>

Rename event.* to clock_event.*

cd90a77

Signed-off-by: Shane Loretz <sloretz@openrobotics.org>

clalancette reviewed Dec 15, 2021

View reviewed changes

Event -> ClockEvent

dbf83fe

Signed-off-by: Shane Loretz <sloretz@openrobotics.org>

fujitatomoya approved these changes Dec 15, 2021

View reviewed changes

clalancette approved these changes Dec 15, 2021

View reviewed changes

sloretz merged commit 9b7e50b into master Dec 15, 2021

delete-merged-branch bot deleted the rclpy__sleep_until branch December 15, 2021 21:00

clalancette mentioned this pull request Apr 29, 2022

ROS 2 Humble needed release notes ros2/ros2_documentation#2446

Closed

87 tasks

Conversation

sloretz commented Dec 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sloretz commented Dec 9, 2021

Uh oh!

sloretz commented Dec 13, 2021

Uh oh!

sloretz commented Dec 13, 2021

Uh oh!

sloretz commented Dec 13, 2021

Uh oh!

clalancette commented Dec 14, 2021

Uh oh!

clalancette left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sloretz commented Dec 14, 2021

Uh oh!

clalancette Dec 14, 2021

Choose a reason for hiding this comment

Uh oh!

sloretz Dec 15, 2021

Choose a reason for hiding this comment

Uh oh!

clalancette commented Dec 14, 2021

Uh oh!

clalancette commented Dec 15, 2021

Uh oh!

j-rivero left a comment

Choose a reason for hiding this comment

Uh oh!

j-rivero Dec 15, 2021

Choose a reason for hiding this comment

Uh oh!

sloretz Dec 15, 2021

Choose a reason for hiding this comment

Uh oh!

ivanpauno left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ivanpauno Dec 15, 2021

Choose a reason for hiding this comment

Uh oh!

ivanpauno Dec 15, 2021

Choose a reason for hiding this comment

Uh oh!

clalancette left a comment

Choose a reason for hiding this comment

Uh oh!

fujitatomoya left a comment

Choose a reason for hiding this comment

Uh oh!

sloretz commented Dec 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sloretz commented Dec 9, 2021 •

edited

Loading