Export pid gains as reference #2020

kumar-sanjeeev · 2025-11-21T19:39:34Z

This PR includes the following changes:

Added a new parameter export_gain_references to enable or disable the export of gains (p, i, d) as references.
Updated the update_and_write_commands function based on the value of export_gain_references.
Updated test cases for both scenarios i.e export_gain_references=true and export_gain_references=false
Updated the documentation to include the option for exporting PID gains as a reference.

codecov · 2025-11-21T20:08:03Z

Codecov Report

❌ Patch coverage is 68.25397% with 20 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.06%. Comparing base (ec198b1) to head (7c836d3).

Files with missing lines	Patch %	Lines
pid_controller/src/pid_controller.cpp	56.81%	15 Missing and 4 partials ⚠️
pid_controller/test/test_pid_controller.cpp	93.75%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2020      +/-   ##
==========================================
- Coverage   85.13%   85.06%   -0.07%     
==========================================
  Files         144      144              
  Lines       13968    14021      +53     
  Branches     1201     1211      +10     
==========================================
+ Hits        11891    11927      +36     
- Misses       1670     1685      +15     
- Partials      407      409       +2

Flag	Coverage Δ
unittests	`85.06% <68.25%> (-0.07%)`	⬇️

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

Files with missing lines	Coverage Δ
pid_controller/test/test_pid_controller.hpp	`87.50% <ø> (ø)`
..._controller/test/test_pid_controller_preceding.cpp	`100.00% <100.00%> (ø)`
pid_controller/test/test_pid_controller.cpp	`99.45% <93.75%> (-0.55%)`	⬇️
pid_controller/src/pid_controller.cpp	`66.42% <56.81%> (-1.50%)`	⬇️

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

christophfroehlich

In principle, this looks fine for me.

christophfroehlich · 2025-11-22T22:10:09Z

pid_controller/src/pid_controller.cpp

