Use SetParameter Launch API to set the yaml filename for map_server

[stevedanomodolor]

---

Basic Info

Info	Please fill out this column
Ticket(s) this addresses	(add tickets here #3149 )
Primary OS tested on	(Ubuntu)
Robotic platform tested on	(gazebo)

---

Description of contribution in a few bullet points

• Use SetParameter Launch API to set the yaml filename for map_server

Description of documentation updates required from your changes

• Remove the note added to the navigation.ros.org configuration for map server explaining that yaml_filename is strictly required.

---

Future work that may be required in bullet points

For Maintainers:

• [x] Check that any new parameters added are updated in navigation.ros.org
• [ ] Check that any significant change is added to the migration guide
• [ ] Check that any new features OR changes to existing behaviors are reflected in the tuning guide
• [ ] Check that any new functions have Doxygen added
• [ ] Check that any new features have test coverage
• [ ] Check that any new plugins is added to the plugins page
• [ ] If BT Node, Additionally: add to BT's XML index of nodes for groot, BT package's readme table, and BT library lists

[mergify[bot]]

@stevedanomodolor, please properly fill in PR template in the future. @stevemacenski, use this instead.

• [ ] Check that any new parameters added are updated in navigation.ros.org
• [ ] Check that any significant change is added to the migration guide
• [ ] Check that any new features OR changes to existing behaviors are reflected in the tuning guide
• [ ] Check that any new functions have Doxygen added
• [ ] Check that any new features have test coverage
• [ ] Check that any new plugins is added to the plugins page
• [ ] If BT Node, Additionally: add to BT's XML index of nodes for groot, BT package's readme table, and BT library lists

[stevedanomodolor]

@SteveMacenski I am still not sure if a group action is required in order to use the setparameter or it can be used under the Node action directly, there is not that much documentation. I have to test it to see if it is necessary. I have not tested this commit thats why its on draft.

[mergify[bot]]

@stevedanomodolor, your PR has failed to build. Please check CI outputs and resolve issues. You may need to rebase or pull in main due to API changes (or your contribution genuinely fails).

[SteveMacenski]

if this all works out - you can also remove the yaml_filename from the yaml files. Leave it there, but just comment it out and update the comment above it explaining that it doesn't need to be specified since going to be set by defaults in launch. If you'd rather set it in the yaml, remove the defaults in launch and set the full filepath here.

And subsequent updates in docs

Thanks so much for the help! And @tonynajjar for the idea!

[stevedanomodolor]

@SteveMacenski @tonynajjar. i have tried setting the parameters but still get the same error

[ERROR] [launch]: Caught exception in launch (see debug for traceback): 'NoneType' object is not iterable I am supposed to declare the yaml_filename in a specific way. There is not too much documentation on it.

[mergify[bot]]

@stevedanomodolor, your PR has failed to build. Please check CI outputs and resolve issues. You may need to rebase or pull in main due to API changes (or your contribution genuinely fails).

[SteveMacenski]

What if yaml_filename is non-empty string? Then does it work? I'm curious if its failing because its just not set (and then we could use a launch action condition to only set the parameter if its set to something).

It doesn't seem to be that there needs to be a declaration: https://github.com/ros2/launch_ros/blob/rolling/launch_ros/launch_ros/actions/set_parameter.py#L38-L55 but it does seem to me like it might need to exist

[stevedanomodolor]

What if yaml_filename is non-empty string? Then does it work? I'm curious if its failing because its just not set (and then we could use a launch action condition to only set the parameter if its set to something).

It doesn't seem to be that there needs to be a declaration: https://github.com/ros2/launch_ros/blob/rolling/launch_ros/launch_ros/actions/set_parameter.py#L38-L55 but it does seem to me like it might need to exist

I am performing all test with the tb3_simulation_launch.py with ros rolling(used for development) What if yaml_filename is non-empty string? -> I am assuming you are referring to the yaml_filename in the parameter file. When the yaml_filename parameter in the parameter list file is set to an empty string , the launch file execute with no map independent of if the setparameter was set or not. In this case the param substitution is not used. And when the yaml_filename parameters is not set in the parameter file and the param substitution not used, it gives the error nonetype as said previously. I also tried setting the default parameters in the launch file to non empty string but still gives the nonetype error. The set parameter does not seem to influence at all the behavior. I am assuming that the underlining issue is the fact that the variable type is a string, in comparison to the use_sim_time which is a boolean. @tonynajjar any thought?

[SteveMacenski]

What if yaml_filename is non-empty string? -> I am assuming you are referring to the yaml_filename in the parameter file.

I meant in the launch file declaration, so that when you call SetParameter() there's a non-empty value to it. This is just a shot in the dark but does SetParameter(name='my_param', value='2') API work (explicitly setting name/value)? I've found some oddities like that in the past with rclpy.

I don't think the type has anything to do with it. @tonynajjar any thoughts?

[stevedanomodolor]

@SteveMacenski The issue was from my side. I had an issue with the docker, I have fixed it now.

[tonynajjar]

Great, I was about to have a look. If you have another issue, please make sure to push your code so I can try to reproduce (can't guarantee a timely response though)

[SteveMacenski]

@stevedanomodolor so this works?

• With map file in the .yaml file -- uses that (if default in launch configuration is empty string)
• Without any map defined in the .yaml file -- uses default in launch configuration

I was thinking we might need an IfCondition to apply the SetParameter only if the value of it in the launch configuration was non-empty string. I wasn't sure if an empty string launch configuration would override a real valued parameter definition in the .yaml file.

[stevedanomodolor]

@stevedanomodolor so this works?

• With map file in the .yaml file -- uses that (if default in launch configuration is empty string)
• Without any map defined in the .yaml file -- uses default in launch configuration

I was thinking we might need an IfCondition to apply the SetParameter only if the value of it in the launch configuration was non-empty string. I wasn't sure if an empty string launch configuration would override a real valued parameter definition in the .yaml file.

Scenario 1 -> works Scenario 2 -> works -> current commit The yaml_filename when defined in the .yaml always overrides the launch file. Sorry for this taking longer than it should, I had some issue with the docker version

[SteveMacenski]

Hey, no worries. Its all a process :smile:

The yaml_filename when defined in the .yaml always overrides the launch file.

Oh really? So if we have yaml_filename: "" in .yaml, it'll override the map launch configuration that has some real-existing default value location? I would have thought that the launch configuration would override the yaml file value for any parameter when you call SetParameter().

I'm trying to game out what I would expect to happen in the major situations:

• No launch default + in yaml -> use yaml
• Launch default or set on commandline + in yaml -> use launch default or value on commandline
• No launch default + not in yaml -> completely unset
• Launch default or set on commandline + not in yaml -> use launch default or value on commandline

I think you're saying that the 2nd case is the opposite? I suppose that is fine if that's the way this acts, but that seems suspicious to me that a launch-set parameter would not take priority over a yaml set one.

[stevedanomodolor]

Launch default or set on commandline + in yaml -> use launch default or value on commandline launch default + yaml -> it pays attention to yaml. I am not if it does that because of the way configured_params loads the yaml after the setparameter has been called. This might be a better suggestion I think, I am not sure.

I was thinking we might need an IfCondition to apply the SetParameter only if the value of it in the launch configuration was non-empty string. I wasn't sure if an empty string launch configuration would override a real valued parameter definition in the .yaml file.

[SteveMacenski]

Launch default or set on commandline + in yaml ... launch default + yaml ...

Aren't these the same?

If both in yaml and launch configuration, I would expect it to use the launch override, is there a way to accomplish that? If not, maybe we can document the fact that yaml takes over and only set in the yaml file if you intend not use launch overrides / defaults.

[stevedanomodolor]

The main reason why I think the yaml file takes over is because it is just a redeclaration of the variable. Basically, calling the setparameter before the node is a requirement which initially declares the variable but the yaml file is added to the node causing the redeclaring I was referring too. Probably using an ifcondition to see the state of the variable and based on that redeclare with the yaml might be an alternative. i could try that

[SteveMacenski]

It would be nice to experiment to see if that's possible! But ultimately, if its not, I think we can live with it. We just need to make sure we clearly document it in a few places to ward off confusion. Ex. in the .yaml file above the commented out yaml_filename comment block, in the Nav2 documentation for that parameter, and as a comment in localization.yaml. Basically, mention it anywhere where someone would try to investigate.

---

@clalancette Is this expected behavior? When we use SetParameter() in launch and the same parameter exists in the params.yaml file, the value in params.yaml is used rather than the launch's SetParameter() value which was set via CLI. My intuition would say that launch-time overrides should take precedence, but that's not the behavior we experience. If that is expected behavior, please let us know (and I'll open a PR to document that somewhere).

[clalancette]

@clalancette Is this expected behavior? When we use SetParameter() in launch and the same parameter exists in the params.yaml file, the value in params.yaml is used rather than the launch's SetParameter() value which was set via CLI. My intuition would say that launch-time overrides should take precedence, but that's not the behavior we experience. If that is expected behavior, please let us know (and I'll open a PR to document that somewhere).

I'm not really an expert on what launch does. But here's what I found when playing around with the command-line.

For instance, if I have the following parameters in a file called cam2image.yaml:

/cam2image:
  ros__parameters:
    burger_mode: true
    depth: 10
    device_id: 0
    frame_id: bar
    frequency: 30.0
    height: 240
    history: keep_all
    qos_overrides:
      /parameter_events:
        publisher:
          depth: 1000
          durability: volatile
          history: keep_last
          reliability: reliable
    reliability: reliable
    show_camera: false
    use_sim_time: false
    width: 320

And I run:

ros2 run image_tools cam2image --ros-args --params-file cam2image.yaml

Then the frame_id is set to bar, like we expect.

If I then run:

ros2 run image_tools cam2image --ros-args --params-file cam2image.yaml -p frame_id:=foo

Then the frame_id is overridden to foo, also like we expect. So the command-line seems to be working properly.

To move forward here, I think it would be worthwhile to see what the command-line and parameters file that launch generates look like. That may give us a better idea of where the precedence is being set.

[SteveMacenski]

Related, but not quite the same thing as what we're handling. We have a launch system with a non-trivial number of nodes, so there's no -p <a_specific_parameter>:=val, this is via ros2 launch not ros2 run (setting for a single node, not the whole system). For launch, we set specific LaunchConfiguration's which have val set, and when we SetParameter() of those launch configurations, the yaml is taking priority. That seems like the opposite behavior than when running a single node, which is not good that we have inconsistent behavior between 2 analog processes

An exact analog would be if we set parameters=[configured_params, {'yaml_filename': val}], in the launch Node() function instead of using the SetParameter() launch function. That would create a conflict because the parameter exists in both the yaml and set individually, but if we should expect that this would have the same policy as ros2 run we'd expect that to use the individual parameter override, yes? @stevedanomodolor can you test this?

If that works, that solves our PR needs. But that still leaves a problem outstanding that the SetParameter() method of launch either (a) doesn't handle conflicts as it should (b) it does as it should, but needs to document that policy, or (c) needs to have the option made available to override. I would prefer A or C, but if that's the way folks want it and just needs documentation (B), I can try to see where that would be sensible and do so.

---

ros2 run image_tools cam2image --ros-args

Here nor there, but is the reverse true, if -p frame_id:=foo --params-file cam2image.yaml instead? Wondering if the reason that works is due to ordering of arguments or a specific policy of setting individual parameters over parameter file values.

[stevedanomodolor]

@SteveMacenski

Assuming we used the following yaml file

map_server:
  ros__parameters:
    yaml_filename: "bar"

I replaced all the launch variable name 'map' in the launch file to 'yaml_filename' to make it easier to understand The normal situation without commenting the map_server in the yaml file

ros2 launch nav2_bringup tb3_simulation_launch.py headless:=False 
ros2 param get /map_server yaml_filename

This gives String value is: bar For the second situation, I modified the yaml_filename parameters from the launch argument

ros2 launch nav2_bringup tb3_simulation_launch.py headless:=False yaml_filename:=test
ros2 param get /map_server yaml_filename

This gives String value is: bar The result makes sense because in the end we are just modifying the launch parameter, nothing to do with the node

Now, equivalent to the second example you mentioned @clalancette
parameters=[configured_params, {'yaml_filename': map_yaml_file}],

ros2 launch nav2_bringup tb3_simulation_launch.py headless:=False yaml_filename:=test
ros2 param get /map_server yaml_filename

This gives String value is: test This still means that the SetParameter declares but the fact that configured_params is added to the parameters of the map server node, the value in the yaml file overrides that declaration, and {'yaml_filename': map_yaml_file} overrides the yaml file. In the last case, the yaml file is always overridden and redundant and the yaml_filename must be set in the CLI or launch file. This is equivalent to removing the yaml from the loop entirely and just setting through CLI or launch file

[SteveMacenski]

Does that make sense @clalancette? SetParameter() in Launch only works if never otherwise set, which is very not intuitive and not consistent with the way that ros2 run (as you showed) or manually setting in a launch file's conflicting parameters=[] field handles things (as @stevedanomodolor showed).

Should I file a ticket about this?

---

@stevedanomodolor So for a path forward here, it sounds like we should:

• Add parameters=[configured_params, {'yaml_filename': map_yaml_file}], if map_yaml_file is non-empty string, rather than use SetParameter
• Its not super clean, but that should be possible if we have 2 instances of the Node() for map server, one with a condition=IfCondition(PythonExpression(['not ', map_yaml_file])), to not set the map, and one with condition=IfCondition(PythonExpression([map_yaml_file, ' != ""'])), to set the map (representing that the map is non-empty)

That way, we get:

• If not set on launch commandline or by a launch configuration default, we don't override a yaml default
• If we set it on launch commandline / launch configuration, we do override a yaml default
• If no yaml default exists and not set, throws
• If no yaml is set and anywhere it is set, it uses the new value

I think that gets us what we want, yes? I don't see a cleaner way to have a condition for including a parameter field to the parameter=[] list (unless empty-string for some rule doesn't override a yaml default automatically, but I don't suspect that's true). I'd just add in a 1-line comment about why this was necessary for future users.

[stevedanomodolor]

@SteveMacenski It seems that comparison with variables that are not either boolean or numbers does not work

condition=IfCondition(PythonExpression([map_yaml_file, ' != ""'])) condition=IfCondition(PythonExpression(['not ', map_yaml_file]))

It constantly gives the error [ERROR] [launch]: Caught exception in launch (see debug for traceback): invalid syntax (<string>, line 1)

It seems that pythonExpression does not work with string comparison only numbers and boolean

[SteveMacenski]

Huh, in googling looks like there's new ^{_{completely undocumented or described}} LaunchConfigurationEquals and LaunchConfigurationNotEquals that should do the trick!

https://answers.ros.org/question/360264/ifconditionpythonexpression-am-i-doing-it-right/

[mergify[bot]]

@stevedanomodolor, your PR has failed to build. Please check CI outputs and resolve issues. You may need to rebase or pull in main due to API changes (or your contribution genuinely fails).

[mergify[bot]]

@stevedanomodolor, your PR has failed to build. Please check CI outputs and resolve issues. You may need to rebase or pull in main due to API changes (or your contribution genuinely fails).

[mergify[bot]]

@stevedanomodolor, your PR has failed to build. Please check CI outputs and resolve issues. You may need to rebase or pull in main due to API changes (or your contribution genuinely fails).

[mergify[bot]]

@stevedanomodolor, your PR has failed to build. Please check CI outputs and resolve issues. You may need to rebase or pull in main due to API changes (or your contribution genuinely fails).

[SteveMacenski]

OK - overall this looks good now - I think there's just the action items in the open comments on the reviews. Mostly, just cleaning up what you have and adding some comments in the localization launch file about "why" we did this this way for future generations (e.g. LoadComposableNode for map server twice depending if we should use the value of map from a CLI or launch default or user defined value in the yaml configuration file. They are separated since the conditions currently only work on Load commands and not on the composition node function itself).

The cleanup includes linting (looks like you may have deleted some spaces by accident) and removing unnecessary objects / resetting settings back to previous defaults

[mergify[bot]]

@stevedanomodolor, your PR has failed to build. Please check CI outputs and resolve issues. You may need to rebase or pull in main due to API changes (or your contribution genuinely fails).