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

Add pause and reset services to hector_mapping #86

Conversation

marcelino-pensa
Copy link
Contributor

@marcelino-pensa marcelino-pensa commented Mar 9, 2021

I have an application in which I need to pause/resume/reset hector_mapping. I made these modifications, and I believe that it might be useful to other users as well.

I noticed that it was possible to reset the map by publishing the string reset in the topic sys_msg_topic, but I needed it to be a service call instead of a topic publishing (I needed to block my code execution until the map was effectively reset).

I hope that these modifications are considered useful, and that this can merge to the main branch.

Note: I will add comments to my code changes to clarify everything I did
Note 2: This PR would provide a solution to the old issue in #37
Note 3: This PR is backward-compatible, as it doesn't change any behavior for code that interfaces with hector_mapping, it only adds new services that can be used when interfacing with it

closes #37

Comment on lines +4 to +13
find_package(catkin REQUIRED COMPONENTS
roscpp
nav_msgs
visualization_msgs
tf
message_filters
laser_geometry
tf_conversions
message_generation
std_srvs)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I separated by one-per-line (for readability), but I can revert this if requested...
In addition, I added the package std_srvs so that I can add the service calls to this code

@@ -50,6 +50,7 @@ HectorMappingRos::HectorMappingRos()
, tfB_(0)
, map__publish_thread_(0)
, initial_pose_set_(false)
, pause_scan_processing_(false)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The node starts as "unpaused" (no changes in behavior)

Comment on lines 160 to 162
// Initialize services to reset map, and toggle scan pause
resetMapService_ = node_.advertiseService("reset_map", &HectorMappingRos::resetMapCallback, this);
toggleScanProcessingService_ = node_.advertiseService("pause_mapping", &HectorMappingRos::pauseMapCallback, this);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

new services

Comment on lines +236 to +238
if (pause_scan_processing_) {
return;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

if paused, then we don't process scanCallback

Comment on lines +384 to +390
bool HectorMappingRos::resetMapCallback(std_srvs::Trigger::Request &req,
std_srvs::Trigger::Response &res)
{
ROS_INFO("HectorSM Reset map service called");
slamProcessor->reset();
return true;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

pretty much the same behavior as in sysMsgCallback

Comment on lines 392 to 405
bool HectorMappingRos::pauseMapCallback(std_srvs::SetBool::Request &req,
std_srvs::SetBool::Response &res)
{
if (req.data && !pause_scan_processing_)
{
ROS_INFO("HectorSM Mapping paused");
}
else if (!req.data && pause_scan_processing_)
{
ROS_INFO("HectorSM Mapping no longer paused");
}
pause_scan_processing_ = req.data;
return true;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

simple service callback that sets pause_scan_processing_ to whatever was requested in req

Comment on lines +42 to +43
#include <std_srvs/SetBool.h>
#include <std_srvs/Trigger.h>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

new necessary includes

Comment on lines +81 to +82
bool resetMapCallback(std_srvs::Trigger::Request &req, std_srvs::Trigger::Response &res);
bool pauseMapCallback(std_srvs::SetBool::Request &req, std_srvs::SetBool::Response &res);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

new service callbacks

Comment on lines 123 to 124
ros::ServiceServer resetMapService_;
ros::ServiceServer toggleScanProcessingService_;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ros service servers

Copy link
Member

Choose a reason for hiding this comment

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

Usually, we use snake_case for variable names, though, this is a bit inconsistent in this repo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds good! that goes along with Google's Style Guide! I will make these variables snake_case, and maybe I'll spend sometime in the future to fix this whole class (in another PR).

@@ -140,6 +148,7 @@ class HectorMappingRos
bool initial_pose_set_;
Eigen::Vector3f initial_pose_;

bool pause_scan_processing_;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

class variable that determines whether we should process scans or not

Comment on lines 123 to 124
ros::ServiceServer resetMapService_;
ros::ServiceServer toggleScanProcessingService_;
Copy link
Member

Choose a reason for hiding this comment

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

Usually, we use snake_case for variable names, though, this is a bit inconsistent in this repo.

hector_mapping/src/HectorMappingRos.cpp Outdated Show resolved Hide resolved
@StefanFabian
Copy link
Member

Thank you for your PR!
Apart from some minor details, it looks good to me.

@marcelino-pensa
Copy link
Contributor Author

@StefanFabian thank you very much for your review! I have addressed your concerns, and I hope that the PR is up to standards right now =)

@StefanFabian StefanFabian merged commit 50eadc5 into tu-darmstadt-ros-pkg:noetic-devel Mar 15, 2021
@StefanFabian
Copy link
Member

Thank you for your contribution!

@marcelino-pensa marcelino-pensa deleted the feature/ma-add-pause-reset-services-hector branch March 15, 2021 19:56
@marcelino-pensa
Copy link
Contributor Author

thanks for the prompt reviews, @StefanFabian! I have updated the wiki documentation to reflect the updates in this PR...

In the meantime, I wonder if it would be of interest to have a service call that handles Hector management in a single call (see the definition below). I have been using this in a fork of mine, but it might benefit other ROS hector users:

# Service definition for hector processing management
# -------------------------------------------------------------
# - - - - - Inputs - - - - -
# pause: whether hector should pause or resume mapping
# reset_map: if true, resets the whole map
# reset_pose: if true, reset hector's pose (only x, y, and yaw are captured)
# pose: if (reset_pose == true), then this pose is captured to reset hector's pose
# - - - - - Outputs - - - - - 
# success: boolean indicating whether the service call was successful
# -------------------------------------------------------------
bool pause
bool reset_map
bool reset_pose
geometry_msgs/Pose pose
---
bool success

@jan0145
Copy link

jan0145 commented Jul 9, 2021

This maybe a dumb question but I made all the changes and now I'm wondering what button to push or command to write to pause Hector slam. (I'm not too familiar with cpp).

@marcelino-pensa
Copy link
Contributor Author

If you're okay with using the command line, then here are some commands that you can use:

  1. Pause mapping:
    rosservice call /pause_mapping "data: true"

  2. Resume mapping:
    rosservice call /pause_mapping "data: false"

  3. Reset map:
    rosservice call /reset_map "{}"

  4. Restart map and set new initial pose for hector:

rosservice call /restart_mapping_with_new_pose "initial_pose:
  position:
    x: 0.0
    y: 0.0
    z: 0.0
  orientation:
    x: 0.0
    y: 0.0
    z: 0.0
    w: 1.0"

@marcelino-pensa
Copy link
Contributor Author

marcelino-pensa commented Jul 9, 2021

If you want to have it in your code, you could either incorporate system calls to the commands above, or (if you want to do it right) setup a ROS service client. Here is the ROS reference for that: http://wiki.ros.org/ROS/Tutorials/WritingServiceClient%28c%2B%2B%29

@jan0145
Copy link

jan0145 commented Jul 10, 2021 via email

@StefanFabian
Copy link
Member

If you want to have it in your code, you could either incorporate system calls to the commands above, or setup a ROS service client. Here is the ROS reference for that: http://wiki.ros.org/ROS/Tutorials/WritingServiceClient%28c%2B%2B%29

Please absolutely never use system calls in C++.

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.

Any way to easily "Pause" hector slam or temporarily ignore scans
3 participants