[WIP] Add functions to return formatted Node Name-Namespace strings #669

jhdcs · 2019-03-26T21:25:17Z

Implements functions to add formatted node name and namespace strings. Still needs unit tests. Design tips and suggestions for useful tests is greatly appreciated!

Signed-off-by: Jacob Hassold jhassold@dcscorp.com

Connects to #643

Signed-off-by: Jacob Hassold <jhassold@dcscorp.com>

wjwwood

Thanks for opening the pull request. As I said in this description:

#643 (comment)

I think changing get_node_names() to return the fully formatted node name + namespace (a.k.a. the fully qualified node name), along with a way to get them separate (like get_node_names_and_namespaces()), is all we want to do.

rclcpp/include/rclcpp/node.hpp

…pace method Signed-off-by: Oswin So <oswinso@gmail.com>

Signed-off-by: Jacob Hassold <jhassold@dcscorp.com>

jhdcs · 2019-04-01T17:46:17Z

@wjwwood I believe we've addressed the issues that you raised. Any further things that jump out at you?

Also, CLion is warning me of a bunch of Clang violations as I'm going through the code. Should these be cleaned up? Should we make an issue for them? Or should I ignore them?

wjwwood

Make sure you run the tests that our buildfarm would run to help us catch issues early, as CI turn around is pretty long, see: https://index.ros.org/doc/ros2/Tutorials/Colcon-Tutorial/#run-tests

rclcpp/src/rclcpp/node_interfaces/node_graph.cpp

rclcpp/test/test_node.cpp

jhdcs · 2019-04-02T11:48:21Z

Make sure you run the tests that our buildfarm would run to help us catch issues early, as CI turn around is pretty long, see: https://index.ros.org/doc/ros2/Tutorials/Colcon-Tutorial/#run-tests

I've been using "colcon test --packages-select rclcpp" from the root of my development workspace. Is this not correct? I do get several errors about style formatting left around the entire rclcpp package, which was part of the reason I was wondering if you wanted me to go and give the code a good scrubbing.

EDIT I think I found what you were hinting at. I'm now adjusting my IDE to conform to ROS 2 specifications for code style.

Signed-off-by: Jacob Hassold <jhassold@dcscorp.com>

see: https://google.github.io/styleguide/cppguide.html#Vertical_Whitespace Signed-off-by: William Woodall <william@osrfoundation.org>

wjwwood · 2019-04-02T20:19:02Z

I do get several errors about style formatting left around the entire rclcpp package, which was part of the reason I was wondering if you wanted me to go and give the code a good scrubbing.

That's odd, on master it should produce no errors or failures, so any failures should be due to your diff or maybe some misconfiguration on your system (wrong or missing test dependencies?).

We'll see what CI says:

Linux
Linux-aarch64
macOS
Windows

jhdcs · 2019-04-02T20:32:16Z

I should clarify - the IDE I'm using (CLion) is giving Clang style warnings. They're more like guidelines that something looks odd, rather than an actual error.

Apologies for the confusion, I misspoke.

jhdcs · 2019-04-05T14:06:37Z

