Add clamping to pid controller, make arguments more readable

src/core/ext/transport/chttp2/transport/chttp2_transport.c

@@ -252,7 +252,15 @@ static void init_transport(grpc_exec_ctx *exec_ctx, grpc_chttp2_transport *t,
   grpc_bdp_estimator_init(&t->bdp_estimator);
   t->last_bdp_ping_finished = gpr_now(GPR_CLOCK_MONOTONIC);
   t->last_pid_update = t->last_bdp_ping_finished;
-  grpc_pid_controller_init(&t->pid_controller, log2(DEFAULT_WINDOW), 4, 4, 0);
+  grpc_pid_controller_init(
+      &t->pid_controller,
+      (grpc_pid_controller_args){.gain_p = 4,
+                                 .gain_i = 8,
+                                 .gain_d = 0,
+                                 .initial_control_value = log2(DEFAULT_WINDOW),
+                                 .min_control_value = -1,
+                                 .max_control_value = 22,
+                                 .integral_range = 10});
 
   grpc_chttp2_goaway_parser_init(&t->goaway_parser);
   grpc_chttp2_hpack_parser_init(&t->hpack_parser);

src/core/lib/transport/pid_controller.c

@@ -32,14 +32,12 @@
  */
 
 #include "src/core/lib/transport/pid_controller.h"
+#include <grpc/support/useful.h>
 
 void grpc_pid_controller_init(grpc_pid_controller *pid_controller,
-                              double initial_control_value, double gain_p,
-                              double gain_i, double gain_d) {
-  pid_controller->last_control_value = initial_control_value;
-  pid_controller->gain_p = gain_p;
-  pid_controller->gain_i = gain_i;
-  pid_controller->gain_d = gain_d;
+                              grpc_pid_controller_args args) {
+  pid_controller->args = args;
+  pid_controller->last_control_value = args.initial_control_value;
   grpc_pid_controller_reset(pid_controller);
 }
 
@@ -53,13 +51,20 @@ double grpc_pid_controller_update(grpc_pid_controller *pid_controller,
   /* integrate error using the trapezoid rule */
   pid_controller->error_integral +=
       dt * (pid_controller->last_error + error) * 0.5;
+  pid_controller->error_integral = GPR_CLAMP(
+      pid_controller->error_integral, -pid_controller->args.integral_range,
+      pid_controller->args.integral_range);
   double diff_error = (error - pid_controller->last_error) / dt;
   /* calculate derivative of control value vs time */
-  double dc_dt = pid_controller->gain_p * error +
-                 pid_controller->gain_i * pid_controller->error_integral +
-                 pid_controller->gain_d * diff_error;
+  double dc_dt = pid_controller->args.gain_p * error +
+                 pid_controller->args.gain_i * pid_controller->error_integral +
+                 pid_controller->args.gain_d * diff_error;
+  /* and perform trapezoidal integration */
   double new_control_value = pid_controller->last_control_value +
                              dt * (pid_controller->last_dc_dt + dc_dt) * 0.5;
+  new_control_value =
+      GPR_CLAMP(new_control_value, pid_controller->args.min_control_value,
+                pid_controller->args.max_control_value);
   pid_controller->last_error = error;
   pid_controller->last_dc_dt = dc_dt;
   pid_controller->last_control_value = new_control_value;

src/core/lib/transport/pid_controller.h

@@ -45,16 +45,23 @@ typedef struct {
   double gain_p;
   double gain_i;
   double gain_d;
+  double initial_control_value;
+  double min_control_value;
+  double max_control_value;
+  double integral_range;
+} grpc_pid_controller_args;
+
+typedef struct {
   double last_error;
   double error_integral;
   double last_control_value;
   double last_dc_dt;
+  grpc_pid_controller_args args;
 } grpc_pid_controller;
 
 /** Initialize the controller */
 void grpc_pid_controller_init(grpc_pid_controller *pid_controller,
-                              double initial_control_value, double gain_p,
-                              double gain_i, double gain_d);
+                              grpc_pid_controller_args args);
 
 /** Reset the controller: useful when things have changed significantly */
 void grpc_pid_controller_reset(grpc_pid_controller *pid_controller);

test/core/transport/pid_controller_test.c

@@ -33,6 +33,7 @@
 
 #include "src/core/lib/transport/pid_controller.h"
 
+#include <float.h>
 #include <math.h>
 
 #include <grpc/support/alloc.h>
@@ -45,7 +46,14 @@
 static void test_noop(void) {
   gpr_log(GPR_INFO, "test_noop");
   grpc_pid_controller pid;
-  grpc_pid_controller_init(&pid, 0, 1, 1, 1);
+  grpc_pid_controller_init(
+      &pid, (grpc_pid_controller_args){.gain_p = 1,
+                                       .gain_i = 1,
+                                       .gain_d = 1,
+                                       .initial_control_value = 1,
+                                       .min_control_value = DBL_MIN,
+                                       .max_control_value = DBL_MAX,
+                                       .integral_range = DBL_MAX});
 }
 
 static void test_simple_convergence(double gain_p, double gain_i, double gain_d,
@@ -55,7 +63,14 @@ static void test_simple_convergence(double gain_p, double gain_i, double gain_d,
           "start=%lf",
           gain_p, gain_i, gain_d, dt, set_point, start);
   grpc_pid_controller pid;
-  grpc_pid_controller_init(&pid, start, 0.2, 0.1, 0.1);
+  grpc_pid_controller_init(
+      &pid, (grpc_pid_controller_args){.gain_p = gain_p,
+                                       .gain_i = gain_i,
+                                       .gain_d = gain_d,
+                                       .initial_control_value = start,
+                                       .min_control_value = DBL_MIN,
+                                       .max_control_value = DBL_MAX,
+                                       .integral_range = DBL_MAX});
 
   for (int i = 0; i < 1000; i++) {
     grpc_pid_controller_update(&pid, set_point - grpc_pid_controller_last(&pid),