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

Using math::SpeedLimiter on the ackermann_steering controller. #837

Merged

Conversation

LolaSegura
Copy link
Contributor

@LolaSegura LolaSegura commented May 27, 2021

Signed-off-by: LolaSegura [email protected]

Summary

This replace the ackermann_steering controller speed limiter class for the one implemented in ignition math. Part of #810.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers
    🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸

@codecov
Copy link

codecov bot commented May 27, 2021

Codecov Report

❗ No coverage uploaded for pull request base (ign-gazebo4@c970caf). Click here to learn what that means.
The diff coverage is 75.00%.

Impacted file tree graph

@@              Coverage Diff               @@
##             ign-gazebo4     #837   +/-   ##
==============================================
  Coverage               ?   65.66%           
==============================================
  Files                  ?      240           
  Lines                  ?    17788           
  Branches               ?        0           
==============================================
  Hits                   ?    11681           
  Misses                 ?     6107           
  Partials               ?        0           
Impacted Files Coverage Δ
...rc/systems/ackermann_steering/AckermannSteering.cc 85.98% <75.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c970caf...ea8b861. Read the comment docs.

Copy link
Contributor

@caguero caguero left a comment

Choose a reason for hiding this comment

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

It looks good to me!

Copy link
Contributor

@caguero caguero left a comment

Choose a reason for hiding this comment

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

Nit: I think we don't need the #include <limits> anymore.

@LolaSegura LolaSegura force-pushed the LolaSegura/math_speedlimiter_inside_ackermann_steering branch from 61859e1 to ea8b861 Compare June 1, 2021 17:20
@LolaSegura
Copy link
Contributor Author

Nit: I think we don't need the #include anymore.

@caguero you are right, I deleted it. Thanks for catching that.

@scpeters
Copy link
Member

scpeters commented Jun 3, 2021

looks good to me; I think the only test failures are flaky

@scpeters scpeters merged commit 657ca6d into ign-gazebo4 Jun 3, 2021
@scpeters scpeters deleted the LolaSegura/math_speedlimiter_inside_ackermann_steering branch June 3, 2021 19:26
@scpeters scpeters mentioned this pull request Jun 24, 2021
chapulina pushed a commit that referenced this pull request Jul 26, 2022
chapulina added a commit that referenced this pull request Jul 26, 2022
* Ackermann Steering Plugin (#618)

Signed-off-by: Kevin <[email protected]>

Co-authored-by: Kevin <[email protected]>
Co-authored-by: Louise Poubel <[email protected]>

* Using math::SpeedLimiter on the ackermann_steering controller. (#837)

Signed-off-by: LolaSegura <[email protected]>

* Add Tf publishing to AckermannSteering system (#1576)

Signed-off-by: Andrew Ealovega <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>

Co-authored-by: Alejandro Hernández Cordero <[email protected]>
Co-authored-by: Louise Poubel <[email protected]>

Co-authored-by: knoedler <[email protected]>
Co-authored-by: Kevin <[email protected]>
Co-authored-by: LolaSegura <[email protected]>
Co-authored-by: Andrew Ealovega <[email protected]>
Co-authored-by: Alejandro Hernández Cordero <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔮 dome Ignition Dome
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants