[Feature] Fallback controllers #1789

saikishor · 2024-10-12T08:46:35Z

We already introduced the necessary information for setting up the fallback controllers via (#1499 and #1503). This PR aims to complete the Fallback controllers feature (When a controller is returning ERROR in the update cycle, it will be deactivated and any fallback controllers linked to this controller will be activated)

This PR has the main logic to have the fallback controllers working, pending some tests

Closes #1468

add fallback controllers list to the ControllerInterfaceBase do not activate the controller if it's fallback controllers are not found or not in inactive state add formatting changes added the first logic of switching to the fallback controller upon error Added a method to get the active controllers that use the command interfaces of the given controller Compute the complete list of active controllers that already use the command interfaces needed by the fallback controllers added fixes for the compilation retouch for the new structure

…k controllers

…es_of_controller method

codecov · 2024-10-13T20:34:17Z

Codecov Report

Attention: Patch coverage is 94.93671% with 28 lines in your changes missing coverage. Please review.

Project coverage is 87.29%. Comparing base (0ba1428) to head (5c2d2d5).
Report is 3 commits behind head on master.

Files with missing lines	Patch %	Lines
controller_manager/src/controller_manager.cpp	82.29%	5 Missing and 12 partials ⚠️
...ontroller_manager/test/test_controller_manager.cpp	97.57%	10 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1789      +/-   ##
==========================================
+ Coverage   86.83%   87.29%   +0.45%     
==========================================
  Files         121      121              
  Lines       11542    12093     +551     
  Branches     1054     1079      +25     
==========================================
+ Hits        10023    10557     +534     
- Misses       1144     1148       +4     
- Partials      375      388      +13

Flag	Coverage Δ
unittests	`87.29% <94.93%> (+0.45%)`	⬆️

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

Files with missing lines	Coverage Δ
.../include/controller_manager/controller_manager.hpp	`100.00% <100.00%> (ø)`
...r_manager/test/test_controller/test_controller.cpp	`95.55% <100.00%> (+0.20%)`	⬆️
...r_manager/test/test_controller/test_controller.hpp	`100.00% <ø> (ø)`
...ontroller_manager/test/test_controller_manager.cpp	`96.59% <97.57%> (+2.72%)`	⬆️
controller_manager/src/controller_manager.cpp	`77.70% <82.29%> (+1.02%)`	⬆️

... and 1 file with indirect coverage changes

…llerSpec

…r itself

…g interface to activate

…g state interface to activate

…ady active

…lready

controller_manager/include/controller_manager/controller_manager.hpp

controller_manager/src/controller_manager.cpp

bmagyar · 2024-10-16T12:10:36Z

controller_manager/src/controller_manager.cpp

+    std::string controllers_string;
+    controllers_string.reserve(500);


quite interesting that there isn't a pre-reserve constructor for strings...

Yes it sucks. I want to optimize all of these sooner in the following PRs

Can't we pre-reserve all of this?

Co-authored-by: Bence Magyar <[email protected]>

destogl

Looks good. We should proceed and then optimize the vector and string initialization part.

destogl · 2024-10-16T17:48:26Z

controller_manager/src/controller_manager.cpp

-    std::string failed_controllers;
+    const auto FALLBACK_STACK_MAX_SIZE = 500;
+    std::vector<std::string> active_controllers_using_interfaces(failed_controllers_list);
+    active_controllers_using_interfaces.reserve(FALLBACK_STACK_MAX_SIZE);


We are here in update, can we pre-reserve this?

This can be also done in a follow-up PR, but we should make sure that it is resolved.

destogl · 2024-10-16T18:16:25Z

controller_manager/src/controller_manager.cpp

+    std::string controllers_string;
+    controllers_string.reserve(500);


Can't we pre-reserve all of this?

destogl · 2024-10-16T18:27:39Z

Merging this as discussed with @bmagyar. We should then optimize the memory allocation

saikishor added 13 commits October 11, 2024 17:17

remove the unnecessary if condition

11b3b11

check that all command and state interfaces are available for fallbac…

df9f696

…k controllers

change the logic of the get_active_controllers_using_command_interfac…

825e591

…es_of_controller method

deactivate all related controller together

edcd56c

cleanup some redundant conditioning

e84fbdc

add more clear checks on the command interfaces of fallback controllers

73f03de

added a separate method to check fallback controllers state

6ecbec5

extend the checks for the needed state interfaces

c82303b

remove the newly added unused helper functions

f473278

Use references while iterating in loop

2f96856

update the logic in update to activate controllers altogether

d1daa53

fix the logic for existing tests

fbf9b68

saikishor added 10 commits October 13, 2024 22:39

add set_external_commands_for_testing method to the TestController class

bea7f4d

Add a add_controller method to controller_manager that accepts Contro…

9291406

…llerSpec

Add first basic fallback controller test

e2e622c

Add a test case for the controller cannot be a fallback controller fo…

e302358

…r itself

Added a fail test case when the fallback controllers need non existin…

8178ecd

…g interface to activate

Added a fail test case when the fallback controllers need non existin…

8a23238

…g state interface to activate

Added test to consider when one or more fallback controllers are alre…

01d21b1

…ady active

Improve logging for better introspection

c071a98

Add test on the fallback controllers need to be in configured state a…

0d4698e

…lready

Added test cases when using with chainable controllers

cf309a3

saikishor marked this pull request as ready for review October 14, 2024 21:47

github-actions bot requested review from bmagyar, christophfroehlich, destogl, erickisos and livanov93 October 14, 2024 21:47

github-actions bot requested review from mcbed and VanshGehlot October 14, 2024 21:47

added documentation and update release_note.rst

dae549c

bmagyar reviewed Oct 16, 2024

View reviewed changes

controller_manager/include/controller_manager/controller_manager.hpp Outdated Show resolved Hide resolved

bmagyar reviewed Oct 16, 2024

View reviewed changes

controller_manager/src/controller_manager.cpp Outdated Show resolved Hide resolved

bmagyar reviewed Oct 16, 2024

View reviewed changes

bmagyar previously approved these changes Oct 16, 2024

View reviewed changes

Apply suggestions from code review

17ec65f

Co-authored-by: Bence Magyar <[email protected]>

saikishor dismissed bmagyar’s stale review via 17ec65f October 16, 2024 12:45

Merge branch 'master' into feature/fallback_controllers

5c2d2d5

destogl approved these changes Oct 16, 2024

View reviewed changes

destogl merged commit 4eabd25 into ros-controls:master Oct 16, 2024
21 checks passed

saikishor deleted the feature/fallback_controllers branch October 16, 2024 21:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature] Fallback controllers #1789

[Feature] Fallback controllers #1789

saikishor commented Oct 12, 2024 •

edited by destogl

Loading

codecov bot commented Oct 13, 2024 •

edited

Loading

bmagyar Oct 16, 2024

saikishor Oct 16, 2024

destogl Oct 16, 2024

destogl left a comment

destogl Oct 16, 2024

destogl Oct 16, 2024

destogl commented Oct 16, 2024

		std::string controllers_string;
		controllers_string.reserve(500);

[Feature] Fallback controllers #1789

[Feature] Fallback controllers #1789

Conversation

saikishor commented Oct 12, 2024 • edited by destogl Loading

codecov bot commented Oct 13, 2024 • edited Loading

Codecov Report

bmagyar Oct 16, 2024

Choose a reason for hiding this comment

saikishor Oct 16, 2024

Choose a reason for hiding this comment

destogl Oct 16, 2024

Choose a reason for hiding this comment

destogl left a comment

Choose a reason for hiding this comment

destogl Oct 16, 2024

Choose a reason for hiding this comment

destogl Oct 16, 2024

Choose a reason for hiding this comment

destogl commented Oct 16, 2024

saikishor commented Oct 12, 2024 •

edited by destogl

Loading

codecov bot commented Oct 13, 2024 •

edited

Loading