PIDController atSetpoint thoughts

[narmstro2020]

Should the WPILIB PIDController have an atSetpoint function? Looking at the implementation it requires the error and error derivative to be within default or user defined tolerances. It also requires a measurement to have occurred and a setpoint to have been set. It doesn't do anything directly to the controller and is just a quick means to compare errors to user defined values. Looking at the ProfiledPIDController you have atSetpoint and atGoal which could be potential footguns because of the naming ambiguity. Also this is just my opinion, but I think it makes the class difficult to follow. The default values seem a little ambiguous. More so on the errorTolerance side. Also I've noticed that other vendors don't bother with tolerance functions. That's not to say that we should base WPILib's implementation off of another vendors, but just something I noticed. I could be wrong about all of this, but wanted to put it out there.

[calcmogul]

Looking at the implementation it requires the error and error derivative to be within default or user defined tolerances. It also requires a measurement to have occurred and a setpoint to have been set.

How is this bad? These are all necessary checks to ensure the PIDController is on target and stays there. Just checking the error is insufficient because the measurement could be oscillating, and we have to check if the setpoint and measurement are valid to avoid spurious trues.

If a user implements the atSetpoint check themselves, they'll have to do the same things if they want it to be correct. Here are some ways I've seen students mess it up:

1. Didn't have a valid starting measurement or setpoint, which put the robot in an inconsistent state because atSetpoint() returned true when it shouldn't have.
2. Wrote getSetpoint() == measurement or getError() == 0.0, which are never true in reality so the controller action never finished.
3. Wrote getError() < tolerance instead of Math.abs(getError()) < tolerance.

The atSetpoint() function is widely used by public code on GitHub: https://github.com/search?q=atSetpoint%28%29&type=code. I'm honestly surprised at how many instances given how popular vendor-specific code is instead these days.

We default the error derivative tolerance to infinity for backwards compatibility and because the units affect what a good tolerance is.

Looking at the ProfiledPIDController you have atSetpoint and atGoal which could be potential footguns because of the naming ambiguity.

The setpoint is where the PID controller should be right now, and the goal is where the PID controller should be at the end of the motion profile. Both are relevant things to be able to check, and the implementation of atGoal() isn't entirely trivial. We should expand the class's top-level doc comment and define "setpoint" and "goal".

With that said, I have been pushing for deprecating ProfiledPIDController in favor of users composing their own profile. The main blocker is #5914, since that profile modulus logic was hard to get right. At least the user version of the atGoal check would be simpler:

return Math.abs(positionGoal - positionMeasurement) < positionErrorTolerance
    && Math.abs(velocityGoal - velocityMeasurement) < velocityErrorTolerance;

The default values seem a little ambiguous.

That's easily fixed by just documenting the default tolerances. Users can still set the tolerances to whatever they want (though I doubt many do).

Also I've noticed that other vendors don't bother with tolerance functions.

They expose the reference and error, but I'm not sure how many teams even use those versus letting a swerve wrapper do it for them.

[narmstro2020]

All valid points @calcmogul . I appreciate your time. Just to clarify. I didn't want to imply that the class was poorly written, just noticed some things and my mind got to thinking.

[sciencewhiz]

It's also fairly common to see people asking questions about how to know when vendor PID are done.

[narmstro2020]

Fair point. When ones teaches programming everyday one can issues where none exist. I'll try to keep that in mind more when I look at things. Plus I learn more which helps.