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

[CM] Throw an exception when the components initially fail to be in the required state #1729

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

saikishor
Copy link
Member

We faced another bug during our testing when the HW component failed to configure at the beginning, the CM prints that the Resource Manager is successfully initialized and exposes it's internal services and the spawner calls are being processed on startup.

I believe, If the user's initial intent is supposed to be in a particular state, it is better to have failed when this is not the case rather than continue to expose services and then fail again. Usually, when the services are exposed, it is expected that HW components are successfully initialized.

Copy link

codecov bot commented Aug 28, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 86.74%. Comparing base (eb4c19d) to head (51372f8).

Files with missing lines Patch % Lines
controller_manager/src/controller_manager.cpp 50.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1729      +/-   ##
==========================================
- Coverage   86.77%   86.74%   -0.03%     
==========================================
  Files         116      116              
  Lines       10703    10709       +6     
  Branches      981      981              
==========================================
+ Hits         9288     9290       +2     
- Misses       1062     1066       +4     
  Partials      353      353              
Flag Coverage Δ
unittests 86.74% <50.00%> (-0.03%) ⬇️

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

Files with missing lines Coverage Δ
controller_manager/src/controller_manager.cpp 77.24% <50.00%> (-0.24%) ⬇️

Copy link
Contributor

@christophfroehlich christophfroehlich left a comment

Choose a reason for hiding this comment

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

LGTM! #1530

Copy link
Contributor

@Noel215 Noel215 left a comment

Choose a reason for hiding this comment

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

Tested on real hardware

@christophfroehlich christophfroehlich added backport-humble This label should be used by maintaines only! Label triggers PR backport to ROS2 humble. backport-iron This label should be used by maintaines only! Label triggers PR backport to ROS2 Iron. labels Aug 29, 2024
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.

just a small adjustment for people to be aware

controller_manager/src/controller_manager.cpp Outdated Show resolved Hide resolved
@bmagyar
Copy link
Member

bmagyar commented Aug 30, 2024

Also... would a test for this be possible?

@saikishor
Copy link
Member Author

Also... would a test for this be possible?

Yes! I'll add it when I address #1530. It's in my list. I wanted to do it once the variants and others are merged.

@saikishor
Copy link
Member Author

Thank you @christophfroehlich @bmagyar

saikishor and others added 3 commits September 16, 2024 18:16
Co-authored-by: Bence Magyar <bence.magyar.robotics@gmail.com>
Co-authored-by: Christoph Fröhlich <christophfroehlich@users.noreply.github.com>
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. backport-iron This label should be used by maintaines only! Label triggers PR backport to ROS2 Iron.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants