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

jaisontj · 2019-10-29T19:19:54Z

NOTE: DO NOT MERGE until rmw #186 and rmw_implementation #72 are merged.

This PR makes the necessary changes to implement this feature request. Relevant design discussions can be found here.

The changes can be summarized as follows:

Modified the TopicCache class (topic_cache.hpp) from holding a map of topic_name to a vector of topic_types to a map of topic_name to a vector of tuples (GUID_t, topic_type and qos_profile). The addTopic function was also modified to accept the QoS profile as an additional parameter.
Added tests for TopicCache
The qos_converter.hpp was moved from being a package-private header to being public as this needed to be accessed by topic_cache.hpp.
Implemented rmw_get_publishers_info_by_topic and rmw_get_subscriptions_info_by_topic inside rmw_fastrtps_cpp, rmw_fastrtps_dynamic_cpp and rmw_fastrtps_shared_cpp. These functions are defined in the rmw(PR) and rmw_implementation(PR) packages.

Related to issues in aws-roadmap #84 and #83

Closing #333 and #332 in favor of this PR.

Signed-off-by: Jaison Titus <jaisontj92@gmail.com>

topic_cache.hpp Signed-off-by: Jaison Titus <jaisontj92@gmail.com>

- Modified call to topic_cache.addTopic at custom_participant_info.hpp - Wrote tests for topic_cache Signed-off-by: Jaison Titus <jaisontj92@gmail.com>

rmw_get_subscriptions_info_by_topic Signed-off-by: Jaison Titus <jaisontj92@gmail.com>

rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/qos_converter.hpp

rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/topic_cache.hpp

jaisontj · 2019-10-29T21:18:19Z

@ivanpauno

rmw_fastrtps_shared_cpp/test/test_topic_cache.cpp

rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_participant_info.hpp

rmw_fastrtps_shared_cpp/src/rmw_get_topic_info.cpp

rmw_get_subscriptions_info_by_topic Signed-off-by: Jaison Titus <jaisontj92@gmail.com>

Signed-off-by: Jaison Titus <jaisontj92@gmail.com>

rmw_fastrtps_shared_cpp/CMakeLists.txt

rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/topic_cache.hpp

- changed to using instead of typedef Signed-off-by: Jaison Titus <jaisontj92@gmail.com>

Signed-off-by: Jaison Titus <jaisontj92@gmail.com>

rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/topic_cache.hpp

rmw_fastrtps_shared_cpp/src/rmw_get_topic_info.cpp

ivanpauno · 2019-11-21T15:19:52Z

rmw_fastrtps_shared_cpp/src/rmw_get_topic_info.cpp

+  topic_fqdns.push_back(topic_name);
+  // if mangle
+  if (!no_mangle) {
+    auto ros_prefixes = _get_all_ros_prefixes();


I know this is imitating the rmw_count implementation, but trying with all prefixes is wrong.
I will continue the discussion in a follow up issue.

Let's not forget this point.

- Refactor to accomodate change in rmw_topic_info_array* functions. Signed-off-by: Jaison Titus <jaisontj92@gmail.com>

Signed-off-by: Miaofei <miaofei@amazon.com>

rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/topic_cache.hpp

rmw_fastrtps_shared_cpp/src/rmw_get_topic_info.cpp

hidmic · 2019-12-12T20:32:25Z

rmw_fastrtps_shared_cpp/src/rmw_get_topic_info.cpp

+      return ret;
+    }
+    for (auto i = 0u; i < count; i++) {
+      participants_info->info_array[i] = topic_info_vector.at(i);


@jaisontj do we need an std::vector if we'll eventually just copy it into a primitive array?

It doesn't look trivial to determine what size of a primitive array would need be initially allocated.

That's because of how the loop is written. If iterators that refer to matching topic data were collected first, then it'd be straightforward. This somewhat connects to @ivanpauno 's previous comment here. I can understand why it was done this way initially but the overhead is not necessary. I'm fine with deferring though.

I'm also ok with deferring, but I would really like to see it cleaned up at some point. My experience has been that clean up things like this tend to get overlooked though.

rmw_fastrtps_shared_cpp/test/test_topic_cache.cpp

rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/topic_cache.hpp

Signed-off-by: Miaofei <miaofei@amazon.com>

hidmic

One last observation is that this patch abuses auto a bit. It's a great feature to deal with iterator types and such, but if used even in lieu of int types it obscures code.

rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/topic_cache.hpp

rmw_fastrtps_shared_cpp/src/rmw_get_topic_info.cpp

hidmic · 2019-12-17T20:22:18Z

rmw_fastrtps_shared_cpp/src/rmw_get_topic_info.cpp

+      return ret;
+    }
+    for (auto i = 0u; i < count; i++) {
+      participants_info->info_array[i] = topic_info_vector.at(i);


That's because of how the loop is written. If iterators that refer to matching topic data were collected first, then it'd be straightforward. This somewhat connects to @ivanpauno 's previous comment here. I can understand why it was done this way initially but the overhead is not necessary. I'm fine with deferring though.

rmw_fastrtps_shared_cpp/src/rmw_get_topic_info.cpp

Signed-off-by: Miaofei <miaofei@amazon.com>

rmw_fastrtps_shared_cpp/src/rmw_get_topic_info.cpp

Signed-off-by: Miaofei <miaofei@amazon.com>

rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/topic_cache.hpp

Signed-off-by: Miaofei <miaofei@amazon.com>

wjwwood

lgtm, with some comments

rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/topic_cache.hpp

wjwwood · 2020-01-08T22:36:39Z

rmw_fastrtps_shared_cpp/src/rmw_get_topic_info.cpp

+  topic_fqdns.push_back(topic_name);
+  // if mangle
+  if (!no_mangle) {
+    auto ros_prefixes = _get_all_ros_prefixes();


Let's not forget this point.

rmw_fastrtps_shared_cpp/src/rmw_get_topic_info.cpp

wjwwood · 2020-01-08T22:41:04Z

rmw_fastrtps_shared_cpp/src/rmw_get_topic_info.cpp

+      return ret;
+    }
+    for (auto i = 0u; i < count; i++) {
+      participants_info->info_array[i] = topic_info_vector.at(i);


I'm also ok with deferring, but I would really like to see it cleaned up at some point. My experience has been that clean up things like this tend to get overlooked though.

rmw_fastrtps_shared_cpp/src/rmw_get_topic_info.cpp

rmw_fastrtps_shared_cpp/test/test_topic_cache.cpp

Signed-off-by: Miaofei <miaofei@amazon.com>

jaisontj added 4 commits October 25, 2019 13:57

Simpled test for topic_cache

6721292

Signed-off-by: Jaison Titus <jaisontj92@gmail.com>

Moved qos_converter to include/ since it needs to be accessible to

944e1f3

topic_cache.hpp Signed-off-by: Jaison Titus <jaisontj92@gmail.com>

- Modified topic_cache to include qos policies

e7a4a7e

- Modified call to topic_cache.addTopic at custom_participant_info.hpp - Wrote tests for topic_cache Signed-off-by: Jaison Titus <jaisontj92@gmail.com>

Stubs for rmw_get_publishers_info_by_topic and

d0ac7e0

rmw_get_subscriptions_info_by_topic Signed-off-by: Jaison Titus <jaisontj92@gmail.com>

This was referenced Oct 29, 2019

Enhance TopicCache to include QoS Policies of publishers and subscribers #332

Closed

Stubs for rmw_get_publishers_info_by_topic and rmw_get_subscriptions_info_by_topic #333

Closed

jaisontj commented Oct 29, 2019

View reviewed changes

rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/qos_converter.hpp Outdated Show resolved Hide resolved

jaisontj commented Oct 29, 2019

View reviewed changes

rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/qos_converter.hpp Outdated Show resolved Hide resolved

jaisontj commented Oct 29, 2019

View reviewed changes

rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/topic_cache.hpp Show resolved Hide resolved

jaisontj commented Oct 29, 2019

View reviewed changes

rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/topic_cache.hpp Show resolved Hide resolved

jaisontj commented Oct 29, 2019

View reviewed changes

rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/topic_cache.hpp Outdated Show resolved Hide resolved

jaisontj marked this pull request as ready for review November 6, 2019 18:01

jaisontj mentioned this pull request Nov 6, 2019

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

Merged

prajakta-gokhale suggested changes Nov 12, 2019

View reviewed changes

jaisontj force-pushed the jaisontj/impl_rmw_get_node_info_by_topic branch 2 times, most recently from 58c1fe8 to f029dac Compare November 13, 2019 20:08

jaisontj requested a review from prajakta-gokhale November 13, 2019 20:09

jaisontj force-pushed the jaisontj/impl_rmw_get_node_info_by_topic branch 2 times, most recently from 176cff3 to 17f0695 Compare November 14, 2019 00:40

Implemented rmw_get_publishers_info_by_topic and

e640479

rmw_get_subscriptions_info_by_topic Signed-off-by: Jaison Titus <jaisontj92@gmail.com>

jaisontj force-pushed the jaisontj/impl_rmw_get_node_info_by_topic branch from 17f0695 to e640479 Compare November 14, 2019 00:52

prajakta-gokhale approved these changes Nov 14, 2019

View reviewed changes

- Addressing code review changes: Using const auto instead of auto

1a80e44

Signed-off-by: Jaison Titus <jaisontj92@gmail.com>

zmichaels11 reviewed Nov 14, 2019

View reviewed changes

rmw_fastrtps_shared_cpp/CMakeLists.txt Outdated Show resolved Hide resolved

rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/topic_cache.hpp Outdated Show resolved Hide resolved

jaisontj added 2 commits November 15, 2019 16:55

- Alpha sorted cmakelist.txt

30a2a56

- changed to using instead of typedef Signed-off-by: Jaison Titus <jaisontj92@gmail.com>

- Alphasort

dbfe0b4

Signed-off-by: Jaison Titus <jaisontj92@gmail.com>

zmichaels11 approved these changes Nov 16, 2019

View reviewed changes

ivanpauno requested changes Nov 21, 2019

View reviewed changes

- Refactor to accomodate change in rmw_topic_info_set* functions.

04b29c8

- Refactor to accomodate change in rmw_topic_info_array* functions. Signed-off-by: Jaison Titus <jaisontj92@gmail.com>

fix error handling in _handle_topic_info_fini()

169f218

Signed-off-by: Miaofei <miaofei@amazon.com>

mm318 force-pushed the jaisontj/impl_rmw_get_node_info_by_topic branch from be2cf37 to 169f218 Compare December 11, 2019 00:32

ivanpauno approved these changes Dec 11, 2019

View reviewed changes

This was referenced Dec 11, 2019

Implement the rmw_get_publishers_info_by_topic and rmw_get_subscriptions_info_by_topic methods ros2/rmw_opensplice#299

Closed

Implement the rmw_get_publishers_info_by_topic and rmw_get_subscriptions_info_by_topic methods ros2/rmw_connext#380

Closed

mm318 mentioned this pull request Dec 11, 2019

Implement the rmw_get_publishers_info_by_topic and rmw_get_subscriptions_info_by_topic methods ros2/rmw_cyclonedds#82

Closed

hidmic reviewed Dec 12, 2019

View reviewed changes

address PR comments

36e9718

Signed-off-by: Miaofei <miaofei@amazon.com>

hidmic approved these changes Dec 17, 2019

View reviewed changes

ivanpauno requested changes Dec 18, 2019

View reviewed changes

rmw_fastrtps_shared_cpp/src/rmw_get_topic_info.cpp Outdated Show resolved Hide resolved

address PR comments

8152876

Signed-off-by: Miaofei <miaofei@amazon.com>

mm318 mentioned this pull request Dec 19, 2019

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

Merged

conditionally demangle type name

642d9ed

Signed-off-by: Miaofei <miaofei@amazon.com>

ivanpauno reviewed Dec 19, 2019

View reviewed changes

rmw_fastrtps_shared_cpp/src/rmw_get_topic_info.cpp Outdated Show resolved Hide resolved

reverse condition for demangling type name

38dec2a

Signed-off-by: Miaofei <miaofei@amazon.com>

mm318 force-pushed the jaisontj/impl_rmw_get_node_info_by_topic branch from 8a72fc0 to 38dec2a Compare December 28, 2019 03:29

ivanpauno approved these changes Jan 2, 2020

View reviewed changes

rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/topic_cache.hpp Show resolved Hide resolved

properly initialize history and history depth in topic info

c231f14

Signed-off-by: Miaofei <miaofei@amazon.com>

mm318 force-pushed the jaisontj/impl_rmw_get_node_info_by_topic branch from 4f4959a to c231f14 Compare January 2, 2020 18:40

prajakta-gokhale mentioned this pull request Jan 3, 2020

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

Closed

wjwwood approved these changes Jan 8, 2020

View reviewed changes

mm318 added 3 commits January 9, 2020 16:16

address more PR comments

a1a5e79

Signed-off-by: Miaofei <miaofei@amazon.com>

rename *topic_info* to *topic_endpoint_info*

354cae2

Signed-off-by: Miaofei <miaofei@amazon.com>

fix formatting

1fe66ed

Signed-off-by: Miaofei <miaofei@amazon.com>

ivanpauno approved these changes Jan 10, 2020

View reviewed changes

wjwwood approved these changes Jan 10, 2020

View reviewed changes

ivanpauno merged commit 465d15e into ros2:master Jan 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jaisontj commented Oct 29, 2019 •

edited

Loading

jaisontj commented Oct 29, 2019

ivanpauno Nov 21, 2019

wjwwood Jan 8, 2020

hidmic Dec 12, 2019 •

edited

Loading

mm318 Dec 14, 2019 •

edited

Loading

hidmic Dec 17, 2019

wjwwood Jan 8, 2020

hidmic left a comment

hidmic Dec 17, 2019

wjwwood left a comment

wjwwood Jan 8, 2020

wjwwood Jan 8, 2020

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

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

Conversation

jaisontj commented Oct 29, 2019 • edited Loading

jaisontj commented Oct 29, 2019

ivanpauno Nov 21, 2019

Choose a reason for hiding this comment

wjwwood Jan 8, 2020

Choose a reason for hiding this comment

hidmic Dec 12, 2019 • edited Loading

Choose a reason for hiding this comment

mm318 Dec 14, 2019 • edited Loading

Choose a reason for hiding this comment

hidmic Dec 17, 2019

Choose a reason for hiding this comment

wjwwood Jan 8, 2020

Choose a reason for hiding this comment

hidmic left a comment

Choose a reason for hiding this comment

hidmic Dec 17, 2019

Choose a reason for hiding this comment

wjwwood left a comment

Choose a reason for hiding this comment

wjwwood Jan 8, 2020

Choose a reason for hiding this comment

wjwwood Jan 8, 2020

Choose a reason for hiding this comment

jaisontj commented Oct 29, 2019 •

edited

Loading

hidmic Dec 12, 2019 •

edited

Loading

mm318 Dec 14, 2019 •

edited

Loading