Shoot on the fly (ﾉД`)

[Wu-Fan-529]

No description provided.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Suppressed comments (7)

src/main/java/frc/robot/commands/DriveManual.java:70

• The TOF-based pose prediction is gated on timeOfFlight == 0, which means the estimate only runs when the time delta is zero (so the transform is always 0). For shoot-on-the-fly, this should run when timeOfFlight is non-zero so the drivetrain pose is projected forward by the time of flight.

    if (RobotContainer.positionalInstance.timeOfFlight.equals(Seconds.zero())) {
      RobotContainer.drivetrainInstance.estimatedPoseOverTime = estimatePoseOverTime(
          RobotContainer.positionalInstance.timeOfFlight, velocities);
    }

src/main/java/frc/robot/commands/states/ShootingOnFly.java:56

• end() resets the command-local TOF, but it leaves RobotContainer.positionalInstance.timeOfFlight at its last non-zero value. That can cause DriveManual to keep projecting pose after the command ends; reset the shared field too.

  public void end(boolean interrupted) {
    TOF = Seconds.zero();
  }

src/main/java/frc/robot/commands/states/ShootingOnFly.java:17

• There’s a template comment appended to the import statement, which makes the import invalid-style and hurts readability. Remove the inline comment (or move it to a normal comment block) so the import stands alone.

import frc.robot.RobotContainer;
import frc.robot.subsystems.StateMachine.RobotState;/* You should consider using the more terse Command factories API instead https://docs.wpilib.org/en/stable/docs/software/commandbased/organizing-command-based.html#defining-commands */

src/main/java/frc/robot/commands/states/ShootingOnFly.java:22

• TOF is an instance variable but uses SCREAMING_SNAKE_CASE, which conflicts with the project’s lowerCamelCase variable naming convention. Rename it to something like timeOfFlight for consistency and readability.

This issue also appears on line 54 of the same file.

  Pose2d estimatedPoseOverTime = Pose2d.kZero;
  Time TOF = Seconds.zero();
  Distance distanceToTarget = Inches.zero();

src/main/java/frc/robot/commands/states/ShootingOnFly.java:46

• distanceToTarget is computed using target, but turret aiming uses Pose2d.kZero directly. This will desync distance vs. angle if target is ever changed from zero; use the target variable consistently.

    RobotContainer.positionalInstance.setTurretAngle(RobotContainer.drivetrainInstance
        .snapToTarget(estimatedPoseOverTime, Pose2d.kZero));
    RobotContainer.positionalInstance

src/main/java/frc/robot/constants/ConstPositional.java:26

• These maps are constants/configuration but are named in lowerCamelCase. In this codebase, constants in constants/ are expected to be SCREAMING_SNAKE_CASE to distinguish them from mutable fields; consider renaming (and updating call sites) to e.g. HOOD_ANGLE_MAP / TIME_OF_FLIGHT_MAP.

  public final static InterpolatingDoubleTreeMap hoodAngleMap = new InterpolatingDoubleTreeMap();
  public final static InterpolatingDoubleTreeMap timeOfFlightMap = new InterpolatingDoubleTreeMap();

src/main/java/frc/robot/constants/ConstFreeSpin.java:46

• This map is a constant/configuration value but is named in lowerCamelCase. Constants in this project are expected to be SCREAMING_SNAKE_CASE; consider renaming (and updating call sites) to something like FLYWHEEL_SPEED_MAP.

  public final static InterpolatingDoubleTreeMap flywheelSpeedMap = new InterpolatingDoubleTreeMap();
  static {
    // TODO: tune
    flywheelSpeedMap.put(Inches.of(120).in(Inches), RPM.of(3400).in(RPM));
    flywheelSpeedMap.put(Inches.of(0).in(Inches), RPM.of(3400).in(RPM));

[TaylerUva]

@Wu-Fan-529 merge conflicts

[TaylerUva]

merge conflicts

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

Suppressed comments (6)

src/main/java/frc/robot/subsystems/RobotPoses.java:46

• These pivot-point transforms embed several numeric offsets directly in a subsystem class and expose them as public fields. Per team conventions, physical/mechanical constants (offsets, dimensions) should live in constants/ and be referenced from there to avoid "magic numbers" and keep tuning in one place.

  public final Transform3d turretPivotPoint = new Transform3d(
      Units.Inches.of(-6.5),
      Units.Inches.of(6.5),
      Units.Inches.of(13.5),
      Rotation3d.kZero);
  public final Transform3d hoodPivotPoint = new Transform3d(
      Units.Inches.of(-11.75),
      Units.Inches.of(6.5),
      Units.Inches.of(15.5),
      Rotation3d.kZero);

src/main/java/frc/robot/commands/states/ShootingOnFly.java:72

• end() resets only the command-local TOF, but leaves RobotContainer.positionalInstance.timeOfFlight at its last non-zero value. Since DriveManual uses Positional.timeOfFlight to decide whether to estimate pose-over-time, this can cause stale TOF to keep affecting pose estimation after the command ends.

  @Override
  public void end(boolean interrupted) {
    TOF = Seconds.zero();
  }

src/main/java/frc/robot/commands/DriveManual.java:67

• Using Time.equals(Seconds.zero()) is a fragile way to test for "non-zero" (it relies on exact equality semantics and can be tripped up by floating-point representation). Prefer comparing the numeric magnitude instead.

    if (!RobotContainer.positionalInstance.timeOfFlight.equals(Seconds.zero())) {

src/main/java/frc/robot/subsystems/RobotPoses.java:24

• isOurShift is initialized to true and is never updated anywhere in the codebase, so the getPass() branch is currently unreachable and target will always be the hub. Either wire a setter/toggle so this can actually change at runtime, or remove the unused branch until that control path exists.

  boolean isOurShift = true;
  Pose2d target = Pose2d.kZero;

src/main/java/frc/robot/constants/ConstPositional.java:24

• These interpolation maps are public and mutable. Because they can be modified at runtime from anywhere, it's easy to accidentally change tuning mid-match. Consider making them private static final and accessing them only through Positional mapping methods (or ConstPositional accessor methods) to keep tuning data read-only at runtime.

  public final static InterpolatingDoubleTreeMap hoodAngleMap = new InterpolatingDoubleTreeMap();
  public final static InterpolatingDoubleTreeMap timeOfFlightMap = new InterpolatingDoubleTreeMap();

src/main/java/frc/robot/constants/ConstFreeSpin.java:58

• This interpolation map is public and mutable, which makes it easy for runtime code to accidentally alter shooter tuning. Prefer keeping the map private static final and exposing only read access via FreeSpin.getMappedFlywheelSpeed(...) (or a ConstFreeSpin accessor) so tuning remains effectively read-only.

  public final static InterpolatingDoubleTreeMap flywheelSpeedMap = new InterpolatingDoubleTreeMap();