Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cannot load yaml config to composable node in dashing #715

Closed
christianrauch opened this issue May 3, 2019 · 15 comments
Closed

Cannot load yaml config to composable node in dashing #715

christianrauch opened this issue May 3, 2019 · 15 comments
Assignees
Labels
question Further information is requested

Comments

@christianrauch
Copy link

Bug report

I used to load a yaml configuration file into a composable node with ROS crystal that is started from a standalone executable. This stopped working after porting to dashing. The lack of parameter file loading examples for composable nodes also makes debugging this more difficult.

Required Info:

  • Operating System: Ubuntu 18.04
  • Installation type: binary
  • Version or commit hash: dashing
  • DDS implementation: Fast-RTPS
  • Client library (if applicable): rclcpp

Steps to reproduce issue

  • create a node, e.g. AprilTag2Node::AprilTag2Node(rclcpp::NodeOptions options) : Node("apriltag2", "apriltag", options.use_intra_process_comms(true))
  • create a config config.yaml with with parameters:
apriltag:                           # namespace
    apriltag2:                      # node name
        ros__parameters:
            image_transport: raw
            size: 0.162
            parameter1: 89
            parameter2: 56.89
  • load a yaml file like: ros2 run your_package your_node __params:=<path_to>/config.yaml

Expected behavior

A call to get_parameter_or<double>("size", tag_edge_size, 2.0); within the node should provide the value of the parameter in the yaml file.

Actual behavior

The default parameter is returned.

Additional information

The main change for porting from crystal to dashing is the adaptation of the node options:

-AprilTag2Node::AprilTag2Node() : Node("apriltag2", "apriltag", true) {
+AprilTag2Node::AprilTag2Node(rclcpp::NodeOptions options) : Node("apriltag2", "apriltag", options.use_intra_process_comms(true)) {

This should keep the namespace intact and not change the behaviour of setting parameters.

@christianrauch
Copy link
Author

A minimal example composable node: test.zip. I skipped over some important bits to make the node actually composable to make it compilable on crystal and dashing.

If you compile and run:
ros2 run test test_node __params:=`ros2 pkg prefix test`/share/test/cfg/demo.yaml
it will read the parameter a_string correctly on crystal and print message: Hello world, while on dashing it will use the default value and print message: ....

@wjwwood
Copy link
Member

wjwwood commented May 5, 2019

If I understand your issue correctly, this is an intentional change in behavior with #495. Basically, you should update your code to use node->declare_parameter("size", tag_edge_size, 2.0) once and node->get_parameter("size", tag_edge_size) from then on (assuming you access it more than once).

The other two options to consider are:

  • allow_undeclared_parameters which allows you to get/set parameters without first declaring them, though I don't recommend you use this, it is the easiest flag to flip to go to the old behavior.
    • /// Set the allow_undeclared_parameters, return this for parameter idiom.
      /**
      * If true, allow any parameter name to be set on the node without first
      * being declared.
      * Otherwise, setting an undeclared parameter will raise an exception.
      */
      RCLCPP_PUBLIC
      NodeOptions &
      allow_undeclared_parameters(bool allow_undeclared_parameters);
  • automatically_declare_initial_parameters : which will automatically declare parameters for you based on what you pass in a yaml file or via the node option's initial_parameters (I also don't recommend this either).
    • /// Set the automatically_declare_initial_parameters, return this.
      /**
      * If true, automatically iterate through the node's initial parameters and
      * implicitly declare any that have not already been declared.
      * Otherwise, parameters passed to the node's initial_parameters, and/or the
      * global initial parameter values, which are not explicitly declared will
      * not appear on the node at all.
      * Already declared parameters will not be re-declared, and parameters
      * declared in this way will use the default constructed ParameterDescriptor.
      */
      RCLCPP_PUBLIC
      NodeOptions &
      automatically_declare_initial_parameters(bool automatically_declare_initial_parameters);

I recommend you use declare_parameter, rather than one of these options.

Sorry, we haven't yet updated the migration page on the dashing doc page:

https://index.ros.org/doc/ros2/Releases/Release-Dashing-Diademata/#changes-since-the-crystal-release

But we're working on that right now in preparation for pushing out changes for our API freeze.

@wjwwood wjwwood added the question Further information is requested label May 5, 2019
@christianrauch
Copy link
Author

Indeed, it works after declaring all the parameters beforehand. Thanks.

Maybe a call to get_parameter could give a warning if this parameter has not been declared before?

@christianrauch
Copy link
Author

I am still struggling with configuring a composable node.
Could an example of configuring a composable node (manually and via yaml files) be added to the documentation and the demos repository?

If I launch a composable node via:

def generate_launch_description():
    composable_node = ComposableNode(
            node_name='apriltag',
            package='apriltag_ros', node_plugin='AprilTagNode',
            remappings=[("/apriltag/image", "/camera/image"), ("/apriltag/camera_info", "/camera/camera_info")],
            parameters=[get_package_share_directory('apriltag_ros')+"/cfg/tags_16h5_all.yaml"])
    container = ComposableNodeContainer(
            node_name='tag_container',
            node_namespace='apriltag',
            package='rclcpp_components',
            node_executable='component_container',
            composable_node_descriptions=[composable_node],
            output='screen'
    )

I now get the parameters correctly listed as:

$ ros2 param list 
/apriltag/apriltag:
  size
  [...]

i.e. parameter size of node apriltag in namespace apriltag.

However, it is not recognising the value in the configuration file:

apriltag:
    apriltag:
        ros__parameters:
            size: 0.162             # tag edge size in meter
$ ros2 param get /apriltag/apriltag size
Double value is: 2.0

@wjwwood
Copy link
Member

wjwwood commented May 5, 2019

Maybe a call to get_parameter() could give a warning if this parameter has not been declared before?

It should raise an exception:

* If the parameter has not been declared, then this method may throw the
* rclcpp::exceptions::ParameterNotDeclaredException exception.

Unless you're using allow_undeclared_parameters, which I would advise against.

If you're using get_parameter_or() will intentionally not raise an exception.

However, it is not recognising the value in the configuration file:

Are you using declare_parameters or get_parameter_or, or maybe one of the other options?

@christianrauch
Copy link
Author

Yes, I was using get_parameter_or() before. get_parameter() actually throws an exception (although generic).

@wjwwood
Copy link
Member

wjwwood commented May 6, 2019

I'll try to reproduce this soon (or maybe one of the other maintainers can if I can't), but there are tests for this case (getting values from initial parameter values), e.g.:

