-
Notifications
You must be signed in to change notification settings - Fork 603
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Deprecate PIDCommand and PIDSubsystem #5067
Comments
I agree about PIDSubsystem, though I suspect that many teams use it. As for PIDCommand, I think it's fairly good for use cases that don't need a dynamic feedforward. It doesn't rely on inheritance, and the constructor doesn't have that many arguments (it has 5, including requirements). If errors due to subclassing are common, we should mark the lifecycle methods as What about the other control-loop subsystems/commands? Maybe a first tiny-possibly-meaningless step would be flipping the order of their docs pages, presenting the control-loop commands first and then the counterpart subsystems? |
Marking the lifecycle methods of I agree with the first-step of switching the order in the docs. |
We used PID subsystems last year however this year we're just using regular subsystems and building the pod controllers manually. The PID subsystem tries to be helpful by abstracting stuff but honestly looking back, I think it just made it harder to understand exactly how to implement it which is why we're just doing it manually this year. |
Bringing this back, From TrapezoidProfileSubsystem: @Override
public void periodic() {
var profile = new TrapezoidProfile(m_constraints, m_goal, m_state);
m_state = profile.calculate(m_period);
if (m_enabled) {
useState(m_state);
}
} As for |
Copy-paste from #5452:
|
PIDCommand is an overwrought abstraction that tries very hard to maintain a roughly pre-2020 API shape while being implemented in the new Command-based framework. It's prone to a number of surprising and hard-to-diagnose initialization/state errors due to the reliance on subclassing as an API interaction, which is a pattern we've mostly moved away from elsewhere. The functional side of the PIDCommand API is not very good either, consisting of a rigid and difficult-to-use many-argument constructor. It is easier and better pedagogy to focus on teaching users to compose controllers within commands themselves by writing their own classes or using capture semantics and inline command definition.
PIDSubsystem is an underwrought abstraction that does very little work for the user while circumventing library features to do so. Notably, the architecture of PIDSubsystem runs motor outputs directly through the subystem
periodic()
method, which bypasses the subsystemrequirement
mutex system. This is bad practice, especially given how easy it is to write code that doesn't do this with the subsystem command factory pattern.Both classes should be deprecated and the examples should be changed to show users how to do the relevant composition themselves.
The text was updated successfully, but these errors were encountered: