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

[Proposal] Simplify namespaced bringups and multirobot sim #4715

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

luca-della-vedova
Copy link

Basic Info

Info Please fill out this column
Ticket(s) this addresses N/A, see ros-navigation/nav2_minimal_turtlebot_simulation#15 for background
Primary OS tested on Ubuntu 24.04 Jazzy
Robotic platform tested on TB3 (single / multiple) and TB4 sim on Gazebo Harmonic
Does this PR contain AI generated software? No

Description of contribution in a few bullet points

Reworked namespacing, no more having an rviz config with absolute topics and one for namespaced topics that relies on string substitution, but just a single one that will use the namespace passed to rviz to be resolved, for example:

## BEFORE:
# rviz_default_view.rviz, subscribing to laser scan
topic: /scan
# rviz_namespaced_view.rviz, subscribing to laser scan
topic: <robot_namespace>/scan
## AFTER:
# rviz_default_view.rviz
topic: scan
# When running, just set the rviz namespace, when none is specified it will automatically default to /scan

Note that topic names were also hardcoded in the nav2 params, to my understanding most of the duplication in the config files for the multirobot sim was really just to make sure different robots are subscribed to different topics, however now with proper namespace support all the robots can just reuse the same config file without needing to change topics, do string substitution or anything like that.

Description of documentation updates required from your changes

Not sure, the previous behavior should still work, but now bringing up multiple robots should require significantly less work / config file duplication.


Future work that may be required in bullet points

There is just one rough edge (and the only code change in this PR) that I had to hack to make this work, which I'm not terribly happy with (but not too sure there is any better way), here.

The costmap topics are in the namespace <chosen_namespace>/[local, global]_costmap, which makes it tricky for users to pass a local topic to subscribe to such as scan, since that would be parsed as <chosen_namespace>/local_costmap/scan which is clearly not what most cases would do.
I added that "hack" to manually go up one namespace and make sure that the topic name is relative to the chosen namespace, not the costmap local one. It works well but I can't say I'm too happy with it (I was really hoping there would be a .. operator for topics so I could specify it as ../scan but alas that seems to not be the case).

For Maintainers:

  • Check that any new parameters added are updated in docs.nav2.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

nav2_bringup/rviz/nav2_default_view.rviz Show resolved Hide resolved
nav2_bringup/params/nav2_params.yaml Show resolved Hide resolved
@@ -189,6 +189,17 @@ void ObstacleLayer::onInitialize()
node->get_parameter(name_ + "." + source + "." + "raytrace_min_range", raytrace_min_range);
node->get_parameter(name_ + "." + source + "." + "raytrace_max_range", raytrace_max_range);

// If the user passed a relative topic, use it as relative to this node's parent namespace.
Copy link
Member

@SteveMacenski SteveMacenski Oct 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems unintuitive (and require updating in all layers) - but I also recognize the current behavior is also not overly intuitive to users if they don't specify the full path.

What if the namespace of the topic is different than the namespace of the costmap server? I.e. lidar1 vs robot1 because you have N lidars of the same make and N robots you're simulating. the namespace of this node is usually not related to the namespace of the sensor drivers which are launched independently.

So, I'd argue either keeping the current behavior or if not /, we throw out the namespace entirely and treat it like it was / (but then has the multirobot details to consider). Its hard to know which part of the namespace is the costmap server's namespace vs the robot's namespace. The costmap servers can also be launched independently of the planner/controller server where they can be totally randomly named based on the user's applications. Its hard (or impossible) to know which to strip out and which to keep, I think?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if the namespace of the topic is different than the namespace of the costmap server? I.e. lidar1 vs robot1 because you have N lidars of the same make and N robots you're simulating. the namespace of this node is usually not related to the namespace of the sensor drivers which are launched independently.

I understand, again there is no perfect solution that I could find. Absolute paths will still work so I would say that if you have your costmap server in a robot1 namespace but your lidars in lidar1 namespace you should just set the topic as /lidar1/scan.
The current behavior is not really any better, since if you specify lidar1/scan you would end up subscribing to /robot1/[local, global]_costmap/lidar1/scan.
If you set the namespace of the costmap server to robot1 and you ask it to listen to the scan topic (as a relative path), it sounds more intuitive to me that it would listen to /robot1/scan, rather than the current behavior of silently adding a [local, global]_costmap namespace inbetween.

Still, this is just a proposal, in the current incantation it can get rid of all the special configurations for multirobot / namespaced robots which is imho the ideal scenario, but of course it introduces tradeoffs such as this. Any other compromise might still move in the right direction, it just won't be 100% there but it's still something, for example:

  • Have at least a shared rviz file and don't rely on string substitution
  • Change some of the topics such as /odom to be relative so users don't need to change too many fields in multirobot params

Happy to remove parts of this if you think they are too controversial

Copy link
Member

@SteveMacenski SteveMacenski Nov 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay -- ROSCon / ROS-I and my European travel thereafter was in the way. I'm back at home now.

I understand, again there is no perfect solution that I could find. Absolute paths will still work so I would say that if you have your costmap server in a robot1 namespace but your lidars in lidar1 namespace you should just set the topic as /lidar1/scan.

We agree on this - I generally think it wise to specify the full topic for an application.

Something I'm also considering and maybe this is not such a big deal is that: alot of people use the Nav2 config files as the copy+paste basis for their robot's configurations. So right now we have the absolute path that folks would replace with their absolute path. In exchange for Nav2 software simplicity, I think it pushes some subtle complexity onto the user to realize that they shouldn't put scan, even though we do here, they should put /scan. A few things in ROS are done this way that create some user friction that I generally try to avoid, but also like you say, /robot1/[local, global]_costmap/lidar1/scan is really not much better than /robot1/scan other than the fact that it says the costmaps to know where its coming from more immediately to fix.

Perhaps just adding a comment about this in the yaml file entries (+config docs on docs.nav2.org) for the topics would be sufficient to get past this so that its not subtle but immediately visible to the end-user. And a recommendation to use full topic paths if you don't want to worry about it.

What do you think about that solution? Is that least or more straight forward for a user? If so, I'm OK with this as long as we update all the default plugins (+ reminder for me to also update STVL/NPVL) and add it to our migration guide with those examples you have inline in this block to explain to people updates they need to make.

Copy link

codecov bot commented Oct 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Files with missing lines Coverage Δ
nav2_costmap_2d/plugins/obstacle_layer.cpp 82.41% <100.00%> (+1.88%) ⬆️

... and 13 files with indirect coverage changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants