-
Notifications
You must be signed in to change notification settings - Fork 430
Check robot description validity on AdmittanceController and test for it #2009
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
base: master
Are you sure you want to change the base?
Conversation
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.
Thank you for taking this. I have some doubts if this is tested properly, see the comments
|
Thinking again, is there really a need to check this inside the controller? The controller manager will not start with a broken urdf, right @saikishor ? |
saikishor
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.
I agree with @christophfroehlich that this is not needed as CM provides a valid one.
Can you explain to us, why did you have a need to do this?
|
see the linked issue, but this was before we passed the robot description from the cm to the controllers. sorry for not checking that earlier. @caio-freitas would you mind in reverting this and cleanup the tests as by my comments instead? |
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.
Please fix the failing pre-commit tests: install it locally with pre-commit install, and run pre-commit run --all now to fix.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2009 +/- ##
==========================================
- Coverage 84.91% 84.88% -0.03%
==========================================
Files 148 148
Lines 14367 14379 +12
Branches 1230 1233 +3
==========================================
+ Hits 12200 12206 +6
- Misses 1740 1744 +4
- Partials 427 429 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
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.
the compatibility build fails, please review
…_with_invalid_robot_description
Fix #713
test_admittance_controller.cpp.on_initontest_admittance_controller.hppto only override the robot description with the one defined in theros2_control_test_assetsif not already defined.ros2_control/hardware_interface/src/component_parser.cpp(here)