-
Notifications
You must be signed in to change notification settings - Fork 170
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 ros2 service info
command
#703
Conversation
Signed-off-by: Joe Schornak <[email protected]>
@schornakj 👍 IMO, this option is worth to keep. i will have a look. |
I'm seeing failures in the new unit tests when I run them locally that I think are caused by the output text getting mangled when it's parsed. Not sure how to resolve that right now, since these tests follow the same pattern as the other ones that deal with printed output.
|
@fujitatomoya Do you think it's possible for me to get this in before the feature freeze for Humble? Right now, CI testing is failing for reasons that seem unrelated to the content of this PR, so I'm not sure how to approach resolving that. |
No, sorry. The feature freeze was already yesterday; we are now feature and API complete for Humble. That said, this is still valuable to get in (once freeze is lifted), and if it doesn't break API we can consider backporting it once Humble is released. |
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 pending CI and tests passing.
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.
ros2cli implementation conflicts with #771, probably we can merge them into one implementation, especially test scenario.
parser.add_argument( | ||
'-t', '--show-types', action='store_true', | ||
help='Additionally show the service type') | ||
parser.add_argument( | ||
'-c', '--count', action='store_true', | ||
help='Only display the number of service clients and service servers') |
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.
Since this is info
command, these should be always printed?
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.
ros2 topic info
command always prints type and count, there is not even opt-out
option.
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.
if possible, It seems good to be consistent in the same format and option. (https://docs.ros.org/en/rolling/Tutorials/Beginner-CLI-Tools/Understanding-ROS2-Topics/Understanding-ROS2-Topics.html#ros2-topic-info)
service_servers = [] | ||
|
||
expanded_name = expand_topic_name(service_name, node.get_name(), node.get_namespace()) | ||
validate_full_topic_name(expanded_name) |
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.
validate_full_topic_name(expanded_name) | |
validate_full_topic_name(expanded_name,is_service=True) |
i think we can come back to this PR after #771 is merged. |
Yay, 771 is merged! This is a great feature and would be very handy when connecting non-ROS DDS systems to ROS based systems. Introspection is great, especially with a |
I think we can close this PR, mostly has been done with #771. @schornakj what do you think? if you have the support |
#877 created to follow up |
Adds a new
info
verb toros2 service
, which lists the nodes that advertise clients and servers for a given service. I found that I needed a tool like this to help introspect big collections of ROS nodes, especially since some problems are really difficult to diagnose without it (for example, two nodes having service servers with the same type and topic name).The pattern of implementation is similar to
ros2 action info
, and it produces similar output:I modified the tests for
ros2service
to include a service client node in addition to the existing service server node. This required some changes to tests for other CLI tools since they have built-in assumptions about the number of nodes being run.Signed-off-by: Joe Schornak [email protected]