search

ros-controls / ros2_controllers

Generic robotic controllers to accompany ros2_control
https://control.ros.org
Apache License 2.0
356 stars 319 forks source link

[PID Controller] Export state interfaces for easier chaining with other controllers #1214

Closed saikishor closed 1 month ago

saikishor commented 2 months ago

This PR aims to export some state interfaces from the PID controllers that can be used in cascade with other controllers, this helps in not defining separate parameters for command interfaces (The one exported by the PID controller) and the state interfaces that define the same state as the PID controller, and it simplifies the design of the chained controllers

codecov[bot] commented 2 months ago

Codecov Report

Attention: Patch coverage is 63.88889% with 13 lines in your changes missing coverage. Please review.

Project coverage is 80.35%. Comparing base (cdfc0af) to head (e5540ee). Report is 2 commits behind head on master.

Files Patch % Lines
pid_controller/src/pid_controller.cpp 53.57% 13 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1214 +/- ## ========================================== - Coverage 80.39% 80.35% -0.05% ========================================== Files 105 105 Lines 9357 9392 +35 Branches 821 830 +9 ========================================== + Hits 7523 7547 +24 - Misses 1557 1570 +13 + Partials 277 275 -2 ``` | [Flag](https://app.codecov.io/gh/ros-controls/ros2_controllers/pull/1214/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ros-controls) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/ros-controls/ros2_controllers/pull/1214/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ros-controls) | `80.35% <63.88%> (-0.05%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ros-controls#carryforward-flags-in-the-pull-request-comment) to find out more. | [Files](https://app.codecov.io/gh/ros-controls/ros2_controllers/pull/1214?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ros-controls) | Coverage Δ | | |---|---|---| | [pid\_controller/test/test\_pid\_controller.hpp](https://app.codecov.io/gh/ros-controls/ros2_controllers/pull/1214?src=pr&el=tree&filepath=pid_controller%2Ftest%2Ftest_pid_controller.hpp&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ros-controls#diff-cGlkX2NvbnRyb2xsZXIvdGVzdC90ZXN0X3BpZF9jb250cm9sbGVyLmhwcA==) | `84.78% <100.00%> (+0.16%)` | :arrow_up: | | [...\_controller/test/test\_pid\_controller\_preceding.cpp](https://app.codecov.io/gh/ros-controls/ros2_controllers/pull/1214?src=pr&el=tree&filepath=pid_controller%2Ftest%2Ftest_pid_controller_preceding.cpp&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ros-controls#diff-cGlkX2NvbnRyb2xsZXIvdGVzdC90ZXN0X3BpZF9jb250cm9sbGVyX3ByZWNlZGluZy5jcHA=) | `100.00% <100.00%> (ø)` | | | [pid\_controller/src/pid\_controller.cpp](https://app.codecov.io/gh/ros-controls/ros2_controllers/pull/1214?src=pr&el=tree&filepath=pid_controller%2Fsrc%2Fpid_controller.cpp&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ros-controls#diff-cGlkX2NvbnRyb2xsZXIvc3JjL3BpZF9jb250cm9sbGVyLmNwcA==) | `68.69% <53.57%> (-2.10%)` | :arrow_down: | ... and [4 files with indirect coverage changes](https://app.codecov.io/gh/ros-controls/ros2_controllers/pull/1214/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ros-controls)
christophfroehlich commented 2 months ago

Can't we implement this together with https://github.com/ros-controls/ros2_control/pull/1244? Instead of adding this to every chainable controller?

saikishor commented 2 months ago

Can't we implement this together with ros-controls/ros2_control#1244? Instead of adding this to every chainable controller?

@christophfroehlich #1244 is about publishing the interface information to a topic but not exposing as interfaces which is what we need here