-
Notifications
You must be signed in to change notification settings - Fork 612
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
[wpilib] Add functional interface equivalents to MotorController #6053
[wpilib] Add functional interface equivalents to MotorController #6053
Conversation
5956c5d
to
d890892
Compare
2cad49d
to
487e175
Compare
So MSVC is warning on usage of methods from deprecated base classes, but no other compilers. Guess we'll have to use the "deprecate the constructor" trick again. |
b928665
to
6b02d39
Compare
6b02d39
to
5ecbf82
Compare
...ain/java/edu/wpi/first/wpilibj/examples/drivedistanceoffboard/subsystems/DriveSubsystem.java
Show resolved
Hide resolved
wpilibcExamples/src/main/cpp/examples/HatchbotTraditional/cpp/subsystems/DriveSubsystem.cpp
Show resolved
Hide resolved
@sciencewhiz I assume this will also need fairly substantial RobotBuilder changes before removal, although this is just a deprecation at present. |
RobotBuilder doesn't use the MotorController interface any more, but it does use MotorControllerGroup. I think both the changes for MotorControllerGroup and drive classes will be complicated in RobotBuilder, but I'd have to play with it. |
I'm worried that this change will have a negative impact on low resource teams. I think there's been consistent feedback on commands that teams have trouble with lambdas and functional interfaces. Now that will be needed just to get a robot driving. Teams using swerve and CAN motor controllers aren't really going to be affected, but low resource teams will. To me this feels like RFC on CD this year, and then deprecate in '25 so there's a full cycle of beta feedback and docs (There looks to be around 20 pages or more that need updating on frc-docs). |
As you can see in the snippet in the first post, the drive code is virtually the same as before. It's just passing setters instead of objects. Functions are data in the same way objects are data, and students and mentors shouldn't be so limited in their thinking that OOP is always the answer. Command-based's functional patterns ended up with much shorter, cleaner, and easier to read code compared to the OOP equivalents because it's fundamentally a function/behavior-driven solution domain. As for CAN motor controllers, lots of teams don't even use the drive classes because they aren't aware of the vendor class that implements MotorController (e.g., TalonFX vs WPI_TalonFX. I'm aware the classes changed in Phoenix 6), and the obvious class isn't compatible with DifferentialDrive. At least with the changes in this PR, they can write something like: // Idk what CTRE's set API is, but you get the idea
DifferentailDrive drive = new DifferentialDrive((output) -> leftMotor.set(output, kVoltage),
(output) -> rightMotor.set(output, kVoltage)); which should be in vendor examples, imo (since we don't have vendor examples of our own).
Deprecation doesn't mean removal. We can deprecate something and undeprecate it later. |
Why don’t we add the functional interfaces this year without deprecating the MotorController ones? That would let teams start migrating vendor code (should they choose to experiment with this feature) without creating warnings until we have some more time to consider the potential downsides of deprecation/removal. |
Is it worth trying to do some type of warning if the motor controller to be followed isn't the same type of motor controller? That could work, but could also be bad depending on how mismatched the motor controller curves are. Although we didn't do it for motor controller group |
Depends on whether that can damage motors. If not, it seems like an intentional decision that limited-motor teams make often that we shouldn't warn about. |
wpilibc/src/main/native/include/frc/motorcontrol/MotorController.h
Outdated
Show resolved
Hide resolved
…th-functional-interface
@@ -16,11 +16,21 @@ | |||
|
|||
using namespace frc; | |||
|
|||
WPI_IGNORE_DEPRECATED |
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.
Is this still necessary?
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.
It will be when we eventually deprecate it again, and finding where those go was a lot of work because they're scattered all over the motor controller codebase.
MotorControllerGroup going away wasn't too bad. wpilibsuite/RobotBuilder#576 The drive classes are harder, and I haven't figured out a good way to do it yet. The issue is that RobotBuilder supports plugins of arbitrary components and now that there isn't a MC interface, you can't assume calling ::set is correct. I think I'll have to add another field, which CTRE would then have to implement in their plugin, assuming that they plan to maintain it. @JCaporuscio |
They accidentally got reverted when undeprecating MotorController in the review process for wpilibsuite#6053.
They accidentally got reverted when undeprecating MotorController in the review process for #6053.
The drive classes no longer need this interface, and it rigidly couples us to motor controller vendors who only update and do a major release of their vendordep once per year. We were originally going to do this in wpilibsuite#6053, but we wanted to gather feedback on the alternate approach first (as far as I know, we received none).
The drive classes no longer need this interface, and it rigidly couples us to motor controller vendors who only update and do a major release of their vendordep once per year. We were originally going to do this in wpilibsuite#6053, but we wanted to gather feedback on the alternate approach during a season first (as far as I know, we received none).
The drive classes no longer need this interface, and it rigidly couples us to motor controller vendors who only update and do a major release of their vendordep once per year. We were originally going to do this in wpilibsuite#6053, but we wanted to gather feedback on the alternate approach during a season first (as far as I know, we received none).
The drive classes no longer need this interface, and it rigidly couples us to motor controller vendors who only update and do a major release of their vendordep once per year. We were originally going to do this in wpilibsuite#6053, but we wanted to gather feedback on the alternate approach during a season first (as far as I know, we received none).
The drive classes now accept void(double) functions, which makes them more flexible, and eliminates the ABI breakage caused by changes to the MotorController interface. This is part of our work to decouple motor controller vendor libraries from WPILib (the other major item being Sendable).
The C++ API ended up a bit more verbose, but the Java API is really concise with method references, which is >80% of our userbase. For example:
Lambdas can be passed to interoperate with vendor motor controller APIs that don't have e.g.,
set(double)
, so CTRE doesn't have to maintain theirWPI_
classes anymore.MotorControllerGroup was replaced with PWMMotorController.addFollower() for PWM motor controllers. Users of CAN motor controllers should use their vendor's follower functionality.