{
// int default, with initial
rclcpp::ParameterValue default_value(43);
rclcpp::ParameterValue value = node->declare_parameter("parameter_and_default", default_value);
EXPECT_EQ(value.get_type(), rclcpp::PARAMETER_INTEGER);
EXPECT_EQ(value.get<int>(), 42); // and not 43 which is the default value
}

But it could be an issue with composition specifically, or yaml parsing, or the particular invocation you're using... We'll have to see.

@christianrauch
Copy link
Author

I created a simple composable node with launch files: test.zip

I can load the node successfully with configuration using:

  • ros2 component load /ComponentManager test Test -r __params:=`ros2 pkg prefix test`/share/test/cfg/demo.yaml
  • ros2 launch test test_cfg_dict.launch.py (configuration via dict)

However, loading the config from the yaml file via the parameters argument of ComposableNode (ros2 launch test test_cfg_yaml.launch.py) does not set the parameter.
According to composable_node.py the parameters can be a list of paths to yaml files. Maybe there is an issue with the correct namespace of the node/configuration file?

@christianrauch
Copy link
Author

To load the yaml file in the launch script, I need to provide a configuration file without node name and without ros__parameters, i.e. a simple plain yaml file like this:

some_int: 42
a_string: "Hello world"

although the yaml syntax is given different at https://github.com/ros2/rcl/tree/master/rcl_yaml_param_parser.

Is this intended? It would be good if configuration files could be reused for the same node (with the same name), regardless if it is launched from a standalone node, via the component manager or via the launch file.

However, in this case the launch file won't deal with list. E.g. adding:

some_integers: [1, 2, 3, 4]

to the configuration causes: [ERROR] [launch]: Caught exception in launch (see debug for traceback): invalid parameter value (1, 2, 3, 4)

@mjcarroll
Copy link
Member

Hey @christianrauch, @wjwwood and I spent some time discussing this today, and here's the conclusion for the dashing release:

The way that you are currently passing the parameters to the load command ros2 component load /ComponentManager test Test -r __params:=ros2 pkg prefix test/share/test/cfg/demo.yaml is essentially a remapping argument. The parameter file that is read in that case has to have the parameters nested under node_name/ros__parameters in the YAML file, which is consistent with how a standalone node will work. In this case though, the container does not receive the parameters via the parameters field in the service call, but instead receives a remapping argument for __params.

The current implementation of the ComposableNode launch description does not understand the nested structure in the YAML file, which causes the parameters to be (incorrectly) expanded to:

