PIDController: setSetpoint causes onTarget to be false #29

Open
virtuald opened this Issue May 15, 2016 · 4 comments

Projects

None yet

4 participants

@virtuald
Member

The core of the problem is that onTarget is calculated using the err buffer, which gets cleared when a new setpoint is set. This behavior is incorrect -- onTarget should not depend on the history of the setpoints (which is how error is calculated), but instead it should only depend on the history of the inputs as that's really the question a user is asking when calling onTarget: "is the input [history] close to where I want it to be?"

To that end, I propose that a buffer of inputs is kept in addition to a buffer of errors, and use the inputs to calculate onTarget instead. This buffer would only be cleared when the PIDController is disabled.

@calcmogul
Member

Actually, we only need to keep a buffer of inputs. Here's some math:

a, b, c, and d are inputs. s is the setpoint. "s - a" is the error with respect to a.

Currently, we average the errors in GetAvgError():
avgError = ((s - a) + (s - b) + (s - c) + (s - d)) / 4
= (-(a - s) + -(b - s) + -(c - s) + -(d - s)) / 4
= -((a - s) + (b - s) + (c - s) + (d - s)) / 4
= -(a + b + c + d - 4 * s) / 4
= -(a + b + c + d - 4 * s) / 4
= -(a + b + c + d - 4 * s) / 4
= (4 * s - (a + b + c + d)) / 4
= s - (a + b + c + d) / 4

Therefore, GetAvgError() could instead return the current setpoint minus the input average. This would make anything using GetAvgError(), like OnTarget(), more robust to setpoint changes.

@PeterMitrano
Contributor
PeterMitrano commented May 18, 2016 edited

👍 for application of some basic math.

@calcmogul calcmogul added a commit to calcmogul/allwpilib that referenced this issue Jul 3, 2016
@calcmogul calcmogul PIDController queue now stores inputs instead of errors
Closes #29.
29ada7a
@calcmogul calcmogul added a commit to calcmogul/allwpilib that referenced this issue Jul 3, 2016
@calcmogul calcmogul PIDController queue now stores inputs instead of errors
Closes #29.
bfb6b11
@calcmogul calcmogul added a commit to calcmogul/allwpilib that referenced this issue Jul 4, 2016
@calcmogul calcmogul PIDController queue now stores inputs instead of errors
Closes #29.
3a9275e
@calcmogul calcmogul added a commit to calcmogul/allwpilib that referenced this issue Jul 12, 2016
@calcmogul calcmogul PIDController queue now stores inputs instead of errors
Closes #29.
a6a7aa8
@calcmogul calcmogul added a commit to calcmogul/allwpilib that referenced this issue Jul 29, 2016
@calcmogul calcmogul PIDController queue now stores inputs instead of errors
Closes #29.
0d54dfe
@PeterJohnson PeterJohnson closed this in #138 Aug 13, 2016
@virtuald
Member

This was closed in #138, which was reverted... so I presume that this bug still exists? This should be reopened in that case.

@333fred 333fred reopened this Nov 29, 2016
@calcmogul
Member

How should we go about fixing it this time? Apparently using doubles in a large FIR filter (moving average) causes rounding errors due to so many additions. We could approximate it with an IIR filter and avoid that problem. Using a cutoff frequency isn't necessarily intuitive, but it provides more tuning hints than the size of the FIR filter does. Teams would probably tune both by guessing and checking anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment