Add enable_feedforward parameter

[pascalau]

Added a parameter to enable feedforward of the controller during configuration of the controller.
#1270 also mentions that there is currently no option to enable the feedforward gain with the exception of the service which can be unreliable to do from a launchfile.
Another option would be to set the state depending on the value of the feedforward gain but this could cause unexpected behaviour for users that have the value set but expect current behaviour.

[christophfroehlich]

First of all, thanks for your contribution.

With the current implementation, the parameter is only used at configure() transition. But from the naming and the fact that it is not read_only, I can imagine that a user wants to use this parameter to change the behavior while the controller is active. (what would be easily possible, but would make the service set_feedforward_control obsolete).

[pascalau]

I agree and I also think it would make sense to change this parameter to affect the controller during operation which would make tuning with dynamic_reconfigure or similar tools much nicer. Also setting a parameter already is a service call so it would be fairly similar with respect to set_feedback_control. If desired I will rewrite it that it will change the boolean in the update_parameters function as an writefromRT. Maybe it would also make sense to change the variable control_mode_ to an atomic boolean to reduce the overhead that comes from the RealtimeBuffer.

[christophfroehlich]

I'll bring that to tomorrow's WG meeting, let's see what the original author @destogl thinks about that

[pascalau]

I updated it so it can be changed during runtime. After the WG meeting you can tell me which option should be merged and if the underlying variable type can also be changed or not.

[christophfroehlich]

We came to the conclusion that we can simplify this and remove the service at all. But we shouldn't do that on jazzy maybe, so we have to do that in two steps:

• Add deprecation warnings to this PR for the service call and the new enable parameter (e.g., will be removed for the upcoming ROS 2 Kilted Kaiju release)
• Create a new PR removing the stuff, which will be held until we branch for jazzy (somewhen before the K-turtle release).

[pascalau]

How exactly should I add the deprecation warning?
What I would do:

• Error in rosout
• Deprecation warning in userdoc

Are there any other ways I am missing? Since the interaction of the user is via the service call there are no functions that can be marked deprecated.

[pascalau]

I added deprecation warnings as outlined and also removed the enum feedforward_mode. Currently the control_mode_ variable is still needed since both the parameter and the service need to control the behaviour. After deprecating the service one only needs to change the if statement to directly use the parameter instead of the control_mode_ variable and remove the unit test.

[christophfroehlich]

Thanks for the follow-up. My suggestions:

• If we remove the feedforward_mode_type enum, then we should rename the control_mode_ variable to be more descriptive. feedforward_mode_enabled_ for example.
• We should not use the enable_feedforward parameter later than on_configure. In the future, we want to remove this as well and just set the behavior by setting the feedforward_gain to zero or nonzero. If we use this parameter as suggested now, then we again have to deprecate it. Let it just control the behavior at startup (a temporary fix), and mark it already as deprecated if set to false (which is the default value). So users will switch to use the feedforward_gain directly.

Does that make sense for you?

[codecov]

Codecov Report

Attention: Patch coverage is 91.66667% with 1 line in your changes missing coverage. Please review.

Project coverage is 85.71%. Comparing base (0095ef1) to head (f5789b1).
Report is 5 commits behind head on master.

Files with missing lines	Patch %	Lines
pid_controller/src/pid_controller.cpp	75.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1553      +/-   ##
==========================================
+ Coverage   85.60%   85.71%   +0.11%     
==========================================
  Files         123      123              
  Lines       11873    11896      +23     
  Branches     1016     1015       -1     
==========================================
+ Hits        10164    10197      +33     
+ Misses       1384     1377       -7     
+ Partials      325      322       -3     

Flag	Coverage Δ
unittests	85.71% <91.66%> (+0.11%)	⬆️

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	86.13% <ø> (ø)
...roller/test/test_pid_controller_dual_interface.cpp	100.00% <100.00%> (ø)
..._controller/test/test_pid_controller_preceding.cpp	100.00% <ø> (ø)
pid_controller/src/pid_controller.cpp	73.64% <75.00%> (+1.44%)	⬆️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

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

[pascalau]

Thanks for the clarification. I thought you only planned to deprecate the service.

Thanks for the follow-up. My suggestions:

• If we remove the feedforward_mode_type enum, then we should rename the control_mode_ variable to be more descriptive. feedforward_mode_enabled_ for example.

I agree and will rename the variable.

• We should not use the enable_feedforward parameter later than on_configure. In the future, we want to remove this as well and just set the behavior by setting the feedforward_gain to zero or nonzero. If we use this parameter as suggested now, then we again have to deprecate it. Let it just control the behavior at startup (a temporary fix), and mark it already as deprecated if set to false (which is the default value). So users will switch to use the feedforward_gain directly.

I will add deprecation warnings to the parameter as well in the description of the parameter and the documentation. I would also add a disclaimer to the feedforward_gain parameter that in the forseeable future this will be used to trigger the feedforward term. This should allow users to zero this parameter if they unknowingly have it active and assume it to be running while this is not the case. Regarding the startup behaviour I would disagree since the functionality is already there to enable or disable it during operation while the current behaviour with the service is unchanged. This already allows users to fully control the behaviour via parameters as it will be in the future. If this is ok I would leave the functionality as is and just mark it as deprecated.

[christophfroehlich]

looks good, just some comments on the warning in the docs