Skip to content

Feature/shooter#21

Merged
Orcasphynx merged 8 commits intodevelopmentfrom
feature/shooter
Feb 18, 2026
Merged

Feature/shooter#21
Orcasphynx merged 8 commits intodevelopmentfrom
feature/shooter

Conversation

@Orcasphynx
Copy link
Collaborator

No description provided.

@github-actions
Copy link

github-actions bot commented Feb 14, 2026

AI Code Review

Hey team! This is a fantastic start to the shooter subsystem, it's great to see so much progress on this complex mechanism! You're clearly putting a lot of thought into following best practices, which is awesome.

Positive Highlights

  • WPILib Units System: Excellent use of edu.wpi.first.units throughout ShooterConstants and ShooterSubsystem to prevent conversion errors and improve clarity. This is a huge win for robust robot code!
  • Command Factory Methods: You've done a great job exposing subsystem functionality through command factory methods (e.g., spinFlywheelCommand(), setHoodCommand(), homeShooterCommand()). This aligns perfectly with our command-based architecture and makes commands easy to use.
  • AdvantageKit Logging: Using @Logged for the subsystem and critical variables like velocityTarget and hoodTarget is a fantastic practice for debugging and analysis with AdvantageKit.
  • Homing Command: The homeShooterCommand() using current sensing is a clever and robust way to home a mechanism without an absolute encoder. Great job on that!

Suggestions

Here are a few suggestions to consider as you continue to develop the shooter:

Code Quality & Readability

  • ShooterConstants.java - Hood Limits and Error Tolerance:
    • Consider making ALLOWABLE_HOOD_ERROR, HOOD_FORWARD_LIMIT, and HOOD_REVERSE_LIMIT use Measure<Angle> from the WPILib Units system, similar to how SHOOTER_HEIGHT or GOAL_HEIGHT use Measure<Distance>. This will ensure type safety and prevent unit mix-ups.
    • Line 35: public static final double ALLOWABLE_HOOD_ERROR = 0.1;
    • Line 36-37: public static final double HOOD_FORWARD_LIMIT = 100; and public static final double HOOD_REVERSE_LIMIT = 0;
  • ShooterConstants.java - ROTATIONS_PER_LAUNCH_DEGREE Clarity:
    • Line 62: public static final Angle ROTATIONS_PER_LAUNCH_DEGREE = Rotations.of(1);
    • The name ROTATIONS_PER_LAUNCH_DEGREE implies a conversion ratio, but Rotations.of(1) is just 1 rotation. If this is intended to be a gear ratio (e.g., 1 motor rotation per degree of hood angle), it might be clearer to rename it to something like HOOD_GEAR_RATIO_ROTATIONS_PER_DEGREE and ensure the unit type (RotationsPerDegree) reflects that. If it's a direct 1:1, then a simple HOOD_GEAR_RATIO = 1.0 (or Unitless.of(1.0)) might be more appropriate, and the conversion in LaunchRequest might simplify.
  • ShooterSubsystem.java - Magic Numbers in createLaunchRequest:
    • Lines 135 & 148: The gravitational acceleration (9.8) and the slope -= 0.05 increment are currently "magic numbers." It might be helpful to define these as named constants in ShooterConstants (e.g., GRAVITY_ACCELERATION = MetersPerSecondPerSecond.of(9.8) and SLOPE_ADJUSTMENT_INCREMENT = 0.05). This improves readability and makes tuning easier.
  • Javadoc Documentation:
    • Consider adding Javadoc comments for the LaunchRequest class and the createLaunchRequest method in ShooterSubsystem.java. These complex parts of the code would greatly benefit from explanations of their purpose, parameters, and return values, especially regarding the physics calculations.
    • Also, adding Javadoc to your public command factory methods in ShooterSubsystem would be very helpful for other team members using them.

FRC/WPILib Best Practices

  • ShooterSubsystem.java - periodic() Control Request:
    • Line 86: hoodMotor.setControl(hoodControl.withVelocity(hoodTarget.in(Rotations)));
    • You're using PositionTorqueCurrentFOC for hoodControl, but in periodic(), you're calling withVelocity(). This looks like a small typo! It should likely be hoodControl.withPosition(hoodTarget.in(Rotations)) to set the hood's target position.
  • ShooterSubsystem.java - Cross-Subsystem Constant:
    • Line 105: return Math.abs(hoodMotor.getPosition().getValueAsDouble() - hoodTarget.in(Rotations)) < IntakeConstants.ALLOWABLE_EXTENSION_ERROR;
    • It's generally best practice for subsystems to manage their own constants. IntakeConstants.ALLOWABLE_EXTENSION_ERROR is used here, but the shooter should ideally have its own ALLOWABLE_HOOD_ERROR (which you've defined in ShooterConstants.java). This prevents dependencies between unrelated subsystems and makes it clearer where to find tuning values for the shooter.
  • ShooterSubsystem.java - Homing Command Logic:
    • Lines 120-123: In homeShooterCommand(), the hoodMotor.setPosition(0) call happens in the runEnd() callback. This means the encoder position is reset after the command finishes (or is interrupted). For homing, it's often more precise to set the position immediately when the homing condition is met. Consider moving hoodMotor.setPosition(0) inside the until predicate's block, right after you confirm the current threshold is met, or by using a Commands.runOnce() after the waitUntil.
    • Line 118: hoodMotor.set(ShooterConstants.SAFE_HOMING_EFFORT.Output) uses set() with a raw double. While it works, it's generally cleaner to use setControl() with the DutyCycleOut control request directly, e.g., hoodMotor.setControl(ShooterConstants.SAFE_HOMING_EFFORT).
  • ShooterConstants.java - Enable Software Limits:
    • Lines 44-45: configs.ForwardSoftLimitEnable = false; and configs.ReverseSoftLimitEnable = false;
    • Currently, the hood's software limits are disabled. Once you have physical limits for the hood, remember to enable these (true) and set the ForwardSoftLimitThreshold and ReverseSoftLimitThreshold to safe values to protect the mechanism from over-extending or over-retracting.

Questions

  • ShooterSubsystem.java - LaunchRequest as an Inner Class: The LaunchRequest class is currently an inner class. Does it need access to ShooterSubsystem's private members? If not, it could be made static to reduce implicit coupling and make it slightly more independent. This is a minor point, but good for design discussions!

This is really strong work on the shooter subsystem! You've laid a great foundation with good structure and adherence to many of our guidelines. 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
Copy link

✓ Build successful and code formatting check passed!

@github-actions
Copy link

✓ Build successful and code formatting check passed!

@github-actions
Copy link

✓ Build successful and code formatting check passed!

@Orcasphynx Orcasphynx merged commit 18b1c72 into development Feb 18, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant