[HW Interface] Change creation, exportation and storing of Command-/StateInterface + Support for variant

[mamueluth]

Important from Aug. 12th 2024

This PR is split in multiple parts which can be merged step by step.

1. 🥳 Preparation of Handles for Variant Support #1678
2. 🥳 Introduce Creation of Handles with InterfaceDescription (variant support) #1679
3. 🥳 Rename get_state and set_state Functions to get/set_lifecylce_state (variant support) #1683
4. Automatic Creation of Handles in HW, Adding Getters/Setters (variant support) #1688
5. Adapt controller Reference/StateInterfaces to New Way of Exporting (variant support) #1689

Notes

The concept fully backward compatible, has been tested and works. In the demos repository the example_1 has been adapted to showcase the changes and test the approach.

Needs ros-controls/ros2_controllers#1103 to be mereged.

Idea

The idea is that the creation of the Command-/StateInterface is no longer handled by the HW (user) but instead by the Framework. The user doesn't need to know about creation of Command-/StateInterface and creation and management of local memory. Instead, the Command-/StateInterface are defined in the urdf as before and then everything is handled automatically by the Framework. The HW only needs to set/get states or set/get commands via functions provided by the component interface.
Additionally, the Handles are now prepared to support more than double as datatype (std::variant is used) and could in future provide functionality to check if the Handles has been updated since now the value is stored directly in the handle instead a pointer.

The export_command/state_interfaces_2() function is going to be renamed to export_command/state_interfaces() as soon as the deprecated version is removed. This step is necessary as no return type polymorphism is possible and we later want abase function the user can override to export custom unlisted interfaces.

Description

With this PR the exportation and creation of the Command-/StateInterface is now handled by the Framework with on_export_state_interface() and on_export_command_interface() instead of the HW-component exporting them via the export_state_interface() or export_command_interface().

The Command-/StateInterface are created based on an InterfaceDescription which encapsulates all the information about an interface and is directly parsed from the urdf. This includes data_type, default_values, min and max. Command-/StateInterface are stored in the component interface (Actuator-, Sensor- or SystemInterface). The resource manager gets shared_ptr to the Command-/StateInterface.

The Command-/StateInterface are now store the value directly. There is no raw pointer to memory in the HW passed to the Command-/StateInterface. A component can then set/get values from the Command-/StateInterface via set_state/get_state and set_commad/get_command functions provided by the component interface. In addition, the value is now of typ std::variant

Overview

• replace export_state_interface() or export_command_interface() with on_export_state_interface() and on_export_command_interface() in component interface -> Framework handles creation of Command-/StateInterface
• Memory was moved from HW into the Handle -> HW doesn't need to create/manage memory. Support for std::variant (different datatypes than double in Handles). Support to for example check in Handle if the value of Handle has been changed. Support for distributed control.
• Add set_state/get_state and set_commad/get_command functions to component interface -> since memory is moved to Handle, HW needs to be able to still set states/receive commands.
• Command-/StateInterface are stored in (Actuator-, Sensor- or SystemInterface) in map. -> This was done because the memory is moved to the Handles. HW still needs to be able to set states/receive commands. Otherwise we would need to import "hw-loans" or ptr to the Command-/StateInterface. Map was used to make it explicit rather then implicit. e.g. HW can now set state with set_state("joint1/position", 1.0)
• Create Handles based on InterfaceDescription -> encapsulate all information about Interface.

[codecov]

Codecov Report

Attention: Patch coverage is 91.74757% with 68 lines in your changes missing coverage. Please review.

Project coverage is 87.09%. Comparing base (3cf167d) to head (b05f585).
Report is 1 commits behind head on master.

Files	Patch %	Lines
...r_interface/src/chainable_controller_interface.cpp	57.89%	14 Missing and 2 partials ⚠️
...e/test/test_component_interfaces_custom_export.cpp	88.88%	13 Missing ⚠️
hardware_interface/src/resource_manager.cpp	68.42%	10 Missing and 2 partials ⚠️
...ce/include/hardware_interface/system_interface.hpp	87.50%	9 Missing and 1 partial ⚠️
...dware_interface/test/test_component_interfaces.cpp	98.11%	6 Missing ⚠️
controller_manager/src/controller_manager.cpp	50.00%	4 Missing ⚠️
.../include/hardware_interface/actuator_interface.hpp	93.33%	0 Missing and 4 partials ⚠️
...ce/include/hardware_interface/sensor_interface.hpp	93.10%	0 Missing and 2 partials ⚠️
controller_interface/src/controller_interface.cpp	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1240      +/-   ##
==========================================
+ Coverage   86.70%   87.09%   +0.38%     
==========================================
  Files         115      118       +3     
  Lines       10540    11273     +733     
  Branches      972     1045      +73     
==========================================
+ Hits         9139     9818     +679     
- Misses       1054     1098      +44     
- Partials      347      357      +10     

Flag	Coverage Δ
unittests	87.09% <91.74%> (+0.38%)	⬆️

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

Files	Coverage Δ
...oller_interface/chainable_controller_interface.hpp	100.00% <ø> (ø)
...lude/controller_interface/controller_interface.hpp	100.00% <ø> (ø)
...controller_interface/controller_interface_base.hpp	87.50% <ø> (ø)
...rface/test/test_chainable_controller_interface.cpp	100.00% <100.00%> (ø)
..._interface/include/hardware_interface/actuator.hpp	100.00% <ø> (ø)
...re_interface/include/hardware_interface/handle.hpp	100.00% <100.00%> (ø)
...rface/include/hardware_interface/hardware_info.hpp	100.00% <100.00%> (ø)
...re_interface/include/hardware_interface/sensor.hpp	50.00% <ø> (ø)
...re_interface/include/hardware_interface/system.hpp	100.00% <ø> (ø)
hardware_interface/src/actuator.cpp	75.67% <100.00%> (+2.54%)	⬆️
... and 17 more

[destogl]

In general, this looks good. I just see two bigger things to fix:

• abstract deprecated approach from new approach on the component level and not in the ResourceManager, then we don't need additional on_export_* methods inside components, but just interfaces.
• I didn't see a test with the custom implementation fo a component that exports custom, not in URDF defined, interface manually. This functionality we want to keep and, therefore, should be tested. There is already a test with the current approach for exactly that case.

[saikishor]

Here are some changes I think are necessary. I will review the rest of the code later

[saikishor]

Suggested change

	std::vector<std::shared_ptr<StateInterface>> Actuator::on_export_state_interfaces()
	std::vector<StateInterfaceSharedPtr> Actuator::on_export_state_interfaces()

[mamueluth]

Think i changed everything.
@destogl

1. Please check if the deprecation is now how you mentioned it.
2. Please look at the custom export again int the test if this is how you imagined it. on_export_function is now virtual and the states and command now protected instead of private to allow overriding and adding of custom interfaces.

@saikishor I didn't use unordered_map everywhere as for the name to interface description we want to keep the order in my understanding. O(log(n)) overhead should be ok in this case since its most of the time used to iterate over it. If you don't agree i can change it but then all test_component_interface tests need to be changed as well since they rely on implicit ordering of the interfaces...

[saikishor]

@saikishor I didn't use unordered_map everywhere as for the name to interface description we want to keep the order in my understanding. O(log(n)) overhead should be ok in this case since its most of the time used to iterate over it. If you don't agree i can change it but then all test_component_interface tests need to be changed as well since they rely on implicit ordering of the interfaces...

Usually, the unoredered_map's are faster to access O(1), which is better to use in such a layer. For that reason, I recommended using unordered_map rather than map. They tend to use a bit more or memory, but performance-wise they are much better.

if you have a different opinion on this, we can discuss

[mamueluth]

@saikishor I didn't use unordered_map everywhere as for the name to interface description we want to keep the order in my understanding. O(log(n)) overhead should be ok in this case since its most of the time used to iterate over it. If you don't agree i can change it but then all test_component_interface tests need to be changed as well since they rely on implicit ordering of the interfaces...

Usually, the unoredered_map's are faster to access O(1), which is better to use in such a layer. For that reason, I recommended using unordered_map rather than map. They tend to use a bit more or memory, but performance-wise they are much better.

if you have a different opinion on this, we can discuss

Yes you are completely right about the performance thats why i now use unordered_map for the states and commands which is accessed a lot e.g. for SensorInterface:

std::map<std::string, InterfaceDescription> sensor_state_interfaces_;
std::unordered_map<std::string, StateInterfaceSharedPtr> sensor_states_;

But for the sensor_state_interfaces_ i would still use map, because this is only used to get the InterfaceDescription for a given name and for initialization of the actual StateInterfaces in the Interface e.g. the sensor_states_ later. The user uses this map most of the time by iterating over it like so:

for (const auto & [name, descr] : sensor_state_interfaces_) {
set_state(name, <state>)
....

and in my understanding it would be beneficial to have the same ordering of the Interfaces in this map as they are defined in the URDF.

[VX792]

Thanks for the great work!

These changes make it a lot easier to finish the asynchronous stuff, as currently any syhncronization can be bypassed due to the user having direct access to the double ptr.

I only have one question, which is why shared ptrs are used here instead of unique ptrs. It's possible I missed something, but to me it looks like there's clear ownership (the hardware interface owns the states and commands).

For the sensor_state interfaces example it's also possible to use a presorted vector of pairs instead of a map if it's accessed often and every nanosecond counts (if not, then it shouldn't matter), since the vector has better cache locality and usually performs better for a small number of elements, unless you're also inserting into and deleting elements from the middle.

[saikishor]

and in my understanding it would be beneficial to have the same ordering of the Interfaces in this map as they are defined in the URDF.

Hello @mamueluth!

is there a specific reason to have the same ordering of the interfaces as in the URDF?. Moreover, std::map doesn't ensure this order at all. Well anyway, if you are accessing only a few times but not continuously a std::map is fine

[destogl]

is there a specific reason to have the same ordering of the interfaces as in the URDF?. Moreover, std::map doesn't ensure this order at all. Well anyway, if you are accessing only a few times but not continuously a std::map is fine

@saikishor Manuel is changing this, he was using this a lot in the tests. He will finish their adjustment on friday.

[saikishor]

@saikishor Manuel is changing this, he was using this a lot in the tests. He will finish their adjustment on friday.

Sure. If it's being used a few times std::map should be fine. 🙌🏽🙌🏽🙌🏽

[mamueluth]

Now unorderd_map is used everywhere and tests are adjusted accordingly.
Further new export_command/state_interface_2() function has been introduced which the user can override to export custom unlisted interfaces. This function is going to be renamed as soon as the deprecated version is removed.

[saikishor]

I've added some minor comments and some questions at different parts to discuss

[christophfroehlich]

I like the approach, considering the cleaner code as demonstrated with example_1 and the (as by now, theoretical) support of other datatypes. I don't comment on the code as this has been done extensively by others already.

What is missing for my approval is an update to the documentation:

• A migration guide here.
• Update the Writing a Hardware Component section

[mergify]

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

[bmagyar]

Concluded. Thank you to everyone who took part!

[destogl]

All separation-PRs are merged 🎉