refactor SwitchParams to fix the incosistencies in the spawner tests

[saikishor]

This PR aims to fix the inconsistencies that are observed with the spawner_unspawner tests and could be reproducible at higher frequencies as well. It seems to be that due to the wall_timer etc the manager_switch was trigger twice and causing the crashes, and upon checking there are missing guards and mutexes to protect this context.

The changes are also tested on the real hardware! :)

The following failures are caused with the current setup and is solved with the changes proposed in this PR:

1: [WARN] [1722085254.107041328] [spawner_ctrl_1]: Controller already loaded, skipping load_controller
1: [INFO] [1722085254.107959063] [test_controller_manager]: Configuring controller 'ctrl_1'
1: [WARN] [1722085254.125499381] [rcl_lifecycle]: No transition matching 3 found for current state activating
1: [ERROR] [1722085254.125529455] [ctrl_1]: Unable to start transition 3 from current state activating: Transition is not registered., at ./src/rcl_lifecycle.c:355
1: [ERROR] [1722085254.125577475] [test_controller_manager]: After activation, controller 'ctrl_1' is in state 'activating' (13), expected 'active' (3).
1: [INFO] [1722085254.136365765] [spawner_ctrl_1]: Configured and activated ctrl_1
1: [INFO] [1722085254.308361830] [test_controller_manager]: Loading controller 'ctrl_1'

1: [INFO] [1721556773.163766473] [test_controller_manager]: Configuring controller 'ctrl_3'
1: terminate called after throwing an instance of 'std::runtime_error'
1:   what():  Can not get command interface configuration until the controller is configured. The current state is : activating and ID : 13 and ACTIVE is : 3 and INACTIVE IS : 2
1: Stack trace (most recent call last) in thread 112351:

1: [INFO] [1721560930.460608458] [test_controller_manager]: Configuring controller 'ctrl_3'
1: [WARN] [1721560930.479481009] [rcl_lifecycle]: No transition matching 3 found for current state activating
1: [ERROR] [1721560930.479493504] [ctrl_1]: Unable to start transition 3 from current state activating: Transition is not registered., at ./src/rcl_lifecycle.c:355
1: [ERROR] [1721560930.479538729] [test_controller_manager]: After activation, controller 'ctrl_1' is in state 'activating' (13), expected 'active' (3).
1: terminate called after throwing an instance of 'std::runtime_error'
1:   what():  Can not get command interface configuration until the controller is configured. The current state is : activating and ID : 13 and ACTIVE is : 3 and INACTIVE IS : 2
1: Stack trace (most recent call last) in thread 227964:
1: #21   Object "", at 0xffffffffffffffff, in 
1: [INFO] [1721560930.490672863] [spawner_ctrl_1]: Configured and activated all the parsed controllers list!
1: #20   Source "../sysdeps/unix/sysv/linux/x86_64/clone3.S", line 81, in __clone3 [0x7fe73cdf584f]

