-
Notifications
You must be signed in to change notification settings - Fork 299
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
check for state of the controller node before cleanup #1363
check for state of the controller node before cleanup #1363
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #1363 +/- ##
==========================================
- Coverage 47.49% 47.45% -0.04%
==========================================
Files 41 41
Lines 3556 3561 +5
Branches 1938 1942 +4
==========================================
+ Hits 1689 1690 +1
- Misses 459 460 +1
- Partials 1408 1411 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Co-authored-by: Bence Magyar <bence.magyar.robotics@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good. We should also follow up with the #1236
@saikishor I tried to test the fix this morning but ran into the issue that the whole controller_manager crashes on my But I took a quick look at the changes you made and they look reasonable to me. |
(cherry picked from commit 6f57faf) # Conflicts: # controller_manager/test/test_controller_manager_srvs.cpp
(cherry picked from commit 6f57faf) # Conflicts: # controller_manager/test/test_controller_manager_srvs.cpp
(cherry picked from commit 6f57faf) # Conflicts: # controller_manager/test/test_controller_manager_srvs.cpp
(cherry picked from commit 6f57faf) # Conflicts: # controller_manager/test/test_controller_manager_srvs.cpp
Fixes #1136
If a controller tries to do the cleanup from an unconfigured state, it prints the following error in the terminal