Ball counter

[Fatblabs]

No description provided.

[gemini-code-assist]

Code Review

This pull request introduces ball counting logic to the Flywheels subsystem by monitoring velocity derivatives and adds a method to verify if the flywheel has maintained its target speed for a specific duration. Feedback focuses on critical logic errors in the duration check and the ball counter, specifically regarding the handling of reset states and the failure to update tracking variables. Improvements were also suggested for initial variable states and NetworkTable topic naming consistency.

I am having trouble creating individual review comments. Click here to see my feedback.

src/main/java/frc/robot/subsystems/launcher/Flywheels.java (178-199)

The current logic for hasBeenAtTargetFor will return true even when the flywheel is NOT at the target velocity. When atTarget is false, timeEnteredTargetZone is set to -1, causing the check (now - (-1)) >= duration to evaluate to true (since now + 1 is likely greater than duration). The check should only proceed if a valid start time has been recorded.

  public boolean hasBeenAtTargetFor(double durationSeconds) {
    boolean atTarget = atTargetVelocity(targetVelocity.get(), FLYWHEEL_TOLERANCE);
    if (!atTarget) {
      timeEnteredTargetZone = -1;
      return false;
    }
    if (timeEnteredTargetZone < 0) {
      timeEnteredTargetZone = Timer.getFPGATimestamp();
    }
    boolean hasStopped = (Timer.getFPGATimestamp() - timeEnteredTargetZone) >= durationSeconds;
    if (hasStopped) {
      resetCachedValues();
    }
    return hasStopped;
  }

src/main/java/frc/robot/subsystems/launcher/Flywheels.java (216-226)

The ball counter logic is currently broken because lastTime and lastRPS are never updated. This causes dt to grow indefinitely and slope to be calculated against an initial lastRPS of 0, eventually making the counter unresponsive. These variables must be updated whenever the slope calculation is performed.

    double currentTime = Timer.getFPGATimestamp();
    double dt = currentTime - lastTime;

    if (dt >= 0.02) {
      double slope = (currentRPS - lastRPS) / dt;
      if (slope < minDerivative && !hasBall) {
        hasBall = true;
      } else if (slope > maxDerivative && hasBall) {
        hasBall = false;
        ballCount++;
      }
      lastRPS = currentRPS;
      lastTime = currentTime;
    }

src/main/java/frc/robot/subsystems/launcher/Flywheels.java (60)

Initializing timeEnteredTargetZone to 0 can cause hasBeenAtTargetFor to return true prematurely if the robot has been running for longer than the target duration, as the logic checks (now - 0) >= duration. It should be initialized to -1 to indicate no active timer.

  private double timeEnteredTargetZone = -1;

src/main/java/frc/robot/subsystems/launcher/Flywheels.java (73)

For consistency with the other NetworkTable topics in this class (lines 74-75), consider using the /launcher/ prefix for the ball counter topic.

    ballPub = nt.getIntegerTopic("/launcher/ballsShot").publish();

[Fatblabs]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces ball counting logic by monitoring flywheel velocity derivatives and adds a utility method to verify if the target velocity has been sustained for a specific duration. Several critical logic issues were identified: the duration check in hasBeenAtTargetFor incorrectly handles reset states, leading to premature success; the periodic update loop fails to refresh the previous velocity and timestamp, which breaks the derivative calculation; and initial variable states require adjustment to prevent startup spikes or false triggers.