Compute the actual update period for each controller and parse it

[saikishor]

Right now, the period is the desired controller update period, however, it make sense to have the actual update period in the update cycles of the controller, and the desired can be computed inside the controller always using the update_rate available through get_update_rate method.

This is changed as it is brought up at the ros2_control workshop

[christophfroehlich]

What is the idea behind the change? That the cm-update is jittering, or that controller's and cm's update rate don't have an integer divisor?

[christophfroehlich]

Shouldn't this be something like time-time_old_?

[saikishor]

The next_update_cycle_time is computed such that there is a condition that checks if the time is greater than or equal to the current time and if the condition passes, then it triggers the update. Usually, when it is equal this difference is zero, but that means you have a control period in between.

If not, I can check if I can move some logic around to see if I can avoid adding this controller period at this stage. I will check this evening.

[christophfroehlich]

Ok, maybe this is fine as it is.

[saikishor]

What is the idea behind the change? That the cm-update is jittering, or that controller's and cm's update rate don't have an integer divisor?

Not really, we already dealt with this in the previous PR. It's just the controller update method gets the desired period instead of getting an actual one. In case of the controller manager, it computes and parses it here:
https://github.com/ros-controls/ros2_control/blob/master/controller_manager/src/ros2_control_node.cpp#L76

I think it makes sense that the controller get's the actual one instead of the desired as this can always be computed with 1.0/update.

[christophfroehlich]

Not really, we already dealt with this in the previous PR. It's just the controller update method gets the desired period instead of getting an actual one. In case of the controller manager, it computes and parses it here: https://github.com/ros-controls/ros2_control/blob/master/controller_manager/src/ros2_control_node.cpp#L76

I think it makes sense that the controller get's the actual one instead of the desired as this can always be computed with 1.0/update.

But why would the desired not be the actual one? If it is not a perfect divisor, then we have some jitter. and if it the cm itself has not a constant update rate. But anyways, your fix should deal with both cases.

[saikishor]

But why would the desired not be the actual one? If it is not a perfect divisor, then we have some jitter. and if it the cm itself does not have a constant update rate. But anyways, your fix should deal with both cases.

You are right. It is because of jitter in some cases and in the non-perfect divisor cases, it will be a bit different per cycle but as in the test it should be within some bounds

[codecov]

Codecov Report

Merging #1140 (5fc79f8) into master (2056d6c) will increase coverage by 0.06%.
Report is 2 commits behind head on master.
The diff coverage is 0.00%.

❗ Current head 5fc79f8 differs from pull request most recent head fd0b617. Consider uploading reports for the commit fd0b617 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1140      +/-   ##
==========================================
+ Coverage   47.61%   47.68%   +0.06%     
==========================================
  Files          40       40              
  Lines        3442     3437       -5     
  Branches     1865     1861       -4     
==========================================
  Hits         1639     1639              
- Misses        479      480       +1     
+ Partials     1324     1318       -6     

Flag	Coverage Δ
unittests	47.68% <0.00%> (+0.06%)	⬆️

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

Files	Coverage Δ
controller_manager/src/controller_manager.cpp	38.80% <0.00%> (+0.16%)	⬆️

[bmagyar]

Let's give this some time in rolling before backporting to Iron, hope that's ok