Reset moving average #1984

tonynajjar · 2025-01-07T12:20:28Z

See #1871 (comment)

Signed-off-by: Angsa Deployment Team <[email protected]>

Signed-off-by: Tony Najjar <[email protected]>

Co-authored-by: Christoph Fröhlich <[email protected]>

Signed-off-by: Tony Najjar <[email protected]>

Co-authored-by: Sai Kishor Kothakota <[email protected]>

Signed-off-by: Tony Najjar <[email protected]>

tonynajjar · 2025-01-07T12:21:28Z

controller_manager/src/controller_manager.cpp

@@ -2322,6 +2322,10 @@ std::vector<std::string> ControllerManager::get_controller_names()

 void ControllerManager::read(const rclcpp::Time & time, const rclcpp::Duration & period)
 {
+  if (periodicity_stats_.GetCount() >= 100)


the window size could be made configurable

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

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

…-average

codecov · 2025-01-07T12:39:27Z

Codecov Report

Attention: Patch coverage is 76.92308% with 3 lines in your changes missing coverage. Please review.

Project coverage is 89.25%. Comparing base (6e1059c) to head (b3ac890).
Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
controller_manager/src/controller_manager.cpp	76.92%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1984      +/-   ##
==========================================
- Coverage   89.26%   89.25%   -0.01%     
==========================================
  Files         130      130              
  Lines       14493    14505      +12     
  Branches     1257     1257              
==========================================
+ Hits        12937    12947      +10     
- Misses       1088     1090       +2     
  Partials      468      468

Flag	Coverage Δ
unittests	`89.25% <76.92%> (-0.01%)`	⬇️

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.20% <76.92%> (+<0.01%)`	⬆️

... and 1 file 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 · 2025-01-07T16:20:17Z

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 · 2025-01-07T16:28:02Z

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 · 2025-01-07T16:34:43Z

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

Signed-off-by: Tony Najjar <[email protected]>

christophfroehlich · 2025-01-09T20:19:19Z

I never used the MovingAverageStatistics, but how I understand the docstring

Note: reset() must be called manually in order to start a new measurement window.

and

Reset all calculated values. Equivalent to a new window for a moving average.

is, that if after window_size update steps there is basically no averaging but just the current value and then the number of samples for averaging increases with every update until the next window_size update steps is reached. This means that we don't have a constant horizon for the moving average but a "sawtooth" pattern for the horizon? Is this the desired behavior?
It just doesn't matter if it is way smaller than the diagnostics update period (which is 1s?)

saikishor · 2025-01-09T22:33:40Z

controller_manager/src/controller_manager_parameters.yaml

+          window_size: {
+            type: int,
+            default_value: -1,
+            description: "The number of samples to consider for the mean. The diagnostics will be computed over the last `window_size` samples. -1 means that all samples will be considered.",
+          }


Suggested change

window_size: {

type: int,

default_value: -1,

description: "The number of samples to consider for the mean. The diagnostics will be computed over the last `window_size` samples. -1 means that all samples will be considered.",

}

window_size: {

type: double,

default_value: .inf,

description: "The window size in seconds to consider for the mean. The diagnostics will be computed over the last `window_size` seconds. Default value infinity means that all data points will be considered.",

validation: {

gt<>: 0.0,

}

}

I believe the above validation works if you leave it with default value of infinity. I personally think it is more intuitive to the user to talk in seconds window rather than in the samples. What do you guys think?

saikishor · 2025-01-09T22:35:52Z

controller_manager/src/controller_manager_parameters.yaml

      controllers:
        periodicity:
+          window_size: {
+            type: int,
+            default_value: -1,
+            description: "The number of samples to consider for the mean. The diagnostics will be computed over the last `window_size` samples. -1 means that all samples will be considered.",
+          }
          mean_error:


controllers: window_size: periodicity: mean_error:

For the controllers, I believe this window_size should be a level higher, as it is being used for both periodicity and execution_time

saikishor · 2025-01-09T22:36:51Z

controller_manager/src/controller_manager.cpp

@@ -3347,6 +3365,7 @@ void ControllerManager::controller_manager_diagnostic_callback(
  const std::string periodicity_stat_name = "periodicity";
  const auto cm_stats = periodicity_stats_.GetStatistics();
  stat.add("update_rate", std::to_string(get_update_rate()));
+  stat.add(periodicity_stat_name + ".sample_count", std::to_string(cm_stats.sample_count));


I don't know if this is an intuitive diagnostic for the user, moreover it tend to be a constant rather than a varying value

saikishor · 2025-01-09T22:39:05Z

controller_manager/src/controller_manager.cpp

+            if (
+              loaded_controller.periodicity_statistics->GetCount() >=
+              params_->diagnostics.threshold.controllers.periodicity.window_size)
+            {
+              loaded_controller.periodicity_statistics->Reset();
+            }


Right now this logic will not work for default value -1 as the count will always be greater than window_size. Please check my recommendation to use the convention of seconds

Suggested change

if (

loaded_controller.periodicity_statistics->GetCount() >=

params_->diagnostics.threshold.controllers.periodicity.window_size)

{

loaded_controller.periodicity_statistics->Reset();

}

if ((std::finite(params_->diagnostics.threshold.controllers.window_size) &&

loaded_controller.periodicity_statistics->GetCount() >=

static_cast<unsigned int>(params_->diagnostics.threshold.controllers.window_size * update_rate_)))

{

loaded_controller.periodicity_statistics->Reset();

}

saikishor · 2025-01-09T22:39:51Z

controller_manager/src/controller_manager.cpp

+            if (
+              loaded_controller.execution_time_statistics->GetCount() >=
+              params_->diagnostics.threshold.controllers.periodicity.window_size)
+            {
+              loaded_controller.execution_time_statistics->Reset();
+            }


Same here, this logic doesn't work for the default option. Check the comments below

saikishor · 2025-01-09T22:40:16Z

controller_manager/src/controller_manager.cpp

+  if (
+    periodicity_stats_.GetCount() >=
+    params_->diagnostics.threshold.controller_manager.periodicity.window_size)
+  {
+    periodicity_stats_.Reset();
+  }


Same here, this logic doesn't work for the default option
See the below comments for suggestions

Angsa Deployment Team and others added 12 commits October 22, 2024 12:02

Add call timeout

63de32d

Signed-off-by: Angsa Deployment Team <[email protected]>

fix cherrypick

26c79d3

Signed-off-by: Angsa Deployment Team <[email protected]>

Merge branch 'master' into master

be335ed

Add docs

5735f0d

Signed-off-by: Tony Najjar <[email protected]>

Update controller_manager/doc/userdoc.rst

620e2ac

Co-authored-by: Christoph Fröhlich <[email protected]>

Merge remote-tracking branch 'ros2/master'

274880f

Signed-off-by: Tony Najjar <[email protected]>

fix order

a495547

Signed-off-by: Tony Najjar <[email protected]>

rename

b537260

Signed-off-by: Tony Najjar <[email protected]>

Merge branch 'master' into master

f512643

Update controller_manager/doc/userdoc.rst

22d383c

Co-authored-by: Sai Kishor Kothakota <[email protected]>

Merge branch 'ros-controls:master' into master

87ce781

Reset moving average

2c80244

Signed-off-by: Tony Najjar <[email protected]>

github-actions bot requested review from bmagyar, erickisos, fmauch, homalozoa, Maverobot, olivier-stasse and saikishor January 7, 2025 12:20

tonynajjar commented Jan 7, 2025

View reviewed changes

tonynajjar and others added 2 commits January 7, 2025 13:23

Merge branch 'ros-controls:master' into master

5135ccd

Merge remote-tracking branch 'angsa/master' into add-window-to-moving…

5023884

…-average

tonynajjar mentioned this pull request Jan 7, 2025

[Diagnostics] Add diagnostics of execution time and periodicity of the controllers and controller_manager #1871

Merged

bmagyar previously approved these changes Jan 7, 2025

View reviewed changes

saikishor reviewed Jan 7, 2025

View reviewed changes

Add window_size param

b3ac890

Signed-off-by: Tony Najjar <[email protected]>

tonynajjar dismissed bmagyar’s stale review via b3ac890 January 9, 2025 17:15

tonynajjar requested review from saikishor and bmagyar January 9, 2025 17:18

saikishor requested changes Jan 9, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reset moving average #1984

Reset moving average #1984

tonynajjar commented Jan 7, 2025 •

edited

Loading

tonynajjar Jan 7, 2025

bmagyar Jan 7, 2025

saikishor Jan 7, 2025

codecov bot commented Jan 7, 2025 •

edited

Loading

bmagyar left a comment

saikishor left a comment •

edited

Loading

saikishor commented Jan 7, 2025

tonynajjar commented Jan 7, 2025

saikishor commented Jan 7, 2025

christophfroehlich commented Jan 9, 2025 •

edited

Loading

saikishor Jan 9, 2025

saikishor Jan 9, 2025

saikishor Jan 9, 2025

saikishor Jan 9, 2025 •

edited

Loading

saikishor Jan 9, 2025

saikishor Jan 9, 2025

Reset moving average #1984

Are you sure you want to change the base?

Reset moving average #1984

Conversation

tonynajjar commented Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 7, 2025 • edited Loading

Codecov Report

bmagyar left a comment

Choose a reason for hiding this comment

saikishor left a comment • edited Loading

Choose a reason for hiding this comment

saikishor commented Jan 7, 2025

tonynajjar commented Jan 7, 2025

saikishor commented Jan 7, 2025

christophfroehlich commented Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saikishor Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonynajjar commented Jan 7, 2025 •

edited

Loading

codecov bot commented Jan 7, 2025 •

edited

Loading

saikishor left a comment •

edited

Loading

christophfroehlich commented Jan 9, 2025 •

edited

Loading

saikishor Jan 9, 2025 •

edited

Loading