1: [INFO] [1721716958.916360995] [test_controller_manager]: Loading controller 'ctrl_1'
1: [INFO] [1721716958.935490861] [spawner_ctrl_1]: Loaded ctrl_1
1: [INFO] [1721716958.936473516] [test_controller_manager]: Configuring controller 'ctrl_1'
1: [INFO] [1721716958.943142165] [spawner_ctrl_1]: Configured ctrl_1 successfully
1: [INFO] [1721716958.943535316] [spawner_ctrl_1]: Activating ctrl_1
1: [INFO] [1721716958.944450162] [test_controller_manager]: Found controller 'ctrl_1' that needs to be activateed in list of controllers. Adding to the list!
1: [INFO] [1721716958.952596909] [test_controller_manager]: Entering manager switch and before activate controllers. 3720000000
1: [INFO] [1721716958.952633229] [test_controller_manager]: Entering manager switch and before activate controllers. 3720000000
1: [INFO] [1721716958.952635535] [test_controller_manager]: Activating controllers: ctrl_1, 
1: [INFO] [1721716958.952672064] [test_controller_manager]: Activating controllers: ctrl_1, 
1: [INFO] [1721716958.952687694] [test_controller_manager]: Activating controller 'ctrl_1' with current state : inactive
1: [INFO] [1721716958.952703837] [test_controller_manager]: Activating controller 'ctrl_1' with current state : inactive
1: [INFO] [1721716958.952738683] [test_controller_manager]: Switching done : 0 at 3720000000
1: [ERROR] [1721716958.952773682] [test_controller_manager]: Could not extract state interfaces for controller 'ctrl_1': Can not get state interface configuration until the controller is configured. The current state is : activating and ID : 13 and ACTIVE is : 3 and INACTIVE IS : 2
1: terminate called after throwing an instance of 'std::runtime_error'
1:   what():  Can not get state interface configuration until the controller is configured. The current state is : activating and ID : 13 and ACTIVE is : 3 and INACTIVE IS : 2
1: [INFO] [1721716958.952890889] [test_controller_manager]: Successfully switched controllers. Activated: ctrl_1, 
1: Stack trace (most recent call last) in thread 60496:
1: #20   Object "", at 0xffffffffffffffff, in 
1: [INFO] [1721716958.963346793] [spawner_ctrl_1]: Configured and activated ctrl_1
1: [INFO] [1721716958.964605158] [test_controller_manager]: Loading controller 'ctrl_3'
1: [INFO] [1721716958.973829860] [spawner_ctrl_1]: Loaded ctrl_3

Fixes: #1637
Fixes: #1368
Fixes: #1647
Fixes: #1657
Fixes: #1644
Fixes: ros-controls/ros2_control_ci#105
Fixes: ros-controls/ros2_control_ci#106
Fixes: ros-controls/ros2_control_ci#107
Fixes: ros-controls/ros2_control_ci#108

[codecov]

Codecov Report

Attention: Patch coverage is 84.61538% with 4 lines in your changes missing coverage. Please review.

Project coverage is 88.04%. Comparing base (5cd1a43) to head (1998266).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1638      +/-   ##
==========================================
+ Coverage   88.03%   88.04%   +0.01%     
==========================================
  Files         108      108              
  Lines       10006    10024      +18     
  Branches      891      892       +1     
==========================================
+ Hits         8809     8826      +17     
- Misses        877      878       +1     
  Partials      320      320              

Flag	Coverage Δ
unittests	88.04% <84.61%> (+0.01%)	⬆️

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

Files	Coverage Δ
.../include/controller_manager/controller_manager.hpp	100.00% <100.00%> (ø)
...ller_manager/test/test_controller_manager_srvs.cpp	99.25% <100.00%> (+<0.01%)	⬆️
controller_manager/src/controller_manager.cpp	77.29% <73.33%> (+0.07%)	⬆️

[christophfroehlich]

Thanks for addressing these issues!
Changes look fine, but I'm not sufficiently proficient in reviewing the multi-thread stuff.

The CI failures happen back to humble, can we backport this PR?
ABI breaks might not be that critical because this is an internal class only?

This PR reduces coverage because of the untested fallback branches. Could we add any tests there?

[saikishor]

The CI failures happen back to humble, can we backport this PR?
ABI breaks might not be that critical because this is an internal class only?

I believe yes! This can be backported without any issues as you said it is only an ABI change

[bmagyar]

A pretty elegant solution, I'm only questioning the reset() function part

[destogl]

I find this very good. Just to clarify, I am not sure how this solves multiple calls from the spawner and unspawner. Should't they be already blocked at the service callback level?

[saikishor]

I find this very good. Just to clarify, I am not sure how this solves multiple calls from the spawner and unspawner. Should't they be already blocked at the service callback level?

Hello @destogl!

The problem is not in the spawner per se, the problem in the wall timers that we use inside the tests and other places. The wall timers are sometimes triggering the same callback twice, and hence the manage_switch is being triggered twice and then most of the issues occur due to this coupled triggers. This means the activate/deactivate are being called simultaneously and thereby causing problems. It took me like a week of debugging to understand the root cause XD. Ideally, it shouldn't happen in real usecase.