Parameterize AMCL and GMapping launch files for individual cameras.

[kevincwells]

Without this change, it is not possible to load custom GMapping or AMCL parameters based on what 3D sensor is being used.

To address this, amcl_demo.launch and gmapping_demo.launch were changed to use the TURTLEBOT_3D_SENSOR environment variable to choose which version of the XXXX_amcl.launch.xml, XXXX_costmap_param.yaml, and XXXX_gmapping.launch.xml files it includes. These have also been parameterized to allow you to pass an argument to override these values.

For cameras that do not have custom param files, their corresponding launch files are simply symlinks to the default files. To simplify this, CMakeLists.txt has been modified to auto-generate these symlinks to the default param files at build time.

[rohbotics]

I tested this out and have a couple comments/questions.

The modified gmapping parameters are much better, but the noise of the realsense generates a bunch of false obstacles that end up getting saved into the map. The noise is a much bigger problem for the local planner which often has to make wild curvy paths to get around a bunch of single point false obstacles.

Is there a way to tune the camera to generate better pointclouds for navigation? Otherwise we may have to add a nearest neighbor filter, or something similar, to remove the single point obstaces.

Also is there a way to integrate the realsense's visual odometery?

[kevincwells]

Hi Rohan,

To clarify, are you saying that you didn't see these noisy false obstacles before using the modified gmapping parameters? We are aware of some false obstacle issues, and are exploring a number of solutions right now.

[rohbotics]

There are noisy obstacles even with this patch, it looks like they are just inflated less in the global map, so they have less of an effect.

In the local costmap, the noisy obstacles are still problematic for navigation, as the robot can get stuck because the costmap gets filled with noisy obstacles.

[ghindman]

@tfoote turtlebot integration PR was updated w/ the necessary changes https://github.com/turtlebot/turtlebot/pull/229 is there anything else blocking merge of these changes?

[tfoote]

This generates a lot of files in the source. The build should not be writing to the source space. Any generated files should be generated into build or devel space and installed from there.

Untracked files:
  (use "git add <file>..." to include in what will be committed)

    turtlebot_navigation/launch/includes/amcl/astra_amcl.launch.xml
    turtlebot_navigation/launch/includes/amcl/asus_xtion_pro_amcl.launch.xml
    turtlebot_navigation/launch/includes/amcl/asus_xtion_pro_offset_amcl.launch.xml
    turtlebot_navigation/launch/includes/amcl/kinect_amcl.launch.xml
    turtlebot_navigation/launch/includes/gmapping/astra_gmapping.launch.xml
    turtlebot_navigation/launch/includes/gmapping/asus_xtion_pro_gmapping.launch.xml
    turtlebot_navigation/launch/includes/gmapping/asus_xtion_pro_offset_gmapping.launch.xml
    turtlebot_navigation/launch/includes/gmapping/kinect_gmapping.launch.xml
    turtlebot_navigation/param/astra_costmap_params.yaml
    turtlebot_navigation/param/asus_xtion_pro_costmap_params.yaml
    turtlebot_navigation/param/asus_xtion_pro_offset_costmap_params.yaml
    turtlebot_navigation/param/kinect_costmap_params.yaml

[kevincwells]

The approach taken in this PR relies on using string-substitution of the TURTLEBOT_3D_SENSOR environment variable into the file name path. This requires that such a file actually exist , or it will error out. For cameras that do not (yet/ever) want custom parameters, a file still needs to exist with its name in the file name. We've taken the approach of making these files simply symlinks to the default file.

The changes in CMakeLists.txt were to automate the creation of these symlink files based on the current list of cameras found in the turtlebot_bringup package. We realize it is unconventional to generate files in the source path at build time, but this seemed like the easiest way to automate the process. Would you prefer the files just be generated by hand and checked in for the current list of known cameras?

It's also probably possible in Kinetic and above to use the $(eval) syntax to check for the existence of a file and, if not, return the default file. However, we were aiming for backward compatibility to the Indigo branch as well.

[tfoote]

