wpilibsuite / allwpilib

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

Deprecate PIDSubsystem and ProfiledPIDSubsystem for removal #5452

Closed calcmogul closed 11 months ago

calcmogul commented 1 year ago

Paraphrased from @Oblarg on the FRC Discord:

PIDSubsystem and ProfiledPIDSubsystem don't provide abstraction benefits, and they break the command-based paradigm. Here's an example:

https://github.com/wpilibsuite/allwpilib/tree/main/wpilibjExamples/src/main/java/edu/wpi/first/wpilibj/examples/armbot

The motor output happens outside of commands, so it's not possible for the command scheduler alone to make sure that there's no resource contention.

These classes could also be considered safety hazards. If you write a command for the arm that tells the motor to go at a specific rate (e.g. for debugging) you have to manually disable the PID before using it.

The PIDSubsystem classes were basically just ports of the ones from the old command-based framework, whose asynchronous PID implementation forced there to be these sort of concurrency problems. Now that we do everything synchronously, we can and should do better.

Fluent command factories, on the other hand, automatically avoid any concurrency problems because the implicit mutexing just does the right thing. When you're in the happy path, a lot of failure states are rigorously impossible.

Starlight220 commented 1 year ago

Duplicate of #5067.