Rewrite the scan topic costmap plugins for multi-robot(namespace) before launch navigation.

[hyunseok-yang]

---

Basic Info

Info	Please fill out this column
Ticket(s) this addresses	N/A
Primary OS tested on	Ubuntu
Robotic platform tested on	CLOiSim

---

Description of contribution in a few bullet points

This changes make it more convinient to manage nav2_params.yaml file for multi-robot environment and easy to launch the navigation.

According to latest guideline, each robot requires different nav2_param files since absoulte path of topic ifor scan is required in local_costmap/global_costmap node.

if we set the topic name without slash'/' like 'scan', rclpp node will add prefix automatically for example , local_costmap/local_costmap/scan. So we should specify absoulute path such as /scan or /namespace/scan

But, the code I suggested will rewrite the value for the key 'topic:'.
As far as I know, 'topic' key is only used for costmap plugins

I think this changes quite make sense. :)

Description of documentation updates required from your changes

For multi-robot launch, it should be launched with namespace leading prefix '/'.

ros2 launch nav2_bringup bringup_launch.py  map:=install/cloi_external_config/share/cloi_external_config/map/seocho_tower_B1F_0.yaml use_namespace:=True namespace:=/acloi00

This shall try to subscribe local_costmap/local_costmap/acloi00/scan.

ros2 launch nav2_bringup bringup_launch.py  map:=install/cloi_external_config/share/cloi_external_config/map/seocho_tower_B1F_0.yaml use_namespace:=True namespace:=acloi00

So the guideline for mutli-robot bringup should be noticed.

---

Future work that may be required in bullet points

First time when I faced this inconvenient environemnt, I'd like to describe the 'param_rewirtes' like this.

{'local_costmap.local_costmap.ros__parameters.*.*.topic': (namespace,'/scan')}

I tried to modify RewrittenYaml() class what I thougt and expected but gave up :) because I''m not familiar with this class.

For Maintainers:

• 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

[hyunseok-yang]

@SteveMacenski I'm not 100% sure if this kind of approach is aligned to what you thinking.
How does it look like?

[mergify]

@hyunseok-yang, 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]

@hyunseok-yang if you look at the current multirobot setup - it uses the parent-most launch file to enable the multi-robot behavior. I'm suggesting doing the same thing so it doesn't impact any of the single robot stuff and that both of the multirobot "modes" (using 1 config file vs N config files) look analogous to each other.

So I'm suggesting you create a new multirobot launch file from the current one and adjusting it to use 1 config file (with a new config file with that namespace substitution for the sensor data) and perform the substitution there. Then provide the already substituted configuration to the other nested launch files that they they (e.g navigation.launch.py) don't need to know anything about what happened above it

[hyunseok-yang]

@hyunseok-yang if you look at the current multirobot setup - it uses the parent-most launch file to enable the multi-robot behavior. I'm suggesting doing the same thing so it doesn't impact any of the single robot stuff and that both of the multirobot "modes" (using 1 config file vs N config files) look analogous to each other.

So I'm suggesting you create a new multirobot launch file from the current one and adjusting it to use 1 config file (with a new config file with that namespace substitution for the sensor data) and perform the substitution there. Then provide the already substituted configuration to the other nested launch files that they they (e.g navigation.launch.py) don't need to know anything about what happened above it

@SteveMacenski
hm, I thought last commit I pushed does not impact exist running single robot stuff.
Then, I can sugget to create a new 'bringup_multirobot_launch.py(for example)' launch script to bringup multi-robots with a new config file which has namespace substituion.
This new launch script will not impact any tiny of code to bringup launch script for single robot and does not require new argument.

I will let you check soon.

[hyunseok-yang]

@SteveMacenski
How about this? I guess this kind of approach is quite straighforward and simple.
The user who wants to run whichever single or multi robot can luanch it intuitively.

[hyunseok-yang]

I just pushed new commit.

@SteveMacenski
please review the new file name of 'bringup_multirobot_launch.py' if it is similar to way of your thinking.

[SteveMacenski]

I think we're going in circles here a bit - I don't think this should need so many rounds of editing. You're looking to add a method to do multirobot launching using 1 params file and I agree that's valuable. We have already a multirobot launch file that uses N config files that has a structure to it.

What I'm looking for is basically for you to add that exact same launch file but modified for the needs of 1 param file instead of N param files (e.g. your text substitution and reading in a single params file). If you want to change the way we handle multirobot for your 1-param-file multirobot launch method, you should change the N-param-file multirobot launch method to match so that they're exact analogs except for the necessary and exact changes between them to enable them. Consistency is really important.

But, I don't think you really should need to change much other than reading in the single param file + adding in the text substitution for the <robot_namespace> field in the params file you've already added and been approved. I think this should be pretty simple and I think we're not totally on the same page which is why we're going back and forth so much

Agree?

[hyunseok-yang]

I think we're going in circles here a bit - I don't think this should need so many rounds of editing. You're looking to add a method to do multirobot launching using 1 params file and I agree that's valuable. We have already a multirobot launch file that uses N config files that has a structure to it.

What I'm looking for is basically for you to add that exact same launch file but modified for the needs of 1 param file instead of N param files (e.g. your text substitution and reading in a single params file). If you want to change the way we handle multirobot for your 1-param-file multirobot launch method, you should change the N-param-file multirobot launch method to match so that they're exact analogs except for the necessary and exact changes between them to enable them. Consistency is really important.

But, I don't think you really should need to change much other than reading in the single param file + adding in the text substitution for the <robot_namespace> field in the params file you've already added and been approved. I think this should be pretty simple and I think we're not totally on the same page which is why we're going back and forth so much

Agree?

Right, it looks we're going there and here. It is too simple but I guess there was misunderstanding during our discussion.

In terms of the comment you mentioned that it is redundant above, my first thought was just let's separate two functionality 'applying namespace(replace substitution)' and 'iterating number of robots'.

The functionality of 'iterating number of robots' shall apply the number trailing the namespace(as a prefix).
ex) robot(namespace), the number of robot = 5
then, the each namespace of robot1, robot2, ..., robot5 will be launched by bringup launch script consecutively.
It is why I separated the launch script.

Let me summarize what we need to be on same page.
So, for that, only two files are needed.

• (new) nav2_param file for multirobot including substition -> nav2_namespaced_params.yaml (I think nav2_multirobot_parmas.yaml is more prefered to me)
• (new) bringup_multirobot_launch.py -> applying namespace and iterating bringup launch with different namespace as the nubmer of robots(?)
  • if number_of_robots is 1, then just apply 'namespace' as it is.
  • if number_of_robots is greater than 1, then apply namespace as a prefix, ex) robot(namespace)+index => robot1, ...

@SteveMacenski
Is it now clarified more and on the same page? ;-)
(Again, all I need was applying namespace and launching with same parameter file for multiple robots which is exactly same model)

[SteveMacenski]

In terms of the comment you mentioned that it is redundant above, my first thought was just let's separate two functionality 'applying namespace(replace substitution)' and 'iterating number of robots'.

Oh I don't disagree with you, but I think though that these things are usually (but not always) related. For the most part, there's not really much incentives to throw all of Nav2 into a namespace when there's only 1 robot running on a system/domain ID. I'm not saying it doesn't happen, but its a less common thing. And in that case, they could still just hardcode the namespace for the sensor topics and really not be a problem. Its only a scaling problem when we're talking about N robots so its constantly changing.

The more I think about it, the more I generally agree that adding that <ros2-namespace> field to the sensors makes sense with your text substitution work. But I think changing the default bringup for the single robot case should be done in a followup PR just so that we can have the 2 separate conversations separately. 2 smaller PRs often get merged faster than 1 larger PR. For that, we don't need to add new files, just modify the normal file we already have.

Is it now clarified more and on the same page? ;-)

Yes! On the same page 😄

[hyunseok-yang]

