Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce Creation of Handles with InterfaceDescription (variant support) #1679

Merged

Conversation

mamueluth
Copy link
Member

@mamueluth mamueluth commented Aug 14, 2024

This PR is the second part of multiple breaking down #1240 in smaller changes. For an overview explanation and a lot of comments/discussion, please refer to #1240.
This prepares the handle to be constructed with a description of an Interface (InterfaceDescription). The old ways of creating a Command-/StateInterface is kept as is but deprecated. If the Command-/StateInterface is constructed with the InterfaceDescription no pointer is given anymore but the pointer points to the internal storage of the handle. Variant is still only double and get/set is only available for double as well. One important thing here is that the internal value can not be practically used at this point, since the hardware has not access to the handle after exporting, so creating one with an InterfaceDescription is pointless at this time but will be useful in the next step.

NOTE: Everything is fully backward compatible at this point. There is no change visible to Controllers. For Hardware there are changes visible (additional way of construction of the Handles) but not usable since they have no access to Handles after exporting. Tests are included. Demos can not be adapted since hw has no access to Handles at this point.

  • @anyone: Please check if sufficiently tested in your opinion. Otherwise leave a comment.

Copy link

codecov bot commented Aug 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.66%. Comparing base (eb4316b) to head (75bb5ca).
Report is 6 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1679      +/-   ##
==========================================
+ Coverage   86.64%   86.66%   +0.02%     
==========================================
  Files         115      116       +1     
  Lines       10527    10537      +10     
  Branches      967      971       +4     
==========================================
+ Hits         9121     9132      +11     
+ Misses       1058     1057       -1     
  Partials      348      348              
Flag Coverage Δ
unittests 86.66% <100.00%> (+0.02%) ⬆️

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

Files Coverage Δ
...re_interface/include/hardware_interface/handle.hpp 100.00% <100.00%> (ø)
...rface/include/hardware_interface/hardware_info.hpp 100.00% <100.00%> (ø)
hardware_interface/src/component_parser.cpp 94.38% <100.00%> (+0.25%) ⬆️
hardware_interface/test/test_component_parser.cpp 98.79% <100.00%> (+0.13%) ⬆️
hardware_interface/test/test_handle.cpp 100.00% <100.00%> (ø)

... and 3 files with indirect coverage changes

@bmagyar bmagyar changed the title use interfacedescription for creation of handle, depricate old way use interfacedescription for creation of handle, deprecate old way Aug 15, 2024
@mamueluth mamueluth changed the title use interfacedescription for creation of handle, deprecate old way Introduce creation of Handles with InterfaceDescription (variant support) Aug 15, 2024
@mamueluth mamueluth changed the title Introduce creation of Handles with InterfaceDescription (variant support) Introduce Creation of Handles with InterfaceDescription (variant support) Aug 15, 2024
Copy link
Contributor

mergify bot commented Aug 15, 2024

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would love to see some examples of parsing data_type and size. Or is this not required here yet, and it is tested in another test?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is not supported here yet, i add this later with the full variant support. There i test the initialization of handles with all supported types and the parsing of the datatype for initialization of the handle is tested which includes tests for the data_typeand size.
@destogl should i add tests here nevertheless, or is it enough if the tests are added when the functionality is actually used.

Copy link
Member

@saikishor saikishor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look good to me.

If we need to use get_name at some point in the real-time loop, then maybe you might be interested in my suggestions. If not, they might/might not be interesting.

Thanks for the nice work .

Copy link
Member

@bmagyar bmagyar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you!

@bmagyar bmagyar merged commit 40ea191 into ros-controls:master Aug 21, 2024
19 checks passed
@destogl destogl deleted the interface_description branch August 26, 2024 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants