Reset moving average

[tonynajjar]

See #1871 (comment)

[tonynajjar]

the window size could be made configurable

[bmagyar]

good point. Could you please add a param for that? I think 100 is a good default value for resetting

[saikishor]

I think it is better to leave it to the user to define it. If not defined, let it accumulate from the beginning.

[codecov]

Codecov Report

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

Project coverage is 89.16%. Comparing base (6e1059c) to head (5023884).

Files with missing lines	Patch %	Lines
controller_manager/src/controller_manager.cpp	66.66%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1984      +/-   ##
==========================================
- Coverage   89.26%   89.16%   -0.10%     
==========================================
  Files         130      130              
  Lines       14493    14501       +8     
  Branches     1257     1258       +1     
==========================================
- Hits        12937    12930       -7     
- Misses       1088     1098      +10     
- Partials      468      473       +5     

Flag	Coverage Δ
unittests	89.16% <66.66%> (-0.10%)	⬇️

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

Files with missing lines	Coverage Δ
controller_manager/src/controller_manager.cpp	76.18% <66.66%> (-0.03%)	⬇️

... and 4 files with indirect coverage changes

[bmagyar]

Thank you, this is pretty useful for diagnosing edge cases

[saikishor]

I'm curious to know why your read cycle happened in 0.00001615 sec (61919,5044 Hz)? This doesn't make sense at all in my opinion. Can you share some information regarding your setup. That's the reason that it is reporting the issue.

For the first sample to be a valid one, this change was introduced. We have tests running in the CI with around 10-20% margin as it doesn't have a RT patch

ros2_control/controller_manager/src/ros2_control_node.cpp

Line 128 in 6e1059c

rclcpp::Time previous_time = cm->now() - period;

[saikishor]

I would suggest to take a look at your setup and understand why your min and max periodicity rates are bad. In my opinion, window option is just to avoid the ERROR in diagnostics but not doesn't solve the underlying issue. Moreover, with the current approach, the user is always aware of the worst case scenario after the testing (through min and max values) and doesn't need to rush to take a look at those peaks

[tonynajjar]

In my opinion, window option is just to avoid the ERROR in diagnostics but not doesn't solve the underlying issue.

I fully agree, I just did not want to go through the pain of debugging why only one of the cycles is executed super quickly. I printed out the measurements and it's really just one that is "problematic". It really doesn't affect my application at all and it will be hard to debug. I suspect it's something to do with a CPU/Load spike of my system at startup.

[saikishor]

I fully agree, I just did not want to go through the pain of debugging why only one of the cycles is executed super quickly. I printed out the measurements and it's really just one that is "problematic". It really doesn't affect my application at all and it will be hard to debug. I suspect it's something to do with a CPU/Load spike of my system at startup.

Then we will have to check with #1918, after this is merged. My idea is to add this info into it, so you can diagnose it at run time.

Regarding the CPU/Load spike of my system at startup, it could cause the minimum periodicity, but the max might be coming from the first iteration