In terms of the comment you mentioned that it is redundant above, my first thought was just let's separate two functionality 'applying namespace(replace substitution)' and 'iterating number of robots'.

Oh I don't disagree with you, but I think though that these things are usually (but not always) related. For the most part, there's not really much incentives to throw all of Nav2 into a namespace when there's only 1 robot running on a system/domain ID. I'm not saying it doesn't happen, but its a less common thing. And in that case, they could still just hardcode the namespace for the sensor topics and really not be a problem. Its only a scaling problem when we're talking about N robots so its constantly changing.

The more I think about it, the more I generally agree that adding that <ros2-namespace> field to the sensors makes sense with your text substitution work. But I think changing the default bringup for the single robot case should be done in a followup PR just so that we can have the 2 separate conversations separately. 2 smaller PRs often get merged faster than 1 larger PR. For that, we don't need to add new files, just modify the normal file we already have.

Is it now clarified more and on the same page? ;-)

Yes! On the same page 😄

Like you mentioned, it is better to separate PRs. Then I'd like to modify a bringup launch file on this PR if you don't mind.

I would just add only below section just after rewrite the param yaml file.

   configured_params = RewrittenYaml(
        source_file=params_file,
        root_key=namespace,
        param_rewrites={},
        convert_types=True)

    # '<robot_namespace>' keyword shall be replaced by 'namespace' launch argument
    # in config file 'nav2_namespaced_params.yaml' as a default.
    # User defined config file should containe '<robot_namespace>' keyword
    # for the proper parameter wherever you want to get parameters thet namespace applied.
    namespaced_replaced_params_file = ReplaceString(
        source_file=params_file,
        replacements={'<robot_namespace>': (namespace)})

oh, and new params file will be added too. => nav2_multirobot_params.yaml which contains replaceables substitution.

This will not affect any side-effect for the user who want to bring up just 1 robot or new beginners.
Do you agree?
(If you say OK, will update soon as you response 😊)

We need to handle scaling N of robots on other PR.
Since I've already faced with the error message during launch map_server for second robot. I tried to debug it but I put if off from my tasks due to higher priority of work to do.

[SteveMacenski]

Do you agree?

... close enough 🙃

That change for the normal bringup is what I wanted in another PR since this PR's description + title is about multirobot. So this PR should have the multi-robot changes too - which you have the yaml file, but not the other launch file anymore.

So I'm OK with you putting the change in bringup_launch.py, but this needs to have the complete multirobot stuff too.

How about the for the multirobot file naming we do multirobot_params_all.yaml in contrast to the multirobot_params_{N}.yaml to make clear all robots use it.

[hyunseok-yang]

... close enough 🙃
👍

That change for the normal bringup is what I wanted in another PR since this PR's description + title is about multirobot. So this PR should have the multi-robot changes too - which you have the yaml file, but not the other launch file anymore.
So I'm OK with you putting the change in bringup_launch.py, but this needs to have the complete multirobot stuff too.

Now, we don't need to touch the bringup_launch.py anymore, right~?
And then, you mean I need to create another NEW launch file for multi-robot bringup including the new parameter like 'number_of_robots'. It's correct?

How about the for the multirobot file naming we do multirobot_params_all.yaml in contrast to the multirobot_params_{N}.yaml to make clear all robots use it.

Sure, that's good idea.
(just modified it first)

[SteveMacenski]

Now, we don't need to touch the bringup_launch.py anymore, right~?

Correct. We can call everything in this PR right now as good.

And then, you mean I need to create another NEW launch file for multi-robot bringup including the new parameter like 'number_of_robots'. It's correct?

Correct, because now you have a param file with that <ros2-namespace> which is not currently being used anywhere. That new launch file can be broadly copy-pasted from the existing one, except with the number_of_robots to define the looping and reading in only the single parameter file.

Sorry we're missing each other so much on this PR, next time will be easier, I promise 😄

[hyunseok-yang]

Now, we don't need to touch the bringup_launch.py anymore, right~?

Correct. We can call everything in this PR right now as good.

Ok. that's good to hear 😄

And then, you mean I need to create another NEW launch file for multi-robot bringup including the new parameter like 'number_of_robots'. It's correct?

Correct, because now you have a param file with that <ros2-namespace> which is not currently being used anywhere. That new launch file can be broadly copy-pasted from the existing one, except with the number_of_robots to define the looping and reading in only the single parameter file.

With latest commit, as you can see, the current change in 'bringup_launch.py' shall replace that keyword. So if the new parameter which have <robot-namespace> substitution is passed as an arguement, it will be used. Isn't it? (Just for double-check)

Anyway, this one also will be updated soon. But it needs to take some trick how to looping the number of robots.
(Better hint would be helpful)

Sorry we're missing each other so much on this PR, next time will be easier, I promise 😄

No problem not at all. These kind of discussions are always required to head to a right way! Pleasure to have a talk✌️

[SteveMacenski]

With latest commit, as you can see, the current change in 'bringup_launch.py' shall replace that keyword. So if the new parameter which have substitution is passed as an arguement, it will be used. Isn't it? (Just for double-check)

I don't quite follow you. But what you change does is if <robot_namespace> is present, it replaces it with the value of namespace, regardless if its specified or not on the commandline. But, by default its empty string, so if <robot_namespace> is specified but namespace is not, it'll just insert /<empty string>/scan which is //scan I don't believe that's a problem, but is worth a quick test since that would have double / now that you ask

if ReplaceString has a condition field, you could just only apply it with namespace is not '' and then that solves that trivially

But it needs to take some trick how to looping the number of robots. (Better hint would be helpful)

If you have N, you can loop over N as I believe you've shown before. I think you can use the text substitution with robot_ and current N to set the namespace.

[hyunseok-yang]

With latest commit, as you can see, the current change in 'bringup_launch.py' shall replace that keyword. So if the new parameter which have substitution is passed as an arguement, it will be used. Isn't it? (Just for double-check)

I don't quite follow you. But what you change does is if <robot_namespace> is present, it replaces it with the value of namespace, regardless if its specified or not on the commandline. But, by default its empty string, so if <robot_namespace> is specified but namespace is not, it'll just insert /<empty string>/scan which is //scan I don't believe that's a problem, but is worth a quick test since that would have double / now that you ask

Oh, I just talked and asked like above because you said "now you have a param file with that which is not currently being used anywhere". BUT, I guess that was a lack of my explanation or mis-understanding. (Sorry for that 🥲)

