Fix controller parameter loading issue in different cases

[saikishor]

Lately, we have had reports about issues with loading parameters by parsing the param file to the spawners. This didn't work sometimes because we were trying to initialize the GPL library in the init. When it tries to declare and get the parameters, as they are not available, it throws an exception as the validation sometimes fails (as in ros-controls/ros2_controllers#698 and ros-controls/ros2_controllers#795) (or) that we couldn't also use the read_only parameters in this case, and they need to exist upon the LifeCycleNode creation (ros-controls/ros2_controllers#966).

Upon taking a close look at the documentation, I think we can use the following approach to be able to properly set the parameters to the Node.

The following approach can be backported, however, in some cases that the controllers needs to override their NodeOptions for their particular use-case (as in some tests), this won't work as the default argument from the controller_interface_base.hpp will be overridden by the CM now. Inorder to properly fix this, we will need a part of this PR : #1169

Fixes: #1311
Fixes: ros-controls/ros2_controllers#966
Closes: ros-controls/ros2_controllers#795
Closes: PickNikRobotics/generate_parameter_library#156

[codecov]

Codecov Report

Attention: 24 lines in your changes are missing coverage. Please review.

Comparison is base (71506d5) 47.87% compared to head (604c0eb) 47.68%.
Report is 14 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1293      +/-   ##
==========================================
- Coverage   47.87%   47.68%   -0.19%     
==========================================
  Files          41       41              
  Lines        3453     3477      +24     
  Branches     1878     1896      +18     
==========================================
+ Hits         1653     1658       +5     
- Misses        444      453       +9     
- Partials     1356     1366      +10     

Flag	Coverage Δ
unittests	47.68% <17.24%> (-0.19%)	⬇️

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

Files	Coverage Δ
.../include/controller_manager/controller_manager.hpp	18.18% <ø> (ø)
controller_manager/src/controller_manager.cpp	38.79% <17.24%> (-0.37%)	⬇️

[christophfroehlich]

Looks very good to me :)

[christophfroehlich]

Does it make a difference now if the ParamListener of GPL is created in the on_init or on_configure method? Do we have a clear solution for this PR?

[saikishor]

Does it make a difference now if the ParamListener of GPL is created in the on_init or on_configure method? Do we have a clear solution for this PR?

Thanks for bringing it up. Nope, it doesn't make any difference at all. You can have it created anywhere in on_init or on_configure method. I believe this is exactly what we want.

[Noel215]

Hi,

I've reverted ros-controls/ros2_controllers#967 locally and backported the main functionality of this PR to humble (I needed to adapt it) in order to test it and it works for me. It solves ros-controls/ros2_controllers#966.

[christophfroehlich]

Does it make a difference now if the ParamListener of GPL is created in the on_init or on_configure method? Do we have a clear solution for this PR?

Thanks for bringing it up. Nope, it doesn't make any difference at all. You can have it created anywhere in on_init or on_configure method. I believe this is exactly what we want.

fine. do we have a favorite in doing it in on_init or on_configure? Not sure if it is implemented in the same way with all controllers.

[saikishor]

fine. do we have a favorite in doing it in on_init or on_configure? Not sure if it is implemented in the same way with all controllers.

@christophfroehlich I would favor the on_init, because in that way you still can change the parameters after loading the controller and then configure them. This is something that you brought up in some conversation, as this is very interesting to have.

[destogl]

I generally agree with this, but I would suggest that we don't backport this but create separate PRs for humble and rolling. This one would be for humble and the breaking one for rolling.

Oh and BTW, don't we already have something like PIMPL, where controllers have only on_init. In that case, we should make init not overridable from now on, i.e., it should not be virtual. In this case, we don't break user API, but only internal one.

[destogl]

Hi,

I've reverted ros-controls/ros2_controllers#967 locally and backported the main functionality of this PR to humble (I needed to adapt it) in order to test it and it works for me. It solves ros-controls/ros2_controllers#966.

can you open PR please with this revert and write there that it should be merged when we have this functionality added.

[destogl]

Thanks for following up!

[bmagyar]

Looks good other than the refactor request