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

Add checks if hardware is initialized. #1054

Merged
merged 3 commits into from
Jul 18, 2023
Merged

Conversation

destogl
Copy link
Member

@destogl destogl commented Jun 14, 2023

This fixes potential bugs where interfaces are added but hardware is not initialized.

@destogl destogl added the bug label Jun 14, 2023
@destogl destogl self-assigned this Jun 14, 2023
@mergify
Copy link
Contributor

mergify bot commented Jun 14, 2023

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

@destogl destogl force-pushed the add-checks-if-hw-initialized branch from ad30d48 to 9e630a6 Compare June 14, 2023 19:20
@destogl destogl requested review from VX792 and bmagyar June 14, 2023 19:20
@bmagyar
Copy link
Member

bmagyar commented Jun 15, 2023

Worth adding a test?

@destogl
Copy link
Member Author

destogl commented Jun 16, 2023

Actually, yes, but no motivation... Have to find it first :D

Copy link
Contributor

@olivier-stasse olivier-stasse left a comment

Choose a reason for hiding this comment

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

LGTM

@codecov-commenter
Copy link

codecov-commenter commented Jul 2, 2023

Codecov Report

Merging #1054 (a6f73c8) into master (925f5f3) will decrease coverage by 2.36%.
The diff coverage is 34.81%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master    #1054      +/-   ##
==========================================
- Coverage   34.61%   32.26%   -2.36%     
==========================================
  Files          52       94      +42     
  Lines        2981     9915    +6934     
  Branches     1855     6694    +4839     
==========================================
+ Hits         1032     3199    +2167     
- Misses        310      787     +477     
- Partials     1639     5929    +4290     
Flag Coverage Δ
unittests 32.26% <34.81%> (-2.36%) ⬇️

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

Impacted Files Coverage Δ
controller_manager/src/controller_manager.cpp 37.94% <ø> (-1.77%) ⬇️
controller_manager/src/ros2_control_node.cpp 0.00% <0.00%> (ø)
..._interface/include/hardware_interface/actuator.hpp 100.00% <ø> (ø)
...ce/include/hardware_interface/async_components.hpp 0.00% <0.00%> (ø)
...re_interface/include/hardware_interface/sensor.hpp 100.00% <ø> (ø)
...re_interface/include/hardware_interface/system.hpp 100.00% <ø> (ø)
hardware_interface/src/resource_manager.cpp 46.76% <ø> (-6.87%) ⬇️
hardware_interface/src/sensor.cpp 50.52% <ø> (ø)
hardware_interface/src/system.cpp 55.45% <ø> (ø)
...rface/test/mock_components/test_generic_system.cpp 9.42% <ø> (ø)
... and 70 more

... and 20 files with indirect coverage changes

@destogl destogl force-pushed the add-checks-if-hw-initialized branch from 5b61160 to a6f73c8 Compare July 18, 2023 14:29
@bmagyar bmagyar added the backport-humble This label should be used by maintaines only! Label triggers PR backport to ROS2 humble. label Jul 18, 2023
@bmagyar bmagyar merged commit 6cd8191 into master Jul 18, 2023
1 check passed
mergify bot pushed a commit that referenced this pull request Jul 18, 2023
(cherry picked from commit 6cd8191)

# Conflicts:
#	hardware_interface/src/resource_manager.cpp
@destogl destogl deleted the add-checks-if-hw-initialized branch July 19, 2023 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-humble This label should be used by maintaines only! Label triggers PR backport to ROS2 humble. bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants