Fix equal and higher ctrl update rate #1070

saikishor · 2023-07-01T19:31:20Z

Could it be that we have an issue with controller update rate? I get suddenly all controllers running at 1 Hz when setting them to the same frequency as controller manager. And the code (as mentioned in the review already) doesn't make sense to me. So heads up if you are using this parameter - it might be broken on rolling, iron and humble.

This fixes the issue reported by @destogl over the slack. This also address the issue with controllers set to higher update rate than the controller manager

…rates w.r.t controller manager

saikishor · 2023-07-01T21:05:19Z

The failing test shown below is not related to the changes in this PR

  9: [ RUN      ] TestLoadController.spawner_test_type_in_param
  9: [WARN] [1688244547.227063765] [test_controller_manager]: 'update_rate' parameter not set, using default value.
  9: [INFO] [1688244547.227804187] [test_controller_manager]: Subscribing to '/test_controller_manager/robot_description' topic for robot description.
  9: [INFO] [1688244547.231742802] [test_controller_manager]: Received robot description from topic.
  9: [INFO] [1688244547.232021811] [resource_manager]: Loading hardware 'TestActuatorHardware' 
  9: [INFO] [1688244547.232927537] [resource_manager]: Initialize hardware 'TestActuatorHardware' 
  9: [INFO] [1688244547.233221646] [resource_manager]: Successful initialization of hardware 'TestActuatorHardware'
  9: [INFO] [1688244547.233468953] [resource_manager]: Loading hardware 'TestSensorHardware' 
  9: [INFO] [1688244547.233707760] [resource_manager]: Initialize hardware 'TestSensorHardware' 
  9: [INFO] [1688244547.233848264] [resource_manager]: Successful initialization of hardware 'TestSensorHardware'
  9: [INFO] [1688244547.233992869] [resource_manager]: Loading hardware 'TestSystemHardware' 
  9: [INFO] [1688244547.234227875] [resource_manager]: Initialize hardware 'TestSystemHardware' 
  9: [INFO] [1688244547.234474783] [resource_manager]: Successful initialization of hardware 'TestSystemHardware'
  9: [INFO] [1688244547.234693589] [resource_manager]: 'configure' hardware 'TestSystemHardware' 
  9: [INFO] [1688244547.234812093] [resource_manager]: Successful 'configure' of hardware 'TestSystemHardware'
  9: [INFO] [1688244547.234904595] [resource_manager]: 'activate' hardware 'TestSystemHardware' 
  9: [INFO] [1688244547.234985098] [resource_manager]: Successful 'activate' of hardware 'TestSystemHardware'
  9: [INFO] [1688244547.235101101] [resource_manager]: 'configure' hardware 'TestSensorHardware' 
  9: [INFO] [1688244547.235195004] [resource_manager]: Successful 'configure' of hardware 'TestSensorHardware'
  9: [INFO] [1688244547.235290707] [resource_manager]: 'activate' hardware 'TestSensorHardware' 
  9: [INFO] [1688244547.235368209] [resource_manager]: Successful 'activate' of hardware 'TestSensorHardware'
  9: [INFO] [1688244547.235453311] [resource_manager]: 'configure' hardware 'TestActuatorHardware' 
  9: [INFO] [1688244547.235522013] [resource_manager]: Successful 'configure' of hardware 'TestActuatorHardware'
  9: [INFO] [1688244547.235633417] [resource_manager]: 'activate' hardware 'TestActuatorHardware' 
  9: [INFO] [1688244547.235731120] [resource_manager]: Successful 'activate' of hardware 'TestActuatorHardware'
  9: [INFO] [1688244548.174873910] [test_controller_manager]: Loading controller 'ctrl_1'
  9: [INFO] [1688244548.187106770] [spawner_ctrl_1]: Loaded ctrl_1
  9: [INFO] [1688244548.188393707] [test_controller_manager]: Configuring controller 'ctrl_1'
  9: [INFO] [1688244548.207450567] [spawner_ctrl_1]: Configured and activated ctrl_1
  9: [WARN] [1688244549.049642610] [spawner_ctrl_1]: Controller already loaded, skipping load_controller
  9: [INFO] [1688244549.050678540] [test_controller_manager]: Configuring controller 'ctrl_1'
  Error: OR] [1688244549.050746542] [test_controller_manager]: Controller 'ctrl_1' can not be configured from 'active' state.
  Error: OR] [1688244549.051494764] [spawner_ctrl_1]: Failed to configure controller
  9: [ros2run]: Process exited with failure 1
   9/10 Test  #9: test_spawner_unspawner ..............................***Timeout  60.00 sec

codecov-commenter · 2023-07-01T21:19:27Z

Codecov Report

Merging #1070 (9eb95d0) into master (925f5f3) will decrease coverage by 2.31%.
The diff coverage is 34.91%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master    #1070      +/-   ##
==========================================
- Coverage   34.61%   32.31%   -2.31%     
==========================================
  Files          52       93      +41     
  Lines        2981     9865    +6884     
  Branches     1855     6651    +4796     
==========================================
+ Hits         1032     3188    +2156     
- Misses        310      783     +473     
- Partials     1639     5894    +4255

Flag	Coverage Δ
unittests	`32.31% <34.91%> (-2.31%)`	⬇️

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

