allwpilib
allwpilib copied to clipboard
Documentation for `PIDController.setIntegratorRange` is incorrect; some checks for non-negativity of the PID constants are missing
The in-code documentation for PIDController.setIntegratorRange says
/**
* Sets the minimum and maximum values for the integrator.
*
* <p>When the cap is reached, the integrator value is added to the controller output rather than
* the integrator value times the integral gain.
*
* @param minimumIntegral The minimum value of the integrator.
* @param maximumIntegral The maximum value of the integrator.
*/
The described semantics are very puzzling, but the code doesn't do that -- it does what I'd expect. Suggested replacement:
/**
* Sets the minimum and maximum values for the integrator.
*
* <p>If the integrator value is outside the range [minimumIntegral..maximumIntegral]
* the integral gain times the corresponding limit is added to the controller output
* rather than the integral gain times the integrator value.
*
* @param minimumIntegral The minimum value of the integrator.
* @param maximumIntegral The maximum value of the integrator.
*/
The same problem is present in PIDController.h on the C++ side.
Additionally, in PIDController.java and in the analogous methods in PIDController.cpp the methods setP, setI, setD, and setPID do not check that their arguments are non-negative (though the constructors do make this check and the requirement that they be non-negative is documented).
The suggested rewording above is still wrong.
/**
* Sets the minimum and maximum values for the integrator.
*
* <p>If the integrator value is outside the range [minimumIntegral/ki..maximumIntegral/ki]
* either minimumIntegral or maximumIntegral is added to the controller output
* instead of the integral gain times the integrator value.
* Additionally, the integrator value is set to minimumIntegral/ki or maximumIntegral/ki if
* the integrator value would fall outside the range [minimumIntegral/ki..maximumIntegral/ki].
*
* @param minimumIntegral The minimum contribution of the integral term
* @param maximumIntegral The maximum contribution of the integral term
* */
That is, minimumIntegral and maximumIntegral should be thought of as the minimum and maximum contribution of the integral term to the final result and not as the minimum and maximum values of the integrator value.
The above comment essentially recreates the code in English -- is there a better way to write the comment?
Finally, there is no check that ensures minimumIntegral <= maximumIntegral. Should there be?
Fixed by #6489.