Export pid gains as reference

[kumar-sanjeeev]

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]

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).
⚠️ Report is 15 commits behind head on master.

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]

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

[kumar-sanjeeev]

@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);```

[kumar-sanjeeev]

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]

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.

[kumar-sanjeeev]

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]

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

[kumar-sanjeeev]

Hi @christophfroehlich,

The implementation is complete from my side. Whenever you have time to review it, please let me know if there is anything I may have missed or should improve.

[christophfroehlich]

Another quick look and question

[christophfroehlich]

This is part is not covered by tests. Can you please think of a testcase in doing so? Friending the class and checking pids_ variable might be possible.

[christophfroehlich]

do we really need this parameter? can't they always be exported, and if they are not claimed they just remain nan?