ros-navigation / navigation2

ROS 2 Navigation Framework and System
https://nav2.org/
Other
2.52k stars 1.28k forks source link

How to handle namespaced scan topics? #3772

Closed hilary-luo closed 1 year ago

hilary-luo commented 1 year ago

The Setup

ROS 2 Humble on Ubuntu 22.04 running nav2 for Clearpath Husky / Jackal robots.

Problem

Currently we are setting the scan topic in the nav2 config yaml to sensors/lidar2d_0/scan with the expectation that this topic will be resolved as the global topic /namespace/sensors/lidar2d_0/scan. Instead this is being resolved as /namespace/global_costmap/sensors/lidar2d_0/scan and /namespace/local_costmap/sensors/lidar2d_0/scan.

As a result, nav2 is not able to access the scan data.

The Question

We do not want to hardcode the namespace into the config, so what is the recommended method for remapping the scan topic? or should this be necessary?

Additional Details

The full nav2 yaml can be found here: https://github.com/clearpathrobotics/clearpath_nav2_demos/blob/5ecd0116296cbfce053455dc69505a050323d4b9/config/a200/nav2.yaml#L200 and corresponding launch file here: https://github.com/clearpathrobotics/clearpath_nav2_demos/blob/main/launch/nav2.launch.py

I did try adding SetRemap commands when launching nav2 unsuccessfully:

    nav2 = GroupAction([
        PushRosNamespace(namespace),
        SetRemap('global_costmap/sensors/lidar2d_0/scan', 'sensors/lidar2d_0/scan'),
        SetRemap('local_costmap/sensors/lidar2d_0/scan', 'sensors/lidar2d_0/scan'),

        IncludeLaunchDescription(
            PythonLaunchDescriptionSource(launch_nav2),
            launch_arguments=[
                ('use_sim_time', use_sim_time),
                ('params_file', file_parameters),
                ('use_composition', 'False'),
                ('namespace', namespace)
              ]
        ),
    ])
StetroF commented 1 year ago

I want to know this solution as well. Currently once there is a new robot need to work . Except change the namespace ,i have to change the loaded yaml's global_cost_map and local_cost_map's topic manually. I have a think that maybe we can use nav2_common.launch.RewrittenYaml function ,to rewrite the scan topic from /sensors/lidar2d_0/scan to /namespace/sensors/lidar2d_0/scan rather than change this topic in the config file.

NachtaktiverHalbaffe commented 1 year ago

Have a little bit digged into the topic. In the obstacle layer the topic is simply taken from the yaml configuration. When the topic is "relative" (no leading "/" sign), then it appends also the prefixes "local_costmap"/"global_costmap" additionally to the namespace itself. These seem to be a automatically added by the message filter and subscriptions the obstacle layer utilizes (Maybe these are (sub)namespaces in which the costmap runs, only guessing) where I personally would expect that only the namespace which I give the node when launching is added. The voxel layer inherits this behaviour. Maybe this is an side effect with the changes (in humble?) where the feature was introduced to use "relative" topics and automatically namespacing "relative" topics.

This also breaks the layers when using "relative" topics in the yaml-configuration when running without naemspaces, because in our environment we observed the appending of "local_costmap"/"global_costmap" when no namespace is used. So if you dont use "absolut" topics in the yaml config, then the obstacle layer will highly break.

The only prefix I think has a usecase for (automatically) appending is a namespace specified by the user/developer. I dont know a usecase where you want "local_costmap"/"global_costmap" appended to the observation source topic. This wanted behaviour would allow writing one yaml config with "relative" topics which then works with all namespaced environments without messing around with rewritten yaml in the launch files.

