Conversation
📝 WalkthroughWalkthroughAdds a new Climb subsystem (real and simulated IO), dashboard-tunable controls and utilities, robot integration and controller bindings for climb states, configuration/constants updates, and minor build/simulator descriptor adjustments. Changes
Sequence DiagramsequenceDiagram
participant Pilot as Pilot (Controller)
participant Bindings as ButtonBindings
participant Climb as Climb Subsystem
participant Controller as ProfiledPIDController
participant IO as ClimbIO (Real/Sim)
participant Motor as Motor Hardware
Pilot->>Bindings: Press X/Y/A (setState CLIMB/MIDDLE/STOW)
Bindings->>Climb: setState(desiredState)
loop periodic
Climb->>Controller: compute PID+profile for target height
Controller-->>Climb: control output (voltage)
Climb->>IO: setVoltage(volts)
IO->>Motor: apply voltage / simulate step
Motor-->>IO: report position, velocity, current
IO-->>Climb: updateData()
Climb->>Climb: log telemetry & update visualization
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 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)
No actionable comments were generated in the recent review. 🎉 Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![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
🤖 Fix all issues with AI agents
In `@2026/src/main/java/frc/robot/config/ButtonBindings.java`:
- Around line 3-7: The file has duplicated import blocks (e.g., Logger,
MathUtil, Pose2d, Rotation2d, Robot, OnTheFly, TeleopCommand) — remove the
redundant set so each type is imported only once; locate the two identical
import groups (one at the top around the first imports and the other at lines
containing Logger/MathUtil/Pose2d/Rotation2d/Robot/OnTheFly/TeleopCommand) and
delete the duplicate block, leaving a single clean import section for the class
ButtonBindings.
- Around line 55-58: The three problems are that pilot.a() is bound multiple
times (causing STOW, STOP and the OnTheFly action to all fire) and those lines
use the static pilot field instead of the piCtl parameter; fix by replacing the
duplicate pilot.a() uses with distinct, unoccupied buttons on the piCtl
parameter (e.g., use piCtl.b()/piCtl.x()/piCtl.y() as appropriate), ensure each
climb action maps to a single unique button
(Bind.button(piCtl.<button>()).onTrue(() ->
Robot.climbLeft.setState(ClimbStates.STOW/STOP/CLIMB))), and remove or reassign
any binding that conflicts with the existing OnTheFly binding so no button is
triple-bound; keep references to Bind.button, Robot.climbLeft.setState,
ClimbStates, pilot (static) and piCtl (parameter) in mind when making the
change.
In `@2026/src/main/java/frc/robot/config/ClimbConfig.java`:
- Around line 17-18: The comment in ClimbConfig about a "22 TOOTH, 1/4 in pitch"
sprocket (circumference ≈ 5.5 in, radius ≈ 0.875 in) conflicts with the constant
DRUM_RADIUS which is set to Inches.of(4); verify the actual hardware measurement
and update either the comment or the DRUM_RADIUS value to match the real
drum/sprocket radius (adjust Inches.of(...) accordingly) and ensure the comment
describes the calculation used; reference DRUM_RADIUS in class ClimbConfig when
making the change.
- Line 15: The GEARING constant in ClimbConfig.java is documented with an
incorrect breakdown; update the comment next to public static final double
GEARING = 45 to show the actual gearing stages that multiply to 45 (or remove
the incorrect "16:1 ... 16:1" text) — e.g., replace the current comment with the
correct stage breakdown (something like "stageA:1 followed by stageB:1 (stageA *
stageB = 45)") so the comment accurately reflects how GEARING is derived.
- Around line 60-61: Update the Javadoc in ClimbConfig (the comment above the
states/enum declaration) to replace the copy-pasted reference to "elevator" with
"climb" (e.g., "states for the different possible heights the climb subsystem
needs to reach" or similar), ensuring the description accurately refers to the
climb subsystem and uses proper capitalization/grammar; locate the comment near
the states enum in class ClimbConfig and edit that Javadoc accordingly.
In `@2026/src/main/java/frc/robot/config/RobotConfig.java`:
- Around line 60-61: CLIMB_LEFT_ID and CLIMB_RIGHT_ID are both set to 0 which
creates a CAN ID collision and uses a reserved ID; update the constants
CLIMB_LEFT_ID and CLIMB_RIGHT_ID in RobotConfig (the public static final int
fields) to two unique, non-zero CAN IDs that don’t collide with other IDs
defined in the same file (pick unused integers consistent with your team’s
wiring plan) so each climb motor has its own distinct CAN ID.
In `@2026/src/main/java/frc/robot/Robot.java`:
- Around line 26-29: The import frc.robot.subsystems.Climb.Climb is duplicated
in the import block (referenced as Climb); remove the redundant import so Climb
is imported only once and leave the other imports (ExampleElevator, Swerve)
intact; ensure no other code changes are made and that any references to the
Climb class still resolve after removing the duplicate import.
In `@2026/src/main/java/frc/robot/subsystems/Climb/Climb.java`:
- Line 182: The logged height goal is using a magic offset (0.076207) so
Logger.recordOutput("Climb/HeightGoal", state.height.in(Meters) + 0.076207)
reports a misleading value; change the Logger.recordOutput call in Climb.java to
log the actual target without the offset (e.g.,
Logger.recordOutput("Climb/HeightGoal", state.height.in(Meters))) and remove the
magic constant so the logged value matches the real control target (keep
references to state.height and Meters intact).
- Around line 94-103: The code unconditionally overwrites climbIO with a
ClimbSimulation instance, bypassing the RobotBase.isReal() branch; remove the
stale assignment "climbIO = new ClimbSimulation(data)" so that climbIO is only
set to ClimbReal when RobotBase.isReal() is true and to ClimbSimulation
otherwise (leave the existing if/else that constructs ClimbReal or
ClimbSimulation and keep the subsequent profile.reset(data.height.in(Meters))
call).
- Around line 55-56: The LoggedNetworkNumber path for maxAcceleration in
Climb.java is missing the "/Tuning/" prefix and should match maxVelocity's
subtree; update the LoggedNetworkNumber instantiation for maxAcceleration to use
the path "/Tuning/Climb/max acceleration" so both maxVelocity and
maxAcceleration live under the same "/Tuning/Climb" NetworkTables subtree
(adjust the LoggedNetworkNumber(...) call for the maxAcceleration field
accordingly).
In `@2026/src/main/java/frc/robot/subsystems/Climb/ClimbIO.java`:
- Line 13: Remove the unused import ExampleElevatorConfig from ClimbIO.java;
locate the import line "import frc.robot.config.ExampleElevatorConfig;" at the
top of the ClimbIO class and delete it so only actually referenced imports
remain (no other code changes required).
- Line 5: The import javax.xml.crypto.Data in ClimbIO.java is unused and
unrelated to the climb subsystem; remove that import statement from the top of
the file so the file only contains necessary imports (locate the import line in
ClimbIO.java and delete it), then rebuild to ensure no missing references.
In `@2026/src/main/java/frc/robot/subsystems/Climb/real/ClimbReal.java`:
- Around line 50-55: The follower configuration for rightMotor currently calls
rightMotor.follow(leftMotor) then rightMotor.setInverted(...); change this to
use the two-parameter follower overload so inversion is applied as part of
follower setup: call rightMotor.follow(leftMotor,
ClimbSpecs.RIGHT_MOTOR_INVERTED) (from OptixSpark) and remove the separate
setInverted call; locate this in ClimbReal where rightMotor is created via
OptixSpark.ofSparkMax and configured after leftMotor.
In `@2026/src/main/java/frc/robot/utils/LoggedTunableNumber.java`:
- Around line 121-126: The current ifChanged method uses
Arrays.stream(...).anyMatch(tunableNumber -> tunableNumber.hasChanged(id)) which
short-circuits and prevents hasChanged(id) being called on remaining
LoggedTunableNumber instances; change the logic to eagerly call hasChanged(id)
for every tunable to update their per-id last-seen state, record whether any
returned true, and only then, if any changed, call action.accept(...) with an
array produced from LoggedTunableNumber::get; i.e., iterate over the
tunableNumbers, call hasChanged(id) on each (collecting a boolean OR), then if
that OR is true build the double[] via mapToDouble(LoggedTunableNumber::get) and
pass it to action.accept.
- Around line 84-86: In LoggedTunableNumber, protect the set(double) method from
NPE by checking the dashboardNumber field (same as get() does) before calling
dashboardNumber.set(value); if dashboardNumber is non-null call set, otherwise
either call initDefault() or throw an IllegalStateException to surface
misuse—update the set(double) implementation in the LoggedTunableNumber class to
perform that null check and handle the uninitialized state consistently with
get() (or explicitly throw IllegalStateException) so callers constructing with
the single-arg constructor can't trigger a NullPointerException.
In `@2026/src/main/java/frc/robot/utils/MiscConstants.java`:
- Around line 12-13: The currentMode calculation uses RobotBase.isReal() and the
hardcoded simMode, making Mode.REPLAY unreachable; update currentMode (and/or
simMode) to respect a launch-time override (e.g., a system property or
environment variable) so REPLAY can be selected without editing code: read a
property like "robot.mode" (or env var) early in MiscConstants and if set to
"REPLAY" (or "SIM"/"REAL") set simMode/currentMode accordingly, otherwise fall
back to the existing RobotBase.isReal() logic; ensure you reference and update
the symbols simMode, currentMode, and use Mode.REPLAY when applying the
override.
- Around line 6-7: The class Javadoc in MiscConstants contains a typo
("commadn")—update the comment for the MiscConstants class to read "command"
instead of "commadn" so the header accurately documents that these are constants
not specific to any given subsystem or command; ensure any related Javadoc
blocks in MiscConstants.java are corrected for spelling consistency.
- Around line 36-43: The Javadoc for the slots array in MiscConstants is
off-by-one and contains typos; update the comment to match the actual array
indices (ClosedLoopSlot.kSlot0..kSlot3) and fix spelling ("position",
"velocity"). Specifically, edit the comment above the public static final
ClosedLoopSlot[] slots to list Slot0..Slot3 with correct descriptions (e.g.,
Slot0: small position error, Slot1: large position error, Slot2: slow velocity,
Slot3: fast velocity) so the documentation aligns with the
ClosedLoopSlot.kSlot0..kSlot3 entries.
🧹 Nitpick comments (8)
2026/simgui-ds.json (1)
1-101: Consider addingsimgui-ds.jsonto.gitignore.This file is auto-generated by the WPILib simulation GUI and reflects local developer preferences (e.g., joystick GUID, window visibility). Committing it can cause unnecessary merge conflicts when different developers run the simulator with different hardware setups. Typically,
simgui.jsonandsimgui-ds.jsonare gitignored in FRC projects.2026/src/main/java/frc/robot/utils/LoggedTunableNumber.java (1)
15-15: Unused import ofMiscConstantsmasked by@SuppressWarnings("unused").
MiscConstantsis imported but never referenced. The class-level@SuppressWarnings("unused")hides this. Was this intended for a tuning-mode guard (e.g., only publishing to the dashboard whencurrentMode != REAL)? If not, remove the import and the suppression.Also applies to: 23-23
2026/src/main/java/frc/robot/subsystems/Climb/real/ClimbReal.java (1)
27-27:prevVelocityshould beprivate.All other instance fields are
private, butprevVelocityispublic. This is an internal implementation detail and should not be part of the public API.Proposed fix
- public LinearVelocity prevVelocity = MetersPerSecond.of(0); + private LinearVelocity prevVelocity = MetersPerSecond.of(0);2026/src/main/java/frc/robot/subsystems/Climb/sim/ClimbSimulation.java (1)
73-75: Consider computing acceleration from velocity delta, asClimbRealdoes.
ClimbReal.updateData()derives acceleration from consecutive velocity samples. Hardcoding0here creates an inconsistency between sim and real telemetry, which could be misleading when analyzing logs or if acceleration is ever consumed by control logic.2026/src/main/java/frc/robot/config/ClimbConfig.java (1)
71-71: Makeheightfieldfinalin the enum.Enum fields that are set once in the constructor should be
finalto prevent accidental mutation.Proposed fix
- public Distance height; + public final Distance height;2026/src/main/java/frc/robot/subsystems/Climb/Climb.java (3)
85-86:heightMetersandsetHeightappear to be dead code.
heightMetersis set viasetHeight()but is never read by any control logic (moveToGoalusesstate.height, notheightMeters). If this is intended for external consumers, consider documenting that; otherwise remove it.Also applies to: 130-133
163-177: Redundant goal-setting inmoveToGoal.
profile.setGoal(...)on line 164 and the goal parameter inprofile.calculate(measurement, goal)on line 167 both set the goal. Thecalculate(measurement, goal)overload internally sets the goal, making line 164 redundant.Proposed fix — remove the redundant setGoal call
private void moveToGoal() { - profile.setGoal(state.height.in(Meters)); - // Calculate PID feedback based on current height double pidOutput = profile.calculate(data.height.in(Meters), state.height.in(Meters));
19-21: Clean up duplicate/commented-out imports.Line 19 imports
ClimbReal, line 20 has it commented out again, andCommand(line 14) is imported but unused.Proposed fix
import frc.robot.subsystems.Climb.real.ClimbReal; -// import frc.robot.subsystems.Climb.real.ClimbReal; // Added Import import frc.robot.subsystems.Climb.sim.ClimbSimulation; // Added Import
| import org.littletonrobotics.junction.Logger; | ||
|
|
||
| import edu.wpi.first.math.MathUtil; | ||
| import edu.wpi.first.math.geometry.Pose2d; | ||
| import edu.wpi.first.math.geometry.Rotation2d; |
There was a problem hiding this comment.
Duplicate import blocks.
Lines 3–7 and 24–31 contain duplicate imports (Logger, MathUtil, Pose2d, Rotation2d, Robot, OnTheFly, TeleopCommand). Remove one set.
Also applies to: 24-31
🤖 Prompt for AI Agents
In `@2026/src/main/java/frc/robot/config/ButtonBindings.java` around lines 3 - 7,
The file has duplicated import blocks (e.g., Logger, MathUtil, Pose2d,
Rotation2d, Robot, OnTheFly, TeleopCommand) — remove the redundant set so each
type is imported only once; locate the two identical import groups (one at the
top around the first imports and the other at lines containing
Logger/MathUtil/Pose2d/Rotation2d/Robot/OnTheFly/TeleopCommand) and delete the
duplicate block, leaving a single clean import section for the class
ButtonBindings.
|
|
||
| public class ClimbConfig { | ||
| public static class ClimbSpecs { | ||
| public static final double GEARING = 45; // 16:1 stage followed by another 16:1 stage |
There was a problem hiding this comment.
Gearing comment is incorrect.
The comment says "16:1 stage followed by another 16:1 stage" which would yield 256:1, not 45. Update the comment to reflect the actual gearing breakdown.
🤖 Prompt for AI Agents
In `@2026/src/main/java/frc/robot/config/ClimbConfig.java` at line 15, The GEARING
constant in ClimbConfig.java is documented with an incorrect breakdown; update
the comment next to public static final double GEARING = 45 to show the actual
gearing stages that multiply to 45 (or remove the incorrect "16:1 ... 16:1"
text) — e.g., replace the current comment with the correct stage breakdown
(something like "stageA:1 followed by stageB:1 (stageA * stageB = 45)") so the
comment accurately reflects how GEARING is derived.
| public void set(double value) { | ||
| dashboardNumber.set(value); | ||
| } |
There was a problem hiding this comment.
set() will throw NullPointerException if called before initDefault().
get() (line 72) guards against an uninitialized dashboardNumber, but set() does not. If anyone constructs via the single-arg constructor and calls set() before initDefault(), this crashes.
Proposed fix
public void set(double value) {
+ if (!hasDefault) {
+ return;
+ }
dashboardNumber.set(value);
}Alternatively, throw an IllegalStateException to surface the misuse early rather than silently ignoring it.
🤖 Prompt for AI Agents
In `@2026/src/main/java/frc/robot/utils/LoggedTunableNumber.java` around lines 84
- 86, In LoggedTunableNumber, protect the set(double) method from NPE by
checking the dashboardNumber field (same as get() does) before calling
dashboardNumber.set(value); if dashboardNumber is non-null call set, otherwise
either call initDefault() or throw an IllegalStateException to surface
misuse—update the set(double) implementation in the LoggedTunableNumber class to
perform that null check and handle the uninitialized state consistently with
get() (or explicitly throw IllegalStateException) so callers constructing with
the single-arg constructor can't trigger a NullPointerException.
| public static void ifChanged( | ||
| int id, Consumer<double[]> action, LoggedTunableNumber... tunableNumbers) { | ||
| if (Arrays.stream(tunableNumbers).anyMatch(tunableNumber -> tunableNumber.hasChanged(id))) { | ||
| action.accept(Arrays.stream(tunableNumbers).mapToDouble(LoggedTunableNumber::get).toArray()); | ||
| } | ||
| } |
There was a problem hiding this comment.
anyMatch short-circuits, skipping hasChanged updates on remaining tunables.
hasChanged(id) is side-effecting (it updates the per-id last-seen value). Stream.anyMatch short-circuits on the first true, so the remaining tunables never get their last-seen values updated. On the next call those tunables will still appear "changed," causing the action to fire spuriously every cycle until all tunables happen to be evaluated.
Evaluate all tunables eagerly before deciding:
Proposed fix
public static void ifChanged(
int id, Consumer<double[]> action, LoggedTunableNumber... tunableNumbers) {
- if (Arrays.stream(tunableNumbers).anyMatch(tunableNumber -> tunableNumber.hasChanged(id))) {
+ boolean changed = false;
+ for (LoggedTunableNumber tunableNumber : tunableNumbers) {
+ changed |= tunableNumber.hasChanged(id);
+ }
+ if (changed) {
action.accept(Arrays.stream(tunableNumbers).mapToDouble(LoggedTunableNumber::get).toArray());
}
}🤖 Prompt for AI Agents
In `@2026/src/main/java/frc/robot/utils/LoggedTunableNumber.java` around lines 121
- 126, The current ifChanged method uses
Arrays.stream(...).anyMatch(tunableNumber -> tunableNumber.hasChanged(id)) which
short-circuits and prevents hasChanged(id) being called on remaining
LoggedTunableNumber instances; change the logic to eagerly call hasChanged(id)
for every tunable to update their per-id last-seen state, record whether any
returned true, and only then, if any changed, call action.accept(...) with an
array produced from LoggedTunableNumber::get; i.e., iterate over the
tunableNumbers, call hasChanged(id) on each (collecting a boolean OR), then if
that OR is true build the double[] via mapToDouble(LoggedTunableNumber::get) and
pass it to action.accept.
| /** | ||
| * Constants not specific to any given subsystem or commadn |
There was a problem hiding this comment.
Typo in Javadoc: "commadn" → "command".
Proposed fix
-/**
- * Constants not specific to any given subsystem or commadn
+/**
+ * Constants not specific to any given subsystem or command📝 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.
| /** | |
| * Constants not specific to any given subsystem or commadn | |
| /** | |
| * Constants not specific to any given subsystem or command |
🤖 Prompt for AI Agents
In `@2026/src/main/java/frc/robot/utils/MiscConstants.java` around lines 6 - 7,
The class Javadoc in MiscConstants contains a typo ("commadn")—update the
comment for the MiscConstants class to read "command" instead of "commadn" so
the header accurately documents that these are constants not specific to any
given subsystem or command; ensure any related Javadoc blocks in
MiscConstants.java are corrected for spelling consistency.
| public static final Mode simMode = Mode.SIM; | ||
| public static final Mode currentMode = RobotBase.isReal() ? Mode.REAL : simMode; |
There was a problem hiding this comment.
REPLAY mode is unreachable with current logic.
currentMode resolves to either REAL or SIM—there is no code path that sets it to REPLAY. If log replay is intended to be supported, consider documenting that simMode must be manually changed to Mode.REPLAY before running, or add a system-property / environment-variable override to select it at launch.
🤖 Prompt for AI Agents
In `@2026/src/main/java/frc/robot/utils/MiscConstants.java` around lines 12 - 13,
The currentMode calculation uses RobotBase.isReal() and the hardcoded simMode,
making Mode.REPLAY unreachable; update currentMode (and/or simMode) to respect a
launch-time override (e.g., a system property or environment variable) so REPLAY
can be selected without editing code: read a property like "robot.mode" (or env
var) early in MiscConstants and if set to "REPLAY" (or "SIM"/"REAL") set
simMode/currentMode accordingly, otherwise fall back to the existing
RobotBase.isReal() logic; ensure you reference and update the symbols simMode,
currentMode, and use Mode.REPLAY when applying the override.
| /** | ||
| * Slot1: small position error | ||
| * Slot2: large posiotion error | ||
| * Slot3: slow velocity | ||
| * Slot4: fast velcity | ||
| */ | ||
| public static final ClosedLoopSlot[] slots = new ClosedLoopSlot[] { ClosedLoopSlot.kSlot0, ClosedLoopSlot.kSlot1, | ||
| ClosedLoopSlot.kSlot2, ClosedLoopSlot.kSlot3 }; |
There was a problem hiding this comment.
Slot comment numbering is off-by-one, plus typos.
The comments reference Slot1–Slot4, but the array uses kSlot0–kSlot3. Also: "posiotion" → "position", "velcity" → "velocity".
Proposed fix
/**
- * Slot1: small position error
- * Slot2: large posiotion error
- * Slot3: slow velocity
- * Slot4: fast velcity
+ * Slot0: small position error
+ * Slot1: large position error
+ * Slot2: slow velocity
+ * Slot3: fast velocity
*/🤖 Prompt for AI Agents
In `@2026/src/main/java/frc/robot/utils/MiscConstants.java` around lines 36 - 43,
The Javadoc for the slots array in MiscConstants is off-by-one and contains
typos; update the comment to match the actual array indices
(ClosedLoopSlot.kSlot0..kSlot3) and fix spelling ("position", "velocity").
Specifically, edit the comment above the public static final ClosedLoopSlot[]
slots to list Slot0..Slot3 with correct descriptions (e.g., Slot0: small
position error, Slot1: large position error, Slot2: slow velocity, Slot3: fast
velocity) so the documentation aligns with the ClosedLoopSlot.kSlot0..kSlot3
entries.
3 participants
Added climb subsystem with set state function in button bindings
created left climb arm in robot.java, need to add in the right one later
Summary by CodeRabbit
New Features
Chores