Over the course of some testing yesterday and today, 449 has uncovered a rather large bug in the java WPILib PIDController object.
The bug concerns the getAvgError() method, and the setSetpoint() method. It occurs as follows:
The getAvgError() method, which is used by the onTarget() method to check if a PID has reached its target, calculates the error (setpoint - position) averaged over the last n iterations of the main loop. The values of the error for each iteration of the loop are stored in a buffer of length n. The sum of the elements in the buffer is stored separately. Each time through the main loop, the main calculate() method of the PID controller object adds the current error to the buffer, adds the value of the current error to the buffer sum, and then, if the buffer has become longer than length n, removes a value from the end of the buffer and subtracts its value from the sum.
Unfortunately, the setSetpoint() method clears the buffer but does not reset the buffer sum to 0! The result of this is that, every time a PID controller’s setpoint is changed, any subsequent values returned by getAvgError() will be increased, irreversibly, by the current value of getAvgError().
The upshot of this is that it rapidly renders onTarget() useless for determining if you have, indeed, reached your setpoint.
Edit: Problem was actually in setSetpoint(), not enable(). Fixed to reflect this.
Actually there was a worse bug in that routine that we discovered last week, and it’s likely the actual cause of your problem. The bug was that they were managing their linked list of error values incorrectly - by using push and pop. The result was that the large errors from the beginning of the routine were never pushed out of the list, which caused the average error to always be high.
I checked the source code on GitHub and someone had corrected this issue on 1/7/16, and the updated WPILib that is available now has the correction in it. While the issue that you found might cause a brief issue, I suspect the real issue behind your symptom is this bug which will be fixed if you install the latest library version.
Thanks for the info, but I’m fairly sure we’ve updated since then and the effects we were seeing were consistent with the bug we found and not with the bug you describe.
And yes, you’re correct: enable doesn’t seem to clear the buffer. setSetpoint does, however (which leads to a different bug, because onTarget will never be true if you keep calling setSetpoint over and over again).
This commit seems to have addressed the bug dvanvoorst is talking about.
If there’s an issue, and you make a fix and don’t have gerrit access, go ahead and make a pull request against the allwpilib repo and I’ll push it into gerrit.
Actually that’s not the bug I was referring to. Here’s the portion of code from the “calculate” method. This is the corrected code:
// Update the buffer.
m_buf.add(m_error);
m_bufTotal += m_error;
// Remove old elements when the buffer is full.
if (m_buf.size() > m_bufLength) {
m_bufTotal -= m_buf.remove();
The original code used m_buf.push and m_buf.pop instead of add and remove, which resulted in the most recent value being popped off instead of the oldest.
I’m running version 0.1.0.201603020231 of the Java WPILib.
Ah, I should have checked the source again before posting this. We were setting the setpoint at the outset of the command along with enabling the PID controller, and I had conflated which method caused the problem.
enable() does not clear the buffer at all.
setSetpoint() clears the buffer, but does not clear m_buftotal.
I have to agree with Oblarg that the failure to clear the accumulated buffer sum is a big problem. That’s a flaw in the plan of never actually re-calculating the sum of the buffered entries - it’s easy to introduce a bug like this when you forget to zero out the total when you empty the buffer.
m_bufTotal should be set to 0 in setSetpoint right after doing the m_buf.clear();
The documentation should also probably specify what each of these functions should and should not do (is enable intended to clear the error? should setsetpoint always force onTarget to be false).
He misspoke - he meant setSetpoint and has corrected his original post.
setSetpoint doesn’t have to force onTarget to be false because it clears the buffer - which makes isAvgErrorValid() false, which forces onTarget() to be false.
However, I’d be interested in seeing isAvgErrorValid() being changed a bit to not return true until your buffer is full. I’ve had issues with OnTarget being true because of one value in the buffer when it first started up, even though I asked for a 20 reading average.
I’d like to propose changing this:
