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

[CM] Fix controller missing update cycles in a real setup (backport #1774) #1858

Merged
merged 1 commit into from
Nov 6, 2024

Conversation

mergify[bot]
Copy link
Contributor

@mergify mergify bot commented Nov 5, 2024

When we introduced supporting different update rates to the controllers. I realized that inside the tests I assumed a perfect system without any jitter and it was performing well.

time += rclcpp::Duration::from_seconds(0.01);

However, this is not the case in reality. We have some influence of system jitter, and due to the jitter if the system sleeps a bit less than what it should, then it skips and waits for the next cycle, and that's the reason we have double the period in most occasions.

rclcpp::Time old_time = time;
cm_->get_clock()->sleep_until(old_time + PERIOD);
time = cm_->get_clock()->now();

This PR introduces a new approach to solving this issue and the tests have been updated to be more realistic than earlier version

Fixes: #1769
Fixes: #1574


This is an automatic backport of pull request #1774 done by Mergify.

@mergify mergify bot added the conflicts label Nov 5, 2024
Copy link
Contributor Author

mergify bot commented Nov 5, 2024

Cherry-pick of 814137d has failed:

On branch mergify/bp/iron/pr-1774
Your branch is up to date with 'origin/iron'.

You are currently cherry-picking commit 814137d.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:
	modified:   controller_interface/include/controller_interface/controller_interface_base.hpp
	modified:   controller_manager/CMakeLists.txt
	modified:   controller_manager/include/controller_manager/controller_manager.hpp
	modified:   controller_manager/include/controller_manager/controller_spec.hpp

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   controller_manager/src/controller_manager.cpp
	both modified:   controller_manager/test/test_controller_manager.cpp

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

@saikishor saikishor force-pushed the mergify/bp/iron/pr-1774 branch 3 times, most recently from 755cb48 to e4c7127 Compare November 5, 2024 14:34
Copy link

codecov bot commented Nov 5, 2024

Codecov Report

Attention: Patch coverage is 65.85366% with 14 lines in your changes missing coverage. Please review.

Project coverage is 62.32%. Comparing base (7797a38) to head (dd2a1f3).
Report is 1 commits behind head on iron.

Files with missing lines Patch % Lines
controller_manager/src/controller_manager.cpp 59.09% 8 Missing and 1 partial ⚠️
...ontroller_manager/test/test_controller_manager.cpp 70.58% 2 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             iron    #1858      +/-   ##
==========================================
- Coverage   62.32%   62.32%   -0.01%     
==========================================
  Files         112      112              
  Lines       13108    13137      +29     
  Branches     8891     8909      +18     
==========================================
+ Hits         8170     8187      +17     
- Misses        951      963      +12     
  Partials     3987     3987              
Flag Coverage Δ
unittests 62.32% <65.85%> (-0.01%) ⬇️

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

Files with missing lines Coverage Δ
...controller_interface/controller_interface_base.hpp 83.33% <ø> (ø)
.../include/controller_manager/controller_manager.hpp 41.02% <100.00%> (ø)
...r_manager/test/test_controller/test_controller.cpp 84.44% <100.00%> (+0.35%) ⬆️
...r_manager/test/test_controller/test_controller.hpp 100.00% <ø> (ø)
...ontroller_manager/test/test_controller_manager.cpp 55.67% <70.58%> (+1.02%) ⬆️
controller_manager/src/controller_manager.cpp 70.27% <59.09%> (-0.42%) ⬇️

... and 2 files with indirect coverage changes

Copy link
Contributor

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

LGTM. Binary build failure is because of missing sync of realtime_tools

@christophfroehlich christophfroehlich merged commit e4d1450 into iron Nov 6, 2024
7 of 11 checks passed
@christophfroehlich christophfroehlich deleted the mergify/bp/iron/pr-1774 branch November 6, 2024 10:02
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