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

Use GMapping nodelet #158

Open
wants to merge 1 commit into
base: indigo
Choose a base branch
from
Open

Conversation

kevincwells
Copy link
Contributor

Modified GMapping launch files to use the new nodelet
version of gmapping instead of the older node. This
allows the GMapping nodelet (which reads /scan) to be
run in the same nodelet manager as depthimage_to_laserscan
(which outputs /scan), allowing for more efficient
data processing.

Note that this was done in conjunction with a forthcoming change to the gmapping package (pull request: ros-perception/slam_gmapping#41), and serves as an example of the two nodelets running together under the same nodelet manager. Initial testing showed a ~11% reduction in overall memory usage and ~20% CPU usage running the slam_gmapping nodelet and depthimage_to_laserscan nodelet together, as compared to running slam_gmapping in a separate node. This PR is thus dependent on the above-linked PR, and needs that to land before it can be merged in.

Modified GMapping launch files to use the new nodelet
version of gmapping instead of the older node. This
allows the GMapping nodelet (which reads /scan) to be
run in the same nodelet manager as depthimage_to_laserscan
(which outputs /scan), allowing for more efficient
data processing.
@kevincwells
Copy link
Contributor Author

The above-referenced PR in the slam_gmapping package has been merged, so this can now be tested and merged if approved.

@kevincwells
Copy link
Contributor Author

@tfoote -- any feedback on this?

Given it's a launch file change only, I wonder why the kinetic build is failing.

@rohbotics
Copy link
Contributor

The kinetic build is failing due to some CMake warnings, which were not introduced by this PR.

I will try to test this sometime in the next couple days.

@@ -3,7 +3,7 @@
<arg name="base_frame" default="base_footprint"/>
<arg name="odom_frame" default="odom"/>

<node pkg="gmapping" type="slam_gmapping" name="slam_gmapping" output="screen">
<node pkg="nodelet" type="nodelet" name="slam_gmapping" args="load SlamGMappingNodelet camera/camera_nodelet_manager" output="screen">
Copy link
Contributor

Choose a reason for hiding this comment

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

People use the turtlebot gmapping with Lidars, in which case they would not have the camera nodelet manager.
I am not entirely sure that it is a good idea to make loading the nodelet the default.

I think it would be better to make a separate laser gmapping launch, that does not use the nodelet, before merging,

@rohbotics
Copy link
Contributor

@tfoote
I tested this and it works, but I would hold on merging as per my review.

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