Skip to content
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

BCP: Use 1 single "Constants" file instead of multiple ones #113

Open
INeedCaffeine opened this issue Sep 10, 2024 · 0 comments
Open

BCP: Use 1 single "Constants" file instead of multiple ones #113

INeedCaffeine opened this issue Sep 10, 2024 · 0 comments
Labels
chore Cleans Code

Comments

@INeedCaffeine
Copy link

INeedCaffeine commented Sep 10, 2024

In looking at the repo I see we have 10 "Constants" files:

Constants.java
Subsystems\actuator\ActuatorConstants.java
Subsystems\arm\ArmConstants.java
Subsystems\drive\DriveConstants.java
Subsystems\feeder\FeederConstants.java
Subsystems\gyro\GyroConstants.java
Subsystems\otbIntake\OTBIntakeConstants.java
Subsystems\shooter\ShooterConstants.java
Subsystems\utbintake\UTBIntakeConstants.java
Subsystems\wrist\WristConstants.java

Several of those subsystem specific constants files import other subsystem constants. While having the constants declared close to the code that uses it, it is a poor coding practice since making an edit to one may require adding additional imports (or forgetting to remove obsolete imports) to the code.

Good code will use a single common constants file (e.g. just Constants.java). That keeps the code cleaner as it is simpler to simply have 1 import where ALL the values you may need are available. It also makes editing things like CAN IDs or specific values much easier as every coder knows exactly where to find the constants; they do not need to search all of the repo to find the correct one.

There are 2 ways to separate out constants that are subsystem or control specific:

  1. Use nested classes inside of the Constants class
  2. Include the subsystem as part of the constant name

With the first approach the way to access is the same that are already do. For example:

driveSparkMax = new CANSparkMax(DriveConstants.DRIVE_MOTOR.FRONT_RIGHT.CAN_ID, MotorType.kBrushless);
would simply be:

driveSparkMax = new CANSparkMax(Constants.Drivetrain.DRIVE_MOTOR.FRONT_RIGHT.CAN_ID, MotorType.kBrushless);

if you put the drivetrain subsystem into a nested class named Drivetrain (or Drive).

With the second approach the names would change from something like:

  /**
   * KP represents the constant multiplied by the current error from setpoint (Proportional Error)
   */
  public static final double KP = 1.0;

(from Subsystem/actuator/ActuatorConstants.java) to something like:

  /**
   * KP represents the constant multiplied by the current error from setpoint (Proportional Error)
   */
  public static final double ACTUATOR_KP = 1.0;

In looking at the various constants files I see that constant naming was NOT consistent which not goodness. The actuator code uses just KP while the drive code uses DRIVE_KP_KRAKEN and STEER_KP_NEO.

NOTE: Making this change would be a non-trivial effort at this point in our 2024 coding. Doing it is best left to after ARL Champs when we have no risk of breaking anything; it would be a good exercise to prepare for handling constants in a better way for 2025's robot.

@INeedCaffeine INeedCaffeine added the chore Cleans Code label Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Cleans Code
Projects
None yet
Development

No branches or pull requests

1 participant