Map fails to load if params-file is given but without map

[superseppl]

Bug report

Required Info:

• Operating System:
• Ubuntu 20.04
• ROS2 Version:
  • Foxy binaries
• Version or commit hash:
  • ros-foxy-navigation2 0.4.7-1focal.20220307.024942
• DDS implementation:
  • rmw_cyclonedds.

Steps to reproduce issue

Hi guys,
while loading a map it occurs that whenever I use

ros2 launch nav2_bringup bringup_launch.py map:=/path/to/map.yaml

the map server works. But as soon as I add a param file without any map_server param inside the file but with the map param

ros2 launch nav2_bringup bringup_launch.py params_file:=/path/to/params.yaml map:=/path/to/map.yaml 

it crashes. As soon as I add

 map_server:
   ros__parameters:
     use_sim_time: False
     yaml_filename: "xyz.yaml"

to the yaml file and start it with

ros2 launch nav2_bringup bringup_launch.py params_file:=/path/to/params.yaml map:=/path/to/map.yaml 

it works.

Issues:

• I cannot launch the file even tough I defined a map in the yaml file as the map parameter is compulsory.
• The yaml file parameter is not used and can be different to the map parameter used with CLI.

Feature request

Feature description

It would be nice to use only one parameter when starting. As the map parameter is always expected when launching the node it should only start with this one and should not require anything given in the param-file.

[padhupradheep]

Strangely I cannot reproduce the issue in the main branch.

Are you sure that, you are providing the absolute path to the map argument during the launch?

[SteveMacenski]

Moreover, while Foxy is still an active distribution, we no longer support it in this project. We were still in heavy development at that time and the sheer volume of changes between foxy and even Galactic are so large it became virtually impossible to provide support for Foxy backwards.

My personal recommendation is to update to any more recent distribution. There are a litany of improvements made in stability/reliability and features (including your issue too, evidently!).

But I would be happy to field any PRs to Foxy to resolve the issue if you find the root cause, but it unfortunately won’t be prioritized by maintainers.

[superseppl]

Alright, thanks for the time checking it. That was the output on the terminal when starting the nav2_bringup and commenting the yaml file param:

[map_server-1] [INFO] [1660655998.977703636] [map_server]: Configuring
[map_server-1] [ERROR] [1660655998.977822372] []: Caught exception in callback for transition 10
[map_server-1] [ERROR] [1660655998.977832220] []: Original error: yaml_filename
[map_server-1] [WARN] [1660655998.977851287] []: Error occurred while doing error handling.
[map_server-1] [FATAL] [1660655998.977863929] [map_server]: Lifecycle node map_server does not have error state implemented

[SteveMacenski]

Ah, I think I see.

There is a non-trivial default in the tb3_simulation_launch.py file for the map which would be remapped over any value in the yaml_filename in the configuration files using RewrittenYaml. But in order for rewritten yaml to work, the key has to exist in the yaml file in the first place, so requiring yaml_filename to be in the yaml but with a bogus and entirely ignored value.

The issue arises that folks wanted to be able to remap the map on the commandline, but if both a launch configuration for yaml_filename and a value for it exists in the yaml file, there's a collision and launch doesn't know which to use (I'm not sure which it would resolve to, or if it would straight up throw an exception if you tried). So we have to rewrite it. However, without providing more information in the launch file, if we wanted to insert yaml_filename into the yaml file, we wouldn't know where to do it if it wasn't already there to overwrite. But inserting could work if it doesn't exist, but would involve some new code.

So it leaves us in a bit of an odd situation, where it has to be in the yaml file to work, but will always be overridden if the map launch configuration contains a default value or specified on the commandline. If you remove that default value though, it should work to use the yaml file's map contents.

Its quite an odd situation that I'm not sure I have a good answer to. I'd be open to suggested changes. Maybe the answer is to insert the absolute path of map_server.ros__parameters.yaml_filename into the yaml on launch so that it works without it being specified, but that is pretty hacky and then would only allow this behavior to work within the nav2 provided launch files. The only other 2 options I see are to (1) remove the commandline support for changing maps or (2) require users to always specify a yaml_filename, even if just an empty string, in the yaml file.

In main, we recently added the ability for the map server to be launched without a map specified (so that later a map can be loaded via the service) so yaml_filename in yaml can just be an empty string by default to indicate to wait for a load request (but its still required in the yaml) that will still be overridden in launch when a default or a CLI launch configuration is provided.

[SteveMacenski]

https://github.com/ros-planning/navigation2/pull/3123/files I changed to the empty string and also added a comment to that effect. What do you think?

[padhupradheep]

Just leaving my opinion. As far as I saw, if you have both the yaml_filename specified in the yaml and the map argument set either as a default arg in launch file / specified in the CLI during the launch, the later is considered by the map server. There is actually no effect, even if you have the yaml_filename specified in your yaml.

The only other 2 options I see are to (1) remove the commandline support for changing maps or (2) require users to always specify a yaml_filename, even if just an empty string, in the yaml file.

Considering the point that I made, I would vote for the second option, as it would definitely remove the confusion regarding the conflicts. If the user does not want to set the path either through the command_line / specify it in the default launch, then they can overwrite the empty string with the absolute path that they want to use. Also, I know a lot of people who use CLI to change the maps or select the maps during launch (me included 😅 )

[SteveMacenski]

Documentation doesn't solve everything, but this is an odd quirk that should definitely be documented if its going to exist: ros-navigation/docs.nav2.org#338 + in the default navigation yaml file there's a note to the same effect.

I think that's the best solution we're going to come up with here unless/until the ROS 2 parameter API changes letting us specify multiple parameter sets with priority levels (which I don't think will ever happen, nor even being discussed to my knowledge).

@superseppl I'm going to close this now since we've come up with the "why" and there isn't really a solution. However, you bring up a very good point that this is very subtle and not well documented, so I went ahead and documented it both in the yaml files and in the official documentation for the parameter so that anyone reading docs or starting from our yaml files will have this clearly denoted to them.

I really appreciate the discussion this generated.

tl;dr for later readers

• If you want to specify in yaml, remove the default from the map launch configuration
• If you use the default / CLI set the map on launch, any value in the yaml is bogus and going to be overridden, but needs to exist in the file.