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 1 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
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