ros2 / rclcpp

rclcpp (ROS Client Library for C++)
Apache License 2.0
541 stars 418 forks source link

composition LoadNode does not set parameters for nodes in other namespaces #2404

Open Aposhian opened 9 months ago

Aposhian commented 9 months ago

Bug report

Required Info:

Steps to reproduce issue

Some projects (in this case Nav2) have nodes that create other nodes that are not necessarily in the same namespace. When these nodes are loaded via composition, parameters passed to the LoadNode service are not propagated to these "sibling" nodes. This is a change in behavior from when not using composition: if you set parameters for the process, and then create any number of nodes, they all get their parameters.

ros2 run tf2_ros static_transform_publisher --frame-id map --child-frame-id base_link

ros2 run rclcpp_components component_container

ros2 service call /ComponentManager/_container/load_node composition_interfaces/srv/LoadNode "{ package_name: "nav2_planner", plugin_name: "nav2_planner::PlannerServer", parameters: [ {name: "global_costmap.global_costmap.resolution", value: { type: 3, double_value: "1.23"}}]}"
ros2 lifecycle set /planner_server configure
ros2 lifecycle set /planner_server activate
ros2 param get /global_costmap/global_costmap resolution

Expected behavior

ros2 param get /global_costmap/global_costmap resolution outputs 1.23

Actual behavior

ros2 param get /global_costmap/global_costmap resolution outputs 0.1 (default)

Additional information

Setting parameters in the container works for this use-case. This may be a "won't fix" if this type of node creation is not desirable, and maybe could be replaced with sub nodes. https://github.com/ros-planning/navigation2/issues/4011

SteveMacenski commented 9 months ago

To be concise: This issue is that sometimes a component node is itself a manually composed set of nodes. The other nodes within the component are not getting their parameters forwarded even though they're in the file passed to the component to load with the node. There's clearly a filter happening somewhere in the parameter namespaces, I think that should be relaxed. I don't see any downsides to passing a nodes more than it asked for, but there are downsides in passing less.

The current way we get around this is to pass the param file to the container on construction which contains all nodes, but that's obviously not always a design pattern that can be followed.

sloretz commented 9 months ago

If I understand correctly, when components are loaded the parameters/remap rules/ros args are passed into the node via NodeOptions. See here where the NodeOptions are created: https://github.com/ros2/rclcpp/blob/7901bb9da4e1addf71a307230afa60bd8d72f4f6/rclcpp_components/src/component_manager.cpp#L238

Outside of NodeOptions, the other way to get arguments is via the CLI. These are considered "Global arguments" because they apply to all nodes in the process. There's an option to disable them.

https://github.com/ros2/rclcpp/blob/7901bb9da4e1addf71a307230afa60bd8d72f4f6/rclcpp/src/rclcpp/node.cpp#L145-L147

I think global arguments on composable nodes are by default disabled. When loading composable nodes one normally doesn't want the node to get the parameters of the component manager, or of any other node loaded into the component manager. There is an option to allow it though:

https://github.com/ros2/rclcpp/blob/7901bb9da4e1addf71a307230afa60bd8d72f4f6/rclcpp_components/src/component_manager.cpp#L177-L187

This issue is that sometimes a component node is itself a manually composed set of nodes. The other nodes within the component are not getting their parameters forwarded even though they're in the file passed to the component to load with the node.

I would recommend instead making the top level node pass the arguments from its own NodeOptions into the NodeOptions of the nodes it is manually composing. Here's the API to get the options off the node that's doing the composition. https://github.com/ros2/rclcpp/blob/7901bb9da4e1addf71a307230afa60bd8d72f4f6/rclcpp/src/rclcpp/node.cpp#L675-L678

SteveMacenski commented 8 months ago

I grokk what you're saying, but not sure that addresses our problem - but happy to be proven obtuse hah. This is the specific example we had in mind:

