[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 93.33333% with 3 lines in your changes missing coverage. Please review.

Project coverage is 84.29%. Comparing base (28845e6) to head (fdfff96).
Report is 4 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.29%   +0.03%     
==========================================
  Files         123      123              
  Lines       11358    11400      +42     
  Branches      961      966       +5     
==========================================
+ Hits         9571     9610      +39     
- Misses       1471     1474       +3     
  Partials      316      316              

Flag	Coverage Δ
unittests	84.29% <93.33%> (+0.03%)	⬆️

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 5 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).

[Juliaj]

@christophfroehlich , @saikishor , I updated the code per your feedback. Could you give it another look when you have the chance ?

P.S. Additional tests have been added to #1538.

[christophfroehlich]

Thx for the latest added test

[saikishor]

LGTM
Thanks for the tests
Just a couple minor and good to be merged ;)

[Juliaj]

Thank you, @christophfroehlich and @saikishor for your review feedback. @christophfroehlich , ❤️ much appreciated for the changes you made to help merge this.