Feature/shot logging

[Orcasphynx]

No description provided.

[github-actions]

✓ Build successful and code formatting check passed!

[github-actions]

AI Code Review

Hey team!

This pull request is making some really solid progress on the shooter logic and is doing a fantastic job integrating modern WPILib practices. I'm especially impressed with the logging and unit safety!

Positive Highlights:

• Excellent Logging with AdvantageKit: The widespread use of @Logged annotations in LaunchState and LaunchRequest, especially with Importance.CRITICAL, is a huge win! This will be incredibly valuable for debugging, analysis, and leveraging deterministic replay. It's great to see SmartDashboard calls being replaced by this more powerful logging.
• Type-Safe Units: The consistent application of edu.wpi.first.units.Units for Angle, AngularVelocity, and Distance throughout LaunchRequest, ShooterConstants, and LaunchCalculator is a fantastic practice. This significantly reduces the risk of conversion errors and makes the code much clearer and more robust.
• Sophisticated Launch Calculation: The iterative lookaheadPose calculation in LaunchCalculator demonstrates a thoughtful and advanced approach to accounting for robot movement during projectile flight, which is crucial for accurate shooting.

Suggestions:

Code Quality & Readability

• RobotContainer.java - Rotational Rate Calculation Clarity: The getDriveAndLaunchRequest() method's rotationalRate calculation is quite dense.

  • Consider: Breaking this calculation down into a few intermediate variables or a helper method. This would make each component of the PID-like calculation clearer and easier to understand, debug, and modify.
  • Why it matters: As logic becomes more complex, clear variable names and breaking down expressions help future you (and your teammates!) understand the code's intent without having to parse a long line.

• networktables.json.bck - File Naming: The .bck extension usually means it's a temporary or backup file.

  • Consider: Renaming this file to networktables.json if these are the intended NetworkTables preferences for the robot. If it's truly a backup, it might be better to remove it from the pull request to avoid confusion.
  • Why it matters: The .bck extension might prevent the robot from loading these preferences, and it can also cause confusion about which preference file is the active one.

FRC/WPILib Best Practices

• ShooterSubsystem.java - Disconnect between LaunchState and Subsystem Control: Currently, launchState.refreshRequest() is called in periodic(), but velocityTarget and hoodTarget in ShooterSubsystem are only updated by specific commands (e.g., spinFlywheelCommand, setHoodCommand) and do not directly read from launchState.getLaunchRequest().

  • Consider:
    1. Clarifying Intent: If the ShooterSubsystem is meant to always follow the LaunchState's calculated request when active, then periodic() should read from launchState.getLaunchRequest() to set velocityTarget and hoodTarget.
    2. Explicit Command: If LaunchState is just for calculation, and commands explicitly trigger its use, then the periodic() call to launchState.refreshRequest() might be redundant if no other part of the system is actively consuming the currentLaunchRequest every periodic cycle.
  • Why it matters: A clear flow of control and data is crucial for debugging and maintaining complex robot behaviors. The current setup could lead to confusion about which values are actually controlling the shooter.

• ShooterSubsystem.java - Hood Motor Control in periodic(): I noticed that the hoodMotor is not currently being set with its hoodControl in the periodic() method.

  • Consider: Adding hoodMotor.setControl(hoodControl.withPosition(hoodTarget.in(Rotations))); to periodic() so the hood actively tracks its hoodTarget.
  • Why it matters: Without this, the hood motor will not move to its target position unless explicitly commanded by a non-periodic control, which is usually not the intent for a PID-controlled mechanism.

• ShooterSubsystem.java - Homing Command Safety: The homeShooterCommand() currently calls hoodMotor.setPosition(0) in the onEnd block. This means it will set the position to 0 even if the command is interrupted before homing is complete.

  • Consider: Moving hoodMotor.setPosition(0) inside the until condition, after the current limit is detected and before the command ends. This ensures the encoder is reset only when homing is successfully achieved.
  • Why it matters: Incorrectly zeroing an encoder can lead to dangerous mechanism movements if the robot thinks it's at a safe position when it's not.

