Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[diff_drive] Remove unused parameter and add simple validation #abi-breaking #958

Merged
merged 1 commit into from
Jan 31, 2024
Merged

[diff_drive] Remove unused parameter and add simple validation #abi-breaking #958

merged 1 commit into from
Jan 31, 2024

Conversation

christophfroehlich
Copy link
Contributor

@christophfroehlich christophfroehlich commented Dec 31, 2023 •
edited
Loading

  • Remove unused parameter wheels_per_side
  • Update description of names parameters (looking for joint names, not link names).
  • add simple validation to names parameters, this needs some rework of the tests
  • let the load_diff_drive test use the yaml from the test file (which had two bugs)

This breaks ABI because of the class variable change, is this a bad idea to backport?

@christophfroehlich christophfroehlich added backport-humble This label should be used by maintainers only! Label triggers PR backport to ROS2 humble. backport-iron labels Dec 31, 2023
@codecov Codecov
Copy link

codecov bot commented Dec 31, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d915497) 47.43% compared to head (364d3c5) 47.47%.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master     #958      +/-   ##
==========================================
+ Coverage   47.43%   47.47%   +0.03%     
==========================================
  Files          41       41              
  Lines        3862     3859       -3     
  Branches     1816     1814       -2     
==========================================
  Hits         1832     1832              
+ Misses        769      767       -2     
+ Partials     1261     1260       -1
Flag Coverage Δ
unittests 47.47% <100.00%> (+0.03%) ⬆️

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

Files Coverage Δ
...de/diff_drive_controller/diff_drive_controller.hpp 100.00% <ø> (ø)
...iff_drive_controller/src/diff_drive_controller.cpp 42.85% <100.00%> (+0.45%) ⬆️

bmagyar
bmagyar approved these changes Jan 31, 2024
View reviewed changes
Copy link
Member

@bmagyar bmagyar left a comment

Choose a reason for hiding this comment

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

Wonderful, thank you!

@bmagyar
Copy link
Member

bmagyar commented Jan 31, 2024

I think we should backport to Iron. Let's leave Humble as-is

@bmagyar bmagyar removed the backport-humble This label should be used by maintainers only! Label triggers PR backport to ROS2 humble. label Jan 31, 2024
@bmagyar bmagyar changed the title [diff_drive] Remove unused parameter and add simple validation [diff_drive] Remove unused parameter and add simple validation #abi-breaking Jan 31, 2024
@bmagyar bmagyar merged commit 2705ca8 into ros-controls:master Jan 31, 2024
13 of 15 checks passed
mergify bot pushed a commit that referenced this pull request Jan 31, 2024
@christophfroehlich @mergify
[diff_drive] Remove unused parameter and add simple validation #abi-b…
9549b42 
…reaking (#958)

(cherry picked from commit 2705ca8)

# Conflicts:
#	diff_drive_controller/test/test_diff_drive_controller.cpp
@mergify mergify bot mentioned this pull request Jan 31, 2024
Closed
@christophfroehlich christophfroehlich deleted the diff_drive_update branch January 31, 2024 21:46
pac48 pushed a commit to pac48/ros2_controllers that referenced this pull request Feb 2, 2024
@christophfroehlich @pac48
[diff_drive] Remove unused parameter and add simple validation #abi-b…
c71ee90 
…reaking (ros-controls#958)
@christophfroehlich christophfroehlich mentioned this pull request Feb 19, 2024
Merged
bmagyar pushed a commit that referenced this pull request Mar 1, 2024
@christophfroehlich
[diff_drive] Remove unused parameter and add simple validation #abi-b…
636585a 
…reaking (backport #958) (#1056)
@christophfroehlich christophfroehlich mentioned this pull request May 7, 2024
Merged
christophfroehlich added a commit that referenced this pull request May 9, 2024
@christophfroehlich
[diff_drive] REwrite test and add simple validation (partial backport #…
a724a45 
…958)
@mergify mergify bot mentioned this pull request May 9, 2024
Merged
@christophfroehlich christophfroehlich mentioned this pull request May 9, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bmagyar bmagyar bmagyar approved these changes

@aprotyas aprotyas Awaiting requested review from aprotyas

@arne48 arne48 Awaiting requested review from arne48

@DasRoteSkelett DasRoteSkelett Awaiting requested review from DasRoteSkelett

@destogl destogl Awaiting requested review from destogl

@jaron-l jaron-l Awaiting requested review from jaron-l

@peterdavidfagan peterdavidfagan Awaiting requested review from peterdavidfagan

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@christophfroehlich @bmagyar