Implementation for rmw_get_pub/sub_info_by_topic by dpotman · Pull Request #97 · ros2/rmw_cyclonedds

dpotman · 2020-02-06T10:32:28Z

Implementation for the rmw_get_publishers_info_by_topic and rmw_get_subscriptions_info_by_topic methods, and some related refactoring:

rewrite rmw_collect_tptyp_for_kind to the more generic rmw_collect_data_for_endpoint
moved code from rmw_get_topic_endpoint_info.cpp to rmw_node.cpp (restructuring of rmw_node.cpp file is planned, but not in this PR)
extracted qos conversion function dds_qos_to_rmw_qos from get_readwrite_qos

When this PR is merged, the rcl test test_rcl_get_publishers_subscription_info_by_topic can be enabled for Cyclone; I'll create a PR for this.

Implementation for the rmw_get_publishers_info_by_topic and rmw_get_subscriptions_info_by_topic methods. Signed-off-by: Dennis Potman <[email protected]>

dpotman · 2020-02-06T10:35:54Z

This PR resolves issue #82 (may need refactoring in case the Participant mapping is moved to Context)

eboasson

Thanks @dennis-adlink for this pull request. As I want to see the Cyclone RMW layer complete, I decided to respond to @ivanpauno's request in #87. It's really mostly nit-picking, the only mistake is in the error handling in get_endpoint_info_by_topic, but I also think there might be a problem in the underlying rmw interfaces ...

eboasson · 2020-02-07T10:29:41Z

+  dds_qos_t * qos = dds_create_qos();
+  if (dds_get_qos(handle, qos) < 0) {
+    RMW_SET_ERROR_MSG("get_readwrite_qos: invalid handle");
+    goto error;


I would suggest removing the goto errors here, considering that only the case of dds_get_qos returning an error needs some error handling.

eboasson · 2020-02-07T10:38:28Z

  void * msg = NULL;
  int32_t n;
-  while ((n = dds_take(rd, &msg, &info, 1, 1)) == 1) {
+  bool cont = true;


All filter/map functions passed to rmw_collect_data_for_endpoint always return true, and so the cont variable isn't needed. I would suggest removing the return value.

eboasson · 2020-02-07T10:42:39Z

+typedef std::map<dds_builtintopic_guid_t, std::pair<std::string, std::string>> node_cache_t;
+
+static rmw_ret_t get_node_name(
+  dds_entity_t rd,


Perhaps it would be better to indicate more clearly that rd must be a reader of the "participant" built-in topic — and possible also to assert that is. A name change would do the trick, ppant_rd could be an option (but I find it hard to guess how much confusion it would eliminate or create for readers that don't know Cyclone well).

eboasson · 2020-02-07T10:44:53Z

+static rmw_ret_t
+set_rmw_topic_endpoint_info(
+  node_cache_t & node_cache,
+  dds_entity_t rd,


See comment concerning rd in get_node_name — the same applies here.

eboasson · 2020-02-07T10:48:07Z

+    node_name = "_NODE_NAME_UNKNOWN_";
+    node_ns = "_NODE_NAMESPACE_UNKNOWN_";
+  }
+  return RMW_RET_OK;


Not returning a value would eliminate the return value checks later on.

eboasson · 2020-02-07T10:51:22Z

+  {
+    return ret;
+  }
+  topic_endpoint_info.qos_profile = epinfo.qos_profile;


Shouldn't this be rmw_topic_endpoint_info_set_qos_profile to continue the pattern?

eboasson · 2020-02-07T10:54:44Z

+            "/");
+          info.topic_type = std::string(demangled_type) + std::string(cm_typ[2]);
+        } else {
+          return true;


Perhaps add a comment that this skips a non-ROS2 endpoint?

eboasson · 2020-02-07T10:57:49Z

+      node_impl->enth, DDS_BUILTIN_TOPIC_DCPSPARTICIPANT, NULL, NULL)) < 0)
+  {
+    RMW_SET_ERROR_MSG("get_endpoint_info_by_topic: failed to create reader");
    return RMW_RET_ERROR;


I think this is missing a call to rmw_topic_endpoint_info_array_fini because the init_with_size function before allocated memory. However, I'm not entirely certain of this because array_fini invokes endpoint_info_fini on its elements and I don't see a guarantee the array init_with_size allocates is properly initialized.

If you first call get_zero_initialized_topic_endpoint_info_array, and then init_with_size, it shouldn't be a problem to call rmw_topic_endpoint_info_array_fini when something fails.

eboasson · 2020-02-07T10:58:17Z

+        handle_topic_endpoint_info_fini(&endpoint_info->info_array[n], allocator);
+      }
+      dds_delete(rd);
+      return ret;


See the comment above

eboasson · 2020-02-07T11:08:31Z

