ros-perception / laser_filters

Assorted filters designed to operate on 2D planar laser scanners, which use the sensor_msgs/LaserScan type.
BSD 3-Clause "New" or "Revised" License
172 stars 204 forks source link

Allow input/output topics to be read from params #166

Closed mrcabo closed 2 years ago

mrcabo commented 2 years ago

Now instead of topic names being hard coded, they can be specified in a config file that gets loaded in the parameter server.

jonbinney commented 2 years ago

Is there a reason topic remapping is insufficient for this use case? http://wiki.ros.org/roslaunch/XML/remap

mrcabo commented 2 years ago

Thanks for getting back to me so quickly, I appreciate it.

We are using containerization for deploying our stack. There are a few of reasons why remapping was insufficient for us. Since remapping is defined in the launchfile (and I don't think there is a way to define that in a config file), any change in namespace/topic would require rebuilding the image.

jonbinney commented 2 years ago

Your use case makes sense - I can see why this would simplify things for this particular case. But I'm not convinced it's worth re-building topic remapping within the code for this node (and presumably you'd need to do this for every ROS node you may need to remap). The main downside of this change is that it makes it harder to understand an existing set of launchfiles - to figure out which topics connect to which you'd need to look for remappings in the launchfile, and then also look through all of the parameter files to see if any of them modify the topics.

Option 1

Keep one top level launchfile outside of the container. In that launchfile can do remappings globally: <remap from="/some_namespace/scan_in" to="/something else"/> Then use <include ...> to include the main launchfile that lives inside the container. If you really want to have exactly one file outside the container, you could also set the params in the top level launchfile, although it is usually easier to have that launchfile load a param file that also lives outside the container.

Option 2

Use environment variables to set the remappings. Keep a shell script outside the container that sets an environment variable to the desired topic, then source that shell script as part of the command executed by the launchfile. Then have the launchfile set the remapping based on the environment variable.

You can't set remappings from ROS params (since subsitutions in launchfiles are evaluated before anything is launched, whereas the params can't be read until after launch). But you can set remappings from environment variables: <remap from="scan_in" to="$(env LASER_FILTER_SCAN_IN_TOPIC)"/>

rpangrazio commented 2 years ago

I have a question. Why can't we do both? If you set a parameter it will change the topic. If you don't set the parameter it defaults to "scan" and the remapping can take place in either case. Am I missing something?

jonbinney commented 2 years ago

I understand where you're coming from (I've run into this exact issue when containerizing ROS systems for robots). Ultimately I've usually gone with the environment variable approach. Even though the parameter could be ignored, it adds potential confusion for users. There ought to be one right way to do things, and for changing topics in ROS systems, that approach is remapping, not parameters.

I put a lot of weight on simplicity and consistency in this code since it is used by so many people in so many different use cases. I'm going to go ahead and close this - sorry for the inconvenience!