Impacted Files	Coverage Δ
controller_manager/src/controller_manager.cpp	`38.15% <ø> (-1.56%)`	⬇️
controller_manager/src/ros2_control_node.cpp	`0.00% <0.00%> (ø)`
..._interface/include/hardware_interface/actuator.hpp	`100.00% <ø> (ø)`
...ce/include/hardware_interface/async_components.hpp	`0.00% <0.00%> (ø)`
...re_interface/include/hardware_interface/sensor.hpp	`100.00% <ø> (ø)`
...re_interface/include/hardware_interface/system.hpp	`100.00% <ø> (ø)`
hardware_interface/src/resource_manager.cpp	`47.32% <ø> (-6.30%)`	⬇️
hardware_interface/src/sensor.cpp	`50.52% <ø> (ø)`
hardware_interface/src/system.cpp	`55.45% <ø> (ø)`
...rface/test/mock_components/test_generic_system.cpp	`9.42% <ø> (ø)`
... and 70 more

... and 19 files with indirect coverage changes

saikishor · 2023-07-03T08:22:57Z

Do you think it is better to have a warning printed in case the controller's update_rate is higher than the CM's update_rate?

bmagyar · 2023-07-03T08:43:59Z

Absolutely! Expectation management is important

saikishor · 2023-07-03T18:47:30Z

Done

saikishor · 2023-07-03T19:07:52Z

@bmagyar I'm not sure that my new commit has caused this.
The test did not generate a result file.

bmagyar · 2023-07-03T19:35:23Z

The spawner tests are sometimes flaky. I've restarted them.

saikishor · 2023-07-03T19:53:17Z

Perfect! Now they passed hahaha

controller_manager/src/controller_manager.cpp

bmagyar · 2023-07-03T21:45:53Z

controller_manager/test/test_controller_manager.cpp

+  auto test_controller = std::make_shared<test_controller::TestController>();
+
+  auto last_internal_counter = 0u;
+  for (unsigned int ctrl_update_rate : {100, 232, 400})


I'll do a small refactor here tomorrow, otherwise we are good to go IMO

olivier-stasse

As the controller_update_rate <= cm_update_rate, LGTM

bmagyar

Thank you!

(cherry picked from commit d39ddcd) # Conflicts: # controller_manager/src/controller_manager.cpp

github-actions bot requested review from arne48, bijoua29, bmagyar, destogl, LukasMacha97, malapatiravi and rosterloh July 1, 2023 19:31

saikishor force-pushed the fix_equal_and_higher_ctrl_update_rate branch from a622517 to 936b602 Compare July 1, 2023 20:39

Added test case to test the controllers with equal and higher update …

bb035a5

…rates w.r.t controller manager

saikishor force-pushed the fix_equal_and_higher_ctrl_update_rate branch from 936b602 to 7da192b Compare July 1, 2023 20:43

Added the fix to solve inconsistency w.r.t controller update rate

2ca3c7b

saikishor force-pushed the fix_equal_and_higher_ctrl_update_rate branch from 7da192b to 2ca3c7b Compare July 1, 2023 21:04

added logging for expectation management

f3c1e8e

bmagyar reviewed Jul 3, 2023

View reviewed changes

controller_manager/src/controller_manager.cpp Show resolved Hide resolved

bmagyar reviewed Jul 3, 2023

View reviewed changes

olivier-stasse approved these changes Jul 4, 2023

View reviewed changes

bmagyar added 3 commits July 4, 2023 19:51

Clean up some compilation warnings

8a2021e

Refactor update rate tests to be parametrized on update rate

505743a

reformat

9eb95d0

bmagyar approved these changes Jul 4, 2023

View reviewed changes

bmagyar added the backport-humble This label should be used by maintainers only! Label triggers PR backport to ROS2 humble. label Jul 4, 2023

bmagyar merged commit d39ddcd into ros-controls:master Jul 4, 2023
1 check passed

mergify bot pushed a commit that referenced this pull request Jul 4, 2023

Fix equal and higher controller update rate (#1070)

9cd3e9b

(cherry picked from commit d39ddcd) # Conflicts: # controller_manager/src/controller_manager.cpp

mergify bot mentioned this pull request Jul 4, 2023

Fix equal and higher ctrl update rate (backport #1070) #1071

Merged

bmagyar pushed a commit that referenced this pull request Jul 5, 2023

Fix equal and higher controller update rate (backport #1070) (#1071)

133f442

saikishor deleted the fix_equal_and_higher_ctrl_update_rate branch August 17, 2024 08:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix equal and higher ctrl update rate #1070

Fix equal and higher ctrl update rate #1070

saikishor commented Jul 1, 2023

saikishor commented Jul 1, 2023

codecov-commenter commented Jul 1, 2023 •

edited

Loading

saikishor commented Jul 3, 2023

bmagyar commented Jul 3, 2023

saikishor commented Jul 3, 2023

saikishor commented Jul 3, 2023

bmagyar commented Jul 3, 2023

saikishor commented Jul 3, 2023

bmagyar Jul 3, 2023

saikishor Jul 3, 2023

olivier-stasse left a comment

bmagyar left a comment

Fix equal and higher ctrl update rate #1070

Fix equal and higher ctrl update rate #1070

Conversation

saikishor commented Jul 1, 2023

saikishor commented Jul 1, 2023

codecov-commenter commented Jul 1, 2023 • edited Loading

Codecov Report

saikishor commented Jul 3, 2023

bmagyar commented Jul 3, 2023

saikishor commented Jul 3, 2023

saikishor commented Jul 3, 2023

bmagyar commented Jul 3, 2023

saikishor commented Jul 3, 2023

bmagyar Jul 3, 2023

Choose a reason for hiding this comment

saikishor Jul 3, 2023

Choose a reason for hiding this comment

olivier-stasse left a comment

Choose a reason for hiding this comment

bmagyar left a comment

Choose a reason for hiding this comment

codecov-commenter commented Jul 1, 2023 •

edited

Loading