wpilibsuite / allwpilib

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

[wpilib] Replace MecanumDriveMotorVoltages with a functional interface #6760

Open WispySparks opened 1 week ago

WispySparks commented 1 week ago

Resolves #5929

github-actions[bot] commented 1 week ago

This PR modifies commands. Please open a corresponding PR in Python Commands and include a link to this PR.

WispySparks commented 1 week ago

This PR modifies commands. Please open a corresponding PR in Python Commands and include a link to this PR.

There doesn't appear to be a corresponding MecanumControllerCommand in python. Also the failing test is unrelated, something flaky.

KangarooKoala commented 1 week ago

Looks pretty good! I'm just wondering, though, about whether we want to remove it without deprecating first since it is a user-facing API element. It could be safe to remove the protobuf definition (since there isn't a WPILib implementation yet so there's likely not many teams using it), but I'm concerned about completely removing the class. (Especially since there's likely multiple examples floating around somewhere that would need time to be updated)

WispySparks commented 1 week ago

So should I keep everything using the new functional interface but just not delete MecanumDriveMotorVoltages and instead deprecate it?

KangarooKoala commented 1 week ago

So should I keep everything using the new functional interface but just not delete MecanumDriveMotorVoltages and instead deprecate it?

The point of deprecating instead of completely removing is to give users time to migrate, so user-facing overloads using MecanumDriveMotorVoltages should not be deleted and instead deprecated, with overloads using the new functional interface being added.

Practically speaking, what this would look like relative to what you already have is adding new overloads with identical signatures to the old ones that use the other overloads. (For example, add the following constructor)

@Deprecated
public MecanumControllerCommand(
      Trajectory trajectory,
      Supplier<Pose2d> pose,
      SimpleMotorFeedforward feedforward,
      MecanumDriveKinematics kinematics,
      PIDController xController,
      PIDController yController,
      ProfiledPIDController thetaController,
      double maxWheelVelocityMetersPerSecond,
      PIDController frontLeftController,
      PIDController rearLeftController,
      PIDController frontRightController,
      PIDController rearRightController,
      Supplier<MecanumDriveWheelSpeeds> currentWheelSpeeds,
      MecanumVoltagesConsumer outputDriveVoltages,
      Subsystem... requirements) {
  this(trajectory,
       pose,
       feedforward,
       kinematics,
       xController,
       yController,
       thetaController,
       maxWheelVelocityMetersPerSecond,
       frontLeftController,
       rearLeftController,
       frontRightController,
       rearRightController,
       currentWheelSpeeds,
       (frontLeftVoltage, frontRightVoltage, rearLeftVoltage, rearRightVoltage) ->
        outputDriveVoltages.accept(
          new MecanumDriveWheelVoltages(frontLeftVoltage, frontRightVoltage, rearLeftVoltage, rearRightVoltage));
       requirements);
}
WispySparks commented 1 week ago

I've added back in the old constructors as overloads and MecanumDriveMotorVoltages and deprecated them instead.

sciencewhiz commented 1 week ago

Hopefully someone else can answer if the since argument should be 2024 or 2025

It should be 2025, since 2025 will be the first season it's deprecated. It's then eligible to be removed before the 2026 season.