Make gains/weights of PID controller mutable.

[FranekStark]

In the current version of the PidController, the PID gains can only be set during instantiation and not changed afterward. I'm currently modeling a system in which the gains are changed during simulation.

Maybe a setter or even other input ports allowing to change the gains would be nice.

I already tried to extend PidController by deriving a child class. However, any member that could be used in order to influence or change the gains is set to private.

[sherm1]

Not sure who to assign this to -- Joe for disposition.

[jwnimmer-tri]

Would the new API proposed in the patch below solve your problem?

I think that kind of change would be uncontroversial, and we'd welcome a pull request with an implementation.

--- a/systems/controllers/pid_controller.h
+++ b/systems/controllers/pid_controller.h
@@ -148,6 +148,24 @@ class PidController : public LeafSystem<T>,
    */
   const VectorX<double>& get_Kd_vector() const { return kd_; }
 
+  /**
+   * Sets the proportional gain vector.
+   * The new vector must be the same size as the existing vector.
+   */
+  void set_Kp(const VectorX<double>& Kp);
+
+  /**
+   * Sets the integral gain vector.
+   * The new vector must be the same size as the existing vector.
+   */
+  void set_Ki(const VectorX<double>& Ki);
+
+  /**
+   * Sets the derivative gain vector.
+   * The new vector must be the same size as the existing vector.
+   */
+  void set_Kd(const VectorX<double>& Kd);
+
   /**
    * Sets the integral part of the PidController to @p value.
    * @p value must be a column vector of the appropriate size.

[FranekStark]

@jwnimmer-tri yes that would ofc be a nice start. However, in my case, I also needed the Gains as input ports. I guess that this stays in conflict with the proposed and current API, right?

[jwnimmer-tri]

So for gains as input ports, I'm not sure whether it would make more sense to tack those onto the current class (e.g., document that when a gain port is connected its value will trump the in-class constant), or to add a distinct class class VariableGainPidController(LeafSystem) (with no built-in constants).

Looking at https://www.mathworks.com/help/simulink/slref/pidcontroller.html makes me think that having Kp,Ki,Kd available on input ports might be okay / consistent with existing practice.

@RussTedrake or @EricCousineau-TRI do you have any votes on which route we should take?

[FranekStark]

Adding them as input ports would lead to a runtime error if there is nothing connected to them, right? In that case, I would say it’s an overkill to add the ports to the existing class, as that would trigger a lot of errors in current code.

[jwnimmer-tri]

Input ports are allowed to be disconnected. If we added ports to the existing class, then the Calc functions would first check if the port was connected or not, and when not would fall back to using the constants. In other words, using the ports would be opt-in.

[EricCousineau-TRI]

Gains as optional input ports sounds great. Some options:

• Add a flag (default true or false) to actually declare the ports (if we're concerned about clutter)
• Change constructor to take optional<T> for the gains. If nullopt, then make the input port value required.

FYI @FranekStark For optional input ports as Jeremy mentioned, see HasValue: https://drake.mit.edu/doxygen_cxx/classdrake_1_1systems_1_1_input_port.html#a5536b94a4642fa4cf47164437dc66ae8 Example usage: https://github.com/RobotLocomotion/drake/blob/163c13106106dc7e3968d276bbbd27ad852327e5/manipulation/kinova_jaco/jaco_command_sender.cc#L27-L31

[jwnimmer-tri]

Change constructor to take optional<T> for the gains. If nullopt, then make the input port value required.

I can understand the motivation behind this, but in practice I don't think optional would work out. We need to know what state dimension to declare, and if all of the gains are null then we'd be lost. So instead of making the gains optional, rather we would introduce constructor(s) where the user provided the state size only, in which case all ports are required. That's probably not worth doing in the first pass.

Add a flag (default true or false) to actually declare the ports (if we're concerned about clutter).

This seems like a good idea. So for all three existing constructors, we'd add a bool use_gain_ports = false additional argument. When false nothing would be different than currently. When true, we'd declare the extra ports and check HasValue() during the Calc functions.

[RussTedrake]

All sounds good to me! Except perhaps that if I set use_gain_ports=true, I would expect that I am definitely using the input ports, not optionally. make_gain_ports or something similar seems more consistent to me.

[jwnimmer-tri]

One other thing we need to consider in terms of API design upgrades:

When the user only ever wants to do PD control not PID control, it is vitally important that the DoCalcTimeDerivatives does not pull on (integrate) the input port. (It's a defect that it does so as of today.)

It many situations it ends up as a non-trivial runtime cost to integrate continuous state, and doing so is useless when Ki is identically zero.

Today we could fix that easily by checking in the constructor whether Ki is identically zero. The question becomes more subtle if we start allowing Ki to be set from places other than the constructor.

It's certainly a surmountable problem, we just need to be a little bit careful.