• ShooterSubsystem.java - Direct Motor set() for Launch: The launchLemonsCommandNoPID() uses flywheelMotorLeader.set(...) with a duty cycle percentage.

  • Consider: While the name implies "no PID," it's generally best practice to use the configured PID control (e.g., VelocityVoltage or DutyCycleOut with a specific target) when possible. If PID tuning is the issue, focusing on that might yield better results. If this is strictly for testing or an emergency override, documenting its purpose more explicitly would be helpful.
  • Why it matters: Bypassing PID can lead to less consistent performance, especially under varying battery voltage or load, and complicates the debugging process if PID is expected to be used elsewhere.

• Robot.java - Match Logging (General Reminder): This isn't in the files you changed, but as a general reminder, it's a good practice to ensure Robot.java includes explicit Logger.recordOutput() calls in autonomousInit() and teleopInit() (and disabledInit(), robotInit() if desired) to log match start times and selected auto routines.

  • Why it matters: This creates a robust log of match events, which is super helpful for analyzing robot performance and identifying issues in post-match analysis.

Performance and Efficiency

• LaunchCalculator.java - Iterative Lookahead Calculation: The for (int i = 0; i < 20; i++) loop runs every periodic cycle inside refreshRequest(). While 20 iterations is a small number, it's good to be mindful of its impact.
  • Consider: Monitoring the robot's loop time closely. If this becomes a bottleneck, you might explore if fewer iterations would be sufficient without significant accuracy loss, or if a more analytical solution is feasible. For now, it's likely fine, but it's good to keep an eye on performance.
  • Why it matters: Excessive computations in periodic methods can cause the robot's control loop to run slower than intended, leading to inconsistent behavior.

Java Standards & Type-Safe Units

• LaunchCalculator.java - Consistent Unit Usage: loopPeriodSecs and phaseDelay are currently doubles.

  • Consider: Defining these as Measure<Time> (e.g., Seconds.of(0.02)) for consistency with other unit-safe code.
  • Why it matters: Maintaining type-safe units throughout helps prevent subtle conversion bugs and makes the code's intent clearer.

• ShooterSubsystem.java - Initializing Control Requests with Units: velocityControl and hoodControl are initialized with raw 0 values.

  • Consider: Initializing them with unit-safe values for clarity, like velocityControl = new VelocityVoltage(RotationsPerSecond.of(0)); and hoodControl = new PositionTorqueCurrentFOC(Rotations.of(0));.
  • Why it matters: This reinforces the use of type-safe units and makes the initial state more explicit.

Safety & Error Handling

• ShooterConstants.java - Hood Software Limits Disabled: The createHoodSoftwareLimitSwitchConfigs() method sets ForwardSoftLimitEnable = false and ReverseSoftLimitEnable = false.
  • Consider: Unless there's a specific reason to disable them (e.g., using robust hardware limits instead), it's generally safer to enable software limits to prevent the mechanism from moving beyond its physical range.
  • Why it matters: Software limits provide a crucial layer of protection against mechanical damage or unsafe robot operation.

Javadoc Documentation

• ShooterSubsystem.java - Public Method Javadoc: Many public methods in ShooterSubsystem (e.g., spinFlywheelCommand, stowHood) could benefit from Javadoc comments.
  • Consider: Adding Javadoc comments explaining what each public method does, its parameters, and what it returns.
  • Why it matters: Good documentation helps other team members understand how to use the subsystem's API, especially as the codebase grows.

Questions:

• Could you clarify the intended interaction between LaunchState.refreshRequest() in ShooterSubsystem.periodic() and how ShooterSubsystem's velocityTarget and hoodTarget are meant to be updated? Are they supposed to directly follow LaunchState's request, or are commands meant to explicitly set them based on ShooterPreferences?

Keep up the great work! Your attention to detail with logging and unit safety is really paying off for building a robust robot.

---

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