Add Clock.sleep_until #858
Conversation
Signed-off-by: Shane Loretz <sloretz@openrobotics.org>
sloretz
changed the title
|
PR job requires ros2/rcl#955 released to pass |
Signed-off-by: Shane Loretz <sloretz@openrobotics.org>
Signed-off-by: Shane Loretz <sloretz@openrobotics.org>
Signed-off-by: Shane Loretz <sloretz@openrobotics.org>
Signed-off-by: Shane Loretz <sloretz@openrobotics.org>
Signed-off-by: Shane Loretz <sloretz@openrobotics.org>
Signed-off-by: Shane Loretz <sloretz@openrobotics.org>
|
CI (build:
|
Signed-off-by: Shane Loretz <sloretz@openrobotics.org>
|
CI (Re-run)
|
Signed-off-by: Shane Loretz <sloretz@openrobotics.org>
|
CI Re-run
|
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). |
There was a problem hiding this comment.
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.
Yeah, the alternative would be to implement it in C++ by calling |
Signed-off-by: Shane Loretz <sloretz@openrobotics.org>
Signed-off-by: Shane Loretz <sloretz@openrobotics.org>
Signed-off-by: Shane Loretz <sloretz@openrobotics.org>
Signed-off-by: Shane Loretz <sloretz@openrobotics.org>
rclpy/test/test_clock.py
Outdated
| clock._set_ros_time_is_active(ros_time_enabled) | ||
|
|
||
| # wait for thread to exit | ||
| time.sleep(0.2) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
join()ing the threads to avoid the second sleep in a8ddedf
Sounds reasonable to me. I think the solution you've chosen is fine. |
|
@ros-pull-request-builder retest this please |
rclpy/src/rclpy/event.hpp
Outdated
| 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> |
There was a problem hiding this comment.
Nit: Do we need to document the until parameter here and in the next method?
There was a problem hiding this comment.
Documenting the 'until' parameter in de582e1
Signed-off-by: Shane Loretz <sloretz@openrobotics.org>
Signed-off-by: Shane Loretz <sloretz@openrobotics.org>
| @@ -12,18 +12,23 @@ | |||
| # See the License for the specific language governing permissions and | |||
| # limitations under the License. | |||
|
|
|||
| import threading | |||
There was a problem hiding this comment.
Some of my comments in the tests cases of #864 also apply here
There was a problem hiding this comment.
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>
Signed-off-by: Shane Loretz <sloretz@openrobotics.org>
There was a problem hiding this comment.
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>
|
CI (build:
|
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