Modify sub node copy constructor to create new node parameters object

[jlack1987]

My attempt at fixing #731. The tldr of the change here is when a sub node is created, the pointer to the new node creates a new pointer to a NodeParameters object instead of being given the "parent" nodes pointer. This results in the following behavior change from parameters,

• Parameters are scoped into the node/sub node
• When a callback is registered via the add_on_set_parameters_callback call, it will only be triggered when a parameter registered to that particular node or sub node is modified, i.e. if node_a has a sub node sub_node_a and a parameter of sub_node_a is modified, callbacks registered to node_a will not be called
• Node options are still past down to sub nodes, so parameter relevant options such as allow_undeclared_parameters will apply to any sub nodes

I feel like the fix to this issue can't be this simple; however, I have been unable to find any test failures or problems that result from this change. I did add unit tests that failed prior to making this modification and then pass after the change.

Signed-off-by: Jordan Lack jlack@houstonmechatronics.com

[aprotyas]

You've probably tried this, but do you observe the expected behavior from #1778 and #731 (comment)?

[jlack1987]

@aprotyas I haven't tried but I believe I was mistaken originally when I made that issue. At the time I was confused by some behavior I was seeing and thought that it was due to that but i'm pretty sure I was just confused. Especially since in the code spinning operates on the node base interface which nodes and their sub nodes share

[fujitatomoya]

if i am not mistaken, sub-nodes share all resource with parent node but only namespace is extended and propagated accordingly. apparently this fix is to allocate the own resources for sub-node, so i am not sure if this is the path we take?? 🤔 any comments ?

[fujitatomoya]

Nice catch ❗

[aprotyas]

if i am not mistaken, sub-nodes share all resource with parent node but only namespace is extended and propagated accordingly. apparently this fix is to allocate the own resources for sub-node, so i am not sure if this is the path we take?? 🤔 any comments ?

There is probably a design discussion to be had here because I'm not sure if @jlack1987 can implement what this PR aims to do without creating a different resource for the subnode.

[jlack1987]

@fujitatomoya I believe you are mistaken, unless i'm mistaken that is 😄 the change here preserves all of the resources shared between nodes except for the pointer to the NodeParameters object which I believe just means the parameters are on a per node/sub-node basis and are not all stored with the NodeParameters pointer owned by the top level node.

It seems like this is the way it should behave based on the discussions here. Are there perhaps some additional unit tests you can think of that I could implement that would help show that this either does or does not result in the desired behavior and/or results in some other regression?

[fujitatomoya]

@jlack1987 thanks for the comment!

NodeParameters object which I believe just means the parameters are on a per node/sub-node basis and are not all stored with the NodeParameters pointer owned by the top level node.

this is what i mean. if this is true, what about the topics and services? (sorry i am not sure either, but it should be consistent behavior for all entities?)

btw, this fix solves #731 (comment)? (I think it does not.)

[jlack1987]