So a simple workaround would be to check if the obstacle layer runs in a namespace and if the given topic is relative. If yes then do the needed string interpolation manually in the onInitialize method. After that the topic is "absolut" and the subscription doesnt append prefixes by itself. It could like following (just an unopimized example, inserted anywhere near here https://github.com/ros-planning/navigation2/blob/main/nav2_costmap_2d/plugins/obstacle_layer.cpp#L140) :

node->get_parameter(name_ + "." + source + "." + "topic", topic);
if(topic[0] !="/" && node -> get_namespace() != "/"){  
   topic = node -> get_namespace() + topic;
}

After that, the configuration can look like this again:

      obstacle_layer:
        plugin: "nav2_costmap_2d::ObstacleLayer"
        enabled: True
        observation_sources: scan
        scan:
          topic: scan  # /scan if you didn't want a namespace appended
          max_obstacle_height: 2.0
          clearing: True
          marking: True
          data_type: "LaserScan"
          raytrace_max_range: 3.0
          raytrace_min_range: 0.0
          obstacle_max_range: 2.5
          obstacle_min_range: 0.0
SteveMacenski commented 1 year ago

Interestingly LG added something that might help recently, which allows things like https://github.com/ros-planning/navigation2/blob/main/nav2_bringup/params/nav2_multirobot_params_all.yaml#L213 to be parsed at launch time via https://github.com/ros-planning/navigation2/blob/6ef3d7bfd0f617bd0751cdbe7a58c3f9f66fe882/nav2_bringup/launch/bringup_launch.py#L61-L64.

Remapping would also generally work if you did it on a per-node basis for just the nodes taking in a scan in the costmaps.

Does that answer your question?

NachtaktiverHalbaffe commented 1 year ago

This seems a proper solution if someone tries to organize namespaced topics in a yaml configuration in that style. However, I have a more design oriented question/disussion about this problem. I try to stay short.

My (personal) expected behaviour regarding namespacing topics My observation is that you use "relative" topics (topics without a leading "/") if you want ROS2 to automatically add the namespace to the topic. When using a "absolut" topic (with leading"/"), then the topic is used as it is without appending anything. A common thing I see when using namespaces in ROS2 navigation (Mainly Navigation2 and Slam-toolbox) is using e.g. "tf" instead of "/tf" to namespace the transformation tree topic or do this via a remapping in the launchfile, so the topic would end up with "/< namespace >/tf" if you used the relatice topic).

I'm not deep enough into the ROS2 community to know if this handling is the common understanding, but my fellow students, my supervisor and his collagues have this understanding. Also IIRC The Construct Sim also teached this in one of their publicly available courses for mult-robot navigation.

The underlying design question My underlying design question is if my expected behaviour like I described should be supported when configuring Navigation2 (or to be more specific obstalce/voxel layer). I personally would say that both my understanding and the solution you provided should both be supported, because as far as I read when it comes to multi-robot-scenarios and using namespaces the two commonly used solutions is remapping the YAML-file like you mentioned (especially before humble) or using relative topics like I mentioned (since humble I think because there the support for this was introduced AFAIK).

If wanting to support "relative" topics Using this relative topics in yaml configuration files in Navigation2 works perfectly, except the obstacle layer (and voxel layer which inherits this behaviour) for the reasons like I described in my original comment. If you want to support the usage of relatice topics like I described, then some adjustments needs to be made in the obstacle layer like the draft I provided in my original comment. If my behaviour shouldnt be supported, when it should somewhere clearly clarified in the documentation that relative topics arent supported for obstacle/voxel, because if you use it it just breaks and its confusing if it works on other parts of navigation 2

Maybe my question/problem is clearer which I tried to describe in my original comment.

Sorry to ramble on like this. For me, this is not just a question about finding a solution/workaround to this problem, but a more fundamental one about how to properly configure namespaced topics when namespacing is used.

SteveMacenski commented 1 year ago

Adding the prefix / makes the topic absolute. Without it its relative to the namespace, which is what you see happening with the added global_costmap etc since that's the node's name. The global costmap you see there is the namespace, since the node is under global_costmap/global_costmap as you can see if you do ros2 node list.

NachtaktiverHalbaffe commented 1 year ago

Yeah, but do the people expect this behaviour? What I try to say is that I, my fellow students and supervisor expect(ed) when we set the topic of the observation sources to scan inside the configuration file, then the namespace defined by the user/developer gets appended. So if the user sets the whole nav2 stack under the user-defined namespace e.g. robot1 that the observation source topic would change from scan to /robot1/scan (or just /scan if we dont pass a namespace).

