SubNode: add "NodeHandle" like concept by fmrico · Pull Request #578 · ros2/rclcpp

fmrico · 2018-11-02T12:14:41Z

Hi all,

This PR is related to issue #531 , providing a SubNode class (implemented as a Proxy Pattern). This let to have code like:

 auto node_ptr = std::make_shared<rclcpp::Node>("my_node", "/my_ns");

  auto sub_node = node_ptr->create_sub_node("/foo_ns");
  auto another_sub_node = sub_node->create_sub_node("/bar_ns");

  // topic would be '/my_ns/foo_ns/bar_ns/chatter'
  auto sub0 = node_ptr->create_subscription<std_msgs::msg::String>("chatter0",topic_callback);
  auto sub1 = sub_node->create_subscription<std_msgs::msg::String>("chatter1",topic_callback);
  auto sub2 = another_sub_node->create_subscription<std_msgs::msg::String>("chatter2",topic_callback);

which produces these topics:

/my_ns/chatter0
/my_ns/foo_ns/bar_ns/chatter2
/my_ns/foo_ns/chatter1

This is a possible starting point to have the desired functionality.

Best

sloretz

Hi @fmrico, thanks for the pull request! This is raises some questions in my mind about what #531 is asking for.

It looks like the SubNode approach requires all API's on the node to be duplicated. One of the comments on #351 seems propose that a Node and a SubNode be the same class with a hidden private Node implementation that they both call to avoid duplicating APIs. That sounds like a huge change to implement in one pull request.

Any ideas on how to iterate towards Node and SubNode being the same class without duplicating the APIs in the meantime?

sloretz · 2018-11-08T21:52:26Z

rclcpp/include/rclcpp/sub_node_impl.hpp

@@ -0,0 +1,227 @@
+// Copyright 2014 Open Source Robotics Foundation, Inc.


sloretz · 2018-11-08T21:52:34Z

rclcpp/include/rclcpp/sub_node.hpp

@@ -0,0 +1,485 @@
+// Copyright 2014 Open Source Robotics Foundation, Inc.


sloretz · 2018-11-08T21:53:55Z

rclcpp/include/rclcpp/sub_node.hpp