As you can see, we don't do anything with parameters / param file fed into the components from the launch file beyond a little bit of remapping of the node's name/namespace. While this is our exact example, it doesn't do anything wonky beyond that remapping node name/namespace, so I feel this is actually a generic problem with composing a node within a component node.

If there's something we can do to get around that like passing in some of the main node's attributes, that would be totally workable -- though seems like something may be wrong with component container's settings since this works fine with "normal" nodes launched via Node() while LoadComponentNode() doesn't (which I logically track to rclcpp_component instead of launch).

Aposhian commented 8 months ago

@sloretz are you suggesting that in the scenario that @SteveMacenski outlined, we could call get_node_options on the PlannerServer, and then feed that NodeOptions object when constructing the Costmap2D?

Since we are using launch_ros, we would also need to verify that it is not also filtering out parameters outside the namespace.

sloretz commented 8 months ago

are you suggesting that in the scenario that @SteveMacenski outlined, we could call get_node_options on the PlannerServer, and then feed that NodeOptions object when constructing the Costmap2D?

Pretty much. I'm not sure if the whole NodeOptions is desirable since there are a lot of options there, but at the very least I think you'd want to create a new NodeOptions and then set its arguments and parameter overrides to the ones gotten from PlannerServer's NodeOptions before passing them to the contructor of Costmap2DROS.

https://github.com/ros2/rclcpp/blob/a5221336f64e9754377e5391663dc6c7f3da3975/rclcpp/include/rclcpp/node_options.hpp#L105-L119

https://github.com/ros2/rclcpp/blob/a5221336f64e9754377e5391663dc6c7f3da3975/rclcpp/include/rclcpp/node_options.hpp#L121-L138

SteveMacenski commented 8 months ago

@Aposhian do you want to prototype to make sure it solves the issue? Happy to merge something like that in. I could do it, but I wouldn't feel confident that all the items you're looking for are resolved as you're the reporter.

wjwwood commented 7 months ago

Closing, based on discussion in our weekly issue triage meeting. Feel free to ask for it to be reopen if anyone thinks it needs more discussion.

SteveMacenski commented 7 months ago

I think we're good, thanks

SteveMacenski commented 6 months ago

I'm attempting to address this in Nav2 (https://github.com/ros-planning/navigation2/issues/4011#issuecomment-2033306272) but running into the issue that it doesn't appear to work by adding in the parent node's arguments and parameter overrides.

@Aposhian tested by manually adding in a parameter into the LoadNode service and still failed, so is there a reason to believe that the NodeOptions is culling by namespace or node name on loading so that if passing a NodeOption object among other internal nodes with their own node names or namespaces it wouldn't be able to obtain its subset of information?

Using the test case he setup

ros2 run rclcpp_components component_container --ros-args -r __node:=nav2_container
ros2 launch nav2_bringup navigation_launch.py params_file:=./nav2_bringup/params/nav2_params.yaml use_composition:=True

I can see that if I change a PlannerServer parameter in the yaml file that that is being represented by when launched (i.e. mistype a plugin name so it throws an error), but I can't seem to see that it ever ACKs changes of sub-nodes even when passing it in the parent node options as shown in the blurb in the link above

I've even tried setting the NodeOptions for internal nodes to the parent's NodeOptions in entirety and adding its specifics on top of it, and it doesn't get the parameters in the yaml either:

: nav2_util::LifecycleNode(name, "",
    rclcpp::NodeOptions(parent_options).arguments({
    "--ros-args", "-r", std::string("__ns:=") +
    nav2_util::add_namespaces(parent_namespace, local_namespace),
    "--ros-args", "-r", name + ":" + std::string("__node:=") + name,
    "--ros-args", "-p", "use_sim_time:=" + std::string(use_sim_time ? "true" : "false"),
  })),

I believe then, this is a problem for any setup with manually composed nodes who's highest level is composed dynamically (making up the manually composed components)