Skip to content
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

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

Conversation

saikishor
Copy link
Member

@saikishor saikishor commented Jul 16, 2024

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

Copy link

codecov bot commented Jul 16, 2024

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 ⚠️
Additional details and impacted files
@@            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 Coverage Δ
unittests 80.35% <63.88%> (-0.05%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
pid_controller/test/test_pid_controller.hpp 84.78% <100.00%> (+0.16%) ⬆️
..._controller/test/test_pid_controller_preceding.cpp 100.00% <100.00%> (ø)
pid_controller/src/pid_controller.cpp 68.69% <53.57%> (-2.10%) ⬇️

... and 4 files with indirect coverage changes

@christophfroehlich
Copy link
Contributor

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

@saikishor
Copy link
Member Author

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

@saikishor saikishor changed the title Export defined state interfaces from PID Controllers [PID Controller] Export state interfaces for easier chaining with other controllers Aug 5, 2024
Copy link
Member

@destogl destogl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This looks good!

@bmagyar bmagyar merged commit a7b2af5 into ros-controls:master Aug 29, 2024
11 of 20 checks passed
@bmagyar bmagyar deleted the export/state_interfaces/pid_controller branch August 29, 2024 11:48
RobertWilbrandt pushed a commit to RobertWilbrandt/ros2_controllers that referenced this pull request Sep 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants