Implement functions to get publisher and subcription informations like QoS policies from topic name #960

Barry-Xu-2018 · 2020-01-07T07:41:56Z

Signed-off-by: Barry Xu [email protected]

Barry-Xu-2018 · 2020-01-07T07:44:32Z

This commit is for #943.

rclcpp/include/rclcpp/node.hpp

rclcpp/include/rclcpp/node_interfaces/node_graph_interface.hpp

rclcpp/src/rclcpp/node_interfaces/node_graph.cpp

ivanpauno · 2020-01-08T12:54:14Z

rclcpp/src/rclcpp/node_interfaces/node_graph.cpp

+    topic_name,
+    rcl_node_get_name(rcl_node_handle),
+    rcl_node_get_namespace(rcl_node_handle),
+    false);    // false = not a service


I wonder if this should be done in rcl layer.
@mm318 @hidmic opinions?

What specifically did you mean should be done in the rcl layer? This whole function?

Is rclcpp::expand_topic_or_service_name() getting the fully qualified name of topic_name, and there's an equivalent in rcl?

What specifically did you mean should be done in the rcl layer? This whole function?

Yes.

Is rclcpp::expand_topic_or_service_name() getting the fully qualified name of topic_name, and there's an equivalent in rcl?

Is getting a name and converting it to a "fully qualified name" (actually, it's just expanding, remappings are missing).
In rcl, there is a function that does the expand, and another that does the remapping. That combination is repeated in many place in the codebase. I think it would be good to combine the whole expand and remap logic in one function (a is_service flag will be needed).

Most rcl functions, like create_publisher, accept a topic name that's then expanded and remapped.
The only exception I know is rcl_count_publishers, which requires a fully qualified name.
The rclcpp counterpart doesn't requires a fully qualified name, but does the expansion (and, AFAIS the remapping part is missing). I wonder if both the expansion and remapping should be done in rcl, which would improve code reusage.

I wonder if both the expansion and remapping should be done in rcl, which would improve code reusage.

Yeah, my first thought was that it's surprising that there's a expand_topic_or_service_name()
in rclcpp. I thought it was going to be at most just a thin wrapper around something in rcl.

I agree that "rclcpp::expand_topic_or_service_name()" should not at rclcpp layer.
rclcpp::expand_topic_or_service_name() not only expand the name, but to check if the name is valid and then throw exception of rclcpp.
If not calling "rclcpp::expand_topic_or_service_name()" at rclcpp layer, I think rcl should check name at each interfaces.
and then rclcpp check the returned value from rcl to throw rclcpp itself exception.
Maybe need a new issue to handle this.

So to answer @ivanpauno's question

I wonder if this should be done in rcl layer.

Yeah, I think so, but it's outside the scope of this PR.

Sounds reasonable, the changes are broader that just this new functions.

Is this resolved?

Following my comment in the conversation about the line just above, I think we need to decide if it is needed or not, and if it is needed then we should probably do it in rcl if we can, but I agree we could do it in a follow up. However, I would like to have a decision on what we should do before continuing here.

I think there's a couple of things going on here.

rclcpp::expand_topic_or_service_name() does topic name expansion and validation, and does not do remapping.

rclpy.expand_topic_name.expand_topic_name() does topic name expansion, does not do validation (the caller is supposed to do it manually), and also does not do remapping.

(My understanding of remapping here is that the returned/output topic name is the remapped version of the input topic name.)

The follow-up should be to implement in rcl a function like rcl_get_expanded_and_remapped_topic_name() (and it also does validation). This function supposedly should be agnostic to the no_mangle parameter as well.

rclcpp/src/rclcpp/node_interfaces/node_graph.cpp

Barry-Xu-2018 · 2020-01-09T07:52:31Z

@ivanpauno

Thanks for your comments.
Since #943 (comment), I have to pend updating codes.
After rcl codes have been fixed, I will continue to update codes.

mm318 · 2020-01-14T18:10:58Z

The set of pull requests remaining in this discussion is:

Implement functions to get publisher and subcription informations like QoS policies from topic name #960
Implement functions to get publisher and subcription informations like QoS policies from topic name rclpy#454
Enhanced ros2 topic info to display node name, node namespace, topic type and qos profile of the publishers and subscribers. ros2cli#385

mm318 · 2020-01-14T18:48:40Z

Hi @Barry-Xu-2018, can you add me as a collaborator to https://github.com/Barry-Xu-2018/rclcpp?

rclcpp/include/rclcpp/node_interfaces/node_graph.hpp

rclcpp/include/rclcpp/node_interfaces/node_graph_interface.hpp

rclcpp/src/rclcpp/node_interfaces/node_graph.cpp

mm318 · 2020-01-14T19:52:06Z

rclcpp/src/rclcpp/node_interfaces/node_graph.cpp

+    topic_name,
+    rcl_node_get_name(rcl_node_handle),
+    rcl_node_get_namespace(rcl_node_handle),
+    false);    // false = not a service


