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

WIP: Add AckermannDriveStamped control to steering library #1171

Open
wants to merge 16 commits into
base: iron
Choose a base branch
from

Conversation

wittenator
Copy link

This PR adds the option to use steering angle and linear velocity for controllers that inherit from the steering library. In anticipation of the merging and backport of the fixes in the ackermann controller, I branched off of the fix/steering_controllers_library_kinematics branch and cherry-picked the changes on top of the iron branch.

Status:

  • Fork the repository.
  • Modify the source; please focus on the specific change you are contributing. If you also reformat all the code, it will be hard for us to focus on your change.
  • Ensure local tests pass. (colcon test and pre-commit run (requires you to install pre-commit by pip3 install pre-commit)
  • Commit to your fork using clear commit messages.
  • Send a pull request, answering any default questions in the pull request interface.
  • Pay attention to any automated CI failures reported in the pull request, and stay involved in the conversation.

Copy link
Contributor

mergify bot commented Jun 17, 2024

@wittenator, all pull requests must be targeted towards the master development branch.
Once merged into master, it is possible to backport to iron, but it must be in master
to have these changes reflected into new distributions.

@wittenator
Copy link
Author

Ah hmm, I can't move our software stack to Jazzy (due to Nvidia dependencies) and therefore have to develop these changes on Iron. Should I develop the feature completely on Iron and then cherry-pick the changes on top of the master? The drawback is that I can't really test in on the master besides a few unit tests maybe.

@christophfroehlich
Copy link
Contributor

Let's merge #1150 first, and then proceed with this one here. Could you please review it and give your feedback (Files changed -> Review changes)?

Copy link
Contributor

mergify bot commented Jun 19, 2024

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

Copy link
Contributor

mergify bot commented Jul 6, 2024

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

@christophfroehlich
Copy link
Contributor

@wittenator is this WIP or ready for review?

Copy link

codecov bot commented Jul 18, 2024

Codecov Report

Attention: Patch coverage is 22.72727% with 51 lines in your changes missing coverage. Please review.

Project coverage is 86.83%. Comparing base (c824345) to head (12d3fa3).

Additional details and impacted files
@@            Coverage Diff             @@
##             iron    #1171      +/-   ##
==========================================
- Coverage   87.30%   86.83%   -0.48%     
==========================================
  Files          92       92              
  Lines        8408     8449      +41     
  Branches      701      711      +10     
==========================================
- Hits         7341     7337       -4     
- Misses        812      852      +40     
- Partials      255      260       +5     
Flag Coverage Δ
unittests 86.83% <22.72%> (-0.48%) ⬇️

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

Files Coverage Δ
...steering_controllers_library/steering_odometry.hpp 100.00% <ø> (ø)
...g_controller/src/ackermann_steering_controller.cpp 80.00% <0.00%> (ø)
...ing_controller/src/bicycle_steering_controller.cpp 75.00% <0.00%> (ø)
...ng_controller/src/tricycle_steering_controller.cpp 78.57% <0.00%> (ø)
...ring_controllers_library/src/steering_odometry.cpp 80.12% <33.33%> (-1.29%) ⬇️
...llers_library/src/steering_controllers_library.cpp 63.34% <23.33%> (-10.83%) ⬇️

... and 3 files with indirect coverage changes

@christophfroehlich
Copy link
Contributor

@wittenator please have a look at the failing pre-commit test, and add tests for your new feature because it is decreasing test coverage. Ping me again if you need guidance on how to write tests for the lib.

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.

2 participants