Rclcpp: Implement functions to get publisher and subcription information from topic name

[mm318]

Feature request

Feature description

Add an API for obtaining participant information, like QoS policies, of the publishers and subscriptions on a topic name.

Implementation considerations

This is essentially implementing for rclcpp what was done for rclpy in https://github.com/ros2/rclpy/pull/454, which can be used as a reference.

[Barry-Xu-2018]

@mm318

Do you start to implement this function ?
If not, I am interesting to implement it.

[suab321321]

@ivanpauno @wjwwood sir I would to contribute to this.

[mm318]

We have not started implementation. From our perspective, it is still open for anyone to implement. Thanks!

[wjwwood]

I feel like this should be implemented as part of https://github.com/ros2/rmw/pull/186, ideally by the original authors.

I'm still planning on reviewing that set of pull requests, and that would be my first bit of feedback, that we shouldn't merge those until an rclcpp API is proposed. If you guys (@jaisontj and friends) want to work with @Barry-Xu-2018, @suab321321, or others that's fine, but please iterate with them before asking for merge on the other pull requests.

[mm318]

The proposed API would just be the addition of get_publishers_info_by_topic() and get_subscriptions_info_by_topic() member functions under the rclcpp::Node class.

I think the most we can do additionally as part of https://github.com/ros2/rmw/pull/186 is to stub out these functions in rclcpp, otherwise the set of PRs is becoming unwieldy.

[wjwwood]

The proposed API would just be the addition of get_publishers_info_by_topic() and get_subscriptions_info_by_topic() member functions under the rclcpp::Node class.

I think it should be at least in the NodeGraphInterface (https://github.com/ros2/rclcpp/blob/master/rclcpp/include/rclcpp/node_interfaces/node_graph_interface.hpp), but ideally it should be symmetric to the rclpy changes, including using C++ data structures where appropriate (rather than using the C-style rmw ones).

[ivanpauno]

I'm still planning on reviewing that set of pull requests, and that would be my first bit of feedback, that we shouldn't merge those until an rclcpp API is proposed.

I think that the rclcpp API is pretty strightforward from the rcl API, is just adapting the data from C style data structures to cpp style. If the proposed rcl API is fine, I wouldn't block more the downstream PRs.

[wjwwood]

I think that the rclcpp API is pretty strightforward from the rcl API, is just adapting the data from C style data structures to cpp style. If the proposed rcl API is fine, I wouldn't block more the downstream PRs.

Sure, but it needs to be done. I don't want to have "oh we'll do the rclcpp one later if there's time" and then it never gets done. It needs to be done directly afterwards and not left up to someone else or only if there's timee. rclcpp and rclpy should not diverge, and we need to be stricter about that as we go forward. The best way to ensure it gets done is to require it before merging the rmw changes. So I would prefer to see the rclcpp API before merging the rmw changes.

[ivanpauno]

Sure, but it needs to be done. I don't want to have "oh we'll do the rclcpp one later if there's time" and then it never gets done. It needs to be done directly afterwards and not left up to someone else or only if there's timee. rclcpp and rclpy should not diverge, and we need to be stricter about that as we go forward. The best way to ensure it gets done is to require it before merging the rmw changes. So I would prefer to see the rclcpp API before merging the rmw changes.

Yes I agree that requiring the rclcpp PR before merging the others is a good way to ensure that it's done. I also think that implementing a feature that has implications from the rmw implementations to the clients libraries is really being a lot of work, and getting some things merged first might do the rest of the work more "fluent". IDK what weights more.

Maybe the developer guide should be more clear about this. I also don't find clear when a design document is required or not. Maybe for all changes that add a new feature (like this) a design document should be required, stating the API changes in each layer.

[wjwwood]

I would feel more comfortable merging everything but the rclpy changes, then require rclcpp be merged with it, if the concern is really to split the work up over several pull requests. What I do not want to allow is divergence between the C++ and Python client libraries.

I don't care for the idea of merging changes to the API which are not being used yet (e.g. merging API changes to rmw or rcl before rclpy/rclcpp/examples), but I prefer that to having rclpy and rclcpp fall out of sync.

Whether or not you need a design document isn't clearly defined. Really the design document is meant to allow you to convey to others what you want to do. You may need it to convince others or solicit feedback or to document the design and alternatives for the future, but if you don't feel you need any of those or no one asks for it, then I guess you can get away without it.

[prajakta-gokhale]

@wjwwood we have not prioritized the implementation work for this right now, but we will try to soon. In the meantime, we think that the feature changes that have been approved can be merged in a staged fashion to avoid having rclpy and rclcpp go out of sync:

1. Merge all changes from rcl package downwards, including rmw and rmw_*
2. Work on rclcpp changes and merge them together with rclpy like you suggest
3. Merge ros2cli and make the complete feature available to users

Does that sound reasonable to you?

[wjwwood]

Yeah, if they _must_ be broken into parts, then that's fine with me. That's what I was suggesting when I said:

I would feel more comfortable merging everything but the rclpy changes, then require rclcpp be merged with it, if the concern is really to split the work up over several pull requests.

But you stated it much more clearly.

[prajakta-gokhale]

We'd prefer it being broken into parts so that it's easy for us to track progress on open PRs. This feature totals 10 PRs right now out of which the following can be merged:

• https://github.com/ros2/rmw_implementation/pull/72, https://github.com/ros2/rmw/pull/186
• https://github.com/ros2/rmw_fastrtps/pull/336, https://github.com/ros2/rmw_connext/pull/377, https://github.com/ros2/rmw_opensplice/pull/289, https://github.com/ros2/rmw_cyclonedds/pull/81
• https://github.com/ros2/rcl/pull/511

[Barry-Xu-2018]

ros2/rmw_implementation#72, ros2/rmw#186
ros2/rcl#511

Based on above commits, I have implemented rclcpp function.

ros2/rmw_implementation#72, ros2/rmw#186
ros2/rmw_fastrtps#336, ros2/rmw_connext#377, ros2/rmw_opensplice#289, ros2/rmw_cyclonedds#81
ros2/rcl#511

After above 10 PRs are merged, please review my commit #960. (@ivanpauno @mm318 @prajakta-gokhale)

[wjwwood]

I reviewed the pull requests up to rcl. I had one or two high level questions which may result in us wanting to refactor a bit, but I'm not sure about them.

Other than that I had one technical thing that needed to be fixed and some comments/style nitpicks. Otherwise it looks good to me, and we would be ready to next actually implement this for rmw_connext (as it is our other tier 1 rmw implementation) and start preparing for the stuff above rcl.

[Barry-Xu-2018]

@wjwwood

okay. After ros2/rcl#511 is complete, I will update my commit based on new change.

[Barry-Xu-2018]

ros2/rcl#511 had been merged.
I continue to update codes based on comments from @ivanpauno and @mm318.

[mm318]

This issue has been resolved with the completion of https://github.com/ros2/rclcpp/pull/960.