Major WPILib PIDController bug

[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

[virtuald]

Pushed to gerrit here: https://usfirst.collab.net/gerrit/1372

[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;
  }