What we get then is instead is /robot1/local_costmap/scan (or /local_costmap/scan) where cleary no roboter is publishing Laserdata. The reason for that is now clear to us like you mentioned (were basically kinda applying two namespaces, the user-defined one and the one from the costmap itself) and as far as I see/experienced the costmaps are the only place in nav2 where the action server run under own namespaces. As a consequence the obstacle layer is the only place were we cant use relative topics inside the yaml configuration file as observation sources and we have to use absolute topics and/or remappings on launch file layer. My question/proposal is that we change the obstacle layer under the hood (basically remapping the topic name in the code instead of remapping in launch files) so the yaml-configuration behaves like I described/expected and relative topics inside yaml-configurations can be used as observation sources in the obstacle layer.

Then it would be the same behaviour like in other parts of the nav2 configuration. Then no custom remappings in (own) launch files would be needed when using user-defined namespaces and the launchfiles provided by nav2 can be used without configuration. Then the obstacle layer works no matter whether I use relative topics in the yaml configuration like I described (my usecase) or remappings via launch files like the solution you described which makes this all less prone no misconfiguration errors. Currently we have to use absolute topics and do remappings via launch files when running with user-defined namespaces

Maybe I have to mention that @hilary-luo and I don't work on the same project. I use the turtlebot 4 as a robot and opened the issue originally on his repo and then we forwarded onto here. Hence we have the same problem, but maybe different needs/goals regarding the solution because of different use cases

SteveMacenski commented 1 year ago

The namespace isn't just what you set though, its also the namespaces already applied to the nodes. I don't think its a good idea to hack out a namespace for one unique situation.

See https://github.com/ros-planning/navigation2/issues/3541 for more discussion about why the namespaces exist for the local/global costmaps. If we can find a way around that, I suppose we could just remove the namespace wrapping the costmaps as a real solution to this behavior.

Otherwise if a generic solution cannot be found, adding the <robot_namespace>/scan ReplaceString in the launch file seems like a good option.

NachtaktiverHalbaffe commented 1 year ago

Ah thanks for the link, have looked into #3541. Although I have one point to dig deeper into the discussion:

The namespace of the costmaps have the purpose of seperating the topics and services in order for the costmap and their API to work when being utilized in planner/controller server at same time. Here the namespace of the costmap has a responsibility like discussed in #3541 and is used inside the nav2-stack to seperate global and local costmap. Their purpose is to namespace the costmap topics etc. and nothing else. They provide reusability of the costmaps and also ensuring unique topics at same times. And they separate within the nav2-stack, but not on a system wide level. Out of the box I dont know a solution other than hardcoding these namespaces into the topics, so maybe its the best to continue using these "subnamespaces" like described in #3541

The topics for the observation sources however reference the topic on which a completly other node outside the nav2 stack publishes. It hasn't directly something todo with the inner workings of nav2, the obstacle layer only reads from it. Further it can be the same for global and local costmap, they dont need to be seperated inside the nav2 stack. As a consequence the topic from the obsaervation sources fall under the namespace of the publishing robot and not of the costmap/nav2 stack. So the namespace under which the observation sources fall is from my point of view completly decoupled from the namespace of the costmaps. The namespace of the observation nodes doesn't need the be assigned to these topic(s) and are basically wrongly assigned in this case because of technical reasons when using relative topics.

However, in multi-roboter-scenarios the user-defined namespaces are usually used to differentiate the individual robots and their running stacks to enable multi-roboter-support on the same ROS-Domain (at least from my point of view). They have a different concern and responsibility than the namespace of the costmap. So its the closest to tie the observation topic to the user-defined namespace responsibility-wise and thus setting it into the user-defined namespace and ignoring the costmap namespace. And finally like I described before in my comments I think the expected way of relative topics inside the yaml-configuration files (not to mix up with the inner workings of the nav2-stack) is that the user-defined namespaces are appended and not both the internal one and the userdefined one (which is how it basically works now).

To sum up treating the namespace of the observation topic different like I described is justified from my point of view because of these reasons. In my opinion this doesnt hack out a namespace, it basically just re-assigns the observation topic to the right, user-defined namespace instead to the automatically added one which is the internal costmap namespace with another responsibility. And the obstacle layer would be the only place which needed to be changes as far as I can see

However, if your offered solution is still the preffered one, then maye it would be good to write a hint/warning it in the configuration documentation for the topics for the ones who don't dig as deep as we do to prohibit future misconfiguration errors.

SteveMacenski commented 1 year ago

So the namespace under which the observation sources fall is from my point of view completly decoupled from the namespace of the costmaps

