-
Notifications
You must be signed in to change notification settings - Fork 222
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
Make rclpy.spin*() use a persistent executor #176
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, with reran CI since it's been a while
b7d61f9
to
2de8cb7
Compare
New CI FYI @mikaelarguedas I rebased with master. It means |
@mikaelarguedas PTAL |
rclpy/rclpy/__init__.py
Outdated
if the global executor has a partially completed coroutine. | ||
|
||
:param node: A node to add to the executor to check for work. | ||
:param timeout_sec: Maximum time in seconds to wait for work. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this blocking forever if negative timeout provided? Is the default behavior to wait forever as well? (I guess it's yes in both questions, but it may be worth mentioning it in the docblock
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup. I copy/pasted the timeout_sec documentation from executor.spin_once()
in 93e42b5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
This fixes a bug where coroutine callbacks that yield while being handled in
rclpy.spin_once()
never get executed again. The task is stored in the executor, but the executor is thrown away after spinning.This PR makes the global methods reuse an executor. The test checks that a task which yields during
rclpy.spin_once()
gets resumed on a second invocation.CI
rcl.TestGetNodeNames__rmw_fastrtps_cpp.test_rcl_get_node_names
--parallel