Major WPILib PIDController bug

[Oblarg]

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.

[dvanvoorst]

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.

[Oblarg]

Originally Posted by dvanvoorst

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.

[virtuald]

Originally Posted by Oblarg

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.

Well, what version are you using?

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.

[dvanvoorst]

Originally Posted by virtuald

This commit seems to have addressed the bug dvanvoorst is talking about.

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.

[virtuald]

Originally Posted by dvanvoorst

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.

Ah yes, here we go. I think I'm misreading everybody's comments tonight...

[dvanvoorst]

Originally Posted by virtuald

Ah yes, here we go. I think I'm misreading everybody's comments tonight...

Yep, that's the one!

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();

[virtuald]

Originally Posted by dvanvoorst

m_bufTotal should be set to 0 in setSetpoint right after doing the m_buf.clear();

I agree, that's a bug there.

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).

[Oblarg]

Originally Posted by virtuald

I agree, that's a bug there. Different from what Oblarg was saying (unless they meant setSetpoint instead of enable).

As I clarified above, I did indeed mean that.

[virtuald]

Originally Posted by Oblarg

As I clarified above, I did indeed mean that.

Missed it.

Also, appears that C++ has an identical error.

Python does not have this error. C# does however.

[Oblarg]

Originally Posted by dvanvoorst

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.

Agreed, this would be a very nice change.

Originally Posted by virtuald

Missed it.

Also, appears that C++ has an identical error.

Python does not have this error. C# does however.

Well, I'm certainly glad we noticed this, then.

[Thad House]

Originally Posted by virtuald

Missed it.

Also, appears that C++ has an identical error.

Python does not have this error. C# does however.

Ah yes it does. I'll fix that in the C# code and hopefully we can get a push up to wpilib

[dvanvoorst]

Originally Posted by virtuald

I agree, that's a bug there. Different from what Oblarg was saying (unless they meant setSetpoint instead of enable).

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:

private synchronized boolean isAvgErrorValid() {
    return m_buf.size() != 0;
  }

to this

  private synchronized boolean isAvgErrorValid() {
    return m_buf.size() == m_bufLength;
  }

[Oblarg]

Originally Posted by virtuald

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).

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.

Editing my OP now to reflect this correction.

The effect of the bug is the same.