Probably a silly question (I'm new to this), but is there anything I should be doing? The CI is saying that the change was on "Force Reinstallation of Numpy", am I reading it incorrectly?

dirk-thomas · 2019-04-05T14:10:27Z

The CI is saying that the change was on "Force Reinstallation of Numpy", am I reading it incorrectly?

That is the latest commit in the ros2/ci repo which provides the logic for the CI jobs.

jhdcs · 2019-04-05T14:29:10Z

So that change pertains to the CI system, rather than my code. Got it.

So the instability is being caused by something I wrote? I guess I'd better start diving again...

Signed-off-by: Jacob Hassold <jhassold@dcscorp.com>

wjwwood · 2019-04-05T21:10:51Z

@jhdcs let me know when you want me to rerun CI, the lastest round has test failures that look related to these changes (based on a cursory look only).

jhdcs · 2019-04-08T12:33:51Z

Yeah. I'm thinking that something might have been expecting the old format. Possibly. I'll let you know once I figure it out on my end.

jhdcs · 2019-04-09T15:34:28Z

Okay, I think I know what's going on.

NodeGraph::get_node_names() is used throughout the API, and the tests seem to expect an unqualified name (i.e: name, but no namespace). I can either update all the tests to accept the new version of get_node_names, or revert get_node_names to its original functionality, and create a new function called NodeGraph::get_qualified_node_names() that implements our desired functionality.

I'm personally partial to the second option, both because it's easier, and because I'm not sure how much of the existing codebase is going to break if we suddenly swap the expected output of an already-in-use function.

@wjwwood, what do you think?

Signed-off-by: Jacob Hassold <jhassold@dcscorp.com>

jhdcs · 2019-04-11T12:47:25Z

@wjwwood Would you be able to re-run CI? I think I might have gotten some things fixed, though I did find something interesting: Apparently if you use std::transform in the original get_node_names() function, you get a timeout error in some of the tests (specifically, gtest_multithreaded_rmw_fastrtps/opensplice, and at least on my system). As such, I've reverted it to the for-based implementation until I can figure out what on earth is going on...

wjwwood · 2019-04-11T22:56:41Z

@jhdcs please revert you most recent commit, the idea is to change the behavior of get_node_names(), because it was classified as a bug that it did not include the namespace from the beginning, and update any tests we have that need to be. The node name needs to always be associated with the namespace, otherwise you may have duplicate node names and no way to disambiguate them.

Please (re)read my suggested solution in the original issue: #643 (comment)

It is to update the behavior of the existing function (and fix anything that breaks due to that) and optionally add a function that returns a list of namespace and names as pairs.

…_names" This reverts commit ed4b166. Signed-off-by: Jacob Hassold <jhassold@dcscorp.com>

Signed-off-by: Jacob Hassold <jhassold@dcscorp.com>

wjwwood · 2019-04-13T00:09:22Z

@jhdcs looks like something got messed up when you force pushed, and now you have changes from master on this branch...

jhdcs · 2019-04-15T11:26:50Z

GAH!

It's just one of those weeks, isn't it?

jhdcs · 2019-04-15T11:38:03Z

Hopefully that fixes it. Sorry for all the trouble, and I really appreciate your guys' patience...

I had to re-create the branch, it seems. I'll get the code actually working, then open a new pull request.

wjwwood · 2019-04-16T00:59:33Z

Actually now the pull request is closed and I cannot reopen it... This usually happens if you delete the upstream branch or fork, but I'm not sure what's happening right now...

jhdcs · 2019-04-16T12:02:24Z

That's basically what happened. Part of the fork was deleted accidentally due to my uncanny ability to mega-derp. No data was lost, but GitHub is really confused.

I was going to open a new pull request that references both this one and the main issue later on, once I feel like I've worked a few more of the kinks out. Unless you want me to open it now, of course.

wjwwood · 2019-04-16T23:48:19Z

I was going to open a new pull request that references both this one and the main issue later on, once I feel like I've worked a few more of the kinks out. Unless you want me to open it now, of course.

Oh that's up to you. Feel free to open a new pr when ever you're ready. I just wanted to make sure you knew I could not reopen this one.

Add functions to return formatted Node Name-Namespace strings

911c349

Signed-off-by: Jacob Hassold <jhassold@dcscorp.com>

tfoote added the in review Waiting for review (Kanban column) label Mar 26, 2019

wjwwood requested changes Mar 26, 2019

View reviewed changes

rclcpp/include/rclcpp/node.hpp Outdated Show resolved Hide resolved

rclcpp/include/rclcpp/node.hpp Outdated Show resolved Hide resolved

wjwwood mentioned this pull request Mar 26, 2019

get_node_names should include namespace #643

Closed

Changed get_node_names to return fully qualified names, removed names…

3c03182

…pace method Signed-off-by: Oswin So <oswinso@gmail.com>

oswinso force-pushed the issue_643 branch from 1c233c1 to 3c03182 Compare March 28, 2019 03:40

Jacob Hassold added 3 commits April 1, 2019 10:34

Removed unnecessary capture-by-reference

06c4ab6

Signed-off-by: Jacob Hassold <jhassold@dcscorp.com>

Added first draft of tests

38fb7f4

Signed-off-by: Jacob Hassold <jhassold@dcscorp.com>

Fixed bug creating phantom empty name/namespaces

97e915f

Signed-off-by: Jacob Hassold <jhassold@dcscorp.com>

wjwwood reviewed Apr 2, 2019

View reviewed changes

rclcpp/src/rclcpp/node_interfaces/node_graph.cpp Outdated Show resolved Hide resolved

rclcpp/src/rclcpp/node_interfaces/node_graph.cpp Outdated Show resolved Hide resolved

rclcpp/test/test_node.cpp Outdated Show resolved Hide resolved

Jacob Hassold and others added 6 commits April 2, 2019 07:54

Re-ordered includes

b0e29c7

Signed-off-by: Jacob Hassold <jhassold@dcscorp.com>

Swap checks to see if name is in set

36c39d4

Signed-off-by: Jacob Hassold <jhassold@dcscorp.com>

Fixed style errors from uncrustify

975fc48

Signed-off-by: Jacob Hassold <jhassold@dcscorp.com>

Swapped to unordered_set

85100af

Signed-off-by: Jacob Hassold <jhassold@dcscorp.com>

Re-ordered includes again

83c1f9f

Signed-off-by: Jacob Hassold <jhassold@dcscorp.com>

nitpick: minimize vertical whitespace

48a9b86

see: https://google.github.io/styleguide/cppguide.html#Vertical_Whitespace Signed-off-by: William Woodall <william@osrfoundation.org>

wjwwood approved these changes Apr 2, 2019

View reviewed changes

Add API documentation for added get_node_names function

91fd106

Signed-off-by: Jacob Hassold <jhassold@dcscorp.com>

Merge remote-tracking branch 'remotes/origin/master' into issue_643

ce5384b

Moved new code to get_qualified_node_names, reverted get_node_names

ed4b166

Signed-off-by: Jacob Hassold <jhassold@dcscorp.com>

Revert "Moved new code to get_qualified_node_names, reverted get_node…

c39cf74

…_names" This reverts commit ed4b166. Signed-off-by: Jacob Hassold <jhassold@dcscorp.com>

jhdcs force-pushed the issue_643 branch from 620c90e to c39cf74 Compare April 12, 2019 12:09

Reverted get_node_names, removed .idea folder

2116b8f

Signed-off-by: Jacob Hassold <jhassold@dcscorp.com>

jhdcs force-pushed the issue_643 branch from d183b52 to 2116b8f Compare April 12, 2019 13:10

wjwwood added in progress Actively being worked on (Kanban column) and removed in review Waiting for review (Kanban column) labels Apr 13, 2019

jhdcs closed this Apr 15, 2019

jhdcs deleted the issue_643 branch April 15, 2019 11:34

tfoote removed the in progress Actively being worked on (Kanban column) label Apr 15, 2019

jhdcs mentioned this pull request Apr 17, 2019

[WIP / Re-Opened] Add functions to return formatted Node Name-Namespace strings #698

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Add functions to return formatted Node Name-Namespace strings #669

[WIP] Add functions to return formatted Node Name-Namespace strings #669

jhdcs commented Mar 26, 2019 •

edited by mjcarroll

Loading

wjwwood left a comment

jhdcs commented Apr 1, 2019

wjwwood left a comment

jhdcs commented Apr 2, 2019 •

edited

Loading

wjwwood commented Apr 2, 2019

jhdcs commented Apr 2, 2019

jhdcs commented Apr 5, 2019

dirk-thomas commented Apr 5, 2019

jhdcs commented Apr 5, 2019

wjwwood commented Apr 5, 2019

jhdcs commented Apr 8, 2019

jhdcs commented Apr 9, 2019

jhdcs commented Apr 11, 2019

wjwwood commented Apr 11, 2019

wjwwood commented Apr 13, 2019

jhdcs commented Apr 15, 2019

jhdcs commented Apr 15, 2019 •

edited

Loading

wjwwood commented Apr 16, 2019

jhdcs commented Apr 16, 2019

wjwwood commented Apr 16, 2019

[WIP] Add functions to return formatted Node Name-Namespace strings #669

[WIP] Add functions to return formatted Node Name-Namespace strings #669

Conversation

jhdcs commented Mar 26, 2019 • edited by mjcarroll Loading

wjwwood left a comment

Choose a reason for hiding this comment

jhdcs commented Apr 1, 2019

wjwwood left a comment

Choose a reason for hiding this comment

jhdcs commented Apr 2, 2019 • edited Loading

wjwwood commented Apr 2, 2019

jhdcs commented Apr 2, 2019

jhdcs commented Apr 5, 2019

dirk-thomas commented Apr 5, 2019

jhdcs commented Apr 5, 2019

wjwwood commented Apr 5, 2019

jhdcs commented Apr 8, 2019

jhdcs commented Apr 9, 2019

jhdcs commented Apr 11, 2019

wjwwood commented Apr 11, 2019

wjwwood commented Apr 13, 2019

jhdcs commented Apr 15, 2019

jhdcs commented Apr 15, 2019 • edited Loading

wjwwood commented Apr 16, 2019

jhdcs commented Apr 16, 2019

wjwwood commented Apr 16, 2019

jhdcs commented Mar 26, 2019 •

edited by mjcarroll

Loading

jhdcs commented Apr 2, 2019 •

edited

Loading

jhdcs commented Apr 15, 2019 •

edited

Loading