search

wpilibsuite / allwpilib

Official Repository of WPILibJ and WPILibC
https://wpilib.org/
Other
1.05k stars 612 forks source link

enableContinousInput(0,360) will produce (wildly) incorrect results #3304

Closed brettle closed 3 years ago

brettle commented 3 years ago

Describe the bug This line:

https://github.com/wpilibsuite/allwpilib/blob/f57c188f2e3d0e9cf0de80e2a1cbb76f10a3e8fa/wpilibj/src/main/java/edu/wpi/first/wpilibj/controller/PIDController.java#L207

means that calling enableContinousInput(0,360) will force the error term to be in that range. As a result it will always be positive, and when it should be small and negative it will be very large and positive. Needless to say, the resulting controller will not be very effective and could actually be dangerous.

To Reproduce Steps to reproduce the behavior:

PIDController c = new PIDController(1, 0, 0);
c.enableContinousInput(0,360);
c.setSetpoint(359);
double output = c.calculate(1);

Expected behavior output should be -2, not 358.

Note that all existing examples and tests call enableContinuousInput(-180,180) or enableContinuousInput(-pi,pi) and those will work properly.

brettle commented 3 years ago

Fwiw, this was touched on in the discussion of #3168.