ros2_control
ros2_control copied to clipboard
[HW Interface] Change creation, exportation and storing of Command-/StateInterface + Support for variant
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()
orexport_command_interface()
withon_export_state_interface()
andon_export_command_interface()
in component interface -> Framework handles creation ofCommand-/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
andset_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 theCommand-/StateInterface
. Map was used to make it explicit rather then implicit. e.g. HW can now set state withset_state("joint1/position", 1.0)
- Create Handles based on InterfaceDescription -> encapsulate all information about Interface.
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.
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 |
Think i changed everything. @destogl
- Please check if the deprecation is now how you mentioned it.
- 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 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 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 thanmap
. 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.
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.
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
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 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. 🙌🏽🙌🏽🙌🏽
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.
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
@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
This pull request is in conflict. Could you fix it @mamueluth?
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.
@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
- 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.
- 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. - 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:)
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, ADDInterfaceDescription
constructors to handles - Adding
export_state_interface_descriptions()
API
This pull request is in conflict. Could you fix it @mamueluth?