+          switch (gain_type)
+          {
+            case 0:  // P gain
+              pids_[i]->set_gains(


Am I missing something? I think there is no functionality to skip setting the gain by passing a NaN value?

@christophfroehlich , you are right. I am thinking of following fix. What do you think ?

Suggested change

pids_[i]->set_gains(

auto current_pid_gains = pids_[i]->get_gains();

for (size_t j = 0; j < GAIN_INTERFACES.size(); ++j)

{

const size_t buffer_index = gains_start_index + i + j * dof_;

const double new_gain_value = reference_interfaces_[buffer_index];

if (std::isfinite(new_gain_value))

{

const size_t gain_type = GAIN_TYPES_INDEX[j];

switch (gain_type)

{

case 0: // P gain

current_pid_gains.p_gain_ = new_gain_value;

pids_[i]->set_gains(current_pid_gains);```

thedevmystic · 2025-11-24T13:42:31Z

pid_controller/include/pid_controller/pid_controller.hpp

  std::vector<double> measured_state_values_;

+  inline static const std::vector<std::string> GAIN_INTERFACES = {"p", "i", "d"};
+  inline static const std::vector<size_t> GAIN_TYPES_INDEX = {0, 1, 2};


A quick question about convention of ROS Controls, do we follow,

[Storage Specificier] [Modifier] [Type] [Variable Name]
Like: static constexpr bool is_ros = true;

Or
[Modifier] [Storage Specificier] [Type] [Variable Name]

Like: constexpr static bool is_ros = true;

If we follow the above convention, then I think you should change the order of specifier.

Am I missing something, or is the code convention different.

I assumed the first convention because it is used pre-dominantely in other codebase.

Thanks!

hi @thedevmystic,

I checked your recommendation, and you are right. I am going to follow the first convention.

Suggested change

inline static const std::vector<size_t> GAIN_TYPES_INDEX = {0, 1, 2};

static inline const std::vector<std::string> GAIN_INTERFACES = {"p", "i", "d"};

static inline const std::vector<size_t> GAIN_TYPES_INDEX = {0, 1, 2};```

kumar-sanjeeev · 2025-11-24T19:54:37Z

In principle, this looks fine for me.

understood. I am trying to understand one thing, in my current implementation, when I set export_gain_references=true, the reference_interfaces_ vector allocates additional memory for the PID gains for each DOF, as expected:

  if (params_.export_gain_references)
  {
    total_reference_size += dof_ * GAIN_INTERFACES.size();
  }
  reference_interfaces_.resize(total_reference_size, std::numeric_limits<double>::quiet_NaN());

Because of this, some of the existing tests are failing. If this implementation is correct, should I update the tests so that they handle both cases (export_gain_references=true and export_gain_references=false)? Or is there something else I might be missing? Or is there any other recommended way to cater this ?

Test config that I used to reproduce the failure case, along with the corresponding log:

test_pid_controller:
  ros__parameters:
    dof_names:
      - joint1

    command_interface: position

    reference_and_state_interfaces: ["position"]

    export_gain_references: true

    gains:
      joint1: {p: 1.0, i: 2.0, d: 3.0, u_clamp_max: 5.0, u_clamp_min: -5.0}

/workspaces/ros2_control_rolling_dev_docker_ws/src/ros-controls/ros2_controllers/pid_controller/test/test_pid_controller.cpp:88: Failure
Expected equality of these values:
  ref_if_conf.size()
    Which is: 4
  dof_state_values_.size()
    Which is: 1

[  FAILED  ] PidControllerTest.check_exported_interfaces (6 ms)

thedevmystic · 2025-11-25T06:16:26Z

I also think implementation is correct @kumar-sanjeeev,
But if implementation is correct then, why are we failing tests?

Overall, I think some tests need to be updated, but this is just my assumption.

…m value

…onsistency

…rences param

kumar-sanjeeev · 2025-11-26T22:08:48Z

hi @christophfroehlich, I have tried few different ways to fix the ABI check, but still haven’t succeeded. Do you have any recommendations? Or is it okay in this feature request, if it breaks ?

thedevmystic · 2025-11-27T08:53:52Z

pid_controller/src/pid_controller.cpp

+      auto current_pid_gains = pids_[i]->get_gains();
+      for (size_t j = 0; j < GAIN_INTERFACES.size(); ++j)
+      {
+        const size_t buffer_index = gains_start_index + i + j * dof_;


Suggested change

const size_t buffer_index = gains_start_index + i + j * dof_;

const size_t buffer_index = gains_start_index + (j * dof_) + i;

As the standard formula for index calculation is,
index = base_offset + (row_index * row_size) + column_index.

While the previous one is perfectly fine, this improves general readability and precedence readability.

thedevmystic · 2025-11-27T08:58:10Z

pid_controller/src/pid_controller.yaml

+      gain_references: {
+        type: bool,
+        default_value: false,
+        description: "If true, exports P, I, D gains as reference interfaces to be claimed by preceding controllers",


If you don't mind me nitpicking on your descriptions, I'll suggest this.

Suggested change

description: "If true, exports P, I, D gains as reference interfaces to be claimed by preceding controllers",

description: "If true, exports P, I, and D gains as reference interfaces to be claimed by preceding controllers",

Just for clarity and grammatical consistency.

thedevmystic · 2025-11-28T09:07:34Z

Phenomenal work, @kumar-sanjeeev. And I'm very glad you completed this, and now this is ready for a review. Good luck for ahead!

christophfroehlich reviewed Nov 22, 2025

View reviewed changes

thedevmystic reviewed Nov 24, 2025

View reviewed changes

kumar-sanjeeev added 6 commits November 26, 2025 18:15

Add bool param to export gains as reference

af1e76f

Add gains to export as references based on export_gain_reference para…

39d9932

…m value

Add export_gain_references param in yaml

4c29c0e

Switch modifier and storage specifier position to maintain codebase c…

d58134f

…onsistency

Do not skip gains by setting them NaN

ff8db06

Update test cases to accomodate for both values of exported_gain_refe…

79473e8

…rences param

kumar-sanjeeev force-pushed the feat-export-pid-gains-as-reference branch from 9914ad5 to 79473e8 Compare November 26, 2025 17:17

kumar-sanjeeev added 2 commits November 26, 2025 21:24

Shift export_gain_references variable to fix abi check

7304887

Add gain references inside new key to pass abi check

5b70746

Fix variable name as per gtest convention

3319cd2

thedevmystic reviewed Nov 27, 2025

View reviewed changes

kumar-sanjeeev added 2 commits November 28, 2025 08:28

Add code review suggestions

1bfe5ef

Update docs with gain reference export possibility

7c836d3

kumar-sanjeeev marked this pull request as ready for review November 28, 2025 08:37

github-actions bot requested review from MarqRazz, bijoua29, destogl, fmauch and mamueluth November 28, 2025 08:37

kumar-sanjeeev changed the title ~~Draf PR : Export pid gains as reference~~ Export pid gains as reference Dec 1, 2025

kumar-sanjeeev requested a review from christophfroehlich December 1, 2025 13:08

	inline static const std::vector<size_t> GAIN_TYPES_INDEX = {0, 1, 2};
	static inline const std::vector<std::string> GAIN_INTERFACES = {"p", "i", "d"};
	static inline const std::vector<size_t> GAIN_TYPES_INDEX = {0, 1, 2};```

	const size_t buffer_index = gains_start_index + i + j * dof_;
	const size_t buffer_index = gains_start_index + (j * dof_) + i;

	description: "If true, exports P, I, D gains as reference interfaces to be claimed by preceding controllers",
	description: "If true, exports P, I, and D gains as reference interfaces to be claimed by preceding controllers",

Export pid gains as reference #2020

Are you sure you want to change the base?

Export pid gains as reference #2020

Uh oh!

Conversation

kumar-sanjeeev commented Nov 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Nov 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

christophfroehlich left a comment

Choose a reason for hiding this comment

Uh oh!

christophfroehlich Nov 22, 2025

Choose a reason for hiding this comment

Uh oh!

kumar-sanjeeev Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thedevmystic Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kumar-sanjeeev Nov 24, 2025

Choose a reason for hiding this comment

Uh oh!

kumar-sanjeeev commented Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thedevmystic commented Nov 25, 2025

Uh oh!

kumar-sanjeeev commented Nov 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thedevmystic Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

thedevmystic Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

thedevmystic commented Nov 28, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kumar-sanjeeev commented Nov 21, 2025 •

edited

Loading

codecov bot commented Nov 21, 2025 •

edited

Loading

kumar-sanjeeev Nov 24, 2025 •

edited

Loading

thedevmystic Nov 24, 2025 •

edited

Loading

kumar-sanjeeev commented Nov 24, 2025 •

edited

Loading

kumar-sanjeeev commented Nov 26, 2025 •

edited

Loading