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

[mamueluth]

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 58.63487% with 503 lines in your changes are missing coverage. Please review.

Project coverage is 62.57%. Comparing base (35bb5f7) to head (b9a4b9c).
Report is 20 commits behind head on master.

❗ Current head b9a4b9c differs from pull request most recent head 476d44b. Consider uploading reports for the commit 476d44b to get more accurate results

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #1240       +/-   ##
===========================================
- Coverage   75.51%   62.57%   -12.94%     
===========================================
  Files          41       99       +58     
  Lines        3328    12392     +9064     
  Branches     1921     8971     +7050     
===========================================
+ Hits         2513     7754     +5241     
- Misses        421      750      +329     
- Partials      394     3888     +3494     

Flag	Coverage Δ
unittests	62.57% <58.63%> (-12.94%)	⬇️

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

Files	Coverage Δ
..._interface/include/hardware_interface/actuator.hpp	100.00% <ø> (ø)
.../include/hardware_interface/actuator_interface.hpp	100.00% <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	100.00% <ø> (ø)
...ce/include/hardware_interface/sensor_interface.hpp	100.00% <100.00%> (ø)
...re_interface/include/hardware_interface/system.hpp	100.00% <ø> (ø)
hardware_interface/src/actuator.cpp	75.75% <100.00%> (+2.87%)	⬆️
hardware_interface/src/component_parser.cpp	93.22% <100.00%> (-0.03%)	⬇️
hardware_interface/src/sensor.cpp	70.75% <100.00%> (+2.06%)	⬆️
... and 9 more

... and 55 files with indirect coverage changes

[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.

[destogl]

I doubt that we need methods here. The code is not so long, and the import is used only once. So if we use here import directly, it might be that we even increase readability.

[mamueluth]

in my opinion the code is more readable like this because you have the functions with a name that explains what is done and then the comments for the functions which explains it in more detail.

[destogl]

Still, I would keep this in the on_init method. You should also comment here, but we should cut the code, and it would still be readable. I find it now even more harder to read, because one has to jump checking it, in other case, everything is just in front of you.

[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

[saikishor]

@mamueluth I've a question regarding std::variant, I started to take a look for a different project inspiring from this proposal you have made. While understanding more about the approach with std::variant, I came across some of the following:

• The overhead introduced by std::variant, especially in terms of memory allocation and deallocation, might introduce variability in execution times, which could be problematic in real-time contexts. -> (What do you think about this?)
• Accessing and manipulating variants might introduce some runtime overhead due to type checks and dynamic memory allocation -> (Do you know more information on this? like any benchmarks etc)
• std::variant requires that all its alternative types are copy-constructible and move-constructible -> (I don't think it is an issue for the types we use)

What do you think about the above points?

[mamueluth]

@saikishor

1. I think this should be fine for us as i understand std::variant works as typesafe union and is not allowed to allocate additional (dynamic) memory. As we set the type at creation time of the Interface and don't change the type during runtime i think this should be fine. How this works with strings however i have to check.
2. I think this should be fine as well as we later use it with directly giving the expected type : get_value()
   But i don't have a benchmark at hand.
3. Should not be a problem as we only allow certain standard types as int, uint_32, double ....

But if you think different or think we should investigate in more detail please let me know or contact me directly:)