Change rmw_count_publishers API, to rcl equivalent rcl_count_publishe…#425
Change rmw_count_publishers API, to rcl equivalent rcl_count_publishe…#425dhood merged 4 commits intoros2:masterfrom
Conversation
…rs and remove the TODO line. Signed-off-by: Sriram Raghunathan <rsriram7@visteon.com>
dhood
left a comment
There was a problem hiding this comment.
thanks for the patch, don't think it won't work as-is though. Could you also update count_subscribers for symmetry please
| size_t count; | ||
| // TODO(wjwwood): use the rcl equivalent methods | ||
| auto ret = rmw_count_publishers(rmw_node_handle, fqdn.c_str(), &count); | ||
| auto ret = rcl_count_publishers(rmw_node_handle, fqdn.c_str(), &count); |
There was a problem hiding this comment.
rcl_count_publishers takes an rcl node handle instead of an rmw one
…ure to topic_names. Signed-off-by: Sriram Raghunathan <rsriram7@visteon.com>
|
Hi @dhood made changes after reading a few things inside rcl. Sorry for the initial patch which was incomplete. Could you review the changes? |
| NodeGraph::count_publishers(const std::string & topic_name) const | ||
| { | ||
| auto rmw_node_handle = rcl_node_get_rmw_handle(node_base_->get_rcl_node_handle()); | ||
| auto fqdn = rclcpp::expand_topic_or_service_name( |
There was a problem hiding this comment.
This step will combine the topic name and namespace into a "fully qualified" topic name (example) and should not be removed
Signed-off-by: Sriram Raghunathan <rsriram7@visteon.com>
|
@dhood made changes to use rcl_* specific API's have a look. |
| { | ||
| auto rcl_node_handle = node_base_->get_rcl_node_handle(); | ||
| auto name = rcl_node_get_name(rcl_node_handle); | ||
| auto namespace_ = rcl_node_get_namespace(rcl_node_handle); |
There was a problem hiding this comment.
please use trailing underscores only for member variables. same below.
There was a problem hiding this comment.
@Karsten1987 I have'nt read variables with '_' prefix, is that a valid declaration within "ros2" coding styles? May I use it here?
There was a problem hiding this comment.
We normally adhere to the C++ coding guidelines: https://google.github.io/styleguide/cppguide.html#Variable_Names
Now, I think you can actually use it here, because namespace is a reserved keyword in C++. I've seen we used it in other places as well, so that I believe in this special case, it's fine.
Please ignore my comment then.
|
@dhood could you review the latest commits? |
and linters are happy after my fixup: |
…ging_log4cxx (ros2#425) Signed-off-by: burekn <burekn@amazon.com>
ros2#436) * Revert "Changes the default 3rd party logger from rcl_logging_noop to rcl_logging_log4cxx (ros2#425)" This reverts commit ac8ee90. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org> * Revert "add explicit dependency on rcl_logging_log4cxx (ros2#435)" This reverts commit 816ce67. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org> * Add dependency on rcl_logging_noop. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Change rmw_count_publishers API to rcl equivalent rcl_count_publishers and remove the TODO comment from the source.
Signed-off-by: Sriram Raghunathan rsriram7@visteon.com