pidctrl_daemon() implements a variation on the classical, discrete PID

controller that tries to handle early invocations of the controller,
in other words, invocations before the expected end of the interval.
However, there were some calculation errors in this early invocation
case.  Notably, if an early invocation occurred while the error was
negative, the derivative term was off by a large amount.  One visible
effect of this error was that processes were being killed by the
virtual memory system's OOM killer when in fact there was plentiful
free memory.

Correct a couple minor errors in the sysctl descriptions, and apply
some style fixes.

Reviewed by:	jeff, markj

1 files changed, 16 insertions, 16 deletions

diff --git a/sys/kern/subr_pidctrl.c b/sys/kern/subr_pidctrl.c
index 3745ad8d7880..a601a4fa4d6e 100644
--- a/sys/kern/subr_pidctrl.c
+++ b/sys/kern/subr_pidctrl.c
@@ -59,14 +59,14 @@ pidctrl_init_sysctl(struct pidctrl *pc, struct sysctl_oid_list *parent)
 	    &pc->pc_olderror, 0, "Error value from last interval");
 	SYSCTL_ADD_INT(NULL, parent, OID_AUTO, "integral", CTLFLAG_RD,
 	    &pc->pc_integral, 0, "Accumulated error integral (I)");
-	SYSCTL_ADD_INT(NULL, parent, OID_AUTO, "derivative",
-	    CTLFLAG_RD, &pc->pc_derivative, 0, "Error derivative (I)");
+	SYSCTL_ADD_INT(NULL, parent, OID_AUTO, "derivative", CTLFLAG_RD,
+	    &pc->pc_derivative, 0, "Error derivative (D)");
 	SYSCTL_ADD_INT(NULL, parent, OID_AUTO, "input", CTLFLAG_RD,
 	    &pc->pc_input, 0, "Last controller process variable input");
 	SYSCTL_ADD_INT(NULL, parent, OID_AUTO, "output", CTLFLAG_RD,
 	    &pc->pc_output, 0, "Last controller output");
 	SYSCTL_ADD_INT(NULL, parent, OID_AUTO, "ticks", CTLFLAG_RD,
-	    &pc->pc_ticks, 0, "Last controler runtime");
+	    &pc->pc_ticks, 0, "Last controller runtime");
 	SYSCTL_ADD_INT(NULL, parent, OID_AUTO, "setpoint", CTLFLAG_RW,
 	    &pc->pc_setpoint, 0, "Desired level for process variable");
 	SYSCTL_ADD_INT(NULL, parent, OID_AUTO, "interval", CTLFLAG_RD,
@@ -96,21 +96,20 @@ pidctrl_classic(struct pidctrl *pc, int input)
 	Kid = MAX(pc->pc_Kid, 1);
 	Kdd = MAX(pc->pc_Kdd, 1);
 
-	/* Compute P (proportional error), I (integral), D (derivative) */
+	/* Compute P (proportional error), I (integral), D (derivative). */
 	pc->pc_error = error;
 	pc->pc_integral =
 	    MAX(MIN(pc->pc_integral + error, pc->pc_bound), -pc->pc_bound);
 	pc->pc_derivative = error - pc->pc_olderror;
 
 	/* Divide by inverse gain values to produce output. */
-	output = ((pc->pc_error / Kpd) +
-	    (pc->pc_integral / Kid)) +
+	output = (pc->pc_error / Kpd) + (pc->pc_integral / Kid) +
 	    (pc->pc_derivative / Kdd);
 	/* Save for sysctl. */
 	pc->pc_output = output;
 	pc->pc_input = input;
 
-	return output;
+	return (output);
 }
 
 int
@@ -121,17 +120,18 @@ pidctrl_daemon(struct pidctrl *pc, int input)
 
 	error = pc->pc_setpoint - input;
 	/*
-	 * When ticks expired we reset our variables and start a new
+	 * When ticks expires we reset our variables and start a new
 	 * interval.  If we're called multiple times during one interval
 	 * we attempt to report a target as if the entire error came at
 	 * the interval boundary.
 	 */
-	if ((u_int)(ticks - pc->pc_ticks) >= pc->pc_interval) {
+	if ((u_int)ticks - pc->pc_ticks >= pc->pc_interval) {
 		pc->pc_ticks = ticks;
 		pc->pc_olderror = pc->pc_error;
 		pc->pc_output = pc->pc_error = 0;
 	} else {
-		error = MAX(error + pc->pc_error, 0);
+		/* Calculate the error relative to the last call. */
+		error -= pc->pc_error - pc->pc_output;
 	}
 
 	/* Fetch gains and prevent divide by zero. */
@@ -139,19 +139,19 @@ pidctrl_daemon(struct pidctrl *pc, int input)
 	Kid = MAX(pc->pc_Kid, 1);
 	Kdd = MAX(pc->pc_Kdd, 1);
 
-	/* Compute P (proportional error), I (integral), D (derivative) */
-	pc->pc_error = error;
+	/* Compute P (proportional error), I (integral), D (derivative). */
+	pc->pc_error += error;
 	pc->pc_integral =
 	    MAX(MIN(pc->pc_integral + error, pc->pc_bound), 0);
-	pc->pc_derivative = error - pc->pc_olderror;
+	pc->pc_derivative = pc->pc_error - pc->pc_olderror;
 
 	/* Divide by inverse gain values to produce output. */
-	output = ((error / Kpd) +
-	    (pc->pc_integral / Kid)) +
+	output = (error / Kpd) + (pc->pc_integral / Kid) +
 	    (pc->pc_derivative / Kdd);
 	output = MAX(output - pc->pc_output, 0);
+	/* Save for sysctl. */
 	pc->pc_output += output;
 	pc->pc_input = input;
 
-	return output;
+	return (output);
 }