Use Generic Steering Controller State Message in Steering Controllers Library

[mbharatheesha]

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.

[christophfroehlich]

LGTM

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.08%. Comparing base (39c8ad3) to head (9863558).
Report is 5 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #836      +/-   ##
==========================================
- Coverage   85.19%   85.08%   -0.12%     
==========================================
  Files         127      123       -4     
  Lines       12006    11914      -92     
  Branches     1010     1005       -5     
==========================================
- Hits        10229    10137      -92     
  Misses       1460     1460              
  Partials      317      317              

Flag	Coverage Δ
unittests	85.08% <ø> (-0.12%)	⬇️

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

see 4 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[mbharatheesha]

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]

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]

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!

[github-actions]

This PR is stale because it has been open for 45 days with no activity. Please tag a maintainer for help on completing this PR, or close it if you think it has become obsolete.

[mergify]

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

[wittenator]

Does this removal make sense if we are going to reintroduce the exact same lines again in #1563 ? Ackermann is also a bit of a misnomer here in my opinion since it refers to classical ackermann-like control inputs (e.g. steering angle and linear velocity) instead of the actual kinematics of the robot. (nothing we can do about that though)

[christophfroehlich]

Does this removal make sense if we are going to reintroduce the exact same lines again in #1563 ?

For the current state it makes sense, and this PR is way older than yours ;) we just forgot to merge it.

Ackermann is also a bit of a misnomer here in my opinion since it refers to classical ackermann-like control inputs (e.g. steering angle and linear velocity) instead of the actual kinematics of the robot.

What else do you suggest? (this is not an issue with this PR here?)

[wittenator]

I just wanted to mention that if my PR gets accepted in a form similar to what is there now, these lines come back exactly how they are about to be removed.