Faeture/launch interface

[Orcasphynx]

No description provided.

[github-actions]

AI Code Review

Hey team!

This pull request introduces some really interesting new architecture for calculating launch requests, which is a fantastic step towards a more modular and robust shooter system. It's great to see the team thinking about these design patterns!

Positive Highlights

• Architectural Improvement: Moving the launch calculation logic out of the ShooterSubsystem into dedicated LaunchRequestBuilder classes is a significant improvement in separation of concerns and follows good object-oriented design principles. This will make the shooter logic easier to test and maintain.
• Type-Safe Units: Excellent use of WPILib's Units system (Angle, AngularVelocity, LinearVelocity, Meters, Radians, Degrees, etc.) across the new builder classes. This is a crucial best practice for preventing hard-to-find unit conversion bugs.
• WPILib Utilities: The use of LinearFilter for smoothing and InterpolatingTreeMap for lookup tables in MappedLaunchRequestBuilder demonstrates good application of WPILib's utility classes.

Suggestions

Code Quality & Java Standards

1. DriveState.java - Singleton Naming:

   • Suggestion: Consider renaming the private static instance variable single_instance to s_instance or instance to better align with common Java naming conventions for singletons.
   • Why it matters: Consistency in naming improves readability and maintainability for everyone working on the codebase.
   • File: src/main/java/frc/robot/statemachines/DriveState.java (Line 15)

2. DriveState.java - Logging Exceptions:

   • Suggestion: In grabVisionEstimateList(), instead of System.out.println, it might be helpful to use Logger.recordOutput() from AdvantageKit or DriverStation.reportWarning() for better visibility of exceptions during a match, especially since System.out.println output is often hard to access on the robot.
   • Why it matters: Using the robot's logging framework ensures that critical information, like exceptions, is captured and available for post-match analysis and debugging.
   • File: src/main/java/frc/robot/statemachines/DriveState.java (Line 46)

3. LaunchRequestBuilder.java & Subclasses - Singleton Pattern:

   • Suggestion: The current getInstance() abstract method in LaunchRequestBuilder and its implementations in MappedLauchRequestBuilder and ParabolicLaunchRequestBuilder seem to be attempting to implement a singleton pattern, but it's not quite correct for how Java singletons typically work, especially in an abstract class hierarchy.
     • An abstract class cannot enforce a singleton pattern for its subclasses directly through an abstract getInstance method returning an instance of itself. Each concrete builder class should manage its own static singleton instance if it's meant to be a singleton.
     • The implementation in MappedLauchRequestBuilder.getInstance() and ParabolicLaunchRequestBuilder.getInstance() is flawed. this.instance is an instance field, not a static one, so if (this.instance != null) will always be true after the first construction of any MappedLauchRequestBuilder instance, potentially leading to unexpected behavior or multiple instances.
   • Recommendation:
     • If the builders (MappedLaunchRequestBuilder, ParabolicLaunchRequestBuilder) are not intended to be singletons (i.e., you might want multiple instances with different targetPose suppliers), then remove the getInstance methods entirely and just use constructors like new MappedLaunchRequestBuilder(supplier).
     • If each concrete builder is intended to be a singleton, then each subclass should have its own private static MappedLaunchRequestBuilder s_instance = null; and a public static synchronized MappedLaunchRequestBuilder getInstance(Supplier<Pose3d> targetPose) method, with a private constructor. The targetPose supplier would then need to be passed to the singleton instance after creation if it's mutable, or the instance would need to be re-created if the supplier changes.
   • Why it matters: Correctly implementing design patterns like Singleton ensures predictable behavior and prevents subtle bugs related to state management.
   • Files: src/main/java/frc/robot/subsystems/shooter/LaunchRequestBuilder.java (Line 29), src/main/java/frc/robot/subsystems/shooter/MappedLauchRequestBuilder.java (Line 16), src/main/java/frc/robot/subsystems/shooter/ParabolicLaunchRequestBuilder.java (Line 15)

4. MappedLauchRequestBuilder.java - Typo in Class Name:

   • Suggestion: There's a small typo in the class name MappedLauchRequestBuilder. It should be MappedLaunchRequestBuilder.
   • Why it matters: Consistency and correctness in naming improves code clarity and searchability.
   • File: src/main/java/frc/robot/subsystems/shooter/MappedLauchRequestBuilder.java (Line 10)

