-
Notifications
You must be signed in to change notification settings - Fork 613
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
[cmd] Control command/subsystem deprecation #5352
[cmd] Control command/subsystem deprecation #5352
Conversation
Another thing to note is that eg. public CommandBase profiledToState(double goal) {
TrapezoidProfile profile;
return runOnce(
() ->
profile =
new TrapezoidProfile(
CONSTRAINTS, new State(goal, 0), new State(getPosition(), getVelocity())))
.andThen(Commands.runTimed(t -> doSomething(profile.calculate(t))));
} I'm not sure how this would be fixed, or if it's an issue. Maybe it would be better to have a more generalized trajectory following command (related to an idea I had about rewriting the trajectory library). |
That example wouldn't work because of reassignment and capture semantics. |
I do think there's value in pre-built base classes or templates for integrating control loops into commands, ie a command class that has prebuilt factories for commands like going to a setpoint etc. The problem is naming: we can't just change the semantics of these classes without a deprecation cycle, so maybe templates and docs are the way to go? |
I think the largest benefit to having predefined factories for controller commands would be "encouraging" users to use features like that in a pragmatic way. However, most of these commands wouldn't actually reduce any boilerplate. Also I just checked the profile command example. That's definitely an issue, since removing |
Have we decided on whether or not we want to keep the control commands? If we are going to deprecate them, we should do it this year so we won't have to wait another year. We can always un-deprecate stuff as well. Personally, I'm okay with leaving the control commands in, since they aren't terrible, and because the alternative is letting teams implement it themselves, which, for such a common use case, is probably not worth it. |
I personally think, at the very least, the control subsystems should be removed in the future. There is more of a case for the existence of control commands, especially |
We tried replacing the controller commands with TrajectoryCommand in #3366, but we couldn't get CI to pass. |
Since we're getting close to kickoff, can this PR be rebased and changed to deprecate the control subsystems and remove the subclassing examples? Best to deprecate sooner rather than later. |
Yep, I've been kind of busy this week, but I should be able to get this and proxyAll finalized by tonight. |
@Starlight220 How do you feel about |
/format |
If the control commands are staying, I'm not sure it's needed. |
/format |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fun tidbit: I think this fixed a bug where SetElevatorSetpoint
wouldn't do anything because Elevator
overrode PIDSubsystem.periodic
without calling it, which is what would command the motor.
wpilibjExamples/src/main/java/edu/wpi/first/wpilibj/examples/gearsbot/subsystems/Claw.java
Outdated
Show resolved
Hide resolved
* @return A command that drives the robot to the provided distance based on the rangefinder. | ||
*/ | ||
public Command driveDistanceFromObstacle(double distance) { | ||
return driveDistance(-distance, () -> -getDistanceToObstacle()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are these negated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They're negated so that the robot drives forward in order to reduce distance. It's a bit jank, but better than the previous version's negative P gain.
wpilibjExamples/src/main/java/edu/wpi/first/wpilibj/examples/gearsbot/commands/Autos.java
Outdated
Show resolved
Hide resolved
The control subsystems need to go. However, the control commands are relatively useful, and I'm not sure we should remove them. Thoughts? |
If we remove control commands, I'm concerned that less experienced teams will struggle to create commands that use PIDControllers or motion profiles. It may not reduce boilerplate by that much, but teams are forced to adhere to the constructor args, which means it's harder for them to mess up. The ease of use to teams is worth more than the potential diffculty they would have creating their own. |
I don't necessarily disagree with the existence of control commands, given the documentation uses them inline rather than subclassing. new PIDCommand(new PIDController(...), sensor::get, setpoint::get, out -> motor.set(out), this); // the current PIDCommand behavior would have to be changed to implement `isFinished` with `controller.atSetpoint` Doesn't seem much better than var controller = new PIDController(...);
run(() -> motor.set(controller.calculate(sensor.get(), setpoint.get()))).until(controller::atSetpoint) Especially in debugging, where teams can have a much better understanding of what their code does in the second example. Regarding the constructor forcing teams to structure their commands in certain way: the added level of opacity makes understanding what the code is actually doing more difficult. |
Re: control commands, please see: https://www.chiefdelphi.com/t/command-based-help/446143/19?u=oblarg When you write it out directly, it's clear that the |
wpilibjExamples/src/main/java/edu/wpi/first/wpilibj/examples/gearsbot/subsystems/Claw.java
Outdated
Show resolved
Hide resolved
…earsbot/subsystems/Claw.java Co-authored-by: Joseph Eng <91924258+KangarooKoala@users.noreply.github.com>
I'm sorry I'm not fluent in github or allwpilib but maybe this belongs to you - I can't find it again so I don't know the URLs. There are two very different implementations of |
See #5067 for motivation. This deprecates
PIDCommand
,ProfiledPIDCommand
,PIDSubsystem
,ProfiledPIDSubsystem
, andTrapezoidProfileSubsystem
.TrapezoidProfileCommand
is kept because it manages a lot of variables (timer, profile, io), although it might be useful to incorporate trapezoid profiles into a future api for general trajectory following.As for documentation, I have so far removed all the ReplaceMeX examples, since they reference deprecated classes and promote bad design practices. There still are many examples that should be edited to avoid using newly deprecated classes, most of which would probably benefit from inlining.
resolves #5452