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

Deactivate the controllers when they return error similar to the hard… #1499

Conversation

saikishor
Copy link
Member

Hello!

This is something we have discussed long back in one of the WG meetings to unify the approach with HW and controllers. Right now, when the hardware returns ERROR, it currently deactivates the controllers that use the hardware's interfaces.

Likewise, we want to enforce that if a controller returns an error, we would like to deactivate it to avoid situations in the next cycles. Moreover, this return info from the controllers are not used at all, so, I think it would make sense to put them to proper use.

Thank you

Copy link

codecov bot commented Apr 22, 2024

Codecov Report

Attention: Patch coverage is 42.16867% with 48 lines in your changes are missing coverage. Please review.

Project coverage is 62.54%. Comparing base (16fbde3) to head (45ea56a).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1499      +/-   ##
==========================================
- Coverage   62.67%   62.54%   -0.14%     
==========================================
  Files          96       96              
  Lines       11311    11394      +83     
  Branches     8151     8210      +59     
==========================================
+ Hits         7089     7126      +37     
- Misses        715      726      +11     
- Partials     3507     3542      +35     
Flag Coverage Δ
unittests 62.54% <42.16%> (-0.14%) ⬇️

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

Files Coverage Δ
controller_manager/src/controller_manager.cpp 68.38% <87.50%> (+0.32%) ⬆️
...r_manager/test/test_controller/test_controller.cpp 82.22% <66.66%> (-1.12%) ⬇️
...est_controller_manager_hardware_error_handling.cpp 43.55% <36.11%> (-1.59%) ⬇️

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.

I'm fine with that changes, thanks! But it's missing tests :/

controller_manager/doc/userdoc.rst Outdated Show resolved Hide resolved
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, Thanks for the quick changes!

@saikishor saikishor added this to the Pre-Jazzy (Feb.'24) milestone Apr 24, 2024
Co-authored-by: Bence Magyar <bence.magyar.robotics@gmail.com>
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.

Very nice, thanks!

@bmagyar bmagyar merged commit cca2070 into ros-controls:master Apr 24, 2024
9 of 11 checks passed
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

3 participants