Testing/feb28

[y0shi]

No description provided.

[github-actions]

AI Code Review

Hey team,

Great work on this pull request! It looks like you're making some really significant strides, especially with integrating multiple vision cameras and setting up new mechanisms. Overall, the changes show a strong commitment to good architecture and best practices.

Positive Highlights

• Multi-Camera Vision Integration: The addition of left and right cameras, along with the DriveState and VisionSubsystem updates to process them, is a fantastic improvement for robot localization. This will significantly boost your autonomous capabilities!
• Command Factory Methods: I'm seeing consistent use of command factory methods (e.g., startIndexerNoPID(), setHoodCommand()), which keeps your Subsystem APIs clean and encourages command-based programming.
• Tunability with Preferences: The extensive use of DoublePreference for PID constants and various thresholds across Drive, Indexer, and Shooter subsystems is excellent for allowing quick tuning without code redeploys.

Suggestions

Here are a few areas you might consider refining:

Code Quality & Organization

• RobotContainer.java - Duplicate disabled() binding:
  • Consider moving the RobotModeTriggers.disabled().whileTrue(drivetrain.applyRequest(() -> new SwerveRequest.Idle()).ignoringDisable(true)); line from configureTestBindings() into the RobotContainer constructor or a dedicated configureRobotBindings() method. This avoids code duplication and ensures the disabled behavior is consistently applied regardless of whether configureTestBindings() is called.

FRC/WPILib Best Practices

• IndexerSubsystem.java - periodic() commented code:
  • In IndexerSubsystem.java (line 83), the periodic() method has a commented-out section for setting the indexerMotor control with a TODO to "PUT IT BACK!". It's important to resolve this before merging. If this code is temporarily removed for testing, consider wrapping it in a conditional (e.g., a DriverStation.isTest() check) or creating a dedicated test command, rather than commenting it out in the main periodic() loop. This ensures the subsystem's intended behavior is clear and not accidentally left disabled.
• ShooterSubsystem.java - Flywheel Follower Control:
  • You've commented out the flywheelMotorFollower.setControl(new Follower(...)) in the constructor and are now explicitly setting control for both leader and follower in periodic(). While this works, using the Follower control mode built into CTRE Phoenix is generally more robust as the hardware handles the synchronization directly, potentially reducing latency or drift between motors.
  • Recommendation: If VelocityVoltage is the desired control mode for both, verify if Follower can accept this. If so, it might be slightly more efficient to revert to the Follower setup. If this change was intentional for a specific reason (e.g., different control modes for leader/follower, or advanced custom control), it might be helpful to add a comment explaining the rationale.

Java Standards

• ParabolicLaunchRequestBuilder.java - "Passing" Logic Clarity:
  • The do-while loop condition for calculating a and b in createLaunchRequest is a bit complex, especially with the passing boolean.
  • Recommendation: Double-check the logic carefully for both passing = true (e.g., passing over a defense or to an alliance partner) and passing = false (shooting into the hub). Specifically, if passing is true, the hitWallCheckX and HUB_HEIGHT condition (!passing && a * Math.pow(hitWallCheckX, 2) + b * hitWallCheckX + y1 < ShooterConstants.HUB_HEIGHT.in(Meters)) will always be false, meaning the loop only checks motorAngle < MIN_HOOD_ANGLE. This might be the intended behavior, but clarifying comments on the physics for each passing scenario would make it easier to understand and verify.

Javadoc Documentation

• VisionSubsystem.java - periodic() comment:
  • In VisionSubsystem.java (line 52), the comment // this makes sure that the different parts of the periodic use different stats is a good start.
  • Recommendation: Consider expanding this to explain why taking a snapshot of driveStats at the beginning of periodic() is important. For example, "This ensures that all vision processing within a single periodic() cycle operates on a consistent snapshot of the robot's drive state, preventing potential race conditions or inconsistencies if the drive state were to update mid-cycle."

Questions

• Indexer/Accelerator PID Tuning: I noticed that all the new PID constants for the IndexerSubsystem accelerator motor are currently set to 0. Are there plans to tune these soon, or will they primarily be used in "no PID" percent output modes for now?

Keep up the fantastic work! It's awesome to see the team pushing forward with these improvements.

Best,
Your Friendly AI Code Reviewer

---

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!