+  rmw_topic_endpoint_info_array_t * endpoint_info)
+{
+  auto node_impl = static_cast<CddsNode *>(node->data);
+  const auto re_typ = std::regex("^(.*::)dds_::(.*)_$", std::regex::extended);


I think it would make sense to create a function to demangle topic names/types, with a parameter specifying whether it concerns data, requests or responses. There was some duplication already, but this adds yet another one and having regexes all over the place is a bit unpleasant.

…fixed enpoint_info_array_fini and a few other minor fixes Signed-off-by: Dennis Potman <[email protected]>

dpotman · 2020-02-08T10:15:17Z

@eboasson Thanks for the review! I fixed the issues in my latest commit:

introduced a function for topic name/type demangle
renamed rd to ppant_rd
fixed cleaning up topic_endpoint_info_array in case of failure
removed return value from get_node_name function
refactored goto's in get_readwrite_qos
removed cont from rmw_collect_data_for_endpoint

Signed-off-by: Dennis Potman <[email protected]>

dpotman · 2020-02-08T10:41:04Z

One more commit to refactor out the extra call to demangle_topic_type in get_endpoint_names_and_types_by_node

Signed-off-by: Erik Boasson <[email protected]>

eboasson · 2020-02-10T12:52:02Z

Thanks @dennis-adlink. The only thing was a warning for an unused argument in a release build, I've pushed a fix for that.

@ivanpauno I think it can be merged, but perhaps you would like to have a quick look or do a CI run first?

ivanpauno · 2020-02-10T13:04:52Z

@ivanpauno I think it can be merged, but perhaps you would like to have a quick look or do a CI run first?

Yes, I will run CI together with the rcl PR, so we check that tests are passing.

ivanpauno

I think error handling should be fixed before merging, otherwise LGTM!

ivanpauno · 2020-02-10T15:30:26Z

+      node_impl->enth, DDS_BUILTIN_TOPIC_DCPSPARTICIPANT, NULL, NULL)) < 0)
+  {
+    RMW_SET_ERROR_MSG("get_endpoint_info_by_topic: failed to create reader");
    return RMW_RET_ERROR;


If you first call get_zero_initialized_topic_endpoint_info_array, and then init_with_size, it shouldn't be a problem to call rmw_topic_endpoint_info_array_fini when something fails.

dpotman · 2020-02-10T16:42:50Z

I think error handling should be fixed before merging, otherwise LGTM!

rmw_topic_endpoint_info_array_init_with_size does not set count, so that's why I initialize this to zero. After that point I use rmw_topic_endpoint_info_array_fini for cleaning up, also in case of an error (note that the code fragments above your remark is outdated). So I think error handling is properly in now.

ivanpauno · 2020-02-10T17:56:35Z

rmw_topic_endpoint_info_array_init_with_size does not set count, so that's why I initialize this to zero

I'm going to send a PR to fix that, because it's confusing (and incongruent with other arrays defined in rcutils/rmw). We can iterate on that in another PR, when the change in rmw_topic_endpoint_info_array_init_with_size is done.

After that point I use rmw_topic_endpoint_info_array_fini for cleaning up, also in case of an error (note that the code fragments above your remark is outdated). So I think error handling is properly in now.

Sounds good!

Signed-off-by: Dennis Potman <[email protected]>

Implementation for rmw_get_pub/sub_info_by_topic

2fdccda

Implementation for the rmw_get_publishers_info_by_topic and rmw_get_subscriptions_info_by_topic methods. Signed-off-by: Dennis Potman <[email protected]>

dpotman mentioned this pull request Feb 6, 2020

Fix test_rcl_get_publishers_subscription_info_by_topic ros2/rcl#571

Merged

ivanpauno mentioned this pull request Feb 6, 2020

Implement the rmw_get_publishers_info_by_topic and rmw_get_subscriptions_info_by_topic methods #82

Closed

eboasson reviewed Feb 7, 2020

View reviewed changes

dpotman mentioned this pull request Feb 7, 2020

Enable tests for get_pub/sub_info_by_topic for Cyclone DDS ros2/rcl#572

Merged

Processed review comments: moved topic/type demangling to functions, …

e04454c

…fixed enpoint_info_array_fini and a few other minor fixes Signed-off-by: Dennis Potman <[email protected]>

Some additional refactoring in get_endpoint_names_and_types_by_node

b06b6b0

Signed-off-by: Dennis Potman <[email protected]>

Fix "unused argument" warning in release builds

2bf02d2

Signed-off-by: Erik Boasson <[email protected]>

eboasson approved these changes Feb 10, 2020

View reviewed changes

ivanpauno reviewed Feb 10, 2020

View reviewed changes

ivanpauno merged commit d70fad6 into ros2:master Feb 10, 2020

eboasson mentioned this pull request Feb 11, 2020

Fix dashing builds wrt rmw_topic_endpoint_info_t #99

Merged

eboasson pushed a commit that referenced this pull request Mar 12, 2020

Implementation for rmw_get_pub/sub_info_by_topic (#97)

ed96b80

Signed-off-by: Dennis Potman <[email protected]>

Conversation

dpotman commented Feb 6, 2020

Uh oh!

dpotman commented Feb 6, 2020

Uh oh!

eboasson left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dpotman commented Feb 8, 2020

Uh oh!

dpotman commented Feb 8, 2020

Uh oh!

eboasson commented Feb 10, 2020

Uh oh!

ivanpauno commented Feb 10, 2020

Uh oh!

ivanpauno left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dpotman commented Feb 10, 2020

Uh oh!

ivanpauno commented Feb 10, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants