-
Notifications
You must be signed in to change notification settings - Fork 413
Controller interface api update to ros2_controller packages #1973
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?
Controller interface api update to ros2_controller packages #1973
Conversation
… with controller_interface migration
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.
Thanks for your PR. But please consider our contributing guidelines, have you run colcon test and pre-commit locally? Almost all jobs fail here.
|
I have done all the tests and fixed the issues. Please review once |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1973 +/- ##
==========================================
- Coverage 85.11% 85.02% -0.10%
==========================================
Files 143 143
Lines 13740 13553 -187
Branches 1201 1154 -47
==========================================
- Hits 11695 11523 -172
+ Misses 1638 1633 -5
+ Partials 407 397 -10
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
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.
Generally looks good to me but please try reducing the repeated stuff as per my comment.
@bmagyar |
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.
Thanks for working on this. I agree with Bence, please apply the same pattern also for GPSSensorBroadcasterTest.
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.
Thanks a lot!
|
|
||
| std::unique_ptr<FriendGpioCommandController> controller_; | ||
|
|
||
| controller_interface::ControllerInterfaceParams create_default_params( |
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.
Only nit-picking, let's rename the methods create_ctrl_params or something similar, as it allows also setting non-default parameters.
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.
That's right! apart from the robot_description, the method can be modified to take values for other arg-spaces in params as well. I'll do that for both gpio_controllers and gps_sensor_broadcaster (create_ctrl_params)
Reviewed tests and deprecation warnings due to controller_interface update for the issue #1946
Modified the warning tests in the following packages;-