ros2_control icon indicating copy to clipboard operation
ros2_control copied to clipboard

Also initialize non-joint components

Open fmauch opened this issue 3 years ago • 6 comments

Before, only joints were reading the initial value parameter. However, also sensors and gpios can have configured initial values.

Initialization of vectors were called, but the implementation only checked for joints. This leaves all sensor state_interfaces initialized with NaN, which in turn breaks other things such as https://github.com/UniversalRobots/Universal_Robots_ROS2_Driver/issues/390.

With the changes suggested here, also sensors and gpios get initialized with the initial values read from the URDF.

I think in conjunction it would make sense to extend the ros2_control_demos with inital values. I've used those as a simplified environment also to verify that my URDF was not the culprit here.

I am not 100% sure whether this would be the most suitable modification, as I haven't completely understood the dataflow here, but that change seemed rather obvious to me. Feel free to point me into another direction if needed.

Anyway, this should also be ported back to Galactic. Probably a direct backmerge is not possible, since the fake->mock renaming didn't happen there, but it should be easy to do nonetheless.

fmauch avatar Sep 27 '22 09:09 fmauch

Could you also add a test for this in this file? It should be straightforward. One should add another xml example and then check if parsing is good.

sure!

fmauch avatar Sep 29 '22 04:09 fmauch

Oh, I was not aware, that by re-requesting a review I would remove other potential reviewers. Sorry if I destroyed the workflow there.

fmauch avatar Sep 29 '22 05:09 fmauch

Oh, I was not aware, that by re-requesting a review I would remove other potential reviewers. Sorry if I destroyed the workflow there.

No this shouldn't be happening. It's not your fault!

destogl avatar Sep 29 '22 06:09 destogl

FYI: I just converted it to draft and back to re-trigger reviewer-lottery workflow. (closing was unintentional :D)

destogl avatar Sep 29 '22 06:09 destogl

Codecov Report

Merging #822 (32a67bb) into master (925f5f3) will decrease coverage by 2.04%. The diff coverage is 38.09%.

@@            Coverage Diff             @@
##           master     #822      +/-   ##
==========================================
- Coverage   34.61%   32.57%   -2.05%     
==========================================
  Files          52       91      +39     
  Lines        2981     9295    +6314     
  Branches     1855     6251    +4396     
==========================================
+ Hits         1032     3028    +1996     
- Misses        310      720     +410     
- Partials     1639     5547    +3908     
Flag Coverage Δ
unittests 32.57% <38.09%> (-2.05%) :arrow_down:

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

Impacted Files Coverage Δ
controller_manager/src/controller_manager.cpp 36.63% <ø> (-3.08%) :arrow_down:
controller_manager/src/realtime.cpp 0.00% <0.00%> (ø)
controller_manager/src/ros2_control_node.cpp 0.00% <0.00%> (ø)
..._interface/include/hardware_interface/actuator.hpp 100.00% <ø> (ø)
...re_interface/include/hardware_interface/sensor.hpp 100.00% <ø> (ø)
...re_interface/include/hardware_interface/system.hpp 100.00% <ø> (ø)
hardware_interface/src/system.cpp 55.45% <ø> (ø)
...rface/test/mock_components/test_generic_system.cpp 9.14% <ø> (ø)
...dware_interface/test/test_component_interfaces.cpp 32.44% <ø> (+4.25%) :arrow_up:
hardware_interface/test/test_component_parser.cpp 8.52% <ø> (-3.13%) :arrow_down:
... and 107 more

codecov-commenter avatar Oct 05 '22 17:10 codecov-commenter

Any updates on merging this? The failing tests seem to be unrelated but I might be wrong there.

fmauch avatar Oct 11 '22 07:10 fmauch

@mergifyio backport humble

destogl avatar Apr 16 '23 10:04 destogl

backport humble

✅ Backports have been created

mergify[bot] avatar Apr 16 '23 10:04 mergify[bot]