ros2_control icon indicating copy to clipboard operation
ros2_control copied to clipboard

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

Open mamueluth opened this issue 1 year ago • 15 comments

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 https://github.com/ros-controls/ros2_controllers/pull/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.

mamueluth avatar Dec 20 '23 14:12 mamueluth

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 :warning:
...e/test/test_component_interfaces_custom_export.cpp 88.88% 13 Missing :warning:
hardware_interface/src/resource_manager.cpp 68.42% 10 Missing and 2 partials :warning:
...ce/include/hardware_interface/system_interface.hpp 87.50% 9 Missing and 1 partial :warning:
...dware_interface/test/test_component_interfaces.cpp 98.11% 6 Missing :warning:
controller_manager/src/controller_manager.cpp 50.00% 4 Missing :warning:
.../include/hardware_interface/actuator_interface.hpp 93.33% 0 Missing and 4 partials :warning:
...ce/include/hardware_interface/sensor_interface.hpp 93.10% 0 Missing and 2 partials :warning:
controller_interface/src/controller_interface.cpp 50.00% 1 Missing :warning:
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%) :arrow_up:

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%) :arrow_up:
... and 17 more

codecov[bot] avatar Dec 20 '23 14:12 codecov[bot]

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

mamueluth avatar Jan 23 '24 14:01 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

saikishor avatar Jan 23 '24 15:01 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

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.

mamueluth avatar Jan 24 '24 12:01 mamueluth

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.

VX792 avatar Jan 25 '24 08:01 VX792

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

saikishor avatar Jan 26 '24 11:01 saikishor

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.

destogl avatar Jan 26 '24 15:01 destogl

@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. 🙌🏽🙌🏽🙌🏽

saikishor avatar Jan 26 '24 15:01 saikishor

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.

mamueluth avatar Jan 29 '24 09:01 mamueluth

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](https://github.com/ros-controls/ros2_control/blob/master/doc/migration/Iron.rst).

* [ ]  Update the [Writing a Hardware Component](https://control.ros.org/master/doc/ros2_control/hardware_interface/doc/writing_new_hardware_component.html#writing-a-hardware-component) section

@christophfroehlich can you please check the migration guide and the updated docs

mamueluth avatar Apr 11 '24 08:04 mamueluth

@christophfroehlich can you please check the migration guide and the updated docs

Thanks for the docs, I have fixed the rst syntax and some minor typos: https://github.com/StoglRobotics-forks/ros2_control/pull/18

christophfroehlich avatar Apr 12 '24 08:04 christophfroehlich

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

mergify[bot] avatar Apr 29 '24 15:04 mergify[bot]

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

@mamueluth I could only fix this by a merge commit. If you want to rebase, pls move the migration notes to the renamed file Jazzy.rst below the hardware_interface section.

christophfroehlich avatar Apr 29 '24 16:04 christophfroehlich

@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?

saikishor avatar May 13 '24 10:05 saikishor

@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:)

mamueluth avatar May 22 '24 15:05 mamueluth

Can we please start splitting this up as such:

  • changes related to handle.hpp -- essentially everything that is in the current changeset in that file but nothing about exporting (getters and setters included) -- update example_1 to support this WITHOUT InterfaceDescription constructors
  • InterfaceDescription and parsing of it and related tests, ADD InterfaceDescription constructors to handles
  • Adding export_state_interface_descriptions() API

bmagyar avatar Aug 05 '24 18:08 bmagyar

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

mergify[bot] avatar Aug 15 '24 12:08 mergify[bot]