-
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
Add timeout to executor_spin_until_future_complete #301
Add timeout to executor_spin_until_future_complete #301
Conversation
Signed-off-by: vinnamkim <vinnam.kim@gmail.com>
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.
Thanks for the addition!
rclpy/test/test_executor.py
Outdated
|
||
def timer_callback(): | ||
pass | ||
timer = self.node.create_timer(0.1, timer_callback) |
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.
Why do we need to create a timer? Is this so that the underlying spin_once
will return (ie not timeout) and we can check the future? Seems like the spin_until_future_complete logic could be improved by not making it rely on other callbacks. This type of improvement does not have to happen in this PR since it looks like it has always been this way.
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.
Right, spin_until_future_complete has underlying spin_once
needed to return like rclcpp.
Signed-off-by: vinnamkim <vinnam.kim@gmail.com>
a06cbca
to
933049a
Compare
@jacobperron Thanks for review. updated commit. |
Signed-off-by: vinnamkim <vinnam.kim@gmail.com>
@vinnamkim Sorry, a recent merge to master seems to have caused conflicts. For future, a rebase instead of a merge should help avoid this. After the conflict is resolved I'll merge this PR, thanks! |
Fix #261.
Signed-off-by: vinnamkim vinnam.kim@gmail.com