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

fix shouldProcessScan() to do what is advertised #709

Open
wants to merge 4 commits into
base: humble
Choose a base branch
from

Conversation

Nibanovic
Copy link

Basic Info

Info Please fill out this column
Ticket(s) this addresses #451 #499 #545
Primary OS tested on Ubuntu 22.04.4 LTS
ROS distro: 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:
    1. it is the first scan
    2. if enough time has passed between two scans (as set by minimum_time_interval)
    3. if the robot position has changed enough (as set by minimum_travel_distance)
    4. if the robot heading has changed enough (as set by minimum_travel_heading)
  • as it currently stands, the function does not check iv at all, and the parameter in ii is set in the node, but is not propagated to the Mapper instance.
  • 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

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.

1 participant