[pid_controller] Fix logic for feedforward_mode with single reference interface

[Juliaj]

Address #1270. The scope of the fix only covers single reference interface. The changes for two reference interfaces will be covered in different PR.

Testing of the code changes was done with ros-controls/ros2_control_demos#710.

[codecov]

Codecov Report

Attention: Patch coverage is 88.46154% with 3 lines in your changes missing coverage. Please review.

Project coverage is 84.27%. Comparing base (28845e6) to head (db7433c).
Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
pid_controller/src/pid_controller.cpp	50.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1520      +/-   ##
==========================================
+ Coverage   84.26%   84.27%   +0.01%     
==========================================
  Files         123      123              
  Lines       11358    11380      +22     
  Branches      961      964       +3     
==========================================
+ Hits         9571     9591      +20     
- Misses       1471     1473       +2     
  Partials      316      316              

Flag	Coverage Δ
unittests	84.27% <88.46%> (+0.01%)	⬆️

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.cpp	100.00% <100.00%> (ø)
pid_controller/test/test_pid_controller.hpp	84.78% <ø> (ø)
pid_controller/src/pid_controller.cpp	67.39% <50.00%> (-1.03%)	⬇️

... and 4 files with indirect coverage changes

[christophfroehlich]

TBH, I don't understand the use-case for the set_feedforward_control service, but your change looks correct. Thanks for fixing this.

Would you mind writing a test for both branches, with a non-zero feedforward_gain?

[Juliaj]

Would you mind writing a test for both branches, with a non-zero feedforward_gain?

I added a test case to cover the single interface. For the use case of two reference interfaces, I need some time to review the current code especially around measured_state_values_ handling. I would also like to update some of the existing tests with more validation steps. I can do these in a follow up PR.

[Juliaj]

@catcracker, this is to address the suggestions you made in #1260 (comment). If you have some bandwidth, could you review and give it a spin?

[christophfroehlich]

Would you mind writing a test for both branches, with a non-zero feedforward_gain?

I added a test case to cover the single interface. For the use case of two reference interfaces, I need some time to review the current code especially around measured_state_values_ handling. I would also like to update some of the existing tests with more validation steps. I can do these in a follow up PR.

sure, let's do that. A simple request for testing the actual command value without feedforward. (or is there another existing test, but I can't find it now).

[christophfroehlich]

should we add a test with feedforward_mode_type::OFF as well?