What specifically did you mean should be done in the rcl layer? This whole function?

Is rclcpp::expand_topic_or_service_name() getting the fully qualified name of topic_name, and there's an equivalent in rcl?

rclcpp/src/rclcpp/node_interfaces/node_graph.cpp

rclcpp/include/rclcpp/node_interfaces/node_graph.hpp

ivanpauno · 2020-01-14T20:38:51Z

rclcpp/src/rclcpp/node_interfaces/node_graph.cpp

+    topic_name,
+    rcl_node_get_name(rcl_node_handle),
+    rcl_node_get_namespace(rcl_node_handle),
+    false);    // false = not a service


What specifically did you mean should be done in the rcl layer? This whole function?

Yes.

Is rclcpp::expand_topic_or_service_name() getting the fully qualified name of topic_name, and there's an equivalent in rcl?

Is getting a name and converting it to a "fully qualified name" (actually, it's just expanding, remappings are missing).
In rcl, there is a function that does the expand, and another that does the remapping. That combination is repeated in many place in the codebase. I think it would be good to combine the whole expand and remap logic in one function (a is_service flag will be needed).

Most rcl functions, like create_publisher, accept a topic name that's then expanded and remapped.
The only exception I know is rcl_count_publishers, which requires a fully qualified name.
The rclcpp counterpart doesn't requires a fully qualified name, but does the expansion (and, AFAIS the remapping part is missing). I wonder if both the expansion and remapping should be done in rcl, which would improve code reusage.

rclcpp/src/rclcpp/node_interfaces/node_graph.cpp

Barry-Xu-2018 · 2020-01-15T08:46:34Z

@mm318

Hi @Barry-Xu-2018, can you add me as a collaborator to https://github.com/Barry-Xu-2018/rclcpp?

OK. I added you.

Barry-Xu-2018 · 2020-01-15T09:41:08Z

@ivanpauno @mm318

I have updated codes based on your comments.
Please review again.

fujitatomoya

looks good to me.

rclcpp/src/rclcpp/node_interfaces/node_graph.cpp

Barry-Xu-2018 · 2020-01-17T08:11:15Z

@mm318 @ivanpauno @fujitatomoya

Sorry. Next I am going to take Chinese New Year holiday. During holiday, maybe I cannot response your comments. I will be back on Feb 1st.

mm318 · 2020-01-17T08:19:31Z

Sorry. Next I am going to take Chinese New Year holiday. During holiday, maybe I cannot response your comments. I will be back on Feb 1st.

I should be able to address comments since I'm a collaborator.

Have a great holidays!

Barry-Xu-2018 · 2020-01-17T08:25:34Z

Sorry. Next I am going to take Chinese New Year holiday. During holiday, maybe I cannot response your comments. I will be back on Feb 1st.

I should be able to address comments since I'm a collaborator.

Have a great holidays!

Thank you :)

rclcpp/include/rclcpp/node.hpp

rclcpp/include/rclcpp/node_interfaces/node_graph_interface.hpp

ivanpauno · 2020-01-17T17:42:25Z

rclcpp/src/rclcpp/node_interfaces/node_graph.cpp

+    topic_name,
+    rcl_node_get_name(rcl_node_handle),
+    rcl_node_get_namespace(rcl_node_handle),
+    false);    // false = not a service


Sounds reasonable, the changes are broader that just this new functions.

rclcpp/src/rclcpp/node_interfaces/node_graph.cpp

