Fully working shooter + dependencies#5
Merged
Merged
Conversation
NEEDS TO BE REVIEWED. This was 10x programmed in 40 minutes
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds coordinated velocity-based shooting: new ParallelCommandGroup IndexIntoShooterAndShoot bound in RobotContainer, new continuous "run at velocity" commands for belt/kicker/deflector/shooter, centralized subsystem constants with clamping gated by LIMITS_ENABLED, deflector IO gains velocity PID support, and logging keys standardized. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Operator
participant RobotContainer
participant IndexGroup as IndexIntoShooterAndShoot
participant BeltCmd as DefaultBeltRunAtVelocity
participant KickerCmd as DefaultKickerRunAtVelocity
participant ShooterCmd as ShooterShootAtVelocity
participant BeltSub as BeltSubsystem
participant KickerSub as KickerSubsystem
participant ShooterSub as ShooterSubsystem
Operator->>RobotContainer: press shoot button
RobotContainer->>IndexGroup: enable whileTrue binding
IndexGroup->>BeltCmd: start (parallel)
IndexGroup->>KickerCmd: start (parallel)
IndexGroup->>ShooterCmd: start (parallel)
BeltCmd->>BeltSub: runBeltToTargetVelocity(supplier)
KickerCmd->>KickerSub: runToTargetVelocity(supplier)
ShooterCmd->>ShooterSub: runToTargetVelocity(supplier)
Note right of BeltSub: clamp via BeltConstants.Limits (rgba(0,128,0,0.5))
Note right of KickerSub: clamp via KickerConstants.Limits (rgba(0,128,255,0.5))
Note right of ShooterSub: clamp via ShooterConstants.Limits (rgba(255,128,0,0.5))
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/constants/DeflectorTalonFXSConstants.java (2)
25-26:⚠️ Potential issue | 🟡 MinorStale comment references "turret mechanism" — should say "deflector mechanism".
This looks like a copy-paste artifact from a turret subsystem.
📝 Proposed fix
- // Gear ratio between the turret mechanism and the motor sensor 10 / 1 being a reduction + // Gear ratio between the deflector mechanism and the motor sensor 100 / 1 being a reduction public static final double SENSOR_TO_MECHANISM_RATIO = 100.0 / 1.0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/constants/DeflectorTalonFXSConstants.java` around lines 25 - 26, The comment above SENSOR_TO_MECHANISM_RATIO in DeflectorTalonFXSConstants is a stale copy-paste referring to "turret mechanism" — update the comment to reference the deflector mechanism (e.g., "Gear ratio between the deflector mechanism and the motor sensor") so it matches the class and constant meaning; locate SENSOR_TO_MECHANISM_RATIO in class DeflectorTalonFXSConstants and replace "turret" with "deflector" in the comment.
21-23:⚠️ Potential issue | 🔴 CriticalAdd missing velocity PID constants and configure Slot1 in the motor controller.
The
setDeflectorVelocity()method usesPIDSlots.VELOCITY(Slot1), but Slot1 is never configured with PID gains inDeflectorIOTalonFXS. This causes velocity commands to use unconfigured/default firmware values instead of tuned gains.Add velocity PID constants to
DeflectorTalonFXSConstants:public static final double VELOCITY_P_GAIN = 2.5; public static final double VELOCITY_I_GAIN = 0.0; public static final double VELOCITY_D_GAIN = 0.0; public static final double VELOCITY_V_GAIN = 0.125; public static final double VELOCITY_S_GAIN = 0.0090433;Then configure Slot1 in the
DeflectorIOTalonFXSconstructor after the Slot0 configuration:deflectorMotorConfig.Slot1.kP = DeflectorTalonFXSConstants.VELOCITY_P_GAIN; deflectorMotorConfig.Slot1.kI = DeflectorTalonFXSConstants.VELOCITY_I_GAIN; deflectorMotorConfig.Slot1.kD = DeflectorTalonFXSConstants.VELOCITY_D_GAIN; deflectorMotorConfig.Slot1.kV = DeflectorTalonFXSConstants.VELOCITY_V_GAIN; deflectorMotorConfig.Slot1.kS = DeflectorTalonFXSConstants.VELOCITY_S_GAIN;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/constants/DeflectorTalonFXSConstants.java` around lines 21 - 23, Add missing velocity PID constants to DeflectorTalonFXSConstants (VELOCITY_P_GAIN=2.5, VELOCITY_I_GAIN=0.0, VELOCITY_D_GAIN=0.0, VELOCITY_V_GAIN=0.125, VELOCITY_S_GAIN=0.0090433) and then configure Slot1 in the DeflectorIOTalonFXS constructor (immediately after the existing Slot0 configuration) by assigning Slot1.kP, kI, kD, kV and kS from those new constants so PIDSlots.VELOCITY (used by setDeflectorVelocity) uses the tuned gains instead of firmware defaults.src/main/java/frc/alotobots/rebuilt/subsystems/launcher/turret/TurretSubsystem.java (1)
79-92:⚠️ Potential issue | 🔴 Critical
targetAnglefield is never updated —isAtTargetAngle()always compares against 0°
runToTargetAnglecorrectly clamps and commands the hardware, but it never writes the result back tothis.targetAngle. BecausetargetAnglestays atDegrees.zero()(its initialization value at line 42),isAtTargetAngle()always computes the error against 0 degrees instead of the intended setpoint — meaning the turret will report "at target" only when it happens to be pointing straight ahead, regardless of what was commanded.As a secondary issue on line 91:
Logger.recordOutput("Turret/AdjustedTargetAngle", targetAngle)also logs this stale zero instead of the clamped value; it should referenceRadians.of(adjustedAngle).🐛 Proposed fix
public void runToTargetAngle(Angle angle) { var adjustedAngle = MathUtil.clamp(angle.in(Radian), Degrees.of(-90).in(Radian), Degrees.of(90).in(Radians)); + targetAngle = Radians.of(adjustedAngle); try { io.setTurretPosition(Radians.of(adjustedAngle), TurretIO.PIDSlots.DEFAULT_POSITION); } catch (Exception e) { System.out.println(e.getMessage()); } Logger.recordOutput("Turret/TargetAngle", angle); - Logger.recordOutput("Turret/AdjustedTargetAngle", targetAngle); + Logger.recordOutput("Turret/AdjustedTargetAngle", targetAngle); // now logs the clamped value }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/turret/TurretSubsystem.java` around lines 79 - 92, runToTargetAngle currently computes adjustedAngle and commands the hardware but never updates the subsystem setpoint, so this.targetAngle remains at Degrees.zero(); update this.targetAngle to the clamped value (assign this.targetAngle = Radians.of(adjustedAngle) or the appropriate Angle type conversion) after computing adjustedAngle, then change the second log to Logger.recordOutput("Turret/AdjustedTargetAngle", Radians.of(adjustedAngle)) so both isAtTargetAngle() and logging compare/report the actual commanded setpoint; ensure you use the same Angle unit/type as targetAngle to avoid conversion bugs (methods: runToTargetAngle, targetAngle, adjustedAngle, isAtTargetAngle, Logger.recordOutput).src/main/java/frc/alotobots/rebuilt/subsystems/belt/BeltSubsystem.java (1)
42-45:⚠️ Potential issue | 🟡 MinorStale Javadoc — describes "kicker" and references
MAX_OPERATOR_VELOCITYinstead ofMAX_SPEED.This was clearly copied from
KickerSubsystem. Update to reference the belt subsystem and theMAX_SPEEDconstant that the method actually uses.📝 Suggested fix
- * Controls the kicker to move to a specified velocity using closed-loop velocity control. + * Controls the belt to move at a specified velocity using closed-loop velocity control. * * `@param` velocity Target velocity in radians per second, automatically constrained between - * -MAX_OPERATOR_VELOCITY and MAX_OPERATOR_VELOCITY + * -MAX_SPEED and MAX_SPEED🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/frc/alotobots/rebuilt/subsystems/belt/BeltSubsystem.java` around lines 42 - 45, Update the stale Javadoc in BeltSubsystem for the method that takes the parameter velocity (the doc block currently saying "Controls the kicker...") to describe the belt subsystem instead of the kicker and change the referenced constant from MAX_OPERATOR_VELOCITY to MAX_SPEED; also update the `@param` line to state the target velocity is constrained between -MAX_SPEED and MAX_SPEED so it matches the actual implementation in BeltSubsystem and its MAX_SPEED constant.src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/io/DeflectorIOTalonFXS.java (3)
106-121:⚠️ Potential issue | 🔴 Critical
currentPidSlotis never refreshed before it is read.
currentPidSlotis registered for frequency updates (line 98) but is absent from theBaseStatusSignal.refreshAll()call. Every timeupdateInputsruns,currentPidSlot.getValue()returns its last latched value rather than the freshly-sampled one, soinputs.deflectorMotorPidSlotmay silently lag behind the motor's actual active slot.🐛 Proposed fix — add the signal to
refreshAllvar motorSignals = BaseStatusSignal.refreshAll( deflectorMotorPosition, deflectorMotorVelocity, deflectorMotorAcceleration, deflectorMotorVoltage, deflectorMotorCurrent, + currentPidSlot, deflectorMotorControlMode);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/io/DeflectorIOTalonFXS.java` around lines 106 - 121, The code reads currentPidSlot.getValue() without refreshing it in updateInputs; add currentPidSlot to the BaseStatusSignal.refreshAll(...) call so it is sampled each update, then use the refreshed value to set inputs.deflectorMotorPidSlot (referencing BaseStatusSignal.refreshAll, currentPidSlot, inputs.deflectorMotorPidSlot, and updateInputs).
76-78:⚠️ Potential issue | 🔴 CriticalVelocity PID gains (Slot 1) are never configured — velocity control will be inert.
Only
Slot0(position) gains are written to theTalonFXSConfiguration.setDeflectorVelocitycommands the motor with.withSlot(PIDSlots.VELOCITY.ordinal())= slot 1, which has all gains at their default of0. The motor will receive a velocity setpoint but apply zero output.Slot 1 must be populated with at least a
kVfeedforward and tunedkPbefore velocity closed-loop can function:🐛 Sketch of the required addition (constants should come from
DeflectorTalonFXSConstants)deflectorMotorConfig.Slot0.kP = TurretTalonFXSConstants.POSITION_P_GAIN; deflectorMotorConfig.Slot0.kI = TurretTalonFXSConstants.POSITION_I_GAIN; deflectorMotorConfig.Slot0.kD = TurretTalonFXSConstants.POSITION_D_GAIN; + + deflectorMotorConfig.Slot1.kV = DeflectorTalonFXSConstants.VELOCITY_V_GAIN; + deflectorMotorConfig.Slot1.kP = DeflectorTalonFXSConstants.VELOCITY_P_GAIN; + deflectorMotorConfig.Slot1.kI = DeflectorTalonFXSConstants.VELOCITY_I_GAIN; + deflectorMotorConfig.Slot1.kD = DeflectorTalonFXSConstants.VELOCITY_D_GAIN;Also applies to: 152-164
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/io/DeflectorIOTalonFXS.java` around lines 76 - 78, The velocity PID (Slot1) is never configured in DeflectorIOTalonFXS so setDeflectorVelocity (which uses PIDSlots.VELOCITY.ordinal()) has zero gains; update the TalonFXSConfiguration instance deflectorMotorConfig to populate Slot1 (e.g., deflectorMotorConfig.Slot1.kF, kP, kI, kD and any kV/kS feedforward constants) using values from DeflectorTalonFXSConstants, mirroring how Slot0 is set for position, and ensure the same fix is applied to the other block referenced around lines 152-164 so velocity closed-loop control uses the correct tuned gains.
138-150:⚠️ Potential issue | 🟡 MinorStale
@throwsJavadoc after removing the validation.The
@throws IllegalArgumentExceptionclause (line 143) still claims null/invalid inputs are rejected, but the validation code was removed — the method now silently proceeds with any input.📝 Proposed fix
/** * * Commands the deflector to move to a specified position using closed-loop control. * * `@param` position The target position for the deflector * `@param` pidSlot The PID slot to use for the control - * `@throws` IllegalArgumentException if position or pidSlot is null, or if pidSlot is invalid */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/io/DeflectorIOTalonFXS.java` around lines 138 - 150, The Javadoc for setDeflectorPosition incorrectly documents an `@throws` IllegalArgumentException that's no longer applicable; remove the `@throws` line from the method comment and instead either (A) keep the current implementation and add a brief behavior note that null inputs are not explicitly validated, or (B) if you prefer to enforce validation, reintroduce null/invalid checks for Angle position and PIDSlots pidSlot at the start of setDeflectorPosition (method name) and throw IllegalArgumentException with a clear message before calling deflectorMotor.setControl(positionControl.withPosition(...).withSlot(...)).
🧹 Nitpick comments (2)
src/main/java/frc/alotobots/rebuilt/subsystems/launcher/turret/TurretSubsystem.java (1)
82-82: NormalizeRadian/Radiansusage in this clamp callThe min-bound uses
Radianwhile the max-bound usesRadians. Both are aliases for the sameAngleUnitin WPILib, so this is functionally harmless but inconsistent. Use one form consistently throughout the call (recommendRadiansto match line 85).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/turret/TurretSubsystem.java` at line 82, The MathUtil.clamp call uses inconsistent AngleUnit aliases: angle.in(Radian) versus Degrees.of(...).in(Radians); update the clamp invocation (MathUtil.clamp) so both bounds and the angle use the same alias (use Radians as recommended) by changing angle.in(Radian) to angle.in(Radians) (and confirm Degrees.of(...).in(Radians) remains), ensuring consistent use of the Radians symbol across the clamp call in TurretSubsystem.src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/constants/DeflectorConstants.java (1)
46-49:MAX_OPERATOR_VELOCITYandMAX_SPEEDare identical — consider having one reference the other.Both are
DegreesPerSecond.of(90), which creates a maintenance hazard: changing one won't change the other.♻️ Suggested refactor
- /** Max speed (magnitude) */ - public static final AngularVelocity MAX_OPERATOR_VELOCITY = DegreesPerSecond.of(90); - /** Max speed (magnitude) */ public static final AngularVelocity MAX_SPEED = DegreesPerSecond.of(90); + + /** Max operator-commanded speed (magnitude) */ + public static final AngularVelocity MAX_OPERATOR_VELOCITY = MAX_SPEED;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/constants/DeflectorConstants.java` around lines 46 - 49, MAX_OPERATOR_VELOCITY and MAX_SPEED both call DegreesPerSecond.of(90) separately; change one to reference the other to avoid duplication. For example, keep MAX_OPERATOR_VELOCITY as the canonical value and set MAX_SPEED = MAX_OPERATOR_VELOCITY (or vice versa), update the Javadoc comments for MAX_SPEED/ MAX_OPERATOR_VELOCITY to reflect the aliasing, and ensure any consumers expecting distinct semantics are still correct after the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/frc/alotobots/rebuilt/commands/groups/IndexIntoShooterAndShoot.java`:
- Line 19: IndexIntoShooterAndShoot is currently calling new
ShooterShootAtVelocity(..., () ->
ShooterConstants.Setpoints.SHOOTER_TEST_VELOCITY) which exposes a tuning
constant to drivers; replace or rename that constant to a production-safe name
(e.g., SHOOT_VELOCITY) in ShooterConstants.Setpoints and update all call sites
(including IndexIntoShooterAndShoot and any bindings in RobotContainer) to use
the new SHOOT_VELOCITY; keep SHOOTER_TEST_VELOCITY only for characterization
code and ensure its usage is removed from runtime command bindings.
- Around line 1-22: This file is missing the ALOTOBOTS license header and has
Spotless formatting violations; add the same license header block present in
other files to the top of this file and then run ./gradlew spotlessApply to fix
formatting (ensure imports and indentation around the IndexIntoShooterAndShoot
class, its constructor, and addCommands call remain correctly formatted); verify
the class name IndexIntoShooterAndShoot and referenced symbols
(DefaultBeltRunAtVelocity, DefaultKickerRunAtVelocity, ShooterShootAtVelocity,
BeltConstants, KickerConstants, ShooterConstants) are unchanged after
formatting.
In `@src/main/java/frc/alotobots/rebuilt/subsystems/belt/BeltSubsystem.java`:
- Around line 47-61: The current clamping always applies even when
BeltConstants.Limits.LIMITS_ENABLED is false; update runBeltToTargetVelocity and
runBeltPercentOutput to consult that flag: if LIMITS_ENABLED is true, clamp
using MAX_SPEED (for runBeltToTargetVelocity) and MAX_OPEN_LOOP_PERCENTAGE (for
runBeltPercentOutput), otherwise pass the raw velocity/percent through to
io.setBeltVelocity and io.setBeltOpenLoop; reference the symbols
BeltConstants.Limits.LIMITS_ENABLED, MAX_SPEED, MAX_OPEN_LOOP_PERCENTAGE,
runBeltToTargetVelocity, runBeltPercentOutput, io.setBeltVelocity, and
io.setBeltOpenLoop when making the change.
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/belt/constants/BeltConstants.java`:
- Around line 13-25: The Javadoc in the nested Thresholds class and nearby
comments was copied from Shooter/Wrist and still mentions "wrist operations" and
"flywheel"; update all comments in BeltConstants, especially the Thresholds
class and the docs for AT_TARGET_VELOCITY_SPEED_THRESHOLD and
AT_TARGET_VELOCITY_TIME_THRESHOLD (and any other nearby nested-class or field
comments around the same area), to reference "belt" operations and belt-specific
terminology (e.g., "belt speed", "belt at target velocity") so the comments
accurately describe BeltConstants' purpose and fields.
- Line 40: BeltConstants currently defines LOAD_INTO_SHOOTER_VELOCITY equal to
MAX_SPEED, so the clamp in BeltSubsystem.runBeltToTargetVelocity never reduces
the requested speed; change LOAD_INTO_SHOOTER_VELOCITY (and the similar value at
line ~53) to a value below MAX_SPEED (e.g. a fixed lower DegreesPerSecond or
compute as MAX_SPEED * safetyFactor) or alter DefaultBeltRunAtVelocity to
request a lower target so the clamp can take effect; update the constant names
referenced (LOAD_INTO_SHOOTER_VELOCITY, MAX_SPEED) and ensure
DefaultBeltRunAtVelocity still uses the adjusted constant.
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/kicker/constants/KickerConstants.java`:
- Around line 13-51: The Javadoc comments in KickerConstants reference "wrist"
instead of the kicker; update the section comments for the nested classes
Thresholds, Limits, and Setpoints to refer to "kicker" (e.g., change "wrist
operations" to "kicker operations", "wrist limits" to "kicker limits", and
"wrist states" to "kicker states") so the docs match the KickerConstants class
and its nested types Thresholds, Limits, and Setpoints.
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/commands/DefaultDeflectorRunAtVelocity.java`:
- Around line 37-45: In DefaultDeflectorRunAtVelocity, replace the end(...)
override to call deflectorSubsystem.stop() (and still call
super.end(interrupted) if desired) so the deflector motor is stopped when the
command is interrupted or ends; also remove the redundant isFinished() override
(it currently just delegates to super.isFinished() which returns false) to avoid
needless code. Ensure you reference the class DefaultDeflectorRunAtVelocity and
the deflectorSubsystem.stop() method when making the change.
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/constants/DeflectorConstants.java`:
- Line 25: Update the Javadoc comment on the DeflectorConstants class to
reference the deflector (and its threshold values) instead of "wrist
operations"; locate the comment above the DeflectorConstants declaration and
change the phrase "wrist operations" to something like "deflector operations" or
"deflector thresholds" so the documentation matches the class purpose.
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/DeflectorSubsystem.java`:
- Around line 26-29: Remove the incorrect import of MAX_OPEN_LOOP_PERCENTAGE
from
frc.alotobots.rebuilt.subsystems.intake.extendo.constants.IntakeExtendoConstants
in DeflectorSubsystem.java and rely on the DeflectorConstants definition instead
(either keep the existing wildcard import from
frc.alotobots.rebuilt.subsystems.launcher.deflector.constants.DeflectorConstants.Limits.*
or reference DeflectorConstants.Limits.MAX_OPEN_LOOP_PERCENTAGE where used);
ensure no other code in this file still imports the intake constant so the
unqualified MAX_OPEN_LOOP_PERCENTAGE resolves to the deflector constant.
- Around line 88-98: The method runToTargetVelocity currently accepts
LinearVelocity and uses MetersPerSecond (unit mismatch) — change its parameter
to AngularVelocity and use RadiansPerSecond when reading/clamping; clamp the
incoming angular value against the same deflector limit constant used by the
caller (either rename or switch to MAX_SPEED consistently) and pass that
AngularVelocity into io.setDeflectorVelocity(…) (update
DefaultDeflectorRunAtVelocity.execute() call site if needed); also fix the
Javadoc text to refer to the deflector (not "intake extendo") so comments match
behavior.
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/io/DeflectorIO.java`:
- Around line 63-67: The Javadoc for the method in DeflectorIO incorrectly
refers to "wrist" instead of the deflector; update the Javadoc comment for the
method in class DeflectorIO (the comment starting "Sets the wrist to run at a
target velocity using closed-loop control.") to reference the deflector (e.g.,
"Sets the deflector to run at a target velocity using closed-loop control.") and
ensure the `@param` descriptions (velocity, pidSlot) remain accurate and
unchanged.
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/io/DeflectorIOTalonFXS.java`:
- Line 40: Remove the dyn4j import org.dyn4j.exception.ArgumentNullException and
replace any throws of ArgumentNullException in DeflectorIOTalonFXS (the switch
default branches around the first occurrence and the second occurrence near
lines 119-121) with a standard Java exception such as IllegalArgumentException
(including a clear message with the unexpected integer value), ensuring the
class no longer depends on the dyn4j library.
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/shooter/constants/ShooterConstants.java`:
- Around line 12-25: The Javadoc for nested classes in ShooterConstants
(Thresholds, Limits, Setpoints) was copied from a wrist/turret file and
incorrectly references "wrist operations"; update each doc block in
ShooterConstants.java to accurately describe shooter/launcher concepts (e.g.,
Thresholds: thresholds for flywheel velocity and timing; Limits: physical limits
and safety thresholds for the shooter/flywheel; Setpoints: shooter/flywheel
target speeds for different states) so the comments match the symbols
Thresholds, Limits, and Setpoints in this class.
- Line 39: Replace the mistaken angular unit in ShooterConstants so the shooter
setpoints are realistic: change the MAX_SPEED and SHOOTER_TEST_VELOCITY
declarations in class ShooterConstants from DegreesPerSecond.of(90) to
RotationsPerSecond.of(90) (or another appropriate RotationsPerSecond value), and
then verify the clamping logic in ShooterSubsystem now has effect; if the
current 90 RPS is only a placeholder, document/tune the final RPM before
competition instead of leaving DegreesPerSecond.
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/shooter/ShooterSubsystem.java`:
- Around line 41-45: The Javadoc for the ShooterSubsystem method that takes an
AngularVelocity parameter incorrectly says "meters per second"; update the
`@param` for the velocity parameter in the ShooterSubsystem method (the one taking
AngularVelocity) to state that the target is an angular velocity in radians per
second (or simply "angular velocity") and mention it is constrained between
-MAX_SPEED and MAX_SPEED to match the AngularVelocity type and existing
constants.
- Around line 57-62: In runShooterPercentOutput the clamped variable
adjustedOutput is computed but the method calls
io.setShooterOpenLoop(percentOutput) with the raw value; change the call to use
adjustedOutput so the MAX_OPEN_LOOP_PERCENTAGE clamp is enforced (i.e., call
io.setShooterOpenLoop(adjustedOutput)), and optionally update
Logger.recordOutput to reflect the actual value being applied if you want the
logged value to match the hardware input.
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/turret/TurretSubsystem.java`:
- Line 71: The log path is inconsistent: Logger.processInputs("Launcher/Turret",
inputs) moves inputs under "Launcher/Turret" but recordOutput calls still write
keys like "Turret/..." causing split logs; update every recordOutput invocation
(e.g., calls that write "Turret/..." at the locations around the recordOutput
calls referenced) to use the matching prefix "Launcher/Turret/..." (for example
change "Turret/angle" → "Launcher/Turret/angle", "Turret/velocity" →
"Launcher/Turret/velocity", etc.) so all telemetry uses the same
"Launcher/Turret" subtree.
In `@src/main/java/frc/alotobots/RobotContainer.java`:
- Around line 214-217: There is a Spotless formatting violation in
RobotContainer.java around the shoot.whileTrue(...) block (the call that
constructs IndexIntoShooterAndShoot with beltSubsystem, kickerSubsystem,
shooterSubsystem); run the code formatter to remove trailing whitespace and
apply project style rules by running ./gradlew spotlessApply (or manually remove
trailing spaces/newlines in the shoot.whileTrue(...) region) and reformat the
file so Spotless CI passes.
---
Outside diff comments:
In `@src/main/java/frc/alotobots/rebuilt/subsystems/belt/BeltSubsystem.java`:
- Around line 42-45: Update the stale Javadoc in BeltSubsystem for the method
that takes the parameter velocity (the doc block currently saying "Controls the
kicker...") to describe the belt subsystem instead of the kicker and change the
referenced constant from MAX_OPERATOR_VELOCITY to MAX_SPEED; also update the
`@param` line to state the target velocity is constrained between -MAX_SPEED and
MAX_SPEED so it matches the actual implementation in BeltSubsystem and its
MAX_SPEED constant.
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/constants/DeflectorTalonFXSConstants.java`:
- Around line 25-26: The comment above SENSOR_TO_MECHANISM_RATIO in
DeflectorTalonFXSConstants is a stale copy-paste referring to "turret mechanism"
— update the comment to reference the deflector mechanism (e.g., "Gear ratio
between the deflector mechanism and the motor sensor") so it matches the class
and constant meaning; locate SENSOR_TO_MECHANISM_RATIO in class
DeflectorTalonFXSConstants and replace "turret" with "deflector" in the comment.
- Around line 21-23: Add missing velocity PID constants to
DeflectorTalonFXSConstants (VELOCITY_P_GAIN=2.5, VELOCITY_I_GAIN=0.0,
VELOCITY_D_GAIN=0.0, VELOCITY_V_GAIN=0.125, VELOCITY_S_GAIN=0.0090433) and then
configure Slot1 in the DeflectorIOTalonFXS constructor (immediately after the
existing Slot0 configuration) by assigning Slot1.kP, kI, kD, kV and kS from
those new constants so PIDSlots.VELOCITY (used by setDeflectorVelocity) uses the
tuned gains instead of firmware defaults.
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/io/DeflectorIOTalonFXS.java`:
- Around line 106-121: The code reads currentPidSlot.getValue() without
refreshing it in updateInputs; add currentPidSlot to the
BaseStatusSignal.refreshAll(...) call so it is sampled each update, then use the
refreshed value to set inputs.deflectorMotorPidSlot (referencing
BaseStatusSignal.refreshAll, currentPidSlot, inputs.deflectorMotorPidSlot, and
updateInputs).
- Around line 76-78: The velocity PID (Slot1) is never configured in
DeflectorIOTalonFXS so setDeflectorVelocity (which uses
PIDSlots.VELOCITY.ordinal()) has zero gains; update the TalonFXSConfiguration
instance deflectorMotorConfig to populate Slot1 (e.g.,
deflectorMotorConfig.Slot1.kF, kP, kI, kD and any kV/kS feedforward constants)
using values from DeflectorTalonFXSConstants, mirroring how Slot0 is set for
position, and ensure the same fix is applied to the other block referenced
around lines 152-164 so velocity closed-loop control uses the correct tuned
gains.
- Around line 138-150: The Javadoc for setDeflectorPosition incorrectly
documents an `@throws` IllegalArgumentException that's no longer applicable;
remove the `@throws` line from the method comment and instead either (A) keep the
current implementation and add a brief behavior note that null inputs are not
explicitly validated, or (B) if you prefer to enforce validation, reintroduce
null/invalid checks for Angle position and PIDSlots pidSlot at the start of
setDeflectorPosition (method name) and throw IllegalArgumentException with a
clear message before calling
deflectorMotor.setControl(positionControl.withPosition(...).withSlot(...)).
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/turret/TurretSubsystem.java`:
- Around line 79-92: runToTargetAngle currently computes adjustedAngle and
commands the hardware but never updates the subsystem setpoint, so
this.targetAngle remains at Degrees.zero(); update this.targetAngle to the
clamped value (assign this.targetAngle = Radians.of(adjustedAngle) or the
appropriate Angle type conversion) after computing adjustedAngle, then change
the second log to Logger.recordOutput("Turret/AdjustedTargetAngle",
Radians.of(adjustedAngle)) so both isAtTargetAngle() and logging compare/report
the actual commanded setpoint; ensure you use the same Angle unit/type as
targetAngle to avoid conversion bugs (methods: runToTargetAngle, targetAngle,
adjustedAngle, isAtTargetAngle, Logger.recordOutput).
---
Nitpick comments:
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/constants/DeflectorConstants.java`:
- Around line 46-49: MAX_OPERATOR_VELOCITY and MAX_SPEED both call
DegreesPerSecond.of(90) separately; change one to reference the other to avoid
duplication. For example, keep MAX_OPERATOR_VELOCITY as the canonical value and
set MAX_SPEED = MAX_OPERATOR_VELOCITY (or vice versa), update the Javadoc
comments for MAX_SPEED/ MAX_OPERATOR_VELOCITY to reflect the aliasing, and
ensure any consumers expecting distinct semantics are still correct after the
change.
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/turret/TurretSubsystem.java`:
- Line 82: The MathUtil.clamp call uses inconsistent AngleUnit aliases:
angle.in(Radian) versus Degrees.of(...).in(Radians); update the clamp invocation
(MathUtil.clamp) so both bounds and the angle use the same alias (use Radians as
recommended) by changing angle.in(Radian) to angle.in(Radians) (and confirm
Degrees.of(...).in(Radians) remains), ensuring consistent use of the Radians
symbol across the clamp call in TurretSubsystem.
Comment on lines
+13
to
+25
| /** | ||
| * Contains threshold values for various wrist operations. | ||
| */ | ||
| public static final class Thresholds { | ||
| /** | ||
| * Acceptable PID error that will classify as "at velocity" | ||
| */ | ||
| public static final AngularVelocity AT_TARGET_VELOCITY_SPEED_THRESHOLD = DegreesPerSecond.of(3); | ||
|
|
||
| /** | ||
| * How long the flywheel must be "at velocity" to classify as "at velocity" | ||
| */ | ||
| public static final Time AT_TARGET_VELOCITY_TIME_THRESHOLD = Seconds.of(.2); |
There was a problem hiding this comment.
Same stale Javadoc as ShooterConstants — all nested-class descriptions reference "wrist operations" and "flywheel."
The comments were copied verbatim from a wrist/turret constants file. Update to reference "belt" operations throughout.
Also applies to: 28-29, 48-50
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/belt/constants/BeltConstants.java`
around lines 13 - 25, The Javadoc in the nested Thresholds class and nearby
comments was copied from Shooter/Wrist and still mentions "wrist operations" and
"flywheel"; update all comments in BeltConstants, especially the Thresholds
class and the docs for AT_TARGET_VELOCITY_SPEED_THRESHOLD and
AT_TARGET_VELOCITY_TIME_THRESHOLD (and any other nearby nested-class or field
comments around the same area), to reference "belt" operations and belt-specific
terminology (e.g., "belt speed", "belt at target velocity") so the comments
accurately describe BeltConstants' purpose and fields.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/io/DeflectorIOTalonFXS.java (2)
76-81:⚠️ Potential issue | 🟠 MajorVelocity Slot 1 (PIDSlots.VELOCITY) has no gains configured — velocity closed-loop will produce zero output.
The constructor configures Slot0 only (
kP,kI,kD).PIDSlots.VELOCITY.ordinal()= 1, so allsetDeflectorVelocitycalls target Slot 1. Phoenix 6 requires gains to be explicitly set per slot — gain values default to 0 unless configured (kP=0, kI=0, kD=0, kV=0, kS=0). With all-zero gains, the velocity closed-loop will command zero voltage regardless of velocity error, making the velocity control mode completely non-functional.Add a
Slot1Configsblock for velocity gains (at minimumkVfor feedforward andkPfor feedback):🐛 Proposed fix
deflectorMotorConfig.Slot0.kP = TurretTalonFXSConstants.POSITION_P_GAIN; deflectorMotorConfig.Slot0.kI = TurretTalonFXSConstants.POSITION_I_GAIN; deflectorMotorConfig.Slot0.kD = TurretTalonFXSConstants.POSITION_D_GAIN; + + // Slot 1 — velocity closed-loop gains (tune these values) + deflectorMotorConfig.Slot1.kV = TurretTalonFXSConstants.VELOCITY_V_GAIN; + deflectorMotorConfig.Slot1.kP = TurretTalonFXSConstants.VELOCITY_P_GAIN; + deflectorMotorConfig.Slot1.kD = TurretTalonFXSConstants.VELOCITY_D_GAIN;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/io/DeflectorIOTalonFXS.java` around lines 76 - 81, The velocity PID slot (PIDSlots.VELOCITY / Slot1) is never configured so velocity closed-loop outputs zero; update deflectorMotorConfig to configure Slot1 (e.g., deflectorMotorConfig.Slot1.kP, kV and other needed gains) with appropriate constants and then apply the updated config via the existing PhoenixUtil.tryUntilOk call that uses deflectorMotor.getConfigurator().apply(deflectorMotorConfig, 0.25); ensure the gains used match the velocity tuning (reference setDeflectorVelocity uses Slot1) so velocity control actually produces output.
104-121:⚠️ Potential issue | 🟠 Major
currentPidSlotis not included inBaseStatusSignal.refreshAll()— its value may be stale when read.
refreshAllatomically refreshes the listed signals from the CAN bus, butcurrentPidSlotis omitted from that list (lines 107–113). The switch on line 116 then readscurrentPidSlot.getValue()against a potentially stale cached frame rather than the just-refreshed snapshot, breaking the synchronization guarantee for AdvantageKit logging.🐛 Proposed fix — add currentPidSlot to refreshAll
var motorSignals = BaseStatusSignal.refreshAll( deflectorMotorPosition, deflectorMotorVelocity, deflectorMotorAcceleration, deflectorMotorVoltage, deflectorMotorCurrent, + currentPidSlot, deflectorMotorControlMode);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/io/DeflectorIOTalonFXS.java` around lines 104 - 121, The switch in updateInputs reads currentPidSlot.getValue() but currentPidSlot is not included in the atomic refresh, so its value may be stale; update the BaseStatusSignal.refreshAll(...) call in updateInputs to include currentPidSlot alongside deflectorMotorPosition/Velocity/Acceleration/Voltage/Current/ControlMode so the PID slot is refreshed atomically, then use the refreshed currentPidSlot.getValue() when assigning inputs.deflectorMotorPidSlot to ensure synchronization for AdvantageKit logging.
🧹 Nitpick comments (3)
src/main/java/frc/alotobots/rebuilt/subsystems/kicker/KickerSubsystem.java (1)
44-44: Truncated@paramJavadoc — missing the clamping bounds."automatically constrained" doesn't specify the range. Compare to
ShooterSubsystemwhich documents "between -MAX_SPEED and MAX_SPEED".📝 Proposed fix
- * `@param` velocity Target velocity in radians per second, automatically constrained + * `@param` velocity Target velocity, automatically constrained between -MAX_SPEED and MAX_SPEED🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/frc/alotobots/rebuilt/subsystems/kicker/KickerSubsystem.java` at line 44, Update the Javadoc for the KickerSubsystem method parameter "velocity" to document the clamping bounds (like ShooterSubsystem does) — state that velocity is constrained between -MAX_SPEED and MAX_SPEED (or the actual Kicker-specific constant if named differently) so callers know the valid range; modify the `@param` velocity line in KickerSubsystem's Javadoc to mirror the wording used in ShooterSubsystem (e.g., "Target velocity in radians per second, constrained between -MAX_SPEED and MAX_SPEED") and ensure you reference the correct constant name used by the kicker implementation.src/main/java/frc/alotobots/rebuilt/subsystems/launcher/shooter/commands/ShooterShootAtVelocity.java (1)
22-23: Naming inconsistency:targetVelocityshould betargetVelocitySupplier.The field type is
Supplier<AngularVelocity>, not anAngularVelocity. Peer commandsDefaultBeltRunAtVelocityandDefaultKickerRunAtVelocityboth name their equivalent fieldvelocitySupplier, which more accurately reflects the type.♻️ Proposed rename
- private final Supplier<AngularVelocity> targetVelocity; + private final Supplier<AngularVelocity> targetVelocitySupplier; public ShooterShootAtVelocity( ShooterSubsystem shooterSubsystem, Supplier<AngularVelocity> targetVelocity) { this.shooterSubsystem = shooterSubsystem; - this.targetVelocity = targetVelocity; + this.targetVelocitySupplier = targetVelocity; addRequirements(shooterSubsystem); } ... public void execute() { - shooterSubsystem.runToTargetVelocity(targetVelocity.get()); + shooterSubsystem.runToTargetVelocity(targetVelocitySupplier.get()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/shooter/commands/ShooterShootAtVelocity.java` around lines 22 - 23, The field name targetVelocity in class ShooterShootAtVelocity is misleading because its type is Supplier<AngularVelocity>; rename it to targetVelocitySupplier (or follow peer naming like velocitySupplier) and update all references (constructor parameters, the field declaration, any usages inside methods such as execute/initialize) to use the new name; ensure the constructor assignment and any imports/annotations remain intact and run a build to catch any remaining references.src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/constants/DeflectorConstants.java (1)
45-49:MAX_OPERATOR_VELOCITYandMAX_SPEEDare identical — remove one.Both constants are
DegreesPerSecond.of(90).DefaultDeflectorRunAtVelocityusesMAX_SPEEDto scale input, whileDeflectorSubsystem.runToTargetVelocityclamps againstMAX_OPERATOR_VELOCITY. Having two constants with the same value but different names creates a maintenance hazard — they'll silently diverge if someone updates only one.♻️ Proposed fix — remove the duplicate
- /** Max speed (magnitude) */ - public static final AngularVelocity MAX_OPERATOR_VELOCITY = DegreesPerSecond.of(90); - /** Max speed (magnitude) */ public static final AngularVelocity MAX_SPEED = DegreesPerSecond.of(90);Then update
DeflectorSubsystem.runToTargetVelocityto clamp againstMAX_SPEEDinstead ofMAX_OPERATOR_VELOCITY.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/constants/DeflectorConstants.java` around lines 45 - 49, Remove the duplicated constant by deleting MAX_OPERATOR_VELOCITY (DegreesPerSecond.of(90)) from DeflectorConstants and update any callers to use MAX_SPEED instead; specifically, change DeflectorSubsystem.runToTargetVelocity to clamp against DeflectorConstants.MAX_SPEED and ensure DefaultDeflectorRunAtVelocity continues to use MAX_SPEED for scaling so a single source of truth remains.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/frc/alotobots/rebuilt/commands/groups/IndexIntoShooterAndShoot.java`:
- Around line 31-37: The current IndexIntoShooterAndShoot launches
DefaultBeltRunAtVelocity, DefaultKickerRunAtVelocity, and ShooterShootAtVelocity
simultaneously, causing feeding before the shooter is up to speed; change
addCommands to first spin the shooter until it reaches target by running
ShooterShootAtVelocity and using shooterSubsystem::isAtTargetVelocity (or an
equivalent until() condition), then start a ParallelCommandGroup containing
DefaultBeltRunAtVelocity, DefaultKickerRunAtVelocity, and ShooterShootAtVelocity
so the belt/kicker only feed once isAtTargetVelocity is true; alternatively,
remove shooter from this command and rely on ShooterSubsystem default command to
pre-warm.
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/kicker/constants/KickerConstants.java`:
- Around line 40-50: MAX_SPEED and LOAD_INTO_SHOOTER_VELOCITY are incorrectly
using DegreesPerSecond.of(90) (≈15 RPM); replace with the intended rotational
unit (e.g., RotationsPerSecond.of(...) or RotationsPerMinute.of(...)) and pick
the correct numeric value (for example RotationsPerSecond.of(90) if 90 RPS was
intended) in KickerConstants (symbols: MAX_SPEED, LOAD_INTO_SHOOTER_VELOCITY)
and apply the same correction in BeltConstants to avoid the copy-paste unit
error.
---
Outside diff comments:
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/io/DeflectorIOTalonFXS.java`:
- Around line 76-81: The velocity PID slot (PIDSlots.VELOCITY / Slot1) is never
configured so velocity closed-loop outputs zero; update deflectorMotorConfig to
configure Slot1 (e.g., deflectorMotorConfig.Slot1.kP, kV and other needed gains)
with appropriate constants and then apply the updated config via the existing
PhoenixUtil.tryUntilOk call that uses
deflectorMotor.getConfigurator().apply(deflectorMotorConfig, 0.25); ensure the
gains used match the velocity tuning (reference setDeflectorVelocity uses Slot1)
so velocity control actually produces output.
- Around line 104-121: The switch in updateInputs reads
currentPidSlot.getValue() but currentPidSlot is not included in the atomic
refresh, so its value may be stale; update the BaseStatusSignal.refreshAll(...)
call in updateInputs to include currentPidSlot alongside
deflectorMotorPosition/Velocity/Acceleration/Voltage/Current/ControlMode so the
PID slot is refreshed atomically, then use the refreshed
currentPidSlot.getValue() when assigning inputs.deflectorMotorPidSlot to ensure
synchronization for AdvantageKit logging.
---
Duplicate comments:
In
`@src/main/java/frc/alotobots/rebuilt/commands/groups/IndexIntoShooterAndShoot.java`:
- Around line 36-37: IndexIntoShooterAndShoot is wiring the shooter command to
the test constant ShooterConstants.Setpoints.SHOOTER_TEST_VELOCITY; replace that
with the correct competition shot setpoint constant (or add one if missing) so
the live robot uses the intended velocity. Locate the new
ShooterShootAtVelocity(...) call in IndexIntoShooterAndShoot and swap
ShooterConstants.Setpoints.SHOOTER_TEST_VELOCITY for the appropriate constant
(e.g., ShooterConstants.Setpoints.SHOOTER_COMPETITION_VELOCITY or
SHOOTER_FAR_SHOT) or create and document a clearly named constant in
ShooterConstants.Setpoints if none exists. Ensure the chosen constant reflects
the live match setpoint and update any comments to avoid future test/competition
mixups.
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/kicker/constants/KickerConstants.java`:
- Around line 25-46: The Javadoc comments mistakenly refer to "wrist" instead of
the kicker subsystem; update the docstrings for the nested classes Thresholds,
Limits, and Setpoints to reference the kicker (or kicker/flywheel) instead of
"wrist" — e.g., change "threshold values for various wrist operations" to
"threshold values for kicker operations", "physical limits and safety thresholds
for the wrist" to "physical limits and safety thresholds for the kicker", and
"position setpoints for different wrist states" to "position setpoints for
different kicker states" so the comments match the KickerConstants class and its
fields.
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/commands/DefaultDeflectorRunAtVelocity.java`:
- Around line 47-55: DefaultDeflectorRunAtVelocity currently leaves the motor
running because end(boolean interrupted) only calls the no-op super and
isFinished() just delegates to super; update end(boolean interrupted) to
explicitly stop the deflector motor (call the subsystem's stop() or
setVelocity(0) method on your deflector instance) so the motor ceases when the
command ends/interrupted, and remove or simplify the isFinished() override
(either delete it or return false explicitly) since delegating to
super.isFinished() is a no-op.
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/constants/DeflectorConstants.java`:
- Around line 25-35: Update the Javadocs in DeflectorConstants to refer to
"deflector" instead of the leftover "wrist" copy-paste text: change the comment
for the inner class Thresholds (currently "threshold values for various wrist
operations") to mention deflector thresholds, and change the comment for Limits
(currently "physical limits and safety thresholds for the wrist") to mention
deflector physical limits/safety thresholds; also scan the DeflectorConstants
class for any other stray "wrist" references and correct them to "deflector" to
keep documentation accurate.
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/DeflectorSubsystem.java`:
- Around line 77-82: Update the Javadoc for runToTargetVelocity in
DeflectorSubsystem to replace the copy-paste phrase "intake extendo" with
"deflector" (or more specific deflector component wording), ensuring the
description now reads that it controls the deflector to move to a specified
velocity and retains the existing param description about automatic clamping
between -MAX_OPERATOR_VELOCITY and MAX_OPERATOR_VELOCITY.
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/io/DeflectorIO.java`:
- Around line 63-67: Update the Javadoc in the DeflectorIO class for the method
that sets a target velocity (the docblock currently referencing "wrist") to say
"deflector" instead of "wrist", and verify the param tags remain correct for
velocity and pidSlot; locate the comment above the setter method in class
DeflectorIO (the docblock describing running at a target velocity with params
velocity and pidSlot) and change the description text to "Sets the deflector to
run at a target velocity using closed-loop control."
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/io/DeflectorIOTalonFXS.java`:
- Line 40: The code currently imports and throws
org.dyn4j.exception.ArgumentNullException in DeflectorIOTalonFXS, which is
incorrect; remove the import for ArgumentNullException and replace any
throws/new ArgumentNullException(...) with throws/new IllegalStateException(...)
(or new IllegalStateException(...) where the code constructs the exception) in
the DeflectorIOTalonFXS class (check the constructor/initializer and the branch
around the integer-value check and the block at the 119-121 region) so the
exception semantics match an unexpected state rather than a physics-engine
null-argument exception.
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/shooter/constants/ShooterConstants.java`:
- Around line 24-49: Update the stale Javadoc and unrealistic velocity values:
change the comments in the nested classes Thresholds, Limits, and Setpoints to
refer to the shooter/flywheel (not "wrist"), and adjust the velocity constants
AT_TARGET_VELOCITY_SPEED_THRESHOLD, MAX_SPEED, and SHOOTER_TEST_VELOCITY to
realistic shooter units (use an appropriate DegreesPerSecond or RPM-equivalent
value instead of DegreesPerSecond.of(90)), ensuring SHOOTER_TEST_VELOCITY is
strictly less than MAX_SPEED so the clamp in
ShooterSubsystem.runToTargetVelocity is effective; leave other limits
(MAX_OPEN_LOOP_PERCENTAGE, LIMITS_ENABLED) unchanged unless you intentionally
want to modify safety behavior.
In `@src/main/java/frc/alotobots/RobotContainer.java`:
- Line 214: The file RobotContainer.java has a Spotless formatting violation
around the statement using shoot.whileTrue(new IndexIntoShooterAndShoot(...)); —
run the formatter and commit the changes: execute ./gradlew spotlessApply
locally (or apply your IDE's Spotless reformat) to auto-fix formatting across
the file, verify RobotContainer.java (and the line creating the
IndexIntoShooterAndShoot command) is updated, then stage and push the
reformatted file so CI passes.
---
Nitpick comments:
In `@src/main/java/frc/alotobots/rebuilt/subsystems/kicker/KickerSubsystem.java`:
- Line 44: Update the Javadoc for the KickerSubsystem method parameter
"velocity" to document the clamping bounds (like ShooterSubsystem does) — state
that velocity is constrained between -MAX_SPEED and MAX_SPEED (or the actual
Kicker-specific constant if named differently) so callers know the valid range;
modify the `@param` velocity line in KickerSubsystem's Javadoc to mirror the
wording used in ShooterSubsystem (e.g., "Target velocity in radians per second,
constrained between -MAX_SPEED and MAX_SPEED") and ensure you reference the
correct constant name used by the kicker implementation.
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/constants/DeflectorConstants.java`:
- Around line 45-49: Remove the duplicated constant by deleting
MAX_OPERATOR_VELOCITY (DegreesPerSecond.of(90)) from DeflectorConstants and
update any callers to use MAX_SPEED instead; specifically, change
DeflectorSubsystem.runToTargetVelocity to clamp against
DeflectorConstants.MAX_SPEED and ensure DefaultDeflectorRunAtVelocity continues
to use MAX_SPEED for scaling so a single source of truth remains.
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/shooter/commands/ShooterShootAtVelocity.java`:
- Around line 22-23: The field name targetVelocity in class
ShooterShootAtVelocity is misleading because its type is
Supplier<AngularVelocity>; rename it to targetVelocitySupplier (or follow peer
naming like velocitySupplier) and update all references (constructor parameters,
the field declaration, any usages inside methods such as execute/initialize) to
use the new name; ensure the constructor assignment and any imports/annotations
remain intact and run a build to catch any remaining references.
Rkortlandt
reviewed
Feb 21, 2026
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
src/main/java/frc/alotobots/rebuilt/subsystems/kicker/KickerSubsystem.java (2)
43-43: Javadoc is slightly misleading: method accepts anyAngularVelocityunit, not just radians per second.The internal conversion handles arbitrary units correctly, so the
@paramshould just say "Target velocity" without specifying "in radians per second."📝 Suggested Javadoc fix
- * `@param` velocity Target velocity in radians per second, automatically constrained + * `@param` velocity Target velocity, automatically constrained to MAX_SPEED limits🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/frc/alotobots/rebuilt/subsystems/kicker/KickerSubsystem.java` at line 43, The Javadoc for the method in KickerSubsystem incorrectly states the parameter is "in radians per second"; update the doc for the parameter named velocity (type AngularVelocity) to remove the unit claim and state something like "Target velocity; any AngularVelocity unit is accepted and converted internally," referencing the KickerSubsystem method that takes velocity so reviewers can locate and edit the Javadoc.
53-53: Inconsistent control-type logging: enum constant vs. string literal.Line 53 logs
KickerIO.PIDSlots.DEFAULT_VELOCITY(a constant/enum), while Line 60 logs the raw string"PERCENT_OUTPUT". Consider using a consistent approach for both — either define a constant forPERCENT_OUTPUTalongside the PID slots, or use string literals for both.Also applies to: 59-60
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/frc/alotobots/rebuilt/subsystems/kicker/KickerSubsystem.java` at line 53, Replace the string literal "PERCENT_OUTPUT" with a shared constant so control-type logs are consistent: add a PERCENT_OUTPUT constant (or enum value) alongside KickerIO.PIDSlots.DEFAULT_VELOCITY in KickerIO (e.g., KickerIO.PIDSlots.PERCENT_OUTPUT) and change the Logger.recordOutput call that currently passes "PERCENT_OUTPUT" to use KickerIO.PIDSlots.PERCENT_OUTPUT; update both occurrences (lines logging control type) to reference that symbol.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/intake/extendo/IntakeExtendoSubsystem.java`:
- Around line 69-70: targetExtension is being set to adjustedExtension even when
LIMITS_ENABLED is false, so isAtTargetExtension() compares against the clamped
value while io.setIntakeExtendoPosition(...) is commanded with the raw
extension; change the logic so targetExtension is assigned the actual commanded
value (set targetExtension = adjustedExtension when LIMITS_ENABLED is true,
otherwise targetExtension = extension), and then call
io.setIntakeExtendoPosition(...) with the same value; also split the long
ternary/statement into two clearer statements to satisfy spotless formatting
(refer to targetExtension, adjustedExtension, extension, LIMITS_ENABLED,
io.setIntakeExtendoPosition, and isAtTargetExtension()).
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/intake/roller/constants/IntakeRollerConstants.java`:
- Around line 27-28: Remove the trailing whitespace after the LIMITS_ENABLED
constant in the IntakeRollerConstants class so the file passes Spotless
formatting; open
src/main/java/frc/alotobots/rebuilt/subsystems/intake/roller/constants/IntakeRollerConstants.java,
trim any extra spaces/newline after the line defining public static final
boolean LIMITS_ENABLED = true;, and run ./gradlew spotlessApply to auto-format
and verify the change.
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/intake/roller/IntakeRollerSubsystem.java`:
- Around line 22-23: The static import of LIMITS_ENABLED in
IntakeRollerSubsystem.java is in the wrong import group and breaks spotless;
move the line "import static
frc.alotobots.rebuilt.subsystems.intake.roller.constants.IntakeRollerConstants.Limits.LIMITS_ENABLED"
into the project's static-import group (after normal imports) so imports follow
the project's ordering rules, then run ./gradlew spotlessApply to reformat and
verify; check the IntakeRollerSubsystem class imports to ensure no other static
imports are mis-grouped.
- Line 45: The call to io.setIntakeRollerOpenLoop currently sends percentOutput
un-clamped when LIMITS_ENABLED is false; modify IntakeRollerSubsystem so you
always clamp the raw input to the safe motor range (e.g.
MathUtil.clamp(percentOutput, -1.0, 1.0)) before calling
io.setIntakeRollerOpenLoop, and then, if LIMITS_ENABLED is true, additionally
apply the tighter subsystem cap (use adjustedOutput = MathUtil.clamp(clampedRaw,
-MAX_OPEN_LOOP_PERCENTAGE, MAX_OPEN_LOOP_PERCENTAGE)) so the gate only controls
the subsystem-specific limit while the base [-1,1] clamp is always enforced;
update usages of adjustedOutput/percentOutput and the io.setIntakeRollerOpenLoop
call accordingly.
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/DeflectorSubsystem.java`:
- Line 59: The log path used in Logger.recordOutput is inconsistent: change the
call Logger.recordOutput("Deflector/TargetAngle", targetAngle.in(Degree)) to use
the same prefix as the rest of the subsystem (i.e.,
"Launcher/Deflector/TargetAngle"); locate the call to Logger.recordOutput that
references targetAngle.in(Degree) in DeflectorSubsystem and update the string
key to "Launcher/Deflector/TargetAngle" so it matches the other
recordOutput/processInputs entries.
- Around line 68-75: In runToTargetAngle, the ternary passed to
io.setDeflectorPosition uses `adjustedAngle: angle` without the required space
before the colon; update the expression to include a space before `:` (i.e.,
`LIMITS_ENABLED ? adjustedAngle : angle`) in the call to io.setDeflectorPosition
(still using DeflectorIO.PIDSlots.DEFAULT_POSITION), then run ./gradlew
spotlessApply (or reformat) to satisfy spotlessJavaCheck.
- Line 108: Logger.recordOutput is writing a raw String "PERCENT_OUTPUT" to the
same key ("Launcher/Deflector/ControlType") that elsewhere records a
DeflectorIO.PIDSlots enum, causing a type mismatch; fix by adding a
PERCENT_OUTPUT constant to the DeflectorIO.PIDSlots enum and change the call at
DeflectorSubsystem (the Logger.recordOutput call that currently passes
"PERCENT_OUTPUT") to pass the DeflectorIO.PIDSlots.PERCENT_OUTPUT enum value so
all writes use the same type, or alternatively replace the other enum writes
with a consistent String—prefer the enum extension (add PERCENT_OUTPUT to
DeflectorIO.PIDSlots and use that enum in Logger.recordOutput).
---
Duplicate comments:
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/DeflectorSubsystem.java`:
- Around line 77-78: The Javadoc above the method in DeflectorSubsystem still
incorrectly says "intake extendo" — update that comment to reference "deflector"
and accurately describe the method's behavior (e.g., that it controls the
deflector to move to a specified velocity using closed-loop velocity control);
locate the Javadoc attached to the DeflectorSubsystem method responsible for
velocity control and replace the stale copy-paste text accordingly.
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/shooter/ShooterSubsystem.java`:
- Around line 40-44: The Javadoc for the shooter velocity method incorrectly
says "meters per second" while the parameter is an AngularVelocity; update the
Javadoc in ShooterSubsystem to describe the parameter as an angular velocity
(e.g., "Target angular velocity in radians per second") and retain the note that
it is constrained between -MAX_SPEED and MAX_SPEED; ensure the `@param` tag names
the AngularVelocity parameter exactly and uses correct units/terminology so the
documentation matches the method signature.
---
Nitpick comments:
In `@src/main/java/frc/alotobots/rebuilt/subsystems/kicker/KickerSubsystem.java`:
- Line 43: The Javadoc for the method in KickerSubsystem incorrectly states the
parameter is "in radians per second"; update the doc for the parameter named
velocity (type AngularVelocity) to remove the unit claim and state something
like "Target velocity; any AngularVelocity unit is accepted and converted
internally," referencing the KickerSubsystem method that takes velocity so
reviewers can locate and edit the Javadoc.
- Line 53: Replace the string literal "PERCENT_OUTPUT" with a shared constant so
control-type logs are consistent: add a PERCENT_OUTPUT constant (or enum value)
alongside KickerIO.PIDSlots.DEFAULT_VELOCITY in KickerIO (e.g.,
KickerIO.PIDSlots.PERCENT_OUTPUT) and change the Logger.recordOutput call that
currently passes "PERCENT_OUTPUT" to use KickerIO.PIDSlots.PERCENT_OUTPUT;
update both occurrences (lines logging control type) to reference that symbol.
Member
Author
|
@coderabbitai please make a list of all of the minor changes like docs that need to be fixed so I can add them to a github issue |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 26903
Documentation Issues to Fix1. Missing Class-Level Javadoc (New Files in PR)The following new files need class-level Javadoc documentation: Command Classes:
Constants Classes:
2. Missing Class-Level Javadoc (Modified Subsystems)These subsystem files that were modified in the PR are also missing class Javadoc:
3. Missing Method DocumentationThe new public methods added should have Javadoc:
4. Constants DocumentationWhile most constants have inline comments, consider adding more descriptive comments for:
RecommendationI suggest creating a GitHub issue titled "Add documentation to shooter subsystem and commands" with the items listed above. This will help ensure the code is maintainable and understandable for future contributors. Would you like me to create this issue for you? |
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/main/java/frc/alotobots/rebuilt/subsystems/launcher/turret/TurretSubsystem.java (1)
79-92:⚠️ Potential issue | 🔴 Critical
targetAnglefield is never updated —isAtTargetAngle()always computes error againstDegrees.zero(), and line 91 logs the wrong variable
runToTargetAnglecomputesadjustedAngle, commands the motor with it, but never stores it back intothis.targetAngle. Consequently:
isAtTargetAngle()(line 154) evaluatestargetAngle.minus(inputs.turretMotorPosition)against the initialDegrees.zero()— the turret can never correctly report it has reached its setpoint.- Line 91's
Logger.recordOutput("Launcher/Turret/AdjustedTargetAngle", targetAngle)logs the stale field (Degrees.zero()) rather than the actual clamped value.🐛 Proposed fix
public void runToTargetAngle(Angle angle) { var adjustedAngle = MathUtil.clamp(angle.in(Radian), Degrees.of(-90).in(Radian), Degrees.of(90).in(Radians)); + targetAngle = Radians.of(adjustedAngle); try { io.setTurretPosition(Radians.of(adjustedAngle), TurretIO.PIDSlots.DEFAULT_POSITION); } catch (Exception e) { System.out.println(e.getMessage()); } Logger.recordOutput("Launcher/Turret/TargetAngle", angle); - Logger.recordOutput("Launcher/Turret/AdjustedTargetAngle", targetAngle); + Logger.recordOutput("Launcher/Turret/AdjustedTargetAngle", Radians.of(adjustedAngle)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/turret/TurretSubsystem.java` around lines 79 - 92, runToTargetAngle computes an adjustedAngle and commands the motor but never updates the subsystem state, so isAtTargetAngle() and the telemetry log use the stale this.targetAngle; assign the clamped value to the field (e.g., set this.targetAngle = Angle.of(adjustedAngle) or convert adjustedAngle back to the Angle/Degrees type used by targetAngle) immediately after computing adjustedAngle, and change the Logger.recordOutput call to log this.targetAngle (or adjustedAngle) so the reported and checked setpoint match; ensure the unit conversion is correct when assigning.src/main/java/frc/alotobots/rebuilt/subsystems/launcher/shooter/ShooterSubsystem.java (1)
1-90:⚠️ Potential issue | 🟡 MinorCI failure:
spotlessJavaCheckformatting violations — run./gradlew :spotlessApply.The pipeline reports Spotless formatting violations in this file. No manual fix needed; just run the formatter before pushing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/shooter/ShooterSubsystem.java` around lines 1 - 90, The CI failed due to Spotless formatting violations in the ShooterSubsystem class; run the formatter and commit the changes by executing ./gradlew :spotlessApply (or your repo's equivalent) to auto-fix formatting for this file (class ShooterSubsystem, methods runToTargetVelocity, runShooterPercentOutput, isAtTargetVelocity, periodic), then re-run CI and push the formatted commit.src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/io/DeflectorIOTalonFXS.java (3)
165-165:⚠️ Potential issue | 🟡 MinorLog key
"deflector/openLoopPercentOut"uses lowercase prefix — inconsistent with the rest of the IO layer.All other log keys written by the deflector subsystem and IO use
"Launcher/Deflector/..."(capitalD). This entry will appear at a different tree node in AdvantageScope/logs.🐛 Proposed fix
- Logger.recordOutput("deflector/openLoopPercentOut", percentOutput); + Logger.recordOutput("Launcher/Deflector/openLoopPercentOut", percentOutput);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/io/DeflectorIOTalonFXS.java` at line 165, The log key uses a lowercase prefix and must match the IO layer naming convention; update the Logger.recordOutput call in DeflectorIOTalonFXS (the line invoking Logger.recordOutput("deflector/openLoopPercentOut", percentOutput)) to use the same prefix as other deflector entries, e.g. Logger.recordOutput("Launcher/Deflector/openLoopPercentOut", percentOutput), so it appears under the same AdvantageScope/logs tree.
143-144:⚠️ Potential issue | 🟡 MinorStale
@throwsJavadoc: null checks were removed but the doc still claimsIllegalArgumentExceptionfor null inputs.The current implementation does not check for null
positionorpidSlot; a null would instead throw an NPE from the motor controller call. Remove the false null-check clauses.🐛 Proposed fix
- * `@throws` IllegalArgumentException if position or pidSlot is null, or if pidSlot is invalid + * `@throws` IllegalArgumentException if pidSlot is an unrecognized slot value🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/io/DeflectorIOTalonFXS.java` around lines 143 - 144, The Javadoc in DeflectorIOTalonFXS mentions throwing IllegalArgumentException for null position/pidSlot but the method no longer performs null checks; update the Javadoc above the affected method in class DeflectorIOTalonFXS to remove the stale "if position or pidSlot is null" clause (or replace it with the actual behavior, e.g., note that a NullPointerException may occur or document motor-controller-specific exceptions) and ensure the `@throws` tag reflects real exceptions thrown by the implementation rather than removed null checks.
75-80:⚠️ Potential issue | 🟠 MajorSlot 1 (velocity PID) is never configured — velocity control will produce zero output.
VelocityVoltageusesPIDSlots.VELOCITY.ordinal() = 1, but onlySlot0is configured in the constructor.Slot1is left with all-zero gains (kP,kI,kD,kS,kV,kA), so the motor will not respond to velocity commands.Configure
Slot1with velocity gains before applying the configuration. You'll also need to addVELOCITY_*_GAINconstants toDeflectorTalonFXSConstants:🐛 Proposed fix
deflectorMotorConfig.Slot0.kP = TurretTalonFXSConstants.POSITION_P_GAIN; deflectorMotorConfig.Slot0.kI = TurretTalonFXSConstants.POSITION_I_GAIN; deflectorMotorConfig.Slot0.kD = TurretTalonFXSConstants.POSITION_D_GAIN; + + deflectorMotorConfig.Slot1.kP = DeflectorTalonFXSConstants.VELOCITY_P_GAIN; + deflectorMotorConfig.Slot1.kD = DeflectorTalonFXSConstants.VELOCITY_D_GAIN; + deflectorMotorConfig.Slot1.kS = DeflectorTalonFXSConstants.VELOCITY_S_GAIN; + deflectorMotorConfig.Slot1.kV = DeflectorTalonFXSConstants.VELOCITY_V_GAIN;Then define the missing velocity constants in
DeflectorTalonFXSConstants(e.g.,VELOCITY_P_GAIN,VELOCITY_D_GAIN,VELOCITY_S_GAIN,VELOCITY_V_GAIN).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/io/DeflectorIOTalonFXS.java` around lines 75 - 80, The velocity PID (Slot1) is never set in DeflectorIOTalonFXS so velocity control produces zero output; update the constructor to populate deflectorMotorConfig.Slot1.kP/kI/kD and the feedforward terms kS/kV/kA using new DeflectorTalonFXSConstants (e.g., VELOCITY_P_GAIN, VELOCITY_I_GAIN, VELOCITY_D_GAIN, VELOCITY_S_GAIN, VELOCITY_V_GAIN, VELOCITY_A_GAIN) before calling PhoenixUtil.tryUntilOk(...).Add those missing VELOCITY_*_GAIN constants to DeflectorTalonFXSConstants and then apply the full config with deflectorMotor.getConfigurator().apply(deflectorMotorConfig, 0.25).
🧹 Nitpick comments (1)
src/main/java/frc/alotobots/rebuilt/subsystems/intake/extendo/io/IntakeExtendoIOTalonFX.java (1)
135-142:currentPidSlotis not included inrefreshAll, so its value is not frame-synchronized.
currentPidSlotis registered at 50 Hz (line 128) but is absent fromBaseStatusSignal.refreshAll(...)inupdateInputs. Phoenix 6 will cache its latest background-received value, but it won't be synchronized to the same CAN frame as the other signals. More importantly, if an unexpected slot value arrives and hits thedefaultbranch,updateInputsthrows an uncheckedIllegalArgumentExceptionduring the periodic loop — which would crash robot code in competition.Two improvements:
- Add
currentPidSlottorefreshAllfor frame-consistency.- Consider a safe fallback (e.g., log a warning + keep the previous slot value) instead of throwing inside a periodic callback.
♻️ Proposed fix
var intakeExtendoSignals = BaseStatusSignal.refreshAll( intakeExtendoPosition, intakeExtendoVelocity, intakeExtendoAcceleration, intakeExtendoAppliedVoltage, - intakeExtendoAppliedCurrent); + intakeExtendoAppliedCurrent, + currentPidSlot);inputs.intakeExtendoMotorPIDSlot = switch (currentPidSlot.getValue()) { case 0 -> PIDSlots.VELOCITY; case 1 -> PIDSlots.MOTION_MAGIC_POSITION; - default -> throw new IllegalArgumentException( - "No defined PID slot for value: " + currentPidSlot.getValue()); + default -> { + DriverStation.reportWarning( + "IntakeExtendo: unexpected PID slot " + currentPidSlot.getValue(), false); + yield inputs.intakeExtendoMotorPIDSlot; // retain last known-good value + } };Also applies to: 156-162
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/frc/alotobots/rebuilt/subsystems/intake/extendo/io/IntakeExtendoIOTalonFX.java` around lines 135 - 142, The currentPidSlot signal is not being frame-synchronized because it's missing from BaseStatusSignal.refreshAll in updateInputs (class IntakeExtendoIOTalonFX), so add currentPidSlot to the argument list for the refreshAll call(s) that build intakeExtendoSignals (also update the second refreshAll usage around the 156-162 area) to ensure it is refreshed with the same CAN frame as the other signals; additionally, replace the unchecked throw in the switch/default that interprets the pid slot with a safe fallback: log a warning (including the unexpected value), retain the previous pid slot value, and avoid throwing inside the periodic update to prevent crashing robot code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/frc/alotobots/rebuilt/commands/groups/IndexIntoShooterAndShoot.java`:
- Around line 1-12: Spotless formatting violations were reintroduced in
IndexIntoShooterAndShoot.java—run the formatter and fix the header/comment
formatting: execute ./gradlew spotlessApply, then ensure the top-of-file comment
block (the file header in IndexIntoShooterAndShoot.java) matches the project's
Spotless/Google Java formatting rules (consistent indentation, line breaks,
spacing, and license header text) and re-commit the changes so spotlessJavaCheck
passes in CI.
- Around line 26-37: The class IndexIntoShooterAndShoot currently extends
ParallelCommandGroup causing duplicate requirements for shooterSubsystem; change
the group to a SequentialCommandGroup so the shooter spins up first then feeding
runs. Specifically, make IndexIntoShooterAndShoot extend SequentialCommandGroup
and call addCommands with first ShooterShootAtVelocity(shooterSubsystem, () ->
ShooterConstants.Setpoints.SHOOTER_TEST_VELOCITY).until(shooterSubsystem::isAtTargetVelocity)
followed by a ParallelCommandGroup containing
DefaultBeltRunAtVelocity(beltSubsystem, () ->
BeltConstants.Setpoints.LOAD_INTO_SHOOTER_VELOCITY) and
DefaultKickerRunAtVelocity(kickerSubsystem, () ->
KickerConstants.Setpoints.LOAD_INTO_SHOOTER_VELOCITY) (omit the second
ShooterShootAtVelocity instance), ensuring only the feeding commands run in
parallel after the shooter is at speed.
- Around line 32-33: The gate fires immediately because
ShooterSubsystem.targetVelocity is final and left at zero, so isAtTargetVelocity
compares actual speed to 0; make targetVelocity non-final and assign the
intended velocity inside ShooterSubsystem.runToTargetVelocity (store the clamped
or raw target you use to command the motor) so that isAtTargetVelocity and the
debouncer compare against the correct SHOOTER_TEST_VELOCITY; update references
to targetVelocity accordingly so
ShooterShootAtVelocity.until(shooterSubsystem::isAtTargetVelocity) waits for the
real spin-up.
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/shooter/ShooterSubsystem.java`:
- Line 62: The log key "Launcher/Shooter/ControlType" is written first with an
enum (ShooterIO.PIDSlots.DEFAULT_VELOCITY) and later with a String
("PERCENT_OUTPUT"), causing AdvantageKit to drop one path; fix by making both
writes the same type—either call .name() on ShooterIO.PIDSlots.DEFAULT_VELOCITY
when calling Logger.recordOutput("Launcher/Shooter/ControlType", ...) or change
the later "PERCENT_OUTPUT" to use the corresponding ShooterIO.PIDSlots enum
constant (or add one) so Logger.recordOutput always receives the same Java type.
- Line 36: targetVelocity is declared final and left at RadiansPerSecond.zero(),
so isAtTargetVelocity() always compares to zero; make targetVelocity mutable
(remove final) and update it inside runToTargetVelocity(...) by assigning the
computed adjustedVelocity (the value currently computed but not stored) so
future isAtTargetVelocity() checks use the commanded setpoint, and also reset
targetVelocity back to RadiansPerSecond.zero() inside stop() so the subsystem
reports not-at-target after halting; update references in isAtTargetVelocity(),
runToTargetVelocity(), and stop() accordingly.
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/turret/TurretSubsystem.java`:
- Line 70: The log key "Launcher/Turret/TargetAngle" is written as a double in
periodic() using targetAngle.in(Degree) and as an Angle object in
runToTargetAngle(), causing the second write to be silently dropped; fix this by
making both writes the same type—either remove the Logger.recordOutput call in
runToTargetAngle() or change it to record the same double value (e.g.,
targetAngle.in(Degree)) so the key consistently logs a double; look for
Logger.recordOutput("Launcher/Turret/TargetAngle", ...) in periodic() and
runToTargetAngle() and align/remove the latter accordingly.
---
Outside diff comments:
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/io/DeflectorIOTalonFXS.java`:
- Line 165: The log key uses a lowercase prefix and must match the IO layer
naming convention; update the Logger.recordOutput call in DeflectorIOTalonFXS
(the line invoking Logger.recordOutput("deflector/openLoopPercentOut",
percentOutput)) to use the same prefix as other deflector entries, e.g.
Logger.recordOutput("Launcher/Deflector/openLoopPercentOut", percentOutput), so
it appears under the same AdvantageScope/logs tree.
- Around line 143-144: The Javadoc in DeflectorIOTalonFXS mentions throwing
IllegalArgumentException for null position/pidSlot but the method no longer
performs null checks; update the Javadoc above the affected method in class
DeflectorIOTalonFXS to remove the stale "if position or pidSlot is null" clause
(or replace it with the actual behavior, e.g., note that a NullPointerException
may occur or document motor-controller-specific exceptions) and ensure the
`@throws` tag reflects real exceptions thrown by the implementation rather than
removed null checks.
- Around line 75-80: The velocity PID (Slot1) is never set in
DeflectorIOTalonFXS so velocity control produces zero output; update the
constructor to populate deflectorMotorConfig.Slot1.kP/kI/kD and the feedforward
terms kS/kV/kA using new DeflectorTalonFXSConstants (e.g., VELOCITY_P_GAIN,
VELOCITY_I_GAIN, VELOCITY_D_GAIN, VELOCITY_S_GAIN, VELOCITY_V_GAIN,
VELOCITY_A_GAIN) before calling PhoenixUtil.tryUntilOk(...).Add those missing
VELOCITY_*_GAIN constants to DeflectorTalonFXSConstants and then apply the full
config with deflectorMotor.getConfigurator().apply(deflectorMotorConfig, 0.25).
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/shooter/ShooterSubsystem.java`:
- Around line 1-90: The CI failed due to Spotless formatting violations in the
ShooterSubsystem class; run the formatter and commit the changes by executing
./gradlew :spotlessApply (or your repo's equivalent) to auto-fix formatting for
this file (class ShooterSubsystem, methods runToTargetVelocity,
runShooterPercentOutput, isAtTargetVelocity, periodic), then re-run CI and push
the formatted commit.
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/turret/TurretSubsystem.java`:
- Around line 79-92: runToTargetAngle computes an adjustedAngle and commands the
motor but never updates the subsystem state, so isAtTargetAngle() and the
telemetry log use the stale this.targetAngle; assign the clamped value to the
field (e.g., set this.targetAngle = Angle.of(adjustedAngle) or convert
adjustedAngle back to the Angle/Degrees type used by targetAngle) immediately
after computing adjustedAngle, and change the Logger.recordOutput call to log
this.targetAngle (or adjustedAngle) so the reported and checked setpoint match;
ensure the unit conversion is correct when assigning.
---
Duplicate comments:
In
`@src/main/java/frc/alotobots/rebuilt/commands/groups/IndexIntoShooterAndShoot.java`:
- Line 32: IndexIntoShooterAndShoot currently constructs ShooterShootAtVelocity
with the low test constant ShooterConstants.Setpoints.SHOOTER_TEST_VELOCITY;
replace that reference with the tuned competition setpoint constant from
ShooterConstants.Setpoints (for example a dedicated SHOOTER_COMPETITION_VELOCITY
or the existing match setpoint like SHOOTER_FAR_*), and if no suitable constant
exists add a properly named competition setpoint in ShooterConstants and use it
in the ShooterShootAtVelocity constructor so the live shoot command does not use
the test velocity.
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/DeflectorSubsystem.java`:
- Around line 78-82: The Javadoc for the velocity-control method in
DeflectorSubsystem still says "intake extendo" — update the comment to refer to
the deflector (e.g., "Controls the deflector to move to a specified velocity
using closed-loop velocity control") and keep the rest of the description/param
details (including the constraint between -MAX_OPERATOR_VELOCITY and
MAX_OPERATOR_VELOCITY) intact; locate the docblock above the velocity-control
method in class DeflectorSubsystem and replace "intake extendo" with
"deflector".
- Line 59: The log key used in Logger.recordOutput currently uses the wrong
prefix "Deflector/TargetAngle"; update the string to
"Launcher/Deflector/TargetAngle" to match the other entries in this
subsystem—locate the call to Logger.recordOutput("Deflector/TargetAngle",
targetAngle.in(Degree)) and change only the first argument to the correct prefix
so it aligns with the other "Launcher/Deflector/..." logs.
- Line 108: Your AdvantageKit key "Launcher/Deflector/ControlType" is being
written with mixed types (a raw String at the Logger.recordOutput call and
DeflectorIO.PIDSlots enum elsewhere), causing writes to be dropped; add a
PERCENT_OUTPUT entry to the DeflectorIO.PIDSlots enum and change the
Logger.recordOutput call in DeflectorSubsystem to pass
DeflectorIO.PIDSlots.PERCENT_OUTPUT instead of the String so all writes use the
same enum type; update any other recordOutput usages for that key to use the
PIDSlots enum to ensure type consistency.
- Line 71: In DeflectorSubsystem fix the Spotless formatting error in the
ternary assignment to targetAngle (symbols: targetAngle, LIMITS_ENABLED,
adjustedAngle, angle) by inserting the missing space before the colon so it
reads "adjustedAngle : angle"; after updating the expression run ./gradlew
spotlessApply to reformat and unblock the spotlessJavaCheck CI.
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/shooter/ShooterSubsystem.java`:
- Around line 48-52: The Javadoc for the shooter control method in
ShooterSubsystem incorrectly states the `@param` units as "meters per second" for
the parameter named velocity which is of type AngularVelocity; update the `@param`
description to specify angular units (e.g., "radians per second" or the
project's chosen angular unit) and mention that the value will be constrained
between -MAX_SPEED and MAX_SPEED, ensuring the description matches the
AngularVelocity type and parameter name.
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/turret/TurretSubsystem.java`:
- Line 59: Update the legacy logging key used in the SysId state lambda inside
TurretSubsystem: replace the Logger.recordOutput call that currently logs to
"Turret/SysIdState" with the standardized key "Launcher/Turret/SysIdState" so
SysId telemetry follows the new namespace; locate the lambda that passes (state)
-> Logger.recordOutput("Turret/SysIdState", state.toString()) and change the
string literal only.
---
Nitpick comments:
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/intake/extendo/io/IntakeExtendoIOTalonFX.java`:
- Around line 135-142: The currentPidSlot signal is not being frame-synchronized
because it's missing from BaseStatusSignal.refreshAll in updateInputs (class
IntakeExtendoIOTalonFX), so add currentPidSlot to the argument list for the
refreshAll call(s) that build intakeExtendoSignals (also update the second
refreshAll usage around the 156-162 area) to ensure it is refreshed with the
same CAN frame as the other signals; additionally, replace the unchecked throw
in the switch/default that interprets the pid slot with a safe fallback: log a
warning (including the unexpected value), retain the previous pid slot value,
and avoid throwing inside the periodic update to prevent crashing robot code.
Comment on lines
+26
to
+37
| public class IndexIntoShooterAndShoot extends ParallelCommandGroup { | ||
| public IndexIntoShooterAndShoot( | ||
| BeltSubsystem beltSubsystem, | ||
| KickerSubsystem kickerSubsystem, | ||
| ShooterSubsystem shooterSubsystem) { | ||
| addCommands( | ||
| new ShooterShootAtVelocity(shooterSubsystem, () -> ShooterConstants.Setpoints.SHOOTER_TEST_VELOCITY) | ||
| .until(shooterSubsystem::isAtTargetVelocity), | ||
| new ParallelCommandGroup( | ||
| new DefaultBeltRunAtVelocity(beltSubsystem, () -> BeltConstants.Setpoints.LOAD_INTO_SHOOTER_VELOCITY), | ||
| new DefaultKickerRunAtVelocity(kickerSubsystem, () -> KickerConstants.Setpoints.LOAD_INTO_SHOOTER_VELOCITY), | ||
| new ShooterShootAtVelocity(shooterSubsystem, () -> ShooterConstants.Setpoints.SHOOTER_TEST_VELOCITY))); |
There was a problem hiding this comment.
extends ParallelCommandGroup causes a duplicate-subsystem IllegalArgumentException at construction — will crash on robot startup.
WPILib enforces that a parallel group may not contain multiple commands that require the same subsystem. Both commands passed to the outer addCommands require shooterSubsystem:
new ShooterShootAtVelocity(...).until(...)— the.until()decorator creates aParallelRaceGroup, which still requires all subsystems its components require, i.e.,shooterSubsystem.- The nested
ParallelCommandGroupalso containsnew ShooterShootAtVelocity(shooterSubsystem, ...).
The outer ParallelCommandGroup.addCommands() will throw IllegalArgumentException when the command group is constructed (in RobotContainer), crashing the robot program before a match even starts.
The intended semantics — spin up the shooter, then feed — require a SequentialCommandGroup. Command compositions inherit the union of their components' requirements. A SequentialCommandGroup that runs a shooter, then an indexer, would reserve all required subsystems across its stages, which is perfectly legal.
🐛 Proposed fix
import edu.wpi.first.wpilibj2.command.ParallelCommandGroup;
+import edu.wpi.first.wpilibj2.command.SequentialCommandGroup;
...
-public class IndexIntoShooterAndShoot extends ParallelCommandGroup {
+public class IndexIntoShooterAndShoot extends SequentialCommandGroup {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public class IndexIntoShooterAndShoot extends ParallelCommandGroup { | |
| public IndexIntoShooterAndShoot( | |
| BeltSubsystem beltSubsystem, | |
| KickerSubsystem kickerSubsystem, | |
| ShooterSubsystem shooterSubsystem) { | |
| addCommands( | |
| new ShooterShootAtVelocity(shooterSubsystem, () -> ShooterConstants.Setpoints.SHOOTER_TEST_VELOCITY) | |
| .until(shooterSubsystem::isAtTargetVelocity), | |
| new ParallelCommandGroup( | |
| new DefaultBeltRunAtVelocity(beltSubsystem, () -> BeltConstants.Setpoints.LOAD_INTO_SHOOTER_VELOCITY), | |
| new DefaultKickerRunAtVelocity(kickerSubsystem, () -> KickerConstants.Setpoints.LOAD_INTO_SHOOTER_VELOCITY), | |
| new ShooterShootAtVelocity(shooterSubsystem, () -> ShooterConstants.Setpoints.SHOOTER_TEST_VELOCITY))); | |
| public class IndexIntoShooterAndShoot extends SequentialCommandGroup { | |
| public IndexIntoShooterAndShoot( | |
| BeltSubsystem beltSubsystem, | |
| KickerSubsystem kickerSubsystem, | |
| ShooterSubsystem shooterSubsystem) { | |
| addCommands( | |
| new ShooterShootAtVelocity(shooterSubsystem, () -> ShooterConstants.Setpoints.SHOOTER_TEST_VELOCITY) | |
| .until(shooterSubsystem::isAtTargetVelocity), | |
| new ParallelCommandGroup( | |
| new DefaultBeltRunAtVelocity(beltSubsystem, () -> BeltConstants.Setpoints.LOAD_INTO_SHOOTER_VELOCITY), | |
| new DefaultKickerRunAtVelocity(kickerSubsystem, () -> KickerConstants.Setpoints.LOAD_INTO_SHOOTER_VELOCITY), | |
| new ShooterShootAtVelocity(shooterSubsystem, () -> ShooterConstants.Setpoints.SHOOTER_TEST_VELOCITY))); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/frc/alotobots/rebuilt/commands/groups/IndexIntoShooterAndShoot.java`
around lines 26 - 37, The class IndexIntoShooterAndShoot currently extends
ParallelCommandGroup causing duplicate requirements for shooterSubsystem; change
the group to a SequentialCommandGroup so the shooter spins up first then feeding
runs. Specifically, make IndexIntoShooterAndShoot extend SequentialCommandGroup
and call addCommands with first ShooterShootAtVelocity(shooterSubsystem, () ->
ShooterConstants.Setpoints.SHOOTER_TEST_VELOCITY).until(shooterSubsystem::isAtTargetVelocity)
followed by a ParallelCommandGroup containing
DefaultBeltRunAtVelocity(beltSubsystem, () ->
BeltConstants.Setpoints.LOAD_INTO_SHOOTER_VELOCITY) and
DefaultKickerRunAtVelocity(kickerSubsystem, () ->
KickerConstants.Setpoints.LOAD_INTO_SHOOTER_VELOCITY) (omit the second
ShooterShootAtVelocity instance), ensuring only the feeding commands run in
parallel after the shooter is at speed.
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/frc/alotobots/Constants.java (1)
82-89:⚠️ Potential issue | 🔴 Critical
DEFLECTOR_MOTOR_CAN_IDandDEFLECTOR_ENCODER_CAN_IDconflict at CAN ID 0, and the oldDEFLECTOR_CAN_ID = 35is orphaned.Three verified issues:
DEFLECTOR_CAN_ID = 35is dead code: The subsystem (DeflectorIOVortex.javalines 52–53) has migrated toDEFLECTOR_MOTOR_CAN_IDandDEFLECTOR_ENCODER_CAN_ID. The old constant at line 82 is no longer referenced and should be removed to avoid confusion.CAN ID
0conflict: BothDEFLECTOR_ENCODER_CAN_ID(line 87) andDEFLECTOR_MOTOR_CAN_ID(line 89) are hardcoded to0. When the deflector subsystem initializes, two distinct CAN devices will compete at the same bus address, producing undefined behavior and preventing hardware operation.Incomplete TODO: The
// TODO get actual CAN IDcomment (line 86) precedes onlyDEFLECTOR_ENCODER_CAN_ID, butDEFLECTOR_MOTOR_CAN_IDalso requires a real value.Assign distinct real CAN IDs to both new constants before deploying to hardware, and remove the orphaned
DEFLECTOR_CAN_ID = 35constant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/frc/alotobots/Constants.java` around lines 82 - 89, Remove the orphaned DEFLECTOR_CAN_ID constant, and replace the two zero values so DEFLECTOR_ENCODER_CAN_ID and DEFLECTOR_MOTOR_CAN_ID are set to distinct, non-zero CAN IDs (do not leave them both as 0); update the existing TODO to mention both encoder and motor IDs need real values, and verify the chosen IDs do not collide with other constants in the file or the rest of the codebase (also confirm DeflectorIOVortex.java references still use DEFLECTOR_ENCODER_CAN_ID and DEFLECTOR_MOTOR_CAN_ID).
🧹 Nitpick comments (2)
src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/constants/DeflectorVortexConstants.java (1)
17-17: Add a private constructor to prevent instantiationConstants holder classes should not be instantiable.
♻️ Proposed refactor
public class DeflectorVortexConstants { + private DeflectorVortexConstants() {} + public static final NeutralModeValue NEUTRAL_MODE = NeutralModeValue.Brake;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/constants/DeflectorVortexConstants.java` at line 17, The DeflectorVortexConstants class is a constants holder and should not be instantiable; add a private constructor (e.g., private DeflectorVortexConstants() { throw new AssertionError("Utility class"); }) inside the DeflectorVortexConstants class to prevent instantiation and signaling its utility-only purpose.src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/io/DeflectorIOVortex.java (1)
85-92: Style nit: prefer!deflectorMotor.hasActiveFault()over== false.Proposed fix
inputs.deflectorMotorConnected = deflectorMotorConnectedDebouncer.calculate( - deflectorMotor.hasActiveFault() == false // Basic connection/fault check + !deflectorMotor.hasActiveFault() // Basic connection/fault check );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/io/DeflectorIOVortex.java` around lines 85 - 92, In updateInputs replace the boolean comparison "deflectorMotor.hasActiveFault() == false" with the more idiomatic negation "!deflectorMotor.hasActiveFault()" (used inside the deflectorMotorConnectedDebouncer.calculate call) to improve readability; keep the surrounding call to deflectorMotorConnectedDebouncer.calculate and BaseStatusSignal.refreshAll(encoderPosition, encoderVelocity) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/intake/roller/constants/IntakeRollerConstants.java`:
- Line 27: Add a Javadoc comment to the IntakeRollerConstants.LIMITS_ENABLED
constant that documents its effect: explain what behavior occurs when
LIMITS_ENABLED is true vs false (which clamping/adjusted code paths in
IntakeRollerSubsystem are enabled or bypassed), include any units/constraints or
examples if relevant, and keep the comment consistent in style with the other
constants in IntakeRollerConstants so callers and reviewers can immediately
understand how IntakeRollerSubsystem will behave based on this flag.
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/constants/DeflectorVortexConstants.java`:
- Line 19: The constant ABSOLUTE_ENCODER_ZERO_OFFSET in DeflectorVortexConstants
is a placeholder (0.0) and must be calibrated per-robot; replace the literal 0.0
with the measured encoder zero offset for the current robot (or load it from a
per-robot configuration/environment/RobotPreferences at startup) and add a clear
TODO/comment indicating it must be measured and saved for competition so
position control uses the correct offset.
- Around line 25-26: Update the stale comment in DeflectorVortexConstants
referencing the wrong subsystem: change the comment for
SENSOR_TO_MECHANISM_RATIO to mention the deflector mechanism instead of the
turret mechanism so it correctly reads that this is the gear ratio between the
deflector mechanism and the motor sensor.
- Around line 15-18: Remove the unused CTRE import and constant: delete the
import of NeutralModeValue and the public static final NeutralModeValue
NEUTRAL_MODE from class DeflectorVortexConstants since the Vortex implementation
uses REV SparkFlex idle modes instead; add missing velocity PID gains by
defining VELOCITY_P, VELOCITY_I, and VELOCITY_D constants in
DeflectorVortexConstants to back DeflectorIO.PIDSlots.VELOCITY; and update the
stale inline comment that references the "turret mechanism" to read "deflector
mechanism" so the comment accurately describes the DeflectorVortexConstants
class.
- Around line 21-23: The code is missing velocity PID constants and never
configures ClosedLoopSlot.kSlot1 used by setDeflectorVelocity(); add
VELOCITY_P_GAIN, VELOCITY_I_GAIN, and VELOCITY_D_GAIN to
DeflectorVortexConstants (and mirror in DeflectorTalonFXSConstants if present),
then in the motor/controller configuration where slot 0 is initialized (the same
initialization block that sets POSITION_P_GAIN/POSITION_I_GAIN/POSITION_D_GAIN)
call the controller's config_kP/config_kI/config_kD (or equivalent) for
ClosedLoopSlot.kSlot1 using the new VELOCITY_* constants so slot 1 is properly
set up for velocity control referenced by setDeflectorVelocity().
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/io/DeflectorIOVortex.java`:
- Around line 43-44: The backLimitSwitch DigitalInput is allocated but never
read—update updateInputs to sample backLimitSwitch and populate inputs.backLimit
(using backLimitDebouncer to debounce the raw backLimitSwitch.get() value) so
the inputs.backLimit field reflects the physical switch; locate backLimitSwitch
and backLimitDebouncer in DeflectorIOVortex and add a line like inputs.backLimit
= backLimitDebouncer.calculate(backLimitSwitch.get()) (or the appropriate
Debouncer method) when filling inputs in updateInputs, or remove the unused
backLimitSwitch/backLimitDebouncer if you prefer to free the DIO port.
- Around line 152-156: The log key used in setDeflectorOpenLoop is inconsistent
with the subsystem convention; update the Logger.recordOutput call in the
setDeflectorOpenLoop method to use the "Launcher/Deflector/…" prefix (matching
other entries) instead of "deflector/openLoopPercentOut" so it follows the same
naming scheme used across the subsystem; leave the percentOutput argument and
the deflectorMotor.set(percentOutput) call unchanged.
- Around line 110-117: seedInternalEncoder currently seeds the internal encoder
with raw rotations but the encoder has positionConversionFactor set to
SENSOR_TO_MECHANISM_RATIO; update the seeding in seedInternalEncoder so the
value passed to deflectorMotor.getEncoder().setPosition(...) multiplies
absolutePosition.in(Rotations) by
DeflectorVortexConstants.SENSOR_TO_MECHANISM_RATIO to account for the configured
conversion factor (i.e., seed using converted units instead of raw rotations).
- Around line 59-83: The constructor configures PID only for
ClosedLoopSlot.kSlot0 so velocity closed-loop (mapped to PIDSlots.VELOCITY /
ClosedLoopSlot.kSlot1 in setDeflectorVelocity) has no gains; add
VELOCITY_P_GAIN, VELOCITY_I_GAIN, VELOCITY_D_GAIN constants to
DeflectorVortexConstants and call deflectorMotorConfig.closedLoop.pid(...,
ClosedLoopSlot.kSlot1) in the DeflectorIOVortex constructor to set them. Also
fix the double-applied gear ratio by either making SENSOR_TO_MECHANISM_RATIO
reflect the actual conversion (e.g., 0.5 for 2:1) or remove the extra manual
division where encoder position is adjusted; replace comparisons of
deflectorMotor.hasActiveFault() == false with !deflectorMotor.hasActiveFault();
read and populate the previously unused backLimitSwitch state in updateInputs();
and change the telemetry/log key path to follow the existing
"Launcher/Deflector/…" naming convention.
- Around line 94-100: The code applies the sensor-to-mechanism ratio twice:
positionConversionFactor is already applied to deflectorMotor.getEncoder() (via
positionConversionFactor) so inputs.deflectorMotorPosition is in mechanism
rotations, yet inputs.deflectorPosition divides that value by
DeflectorVortexConstants.SENSOR_TO_MECHANISM_RATIO again; fix by choosing one
convention and making it consistent — either remove the manual division in the
computation of inputs.deflectorPosition (keep positionConversionFactor as
SENSOR_TO_MECHANISM_RATIO) or change the positionConversionFactor call to use
1.0 / DeflectorVortexConstants.SENSOR_TO_MECHANISM_RATIO and keep the existing
division; update only the code around inputs.deflectorMotorPosition /
inputs.deflectorPosition and the positionConversionFactor usage so the ratio is
applied exactly once.
---
Outside diff comments:
In `@src/main/java/frc/alotobots/Constants.java`:
- Around line 82-89: Remove the orphaned DEFLECTOR_CAN_ID constant, and replace
the two zero values so DEFLECTOR_ENCODER_CAN_ID and DEFLECTOR_MOTOR_CAN_ID are
set to distinct, non-zero CAN IDs (do not leave them both as 0); update the
existing TODO to mention both encoder and motor IDs need real values, and verify
the chosen IDs do not collide with other constants in the file or the rest of
the codebase (also confirm DeflectorIOVortex.java references still use
DEFLECTOR_ENCODER_CAN_ID and DEFLECTOR_MOTOR_CAN_ID).
---
Duplicate comments:
In
`@src/main/java/frc/alotobots/rebuilt/commands/groups/IndexIntoShooterAndShoot.java`:
- Around line 26-41: IndexIntoShooterAndShoot currently extends
ParallelCommandGroup which causes a duplicate requirement on shooterSubsystem
because ShooterShootAtVelocity (used in the first command and again inside the
nested ParallelCommandGroup) both require the same subsystem; change the outer
class to extend SequentialCommandGroup so the first ShooterShootAtVelocity (the
one using .until(shooterSubsystem::isAtTargetVelocity)) runs to completion
before the nested ParallelCommandGroup with DefaultBeltRunAtVelocity,
DefaultKickerRunAtVelocity, and the second ShooterShootAtVelocity starts,
preventing the overlapping requirement and the IllegalArgumentException.
- Line 33: The live command currently uses the test constant
SHOOTER_TEST_VELOCITY; replace that usage in IndexIntoShooterAndShoot (the
lambda passed to shooterSubsystem) with a new production constant (e.g.,
SHOOT_VELOCITY) defined in ShooterConstants.Setpoints, and leave
SHOOTER_TEST_VELOCITY only for characterization/testing routines; update
ShooterConstants.Setpoints to add SHOOT_VELOCITY with the tuned competition
value, change all live-command references from SHOOTER_TEST_VELOCITY to
SHOOT_VELOCITY (including the lambda in IndexIntoShooterAndShoot), and run a
project-wide search to ensure no other live/driver-facing code still references
the test constant.
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/DeflectorSubsystem.java`:
- Line 108: Logger.recordOutput is writing the string "PERCENT_OUTPUT" to the
"Launcher/Deflector/ControlType" key which conflicts with earlier writes of the
DeflectorIO.PIDSlots enum; update the DeflectorIO.PIDSlots enum to include a
PERCENT_OUTPUT member and change the Logger.recordOutput call in
DeflectorSubsystem to pass DeflectorIO.PIDSlots.PERCENT_OUTPUT (rather than the
raw string) so the logged type remains consistent for that key.
- Around line 59-60: The log key for targetAngle is inconsistent: change the
Logger.recordOutput call that currently uses "Deflector/TargetAngle" to the
consistent "Launcher/Deflector/TargetAngle" so it matches the
Logger.processInputs namespace and other keys; update the call in
DeflectorSubsystem where Logger.recordOutput("Deflector/TargetAngle",
targetAngle.in(Degree)) is invoked to use
Logger.recordOutput("Launcher/Deflector/TargetAngle", targetAngle.in(Degree)).
- Around line 77-92: The Javadoc for runToTargetVelocity incorrectly mentions
"intake extendo" due to a copy-paste artifact; update the comment to describe
that this method controls the deflector and moves it to a specified velocity (in
radians per second) constrained by MAX_OPERATOR_VELOCITY, keeping the parameter
and behavior the same (references: runToTargetVelocity, MAX_OPERATOR_VELOCITY,
io.setDeflectorVelocity, Logger.recordOutput). Ensure the summary and `@param`
text refer to the deflector rather than the intake extendo and that the units
and constraint description remain accurate.
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/io/DeflectorIO.java`:
- Around line 66-74: The Javadoc for setDeflectorVelocity mistakenly refers to
"wrist" instead of "deflector"; update the comment above both overloaded methods
setDeflectorVelocity(AngularVelocity, PIDSlots) and
setDeflectorVelocity(AngularVelocity) to say "Sets the deflector to run at a
target velocity using closed-loop control" (and adjust any parameter
descriptions if needed) so the doc matches the interface's purpose.
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/shooter/ShooterSubsystem.java`:
- Line 36: The bug is that targetVelocity is final and never updated so
isAtTargetVelocity() always compares to 0 rad/s; in runToTargetVelocity(...) you
compute adjustedVelocity but never store it, letting the debouncer in
AT_TARGET_VELOCITY_TIME_THRESHOLD fire immediately. Fix by making targetVelocity
mutable (remove final) and assign it the adjustedVelocity computed in
runToTargetVelocity, ensure isAtTargetVelocity() compares against that stored
value, and reset targetVelocity to zero (or a sentinel) in stop() so the
IndexIntoShooterAndShoot.until(isAtTargetVelocity) gate closes when the shooter
halts.
- Around line 51-52: The Javadoc for the ShooterSubsystem method incorrectly
states the unit as "meters per second" for the parameter named velocity which is
actually an AngularVelocity; update the `@param` description to reference
AngularVelocity (and the expected angular units, e.g., rad/s or RPM as used
elsewhere) and state the valid range is constrained between -MAX_SPEED and
MAX_SPEED; locate the comment on the velocity parameter in ShooterSubsystem
(parameter name velocity) and replace the text to accurately describe the type
and range.
- Around line 62-69: Logger.recordOutput is writing different Java types for the
same key "Launcher/Shooter/ControlType" (an enum via
ShooterIO.PIDSlots.DEFAULT_VELOCITY and a String "PERCENT_OUTPUT"), causing
later writes to be dropped; update the writes to use a consistent type—e.g.,
convert the enum to a String when recording (replace the first
Logger.recordOutput call that logs ShooterIO.PIDSlots.DEFAULT_VELOCITY with its
name/value) or change the percent-output path to record the same enum constant
(modify runShooterPercentOutput to record the matching ShooterIO.PIDSlots enum)
so both Logger.recordOutput calls use the same type and key consistently.
---
Nitpick comments:
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/constants/DeflectorVortexConstants.java`:
- Line 17: The DeflectorVortexConstants class is a constants holder and should
not be instantiable; add a private constructor (e.g., private
DeflectorVortexConstants() { throw new AssertionError("Utility class"); })
inside the DeflectorVortexConstants class to prevent instantiation and signaling
its utility-only purpose.
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/io/DeflectorIOVortex.java`:
- Around line 85-92: In updateInputs replace the boolean comparison
"deflectorMotor.hasActiveFault() == false" with the more idiomatic negation
"!deflectorMotor.hasActiveFault()" (used inside the
deflectorMotorConnectedDebouncer.calculate call) to improve readability; keep
the surrounding call to deflectorMotorConnectedDebouncer.calculate and
BaseStatusSignal.refreshAll(encoderPosition, encoderVelocity) unchanged.
| /** Contains physical limits and safety thresholds for the intake roller. */ | ||
| public static final class Limits { | ||
|
|
||
| public static final boolean LIMITS_ENABLED = true; |
There was a problem hiding this comment.
Add a Javadoc comment to LIMITS_ENABLED documenting its effect.
Unlike the other constants in this class, LIMITS_ENABLED has no doc comment explaining what happens when it is true vs false (e.g., which clamping/adjusted paths activate in IntakeRollerSubsystem). This is consistent with the PR-wide documentation gap flagged for all subsystem constants files.
📝 Suggested addition
+ /**
+ * When {`@code` true}, enforces output limits (MAX_OPEN_LOOP_PERCENTAGE,
+ * MAX_OPEN_LOOP_INTAKE_PERCENTAGE, MAX_OPEN_LOOP_EJECT_PERCENTAGE) in the subsystem.
+ * Set to {`@code` false} to bypass limits during testing.
+ */
public static final boolean LIMITS_ENABLED = true;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public static final boolean LIMITS_ENABLED = true; | |
| /** | |
| * When {`@code` true}, enforces output limits (MAX_OPEN_LOOP_PERCENTAGE, | |
| * MAX_OPEN_LOOP_INTAKE_PERCENTAGE, MAX_OPEN_LOOP_EJECT_PERCENTAGE) in the subsystem. | |
| * Set to {`@code` false} to bypass limits during testing. | |
| */ | |
| public static final boolean LIMITS_ENABLED = true; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/intake/roller/constants/IntakeRollerConstants.java`
at line 27, Add a Javadoc comment to the IntakeRollerConstants.LIMITS_ENABLED
constant that documents its effect: explain what behavior occurs when
LIMITS_ENABLED is true vs false (which clamping/adjusted code paths in
IntakeRollerSubsystem are enabled or bypassed), include any units/constraints or
examples if relevant, and keep the comment consistent in style with the other
constants in IntakeRollerConstants so callers and reviewers can immediately
understand how IntakeRollerSubsystem will behave based on this flag.
|
|
||
| public class DeflectorVortexConstants { | ||
| public static final NeutralModeValue NEUTRAL_MODE = NeutralModeValue.Brake; | ||
| public static final double ABSOLUTE_ENCODER_ZERO_OFFSET = 0.0; |
There was a problem hiding this comment.
ABSOLUTE_ENCODER_ZERO_OFFSET = 0.0 is an uncalibrated placeholder
A raw value of 0.0 will only be correct if the physical mechanism is aligned to the encoder's factory zero on every robot. This needs to be measured and set per-robot before competition deployment, otherwise the deflector position control will be offset.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/constants/DeflectorVortexConstants.java`
at line 19, The constant ABSOLUTE_ENCODER_ZERO_OFFSET in
DeflectorVortexConstants is a placeholder (0.0) and must be calibrated
per-robot; replace the literal 0.0 with the measured encoder zero offset for the
current robot (or load it from a per-robot
configuration/environment/RobotPreferences at startup) and add a clear
TODO/comment indicating it must be measured and saved for competition so
position control uses the correct offset.
Comment on lines
+43
to
+44
| private final DigitalInput backLimitSwitch; | ||
| private final Debouncer backLimitDebouncer; |
There was a problem hiding this comment.
backLimitSwitch is created but never read — inputs.backLimit stays false.
A DigitalInput is allocated on DIO port 0 (line 57) but updateInputs never populates inputs.backLimit. Either wire it up or remove the unused allocation to free the DIO port.
Option A — populate the field
inputs.deflectorEncoderPosition = encoderPosition.getValue();
inputs.deflectorEncoderVelocity = encoderVelocity.getValue();
+
+ inputs.backLimit = backLimitDebouncer.calculate(backLimitSwitch.get());
}Also applies to: 86-108
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/io/DeflectorIOVortex.java`
around lines 43 - 44, The backLimitSwitch DigitalInput is allocated but never
read—update updateInputs to sample backLimitSwitch and populate inputs.backLimit
(using backLimitDebouncer to debounce the raw backLimitSwitch.get() value) so
the inputs.backLimit field reflects the physical switch; locate backLimitSwitch
and backLimitDebouncer in DeflectorIOVortex and add a line like inputs.backLimit
= backLimitDebouncer.calculate(backLimitSwitch.get()) (or the appropriate
Debouncer method) when filling inputs in updateInputs, or remove the unused
backLimitSwitch/backLimitDebouncer if you prefer to free the DIO port.
Comment on lines
+110
to
+117
| public void seedInternalEncoder() { | ||
| // Wait a moment for the CANcoder to send its initial data over the bus | ||
| Angle absolutePosition = deflectorEncoder.getAbsolutePosition().getValue(); | ||
|
|
||
| // Tell the Vortex internal encoder that its current position is the CANcoder's absolute | ||
| // position | ||
| deflectorMotor.getEncoder().setPosition(absolutePosition.in(Rotations)); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's explore the file structure
find . -name "DeflectorIOVortex.java" -type fRepository: 5152Alotobots/5152_Rebuilt
Length of output: 163
🏁 Script executed:
# Search for positionConversionFactor in the codebase
rg "positionConversionFactor" --type javaRepository: 5152Alotobots/5152_Rebuilt
Length of output: 247
🌐 Web query:
REVLib SparkFlex encoder setPosition positionConversionFactor unit conversion
💡 Result:
In REVLib (SPARK Flex), the encoder’s position is natively in motor rotations, but positionConversionFactor scales both what you read (getPosition) and what you command (setPosition / position setpoints). (codedocs.revrobotics.com)
What units does setPosition(x) expect?
- If you did not set a position conversion factor:
setPosition(x)expects rotations. (codedocs.revrobotics.com) - If you did set
positionConversionFactor = f:setPosition(x)expects “your units”, wheregetPosition()returns:rotations * fsetPosition(x)sets the encoder such that the reported position becomesx(in those same units). (codedocs.revrobotics.com)
Conversion math (useful when mixing native rotations and converted units)
Let f = positionConversionFactor in (your units / rotation).
- Convert rotations → units:
units = rotations * f - Convert units → rotations:
rotations = units / f
So:
- If you want the encoder to read
X_units, callsetPosition(X_units)(no extra math). - If you only have a value in rotations but your encoder is configured with factor
f, callsetPosition(rotations * f).
Setting the factor in REVLib 2025+ (SparkFlex)
REVLib moved these settings into config objects (rather than encoder.setPositionConversionFactor(...) in many cases). (docs.revrobotics.com)
Use the appropriate config for your encoder type (e.g., EncoderConfig.positionConversionFactor(...), ExternalEncoderConfig.positionConversionFactor(...)). (codedocs.revrobotics.com)
Position control setpoints
Closed-loop Position setpoints are in rotations by default, or in whatever unit your position conversion factor defines. (docs.revrobotics.com)
Citations:
- 1: https://codedocs.revrobotics.com/java/com/revrobotics/spark/sparkflexexternalencoder?utm_source=openai
- 2: https://codedocs.revrobotics.com/java/com/revrobotics/spark/sparkflexexternalencoder?utm_source=openai
- 3: https://codedocs.revrobotics.com/java/com/revrobotics/spark/sparkflexexternalencoder?utm_source=openai
- 4: https://docs.revrobotics.com/revlib/home/install/changelog?utm_source=openai
- 5: https://codedocs.revrobotics.com/java/com/revrobotics/spark/sparkflexexternalencoder?utm_source=openai
- 6: https://docs.revrobotics.com/revlib/spark/closed-loop/position-control-mode?utm_source=openai
🏁 Script executed:
cat -n ./src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/io/DeflectorIOVortex.javaRepository: 5152Alotobots/5152_Rebuilt
Length of output: 7309
🏁 Script executed:
# Also find the constants file to see what SENSOR_TO_MECHANISM_RATIO is
find . -name "DeflectorVortexConstants.java" -type fRepository: 5152Alotobots/5152_Rebuilt
Length of output: 177
🏁 Script executed:
cat -n ./src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/constants/DeflectorVortexConstants.javaRepository: 5152Alotobots/5152_Rebuilt
Length of output: 1316
seedInternalEncoder must apply positionConversionFactor when seeding the internal encoder position.
The encoder is configured with positionConversionFactor = SENSOR_TO_MECHANISM_RATIO (value 2). Per REVLib documentation, setPosition() expects a value in converted units when a conversion factor is configured. Currently, line 116 passes absolutePosition.in(Rotations) (raw rotations), but it should pass absolutePosition.in(Rotations) * DeflectorVortexConstants.SENSOR_TO_MECHANISM_RATIO to account for the conversion factor. Without this, the encoder will seed to half the intended position.
Correct the call to:
deflectorMotor.getEncoder().setPosition(absolutePosition.in(Rotations) * DeflectorVortexConstants.SENSOR_TO_MECHANISM_RATIO);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/io/DeflectorIOVortex.java`
around lines 110 - 117, seedInternalEncoder currently seeds the internal encoder
with raw rotations but the encoder has positionConversionFactor set to
SENSOR_TO_MECHANISM_RATIO; update the seeding in seedInternalEncoder so the
value passed to deflectorMotor.getEncoder().setPosition(...) multiplies
absolutePosition.in(Rotations) by
DeflectorVortexConstants.SENSOR_TO_MECHANISM_RATIO to account for the configured
conversion factor (i.e., seed using converted units instead of raw rotations).
follow commands never end, they take suppliers to commands stop once they reach the target
|
Note Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
|
✅ Created PR with unit tests: #6 |
Member
Author
@coderabbitai I didn't ask for unit tests. We don't use them. I wanted you to review this pr |
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/frc/alotobots/rebuilt/subsystems/launcher/turret/io/TurretIO.java (1)
52-58:⚠️ Potential issue | 🟡 MinorIncorrect Javadoc: PID slot mapping is reversed.
The comment says
(0 for velocity, 1 for position)but thePIDSlotsenum definesDEFAULT_POSITIONat ordinal 0 andVELOCITYat ordinal 1. This will mislead implementors.📝 Proposed fix
/** * Sets the turret to run to a target position using closed-loop control. * * `@param` position The target angle to move to - * `@param` pidSlot The PID slot to use (0 for velocity, 1 for position) + * `@param` pidSlot The PID slot to use (0 for position, 1 for velocity) */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/turret/io/TurretIO.java` around lines 52 - 58, The Javadoc for setTurretPosition incorrectly documents PID slot ordinals; update the comment to match the PIDSlots enum (DEFAULT_POSITION is ordinal 0 and VELOCITY is ordinal 1) so implementors aren't misled—modify the param description for pidSlot in the setTurretPosition Javadoc to state "(0 for position, 1 for velocity)" or reference the PIDSlots values DEFAULT_POSITION and VELOCITY by name.
♻️ Duplicate comments (5)
src/main/java/frc/alotobots/rebuilt/subsystems/belt/constants/BeltConstants.java (1)
26-48: Update stale Javadocs that reference “wrist”/“flywheel.”These nested-class comments are copy‑pasted and don’t describe belt behavior, which is misleading for future maintainers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/frc/alotobots/rebuilt/subsystems/belt/constants/BeltConstants.java` around lines 26 - 48, The Javadoc comments for the nested classes in BeltConstants (Thresholds, Limits, Setpoints) refer to "wrist" and "flywheel" but this file defines belt-related constants; update those comments to accurately describe belt behavior and intent. Open the BeltConstants class and replace misleading descriptions (e.g., the class-level comment and the nested-class comments above Thresholds, Limits, and Setpoints) so they reference belt thresholds, belt physical limits/safety, and belt position setpoints, and adjust any inline Javadoc for constants like AT_TARGET_VELOCITY_SPEED_THRESHOLD, AT_TARGET_VELOCITY_TIME_THRESHOLD, MAX_OPEN_LOOP_PERCENTAGE, MAX_SPEED, and LIMITS_ENABLED to mention belt-specific semantics.src/main/java/frc/alotobots/rebuilt/subsystems/kicker/constants/KickerConstants.java (1)
26-48:⚠️ Potential issue | 🟡 MinorFix copy‑pasted "wrist" Javadocs to reference the kicker.
📝 Suggested fixes
- /** Contains threshold values for various wrist operations. */ + /** Contains threshold values for various kicker operations. */ @@ - /** Contains physical limits and safety thresholds for the wrist. */ + /** Contains physical limits and safety thresholds for the kicker. */ @@ - /** Contains position setpoints for different wrist states. */ + /** Contains velocity setpoints for different kicker states. */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/frc/alotobots/rebuilt/subsystems/kicker/constants/KickerConstants.java` around lines 26 - 48, The Javadoc comments incorrectly refer to "wrist" — update the documentation to reference the kicker across this file: change the class-level and nested Javadocs for KickerConstants and its inner classes Thresholds, Limits, and Setpoints (symbols: KickerConstants, Thresholds, Limits, Setpoints) so the descriptions mention "kicker" and relevant kicker terminology (e.g., flywheel, max speed, setpoints) instead of "wrist".src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/constants/DeflectorConstants.java (1)
25-53:⚠️ Potential issue | 🟡 MinorUpdate "wrist" Javadocs to reference the deflector.
📝 Suggested fixes
- /** Contains threshold values for various wrist operations. */ + /** Contains threshold values for various deflector operations. */ @@ - /** How long the wrist must be "at position" to classify as "at position" */ + /** How long the deflector must be "at position" to classify as "at position" */ @@ - /** Contains physical limits and safety thresholds for the wrist. */ + /** Contains physical limits and safety thresholds for the deflector. */ @@ - /** Contains position setpoints for different wrist states. */ + /** Contains position setpoints for different deflector states. */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/constants/DeflectorConstants.java` around lines 25 - 53, The Javadocs incorrectly reference "wrist" instead of the deflector; update the Javadoc text for the nested classes and constants (e.g., Thresholds, Limits, Setpoints and their fields like DEFLECTOR_AT_TARGET_ANGLE_POSITION_THRESHOLD, DEFLECTOR_AT_TARGET_ANGLE_TIME_THRESHOLD, DEFLECTOR_MAX_ANGLE, DEFLECTOR_MIN_ANGLE) to say "deflector" (or "deflector angle/deflector operations") where it currently says "wrist" so the documentation matches the DeflectorConstants semantics. Ensure all occurrences in the comments above these classes/fields are changed accordingly.src/main/java/frc/alotobots/rebuilt/commands/groups/IndexIntoShooterAndShoot.java (1)
33-35:SHOOTER_TEST_VELOCITYis still used as the live competition shot setpoint.This was flagged in a prior review. Based on learnings, the test velocity values are intentionally low for testing and need to be tuned to competition values before deployment. Consider renaming to a production constant or adding a tracking TODO before this goes to competition.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/frc/alotobots/rebuilt/commands/groups/IndexIntoShooterAndShoot.java` around lines 33 - 35, IndexIntoShooterAndShoot is using the test setpoint ShooterConstants.Setpoints.SHOOTER_TEST_VELOCITY for live shooting; replace this with the intended production setpoint (e.g., add or use ShooterConstants.Setpoints.SHOOTER_COMPETITION_VELOCITY or a named production constant) or at minimum rename SHOOTER_TEST_VELOCITY to a production name and add a clear TODO comment; update the DefaultShooterRunAtVelocity invocation (and any tests) to reference the new production constant and ensure shooterSubsystem::isAtTargetVelocity logic remains compatible.src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/io/DeflectorIO.java (1)
68-73: Update Javadoc to reference the deflector, not the wrist.The description is copy‑pasted and misleading for this subsystem.
✏️ Proposed fix
- * Sets the wrist to run at a target velocity using closed-loop control. + * Sets the deflector to run at a target velocity using closed-loop control.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/io/DeflectorIO.java` around lines 68 - 73, Update the incorrect Javadoc in the DeflectorIO class for the method currently documented as "Sets the wrist to run at a target velocity using closed-loop control." Replace "wrist" with "deflector" and adjust the description to accurately describe the deflector's behavior; keep the param tags but ensure they reference the deflector's target velocity and PID slot. Locate the Javadoc above the method in DeflectorIO (the method with the existing "Sets the wrist..." comment) and edit the summary and param descriptions accordingly.
🧹 Nitpick comments (9)
src/main/java/frc/alotobots/rebuilt/subsystems/launcher/shooter/constants/ShooterConstants.java (1)
46-50: Document that the test velocity is a placeholder and should be tuned.Based on learnings: In ShooterConstants.java, the SHOOTER_TEST_VELOCITY constant is intentionally set to a low value (DegreesPerSecond.of(90)) for testing purposes and needs to be tuned to competition values before deployment.📝 Suggested note
- public static final AngularVelocity SHOOTER_TEST_VELOCITY = RotationsPerSecond.of(45); + // TODO: Tune to competition velocity; current value is for testing + public static final AngularVelocity SHOOTER_TEST_VELOCITY = RotationsPerSecond.of(45);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/shooter/constants/ShooterConstants.java` around lines 46 - 50, Update the documentation for the Setpoints class / SHOOTER_TEST_VELOCITY constant to explicitly state this value is a low placeholder for testing and must be tuned to competition values before deployment; add a short javadoc above the Setpoints class or the SHOOTER_TEST_VELOCITY field calling out it's intentionally conservative, how to tune it, and a TODO/reminder to adjust before release so reviewers/maintainers won't assume it's final.src/main/java/frc/alotobots/rebuilt/subsystems/launcher/shooter/io/ShooterIOTalonFX.java (2)
189-191: Missing braces onifstatement.Single-line
ifwithout braces is error-prone and inconsistent with the rest of the codebase style (other blocks use braces).Proposed fix
- if (pidSlot == PIDSlots.OPEN_LOOP) - throw new IllegalArgumentException( - "PIDSlots value OPEN_LOOP cannot be used in a closed loop control mode"); + if (pidSlot == PIDSlots.OPEN_LOOP) { + throw new IllegalArgumentException( + "PIDSlots value OPEN_LOOP cannot be used in a closed loop control mode"); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/shooter/io/ShooterIOTalonFX.java` around lines 189 - 191, The single-line if using PIDSlots.OPEN_LOOP in ShooterIOTalonFX.java must be converted to a braced block to match project style; replace the one-line if throw with a standard if { ... } block containing the throw new IllegalArgumentException(...) and preserve the existing message and indentation so the check for PIDSlots.OPEN_LOOP remains functionally identical but uses braces.
168-173:IllegalArgumentExceptionindefaultcase is a hard crash during a match — consider a safer fallback.If the TalonFX ever reports an unexpected closed-loop slot value, this
switchwill throw an unhandledIllegalArgumentException, which could crash the robot code during a match. While the current code paths shouldn't trigger this (only slot 0 is used), a defensive approach would be safer for competition.Proposed fix: log a warning instead of throwing
inputs.shooterMotorsPIDSlot = switch (currentPidSlot.getValue()) { case 0 -> PIDSlots.DEFAULT_VELOCITY; - default -> throw new IllegalArgumentException( - "No defined PID slot for value: " + currentPidSlot.getValue()); + default -> { + System.err.println("Unexpected PID slot value: " + currentPidSlot.getValue()); + yield PIDSlots.DEFAULT_VELOCITY; + } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/shooter/io/ShooterIOTalonFX.java` around lines 168 - 173, The switch in ShooterIOTalonFX that assigns inputs.shooterMotorsPIDSlot using currentPidSlot.getValue() should not throw an IllegalArgumentException on the default branch; instead, replace the throw with a safe fallback that logs a warning about the unexpected PID slot (include currentPidSlot.getValue() in the message) and assigns PIDSlots.DEFAULT_VELOCITY so the robot keeps running; update the default branch of that switch to perform the logging via the existing team logging facility (or DriverStation/Logger) and then set inputs.shooterMotorsPIDSlot = PIDSlots.DEFAULT_VELOCITY.src/main/java/frc/alotobots/OI.java (1)
128-138: Temporary button remapping acknowledged — ensure the TODO is tracked.The
shoottrigger was moved todriverController.a()(previouslyclimbAction's binding) andclimbActionis commented out. The TODO on line 137 notes this should be reverted. Consider creating a tracking issue so it doesn't get lost before competition.Minor: typo on line 128 — "unnessecary" → "unnecessary".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/frc/alotobots/OI.java` around lines 128 - 138, The shoot Trigger was temporarily rebound to driverController.a() and climbAction was commented out; create a tracking task (e.g., Jira/GitHub issue) to revert the control mapping before competition and document the intended binding in the code where shoot and climbAction are defined (referencing shoot, climbAction, and driverController.a()); also restore the original binding when the issue is resolved (TODO marked near the shoot declaration) and fix the comment typo "unnessecary" → "unnecessary" near the climbAction line to avoid confusion.src/main/java/frc/alotobots/rebuilt/subsystems/launcher/turret/TurretSubsystem.java (1)
76-85: Missing Javadoc onrunToTargetVelocity.This public method lacks documentation. The PR comments already identified this as a gap.
Proposed fix
+ /** + * Commands the turret to run at a target velocity using closed-loop control. + * + * `@param` velocity The target angular velocity for the turret + */ public void runToTargetVelocity(AngularVelocity velocity) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/turret/TurretSubsystem.java` around lines 76 - 85, Add Javadoc for the public method runToTargetVelocity(AngularVelocity velocity) explaining its purpose (commands the turret to a target angular velocity), parameters (describe the velocity argument and units), behavior (clamps the input to TURRET_MAX_VELOCITY when TURRET_LIMITS_ENABLED is true, otherwise passes the value through), side effects (calls io.setTurretVelocity and records control type via Logger.recordOutput with TurretIO.PIDSlots.VELOCITY), and any exceptions or constraints; reference the method name runToTargetVelocity, the AngularVelocity type, TURRET_LIMITS_ENABLED, TURRET_MAX_VELOCITY, io.setTurretVelocity, and Logger.recordOutput in the Javadoc.src/main/java/frc/alotobots/rebuilt/subsystems/launcher/turret/io/TurretIO.java (1)
62-64: Missing Javadoc on newsetTurretVelocityoverloads.Both new velocity methods lack documentation. Add at minimum
@paramdescriptions to match the style of the existingsetTurretPositionmethod.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/turret/io/TurretIO.java` around lines 62 - 64, Add Javadoc for the two new overloads of setTurretVelocity(AngularVelocity, PIDSlots) and setTurretVelocity(AngularVelocity) mirroring the style of the existing setTurretPosition method: include a short description of what the method does and add `@param` tags for the AngularVelocity parameter (describe units/meaning) and for the PIDSlots parameter on the two-arg overload (describe its purpose); ensure the comment formatting and wording match other methods in TurretIO so tooling and IDEs pick up the docs.src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/io/DeflectorIOVortex.java (2)
47-48:backLimitSwitchis explicitlynull— any future dereference will NPE.
backLimitSwitchis set tonull(line 63) andbackLimitDebounceris allocated but never used. The TODO at line 162 acknowledges the limit switch is incomplete. This is fine as a placeholder, but thenullassignment is a latent NPE risk if someone later addsbackLimitSwitch.get()without checking. Consider either removing both fields until the feature is ready, or initializing theDigitalInputwith the correct DIO port now.Also applies to: 62-63
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/io/DeflectorIOVortex.java` around lines 47 - 48, The fields backLimitSwitch and backLimitDebouncer in DeflectorIOVortex are left as a null placeholder which risks an NPE later; either remove both fields until the limit-switch feature is implemented or properly initialize them now by constructing a DigitalInput with the correct DIO port and creating the Debouncer instance (and replace the TODO usage with calls to backLimitDebouncer.calculate(backLimitSwitch.get()) or similar). Ensure any future reads of backLimitSwitch are guarded (or eliminated) so no direct dereference of a null DigitalInput can occur.
139-163:updateInputscallsdeflectorMotor.getEncoder()three times per cycle — cache the encoder reference.Each call to
getEncoder()is cheap but repeated field lookups add up. The position is read twice (lines 148 and 151) with the same value. Cache theRelativeEncoderin a field and the position reading in a local variable.♻️ Suggested approach
public void updateInputs(DeflectorIOInputs inputs) { var deflectorEncoderSignals = BaseStatusSignal.refreshAll(encoderPosition, encoderVelocity); ... - inputs.deflectorMotorAngle = Rotations.of(deflectorMotor.getEncoder().getPosition()); + double motorPos = deflectorMotor.getEncoder().getPosition(); + inputs.deflectorMotorAngle = Rotations.of(motorPos); inputs.deflectorEncoderAngle = encoderPosition.getValue(); inputs.deflectorAngle = - vortexToDeflectorAngle(Rotations.of(deflectorMotor.getEncoder().getPosition())); + vortexToDeflectorAngle(Rotations.of(motorPos));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/io/DeflectorIOVortex.java` around lines 139 - 163, The updateInputs method repeatedly calls deflectorMotor.getEncoder() and re-reads the same position; cache the encoder once (e.g., a private RelativeEncoder field or a local var assigned from deflectorMotor.getEncoder()) and read the position into a local variable, then use those cached references for Rotations.of(...), vortexToDeflectorAngle(...), and RotationsPerSecond.of(...) as well as any other reads; update uses of deflectorMotor.getEncoder().getPosition() and deflectorMotor.getEncoder().getVelocity() to use the cached encoder and the cached position/velocity values, and keep encoderPosition and encoderVelocity usage unchanged.src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/commands/DeflectorRunToPosition.java (1)
30-46: Consider whetherstop()inend()is desired when the command completes normally.When
isFinished()returnstrue(deflector at target),end(false)callsstop(), which will release the deflector from holding position. If the mechanism is back-drivable (gravity or external forces), it may drift away from the target angle before the next command takes over. If holding at the target is needed, consider only stopping wheninterruptedis true.💡 Suggested approach
`@Override` public void end(boolean interrupted) { - deflectorSubsystem.stop(); + if (interrupted) { + deflectorSubsystem.stop(); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/commands/DeflectorRunToPosition.java` around lines 30 - 46, The command DeflectorRunToPosition currently calls deflectorSubsystem.stop() unconditionally in end(boolean interrupted), which releases the mechanism even when isFinished() returns true; change end to only call deflectorSubsystem.stop() when interrupted is true (i.e., if (interrupted) deflectorSubsystem.stop();), and when ending normally either leave the motor command active or call a hold/maintain method on DeflectorSubsystem (e.g., deflectorSubsystem.holdPosition() or keep runToTargetAngle(angle) active) so the deflector remains at the target; update end(boolean interrupted), and ensure runToTargetAngle(angle), isAtTargetAngle(), and stop() usage is consistent with this behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/frc/alotobots/rebuilt/commands/groups/IndexIntoShooterAndShoot.java`:
- Around line 32-42: The spin-up command DefaultShooterRunAtVelocity currently
calls shooterSubsystem.stop() in its end(), causing a brief deceleration when
its .until() finishes before the ParallelCommandGroup starts; update the
sequence so the shooter is not stopped between stages by either overriding
DefaultShooterRunAtVelocity.end() for the spin-up instance to avoid calling
shooterSubsystem.stop() on normal completion, or convert the pattern to a
ProxyCommand/deadline (keep-running) pattern that uses a single shooter command
shared across the spin-up and feed stages (modify IndexIntoShooterAndShoot to
use the adjusted command or the deadline wrapper so the shooter velocity command
persists across the transition).
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/belt/constants/BeltConstants.java`:
- Around line 23-50: Add a concise class-level Javadoc to BeltConstants
describing its purpose as a container of static configuration constants for the
belt subsystem, and update the placeholder comment inside the Setpoints inner
class to either a clear descriptive sentence about what each setpoint represents
(e.g., "Velocity setpoints used for loading and shooting states") or a tracked
TODO with a ticket/issue reference; ensure references to inner classes
Thresholds, Limits, and Setpoints and the constant LOAD_INTO_SHOOTER_VELOCITY
are mentioned in the Javadoc or comment so future readers know where to find
velocity thresholds, physical limits, and position/velocity setpoints.
- Around line 40-41: BeltConstants currently sets MAX_SPEED to
RotationsPerSecond.of(90), which is likely the wrong unit and excessively large;
change the constants to the intended units and values (e.g., use
DegreesPerSecond.of(90) for low-test speeds or set a proper RotationsPerSecond
value for competition) and ensure LOAD_INTO_SHOOTER_VELOCITY uses the same unit
system; update MAX_SPEED and LOAD_INTO_SHOOTER_VELOCITY in BeltConstants to the
correct unit factory (DegreesPerSecond.of(...) for test-mode ~90 deg/s ≈15 RPM)
and add a comment or config flag indicating test vs competition values so they
can be safely tuned before deployment.
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/constants/DeflectorVortexConstants.java`:
- Around line 42-63: Comments in VelocityPIDConstants incorrectly refer to
"Position control" and mention the hood; update the Javadoc and inline comments
to accurately describe velocity-mode and the deflector (e.g., change "Position
control proportional gain" to "Velocity control proportional gain" for
DEFLECTOR_VELOCITY_KP and similarly adjust KI/KD/KG/KS/KV and the allowed
closed-loop error comment to reference rotations in velocity mode for
DEFLECTOR_VELOCITY_ALLOWED_CLOSED_LOOP_ERROR); also make the same wording fixes
in the other commented block referenced (the comments around the DEFLECTOR_*
velocity constants at lines ~84-87) so all references mention deflector/velocity
instead of position/hood.
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/DeflectorSubsystem.java`:
- Around line 41-42: The AutoLogOutput key for the deflector's target angle is
incorrect and currently logs as "Launcher/Shooter/TargetAngle"; update the
AutoLogOutput annotation on the targetAngle field in DeflectorSubsystem (the
private Angle targetAngle = Degrees.zero()) to use
"Launcher/Deflector/TargetAngle" so telemetry is routed to the correct deflector
path and avoids type collisions with shooter logs.
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/io/DeflectorIOTalonFXS.java`:
- Around line 111-117: The switch assigning inputs.deflectorMotorPidSlot
currently only handles ordinals 0 and 1 and will throw for PID slot 2; update
the switch on currentPidSlot.getValue() (used where inputs.deflectorMotorPidSlot
is set) to include case 2 -> PIDSlots.OPEN_LOOP (or map it to a safe fallback
like PIDSlots.DEFAULT_POSITION) instead of throwing in the default branch so
updateInputs cannot crash when the motor reports the OPEN_LOOP slot.
- Around line 72-73: DeflectorIOTalonFXS lacks PID gain configuration but still
exposes closed-loop methods; update the class constructor to configure the
TalonFXS PID slots (position and velocity) with appropriate kP/kI/kD/kF and slot
indices used by setDeflectorPosition and setDeflectorVelocity, or remove/disable
those closed-loop methods to avoid latent zero-output behavior; specifically,
add PID config calls in the DeflectorIOTalonFXS constructor for the same slot
numbers referenced by setDeflectorPosition/setDeflectorVelocity (and initialize
any feedforward/peak outputs/closed-loop params), or delete/throw in those
methods so only DeflectorIOVortex provides closed-loop control.
- Line 60: The deflector motor is being constructed with the wrong CAN ID; in
DeflectorIOTalonFXS replace the use of Constants.CanId.TURRET_CAN_ID with
Constants.CanId.DEFLECTOR_MOTOR_CAN_ID when creating deflectorMotor (the
TalonFXS instantiation), ensuring you still pass RIO_CAN_BUS as the bus argument
so the deflector uses its correct unique CAN ID instead of the turret's.
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/io/DeflectorIOVortex.java`:
- Around line 231-242: Update the Javadoc for vortexToDeflectorAngle (and the
paired deflectorAngleToVortex) to reflect the actual offset constant used in the
implementation: change the documented formula from "y = SLOPE * x + MIN_ANGLE"
to "y = SLOPE * x + DEFLECTOR_MAX_ANGLE" (and mention that the offset is
DEFLECTOR_MAX_ANGLE), so the comment matches the code using DEFLECTOR_MAX_ANGLE
as the additive term.
- Line 156: Replace the unprofessional comment in DeflectorIOVortex (the line
"// No deflector motor acceleration because REV is shit") with a neutral,
professional explanation that states the technical reason—e.g., indicate that
the REV motor controller/hardware does not support acceleration profiling or
that acceleration control is intentionally omitted due to hardware/firmware
limitations—so use a comment like "// No deflector motor acceleration: REV
controller does not support acceleration profiling (omit to avoid unsupported
behavior)". Ensure the new comment is clear and non-offensive and update only
the comment near the deflector motor acceleration handling in the
DeflectorIOVortex class.
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/shooter/constants/ShooterTalonFXConstants.java`:
- Around line 28-35: The Javadoc comments above SHOOTER_VELOCITY_KP,
SHOOTER_VELOCITY_KI, and SHOOTER_VELOCITY_KD incorrectly say "Position control";
update them to accurately describe velocity PID gains (e.g., "Velocity control
proportional gain", "Velocity control integral gain", "Velocity control
derivative gain") so the comments match the constant names and usage in
ShooterTalonFXConstants.
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/shooter/io/ShooterIOTalonFX.java`:
- Around line 83-87: Remove or clearly document the dead inversion assignment on
motorRightConfig in ShooterIOTalonFX: the motorRightConfig.MotorOutput.Inverted
setting (which uses ShooterTalonFXConstants.SHOOTER_MOTOR_DIRECTION) has no
effect for a Follower because Phoenix 6 uses the MotorAlignmentValue.Opposed
passed to the Follower request to determine inversion; either delete the
motorRightConfig.MotorOutput.Inverted branch or replace it with a concise
comment referencing the Follower request and MotorAlignmentValue.Opposed (used
in the Follower setup) so future readers know the inversion is controlled by the
Follower alignment rather than the motor's Inverted field.
- Line 70: Remove the gravity-compensation assignment from the shooter Talon FX
config: in ShooterIOTalonFX locate where motorLeftConfig.Slot0.GravityType (and
the symmetric motorRightConfig if present) is being set to
GravityTypeValue.Elevator_Static and delete that assignment so the shooter uses
the default/no gravity compensation (or simply omit setting GravityType for
Slot0); ensure no other code relies on SHOOTER_VELOCITY_KG being zero to mask
gravity compensation.
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/turret/commands/TurretFollowPosition.java`:
- Around line 43-46: TurretFollowPosition's isFinished() currently returns
turretSubsystem.isAtTargetAngle(), which ends the command after the first target
and breaks continuous tracking that relies on the dynamic Supplier<Angle> used
in execute(); change isFinished() in TurretFollowPosition to always return false
(so the command keeps running and re-reads the Supplier each cycle) or, if you
intended a one-shot, replace usage with TurretRunToPosition instead—update the
isFinished() method in TurretFollowPosition accordingly.
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/turret/constants/TurretTalonFXSConstants.java`:
- Around line 42-49: Update the Javadoc comments for the velocity PID constants
in TurretTalonFXSConstants: change the three comments that currently read
"Position control proportional/integral/derivative gain" to "Velocity control
proportional gain", "Velocity control integral gain", and "Velocity control
derivative gain" respectively so the documentation matches the constants
TURRET_VELOCITY_KP, TURRET_VELOCITY_KI, and TURRET_VELOCITY_KD.
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/turret/io/TurretIOTalonFXS.java`:
- Around line 143-149: The switch in updateInputs that sets
inputs.turretMotorPIDSlot based on currentPidSlot.getValue() must not throw on
unexpected values; replace the default branch so it assigns a safe fallback
(e.g., PIDSlots.DEFAULT_POSITION) and emit a warning log containing the
unexpected currentPidSlot.getValue() instead of throwing; keep the explicit
cases for 0 and 1 and change the default from throwing IllegalArgumentException
to logging (e.g., logger.warn/ System.err.println) with the bad value and
assigning PIDSlots.DEFAULT_POSITION.
---
Outside diff comments:
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/turret/io/TurretIO.java`:
- Around line 52-58: The Javadoc for setTurretPosition incorrectly documents PID
slot ordinals; update the comment to match the PIDSlots enum (DEFAULT_POSITION
is ordinal 0 and VELOCITY is ordinal 1) so implementors aren't misled—modify the
param description for pidSlot in the setTurretPosition Javadoc to state "(0 for
position, 1 for velocity)" or reference the PIDSlots values DEFAULT_POSITION and
VELOCITY by name.
---
Duplicate comments:
In
`@src/main/java/frc/alotobots/rebuilt/commands/groups/IndexIntoShooterAndShoot.java`:
- Around line 33-35: IndexIntoShooterAndShoot is using the test setpoint
ShooterConstants.Setpoints.SHOOTER_TEST_VELOCITY for live shooting; replace this
with the intended production setpoint (e.g., add or use
ShooterConstants.Setpoints.SHOOTER_COMPETITION_VELOCITY or a named production
constant) or at minimum rename SHOOTER_TEST_VELOCITY to a production name and
add a clear TODO comment; update the DefaultShooterRunAtVelocity invocation (and
any tests) to reference the new production constant and ensure
shooterSubsystem::isAtTargetVelocity logic remains compatible.
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/belt/constants/BeltConstants.java`:
- Around line 26-48: The Javadoc comments for the nested classes in
BeltConstants (Thresholds, Limits, Setpoints) refer to "wrist" and "flywheel"
but this file defines belt-related constants; update those comments to
accurately describe belt behavior and intent. Open the BeltConstants class and
replace misleading descriptions (e.g., the class-level comment and the
nested-class comments above Thresholds, Limits, and Setpoints) so they reference
belt thresholds, belt physical limits/safety, and belt position setpoints, and
adjust any inline Javadoc for constants like AT_TARGET_VELOCITY_SPEED_THRESHOLD,
AT_TARGET_VELOCITY_TIME_THRESHOLD, MAX_OPEN_LOOP_PERCENTAGE, MAX_SPEED, and
LIMITS_ENABLED to mention belt-specific semantics.
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/kicker/constants/KickerConstants.java`:
- Around line 26-48: The Javadoc comments incorrectly refer to "wrist" — update
the documentation to reference the kicker across this file: change the
class-level and nested Javadocs for KickerConstants and its inner classes
Thresholds, Limits, and Setpoints (symbols: KickerConstants, Thresholds, Limits,
Setpoints) so the descriptions mention "kicker" and relevant kicker terminology
(e.g., flywheel, max speed, setpoints) instead of "wrist".
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/constants/DeflectorConstants.java`:
- Around line 25-53: The Javadocs incorrectly reference "wrist" instead of the
deflector; update the Javadoc text for the nested classes and constants (e.g.,
Thresholds, Limits, Setpoints and their fields like
DEFLECTOR_AT_TARGET_ANGLE_POSITION_THRESHOLD,
DEFLECTOR_AT_TARGET_ANGLE_TIME_THRESHOLD, DEFLECTOR_MAX_ANGLE,
DEFLECTOR_MIN_ANGLE) to say "deflector" (or "deflector angle/deflector
operations") where it currently says "wrist" so the documentation matches the
DeflectorConstants semantics. Ensure all occurrences in the comments above these
classes/fields are changed accordingly.
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/io/DeflectorIO.java`:
- Around line 68-73: Update the incorrect Javadoc in the DeflectorIO class for
the method currently documented as "Sets the wrist to run at a target velocity
using closed-loop control." Replace "wrist" with "deflector" and adjust the
description to accurately describe the deflector's behavior; keep the param tags
but ensure they reference the deflector's target velocity and PID slot. Locate
the Javadoc above the method in DeflectorIO (the method with the existing "Sets
the wrist..." comment) and edit the summary and param descriptions accordingly.
---
Nitpick comments:
In `@src/main/java/frc/alotobots/OI.java`:
- Around line 128-138: The shoot Trigger was temporarily rebound to
driverController.a() and climbAction was commented out; create a tracking task
(e.g., Jira/GitHub issue) to revert the control mapping before competition and
document the intended binding in the code where shoot and climbAction are
defined (referencing shoot, climbAction, and driverController.a()); also restore
the original binding when the issue is resolved (TODO marked near the shoot
declaration) and fix the comment typo "unnessecary" → "unnecessary" near the
climbAction line to avoid confusion.
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/commands/DeflectorRunToPosition.java`:
- Around line 30-46: The command DeflectorRunToPosition currently calls
deflectorSubsystem.stop() unconditionally in end(boolean interrupted), which
releases the mechanism even when isFinished() returns true; change end to only
call deflectorSubsystem.stop() when interrupted is true (i.e., if (interrupted)
deflectorSubsystem.stop();), and when ending normally either leave the motor
command active or call a hold/maintain method on DeflectorSubsystem (e.g.,
deflectorSubsystem.holdPosition() or keep runToTargetAngle(angle) active) so the
deflector remains at the target; update end(boolean interrupted), and ensure
runToTargetAngle(angle), isAtTargetAngle(), and stop() usage is consistent with
this behavior.
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/io/DeflectorIOVortex.java`:
- Around line 47-48: The fields backLimitSwitch and backLimitDebouncer in
DeflectorIOVortex are left as a null placeholder which risks an NPE later;
either remove both fields until the limit-switch feature is implemented or
properly initialize them now by constructing a DigitalInput with the correct DIO
port and creating the Debouncer instance (and replace the TODO usage with calls
to backLimitDebouncer.calculate(backLimitSwitch.get()) or similar). Ensure any
future reads of backLimitSwitch are guarded (or eliminated) so no direct
dereference of a null DigitalInput can occur.
- Around line 139-163: The updateInputs method repeatedly calls
deflectorMotor.getEncoder() and re-reads the same position; cache the encoder
once (e.g., a private RelativeEncoder field or a local var assigned from
deflectorMotor.getEncoder()) and read the position into a local variable, then
use those cached references for Rotations.of(...), vortexToDeflectorAngle(...),
and RotationsPerSecond.of(...) as well as any other reads; update uses of
deflectorMotor.getEncoder().getPosition() and
deflectorMotor.getEncoder().getVelocity() to use the cached encoder and the
cached position/velocity values, and keep encoderPosition and encoderVelocity
usage unchanged.
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/shooter/constants/ShooterConstants.java`:
- Around line 46-50: Update the documentation for the Setpoints class /
SHOOTER_TEST_VELOCITY constant to explicitly state this value is a low
placeholder for testing and must be tuned to competition values before
deployment; add a short javadoc above the Setpoints class or the
SHOOTER_TEST_VELOCITY field calling out it's intentionally conservative, how to
tune it, and a TODO/reminder to adjust before release so reviewers/maintainers
won't assume it's final.
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/shooter/io/ShooterIOTalonFX.java`:
- Around line 189-191: The single-line if using PIDSlots.OPEN_LOOP in
ShooterIOTalonFX.java must be converted to a braced block to match project
style; replace the one-line if throw with a standard if { ... } block containing
the throw new IllegalArgumentException(...) and preserve the existing message
and indentation so the check for PIDSlots.OPEN_LOOP remains functionally
identical but uses braces.
- Around line 168-173: The switch in ShooterIOTalonFX that assigns
inputs.shooterMotorsPIDSlot using currentPidSlot.getValue() should not throw an
IllegalArgumentException on the default branch; instead, replace the throw with
a safe fallback that logs a warning about the unexpected PID slot (include
currentPidSlot.getValue() in the message) and assigns PIDSlots.DEFAULT_VELOCITY
so the robot keeps running; update the default branch of that switch to perform
the logging via the existing team logging facility (or DriverStation/Logger) and
then set inputs.shooterMotorsPIDSlot = PIDSlots.DEFAULT_VELOCITY.
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/turret/io/TurretIO.java`:
- Around line 62-64: Add Javadoc for the two new overloads of
setTurretVelocity(AngularVelocity, PIDSlots) and
setTurretVelocity(AngularVelocity) mirroring the style of the existing
setTurretPosition method: include a short description of what the method does
and add `@param` tags for the AngularVelocity parameter (describe units/meaning)
and for the PIDSlots parameter on the two-arg overload (describe its purpose);
ensure the comment formatting and wording match other methods in TurretIO so
tooling and IDEs pick up the docs.
In
`@src/main/java/frc/alotobots/rebuilt/subsystems/launcher/turret/TurretSubsystem.java`:
- Around line 76-85: Add Javadoc for the public method
runToTargetVelocity(AngularVelocity velocity) explaining its purpose (commands
the turret to a target angular velocity), parameters (describe the velocity
argument and units), behavior (clamps the input to TURRET_MAX_VELOCITY when
TURRET_LIMITS_ENABLED is true, otherwise passes the value through), side effects
(calls io.setTurretVelocity and records control type via Logger.recordOutput
with TurretIO.PIDSlots.VELOCITY), and any exceptions or constraints; reference
the method name runToTargetVelocity, the AngularVelocity type,
TURRET_LIMITS_ENABLED, TURRET_MAX_VELOCITY, io.setTurretVelocity, and
Logger.recordOutput in the Javadoc.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (38)
src/main/java/frc/alotobots/Constants.javasrc/main/java/frc/alotobots/OI.javasrc/main/java/frc/alotobots/Robot.javasrc/main/java/frc/alotobots/RobotContainer.javasrc/main/java/frc/alotobots/library/subsystems/swervedrive/io/ModuleIOTalonFX.javasrc/main/java/frc/alotobots/rebuilt/commands/groups/IndexIntoShooterAndShoot.javasrc/main/java/frc/alotobots/rebuilt/subsystems/belt/constants/BeltConstants.javasrc/main/java/frc/alotobots/rebuilt/subsystems/belt/io/BeltIOTalonFX.javasrc/main/java/frc/alotobots/rebuilt/subsystems/intake/extendo/constants/IntakeExtendoTalonFXConstants.javasrc/main/java/frc/alotobots/rebuilt/subsystems/intake/extendo/io/IntakeExtendoIOTalonFX.javasrc/main/java/frc/alotobots/rebuilt/subsystems/intake/roller/io/IntakeRollerIOTalonFX.javasrc/main/java/frc/alotobots/rebuilt/subsystems/kicker/constants/KickerConstants.javasrc/main/java/frc/alotobots/rebuilt/subsystems/kicker/io/KickerIOTalonFX.javasrc/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/DeflectorSubsystem.javasrc/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/commands/DefaultDeflectorRunAtVelocity.javasrc/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/commands/DeflectorFollowPosition.javasrc/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/commands/DeflectorRunToPosition.javasrc/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/constants/DeflectorConstants.javasrc/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/constants/DeflectorVortexConstants.javasrc/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/io/DeflectorIO.javasrc/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/io/DeflectorIOTalonFXS.javasrc/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/io/DeflectorIOVortex.javasrc/main/java/frc/alotobots/rebuilt/subsystems/launcher/shooter/ShooterSubsystem.javasrc/main/java/frc/alotobots/rebuilt/subsystems/launcher/shooter/commands/DefaultShooterRunAtVelocity.javasrc/main/java/frc/alotobots/rebuilt/subsystems/launcher/shooter/constants/ShooterConstants.javasrc/main/java/frc/alotobots/rebuilt/subsystems/launcher/shooter/constants/ShooterTalonFXConstants.javasrc/main/java/frc/alotobots/rebuilt/subsystems/launcher/shooter/io/ShooterIO.javasrc/main/java/frc/alotobots/rebuilt/subsystems/launcher/shooter/io/ShooterIOTalonFX.javasrc/main/java/frc/alotobots/rebuilt/subsystems/launcher/turret/TurretSubsystem.javasrc/main/java/frc/alotobots/rebuilt/subsystems/launcher/turret/commands/DefaultTurretRunAtVelocity.javasrc/main/java/frc/alotobots/rebuilt/subsystems/launcher/turret/commands/RunTurretToTarget.javasrc/main/java/frc/alotobots/rebuilt/subsystems/launcher/turret/commands/TurretFollowPosition.javasrc/main/java/frc/alotobots/rebuilt/subsystems/launcher/turret/commands/TurretRunToPosition.javasrc/main/java/frc/alotobots/rebuilt/subsystems/launcher/turret/constants/TurretConstants.javasrc/main/java/frc/alotobots/rebuilt/subsystems/launcher/turret/constants/TurretTalonFXSConstants.javasrc/main/java/frc/alotobots/rebuilt/subsystems/launcher/turret/io/TurretIO.javasrc/main/java/frc/alotobots/rebuilt/subsystems/launcher/turret/io/TurretIOSparkMax.javasrc/main/java/frc/alotobots/rebuilt/subsystems/launcher/turret/io/TurretIOTalonFXS.java
💤 Files with no reviewable changes (2)
- src/main/java/frc/alotobots/rebuilt/subsystems/launcher/turret/io/TurretIOSparkMax.java
- src/main/java/frc/alotobots/rebuilt/subsystems/launcher/turret/commands/RunTurretToTarget.java
✅ Files skipped from review due to trivial changes (1)
- src/main/java/frc/alotobots/rebuilt/subsystems/belt/io/BeltIOTalonFX.java
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/frc/alotobots/rebuilt/subsystems/launcher/shooter/ShooterSubsystem.java
- src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/commands/DefaultDeflectorRunAtVelocity.java
Comment on lines
+32
to
+42
| addCommands( | ||
| new DefaultShooterRunAtVelocity( | ||
| shooterSubsystem, () -> ShooterConstants.Setpoints.SHOOTER_TEST_VELOCITY) | ||
| .until(shooterSubsystem::isAtTargetVelocity), | ||
| new ParallelCommandGroup( | ||
| new DefaultBeltRunAtVelocity( | ||
| beltSubsystem, () -> BeltConstants.Setpoints.LOAD_INTO_SHOOTER_VELOCITY), | ||
| new DefaultKickerRunAtVelocity( | ||
| kickerSubsystem, () -> KickerConstants.Setpoints.LOAD_INTO_SHOOTER_VELOCITY), | ||
| new DefaultShooterRunAtVelocity( | ||
| shooterSubsystem, () -> ShooterConstants.Setpoints.SHOOTER_TEST_VELOCITY))); |
There was a problem hiding this comment.
Brief shooter deceleration between spin-up and feed stages.
When the .until() gate fires, end() on the first DefaultShooterRunAtVelocity calls shooterSubsystem.stop() before the parallel group starts the second instance. In a SequentialCommandGroup, the transition is within the same scheduler cycle (~20ms), so the impact on a heavy flywheel is minimal, but a stop() call may still send a zero/brake command to the motor controller for one cycle. If you observe velocity dips, consider overriding end() in the spin-up stage to not stop, or use a ProxyCommand / deadline pattern to keep the shooter running continuously across both stages.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/frc/alotobots/rebuilt/commands/groups/IndexIntoShooterAndShoot.java`
around lines 32 - 42, The spin-up command DefaultShooterRunAtVelocity currently
calls shooterSubsystem.stop() in its end(), causing a brief deceleration when
its .until() finishes before the ParallelCommandGroup starts; update the
sequence so the shooter is not stopped between stages by either overriding
DefaultShooterRunAtVelocity.end() for the spin-up instance to avoid calling
shooterSubsystem.stop() on normal completion, or convert the pattern to a
ProxyCommand/deadline (keep-running) pattern that uses a single shooter command
shared across the spin-up and feed stages (modify IndexIntoShooterAndShoot to
use the adjusted command or the deadline wrapper so the shooter velocity command
persists across the transition).
There was a problem hiding this comment.
@SeanErn, here are three approaches to keep the shooter running continuously between stages:
Option 1: Custom Spin-Up Command (Simplest)
Create a custom command that doesn't stop the shooter when it finishes:
// Create this as a private static class inside IndexIntoShooterAndShoot
private static class ShooterSpinUpOnly extends CommandBase {
private final ShooterSubsystem shooterSubsystem;
private final Supplier<AngularVelocity> velocitySupplier;
public ShooterSpinUpOnly(ShooterSubsystem subsystem, Supplier<AngularVelocity> velocity) {
this.shooterSubsystem = subsystem;
this.velocitySupplier = velocity;
addRequirements(subsystem);
}
`@Override`
public void execute() {
shooterSubsystem.runToTargetVelocity(velocitySupplier.get());
}
`@Override`
public void end(boolean interrupted) {
// Only stop if interrupted, not on normal completion
if (interrupted) {
shooterSubsystem.stop();
}
}
}Then update your command group:
addCommands(
new ShooterSpinUpOnly(
shooterSubsystem, () -> ShooterConstants.Setpoints.SHOOTER_TEST_VELOCITY)
.until(shooterSubsystem::isAtTargetVelocity),
new ParallelCommandGroup(
new DefaultBeltRunAtVelocity(
beltSubsystem, () -> BeltConstants.Setpoints.LOAD_INTO_SHOOTER_VELOCITY),
new DefaultKickerRunAtVelocity(
kickerSubsystem, () -> KickerConstants.Setpoints.LOAD_INTO_SHOOTER_VELOCITY),
new DefaultShooterRunAtVelocity(
shooterSubsystem, () -> ShooterConstants.Setpoints.SHOOTER_TEST_VELOCITY)));Option 2: Deadline Pattern (Most Robust)
Restructure to run the shooter continuously with the feeding phase as a deadline:
public IndexIntoShooterAndShoot(
BeltSubsystem beltSubsystem,
KickerSubsystem kickerSubsystem,
ShooterSubsystem shooterSubsystem) {
addCommands(
// Single shooter command runs for entire sequence
new DefaultShooterRunAtVelocity(
shooterSubsystem, () -> ShooterConstants.Setpoints.SHOOTER_TEST_VELOCITY)
.deadlineWith(
new SequentialCommandGroup(
// Wait for shooter to spin up
new WaitUntilCommand(shooterSubsystem::isAtTargetVelocity),
// Then feed
new ParallelCommandGroup(
new DefaultBeltRunAtVelocity(
beltSubsystem, () -> BeltConstants.Setpoints.LOAD_INTO_SHOOTER_VELOCITY),
new DefaultKickerRunAtVelocity(
kickerSubsystem, () -> KickerConstants.Setpoints.LOAD_INTO_SHOOTER_VELOCITY)))));
}Option 3: alongWith Pattern
Keep shooter running alongside the sequential stages:
public IndexIntoShooterAndShoot(
BeltSubsystem beltSubsystem,
KickerSubsystem kickerSubsystem,
ShooterSubsystem shooterSubsystem) {
addCommands(
new ParallelRaceGroup(
new DefaultShooterRunAtVelocity(
shooterSubsystem, () -> ShooterConstants.Setpoints.SHOOTER_TEST_VELOCITY),
new SequentialCommandGroup(
new WaitUntilCommand(shooterSubsystem::isAtTargetVelocity),
new ParallelCommandGroup(
new DefaultBeltRunAtVelocity(
beltSubsystem, () -> BeltConstants.Setpoints.LOAD_INTO_SHOOTER_VELOCITY),
new DefaultKickerRunAtVelocity(
kickerSubsystem, () -> KickerConstants.Setpoints.LOAD_INTO_SHOOTER_VELOCITY)))));
}Recommendation
I recommend Option 2 (Deadline Pattern) because:
- ✅ Single shooter command instance (cleaner)
- ✅ No custom classes needed
- ✅ Clear semantics: shooter runs for duration of feeding sequence
- ✅ Standard WPILib pattern
Option 1 is also good if you prefer explicit control over the stop behavior. Choose based on your team's preference!
🧠 Learnings used
Learnt from: Rkortlandt
Repo: 5152Alotobots/5152_Rebuilt PR: 5
File: src/main/java/frc/alotobots/rebuilt/subsystems/kicker/constants/KickerConstants.java:40-50
Timestamp: 2026-02-21T19:10:14.286Z
Learning: In ShooterConstants.java, the SHOOTER_TEST_VELOCITY constant is intentionally set to a low value (DegreesPerSecond.of(90)) for testing purposes and needs to be tuned to competition values before deployment.
Learnt from: Rkortlandt
Repo: 5152Alotobots/5152_Rebuilt PR: 5
File: src/main/java/frc/alotobots/rebuilt/subsystems/kicker/constants/KickerConstants.java:40-50
Timestamp: 2026-02-21T19:10:14.286Z
Learning: In BeltConstants.java, the MAX_SPEED and LOAD_INTO_SHOOTER_VELOCITY constants are intentionally set to low values (DegreesPerSecond.of(90)) for testing purposes and need to be tuned to competition values before deployment.
Learnt from: SeanErn
Repo: 5152Alotobots/5152_Rebuilt PR: 5
File: src/main/java/frc/alotobots/rebuilt/subsystems/launcher/deflector/DeflectorSubsystem.java:108-108
Timestamp: 2026-02-21T19:55:04.601Z
Learning: In AdvantageKit, Logger.recordOutput enforces type consistency per key. When a key is written with a specific type (for example an enum), subsequent writes to the same key with a different type (for example String) are silently skipped. Ensure a log key always uses a consistent type across its lifetime. During reviews, look for log keys that change the type they store and refactor to use either the same type or new, separate keys. When adding new log keys, document the expected type and consider adding unit tests to validate type consistency and avoid silent skips.
Rkortlandt
requested changes
Feb 24, 2026
| /** Contains threshold values for various wrist operations. */ | ||
| public static final class Thresholds { | ||
| /** Acceptable PID error that will classify as "at velocity" */ | ||
| public static final AngularVelocity AT_TARGET_VELOCITY_SPEED_THRESHOLD = DegreesPerSecond.of(3); |
Member
There was a problem hiding this comment.
Change or leave todo comment
| /** Contains threshold values for various wrist operations. */ | ||
| public static final class Thresholds { | ||
| /** Acceptable PID error that will classify as "at velocity" */ | ||
| public static final AngularVelocity AT_TARGET_VELOCITY_SPEED_THRESHOLD = DegreesPerSecond.of(3); |
| deflectorMotor.stopMotor(); | ||
| } | ||
|
|
||
| public void seedVortexInternalEncoder() { |
| /** Contains physical limits and safety thresholds for the turret. */ | ||
| public static final class Limits { | ||
| /** Maximum allowed angle */ | ||
| public static final Angle TURRET_MAX_ANGLE = Degrees.of(360); |
Member
There was a problem hiding this comment.
zero is turret strieight forward should be -180 to pos 180
I have nothing to do with the sim stuff. Not sure if it works.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
Refactor