-
Notifications
You must be signed in to change notification settings - Fork 386
Add handle_exceptions parameter to controller manager
#2807
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 handle_exceptions parameter to controller manager
#2807
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2807 +/- ##
==========================================
- Coverage 89.64% 89.48% -0.17%
==========================================
Files 152 152
Lines 17815 17960 +145
Branches 1455 1464 +9
==========================================
+ Hits 15971 16072 +101
- Misses 1260 1301 +41
- Partials 584 587 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
destogl
left a comment
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.
What is the main idea behind this? We are adding handling of failure on HW exception. But I don't know why we should do this just per-default? I agree that should never crash but not like this, instead by making sure we manage the all cases.
Recently, we are coming across the raised exceptions from third party libraries etc and the problem is if they don't have a proper exception message it is very hard to debug. By crashing, it prints the stack trace and with this we can find the issue easily. For instance, we have been having an issue last couple of weeks and we spent lot of time on it and today just removing the exception catching and making it to crash with the stack trace it print, we were able to pin point the issue in less than 5 min. If this kind of issue happens in future, it is better to launch it with this parameter and you could find the issue right away. It is more for the debugging purposes. |
christophfroehlich
left a comment
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.
This is great. Let's just add a description in the debugging section to the docs.
christophfroehlich
left a comment
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.
Shouldn't we also update the parameter section of the controller_manager, and maybe the release notes?
Isn't this autogenerated by GPL? |
christophfroehlich
left a comment
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.
oh, sure, you are right.
bmagyar
left a comment
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.
Thank you! Let's move forward with this flag & also start thinking about how to go the exact opposite way by catching more exceptions and allowing less "half and catch fire" moments (guarded by the flag, obviously)
b5fd514
into
ros-controls:master
(cherry picked from commit b5fd514) # Conflicts: # doc/release_notes.rst # hardware_interface/src/resource_manager.cpp
(cherry picked from commit b5fd514) # Conflicts: # doc/release_notes.rst
No description provided.