What you desribed is expected scenario(Yes, your're right).
And, I've thought double / would be a problem, for example //scan. However, it'd be OK if it is not a problem.

Then, I will leave the 'bringup_launch.py' as it is on my last commit(6947321)

if ReplaceString has a condition field, you could just only apply it with namespace is not '' and then that solves that trivially

It would be great if ReplaceString has a condition field.
I will use like this, just imagine it.

 configured_params = ReplaceString(
        condition=IfCondition(PythonExpression(['not ', namespace])),
        source_file=configured_params,
        replacements={'<robot_namespace>': ('/', namespace)})

Or, just give a guidance as a comment do not use the param file which have substitutions like nav2_params_multirobot_all.yaml without namespace(empty namespace).

But it needs to take some trick how to looping the number of robots. (Better hint would be helpful)

If you have N, you can loop over N as I believe you've shown before. I think you can use the text substitution with robot_ and current N to set the namespace.

Next job I have to do is iterating N(number of robots) robots.
The problem is that there is no way to retrieve an value from the launch argument before generate the launch description unless it uses
defined the variables.
.

That's why I've utilized a OpaqueFunction. If it is not appropriate, I think we should use a SetEnvironmentVariable.
For example,

export NAV2_NUM_OF_ROBOTS=3
ros2 launch nav2_bringup bringup_multirobot_launch.py

[hyunseok-yang]

@SteveMacenski

Just tried if //topic_name works well as a test.
I've got this error with ros2-cli. looks it's impossible to use double /.

rclpy.exceptions.InvalidTopicNameException: Invalid topic name: topic name must not contain repeated '/': '//base_controller/cmd_vel_unstamped'
    ^
yg@YG-ROS:navigation2$ ros2 topic pub -r 1 //base_controller/cmd_vel_unstamped geometry_msgs/msg/Twist {}

And, accordint to these document. It's not allows 😩
https://design.ros2.org/articles/topic_and_service_names.html

must not be empty, e.g. the name //bar is not allowed
rationale: it removes the chance for accidental // from concatenation and therefore the need to collapse // to /

[SteveMacenski]

The problem is that there is no way to retrieve an value from the launch argument before generate the launch description unless it uses

Can't you use the same PythonExpression to make a loop over the launch configuration? There have to be examples of things like that I'd think ... But if not, then i can understand why it is you did the function thing before - lets just make a note of that inline so that people can understand why for the future.

I've got this error with ros2-cli. looks it's impossible to use double /.

condition=IfCondition(PythonExpression(['not ', namespace])), it is then 😄

[hyunseok-yang]

The problem is that there is no way to retrieve an value from the launch argument before generate the launch description unless it uses

Can't you use the same PythonExpression to make a loop over the launch configuration? There have to be examples of things like that I'd think ... But if not, then i can understand why it is you did the function thing before - lets just make a note of that inline so that people can understand why for the future.

I'd really like to use the PythonExpression to make an iteration. But it was impossible to parse it at that time of process.
Let me think about it if there is other option.

If not, it would be better to leave a note like you mentioned.

I've got this error with ros2-cli. looks it's impossible to use double /.

condition=IfCondition(PythonExpression(['not ', namespace])), it is then 😄

Then I guss I need to modify ReplaceString method also.

[SteveMacenski]

No this: https://github.com/ros-planning/navigation.ros.org/blob/master/migration/Iron.rst

[hyunseok-yang]

No this: https://github.com/ros-planning/navigation.ros.org/blob/master/migration/Iron.rst

Please review this PR. :)
ros-navigation/docs.nav2.org#455

[SteveMacenski]

Just merge those grammatical fixes into your branch + the linting failures and we can merge this! I'd merge those grammatical changes in myself, but your branch appears to be protected

[hyunseok-yang]

Just merge those grammatical fixes into your branch + the linting failures and we can merge this! I'd merge those grammatical changes in myself, but your branch appears to be protected

I added your suggestion into commit now.

[SteveMacenski]

Still have the linting errors: https://app.circleci.com/pipelines/github/ros-planning/navigation2/9881/workflows/2fb30bf2-57ed-47df-92cd-dfa0d232ae79/jobs/31563/tests

That's the only thing blocking!

[hyunseok-yang]

Still have the linting errors: https://app.circleci.com/pipelines/github/ros-planning/navigation2/9881/workflows/2fb30bf2-57ed-47df-92cd-dfa0d232ae79/jobs/31563/tests

That's the only thing blocking!

hm, :(
I will check ament_ linting again.

[hyunseok-yang]

Still have the linting errors: https://app.circleci.com/pipelines/github/ros-planning/navigation2/9881/workflows/2fb30bf2-57ed-47df-92cd-dfa0d232ae79/jobs/31563/tests
That's the only thing blocking!

hm, :( I will check ament_ linting again.

Oh, I only checked ament_flake8

$ ament_flake8 nav2_bringup/launch/cloned_multi_tb3_simulation_launch.py 

1 files checked
No problems found

Checked files:

* nav2_bringup/launch/cloned_multi_tb3_simulation_launch.py

Here's the result for ament_pep257 after fixed.

yg@YG-ROS:navigation2$ ament_pep257 nav2_bringup/launch/cloned_multi_tb3_simulation_launch.py 
checking nav2_bringup/launch/cloned_multi_tb3_simulation_launch.py
No problems found

[SteveMacenski]

Great!! Happy to have this in finally 🙂