No Anti-Windup Logic in PIDLoop

[KrazyKerbalnaut]

In the PIDLoop function, the kerboscript equivalent code has no logic for anti-windup on the integram term. There is minimum and maximum output logic, but it would be nice if we could also set parameters for anti-windup. The code for such logic would be as follows:

...
if Ki <> 0 {
     set ITerm to (ErrorSum + Error * dt) * Ki.

     if ITerm > KiMax set ITerm to KiMax.
     if ITerm < KiMin set ITerm to KiMin.
}
...

And then edit this maximum/maximum section to:

...
if Output > MaxOutput {
    // we already took care of ITerm windup in previous section
    set Output to MaxOutput.
} else if Output < MinOutput {
    // we already took care of ITerm windup in previous section
    set Output to MinOutput.
}
...

This reduces the number of math operations as well, retains the output limiting function and clamps the integral term. It also lets the user select a clamping value for the max/min integral term independent of the max/min output functions of the controller as a whole.

[nuggreat]

There is anti I wind up logic in the kerboScript equivalent code it admittedly only kicks in when the out put is larger than the defined min and max but it is there.

See this section for where the clamping of the I term is imposed

    // if the output goes beyond the max/min limits, reset it and adjust ITerm.
    if Output > MaxOutput {
        set Output to MaxOutput.
        // adjust the value of ITerm as well to prevent the value
        // from winding up out of control.
        if (Ki <> 0) and (LastSampleTime < sampleTime) {
            set ITerm to Output - min(Pterm + DTerm, MaxOutput).
        }
    }
    else if Output < MinOutput {
        set Output to MinOutput.
        // adjust the value of ITerm as well to prevent the value
        // from winding up out of control.
        if (Ki <> 0) and (LastSampleTime < sampleTime) {
            set ITerm to Output - max(Pterm + DTerm, MinOutput).
        }
    }

Part of the point of clamping the I term this way is so that the limit on it is dynamic and as a good clamp when P and D are low is not going to be a good clamp when P and D are high.

But the current clamping for the I term does mean that you MUST define a max and min for the PID if you want to the I clamping to function at all.

Also changing this is that altering the PID logic in such a fundamental way has a good chance to break scripts that rely on the way PIDs currently function and are tuned with that in mind.

As for reducing the math you only end up saving 1 addition, 1 subtraction, 1 function call, and 1 comparison which in the context of kOS is a functionally insignificant savings compared to many many other places in the language.

[KrazyKerbalnaut]

Yes, I did see that, and I agree that it would break a lot of tunings and the way scripts currently work. Using a new suffix without changing the underlying code for people who don't use the suffix would work though. Then if the suffix, PIDLoop:KIMAX, PIDLoop:KIMIN are zero, functionality would remain the same. Since it is programmed in C#, it could be added as a subclass that is only called if those two suffixes are <> 0. Thus preserving old scripts and able to run in line with current code. Or when the controller gets instantiated, if the terms for ItermMax/Min get used, it could create the object object with the newer code so as not to break the current implementation, kind of like other classes that get created to not break peoples implementation.

With the current implementation, the I term gets "lost in time" so to speak.

My implementation is the following (I've removed parameters to keep it simple. We all know it needs them).
Also note that this is just the basic pseudo code and the global variables should be replaced with other terms.

SET IArea TO 0.0.
SET ErrorLast TO 0.0.
SET Output TO 0.0.

FUNCTION pidController {
    // if time has changed since last call, iterate the loop. Otherwise, return the last value
    IF deltaT > 0 {
        // find current error
        LOCAL Error TO setPoint - processVariable.

        // find change in error
        LOCAL deltaError TO Error - ErrorLast.
    
        // calculate the terms in this current time frame
        LOCAL PTerm TO Error * Kp.
        SET IArea TO (IArea + (Error * deltaT)) * Ki.
        LOCAL DTerm TO (deltaError / deltaT) * Kd.

        // we need to know what the delta error will be for the next iteration
        SET ErrorLast TO Error.

        // this is where we should be calculating the anti windup for the integral term
        // we shouldn't care if the whole loops value exceeds the max output THEN clamp the integral term
        IF ITerm > KiMax AND KiMax <> 0 {
            SET ITerm TO KiMax.
        } ELSE IF ITerm < KiMin AND KiMin <> 0 {
            SET ITerm TO KiMin.
        }

        // then we can calculate the output
        LOCAL Output TO PTerm + ITerm + DTerm.

        // this should be the last step to check if we exceed the loops max value as a whole
        IF Output > PIDmax AND PIDmax <> 0 {
            SET Output TO PIDmax.
        } ELSE IF Output < PIDmin AND PIDmin <> 0 {
            SET Output TO PIDmin.
        }
    } 

    RETURN Output.
}

[nuggreat]

I still don't understand why you want to change it as my understanding was that the current way kOS handles the problem of I windup in PIDs is better than than just using a hard clamp though I could be wrong about that as I only have a limited understanding of the underlying science to PIDs.

Though what ever else this will likely be resolved by PR #2922 should that go into kOS as it is currently written.

[KrazyKerbalnaut]

Yeah, I have been running some tests and I do get more stability with the script I'm running. It just takes too long for the I term to level after the whole loop returns to a value less than when clamping as a whole unit. I actually even searched for requested for it haha. Thanks for pointing that out. I couldn't find them initially. I guess I didn't try as hard as I thought I had. Thank you :)