Enable non-loaded hardware components after start of CM.

[destogl]

The PR introduces an option to provide "non_loaded" option for hardware components when starting controller manager.

One example when this is useful is when running multiple controller managers in multiple machines or in multiple environments for separate control of different subcomponents. This way, you can still stick to one robot description with all ros2_control tags and then load only hardware to the controller manager who is required for the particular subsystem.

Downstream PRs

• Update Resource Manager API cyberbotics/webots_ros2#1025
• Update Resource Manager API gz_ros2_control#478

Question

Should we here change the API or add this change as new method and from the old method call the new one with the default argument?

[mergify]

This pull request is in conflict. Could you fix it @destogl?

[mergify]

This pull request is in conflict. Could you fix it @destogl?

[codecov]

Codecov Report

Attention: Patch coverage is 97.56098% with 1 line in your changes missing coverage. Please review.

Project coverage is 89.41%. Comparing base (00c7088) to head (4dfe63e).

Files with missing lines	Patch %	Lines
controller_manager/src/controller_manager.cpp	75.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1049      +/-   ##
==========================================
+ Coverage   89.29%   89.41%   +0.11%     
==========================================
  Files         130      130              
  Lines       14523    14559      +36     
  Branches     1258     1259       +1     
==========================================
+ Hits        12969    13018      +49     
+ Misses       1085     1077       -8     
+ Partials      469      464       -5     

Flag	Coverage Δ
unittests	89.41% <97.56%> (+0.11%)	⬆️

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

Files with missing lines	Coverage Δ
...ler_manager/test/test_hardware_management_srvs.cpp	98.75% <100.00%> (+0.18%)	⬆️
hardware_interface/src/resource_manager.cpp	75.40% <100.00%> (+0.37%)	⬆️
...e_interface_testing/test/test_resource_manager.cpp	99.47% <100.00%> (+<0.01%)	⬆️
controller_manager/src/controller_manager.cpp	76.37% <75.00%> (+0.22%)	⬆️

... and 4 files with indirect coverage changes

[saikishor]

I see the necessity of this change. I think it would be needed at some point.

I was thinking if there is a way to get this into without having this breaking change. I thought of the following. If you think this breaking change is OK, then simply ignore them.

1. Maybe we can add a method called "blacklist_components" or "skip_components" and use this information inside the logic of the existing method. This way we won't break any API.

2. Maybe we can also extend the information parsed from the URDF to HardwareInfo to also have a desired state. This way we simply update the parser and that's it

[saikishor]

Suggested change

	not_loaded: {
	unloaded: {

Can we name this unloaded instead of not_loaded, in the different parts of code. What's your opinion?

[destogl]

I am open to it. Althrough for me personaly, not_loaded is clearer, unloaded is more for unload (verb) - but can change, no problem.

@bmagyar, @christophfroehlich what do you think about this?

[saikishor]

Sure. I don't have a strong opinion here. If they are fine, all good :)

[destogl]

I see the necessity of this change. I think it would be needed at some point.

I was thinking if there is a way to get this into without having this breaking change. I thought of the following. If you think this breaking change is OK, then simply ignore them.

1. Maybe we can add a method called "blacklist_components" or "skip_components" and use this information inside the logic of the existing method. This way we won't break any API.

Do you mean here that we have a method skip_components that sets the local variable and has to be called before load_and_initialize? I am not a big fan of this approach, as we have additional “state” of the resource manager and calls for usage errors. I would then rather propose that we go with the same-named method with 3 arguments where we move the functionality, and call that one from the current method with 2 arguments. This way we keep the API and can deprecate any of the methods if we decide to remove any of them in the future.

2. Maybe we can also extend the information parsed from the URDF to `HardwareInfo` to also have a desired state. This way we simply update the parser and that's it

I would not necessarily put desired state to URDF, as this is more “dynamic” parameterization part. I mean it is definitely possible, but this would add some additional parameters to XACRO files, and we have already usually about 15–20 parameters 😄
For me, it is also somehow more natural to have this as a parameter to CM as it might be changed even for the same application. Nevertheless, glad to hear opinions of others 👍

[saikishor]

Thanks for the explanation @destogl