-
Notifications
You must be signed in to change notification settings - Fork 160
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
[ros2param] Add timeout to ros2param list #469
Conversation
Signed-off-by: Donghee Ye <donghee.ye@samsung.com>
@@ -87,7 +87,7 @@ def main(self, *, args): # noqa: D102 | |||
|
|||
# wait for all responses | |||
for future in futures.values(): | |||
rclpy.spin_until_future_complete(node, future) | |||
rclpy.spin_until_future_complete(node, future, timeout_sec=1.0) |
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.
not sure just ignoring timeout is good enough here. i would add check the FutureReturnCode, and if gets TIMEOUT, notify the user what node's parameter cannot be queried. i would like to hear from others.
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.
I agree, if there is a problem communicating with one of the parameter servers then we should warn the user, but I think we are getting some feedback with the "exception" printed at L104.
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.
The change looks reasonable to me. Thanks for the PR!
@@ -87,7 +87,7 @@ def main(self, *, args): # noqa: D102 | |||
|
|||
# wait for all responses | |||
for future in futures.values(): | |||
rclpy.spin_until_future_complete(node, future) | |||
rclpy.spin_until_future_complete(node, future, timeout_sec=1.0) |
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.
I agree, if there is a problem communicating with one of the parameter servers then we should warn the user, but I think we are getting some feedback with the "exception" printed at L104.
If the node use default node option ( start_parameter_services is ture) and use not spin() but spin_until_future_complete(), then ros2param list send a request to the node but the callback of parameter service list parameter request can not be called. and ros2param spin_until_future_complete can not be finished.