@fujitatomoya topics and services stay the same here(i'll write some more unit tests to show that today). And as far as the comments you referenced here, the short answer is that it kinda sorta fixes it 😜.

It does fix the behavior as far as the interaction with the param server from the users frame of reference goes, but the way it fixes it does not match how topics/services are handled via simple namespace extension; however, after getting into the code I personally don't believe appending/prepending namespaces to the params is a good/clean way to fix that. I'll try to explain my reasoning on that. First though if you haven't please check out this simple test to see how the user interacts with parameters from sub-nodes.

Currently(on released code and master) params are stored with the NodeParameters pointer owned by a Node, and when sub nodes are created they are simply given the parent nodes pointer to the NodeParameters which really breaks parameter interaction from subnodes because multiple subnodes then can't create parameters of the same name. One option is to prepend the name of the parameter with the sub nodes namespace; however this imo isn't a great solution for parameters because when the on_set_parameters_callback is called, the parameter name would be /ns1/ns2/.../name. Yeah we could string split that suppose and only send the last bit, but that seems kinda sloppy to me bc the name you're sending to the callback isn't the actual stored name of the parameter; however, if you all prefer that implementation I can try that instead.

The way I did it was to create a new NodeParameters pointer for sub nodes, which means that sub node parameters aren't accessible from the parent node nor any of the other sub nodes even if they share a common parent. Imo this simplifies the interaction from the user though in that the parameter name is simply what you give it and there's no internal parameter name expansion or crunching each time the user tries to interact with it. It's possible though there's some fallout that I haven't seen yet from this implementation, but the tests pass so I haven't found that yet 😄

[aprotyas]

sub node parameters aren't accessible from the parent node nor any of the other sub nodes even if they share a common parent

I'm pretty sure there isn't a "correct" answer to this since #731 says how parameters and "sub-nodes" interact is not documented and is not consistent, but is there (should there be?) a need for sub-nodes to be able to access their parent nodes' parameters? I can make a case either way mentally, but I'm not sure what is the desired behavior here.

[jlack1987]

@aprotyas i'm referring to how it behaves with the change in this PR. I honestly don't have a preference one way or another whether params are accessible from other subnodes or parent nodes. I'm currently migrating a large code base from ros1->ros2 and I either have to fix the param behavior for sub nodes in ros2 or not use ros2 params 😭 so i'm very invested in fixing this however you all believe it should be done.

Based on the user interaction that I get from the way I have fixed it here, I think what I have here works, but I get it if it's not what you all want, so I'm open to changing the implementation

[clalancette]

I'm currently migrating a large code base from ros1->ros2 and I either have to fix the param behavior for sub nodes in ros2 or not use ros2 params sob so i'm very invested in fixing this however you all believe it should be done.

I think it is clear that subnodes are currently under-specified, so one other way out of this conundrum is to not use subnodes. That is, you may not want to get bogged down in fully specifying subnodes at this point, but you definitely want to use parameters; they are the right way forward. If you describe what you are using subnodes for, maybe we can suggest another way to do things.

Of course, if you want to fully specify subnodes, then that is great too. I think at that point we'd want to move this to a design discussion instead.

[jlack1987]

Yeah @clalancette i'll try to describe the conundrum i'm in from a code structure perspective. A representative example would ultimately be that I have classes that create classes that create classes and so on and so on for as long as you'd like to go. At every step of the way, the classes use the node handle given to them to obtain their configuration, and for the classes they create they pass it along downstream where more namespace branches are created for others to retrieve their configuration. At the end of the day everybody looks on their specific branch of the namespace tree to get their configuration, subscribe to and advertise their topics/services/actions etc. I then end up having 2 threads, one non-realtime thread that calls ros::spin() and the other RT thread runs everythings update loops. Also nearly every parameter I have is runtime configurable via dynamic reconfigure, so there's that wrench in there as well that's really pushing me to figure out how to fix this stuff so I can use ros2 parameters.

Fast forward to me trying to update this to ros2 and the way I configure the stack won't work anymore bc if I create two classes that have params of the same name and pass them sub nodes then there is a conflict, and this is something that happens at least hundreds of times in my code base. I thought perhaps as a work around I could just have all these classes create their own top level node, but then there's the issue of having hundreds of execution contexts. I guess for that i'd have to pass an executor all around my code base and manually add all the nodes to it, and i'm not even sure how well that would work for the nodes and their parameters. Here's a simple chunk of code that I think shows my plight

Keep in mind this is a silly chunk of code that wouldn't even compile, but I think it should display the type of problem i'm running into with using parameters in converting from ros1 to ros2. I'm gonna rely on you all reading between the lines a bit here with this example but I think you'll get it 😄

// Some basic PID controller class to show how it could be configured from rosparam
class PID
{
	PID(rclcpp::NodeSharedPtr node)
	{
		// create another leaf in the namespace tree, and grab your PID configuration parameters from it.
		// These calls would fail after the first PID construction with the current behavior of sub node params, bc 
		// they would be duplicate parameters
		node_ = node->create_sub_node("pid");
		node_->declare_param("kp");
		node_->declare_param("kd");
		node_->declare_param("ki");
	}
	rclcpp::NodeSharedPtr node_;
};
// simple joint struct
class Joint
{
	Joint(const std::string name) : name_(name)
	{
	}
	double position_, velocity_, effort_;
	std::string name_;
};
// Controller class that aggregates PID's 
class Controller
{
    Controller(rclcpp::NodeSharedPtr node, std::vector<JointsPtr> joints)
    {
        joints_ = joints;
    	// iterate through the joints, creating a PID for each joint
    	for(auto& joint : joints)
    	{
    		// make a PID for each joint, further branching the namespace out to the joint name
    		pids_->push_back(std::make_shared<PID>(node->create_sub_node(joint->name)))
    	}
    }
    std::vector<JointsPtr> joints_;
    std::vector<PIDPtr> pids_;
};
// "wholebody" controller that splits out control of subsystems
class WholeBody
{
    WholeBody() : node_(new rclcpp::Node("whole_body"))
    {
    }
    // joints vector here contains all joints on a bot
    bool init(std::vector<JointsPtr> joints)
    {
    	// assume I separate out the joints for each controller
    	left_arm_joints = ...
    	right_arm_joints = ...
    	left_leg_joints = ...
    	right_leg_joints = ...
    	head_joints = ...
    	//

    	// create some sub-system controllers, branching out the namespace tree to each one
    	controllers_.push_back(Controller(node_->create_sub_node("left_arm"), left_arm_joints));
    	controllers_.push_back(Controller(node_->create_sub_node("right_arm"), right_arm_joints));
    	controllers_.push_back(Controller(node_->create_sub_node("left_leg") left_leg_joints));
    	controllers_.push_back(Controller(node_->create_sub_node("right_leg"), right_leg_joints));
    	controllers_.push_back(Controller(node_->create_sub_node("head"), head_joints));
    	// and so on...
    }
	rclcpp::NodeSharedPtr node_;
	my_ns::ControllerPtr controllers_;
};

[fujitatomoya]

@jlack1987 @aprotyas @clalancette

thanks for constructive opinion, appreciate it.

atm, i was considering that subordinate node is just syntactic sugar to handle the namespace.

rclcpp/rclcpp/include/rclcpp/node.hpp

Lines 1236 to 1244 in d04319a

	/// Create a sub-node, which will extend the namespace of all entities created with it.
	/**
	* A sub-node (short for subordinate node) is an instance of this class
	* which has been created using an existing instance of this class, but which
	* has an additional sub-namespace (short for subordinate namespace)
	* associated with it.
	* The sub-namespace will extend the node's namespace for the purpose of
	* creating additional entities, such as Publishers, Subscriptions, Service
	* Clients and Servers, and so on.

but as you mentioned, there are some area we need to discuss.

but is there (should there be?) a need for sub-nodes to be able to access their parent nodes' parameters?

probably sub-nodes should not, but parent should?

[jlack1987]

@fujitatomoya is there any chance we could split the discussion? Yes there are design discussion things like you are asking about sub node param visibility that perhaps could go in other tickets, but there's also the situation of parameters for sub nodes being nearly unusable which is broken core functionality that, if it's acceptable to you all, i'd like to address as soon as we can all agree on a fix

[fujitatomoya]

I am still inclined to think that subordinate node manages the namespace expansion internally. but i understand that you have concrete problem, so if that is major opinion, i am okay to go. any advice? @aprotyas @clalancette @wjwwood @ivanpauno @iuhilnehc-ynos

[jlack1987]

manages the namespace expansion internally

@fujitatomoya I actually initially implemented the fix this way, but I realized quickly that namespacing parameters sorta doesn't make sense because it would mean the internally stored name of the parameter would have a bunch of namespaces in it which would be problematic when calling a users on_set_parameters_callback as well as for the has_param API bc it would have to search in names that have a bunch of namespaces prepended to the param name.

Think about this case,

rclcpp::Node::SharedPtr node("my_node");
rclcpp::Node::SharedPtr sub1 = node->create_sub_node("sub1");
rclcpp::Node::SharedPtr sub2 = node->create_sub_node("sub2");

sub1->declare_parameter("a");
sub2->declare_parameter("a");

// If the nodes all have a pointer to the same NodeParameters struct as it is now, and we implement namespace expansion
//  handling internally, then these two params would be stored internally as
// /my_node/sub1/a
// &
// /my_node/sub2/a

// what should this return? How does it know which one you want?
node->has_param("a");

// the user land code would have to check for /my_node/sub1/a to see if it exists

Additionally, when one of these params changes, the users on_set_parameters_callback would be getting parameters named /my_node/sub1/a instead of just a and it would be on the sub node user to know which a has been changed I guess.

[fujitatomoya]

@jlack1987

sub1->declare_parameter("a");
sub2->declare_parameter("a");

// If the nodes all have a pointer to the same NodeParameters struct as it is now, and we implement namespace expansion
// handling internally, then these two params would be stored internally as
// /my_node/sub1/a
// &
// /my_node/sub2/a

// what should this return? How does it know which one you want?
// it will be false if namespace is correctly propageted in sub-node.
node->has_param("a"); // FALSE
// these are true.
sub1->has_param("a"); // TRUE
sub2->has_param("a"); // TRUE

Additionally, when one of these params changes, the users on_set_parameters_callback would be getting parameters named /my_node/sub1/a instead of just a and it would be on the sub node user to know which a has been changed I guess.

isn't that sub1.a if sub1 calls set_parameter("a") and namespace is expended internally?
and your concern is that it is user callback needs to be aware of the namespace sub1 even if sub1 node calls add_on_set_parameters_callback? (saying this is the inconsistency reflects back on user application against normal Node? just checking if i understand correctly.)

[jlack1987]

@fujitatomoya I went ahead and implemented most of the "extend namespace internally" implementation so I could display the behavior and why I think it's not the best approach. Best way to look is to look at my test. So first I make a sub node and a sub node on that sub node here.

The end behavior is largely the same between the two implementations until we get to [adding on_set_parameters_callback callbacks] to the nodes. Looking then at the next line where I set subsubnode's parameter. When that is set, it calls the callback for both the other subnode and the parent nodes callbacks and notifies them that /sub_ns/subsub_ns/a has changed.

While this may seem like a minor annoyance for this simple test, for a large node that has dozens of sub nodes that all have sub nodes of their own and so on, it would mean possibly hundreds of callbacks would be hit every time any one of those subnodes parameters changes and in almost all of those callbacks they're probably being notified of a parameter change on a parameter that they have no knowledge even exists bc it was created in another sub node somewhere else in the tree of nodes/subnodes. In addition, the name of the parameter that changed would have to have the entire sub namespace prepended to it when the callback is called otherwise there'd be no way of differentiating between a parameter "a" that exists in multiple sub nodes. I hope this makes sense. Let me know what you think!

[fujitatomoya]

@jlack1987 yeah, i think that can be a problem for user experience. thanks for the explanation.

[fujitatomoya]

btw, this fix changes the current behavior for ros2 param list. Having different NodeParameters from parent node, it conceals subordinate node's parameter via ros2 param list.

[jlack1987]

@fujitatomoya yeah you are correct. I found a similar-ish issue where the list, get, etc services that nodes offer for their parameters weren't being advertised for sub nodes. I am able to fix that one relatively easily, but the ros2 param list issue I don't see how to fix. It seems to me that ros2cli gets a list of nodes and calls their <node_name>/list_parameters service to get all the parameters, but subnodes don't show up in the list of nodes to ros2cli and it's not entirely clear to me how it gets the list and what about sub nodes makes it not show up. If I had to take a wild guess i'd say it's probably something to do the node graph.

[jlack1987]

@fujitatomoya I think i'm at the end of my road here. I don't fully understand all the code enough to say for sure, but it seems that I can't make sub node params show up with ros2 param list calls and I can't get sub nodes to access parameters loaded from yaml files because subnodes share the same NodeBase pointer. But making it such that sub nodes don't share the same NodeBase pointer would mean that calling spin() on the parent node wouldn't spin the sub nodes.

If anybody has any advice on a possible implementation to fix this issue i'm open to any suggestions 🙏

[fujitatomoya]

but it seems that I can't make sub node params show up with ros2 param list

for allowing us to do that, i think eventually we need to create NodeBase (calling rcl_init_node) for subordinate node, which makes me think what's the difference between node and subordinate node ??? 🤔 ...

But making it such that sub nodes don't share the same NodeBase pointer would mean that calling spin() on the parent node wouldn't spin the sub nodes

this is true.

we can have discussion in MW WG, if you'd like to bring the topic. https://docs.ros.org/en/rolling/Governance.html#middleware, always welcome!

[wjwwood]

Sorry, I need to catch on this (long) thread to make a more thoughtful response, but my general feedback after skimming, is that a "sub-node" should completely be syntactic sugar on top of a Node, and to that end, it shouldn't require any additional entities at the rcl level (like additional node objects etc...). Additionally, I think it shouldn't require new instances of NodeParameter or anything like that... It should simply modify the incoming arguments when interacting with parameters, by adding implicit namespace unless the argument is absolute already.

Any other issues, like number of callbacks to process when there are lots of parameters, or separation of concern when listing parameters, should be address some other way other than having multiple places where we can store parameters within a node.

This is all my professional opinion about it, but I could be wrong and/or convinced otherwise. Hopefully I can find more time to iterate with you guys on it soon.

[wjwwood]

Added "more-information-needed" as a stand-in for "needs more discussion".

[jlack1987]

Yeah I agree with you @wjwwood, initially I thought there was a simple solution to the problem and implemented said solution but upon further digging it became clear that solving it is much more difficult than I thought. My only workaround right now is a painful one but it's all I can come up with, and it's to name parameters with their full namespace in the param file. Something like,

my_node:
  ros__parameters:
    /sub1/sub/sub3/param1: 1
    /sub1/sub/sub5/param1: 1
    /sub1/sub/sub3/sub4/param2: 3

Obviously this isn't great bc I have hundreds of parameters and some very long namespaces but I don't see any other solution.

[jlack1987]

Also, feel free to close this MR whenever as it's not a complete solution

[fujitatomoya]

"sub-node" should completely be syntactic sugar on top of a Node

i still think this is the way it is. it just takes care of namespace on top of parent node.

[jlack1987]

"sub-node" should completely be syntactic sugar on top of a Node

i still think this is the way it is. it just takes care of namespace on top of parent node

You are correct I think with the exception of parameters. With respect to sub nodes, the behavior of parameters is not intuitive and not the way they "should" behave in my opinion. I know ros2 is different from ros1, but there's a really nice mechanic from ros1 that we're obviously trying to mimic here and that's

// the ros1 way
ros::NodeHandle node("ns");
ros::NodeHandle sub_node(node, "sub_ns"); // topics, services, parameters are all namespaced here

// the ros2 way
rclcpp::Node::SharedPtr node("my_node");
rclcpp::Node::SharedPtr sub_node = node->create_sub_node("sub_ns"); // topics and services are namespaced, but parameters are not

and the current state of things does give the desired mechanics for topics and services, but not for parameters.

[tylerjw]

For context, I did not know about subnodes until I read this thread.

I've also been struggling with the idea of how to namespace parameters for different subsystems in a node and then handle reconfiguring those subsystems when a parameter is dynamically updated. The approach I've been working on so far is to have a single object that I pass around to each subsystem to handle the dynamic reconfiguring of subsystems. Each subsystem then registers callbacks for parameter updates to this dynamic reconfigure object. This dynamic reconfigure object has the one on_set_parameters_callback callback in my node and handles splitting the names of the parameters into namespaces and calling the appropriate subsystem's callback so it can then reconfigure itself. If there was a pattern built into ros2 for handling subsystems and their parameters with callbacks for changes it would be much nicer than implementing that functionality myself.

One large downside to my approach is that each subsystem has to have a namespace it is initialized with and uses for declaring and getting parameters. It also uses this namespace to register a callback for when any parameter in its namespace is changed.

So far I have not had to deal with sub-subsystems. I've done this so far by just having one level of namespaces for subsystems. Having features built into ROS for handling parameters of subsystems within a node would be really nice.

[aprotyas]

@fujitatomoya do you know if this was discussed in a MW WG meeting?

[fujitatomoya]

i remember that we had a quick talk but not really discussed, probably it would be nice to bring this back to topic.

[jlack1987]

Gonna glose this MR as the approach is ultimately flawed. I have linked to this PR though in the issue for which this PR attempts to fix so others can find their way here and see the discussion which I think could be useful to others.