[CM] Fix controller missing update cycles in a real setup

[saikishor]

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.

ros2_control/controller_manager/test/test_controller_manager.cpp

Line 512 in ab84b74

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.

ros2_control/controller_manager/test/test_controller_manager.cpp

Lines 502 to 504 in 3c985db

	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

[codecov]

Codecov Report

Attention: Patch coverage is 97.72727% with 1 line in your changes missing coverage. Please review.

Project coverage is 87.60%. Comparing base (d55def1) to head (69095e5).

Files with missing lines	Patch %	Lines
...ontroller_manager/test/test_controller_manager.cpp	97.05%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1774      +/-   ##
==========================================
- Coverage   87.62%   87.60%   -0.03%     
==========================================
  Files         120      120              
  Lines       12217    12227      +10     
  Branches     1093     1093              
==========================================
+ Hits        10705    10711       +6     
- Misses       1123     1126       +3     
- Partials      389      390       +1     

Flag	Coverage Δ
unittests	87.60% <97.72%> (-0.03%)	⬇️

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	92.30% <ø> (ø)
.../include/controller_manager/controller_manager.hpp	100.00% <100.00%> (ø)
controller_manager/src/controller_manager.cpp	77.98% <100.00%> (-0.19%)	⬇️
...ontroller_manager/test/test_controller_manager.cpp	96.25% <97.05%> (-0.11%)	⬇️

[fmauch]

This does absolutely make sense and I can confirm that this solves the problem I had in #1769. With this PR I get:

Thanks a lot, @saikishor

[christophfroehlich]

Sounds reasonable for me

[fmauch]

One comment I forgot earlier: It might be good to add an API doc change to this PR:

ros2_control/controller_interface/include/controller_interface/controller_interface_base.hpp

Line 150 in ab84b74

* \param[in] period The measured time taken by the last control loop iteration

Maybe that should be changed to

* \param[in] period The measured time since this method was called last

[christophfroehlich]

Thanks for the explanation

[bijoua29]

Funny. I just ran into this issue with my controller in the last day. I had a controller manager and controller running at 100 Hz. And due to some jitter in one of the first samples, it was calling my controller at 100 Hz but passing a period of 20 ms continuously. I hacked a quick workaround which fixed the issue but this approach works too. Thanks for fixing this @saikishor .

My only concern is that previously we were tracking the next update time based on an absolute time reference at the beginning. With this change, we are determining the next update time based on a moving time reference i.e. the last update time. Maybe, it doesn't matter in practical terms but I feel the previous version was "more" correct in that respect.

[saikishor]

My only concern is that previously we were tracking the next update time based on an absolute time reference at the beginning. With this change, we are determining the next update time based on a moving time reference i.e. the last update time. Maybe, it doesn't matter in practical terms but I feel the previous version was "more" correct in that respect.

@bijoua29 Even though the previous one might feel right, in reality, due to the system jitter it only creates issues in the long run. Conceptually, the proposed method is more robust to the system jitter and functionally better

[bijoua29]

My only concern is that previously we were tracking the next update time based on an absolute time reference at the beginning. With this change, we are determining the next update time based on a moving time reference i.e. the last update time. Maybe, it doesn't matter in practical terms but I feel the previous version was "more" correct in that respect.

@bijoua29 Even though the previous one might feel right, in reality, due to the system jitter it only creates issues in the long run. Conceptually, the proposed method is more robust to the system jitter and functionally better

@saikishor I think you can track both next update time to make it a fixed frrequency without jitter in addition to the last update time fix you added to account for current time jitter and setting the period correctly. It does mean you have to track two variables. But if you think this is sufficient, that is fine.

[mergify]

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

[mergify]

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

[fmauch]

We could probably also add #1574 to the linked issues of this PR.

[saikishor]

We could probably also add #1574 to the linked issues of this PR.

Done. Thank you :)