CrossTheRoadElec / Phoenix-api

CTRE Phoenix language API (targets FIRST Robotics Competition, Linux, RaspPi, Windows)
Other
39 stars 28 forks source link

configSupplyCurrentLimit() should be declared in IMotorControllerEnhanced interface #71

Closed richiksc closed 3 years ago

richiksc commented 4 years ago

The TalonFX has a new API for configuring current limits, configSupplyCurrentLimit(). I was glad to see that the same API has been added to TalonSRX. However, the method is not declared in the interface itself. Now that the two motors share the same API for current limiting, albeit with understandably different implementations, I believe configSupplyCurrentLimit() should be declared in the IMotorControllerEnhanced interface as well as abstract in BaseTalon.

Why it is beneficial.

Our team stores references to motor controllers as IMotorControllerEnhanced. This allows us to easily swap between TalonFX and TalonSRXs, as well as "ghost" motor controllers, which uses a custom implementation of IMotorControllerEnhanced which is essentially a no-op so we can disable motors and even remove them physically without breaking robot code or having to significantly refactor. However, after the splitting of the current limit APIs, we can no longer simply current limit a motor without having to a type check and cast, which is unwieldy:


if (motor instanceof TalonFX) {
  ((TalonFX) motor).configSupplyCurrentLimit(new SupplyCurrentLimitConfiguration(...))
} else if (motor instanceof TalonSRX) {
  ((TalonSRX) motor).configSupplyCurrentLimit(new SupplyCurrentLimitConfiguration(...))
} else {
  ...
}

## A series of steps to test the feature after it is implemented

Create a motor variable of type `IMotorControllerEnhanced` and initialize it to either a `TalonFX` or a `TalonSRX`. Call `configSupplyCurrentLimit` without casting it to a subtype.
ozrien commented 4 years ago

One workaround might be to use BaseTalon references instead of IMotorControllerEnhanced references, which brings in the new configSupplyCurrentLimit, which you can then override in your custom child class.

richiksc commented 4 years ago

@ozrien Without declaring it in the interface, we will not be able to use the "Ghost Talons" cleanly - which are essentially no-op implementations of IMotorControllerEnhanced. If we were to override BaseTalon, we would have to somehow no-op all the functionality currently defined in the abstract class.

Also, configSupplyCurrentLimit does not seem to be currently defined in BaseTalon, so we would be unable to use a BaseTalon reference anyway.

JCaporuscio commented 3 years ago

This has been fixed with the 2021 Kickoff release (5.19.4.1)