CompositionContainer received parameters:
name: test_node_otto.ros__parameters.some_int, type: integer
name: test_node_otto.ros__parameters.a_string, type: string

I think that there are two potential short-term workarounds (without requiring major changes in the way that the ComposableNode description works)

  1. Pass your existing yaml file in as a remapping argument in the launch description:
composable_node = ComposableNode(package='test', node_plugin='Test',
                                 remappings=[('__params', get_package_share_directory('test')+"/cfg/demo.yaml")])

This will then pass in parameters similar to the load call above.

  1. Use a new yaml file that doesn't have any of the nesting:
some_int: 42
a_string: "Hello World"

And then pass that in as parameters to the ComposableNode description:

composable_node = ComposableNode(package='test', node_plugin='Test',
                                     parameters=[get_package_share_directory('test')+"/cfg/demo_no_nesting.yaml"])

The caveat on approach 1 is that any additional loose parameters passed via the parameters kwarg will always supersede what's in the remappings argument, where in approach 2, the ordering in the list passed to parameters is important (later definitions will supersede earlier definitions).

So in the case of:

composable_node = ComposableNode(
        package='test',
        node_plugin='Test',
        parameters=[
            'some_file.yaml',  # {something: 'foo'}
            {'something': 'bar'},
            'some_other_file.yaml',  # {something: 'baz'}
        ])

The parameter something will be baz, but in the case of:

composable_node = ComposableNode(
        package='test',
        node_plugin='Test',
        parameters=[{'something': 'blah'}],
        remappings=[('__params', get_package_share_directory('test')+"/cfg/demo.yaml")])

The parameter something will be blah.

sharronliu pushed a commit to sharronliu/ros2_intel_realsense that referenced this issue Jul 2, 2019
According to discussion in RCLCPP, for ROS2 Dashing release,
"declare parameter" is expected to load configures from yaml file.

See detail discussion here:
ros2/rclcpp#715

Signed-off-by: Sharron LIU <sharron.liu@intel.com>
sharronliu pushed a commit to sharronliu/ros2_intel_realsense that referenced this issue Jul 2, 2019
Fix issue intel#55

According to discussion in RCLCPP, for ROS2 Dashing release,
"declare parameter" is expected to load configures from yaml file.

See detail discussion here:
ros2/rclcpp#715

Signed-off-by: Sharron LIU <sharron.liu@intel.com>
sharronliu pushed a commit to sharronliu/ros2_intel_realsense that referenced this issue Jul 2, 2019
Fix issue intel#55

According to discussion in RCLCPP, for ROS2 Dashing release,
"declare parameter" is expected to load configures from yaml file.

See detail discussion here:
ros2/rclcpp#715

Signed-off-by: Sharron LIU <sharron.liu@intel.com>
@SteveMacenski
Copy link
Collaborator

@mjcarroll any update on this? The last of the discussion looks to be before the dashing release. We're coming up on Foxy and this feels like something that should be included in the next LTS.

@Myzhar
Copy link

Myzhar commented Oct 21, 2020

I cannot understand why ComposableNode cannot work in the same way as Node works...

I'm facing this issue too: I could not understand why my component manually composed in a launch file did not correctly load the parameters from the YAML files and I finally found this issue.

I think that the workarounds proposed by @mjcarroll are good, but I cannot use both of them:

  1. My node loads the parameters from two different YAML files, so __params as argument cannot be used
  2. I cannot modify the YAML file removing the nesting because it is used by other nodes with the /**: "trick".

Other solutions?

PS I'm using Eloquent...

@facontidavide
Copy link

I have been bitten by this too :(

@RFRIEDM-Trimble
Copy link

Me too! I ran into it in Foxy. We cannot use the same param files for ComposableNode as Node

@ivanpauno
Copy link
Member

The fact that in launch the parameters file format is different when using ComposableNode than when using Node, is a launch issue and not an issue in rclcpp, so I will close in favor of ros2/launch_ros#156.

The other issues mentioned here seem to have been resolved.

nnmm pushed a commit to ApexAI/rclcpp that referenced this issue Jul 9, 2022
* Fix missing call fini() for lifecycle_transition and node in test_rcl_lifecycle

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

* Fix error overwritten while allocator is Nullptr.

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

* Optimize used variables

Signed-off-by: Barry Xu <barry.xu@sony.com>
DensoADAS pushed a commit to DensoADAS/rclcpp that referenced this issue Aug 5, 2022
* use rclcpp::Node for generic pub/sub

Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>

* address review comments

Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>

* export symbols on windows

Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>

* use rclcpp logging macros

Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>

* do not use node for error logging

Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

8 participants