Feature/new shooter by y0shi · Pull Request #79 · IgniteRobotics/Robot-2026

y0shi · 2026-03-31T23:11:25Z

No description provided.

Co-authored-by: TheGreatPintoJ <TheGreatPintoJ@users.noreply.github.com>

github-actions · 2026-03-31T23:12:09Z

AI Code Review

Hey team!

This pull request introduces some significant changes for the new shooter, including motor ID updates and a shift in control strategies. It's great to see the progress here, and I appreciate the effort in updating the code to reflect the new hardware setup.

Positive Highlights:

Centralized Motor IDs: The motor-ids.txt file is a clear and helpful way to manage CAN IDs, which is a good practice for organization.
Safe Command Termination: The use of runEnd() in IndexerSubsystem's "no PID" commands is an excellent safety improvement, ensuring motors stop when commands are interrupted or finish. This is a very important detail!
Command Factory Methods: The new commands in ShooterSubsystem and RobotContainer (e.g., setHoodTargetCommand(), setFlywheelOutputCommand(), collectCommand()) correctly encapsulate behavior and keep subsystem internals private, which aligns well with our guidelines.
Hardware Abstraction: The setFlywheelMotorOutput() method in ShooterSubsystem effectively abstracts the control of the new three-motor flywheel setup, making the code cleaner and easier to manage.

Suggestions:

Code Quality & Organization

Commented-Out Code: I've noticed a substantial amount of commented-out code in Robot.java, RobotContainer.java, IndexerSubsystem.java, ShooterConstants.java, ShooterPreferences.java, and ShooterSubsystem.java.
- Why it matters: While this is perfectly normal during active development and experimentation, large blocks of commented code can make it harder for others (and your future self!) to read, understand, and maintain the active parts of the codebase. It can also make the code appear more complex than it is.
- Consider: If the commented code is truly no longer needed (e.g., old control schemes that won't be revisited), it might be helpful to remove it entirely. If it's temporarily disabled for a feature or tuning, adding a clear TODO comment explaining its purpose and when it will be re-enabled or removed would be great. For major architectural shifts, sometimes moving old implementations to a legacy or deprecated package can be a good way to archive them without cluttering the active code.
Robot.java - autonomousInit() Logging: The autonomousInit() block was commented out, which removed the match logging lines that were there previously.
- Why it matters: Explicit match logging (e.g., Logger.recordOutput("Match/AutonomousStart", Timer.getFPGATimestamp());) is a custom guideline and crucial for post-match analysis.
- Consider: Re-adding these match logging lines to autonomousInit() would ensure we continue to capture valuable data during autonomous periods.
IndexerPreferences.java and ShooterPreferences.java - Unused Preferences: Many preferences have been commented out.
- Why it matters: Similar to commented-out code, these can clutter the preferences file if they are truly not used.
- Consider: If these preferences are permanently removed from the control scheme, it might be beneficial to delete them entirely. If they are intended to be re-enabled later (e.g., for PID tuning), adding TODO comments explaining their future use would be helpful.
motor-ids.txt - File Format: While motor-ids.txt is clear and works, it's not a standard Java file.
- Why it matters: For consistency and to leverage Java's type safety and build process, motor IDs are often defined as public static final int constants within a Constants.java file or a subsystem's Constants class. This makes them compile-time checked and easier to refactor with IDE tools.
- Consider: Moving these IDs into frc.robot.Constants.java or directly into the relevant subsystem constants files (e.g., IndexerConstants.java, ShooterConstants.java) might be a good next step.

FRC/WPILib Best Practices

IndexerSubsystem.java & ShooterSubsystem.java - Control Mode Transition: The shift from PID velocity/position control to direct percent output for the indexer and flywheel is a significant change.
- Why it matters: Direct percent output can be less consistent and harder to tune for optimal performance compared to closed-loop PID control, especially under varying loads (e.g., different note types, wear and tear). It's great for initial testing and getting things moving!
- Consider: Clearly documenting the rationale for this change, perhaps in a TODO comment, would be helpful. If PID control is the long-term goal, ensuring that the commented-out PID-related variables and control logic (e.g., velocityTarget, velocityControl) are re-integrated when tuning begins would be important. The existing TODO: PUT IT BACK! comments are a good start!
ShooterSubsystem.java - Hood Control in periodic(): The line // hoodMotor.setControl(hoodControl.withPosition(hoodTarget)); is commented out in periodic().
- Why it matters: WPILib's PositionVoltage control (and most CTRE controls) needs to be continuously commanded in periodic() for the motor to hold its position or move to a target. If this line remains commented, the hood motor will not respond to setHoodTargetCommand() and will not actively hold its position.
- Consider: Unless the hood is not intended to be PID-controlled at all, this line should be active. If the hood is controlled by PositionVoltage, it should always be commanding its target in periodic() to ensure proper operation.
ShooterSubsystem.java - Automatic Homing: The homeShooterCommand() is commented out.
- Why it matters: Automatic homing is a critical custom guideline for us. For mechanisms without absolute encoders, homing at startup is essential for the robot to know the mechanism's true position and operate safely within its limits. Without it, the hood could potentially crash into its physical limits or operate at incorrect positions.
- Consider: Re-implementing automatic homing for the hood, or confirming that the hood motor is equipped with an absolute encoder that provides a reliable zero reference at all times, would be a good safety measure.
RobotContainer.java - LaunchState Inconsistency: LaunchState is imported in Robot.java but its usage is commented out. In RobotContainer.java, the getDriveAndLaunchRequest() method still heavily relies on LaunchState, but the commands that would call this method are commented out.
- Why it matters: Inconsistent usage makes the code confusing and can lead to unexpected behavior if parts are re-enabled without full context.
- Consider: Consolidating the plan for LaunchState. If it's being removed, remove all references. If it's being refactored, ensuring its integration is consistent across files would be helpful.

Performance and Efficiency

RobotContainer.java - Duplicate Command: The line driverJoystick.x().onTrue(intake.stopRollerNoPID()); appears twice, once commented out and once active.
- Why it matters: While minor, it's code duplication that can lead to confusion if changes are made to one instance but not the other.
- Consider: Removing the commented-out duplicate line would clean this up.

Questions:

Could you elaborate a bit on the long-term plan for the indexer and flywheel control? Is the current percent output approach temporary for initial testing, with PID control planned for later, or is this the intended final control strategy?
What is the plan for automatic homing for the hood mechanism, especially since homeShooterCommand() is currently commented out? Is there an absolute encoder providing a zero reference for the hood?
Regarding LaunchState, is it intended to be removed, refactored, or is its current commented-out state temporary?

Keep up the great work on the new shooter! It's clear a lot of thought has gone into updating the hardware and initial control. I'm excited to see it in action!

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

github-actions · 2026-03-31T23:12:22Z