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

Use Generic Steering Controller State Message in Steering Controllers Library #836

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

mbharatheesha
Copy link

I was reading through the code as a part of learning how to write a new steering controller. I understood that a new steering controller inherits from the SteeringControllersLibrary. But I also found the use of Ackermannxxx in the base class code for a status message and the same was being used in other steering controllers such as BicycleSteeringController and TricycleSteeringController.

If I understood the inheritance and the code structure correctly, the SteerinControllersLibrary should be agnostic to the specifics of a steering controller that derives from it. Therefore, this PR introduces the following changes:

  • Remove the use of Ackermannxxx declarations in the SteeringControllersLibrary base class and rename it to SteeringControllerStateMsg
  • Modify the tests and state publishers to use the renamed declaration
  • Remove ackermann_msgs dependency from steering_controllers_library package.

All tests for the modified packages were run successfully (locally).

P.S: Sorry, this took longer than planned. It took me some time to figure out the relevant workspace overlays to make sure the correct version of controller_interface was used for the build to succeed.

Copy link
Contributor

@christophfroehlich christophfroehlich left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

codecov bot commented Nov 15, 2023

Codecov Report

Attention: Patch coverage is 0% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 47.69%. Comparing base (0b43291) to head (fd71b75).
Report is 8 commits behind head on master.

❗ Current head fd71b75 differs from pull request most recent head bfc78b8. Consider uploading reports for the commit bfc78b8 to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           master     #836       +/-   ##
===========================================
- Coverage   71.86%   47.69%   -24.17%     
===========================================
  Files          41       41               
  Lines        3650     3862      +212     
  Branches     1794     1816       +22     
===========================================
- Hits         2623     1842      -781     
- Misses        707      758       +51     
- Partials      320     1262      +942     
Flag Coverage Δ
unittests 47.69% <0.00%> (-24.17%) ⬇️

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

Files Coverage Δ
...llers_library/src/steering_controllers_library.cpp 41.69% <0.00%> (-30.96%) ⬇️

... and 38 files with indirect coverage changes

@mbharatheesha
Copy link
Author

The build failures I am seeing on the workflow with rolling binaries are exactly the same I had because the latest master of controller_interface pkg in ros2_control wasn't yet available on rolling binaries as of yesterday. Basically, this change is not yet available on rolling binaries.

I am not sure how the changes I made have impacted code coverage because I mainly renamed declarations and removed unused declarations. If that needs to be fixed, I could use some help on the same. Thank you!

@christophfroehlich
Copy link
Contributor

We know of the failing CI of the binary builds, and the coverage diff of 0.05% is some "noise" you don't have to worry about.

@mbharatheesha
Copy link
Author

We know of the failing CI of the binary builds, and the coverage diff of 0.05% is some "noise" you don't have to worry about.

Alright, thanks!

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