mm318 · 2020-01-18T17:56:16Z

rclcpp/src/rclcpp/node_interfaces/node_graph.cpp

+  std::vector<rclcpp::TopicEndpointInfo> topic_info_list;
+
+  auto rcl_node_handle = node_base->get_rcl_node_handle();
+  auto fqdn = rclcpp::expand_topic_or_service_name(


I'm curious, how does expand_topic_or_service_name() work without a no_mangle parameter?

So, one thing is mangling, i.e. generate a valid middleware name (e.g. DDS) from the ROS name.
Another thing is expansion, i.e. get full names from relative names ('topic' -> '/namespace/topic/') and full names from private names ('~/topic' -> '/namespace/node_name/topic/').
Remapping should be always done after expansion, but it's missing here (can be added later, as it's a preexisting problem in get_publisher_count too).

We're everywhere always doing remapping and expansion, including when we do no_mangle=true (or when this qos setting is set to true). IMO, always doing remapping and expansion is ok, though is arguable. Considering that, expand_topic_or_service_name doesn't need to know about no_mangle.

Has this been resolved?

I think we need to get a holistic summary of how we think no_mangle should be used with each of these functions, and then make sure the implementation matches. I feel like there have been several such conversations and potentially different solutions in each case. Also I haven't heard a compelling story for why one way or the other.

I would prefer to see clarity on this point before moving forward, even if that means we need to follow up after this pr and fix other functions.

Another thing is expansion, i.e. get full names from relative names ('topic' -> '/namespace/topic/') and full names from private names ('~/topic' -> '/namespace/node_name/topic/').

I meant to confirm if the expansion algorithm will work for a middleware topic names well? For example rt/topic -> rt/namespace/topic. (And I guess rt/~/topic is an invalid topic name regardless of the value of no_mangle.)

My understanding is that this is indeed the latter case: if no_mangle==true, then the input into this get_info_by_topic() function and subsequently the expand_topic_or_service_name() function is to be treated as a middleware topic name.

I don't have arguments in favor of applying remapping and expansion when using no_mangle=true.

In the latter case, I think that expanding the input may not be desired, but I could be wrong.

