Support both parameter file configurations for composable nodes

[rebecca-butler]

Closes #156. With these changes, loading a composable node will work with both parameter file configs:

param: value

and

namespace/my_node:
  ros__parameters:
    param: value

Signed-off-by: Rebecca Butler rebecca@openrobotics.org

[ivanpauno]

Seems like a reasonable approach to me:

• Could you add a test for it?
• Could you add a release note related to this?

[hidmic]

@rebecca-butler hmm, a few questions about behavior:

• What if no node name was specified for the node on launch?
• What if there's no entry in the parameter file for that node name?
• What if a wildcard was used in the file?

[rebecca-butler]

I've written a few test cases. While doing this I realized there's another format we should probably support:

namespace:
  node_name:
    ros__parameters:
      param: value

I changed the implementation so now it combines all the keys that come before "ros__parameters" to get the full name.

[rebecca-butler]

@hidmic to address your questions:

What if no node name was specified for the node on launch?

What is the expected behaviour for this case? Currently if no node name is provided, then parameter files using the node_name: ros__parameters format would do nothing since the node name won't match. Parameter files using just the dictionary format will always work regardless of what the node name is.

What if there's no entry in the parameter file for that node name?

We only remove the node_name: ros__parameters part of the dictionary if an entry is found. evaluate_parameter_dict() doesn't work if node_name: ros__parameters is still in the dict, so in cases where no entry is found, the parameters aren't evaluated correctly and nothing happens. This means parameters won't be applied to a node unless it matches the name in the yaml file. (The exception is if the yaml file is using the old format that doesn't contain node_name: ros__parameters, in which case it will always work.)

What if a wildcard was used in the file?

Good point, I didn't consider that. I've added support for the /** wildcard in 97ede21 since it's easy to do, but I'm not sure how this would work for other wildcards. Do we have a public API somewhere for wildcard matching?

[hidmic]

What is the expected behaviour for this case? Currently if no node name is provided, then parameter files using the node_name: ros__parameters format would do nothing since the node name won't match. Parameter files using just the dictionary format will always work regardless of what the node name is.

Assuming launch proceeds (silently or with warning) in that case, we should still document this properly i.e. that if launch is not explicitly told what the node name is, it won't be able to pick up parameters from a parameter file including ros__parameters entries.

Note this is not the case when launching plain Nodes (because the process bearing the nodes knows all node names, hard-coded or not, and it can look up the parameter file).

so in cases where no entry is found, the parameters aren't evaluated correctly and nothing happens.

So long as launch handles this case and proceeds, 👍

Do we have a public API somewhere for wildcard matching?

For parameters I think not. There's a design document for wildcard matching in remapping rules, from which I believe parameter wildcards are inspired on. Anyhow, IIRC /** is the only pattern currently supported.

[ivanpauno]

Assuming launch proceeds (silently or with warning) in that case, we should still document this properly i.e. that if launch is not explicitly told what the node name is, it won't be able to pick up parameters from a parameter file including ros__parameters entries.

Preferedly, we should log a warning in that case, it seems to be something that can be detected

[jacobperron]

I don't think two leading slashes should be a valid node name, I would have expected the name to be /test_node_name.

[rebecca-butler]

I think get_node_name_count() expects the name to be of the form /ns/node_name, so if the namespace is empty, it has to be //node_name 🤷 It doesn't work without the extra slash.

[hidmic]

Kudos for all those tests !

[jacobperron]

I've address the most recent comments. Ready for another round of review.

[hidmic]

Overall LGTM. __format_dict implementation is a bit unusual but OK.

[ivanpauno]

I have some minor comments, LGTM!

[adityapande-1995]

@ros-pull-request-builder retest this please

[jacobperron]

RPr job is failing because it can't find the test data file example_parameters.yaml, though test_node.py uses the same file and doesn't have the same problem.

I can't reproduce locally.

[adityapande-1995]

I tried this as well, couldn't reproduce the error. It is weird cause there are other parameter files as well, which seem to be working fine.

[jacobperron]

On a whim, I've pushed a commit that makes the failing test use a copy of example_parameters.yaml (bd1bc63).

[jacobperron]

On a whim, I've pushed a commit that makes the failing test use a copy of example_parameters.yaml (bd1bc63).

The job passes now... but why? 😕

[jacobperron]

Testing everything above launch_ros:

• Linux
• Linux-aarch64
• macOS
• Windows

[chenjunnn]

This enhancement does a great help to the project I'm working on which is based on ROS Galactic. So I wonder when will it be updated to launch_ros package on Galactic?

And thank you for your great work on ROS.

[jacobperron]

The changes to to_parameters_list break API, so I don't think we should directly apply this to Galactic. Though, it may be easy enough to introduce a new function that is used by composable nodes.

[guru-florida]

I just ran into this on Galactic as well. Seems like an odd and unexpected thing to change parameter config between regular nodes and components. Thanks to everyone here who worked on this! (awaiting back-patch but glad to have a work-around until then.)