ros2_controllers icon indicating copy to clipboard operation
ros2_controllers copied to clipboard

Use Generic Steering Controller State Message in Steering Controllers Library

Open mbharatheesha opened this issue 1 year ago • 4 comments

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.

mbharatheesha avatar Nov 15 '23 15:11 mbharatheesha

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.

:exclamation: 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%) :arrow_down:

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%) :arrow_down:

... and 38 files with indirect coverage changes

codecov[bot] avatar Nov 15 '23 21:11 codecov[bot]

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!

mbharatheesha avatar Nov 16 '23 06:11 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.

christophfroehlich avatar Nov 16 '23 09:11 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.

Alright, thanks!

mbharatheesha avatar Nov 16 '23 10:11 mbharatheesha