fixup parameters and sub-nodes

[wjwwood]

This is a follow up from https://github.com/ros2/rclcpp/pull/495#issuecomment-485906580.

Currently how parameters and "sub-nodes" interact is not documented and is not consistent or tested. That needs to be documented, audited, and tested.

[mikeferguson]

So, is this ticket basically tracking that sub nodes do not actually use the sub_namespace in the name of the parameter? For example:

auto subnode = parent->create_sub_node("controller_name");
std::string controller_type = subnode->declare_parameter<std::string>("type", "");

I would expect the parameter to be "controller_name/type", but instead "ros2 param list" shows only a parameter named "type"? Is this expected behavior? Is this a bug?

[fujitatomoya]

I am not sure about this either, I'd like to know.

I would expect the parameter to be "controller_name/type", but instead "ros2 param list" shows only a parameter named "type"?

currently, yes. only type is declared w/o namespace.

I think that this is bug, it should be extended with sub_namespace?

[wjwwood]

Yeah @mikeferguson I consider it a bug, or at least I think it will be surprising to most users.

The issue (why I didn't just fix it on the spot) is that the way the node class is separated into parts complicates this. The subnode feature was added to Node, but not to one of the components, meaning it's hard to use from the NodeParameters class. This is just my recollection of the issue, but maybe it's easier to fix now, I don't know off hand without trying to do it.

[jlack1987]

I'm willing to spend some time attempting to fix this issue, but i'd appreciate it if I could get a more involved devs input on a suggested fix/implementation. I can just run off and implement a fix, but I don't want to end up spending a lot of time on a fix and it not being an acceptable implementation

[fujitatomoya]

just sharing the update. we had a quick chat on this in MW WG. we consider this is bug, all entities of sub-nodes should be extended and propagated with sub-nodes namespace. (topics, services and parameters) as @wjwwood mentioned https://github.com/ros2/rclcpp/issues/731#issue-446871224 these should be tested well.

[jlack1987]

I'm curious if this could be solved by injecting the sub namespace into the parameter name in the call here. The reason i'm curious if this is an adequate solution is based on how the declare_parameters call behaves in that it already takes a namespace and, in the case where the namespace arg is non-empty, creates parameters that are namespace.name.

So would it be valid to simply namespace parameters into the sub_namespace_ which is already available and if it's empty it'll continue to behave as it does now for a non-subordinate node? I'm relatively new to ros2 so there may be fallout from this strategy that I'm currently unaware. Would appreciate it if I could get a devs input on this. Thanks!

Edit: I think additionally there's an inconsistency with there being no API call to set_parameters that takes a namespace. At least it's not clear to me why there is no such call but there is a declare_parameters call that accepts a namespace. In the case where allow_undeclared_parameters is true I would think that call should be ok

[jlack1987]

I have also found that if you declare parameters with a sub node n with,

std::map<std::string, double> parameters = { { "a", 1. }, { "b", 2. }, { "c", 3. } };
n->declare_parameters(n->get_sub_namespace(), parameters);

and then retrieve those params with,

n->get_parameter<double>(n->get_sub_namespace() + ".b", param_b);
rclcpp::Parameter param = n->get_parameter(n->get_sub_namespace() + ".b");

that the second call there does get the correct parameter, but the first call doesn't find it and returns what looks like some uninitialized number(7.892878e-293 in one run for example). This is because the templated call prepends the parameter name with the sub node namespace, but the other two API calls for getting a parameter don't do that

[aprotyas]

rclcpp::Parameter param = n->get_parameter(n->get_sub_namespace() + ".b");

Quite uninformed thought - and not to divert the topic of conversation here - but aren't namespaces delimited by "/" and not "."?

[jlack1987]

haha yeah I was surprised by this as well @aprotyas. Idk if that's a typo that squeaked through review or what but that single declare_parameters call prepends the param name with a .. I'm guessing since the . key is right next to a / that it went unnoticed?

[clalancette]

rclcpp::Parameter param = n->get_parameter(n->get_sub_namespace() + ".b");

Quite uninformed thought - and not to divert the topic of conversation here - but aren't namespaces delimited by "/" and not "."?

Yes, but parameter "levels" are separated by ., which I think is what this is asking for. I'm not sure though, I'd need to look at the code more closely to see the context.

[jlack1987]

Is this notion of "parameter level" documented somewhere? I did a quick search and didn't see anything

[clalancette]

It's basically this: https://docs.ros.org/en/rolling/How-To-Guides/Node-arguments.html#setting-parameters-from-yaml-files

That is, when you declare a parameter in YAML like:

parameter_blackboard:
    ros__parameters:
        some_int: 42
        some_lists:
            some_integers: [1, 2, 3, 4]
            some_doubles : [3.14, 2.718]

Then you retrieve them using names like some_int, some_lists.some_integers, and some_lists.some_doubles.

[jlack1987]

I had a PR for this here, but the approach was ultimately flawed so I closed it; however, there is a good deal of discussion in there that someone may find useful so I just wanted to link to it here.

[fujitatomoya]

I added this to next MW WG topic not to miss it.

[wjwwood]

@wjwwood to leave an idea about how to do this without creating new NodeParameter instances, but still get namespacing for parameter methods of the node class.