Feature/intake

[Orcasphynx]

No description provided.

[github-actions]

AI Code Review

Hey team!

This is a really solid pull request, especially with the introduction of the new RobotPreference system and the initial implementation of the Intake subsystem. It's great to see a strong foundation being built here!

Positive Highlights

• Excellent RobotPreference Abstraction: The new RobotPreference classes (Boolean, Double, Integer, String) are a fantastic addition! They provide a type-safe and consistent way to manage tunable robot parameters, which will be incredibly helpful for tuning and debugging. This aligns perfectly with good Java and FRC practices.
• Strong Command-Based Implementation for Intake: The IntakeSubsystem demonstrates great use of command factory methods (spinRollerCommand, collectCommand, stowCommand, etc.) and command composition. This makes the intake's behavior clear, reusable, and easy to integrate into larger autonomous routines, directly following our "Command Factory Methods" guideline.
• Effective Use of WPILib Units and CTRE Phoenix 6 Controls: It's awesome to see WPILib's Units system being used for Angle and AngularVelocity with the motors, preventing conversion errors. The use of CTRE Phoenix 6's VelocityVoltage and PositionTorqueCurrentFOC controls is also a great step towards robust motor control, adhering to our "Type-Safe Units" and "Use Position or Velocity Control" guidelines.

Suggestions

Here are a few suggestions to consider, focusing on robustness, clarity, and adherence to our guidelines:

Code Quality & Java Standards

• RobotPreference Redundancy: In the BooleanPreference, DoublePreference, IntegerPreference, and StringPreference classes, you have both a getValue() method and an overridden get() method (from Supplier<T>).
  • Suggestion: Consider having the get() method directly call Preferences.getWhatever() to avoid the getValue() method, making the API slightly cleaner and less redundant.
  • Example: In BooleanPreference, public Boolean get() { return Preferences.getBoolean(super.key, defaultValue); }
• RobotPreference key field:
  • Suggestion: It might be helpful to make the key field in RobotPreference final since it's set in the constructor and shouldn't change. This adds a bit of immutability.
• Javadoc Documentation:
  • Suggestion: Please add Javadoc comments to the new RobotPreference classes and their methods. This will explain their purpose and usage clearly, especially for RobotPreference itself and its subclasses. This aligns with our "Javadoc Documentation" guideline.
  • Example for RobotPreference:

    /**
     * Abstract base class for robot preferences, providing a type-safe wrapper
     * around WPILib's {@link Preferences} system.
     *
     * @param <T> The type of the preference value (e.g., Boolean, Double, Integer, String)
     */
    public abstract class RobotPreference<T> implements Supplier<T> { ... }

• IntakePreferences Constructor:
  • Suggestion: The private IntakePreferences() {}; constructor has an unnecessary semicolon after the block. It should just be private IntakePreferences() {}.

FRC/WPILib Best Practices & Safety

• IntakeConstants - Type-Safe Units for Limits:
  • Suggestion: For constants like INTAKE_FORWARD_LIMIT, INTAKE_REVERSE_LIMIT, and ALLOWABLE_EXTENSION_ERROR, consider using WPILib's Units system (e.g., Rotations.of(100)) to explicitly define their units. This prevents unit confusion and adheres to our "Type-Safe Units" guideline.
  • Example: public static final Measure<Angle> INTAKE_FORWARD_LIMIT = Rotations.of(100);
• IntakeConstants - Software Limits Enable:
  • Suggestion: In createExtensionSoftwareLimitSwitchConfigs(), ForwardSoftLimitEnable and ReverseSoftLimitEnable are set to false. If these limits are intended to be active, they should be set to true. If they are intentionally disabled for now, adding a comment explaining why would be helpful. This relates to our "Mechanism Limits" guideline.
• Current Limiting for Intake Motors:
  • Suggestion: It's crucial to add current limiting configurations for both the rollerMotor and extensionMotor (e.g., SupplyCurrentLimit, StatorCurrentLimit) in IntakeConstants and apply them in the IntakeSubsystem constructor. This is a vital safety measure to protect the motors and the robot's electrical system, as highlighted in our "Current Limiting" guideline.
• Intake Homing Robustness: The homeIntakeCommand() is a good start for homing!
  • Suggestion 1 (Homing State): Our "Automatic Homing" guideline suggests using an explicit HomingState enum within the subsystem's periodic() method. This makes the homing process more robust by managing its state globally for the subsystem. The current homeIntakeCommand is functional but doesn't persist the "homed" state beyond its execution. Implementing a HomingState (NEEDS_HOMING, HOMING, HOMED) would ensure the subsystem always knows its homing status and can prevent accidental position commands before homing is complete.
  • Suggestion 2 (Interruption Safety): The runEnd's end lambda () -> extensionMotor.setPosition(0) is called when the command finishes successfully. If the homeIntakeCommand is interrupted (e.g., by another command or driver input), setPosition(0) might not be called, leaving the encoder uncalibrated. If using the HomingState enum, the periodic method could handle setting the position to 0 only when the state transitions to HOMED.
• Fault Detection and Logging:
  • Suggestion: Consider adding fault detection for the intake motors in the periodic() method of IntakeSubsystem, similar to the example in our guidelines. This would involve checking motor.getFaults() and motor.getStickyFaults() and logging warnings to DriverStation and Logger.recordOutput. This greatly aids in diagnosing issues during matches and testing.
• extensionMotor.setPosition(0) in Constructor:
  • Suggestion: While extensionMotor.setPosition(0) in the constructor is good for initial setup, if the extensionMotor doesn't have an absolute encoder (meaning its position isn't known at boot), this line might be misleading. If it's a relative encoder, the mechanism must be homed before any position control is used. The homeIntakeCommand correctly sets the position to 0 after homing, so the constructor's call might be redundant or could be removed if the expectation is that homeIntakeCommand will always be run first.

Minor Refinements

• IntakePreferences Units Comments:
  • Suggestion: For rollerIntakeSpeed and intakeCollectPosition, adding unit comments (e.g., // in rotations per second, // in rotations) is helpful, but using type-safe units for these preferences themselves (if possible with Preferences) would be even better to prevent runtime errors. For now, the comments are a great start!

Questions

• Are the ForwardSoftLimitEnable and ReverseSoftLimitEnable in IntakeConstants.createExtensionSoftwareLimitSwitchConfigs() intentionally set to false, or should they be true to activate the limits?
• Do the IntakeConstants.INTAKE_FORWARD_LIMIT and INTAKE_REVERSE_LIMIT represent actual physical limits you've measured, or are they placeholders for now?

Overall, this is a fantastic contribution! The new preferences system is a big win for the team, and the Intake subsystem is off to a very strong start with great use of WPILib and CTRE best practices. Keep up the great work!

---

This review was automatically generated by AI. Please use your judgment and feel free to discuss any suggestions!

[github-actions]

✓ Build successful and code formatting check passed!

[github-actions]

✓ Build successful and code formatting check passed!

[github-actions]

✓ Build successful and code formatting check passed!