outputSum typed as int causing float rounding issues

[halmos]

I've been having a lot of trouble with QuickPID after a recent update and finally was able to track down the source of the issue.

QuickPID/src/QuickPID.h

Line 155 in 11a10e1

int outputSum;

outputSum is set to type int. This causes the compute method to drop all the fractional values in the calculation:

QuickPID/src/QuickPID.cpp

Lines 68 to 73 in 11a10e1

	outputSum += iTerm; // include integral amount
	if (outputSum > outMax) outputSum -= outputSum - outMax; // early integral anti-windup at outMax
	else if (outputSum < outMin) outputSum += outMin - outputSum; // early integral anti-windup at outMin
	outputSum = constrain(outputSum, outMin, outMax); // integral anti-windup clamp
	outputSum = constrain(outputSum - pmTerm, outMin, outMax); // include pmTerm and clamp
	*myOutput = constrain(outputSum + peTerm + dmTerm - deTerm, outMin, outMax); // totalize, clamp and drive the output

So any fine grain PID value tuning is mostly lost.

I believe this was intended to be of type float?

[halmos]

Also, one other note, but don't the lines:

QuickPID/src/QuickPID.cpp

Lines 69 to 70 in 11a10e1

	if (outputSum > outMax) outputSum -= outputSum - outMax; // early integral anti-windup at outMax
	else if (outputSum < outMin) outputSum += outMin - outputSum; // early integral anti-windup at outMin

reproduce the same functionality as the constrain method used in the next line?

QuickPID/src/QuickPID.cpp

Line 71 in 11a10e1

outputSum = constrain(outputSum, outMin, outMax); // integral anti-windup clamp

[Dlloydev]

Thanks for your comments.

Yes, outputSum was intended to be float ... also outMin, outMax, and error. I'll change these to float and publish a new version ASAP.

Regarding anti-windup, there is a subtle difference in the two methods ... the early anti-windup was derived from the discussion here.

In a future version I may consider a feature to adjust the amount of anti-windup to apply. I read somewhere that a certain percentage will work well with many systems. For some systems, some windup can be tolerated and beneficial.

[halmos]

Great - thanks maintaining this library!

[halmos]

Regarding anti-windup, there is a subtle difference in the two methods ... the early anti-windup was derived from the discussion here.

Are you referring to this comment?:
http://brettbeauregard.com/blog/2011/04/improving-the-beginner%e2%80%99s-pid-reset-windup/#comment-18721

[Dlloydev]

Yes, that's it.

[halmos]

I think what that comment is proposing is a bit different than your implementation. In the sample code from the comment:

iTerm += ki * error;
…
output = pTerm + iTerm + dTerm;
if (output > maxLimit) iTerm -= output – maxLimit;
output = maxLimit;

Here, iTerm accumulates over time. However in your code, the iTerm is reset (it does not itself accumulate), but instead is added to the outputSum which is accumulating.

In the the comment code, they are doing the 'early' clamp on the i term in order to prevent it from growing too large. I don't think that applies in your code which is doing things a bit differently.

Below, i've annotated your clamping code with example values in the comments. You can see that outputSum -= outputSum - outMax is effectively the same result as constrain(outputSum, outMin, outMax):

// ki = 2;
// error = 10;
// outputSum = 90;
// outMax = 100;

 iTerm = ki * error;  // 2* 10 = 20
...
outputSum += iTerm;  // outputSum = 90 + 20 = 110;
if (outputSum > outMax) outputSum -= outputSum - outMax; // outputSum = 110 - (110 - 100) = 100;
...
outputSum = constrain(outputSum, outMin, outMax);  // outputSum = 100;
...

if you wanted to clamp the integral value separately, I think you would need to accumulate the iTerm value at each compute rather than reseting it.

[Dlloydev]

Thanks for looking at this, much appreciated. Yes, I agree with your analysis. The way it is now, for the first iteration of compute, the outputSum consists of only the iTerm, then the early anti-windup progressively and quickly has less effect on subsequent iterations as the iTerm reaches zero and the outputSum accumulates based on all of the terms.

For a future revision, I'll run some tests where the iTerm is accumulated separately and also test a variable anti-windup feature.

Regarding the rounding issues, I'll get a new revision out this weekend ... it'll have an update to the analogWrite files, one of the examples and to the variables that need to be changed to float.

[TomPcz]

Since outMin and outMax are now floats, please, change SetOutputLimits to accept floats as well.

[Dlloydev]

Done (QuickPID2.4.9)

[Dlloydev]

Published new version QuickPID 2.5.0 with an improved anti-windup feature.