+namespace rclcpp
+{
+
+class Node;


No need to forward declare Node since rclcpp/node.hpp was included above

sloretz · 2018-11-08T22:00:26Z

rclcpp/include/rclcpp/sub_node.hpp

+    CallbackT && callback,
+    const rmw_qos_profile_t & qos_profile = rmw_qos_profile_default,
+    rclcpp::callback_group::CallbackGroup::SharedPtr group = nullptr,
+    bool ignore_local_publications = false,


This approach copies a lot of code. I'm concerned maintaining default values for arguments like this one in two places will be difficult to do in the future. @wjwwood posted this comment about making a sub node and a node the same class.

sloretz · 2018-11-09T00:05:55Z

rclcpp/src/rclcpp/sub_node.cpp

+const std::string
+SubNode::add_extended_ns_prefix(const std::string & original_name) const
+{
+  return std::string(original_->get_namespace()) + "/" + extended_namespace_ + "/" + original_name;


This works for relative names foo or foo/bar, but I'm not sure about the behavior for fully qualified or private names ~/bar.

ROS 1 threw ros::InvalidNameException when trying to create a publisher/subscriber/etc with a private name, asking instead that the NodeHandle be created with a private namespace. In ROS 2 the create_...() APIs allow private names. I'm not sure how to handle all the cases, but there is a proposal here: #531 (comment)

sloretz · 2018-11-09T00:14:57Z

rclcpp/test/test_node.cpp

+    auto node = std::make_shared<rclcpp::Node>("my_node", "/ns");
+    auto subnode = node->create_sub_node("/sub_ns");
+    EXPECT_STREQ("my_node", subnode->get_name());
+    EXPECT_STREQ("/ns/sub_ns", subnode->get_namespace());


Given that the subnode's namespace is fully qualified, I think this result should be /sub_ns. This also matches ROS 1 when a fully qualified name is given as a child NodeHandle namespace.

#include "ros/ros.h" #include "std_msgs/String.h" int main(int argc, char ** argv) { ros::init(argc, argv, "name"); ros::NodeHandle n; ros::NodeHandle nsub(n, "/sub_ns"); ros::Publisher pub = nsub4.advertise<std_msgs::String>("nsub/relaive", 1000); ros::spin(); return 0; }

launched with /path/to/above/executable __ns:=/orig_ns

Creates the topic /sub_ns/nsub/relative, not /orig_ns/sub_ns/...

We could also consider making it so that "sub nodes" can only use relative name prefix or a prefix starting with a ~. That would avoid the issue of ignoring the node namespace.

wjwwood · 2018-11-09T01:13:42Z

Yeah, unfortunately I think the design issues here (like the amount of duplicated code) are going to impede progress. Maybe we could try and setup a voice chat at some point to hash out some of the details.

@fmrico are you interested at all in doing that?

fmrico · 2018-11-09T16:04:36Z

Hi,

Yes, there is a lot of duplicated code because I did not want that SubNode inherits from Node, but the interface had to be the same. I could reconsider this or find another solution.

@wjwwood I am interested in doing that.

fmrico · 2018-11-11T07:52:10Z

Hi @wjwwood and @sloretz ,

I have been iterating on this. I provide another alternative in PR #581

Is uses Node Class, so there is not duplciated code.

Best

Resolves ros2#552 Signed-off-by: Jacob Perron <jacob@openrobotics.org>

Signed-off-by: Barry Xu <barry.xu@sony.com>

fmrico added 3 commits November 1, 2018 12:34

Added SubNode like a "NodeNadle" concept

b88c252

Merge branch 'master' of https://github.com/ros2/rclcpp into sub_node

32e1948

Check for valid namespace

a0f219f

tfoote added the in review Waiting for review (Kanban column) label Nov 2, 2018

fmrico mentioned this pull request Nov 2, 2018

add "NodeHandle" like concept #531

Closed

clalancette requested a review from sloretz November 8, 2018 18:23

sloretz requested changes Nov 9, 2018

View reviewed changes

fmrico mentioned this pull request Nov 11, 2018

Sub Node alternative #581

Merged

fmrico closed this Nov 11, 2018

tfoote removed the in review Waiting for review (Kanban column) label Nov 11, 2018

nnmm pushed a commit to ApexAI/rclcpp that referenced this pull request Jul 9, 2022

Document clock types (ros2#578)

ca64525

Resolves ros2#552 Signed-off-by: Jacob Perron <jacob@openrobotics.org>

DensoADAS pushed a commit to DensoADAS/rclcpp that referenced this pull request Aug 5, 2022

Fix a bug on parsing wrong description in plugin xml file (ros2#578)

ad4b52f

Signed-off-by: Barry Xu <barry.xu@sony.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SubNode: add "NodeHandle" like concept#578

SubNode: add "NodeHandle" like concept#578
fmrico wants to merge 3 commits intoros2:masterfrom
fmrico:sub_node

fmrico commented Nov 2, 2018

Uh oh!

sloretz left a comment

Uh oh!

sloretz Nov 8, 2018

Uh oh!

sloretz Nov 8, 2018

Uh oh!

sloretz Nov 8, 2018

Uh oh!

sloretz Nov 8, 2018

Uh oh!

sloretz Nov 9, 2018

Uh oh!

sloretz Nov 9, 2018

Uh oh!

wjwwood Nov 9, 2018

Uh oh!

wjwwood commented Nov 9, 2018

Uh oh!

fmrico commented Nov 9, 2018

Uh oh!

fmrico commented Nov 11, 2018 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

		@@ -0,0 +1,227 @@
		// Copyright 2014 Open Source Robotics Foundation, Inc.

		@@ -0,0 +1,485 @@
		// Copyright 2014 Open Source Robotics Foundation, Inc.

Conversation

fmrico commented Nov 2, 2018

Uh oh!

sloretz left a comment

Choose a reason for hiding this comment

Uh oh!

sloretz Nov 8, 2018

Choose a reason for hiding this comment

Uh oh!

sloretz Nov 8, 2018

Choose a reason for hiding this comment

Uh oh!

sloretz Nov 8, 2018

Choose a reason for hiding this comment

Uh oh!

sloretz Nov 8, 2018

Choose a reason for hiding this comment

Uh oh!

sloretz Nov 9, 2018

Choose a reason for hiding this comment

Uh oh!

sloretz Nov 9, 2018

Choose a reason for hiding this comment

Uh oh!

wjwwood Nov 9, 2018

Choose a reason for hiding this comment

Uh oh!

wjwwood commented Nov 9, 2018

Uh oh!

fmrico commented Nov 9, 2018

Uh oh!

fmrico commented Nov 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

fmrico commented Nov 11, 2018 •

edited

Loading