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.

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

Fix RealtimeBox API changes #1385

Merged
merged 8 commits into from
Dec 4, 2024
Merged

Fix RealtimeBox API changes #1385

merged 8 commits into from
Dec 4, 2024

Conversation

christophfroehlich
Copy link
Contributor

@christophfroehlich christophfroehlich added the backport-humble This label should be used by maintainers only! Label triggers PR backport to ROS2 humble. label Nov 23, 2024
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.

This PR swaps out 2 controllers' main input to use the stamped version without any deprecation. There is very little actual alignment with the realtime_tools API that I can see. Would it be possible to separate these onto 2 PRs?

@christophfroehlich
Copy link
Contributor Author

No, that is not true: There is no change! I was just puzzled by this alias and renamed it:
using Twist = geometry_msgs::msg::TwistStamped;
I can move that to a follow-up PR if you like to.

@bmagyar
Copy link
Member

bmagyar commented Nov 26, 2024

Yes please and I stand corrected 🫡

Copy link
Contributor

mergify bot commented Nov 27, 2024

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

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.

thank you!

Copy link

codecov bot commented Dec 2, 2024

Codecov Report

Attention: Patch coverage is 72.72727% with 6 lines in your changes missing coverage. Please review.

Project coverage is 83.57%. Comparing base (36068e1) to head (0ad818a).

Files with missing lines Patch % Lines
...iff_drive_controller/src/diff_drive_controller.cpp 72.72% 2 Missing and 1 partial ⚠️
tricycle_controller/src/tricycle_controller.cpp 72.72% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1385      +/-   ##
==========================================
+ Coverage   83.56%   83.57%   +0.01%     
==========================================
  Files         122      122              
  Lines       10992    10993       +1     
  Branches      937      936       -1     
==========================================
+ Hits         9185     9187       +2     
  Misses       1493     1493              
+ Partials      314      313       -1     
Flag Coverage Δ
unittests 83.57% <72.72%> (+0.01%) ⬆️

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

Files with missing lines Coverage Δ
...de/diff_drive_controller/diff_drive_controller.hpp 100.00% <ø> (ø)
...ive_controller/test/test_diff_drive_controller.cpp 93.60% <ø> (ø)
...nclude/tricycle_controller/tricycle_controller.hpp 100.00% <ø> (ø)
...cycle_controller/test/test_tricycle_controller.cpp 89.32% <ø> (ø)
...iff_drive_controller/src/diff_drive_controller.cpp 76.51% <72.72%> (+0.08%) ⬆️
tricycle_controller/src/tricycle_controller.cpp 66.26% <72.72%> (+0.13%) ⬆️

... and 3 files with indirect coverage changes

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.

I'm curious about the effect of the try_get we might have

diff_drive_controller/src/diff_drive_controller.cpp Outdated Show resolved Hide resolved
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.

May be it is better to set the last_command_msg_ to empty in the configure. Same goes for the test changes

diff_drive_controller/src/diff_drive_controller.cpp Outdated Show resolved Hide resolved
diff_drive_controller/src/diff_drive_controller.cpp Outdated Show resolved Hide resolved
@christophfroehlich
Copy link
Contributor Author

May be it is better to set the last_command_msg_ to empty in the configure. Same goes for the test changes

We don't use try_get in the tests?

@saikishor
Copy link
Member

We don't use try_get in the tests?

Sure it is not needed

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.

Looks spot on!

Thanks a lot for fixing things :)

@christophfroehlich christophfroehlich removed the backport-humble This label should be used by maintainers only! Label triggers PR backport to ROS2 humble. label Dec 4, 2024
@christophfroehlich christophfroehlich merged commit f9edc41 into master Dec 4, 2024
10 of 18 checks passed
@christophfroehlich christophfroehlich deleted the fix/realtimebox branch December 4, 2024 10:16
@christophfroehlich
Copy link
Contributor Author

So we don't need to backport this after the branch-off of realtime_tools right?

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.

3 participants