5. MappedLauchRequestBuilder.java - Instance vs. Static Fields:

   • Suggestion: If MappedLaunchRequestBuilder is indeed meant to be a singleton, then lastHoodAngle, lastDriveAngle, hoodAngleFilter, and driveAngleFilter should be static fields. Currently, they are instance fields. This means each time getInstance() (if it were correctly implemented to return a new instance, or if multiple instances were created) is called, these filters and last values would be reinitialized, defeating their purpose for smoothing across calls.
   • Why it matters: Filters and "last value" tracking are typically stateful and should persist across calls to createLaunchRequest within the same logical builder. For a singleton, this means they should be static.
   • File: src/main/java/frc/robot/subsystems/shooter/MappedLauchRequestBuilder.java (Lines 37-41)

6. ShooterSubsystem.java - LaunchRequest Duplication:

   • Suggestion: The LaunchRequest inner class in ShooterSubsystem.java is now redundant and slightly different from the LaunchRequestBuilder.LaunchRequest record. Please remove the inner class from ShooterSubsystem and update any code that uses it to reference LaunchRequestBuilder.LaunchRequest.
   • Why it matters: Avoiding code duplication and using a single, consistent definition for a data structure prevents confusion and potential bugs when different parts of the code expect different fields or types.
   • File: src/main/java/frc/robot/subsystems/shooter/ShooterSubsystem.java (Lines 33-52)

Performance and Efficiency

1. MappedLauchRequestBuilder.java - Lookahead Loop:

   • Suggestion: The for (int i = 0; i < 20; i++) loop within createLaunchRequest() for calculating the lookahead pose runs 20 times every time this method is called. If createLaunchRequest() is called frequently (e.g., every periodic() cycle of the shooter subsystem), this could be computationally intensive.
   • Considerations:
     • Is this level of iteration strictly necessary for the desired accuracy? Could fewer iterations work?
     • Could this calculation be performed less frequently, perhaps on a separate thread if it's not critical for every 20ms cycle, or cached if the inputs don't change rapidly?
   • Why it matters: Unnecessary computations in periodic methods can consume valuable robot CPU time, potentially leading to slower control loops or other performance issues.
   • File: src/main/java/frc/robot/subsystems/shooter/MappedLauchRequestBuilder.java (Line 101)

2. ParabolicLaunchRequestBuilder.java - do-while Loop:

   • Suggestion: Similar to the lookahead loop, the do-while loop in createLaunchRequest() that adjusts slope could run many times. While it's trying to find an optimal solution, it's worth considering the performance impact if this method is called very frequently.
   • Considerations: Can we bound the number of iterations or use a more efficient search algorithm if this becomes a performance bottleneck?
   • Why it matters: Complex iterative calculations in periodic() methods can impact robot performance.
   • File: src/main/java/frc/robot/subsystems/shooter/ParabolicLaunchRequestBuilder.java (Line 42)

Safety & Error Handling

1. ParabolicLaunchRequestBuilder.java - null Return:
   • Suggestion: createLaunchRequest() can return null if motorAngle is too small. While this indicates an invalid state, returning null can lead to NullPointerException errors downstream if the calling code doesn't explicitly check for null.
   • Recommendation: Consider throwing a specific exception (e.g., InvalidLaunchRequestException) when a valid launch request cannot be generated. This makes the error condition explicit and forces the caller to handle it gracefully. Alternatively, you could return an Optional<LaunchRequest>.
   • Why it matters: Explicit error handling makes the code more robust and prevents runtime crashes, especially important in a competitive robotics environment.
   • File: src/main/java/frc/robot/subsystems/shooter/ParabolicLaunchRequestBuilder.java (Line 55)

Questions

• Singleton/Instance Management: Could you clarify the intended usage pattern for LaunchRequestBuilder and its subclasses? Are the builder instances themselves meant to be singletons, or are they designed to be instantiated multiple times with different targetPose suppliers? This will help us refine the getInstance method implementations.
• Performance Impact: What is the expected frequency of calling createLaunchRequest() in MappedLaunchRequestBuilder and ParabolicLaunchRequestBuilder? Understanding this will help assess the real-world performance impact of the iterative calculations.

Great work getting this architectural change in! It's clear a lot of thought went into structuring this for future development. Keep up the excellent 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!