Conversation
|
Connected to rcl |
|
@ross-desmond PTAL at https://github.com/thomas-moulard/ros2cli/tree/node_graph_impl - this fixes issues with flake8 |
clalancette
left a comment
There was a problem hiding this comment.
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.
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.
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.
clalancette
left a comment
There was a problem hiding this comment.
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.
clalancette
left a comment
There was a problem hiding this comment.
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.
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.
| ], | ||
| 'ros2node.verb': [ | ||
| 'list = ros2node.verb.list:ListVerb', | ||
| 'info = ros2node.verb.info:InfoVerb', |
There was a problem hiding this comment.
nit: perhaps this should be alphabetized
jacobperron
left a comment
There was a problem hiding this comment.
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_publisherExample:
Connects to ros2/rcl#333