Humble (same issue in Rolling, but I'm using Humble for now)
Robotic platform tested on
gazebo simulation turtlebot (actually a proprietary robot, but the same as turtlebot in all ways that matter here)
Description of contribution in a few bullet points
we ensure minimum_travel_heading parameter does what is intended - make SLAM process scans if the robot heading has changed enough. See Issue tickets above for more info
we refactorMapper::HasMovedEnough(), breaking the function into three, so that
checks advertised in shouldProcessScan() are implemented in a single place
Mapper::HasMovedEnough() does not do more than it says it does
we ensure minimum_time_interval parameter is propagated from the ROS node to the internal Mapper instance
Contribution in more detail
in the slam_toolbox_common, there is a check whether to process the incoming scan. If the node is not paused or throttled out, we process the scan if either of these is true:
it is the first scan
if enough time has passed between two scans (as set by minimum_time_interval)
if the robot position has changed enough (as set by minimum_travel_distance)
if the robot heading has changed enough (as set by minimum_travel_heading)
In this PR, I propose a fix where shouldProcessScan() would use the checks already implemented in Mapper class, which is already instanced in the ROS node and holds all of the relevant parameters, instead of implementing them again
Along the way, we refactor Mapper::HasMovedEnough() function that is also misleadingly named, as it not only checks has the robot moved enough, but also:
is it the first scan
did the robot turn enough
has enough time passed between scans
We split this function into three functions, so its parts can be accessed from shouldProcessScan(), and introduce clarity into Mapper.cpp. It seems to me this is not a huge change, as the function Mapper::HasMovedEnough() is only called in two places in total, when processing scans.
Lastly, we ensure that minimum_time_interval parameter is properly propagated from the ROS parameters to the internal Mapper instance, so that we can use the newly created Mapper::EnoughTimeHasPassed() function
Basic Info
Description of contribution in a few bullet points
minimum_travel_headingparameter does what is intended - make SLAM process scans if the robot heading has changed enough. See Issue tickets above for more infoMapper::HasMovedEnough(), breaking the function into three, so thatshouldProcessScan()are implemented in a single placeMapper::HasMovedEnough()does not do more than it says it doeswe ensure
minimum_time_intervalparameter is propagated from the ROS node to the internalMapperinstanceContribution in more detail
slam_toolbox_common, there is a check whether to process the incoming scan. If the node is not paused or throttled out, we process the scan if either of these is true:minimum_time_interval)minimum_travel_distance)minimum_travel_heading)ivat all, and the parameter iniiis set in the node, but is not propagated to theMapperinstance.shouldProcessScan()would use the checks already implemented inMapperclass, which is already instanced in the ROS node and holds all of the relevant parameters, instead of implementing them againMapper::HasMovedEnough()function that is also misleadingly named, as it not only checks has the robot moved enough, but also:shouldProcessScan(), and introduce clarity intoMapper.cpp. It seems to me this is not a huge change, as the functionMapper::HasMovedEnough()is only called in two places in total, when processing scans.minimum_time_intervalparameter is properly propagated from the ROS parameters to the internalMapperinstance, so that we can use the newly createdMapper::EnoughTimeHasPassed()function