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

Also initialize non-joint components #822

Merged
merged 6 commits into from
Oct 21, 2022

Conversation

fmauch
Copy link
Contributor

@fmauch fmauch commented Sep 27, 2022

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 UniversalRobots/Universal_Robots_ROS2_Driver#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.

Copy link
Member

@destogl destogl left a comment

Choose a reason for hiding this comment

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

Thanks! This is great!

Excellent idea with the example in ros2_control_demos.

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. There is already test for other initial values. Extending that test would also be fine.

@fmauch
Copy link
Contributor Author

fmauch commented Sep 29, 2022

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
Copy link
Contributor Author

fmauch commented Sep 29, 2022

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

@destogl destogl closed this Sep 29, 2022
@destogl destogl reopened this Sep 29, 2022
@destogl
Copy link
Member

destogl commented Sep 29, 2022

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 destogl marked this pull request as draft September 29, 2022 06:48
@destogl destogl marked this pull request as ready for review September 29, 2022 06:48
@destogl
Copy link
Member

destogl commented Sep 29, 2022

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

Copy link
Member

@destogl destogl left a comment

Choose a reason for hiding this comment

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

This looks great! Thanks!

destogl
destogl previously approved these changes Sep 29, 2022
Copy link
Member

@destogl destogl left a comment

Choose a reason for hiding this comment

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

Thanks for following up!

Before, only joints were reading the initial value parameter. However, also
sensors and gpios can have configured initial values.
@destogl destogl self-requested a review October 5, 2022 16:46
@destogl destogl disabled auto-merge October 5, 2022 16:47
@destogl destogl enabled auto-merge (squash) October 5, 2022 16:48
@codecov-commenter
Copy link

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%) ⬇️

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%) ⬇️
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%) ⬆️
hardware_interface/test/test_component_parser.cpp 8.52% <ø> (-3.13%) ⬇️
... and 107 more

@fmauch
Copy link
Contributor Author

fmauch commented Oct 11, 2022

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

@destogl destogl merged commit f1302cb into ros-controls:master Oct 21, 2022
@fmauch fmauch deleted the init_sensors_and_gpios branch November 15, 2022 13:58
@destogl
Copy link
Member

destogl commented Apr 16, 2023

@Mergifyio backport humble

@mergify
Copy link
Contributor

mergify bot commented Apr 16, 2023

backport humble

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Apr 16, 2023
Co-authored-by: Denis Štogl <denis@stoglrobotics.de>
(cherry picked from commit f1302cb)
destogl added a commit that referenced this pull request Apr 20, 2023
* [MockHardware] Enalbe initialization non-joint components(#822)

Co-authored-by: Denis Štogl <denis@stoglrobotics.de>
(cherry picked from commit f1302cb)

* Fixup tests.

---------

Co-authored-by: Felix Exner <exner@fzi.de>
Co-authored-by: Denis Štogl <denis@stoglrobotics.de>
flochre pushed a commit to flochre/ros2_control that referenced this pull request Jul 5, 2023
…-controls#991)

* [MockHardware] Enalbe initialization non-joint components(ros-controls#822)

Co-authored-by: Denis Štogl <denis@stoglrobotics.de>
(cherry picked from commit f1302cb)

* Fixup tests.

---------

Co-authored-by: Felix Exner <exner@fzi.de>
Co-authored-by: Denis Štogl <denis@stoglrobotics.de>
pac48 pushed a commit to pac48/ros2_control that referenced this pull request Jan 26, 2024
…r.yaml. (ros-controls#822) (ros-controls#823)

(cherry picked from commit 0196eeb29ad0e2c908b547f57d31a8a1ea13e869)

Co-authored-by: Tony Baltovski <tony.baltovski@gmail.com>
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.

None yet

4 participants