Skip to content

Conversation

@kumar-sanjeeev
Copy link
Contributor

@kumar-sanjeeev kumar-sanjeeev commented Nov 8, 2025

This PR includes the following changes:

  • Renamed the Odometry class to SteeringKinematics to better represent its support for both FK and IK.
  • Added deprecation warnings in the old steering_odometry.hpp header file to maintain backward compatibility.
  • Kept the steering_odometry.cpp for backward compatibility and redirected actual implementation to steering_odometry.cpp
  • Updated comments in the header file to reflect the new class name.

@codecov
Copy link

codecov bot commented Nov 8, 2025

Codecov Report

❌ Patch coverage is 81.17647% with 48 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.13%. Comparing base (d71538a) to head (348e34e).
⚠️ Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
...ng_controllers_library/src/steering_kinematics.cpp 83.42% 28 Missing and 1 partial ⚠️
...ring_controllers_library/src/steering_odometry.cpp 0.00% 19 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1996      +/-   ##
==========================================
- Coverage   85.32%   85.13%   -0.20%     
==========================================
  Files         143      144       +1     
  Lines       13936    13968      +32     
  Branches     1201     1201              
==========================================
  Hits        11891    11891              
- Misses       1638     1670      +32     
  Partials      407      407              
Flag Coverage Δ
unittests 85.13% <81.17%> (-0.20%) ⬇️

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

Files with missing lines Coverage Δ
...g_controller/src/ackermann_steering_controller.cpp 81.57% <100.00%> (ø)
...ing_controller/src/bicycle_steering_controller.cpp 76.92% <100.00%> (ø)
...eering_controllers_library/steering_kinematics.hpp 100.00% <100.00%> (ø)
...llers_library/src/steering_controllers_library.cpp 68.40% <100.00%> (ø)
...library/test/test_steering_controllers_library.hpp 97.43% <100.00%> (ø)
...ontrollers_library/test/test_steering_odometry.cpp 100.00% <100.00%> (ø)
...ng_controller/src/tricycle_steering_controller.cpp 81.25% <100.00%> (ø)
...ring_controllers_library/src/steering_odometry.cpp 0.00% <0.00%> (-83.43%) ⬇️
...ng_controllers_library/src/steering_kinematics.cpp 83.42% <83.42%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@kumar-sanjeeev
Copy link
Contributor Author

Hi @christophfroehlich,

I am unable to figure out why the Rolling-ABI CI/CD check is failing, however it builds fine locally. Also, I have updated the header files in all locations where steering_odometry.hpp was used to suppress the deprecation warnings.

@christophfroehlich
Copy link
Contributor

Hi @christophfroehlich,

I am unable to figure out why the Rolling-ABI CI/CD check is failing, however it builds fine locally. Also, I have updated the header files in all locations where steering_odometry.hpp was used to suppress the deprecation warnings.

You are changing the ABI with your PR, so this test is supposed to fail. See the test report. Instead of using type aliases, you could leave the old methods but just call the new one from inside. This should leave ABI untouched.
The current state is fine for rolling, but if we backport this ABI break here is subject for discussion.

@kumar-sanjeeev
Copy link
Contributor Author

Hi @christophfroehlich,
I am unable to figure out why the Rolling-ABI CI/CD check is failing, however it builds fine locally. Also, I have updated the header files in all locations where steering_odometry.hpp was used to suppress the deprecation warnings.

You are changing the ABI with your PR, so this test is supposed to fail. See the test report. Instead of using type aliases, you could leave the old methods but just call the new one from inside. This should leave ABI untouched. The current state is fine for rolling, but if we backport this ABI break here is subject for discussion.

I understand the concern, and I also liked your idea to prevent the ABI check from failing. Regarding your suggestion Instead of using type aliases, you could leave the old methods but just call the new one from inside, should I start working on this, or wait for further discussion?

@christophfroehlich
Copy link
Contributor

Please do so. I now that it is a bit of a tedious work, but then we can easily backport it to kilted.

@kumar-sanjeeev
Copy link
Contributor Author

Please do so. I now that it is a bit of a tedious work, but then we can easily backport it to kilted.

understood, i will do this.

@kumar-sanjeeev
Copy link
Contributor Author

Please do so. I now that it is a bit of a tedious work, but then we can easily backport it to kilted.

Hi @christophfroehlich ,

Is there a recommended way to run the GitHub Actions workflow locally for testing?

@christophfroehlich
Copy link
Contributor

you can use act to run them locally, a brief description is here.

@kumar-sanjeeev
Copy link
Contributor Author

you can use act to run them locally, a brief description is here.

thanks for letting me know. The ABI check is still failing with the new push. I’m digging into logs to find a reason and will update with fix. I’ll let you know once everything is ready from myside.

@kumar-sanjeeev kumar-sanjeeev force-pushed the feat-refactor-odometry-class branch from 4589166 to 348e34e Compare November 18, 2025 17:05
@kumar-sanjeeev kumar-sanjeeev marked this pull request as ready for review November 18, 2025 17:34
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, and ABI checker is also fine with it!

@christophfroehlich christophfroehlich added the backport-jazzy Triggers PR backport to ROS 2 jazzy. label Nov 18, 2025
@christophfroehlich christophfroehlich added the backport-kilted Triggers PR backport to ROS 2 kilted. label Nov 18, 2025
@christophfroehlich christophfroehlich linked an issue Nov 18, 2025 that may be closed by this pull request
8 tasks
Copy link
Member

@saikishor saikishor left a comment

Choose a reason for hiding this comment

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

The code looks fine. Didn't pay too much attention to the logic. I'll trust @christophfroehlich 😉

@christophfroehlich christophfroehlich merged commit f59b5a0 into ros-controls:master Nov 19, 2025
20 checks passed
mergify bot pushed a commit that referenced this pull request Nov 19, 2025
mergify bot pushed a commit that referenced this pull request Nov 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport-jazzy Triggers PR backport to ROS 2 jazzy. backport-kilted Triggers PR backport to ROS 2 kilted.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Refactor odometry class of steering controllers library

3 participants