Faeture/launch interface

[y0shi]

No description provided.

[github-actions]

AI Code Review

Hello team!

This pull request introduces some great architectural improvements to the shooter subsystem and makes good use of WPILib's unit system. It's awesome to see the LaunchRequestBuilder being refactored into an interface, which really enhances flexibility!

Positive Highlights:

• Architectural Improvement: Changing LaunchRequestBuilder to an interface and moving the targetPose supplier to the implementation class is a significant step towards a more flexible and robust design for handling different launch strategies.
• Type-Safe Units: Excellent use of WPILib's Units system (Angle, AngularVelocity, Distance, Meters, Radians, etc.) throughout the LaunchRequestBuilder implementations and ShooterConstants. This is a fantastic practice that will prevent common conversion errors and make your code safer and clearer.
• Logging and Constants: The use of @Logged for subsystems and critical variables, along with well-commented TODOs in ShooterConstants for verification, demonstrates good awareness of debugging and configuration needs.

Suggestions:

FRC/WPILib Best Practices & Performance

• DrivetrainSubsystem.java - Avoid periodic() in Constructor:

  • File/Location: src/main/java/frc/robot/subsystems/drive/DrivetrainSubsystem.java (line 44)
  • Suggestion: Consider removing the periodic() call from the constructor. The periodic() method is designed to be called automatically by the WPILib scheduler at a fixed interval. Calling it manually in the constructor can lead to unexpected behavior or state issues, as the subsystem might not be fully initialized when it runs. If you need to perform initialization that resembles a periodic update, it's usually better to have a dedicated setup() method or ensure that the constructor sets up initial states correctly without relying on a full periodic cycle.
  • Why it matters: It can make the robot's startup behavior unpredictable and might not reflect how the robot will operate during a match.

• ShooterSubsystem.java - Efficient LaunchRequestBuilder Usage:

  • File/Location: src/main/java/frc/robot/subsystems/shooter/ShooterSubsystem.java (line 103)
  • Suggestion: Currently, launchRequestBuilder.createLaunchRequest(); is called in periodic() but its return value is not used. createLaunchRequest() likely involves significant calculations. It might be more efficient to only call this method and update the velocityTarget and hoodTarget when a specific "prepare to shoot" command is active or when the target changes, rather than every 20ms.
  • Why it matters: This can reduce unnecessary CPU load, leaving more resources for critical control loops and other robot functions, especially important for time-sensitive calculations.

• ShooterSubsystem.java - Dynamic Target for LaunchRequestBuilder:

  • File/Location: src/main/java/frc/robot/subsystems/shooter/ShooterSubsystem.java (line 99)
  • Suggestion: The MappedLauchRequestBuilder is currently hardcoded to ShooterConstants.BLUE_TARGET. Consider making the target dynamic. One approach could be to pass a Supplier<Pose3d> to the ShooterSubsystem's command factory methods (e.g., prepareLaunchCommand(Supplier<Pose3d> targetSupplier)) or have ShooterSubsystem itself update its LaunchRequestBuilder's target based on current game state (alliance, driver input, vision data).
  • Why it matters: Robots often need to shoot at different targets (red/blue alliance, speaker/amp, dynamic target tracking), and a hardcoded target limits flexibility.

Java Standards & Code Quality

• ShooterSubsystem.java - Remove Duplicate LaunchRequest Class:

  • File/Location: src/main/java/frc/robot/subsystems/shooter/ShooterSubsystem.java (lines 53-73)
  • Suggestion: There's a LaunchRequest inner class defined in ShooterSubsystem that appears to be a duplicate or very similar to the LaunchRequestBuilder.LaunchRequest record. Please consider removing the inner class and using the LaunchRequestBuilder.LaunchRequest record consistently throughout ShooterSubsystem.
  • Why it matters: Avoiding code duplication makes the codebase easier to maintain, reduces potential for bugs from inconsistent definitions, and improves clarity.

• MappedLauchRequestBuilder.java - Clarify getDriveAngle Logic:

  • File/Location: src/main/java/frc/robot/subsystems/shooter/MappedLauchRequestBuilder.java (lines 183-190)
  • Suggestion: The getDriveAngle method's calculation, especially the hubAngle and subsequent additions, seems quite complex. It might be helpful to add Javadoc comments explaining the mathematical intent and the coordinate frames involved for fieldToHubAngle, hubAngle, and how they combine to form driveAngle. Also, consider edge cases like when target.getDistance(robotPose.getTranslation()) is zero, as that could lead to division by zero.
  • Why it matters: Clear documentation for complex calculations helps other team members understand and debug the code, and ensures the logic is correctly applied.

• ParabolicLaunchRequestBuilder.java - Handle null Return from createLaunchRequest():

  • File/Location: src/main/java/frc/robot/subsystems/shooter/ParabolicLaunchRequestBuilder.java (line 42)
  • Suggestion: The createLaunchRequest() method can return null if no valid launch angle is found. Consider returning an Optional<LaunchRequest> or a default "invalid" LaunchRequest object instead of null. This makes the contract clearer and forces the caller to explicitly handle the absence of a valid request, preventing potential NullPointerExceptions.
  • Why it matters: Explicitly handling potential "no result" scenarios leads to more robust and less error-prone code.

Robot Safety & Error Handling

• ShooterSubsystem.java - Correct Hood Motor Control:

  • File/Location: src/main/java/frc/robot/subsystems/shooter/ShooterSubsystem.java (line 102)
  • Suggestion: It appears hoodMotor.setControl(hoodControl.withVelocity(hoodTarget.in(Rotations))); is attempting to set a position target using a PositionTorqueCurrentFOC control request, but is calling withVelocity on it and passing an Angle converted to Rotations. PositionTorqueCurrentFOC typically expects a position, so hoodControl.withPosition(hoodTarget.in(Rotations)) is likely the intended method call. Please verify and correct this.
  • Why it matters: Incorrect control requests can lead to unexpected motor behavior, poor performance, or even damage if the motor tries to interpret a position as a velocity or vice-versa. This is a critical bug.

• ShooterSubsystem.java - Correct atHoodSetpoint() Tolerance:

  • File/Location: src/main/java/frc/robot/subsystems/shooter/ShooterSubsystem.java (line 125)
  • Suggestion: The atHoodSetpoint() method currently uses IntakeConstants.ALLOWABLE_EXTENSION_ERROR. It's likely that a specific ShooterConstants.ALLOWABLE_HOOD_ERROR (which is already defined) should be used here instead.
  • Why it matters: Using constants from the wrong subsystem can lead to incorrect behavior (e.g., the hood thinking it's at its setpoint when it's not, or vice-versa), making tuning and debugging more difficult.

Questions:

• Could you clarify the specific purpose and mathematical logic behind the hubAngle calculation in MappedLauchRequestBuilder.getDriveAngle? Understanding the coordinate frames and geometric intent would be very helpful.
• What is the intended behavior if ParabolicLaunchRequestBuilder.createLaunchRequest() returns null? How will ShooterSubsystem (or the calling command) handle this scenario to prevent issues?

Keep up the great work, team! You're making excellent progress on building out the shooter functionality and improving the code architecture. The use of interfaces and type-safe units is fantastic!

---

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!