Skip to content

Omni wheel drive odometry update#2286

Open
Devdoot57 wants to merge 6 commits into
ros-controls:masterfrom
Devdoot57:omni-drive-odom-update
Open

Omni wheel drive odometry update#2286
Devdoot57 wants to merge 6 commits into
ros-controls:masterfrom
Devdoot57:omni-drive-odom-update

Conversation

@Devdoot57

Copy link
Copy Markdown
Contributor

Description

This PR updates the omni_wheel_drive_controller odometry implementation to use double dt for integration instead of relying on rclcpp::Time (matching #1854). It also introduces a comprehensive gtest suite to verify the holonomic kinematics and integration math (matching #2099).

This rolls out the architectural improvements requested in #2039 for the omni-wheel base.

Changes:

  • Odometry API: Deprecated updateFromPos, updateFromVel, and updateOpenLoop. Replaced with update_from_pos, update_from_vel, and try_update_open_loop.
  • Controller Loop: Updated the main controller update to account for the above mentioned changes.
  • Testing: Added test_odometry.cpp covering:
    • Independent holonomic Y and X movements.
    • Exact arc integration for simultaneous X, Y, and rotational velocities.
    • Boundary checks for small dt rejection.

@codecov

codecov Bot commented Apr 11, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 84.84848% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.66%. Comparing base (58969bf) to head (600a7a2).

Files with missing lines Patch % Lines
omni_wheel_drive_controller/src/odometry.cpp 77.41% 4 Missing and 3 partials ⚠️
...ive_controller/src/omni_wheel_drive_controller.cpp 25.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2286      +/-   ##
==========================================
- Coverage   86.76%   86.66%   -0.11%     
==========================================
  Files         148      149       +1     
  Lines       15838    15897      +59     
  Branches     1347     1351       +4     
==========================================
+ Hits        13742    13777      +35     
- Misses       1601     1622      +21     
- Partials      495      498       +3     
Flag Coverage Δ
unittests 86.66% <84.84%> (-0.11%) ⬇️

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

Files with missing lines Coverage Δ
...r/include/omni_wheel_drive_controller/odometry.hpp 100.00% <ø> (ø)
omni_wheel_drive_controller/test/test_odometry.cpp 100.00% <100.00%> (ø)
...ive_controller/src/omni_wheel_drive_controller.cpp 83.46% <25.00%> (-0.39%) ⬇️
omni_wheel_drive_controller/src/odometry.cpp 66.66% <77.41%> (-19.25%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions

github-actions Bot commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

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.

@github-actions github-actions Bot added the stale label Jun 1, 2026
@mergify

mergify Bot commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

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

@github-actions github-actions Bot removed the stale label Jun 2, 2026

bool update_from_pos(const std::vector<double> & wheels_pos, double dt);
bool update_from_vel(const std::vector<double> & wheels_vel, double dt);
bool try_update_open_loop(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
bool try_update_open_loop(
bool update_open_loop(

why the try_ prefix here?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I see that this is similar to #1854, what was the intention back then @Amronos? because it won't get updated when dt is close to zero?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't really remember the reason now, update_open_loop would be better though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Needs review

Development

Successfully merging this pull request may close these issues.

3 participants