-
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
Add ros2 node info verb #159
Conversation
Connected to rcl |
@ross-desmond PTAL at https://github.com/thomas-moulard/ros2cli/tree/node_graph_impl - this fixes issues with flake8 |
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.
A couple of minor comments, but once those are addressed, this will be ready with a green CI.
ros2node/ros2node/verb/info.py
Outdated
print('All nodes found:') | ||
print(*[n.full_name for n in node_names], sep=', ') | ||
print('Unable to find node ' + args.node_name) | ||
return |
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.
Fairly minor, but it looks like the return value from main
is used to determine whether the command "succeeded" or "failed". In this else
clause, we probably want to return 'Unable to find node'
so that it is a failure case.
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.
Awesome
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 ended up changing this again in eb3dc36 (sorry I wasn't more clear the first time). Two things: the command-line tools already print out whatever you return here, and the full list of node names is already available through ros2 node list -a
. Thus, I simplified this to just a return error message.
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 code generally looks fine to me now. However, when I go to run it, I am always getting a Segmentation fault (where it is moves around, but I always get it):
ubuntu@ros2-bionic:~/ros2_ws$ ros2 node list
/cam2image
ubuntu@ros2-bionic:~/ros2_ws$ ros2 node info /cam2image
/cam2image
Segmentation fault (core dumped)
I'm going to hold off approving until we get that figured out.
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 code generally looks OK to me now. However, I'm always getting a segmentation fault when running it now:
ubuntu@ros2-bionic:~/ros2_ws$ ros2 node list
/cam2image
ubuntu@ros2-bionic:~/ros2_ws$ ros2 node info /cam2image
/cam2image
Segmentation fault (core dumped)
I'll note that this is in all likelihood not in this code, but I'm going to hold off approving until we get it figured out.
OK, I figured out the segfault and fixed it in ros2/rclpy@dc8edb3 . So I'll approve this now. |
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
eb3dc36
to
08e7da6
Compare
|
||
def parse_node_name(full_node_name): | ||
tokens = full_node_name.split('/') | ||
if 1 > len(tokens): |
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.
Maybe we should have a guard against cases for empty string or '/'
instead? I think split
will always return at least one element, even if it is the empty string.
@@ -34,6 +34,7 @@ | |||
], | |||
'ros2node.verb': [ | |||
'list = ros2node.verb.list:ListVerb', | |||
'info = ros2node.verb.info:InfoVerb', |
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.
nit: perhaps this should be alphabetized
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.
My comments above are pretty minor and don't necessarily have to be addressed. LGTM!
Summary: Provide the info verb under the node command
ros2 node info /minimal_publisher
Example:
Connects to ros2/rcl#333