Add new get_value API for Handles and Interfaces

[saikishor]

Related to ros-controls/ros2_controllers#1442 and ros-controls/ros2_controllers#1443

There are still some deprecated warnings in the following ros2_control packages:

transmission_interface: Once this PR is merged. I will perform the changes here : #1845
hardware components: This needs to be handled in a different PR.

I'll take care of both

[codecov]

Codecov Report

Attention: Patch coverage is 87.31343% with 17 lines in your changes missing coverage. Please review.

Project coverage is 89.28%. Comparing base (45f871e) to head (29d5a26).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
...de/hardware_interface/loaned_command_interface.hpp	46.15%	6 Missing and 1 partial ⚠️
...lude/hardware_interface/loaned_state_interface.hpp	46.15%	6 Missing and 1 partial ⚠️
...re_interface/include/hardware_interface/handle.hpp	62.50%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1976      +/-   ##
==========================================
- Coverage   89.35%   89.28%   -0.07%     
==========================================
  Files         139      139              
  Lines       14984    15001      +17     
  Branches     1286     1286              
==========================================
+ Hits        13389    13394       +5     
- Misses       1114     1123       +9     
- Partials      481      484       +3     

Flag	Coverage Δ
unittests	89.28% <87.31%> (-0.07%)	⬇️

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

Files with missing lines	Coverage Δ
...nclude/semantic_components/force_torque_sensor.hpp	97.95% <100.00%> (ø)
...terface/include/semantic_components/imu_sensor.hpp	97.77% <100.00%> (+0.10%)	⬆️
...erface/include/semantic_components/pose_sensor.hpp	96.55% <100.00%> (ø)
...mantic_components/semantic_component_interface.hpp	90.90% <100.00%> (ø)
...rface/test/test_chainable_controller_interface.cpp	100.00% <100.00%> (ø)
...chainable_controller/test_chainable_controller.cpp	88.37% <100.00%> (+0.27%)	⬆️
...r_manager/test/test_controller/test_controller.cpp	88.73% <100.00%> (ø)
...t_controllers_chaining_with_controller_manager.cpp	99.25% <100.00%> (+0.02%)	⬆️
...rface/test/mock_components/test_generic_system.cpp	99.72% <ø> (-0.04%)	⬇️
...dware_interface/test/test_component_interfaces.cpp	97.68% <100.00%> (+0.10%)	⬆️
... and 7 more

... and 4 files with indirect coverage changes

[christophfroehlich]

Thanks for pushing this forward.

I was thinking of something shorter than 2xget+2xvalue in get().get_value<double>().value(), but still have no better suggestion.

[christophfroehlich]

One thing: Could you please update the release_notes and also add something to the migration notes? We have only a section about the hardware_components there.

[saikishor]

One thing: Could you please update the release_notes and also add something to the migration notes? We have only a section about the hardware_components there.

Sure I'll do that

[destogl]

It is not beautiful, but probably we don't have a better way to do this…

Maybe we think of a simpler API in the future. At least we are good with the types.

Maybe the approach from ROS 2 parameters using method get_value_as_double() is better compared to get_value<double>().value()?

@bmagyar what do you think?

[saikishor]

Maybe the approach from ROS 2 parameters using method get_value_as_double() is better compared to get_value<double>().value()?

If we use optional, we cannot avoid using .has_value() and .value() even with the new method as the ROS2 parameters. That's a bit downside.

I have used .value() directly here as we know most of these tests work synchronously, so we always have value, but in the ROS2 Controllers etc, we will need to have the .get_value() as usual into a variable and check if it has value and if it does have value, then replace other instances of the variable in the code with .value()

[Juliaj]

Minor comment, LGTM.

[christophfroehlich]

I'm fine with the new API itself, I'd just add some more documentation. My proposal below.

Am I overseeing something, but it is never documented what returning false actually means? There is no docstring in handle.hpp or loaned_*_interfaces.hpp? Should we also document this in the permanent writing_new_controller page (or where it would belong..)?

(can be in a follow-up PR to avoid to block this being merged)

[christophfroehlich]

Thanks for the nice addition to the notes and docstrings. (At latest) When we branch for K-turtle, we should evaluate which parts of the migration notes to include in the permanent docs.

[bmagyar]

Thank you so much. Let's discuss further on how to improve this but I don't think we should hold back good work.

[destogl]

Thanks!