For finding the supported cameras I think this sort of approach would be find. This saves the need to grep for files and regex and it even allows a different set of sensors for navigation than bringup.

set(SUPPORTED_SENSORS "astra" "asus_xtion_pro" "kinect" "r200")
foreach(camera_name ${SUPPORTED_SENSORS})
    generateCameraParamFiles(${camera_name})    
endforeach()

For the generated files we just need to generate them into the devel space instead of the source space and add an install rule to get them into the install space.

[tfoote]

The generated files might go best into a place like: set(OUTPUT_DEVEL_DIRECTORY "${CATKIN_DEVEL_PREFIX}/${CATKIN_PACKAGE_SHARE_DESTINATION}/launch/includes/amcl") along these lines: http://answers.ros.org/question/182465/copying-files-in-devel-folder-cmakeliststxt/

[kevincwells]

Re: SUPPORTED_SENSORS variable -- that's a really easy change, but are you sure that's how you'd want it implemented? The changes in CMakeLists.txt were to avoid needing to modify turtlebot_navigation for a new camera unless you want custom parameters. If rosbuild is deprecated, is there a modern way of getting the path to a ROS package?

Though I do agree that grepping and regex always feels a little fragile. With the way Turtlebot 2 uses the TURTLEBOT_3D_SENSOR to select cameras, ideally there'd be one arbiter of what cameras are valid and only one place in the system you'd have to look to find/update that list. Perhaps a YAML list (or something of the sort) that could be read in by whatever package needs it?

[tfoote]

The supported sensors solution is very simple which is what I like. Adding a new sensor to the list is low overhead compared to actually testing it. And expecting any new sensor to work without any testing is a low likelyhood.

The grepping through the source of another package is not valid as you cannot assume that the source of the other package will be available at build time. (It will not be when the debians are being produced.) If it's a resource you need it needs to be declared and installed, and likely exported via a library.

It might make sense to think about setting up a turtlebot configuration package which manages resources and exposes some CMake logic to help other packages find declare resources as well as find other resources.

[stonier]

Question: is there really enough variation in the 3d sensors so that a highly tuned/custom gmapping/amcl configuration is necessary?

If it isn't really necessary, I think it's simpler to have a default reference set of parameters (these could be better) and then open up an option that lets people redirect configuration to their own customised parameter files. My concern is that even though you may have a camera turned set of parameters there, this is not the end of the story - the kind of environment and use case you are running in will also have an affect on the parameters you want to set. Enter turtlebot_configuration to help with this.

[kevincwells]

@tfoote -- I've updated the pull request. After spending some time looking at the changes needed in the CMakeLists.txt file to account for edge-case correctness, we decided that it was cleaner in the long run to simply have the symlinked files checked in, and the expectation that developers would create those symlinks themselves (or actual custom param files) when adding a new camera. If they were going to have to update the CMakeLists.txt file to add support for a new camera, it seems marginally more work to create the symlinks instead.

We were also discussing updating the ROS wiki documentation with a page providing a simple overview of how to add support for a new camera.

@stonier -- From our testing, we've found that even marginal changes to the parameters can make a significant improvement. Part of our change in this pull request also is parameterizing the files as arguments, so that even these defaults can be quickly and easily overridden.

[tfoote]

The symlinks seem like a reasonable compromise. I'll try to test it out tomorrow. Documentation improvements for adding a new camera sound like a great idea. I think you're the first to do it in a real way so that would be great.

[kevincwells]

@tfoote -- We've created this tutorial page with notes on how to add support for a new 3D sensor. I can update this page with instructions for the turtlebot_navigation package once this pull request is accepted.

[tfoote]

This took me a bit longer to debug than I expected. From my testing I had to update the implementation in the realsense_camera nodelets to support namespace remapping: https://github.com/intel-ros/realsense/pull/96

[tfoote]

Sorry for the delay testing this. I ran into some problems with the remapping not working correctly. I needed to patch the realsense_camera to make it work: https://github.com/intel-ros/realsense/pull/96 but with that I can switch between the r200 and the astra successfully with just the command line option.