Debug/launch target velocity

[Orcasphynx]

No description provided.

[github-actions]

AI Code Review

Hello team!

Great work on this pull request, it looks like a focused effort to refine your auto-aiming logic.

Summary

This pull request streamlines the auto-aiming rotational control by removing the explicit targetRobotAngularVelocity from the LaunchRequest and adjusting how the D-term for rotational control is calculated in RobotContainer. This simplifies the LaunchRequest and focuses the rotational control on damping oscillations around the target angle.

Positive Highlights

• Code Simplification: You've effectively removed unnecessary fields and calculations related to targetRobotAngularVelocity from LaunchRequest, LaunchCalculator, and LaunchRequestBuilder, which improves the clarity and maintainability of these classes.
• Intentional Control Logic: The updated kD term calculation in RobotContainer (using the rate of change of the current robot angle) is a common and effective way to implement a damping term in a position-based PID loop, which should help with more stable aiming.
• Consistent Refactoring: The changes are applied consistently across all affected files, demonstrating thoroughness in updating the API.

Suggestions

Code Quality & FRC/WPILib Best Practices

1. D-Term Calculation Clarity in RobotContainer.java:

   • File: src/main/java/frc/robot/RobotContainer.java (lines 317-325 and 341-349)
   • Suggestion: The comment for the kD term // kD * rate of error ((target - currentAngle - (target - previousAngle))/period = (previousAngle - currentAngle)/period) is very helpful! It correctly describes the mathematical equivalence. However, to make the intent of the D-term even clearer for future readers, you could consider adding a brief note that this term primarily provides damping based on the robot's current angular velocity, helping to prevent overshoots and oscillations.
   • Why it matters: While the math is correct, explicitly stating the control purpose (damping) can make the code's behavior more immediately understandable, especially for new team members learning about PID control.

   Example adjustment:

   // kD * rate of change of current angle (effectively damping based on current angular velocity)
   + DrivePreferences.autoAim_kD.getValue()
       * driveState
           .getPreviousDriveStats()
           .Pose
           .getRotation()
           .minus(driveState.getCurrentDriveStats().Pose.getRotation())
           .getRadians()
       / (driveState.getCurrentDriveStats().Timestamp
           - driveState.getPreviousDriveStats().Timestamp);

2. Robustness of delta_t in RobotContainer.java:

   • File: src/main/java/frc/robot/RobotContainer.java (lines 323-325 and 347-349)
   • Suggestion: You're calculating delta_t as (driveState.getCurrentDriveStats().Timestamp - driveState.getPreviousDriveStats().Timestamp). For TimedRobot or LoggedRobot, this delta should be fairly consistent (e.g., 20ms). However, it's good practice to consider edge cases where delta_t could theoretically be zero or very small (e.g., if getCurrentDriveStats() and getPreviousDriveStats() somehow return the same timestamp, or if the robot loop stalls). While unlikely in a well-functioning WPILib robot, adding a small check or clamping delta_t to a minimum positive value could prevent division by zero or extremely large D-terms.
   • Why it matters: Division by zero or a very small number can lead to NaN or Infinity in your rotational rate, which can cause unpredictable robot behavior.
   • Consider:

     double deltaTime = driveState.getCurrentDriveStats().Timestamp - driveState.getPreviousDriveStats().Timestamp;
     // Ensure deltaTime is not zero or excessively small to prevent division by zero or huge D-terms
     if (deltaTime < 1e-6) { // e.g., 1 microsecond
         deltaTime = 1e-6; // Use a very small positive number instead
     }
     double rotationalRate = ... + DrivePreferences.autoAim_kD.getValue() * ... / deltaTime;

     This is a minor robustness improvement and might not be strictly necessary given WPILib's timing, but it's a good thought exercise.

Questions

• Could you explain the reasoning behind switching from a velocity-based D-term (kD * (target_angular_velocity - current_angular_velocity)) to a damping-based D-term (kD * (-current_angular_velocity)) for the auto-aim? What behavior were you observing that prompted this change?

Keep up the fantastic work! This kind of iterative refinement on control systems is crucial for a high-performing robot.

---

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!