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

Update command limiter of diff_drive_controller #1315

Merged
merged 19 commits into from
Dec 16, 2024

Conversation

christophfroehlich
Copy link
Contributor

@christophfroehlich christophfroehlich commented Oct 13, 2024

ros-controls/control_toolbox#212

It additionally adds parameters max_acceleration_reverse and max_deceleration_reverse to configure asymmetric acceleration/deceleration behavior.

Closes #1317

Copy link

codecov bot commented Oct 13, 2024

Codecov Report

Attention: Patch coverage is 68.08511% with 30 lines in your changes missing coverage. Please review.

Project coverage is 83.73%. Comparing base (950c9c1) to head (95a618f).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
...iff_drive_controller/src/diff_drive_controller.cpp 10.34% 20 Missing and 6 partials ⚠️
diff_drive_controller/src/odometry.cpp 0.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1315      +/-   ##
==========================================
+ Coverage   83.62%   83.73%   +0.10%     
==========================================
  Files         122      122              
  Lines       11018    11047      +29     
  Branches      934      937       +3     
==========================================
+ Hits         9214     9250      +36     
+ Misses       1495     1489       -6     
+ Partials      309      308       -1     
Flag Coverage Δ
unittests 83.73% <68.08%> (+0.10%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...de/diff_drive_controller/diff_drive_controller.hpp 100.00% <ø> (ø)
...troller/include/diff_drive_controller/odometry.hpp 100.00% <ø> (ø)
...er/include/diff_drive_controller/speed_limiter.hpp 100.00% <100.00%> (ø)
...ive_controller/test/test_diff_drive_controller.cpp 95.19% <100.00%> (+1.55%) ⬆️
diff_drive_controller/src/odometry.cpp 43.24% <0.00%> (ø)
...iff_drive_controller/src/diff_drive_controller.cpp 70.68% <10.34%> (-6.30%) ⬇️

... and 1 file with indirect coverage changes

Copy link
Member

@destogl destogl left a comment

Choose a reason for hiding this comment

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

Very nice!

destogl
destogl previously approved these changes Nov 22, 2024
@christophfroehlich christophfroehlich changed the title Move speed limiter to control_toolbox repo Update command limiter of diff_drive_controller Nov 22, 2024
Copy link

@Raivias Raivias left a comment

Choose a reason for hiding this comment

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

LGTM except the removing cmath.

@christophfroehlich
Copy link
Contributor Author

@tpoignonec as you are currently working on this topic, could you please test this and leave a review here?

Copy link

@tpoignonec tpoignonec left a comment

Choose a reason for hiding this comment

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

Maybe I missed something, but shouldn't the SpeedLimiter be also modified at this point to accept the new parameters as constructor arguments?
With the new parameters (i.e., max_acceleration_reverse and max_deceleration_reverse), everything is already ready to call

SpeedLimiter(
    double min_velocity = std::numeric_limits<double>::quiet_NaN(),
    double max_velocity = std::numeric_limits<double>::quiet_NaN(),
    double max_acceleration = std::numeric_limits<double>::quiet_NaN(),
    double max_deceleration = std::numeric_limits<double>::quiet_NaN(),
    double max_acceleration_reverse = std::numeric_limits<double>::quiet_NaN(),
    double max_deceleration_reverse = std::numeric_limits<double>::quiet_NaN(),
    double min_jerk = std::numeric_limits<double>::quiet_NaN(),
    double max_jerk = std::numeric_limits<double>::quiet_NaN())

@christophfroehlich
Copy link
Contributor Author

christophfroehlich commented Dec 1, 2024

Maybe I missed something, but shouldn't the SpeedLimiter be also modified at this point to accept the new parameters as constructor arguments? With the new parameters (i.e., max_acceleration_reverse and max_deceleration_reverse), everything is already ready to call

Totally right, I was refactoring that several times because I could not decide what to deprecate and where to use the RateLimiter class directly.
Finally, I left the diff_drive_controller::SpeedLimiter class member variable to avoid breaking ABI (not sure though, if anyone would have linked against this library). The diff_drive_controller::SpeedLimiter is now a wrapper around RateLimiter, where the old constructor with the booleans is deprecated. (it is more likely that someone used the limiter class outside of this repo).

@tpoignonec
Copy link

Functionnality-wise, seems all good.

With parameters

...
linear.x.max_velocity: 1.5
linear.x.min_velocity: -1.5
linear.x.max_acceleration: 2.0
linear.x.max_deceleration: -3.0
linear.x.max_acceleration_reverse: -4.0
linear.x.max_deceleration_reverse: 5.0
linear.x.max_jerk: .NAN
linear.x.min_jerk: .NAN

The asymmetric acceleration clipping works as expected (i.e., max(acc) = 2, -3, -4, and 5, respectively):
Screenshot 2024-12-02 162550

@christophfroehlich
Copy link
Contributor Author

Functionnality-wise, seems all good.

Thanks for testing this, you motivated me to write a gmock test for that case.

Copy link
Contributor

mergify bot commented Dec 4, 2024

This pull request is in conflict. Could you fix it @christophfroehlich?

Copy link
Member

@saikishor saikishor left a comment

Choose a reason for hiding this comment

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

Just few minor things. Overall looks good

diff_drive_controller/src/diff_drive_controller.cpp Outdated Show resolved Hide resolved
diff_drive_controller/src/diff_drive_controller.cpp Outdated Show resolved Hide resolved
diff_drive_controller/src/diff_drive_controller.cpp Outdated Show resolved Hide resolved
diff_drive_controller/src/diff_drive_controller.cpp Outdated Show resolved Hide resolved
diff_drive_controller/src/diff_drive_controller.cpp Outdated Show resolved Hide resolved
diff_drive_controller/src/diff_drive_controller.cpp Outdated Show resolved Hide resolved
Co-authored-by: Sai Kishor Kothakota <[email protected]>
saikishor
saikishor previously approved these changes Dec 6, 2024
Copy link
Member

@saikishor saikishor left a comment

Choose a reason for hiding this comment

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

Thanks for the changes. LGTM

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.

About min_acceleration when reversing
5 participants