Skip to content

[mecanum_drive_controller] Fix Odometry Initialization #1573

Merged
christophfroehlich merged 5 commits intoros-controls:masterfrom
Juliaj:mecanum_drive_controller_init
Mar 11, 2025
Merged

[mecanum_drive_controller] Fix Odometry Initialization #1573
christophfroehlich merged 5 commits intoros-controls:masterfrom
Juliaj:mecanum_drive_controller_init

Conversation

@Juliaj
Copy link
Copy Markdown
Contributor

@Juliaj Juliaj commented Mar 5, 2025

This PR addresses an issue where the odometry object wasn't initialized properly in the mecanum_drive_controller. The odometry_.init() call was missing, which the base_frame_offset_ could contain random values, leading to incorrect calculations in the transformation matrix. Specifically, this affects the line:

tf2::Vector3 linear_transformation_from_center_2_base =
    angular_transformation_from_center_2_base *
    tf2::Vector3(-base_frame_offset_[0], -base_frame_offset_[1], 0.0);

and causes velocity_in_base_frame_linear_x and velocity_in_base_frame_linear_y to grow unbound. A test case was added to guard against this issue.

Copy link
Copy Markdown
Member

@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.

We have a different API now for the handles, see #1565
Can you focus here on the actual bugfix please? @kumar-sanjeeev is already working on the handles fixes in this repo, which is not only a search-and-replace job.

@Juliaj
Copy link
Copy Markdown
Contributor Author

Juliaj commented Mar 5, 2025

We have a different API now for the handles, see #1565
Can you focus here on the actual bugfix please?

Thanks @christophfroehlich, I missed that, reverted my changes.

@kumar-sanjeeev is already working on the handles fixes in this repo, which is not only a search-and-replace job.

Thank you, Kumar for making the fixes. In the new test I added, I'm still making deprecated get_value() call. Let me know whether you'd prefer for me to change it to new style in this PR.

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 5, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.86%. Comparing base (90a9674) to head (fb1772e).
Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1573      +/-   ##
==========================================
+ Coverage   84.74%   84.86%   +0.12%     
==========================================
  Files         124      124              
  Lines       11514    11566      +52     
  Branches      983      987       +4     
==========================================
+ Hits         9758     9816      +58     
+ Misses       1441     1433       -8     
- Partials      315      317       +2     
Flag Coverage Δ
unittests 84.86% <100.00%> (+0.12%) ⬆️

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

Files with missing lines Coverage Δ
...ller/include/mecanum_drive_controller/odometry.hpp 100.00% <100.00%> (ø)
..._drive_controller/src/mecanum_drive_controller.cpp 90.68% <100.00%> (+0.11%) ⬆️
..._controller/test/test_mecanum_drive_controller.cpp 100.00% <100.00%> (ø)
..._controller/test/test_mecanum_drive_controller.hpp 86.51% <ø> (+0.47%) ⬆️

... and 8 files with indirect coverage changes

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Member

@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.

I see your point of the missing init. But the test is succeeding also without the init call. Can you verify that?

@Juliaj
Copy link
Copy Markdown
Contributor Author

Juliaj commented Mar 10, 2025

I see your point of the missing init. But the test is succeeding also without the init call. Can you verify that?

It is because that issue is related to accessing uninitialized memory. Even though it is fairly easy to repro, it is not 100% reproducible. I added explicit steps to verify the base_frame_offset is set properly.

Copy link
Copy Markdown
Member

@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.

Thanks!

Copy link
Copy Markdown
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.

LGTM

@christophfroehlich christophfroehlich merged commit b333def into ros-controls:master Mar 11, 2025
24 of 27 checks passed
@christophfroehlich christophfroehlich added the backport-humble Triggers PR backport to ROS 2 humble. label Mar 11, 2025
mergify bot pushed a commit that referenced this pull request Mar 11, 2025
christophfroehlich pushed a commit that referenced this pull request Mar 11, 2025
… (#1583)

Co-authored-by: Julia Jia <juliajster@gmail.com>
@Juliaj Juliaj deleted the mecanum_drive_controller_init branch March 26, 2025 21:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport-humble Triggers PR backport to ROS 2 humble.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants