From 95234e1baa12c61aa6fbfefb7010e2ea54b52368 Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Tue, 3 Jan 2017 10:48:10 -0800 Subject: [PATCH] Increase stability of integration for PID controller --- .../chttp2/transport/chttp2_transport.c | 16 ++++++------- .../ext/transport/chttp2/transport/internal.h | 1 - src/core/lib/transport/pid_controller.c | 24 +++++++++++++++---- src/core/lib/transport/pid_controller.h | 10 ++++++-- test/core/transport/pid_controller_test.c | 11 ++++----- 5 files changed, 40 insertions(+), 22 deletions(-) diff --git a/src/core/ext/transport/chttp2/transport/chttp2_transport.c b/src/core/ext/transport/chttp2/transport/chttp2_transport.c index 8cce283b7ac..5f3f3d855be 100644 --- a/src/core/ext/transport/chttp2/transport/chttp2_transport.c +++ b/src/core/ext/transport/chttp2/transport/chttp2_transport.c @@ -252,8 +252,7 @@ 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, 4, 4, 0); - t->log2_bdp_guess = log2(DEFAULT_WINDOW); + grpc_pid_controller_init(&t->pid_controller, log2(DEFAULT_WINDOW), 4, 4, 0); grpc_chttp2_goaway_parser_init(&t->goaway_parser); grpc_chttp2_hpack_parser_init(&t->hpack_parser); @@ -1903,21 +1902,22 @@ static void read_action_locked(grpc_exec_ctx *exec_ctx, void *tp, if (memory_pressure > 0.8) { target *= 1 - GPR_MIN(1, (memory_pressure - 0.8) / 0.1); } - bdp_error = target > 0 ? log2(target) - t->log2_bdp_guess - : GPR_MIN(0, -t->log2_bdp_guess); + bdp_error = + target > 0 + ? log2(target) - grpc_pid_controller_last(&t->pid_controller) + : GPR_MIN(0, -grpc_pid_controller_last(&t->pid_controller)); gpr_timespec now = gpr_now(GPR_CLOCK_MONOTONIC); gpr_timespec dt_timespec = gpr_time_sub(now, t->last_pid_update); double dt = (double)dt_timespec.tv_sec + dt_timespec.tv_nsec * 1e-9; if (dt > 3) { grpc_pid_controller_reset(&t->pid_controller); } - t->log2_bdp_guess += + double log2_bdp_guess = grpc_pid_controller_update(&t->pid_controller, bdp_error, dt); - t->log2_bdp_guess = GPR_CLAMP(t->log2_bdp_guess, -5, 21); gpr_log(GPR_DEBUG, "%s: err=%lf cur=%lf pressure=%lf target=%lf", - t->peer_string, bdp_error, t->log2_bdp_guess, memory_pressure, + t->peer_string, bdp_error, log2_bdp_guess, memory_pressure, target); - update_bdp(exec_ctx, t, pow(2, t->log2_bdp_guess)); + update_bdp(exec_ctx, t, pow(2, log2_bdp_guess)); t->last_pid_update = now; } GRPC_CHTTP2_UNREF_TRANSPORT(exec_ctx, t, "keep_reading"); diff --git a/src/core/ext/transport/chttp2/transport/internal.h b/src/core/ext/transport/chttp2/transport/internal.h index e320dd091da..8b7c040358d 100644 --- a/src/core/ext/transport/chttp2/transport/internal.h +++ b/src/core/ext/transport/chttp2/transport/internal.h @@ -330,7 +330,6 @@ struct grpc_chttp2_transport { /* bdp estimator */ grpc_bdp_estimator bdp_estimator; grpc_pid_controller pid_controller; - double log2_bdp_guess; grpc_closure start_bdp_ping_locked; grpc_closure finish_bdp_ping_locked; gpr_timespec last_bdp_ping_finished; diff --git a/src/core/lib/transport/pid_controller.c b/src/core/lib/transport/pid_controller.c index 3cef225d4ba..ba568745039 100644 --- a/src/core/lib/transport/pid_controller.c +++ b/src/core/lib/transport/pid_controller.c @@ -34,7 +34,9 @@ #include "src/core/lib/transport/pid_controller.h" void grpc_pid_controller_init(grpc_pid_controller *pid_controller, - double gain_p, double gain_i, double gain_d) { + 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; @@ -48,10 +50,22 @@ void grpc_pid_controller_reset(grpc_pid_controller *pid_controller) { double grpc_pid_controller_update(grpc_pid_controller *pid_controller, double error, double dt) { - pid_controller->error_integral += error * dt; + /* integrate error using the trapezoid rule */ + pid_controller->error_integral += + dt * (pid_controller->last_error + error) * 0.5; 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 new_control_value = pid_controller->last_control_value + + dt * (pid_controller->last_dc_dt + dc_dt) * 0.5; pid_controller->last_error = error; - return dt * (pid_controller->gain_p * error + - pid_controller->gain_i * pid_controller->error_integral + - pid_controller->gain_d * diff_error); + pid_controller->last_dc_dt = dc_dt; + pid_controller->last_control_value = new_control_value; + return new_control_value; +} + +double grpc_pid_controller_last(grpc_pid_controller *pid_controller) { + return pid_controller->last_control_value; } diff --git a/src/core/lib/transport/pid_controller.h b/src/core/lib/transport/pid_controller.h index 059b5b08346..dd2b1200525 100644 --- a/src/core/lib/transport/pid_controller.h +++ b/src/core/lib/transport/pid_controller.h @@ -47,18 +47,24 @@ typedef struct { double gain_d; double last_error; double error_integral; + double last_control_value; + double last_dc_dt; } grpc_pid_controller; /** Initialize the controller */ void grpc_pid_controller_init(grpc_pid_controller *pid_controller, - double gain_p, double gain_i, double gain_d); + double initial_control_value, double gain_p, + double gain_i, double gain_d); /** Reset the controller: useful when things have changed significantly */ void grpc_pid_controller_reset(grpc_pid_controller *pid_controller); /** Update the controller: given a current error estimate, and the time since - the last update, returns a delta to the control value */ + the last update, returns a new control value */ double grpc_pid_controller_update(grpc_pid_controller *pid_controller, double error, double dt); +/** Returns the last control value calculated */ +double grpc_pid_controller_last(grpc_pid_controller *pid_controller); + #endif diff --git a/test/core/transport/pid_controller_test.c b/test/core/transport/pid_controller_test.c index 9614983b007..3935a25322b 100644 --- a/test/core/transport/pid_controller_test.c +++ b/test/core/transport/pid_controller_test.c @@ -45,7 +45,7 @@ static void test_noop(void) { gpr_log(GPR_INFO, "test_noop"); grpc_pid_controller pid; - grpc_pid_controller_init(&pid, 1, 1, 1); + grpc_pid_controller_init(&pid, 0, 1, 1, 1); } static void test_simple_convergence(double gain_p, double gain_i, double gain_d, @@ -55,15 +55,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, 0.2, 0.1, 0.1); - - double current = start; + grpc_pid_controller_init(&pid, start, 0.2, 0.1, 0.1); for (int i = 0; i < 1000; i++) { - current += grpc_pid_controller_update(&pid, set_point - current, 1); + grpc_pid_controller_update(&pid, set_point - grpc_pid_controller_last(&pid), + 1); } - GPR_ASSERT(fabs(set_point - current) < 0.1); + GPR_ASSERT(fabs(set_point - grpc_pid_controller_last(&pid)) < 0.1); GPR_ASSERT(fabs(pid.error_integral) < 0.1); }