Improve diff_drive_controller odometry velocity and timestep handling #1394
+45
−33
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
use velocity where it claims to, and pass timestep in to every method that makes use of it. This was requested in ros-controls#271. - The result is that the class is very slightly less efficient (but still quite trivial) due to passing around a extra double. - The other result is that it is no longer possible for velocity based odometry updates calculated incorrectly when the timestep fails to keep up. To prevent position based updates from now suffering the inverse of the velocity timestep/dt mismatch, the measured delta is used exclusively for position based updates (retaining the existing behavior), while the configured timestep is used for open and closed loop velocity updates. I also added a 1s throttled warning if there is a major mismatch between measured and configured timestep (+/-20%) since this would cause a slight accumulation of integration errors on curved paths (I believe), depending on velocity, curvature and mismatch size. Similar requested period/measured period mismatches may be a nuisance for open loop (which is velocity based) and closed loop velocity based updates, but the impact is likely negligible (in this class).
github-actions
bot
requested review from
anfemosa,
ARK3r,
bijoua29,
christophfroehlich,
DasRoteSkelett,
destogl and
LukasMacha97
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR updates diff_drive odometry class to
This was requested in #271, although I took a more expansive approach since the odometry class is was at high risk of new bugs due the unclear units of variables at play.
The result is that the class is very slightly less efficient (but still quite trivial) due to passing around a extra double.
The other result is that it is no longer possible for velocity based odometry updates calculated incorrectly when the timestep fails to keep up.
To prevent position based updates from now suffering the inverse of the velocity timestep/dt mismatch, the measured delta is used exclusively for position based updates (retaining the existing behavior), while the configured timestep is used for open and closed loop velocity updates. This decision is made by diff_drive_controller, and out of scope of the odometry class.
I also added a 1s throttled warning if there is a major mismatch between measured and configured timestep (+/-20%) since this would cause a slight accumulation of integration errors on curved paths (I believe), depending on velocity, curvature and mismatch size. This could probably be expanded to a more general diagnostics WARN output, since controller/odometry updates failing to keep up can be serious.
Similar requested period/measured period mismatches may be a nuisance for open loop (which is velocity based) and closed loop velocity based updates, but the impact is likely negligible (in this class).