Then it sounds like no_mangle==true does imply that the input topic_name parameter will need to be fully qualified? (Though we've been removing statements about that from the docstrings.)

@ivanpauno summarized our discussion here:

ros2/ros2#862 (comment)

Basically we want all of the graph query functions (including this one) to do both expansion (foo becomes /node_ns/foo) and remapping, by default. And then we want them all to have an option to skip both expansion and remapping.

Also, any functions that return topic or type names should be able to avoid de-mangling of those names.

Currently this implementation does expansion but not remapping (as far as I can tell), and it has an option no_mangle to make the query assume the input topic should be considered as is, without any additional prefixes or other mangling. It does not, however, have a no_demangle, which some of the other functions do have.

This no_mangle option does not prevent the topic name expansion here, which is a problem in my opinion, what do you think @ivanpauno?

As I see it we need to do these things to get this merged:

add remapping of the input topic name to this function

avoid expansion and remapping when no_mangle is specified

Later we should make a set of pull requests which add a no_demangle option to these query functions. This will affect all the layers unfortunately.

We should also make sure the rclpy version of this matches.

Basically we want all of the graph query functions (including this one) to do both expansion (foo becomes /node_ns/foo) and remapping, by default. And then we want them all to have an option to skip both expansion and remapping.

I completly agree with this.
I think it should be done in rcl, repeating the code in rclcpp and rclpy doesn't make much sense.

Also, any functions that return topic or type names should be able to avoid de-mangling of those names.

Currently, the topic types are being demangled or not based on no_mangle argument.
Considering the use cases mentioned in the issue, I think that's fine.
We could have a better name, like use_ros_conventions, default to true:

true means both input and output are ros names.

false means both input and output are dds names (input shouldn't be expanded/remapped/mangled, output shouldn't be demangled).

This no_mangle option does not prevent the topic name expansion here, which is a problem in my opinion, what do you think @ivanpauno?

Yeah, as we talked, I think it's a problem.

We can left this as-is for now, and then correct all the places where we have this error together (publisher/subscription creation is applying remapping and expansion when ros_conventions qos is false , publisher/subscription count is not doing remapping, etc). I don't feel strongly about which path to take.

I think it should be done in rcl, repeating the code in rclcpp and rclpy doesn't make much sense.

In principle yes, but actually I think the sub-nodes feature in rclcpp prevent this for now. It needs to be more carefully considered I think. I would postpone that to another pull request.

Currently, the topic types are being demangled or not based on no_mangle argument.
Considering the use cases mentioned in the issue, I think that's fine.
We could have a better name, like use_ros_conventions, default to true: ...

I think having just one option can be ok, though it does make the odd cases impossible, because there's no way to manually do mangling/demangling.

If we want to go the route of having one option for both behaviors, then we need to change the no_mangle name, as it doesn't cover the existing behavior correctly, imo.

We can left this as-is for now, and then correct all the places where we have this error together (publisher/subscription creation is applying remapping and expansion when ros_conventions qos is false , publisher/subscription count is not doing remapping, etc). I don't feel strongly about which path to take.

I don't think the two points I highlighted (add remapping and avoid expansion/remapping when no_mangle is true) are very difficult so I'd prefer those to be done now. Everything else can be done in follow up.

Signed-off-by: Miaofei <[email protected]>

rclcpp/src/rclcpp/node_interfaces/node_graph.cpp

Signed-off-by: Miaofei <[email protected]>

ivanpauno · 2020-02-10T14:06:14Z

CI run here.

mm318 · 2020-02-11T01:37:12Z

Hi @ivanpauno, any advice on how to fix the Windows CI failure in rclcpp at https://ci.ros2.org/job/ci_windows/9214/? It looks like std::string members needs to be exported? Has this (or for member variables that are some other type of std container) been done elsewhere in rclcpp before?

EDIT: Or is the recommendation to not have std container members that are public, but have them accessed via getter/setter methods instead?

EDIT2: Oh, the error was actually only in rclpy, which is now fixed. The compiler messages in rclcpp are actually just warnings, but the same question applies since these warnings should be fixed.

ivanpauno · 2020-02-11T16:57:27Z

@mm318 I've read a bit, and this seems to be a problem when a member variable of an exported class is template defined in another library:

Considering that we already have a test that compiles and access those member variables without problems, I would just ignore the warning with a pragma. I don't think that would be an issue.
Another option is switching from a plain struct to a class with setters and getters.

@wjwwood opinions?

rclcpp/include/rclcpp/node_interfaces/node_graph_interface.hpp

Signed-off-by: Miaofei <[email protected]>

mm318 · 2020-02-11T21:56:38Z

In my opinion, this is ready for a CI re-run, @ivanpauno.

wjwwood

I guess this is probably for the best, though it is a lot of boilerplate (welcome to C++ :p).

ivanpauno · 2020-02-12T13:49:04Z

I think that the boilerplate is fine, but the windows warning seems to be non-sense. Ignoring it with a pragma would be fine too.

ivanpauno · 2020-02-12T13:54:51Z

New CI round: ros2/rclpy#454 (comment)

wjwwood · 2020-02-12T19:55:03Z

but the windows warning seems to be non-sense. Ignoring it with a pragma would be fine too.

I'm not sure that's true, but we're not doing that so it's fine.

ivanpauno · 2020-02-14T20:25:25Z

Merged! Thanks @Barry-Xu-2018 and @mm318 for pushing this!

Barry-Xu-2018 mentioned this pull request Jan 7, 2020

Implement functions to get publisher and subcription information from topic name #943

Closed

ivanpauno self-requested a review January 7, 2020 15:43

mm318 mentioned this pull request Jan 7, 2020

Adding required structs and methods to get a list of publishers or subscribers with their respective qos ros2/rmw#186

Merged

ivanpauno reviewed Jan 8, 2020

View reviewed changes

ivanpauno mentioned this pull request Jan 9, 2020

Implement functions to get publisher and subcription informations like QoS policies from topic name ros2/rcl#511

Merged

mm318 mentioned this pull request Jan 14, 2020

Stub functions to get publisher and subcription informations like QoS policies from topic name #959

Closed

ivanpauno added the in review Waiting for review (Kanban column) label Jan 14, 2020

mm318 reviewed Jan 14, 2020

View reviewed changes

ivanpauno reviewed Jan 14, 2020

View reviewed changes

mm318 reviewed Jan 14, 2020

View reviewed changes

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

fujitatomoya approved these changes Jan 16, 2020

View reviewed changes

mm318 mentioned this pull request Jan 16, 2020

Cleanup templated get_info_by_topic() function Barry-Xu-2018/rclcpp#2

Merged

mm318 reviewed Jan 16, 2020

View reviewed changes

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

Barry-Xu-2018 force-pushed the topic-get-topic-info branch 4 times, most recently from 08cce5d to c9e03c8 Compare January 17, 2020 02:23

mm318 approved these changes Jan 17, 2020

View reviewed changes

ivanpauno reviewed Jan 17, 2020

View reviewed changes

mm318 reviewed Jan 18, 2020

View reviewed changes

mm318 mentioned this pull request Jan 18, 2020

Fix some doc strings regarding no_mangle input parameter ros2/rcl#557

Merged

mm318 added 8 commits February 7, 2020 02:53

address some of the PR comments

c9ff531

Signed-off-by: Miaofei <[email protected]>

change rclcpp::EndpointType to an enum class

a79f8e4

Signed-off-by: Miaofei <[email protected]>

fix punctuation in docstring

4db2a31

Signed-off-by: Miaofei <[email protected]>

address more PR comments

67ca5b8

Signed-off-by: Miaofei <[email protected]>

add get_publishers/subscriptions_info_by_topic() to LifecycleNode

88af887

Signed-off-by: Miaofei <[email protected]>

remove weird punctuation from docstring

814117c

Signed-off-by: Miaofei <[email protected]>

add topic name remapping to get_info_by_topic()

634d588

Signed-off-by: Miaofei <[email protected]>

remove useless line of code

bc1b891

Signed-off-by: Miaofei <[email protected]>

mm318 force-pushed the topic-get-topic-info branch from c911445 to bc1b891 Compare February 7, 2020 10:53

ivanpauno approved these changes Feb 7, 2020

View reviewed changes

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

remove non-applicable comment

9911d24

Signed-off-by: Miaofei <[email protected]>

ivanpauno approved these changes Feb 7, 2020

View reviewed changes

wjwwood reviewed Feb 11, 2020

View reviewed changes

rclcpp/include/rclcpp/node_interfaces/node_graph_interface.hpp Outdated Show resolved Hide resolved

make members of TopicEndpointInfo private

cb064f2

Signed-off-by: Miaofei <[email protected]>

wjwwood approved these changes Feb 11, 2020

View reviewed changes

ivanpauno changed the title ~~get publisher and subscription information from topic name~~ Implement functions to get publisher and subcription informations like QoS policies from topic name Feb 14, 2020

ivanpauno merged commit 2d9c6ea into ros2:master Feb 14, 2020

This was referenced Feb 14, 2020

Add rcl_topic_endpoint_info_t and related types and functions ros2/rcl#558

Merged

Enhanced ros2 topic info to display node name, node namespace, topic type and qos profile of the publishers and subscribers. ros2/ros2cli#385

Merged

Implement functions to get publisher and subcription informations like QoS policies from topic name #960

Implement functions to get publisher and subcription informations like QoS policies from topic name #960

Uh oh!

Conversation

Barry-Xu-2018 commented Jan 7, 2020

Uh oh!

Barry-Xu-2018 commented Jan 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Barry-Xu-2018 commented Jan 9, 2020

Uh oh!

mm318 commented Jan 14, 2020

Uh oh!

mm318 commented Jan 14, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Barry-Xu-2018 commented Jan 15, 2020

Uh oh!

Barry-Xu-2018 commented Jan 15, 2020

Uh oh!

fujitatomoya left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Barry-Xu-2018 commented Jan 17, 2020

Uh oh!

mm318 commented Jan 17, 2020

Uh oh!

Barry-Xu-2018 commented Jan 17, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Barry-Xu-2018 commented Jan 7, 2020 •

edited

Loading

mm318 commented Feb 11, 2020 •

edited

Loading