That's nice in theory, look at the code and tell me how to do that. In reality, those topics belong to a node, specifically the costmap node, which is under its own namespace. I'm happy to agree with your philosophical position, but if that's not paired with a technically generic solution, there's not much to be done :shrug:

NachtaktiverHalbaffe commented 1 year ago

Does this need a generic solution? The observation topic in the obstacle layer is the only place I found which needed to be fixed, so my draft should be feasible if placed in https://github.com/ros-planning/navigation2/blob/main/nav2_costmap_2d/plugins/obstacle_layer.cpp#L140 and looking something like:

node->get_parameter(name_ + "." + source + "." + "topic", topic); // topic is observation topic
if(topic[0] !="/"){  
   topic = node -> get_namespace() + topic; 
// some string interpolation has to be done to drop the second namespace i.e ignoring the second "/" comes e.g. /robot1/localcostmap is returned form get_namespace() so when everything after the second "/" (including) is dropped it is /robot1 which is the user-defines namespace

}
//after that that subscriptions subscribe to this topic which is now absolute and thus the namespace of the node isnt //appended anymor

Sure it's a draft and needs testing and polishing (and seeing if I ported correctly, my mind is currently in python mode), but It should achive what I am talking about. All the other parts work already like I mentioned, so I dont know why this kind of specific solution/change to the obstacle layer hurts. But maybe we are talking past each other

SteveMacenski commented 1 year ago

The observation topic in the obstacle layer is the only place I found which needed to be fixed

It would be every sensor topic of every costmap layer

NachtaktiverHalbaffe commented 1 year ago

Oh I did oversee some layers which I arent using. But then we can create a "re-assign-namespace function" in a utils package or in the layer class itself and have to only call a single line in each plugin when a topic parameter from sensors are read?

I get now that it is a manual thing the developer of new costmap plugins have to implement, its a non-automatic solution which only works if the devs know and implement it. The question underlying here is if the responsibility for detecting and handling this namespacing problems lies on the developer to call that reassigning funtion if needed or the user/integrator to detect and handle it with remapping in the launch files. Im obviously see the responsibility more in the developer and if the problem reoccurs on new plugins then its a one line fix. After that, both solutions would work and thus lesser errors and issues should occur (especially with users/integrators which blackbox the layers and arent aware of this problem)

At the end i get that it's a decision between bad luck and brimstone, so I can understand why the decision could fall to just prefer launch file remappings

hilary-luo commented 1 year ago

Interestingly LG added something that might help recently, which allows things like https://github.com/ros-planning/navigation2/blob/main/nav2_bringup/params/nav2_multirobot_params_all.yaml#L213 to be parsed at launch time via

https://github.com/ros-planning/navigation2/blob/6ef3d7bfd0f617bd0751cdbe7a58c3f9f66fe882/nav2_bringup/launch/bringup_launch.py#L61-L64

@SteveMacenski Any chance on this change being ported to / released on humble? It sounds exactly like what I am looking for both for the turtlebot4 as well as the clearpath robots.

In the meantime I will look at remapping the nodes. @NachtaktiverHalbaffe I will update the turtlebot4 repo and then let you know on your ticket when that is done.

SteveMacenski commented 1 year ago

But then we can create a "re-assign-namespace function" in a utils package

This is why I think this is starting to border a hack. Its subtly hiding a behavior that every other sub/pub/service has in those nodes other than the few that are hand selected to not use the namespace (and error prone for new interfaces being added). Then all new layers need to implement this very subtle util that they may or may not be aware of to get analog behavior to the default plugins. But, from your comment it sounds like you get it :-)

The launch substitution is the real, right way of handling this generally and get the behavior you're looking for

@hilary-luo if you open a PR to backport the ReplaceString nav2_common API, I'll merge that. Then adding

    # '<robot_namespace>' keyword shall be replaced by 'namespace' launch argument
    # User defined config file should contain '<robot_namespace>' keyword for the replacements.
    params_file = ReplaceString(
        source_file=params_file,
        replacements={'<robot_namespace>': ('/', namespace)},
        condition=IfCondition(use_namespace))

To your launch file will do that substitution (for namespaces or anything else you like)

SteveMacenski commented 1 year ago

Closing ticket with that answer. @hilary-luo I'll count on you for the Humble PR :smile: