feat(command): add an elevator goto command#53
Conversation
This should be a better solution instead of just moving up and down as soon as we hit the button. Instead, we set the desired stage and then we actually move to it. This should avoid queue issues along with other random stuff like that. GotoElevatorCommand is basically the same as CorrectElevatorCommand, so I might just get rid of that. I like the sag solution though, so I might base GotoElevatorCommand off stageDrift instead of just comparing desiredStage and currentStage, and all I need to do is copy CorrectElevatorCommand but add support for going down.
This should be a better solution instead of just moving up and down as soon as we hit the button. Instead, we set the desired stage and then we actually move to it. This should avoid some queue issues along with other random stuff like that.
# Conflicts: # src/main/kotlin/org/frc5183/commands/elevator/GotoElevatorCommand.kt
WalkthroughRefactors autonomous JSONs to simplify and rename command groups, updates several path control points and pathName literals, replaces CorrectElevatorCommand with a new GotoElevatorCommand (deprecated old), refactors elevator command logic and subsystem limit handling, adds config tests, and applies minor build and editorconfig tweaks. Changes
Sequence Diagram(s)sequenceDiagram
auton->>NamedCommands: request "Goto Elevator"
Note right of NamedCommands #DDEEFF: registry maps name → command
NamedCommands->>GotoElevatorCommand: instantiate(with ElevatorSubsystem)
GotoElevatorCommand->>ElevatorSubsystem: read currentStage, stageDrift
alt need to move up
GotoElevatorCommand->>ElevatorSubsystem: runElevator(+speed)
else need to move down
GotoElevatorCommand->>ElevatorSubsystem: runElevator(-speed)
end
ElevatorSubsystem->>ElevatorSubsystem: block movement if top/bottom limit switch active
GotoElevatorCommand->>GotoElevatorCommand: isFinished? (limit or within drift)
alt finished
GotoElevatorCommand->>ElevatorSubsystem: stopElevator()
GotoElevatorCommand-->>auton: done
else
GotoElevatorCommand-->>GotoElevatorCommand: continue loop
end
sequenceDiagram
auton->>PathPlanner: follow path (e.g., "Left Coral")
auton->>NamedCommands: "Raise Elevator" / "Goto Elevator"
auton->>NamedCommands: "Shoot Coral"
Note right of auton #EFEFEF: Flattened sequence replaces prior nested parallel/race groups
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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: 11
🧹 Nitpick comments (14)
src/main/kotlin/org/frc5183/Robot.kt (1)
187-188: Ensure simulation log matches new command nameEven though this only affects the println stub for non-real modes, keeping the label consistent with the new command avoids confusion when debugging simulations. Please update the message to "Goto Elevator".
Apply this diff:
- "Goto Elevator" to InstantCommand({ println("Correct Elevator") }), + "Goto Elevator" to InstantCommand({ println("Goto Elevator") }),src/main/kotlin/org/frc5183/commands/elevator/RaiseElevatorCommand.kt (1)
27-27: Consider InstantCommand for intent clarity.This behaves like an instant setpoint update. Consider making this an InstantCommand (with the boundary check) to reduce boilerplate.
deploy/pathplanner/autos/Left Full Coral.auto (3)
95-123: Verify repeated “Lower Elevator” sequence matches stage count.Four lowers then a goto assumes stage count/deltas. If fewer stages remain, you’ll no-op; if more, you won’t reach target. Confirm counts align with Config.ELEVATOR_STAGES.
Consider a single “Goto Elevator” before intake if your desiredStage is set appropriately upstream to avoid fragile repetition.
227-255: Repeated “Lower Elevator” blocks — intent check.Another 4x lower followed by goto. Please confirm intentional and that it fits auto time budget.
If the goal is “go to bottom,” a single command that sets desiredStage to 0 plus one “Goto Elevator” is more robust.
353-383: Top-level trailing lowers may be unintended leftover.Multiple “Lower Elevator” at the end (outside prior groups) may duplicate earlier lowers and extend runtime.
If unintentional, remove or consolidate to a single “Goto Elevator” to the desired final stage.
src/main/kotlin/org/frc5183/subsystems/elevator/ElevatorSubsystem.kt (3)
53-57: Helpful telemetry; consider logging after currentStage is updatedThe readable stage names and drift logging are great. Small ordering nit: currentStage is recomputed at Line 58, but logs at Lines 50–57 reflect the old value. Consider moving the stage computation before the logs so they reflect the latest state. Also, verify if you want drift signed (as logged) or absolute; the KDoc above says “absolute difference,” but the value here is signed.
73-77: Good safety check; add observability when commands are blocked by limit switchesEarly-returning on limit switches prevents damage. Add a one-line log so operators know a command was ignored.
Apply this diff:
fun runElevator(speed: Double) { - if (speedMovesUp(speed) && topLimitSwitch) return - if (speedMovesDown(speed) && bottomLimitSwitch) return + if (speedMovesUp(speed) && topLimitSwitch) { + Logger.recordOutput("Elevator/RunIgnored", "Top limit: up command blocked") + return + } + if (speedMovesDown(speed) && bottomLimitSwitch) { + Logger.recordOutput("Elevator/RunIgnored", "Bottom limit: down command blocked") + return + } io.runElevator(speed) }
116-129: De-duplicate stage names; source them from Config to avoid driftHardcoding names here can get out of sync with Config.ELEVATOR_STAGES size/order. Consider adding a Config.ELEVATOR_STAGE_NAMES list (same length and order) and returning names via getOrElse(stage) { "Unknown" }.
Based on learnings
src/test/kotlin/org/frc5183/constants/ConfigTest.kt (2)
19-22: Be intentional about freezing stage count at 5Hard-coding size == 5 prevents accidental regressions, but will require test updates when adding/removing stages. If that’s intended, keep as-is; otherwise, assert minimum expected count or co-validate against stage name list (if added).
24-27: Unit-aware drift check is goodPositive drift in explicit units is spot on. Consider also asserting drift < some reasonable upper bound to catch mis-configurations (optional).
src/main/kotlin/org/frc5183/commands/elevator/LowerElevatorCommand.kt (1)
10-25: Simplify: command always finishes; remove state and unused importfinished is set to true in all paths, so you can drop the field and make isFinished() return true. Also remove the now-unused Config import.
Apply this diff:
-import org.frc5183.constants.Config +import org.frc5183.constants.Config @@ - private var finished: Boolean = false @@ override fun initialize() { if (elevator.desiredStage <= 0) { - finished = true return } elevator.desiredStage -= 1 - - finished = true } - override fun isFinished(): Boolean = finished + override fun isFinished(): Boolean = truesrc/main/kotlin/org/frc5183/commands/elevator/GotoElevatorCommand.kt (1)
17-18: Remove unused fieldinvalid is never read.
Apply this diff:
- var invalid = falsedeploy/pathplanner/paths/Right Cower.path (1)
11-12: Control point tweaks: run preview for curvature/clearanceThe Bezier control point shifts look fine. Please re‑preview this path to ensure:
- No clearance issues around field elements with the new curvature
- Velocity/accel limits still hit the intended time windows
If desired, normalize excessive float precision in saved JSON to reduce churn in future diffs (optional).
Also applies to: 23-24
deploy/pathplanner/paths/Left Coral Return.path (1)
11-12: Path smoothing ok; normalize rotation precision (optional)Edits look good. Consider normalizing goalEndState.rotation from -59.99999999999999 to -60.0 for readability and cleaner diffs.
Also applies to: 23-24
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
deploy/pathplanner/autos/Left Coral.auto(1 hunks)deploy/pathplanner/autos/Left Cower.auto(1 hunks)deploy/pathplanner/autos/Left Full Coral.auto(6 hunks)deploy/pathplanner/autos/Middle Coral.auto(1 hunks)deploy/pathplanner/autos/Right Coral.auto(2 hunks)deploy/pathplanner/autos/Right Cower.auto(1 hunks)deploy/pathplanner/autos/Right Full Coral.auto(6 hunks)deploy/pathplanner/paths/Left Coral Return.path(2 hunks)deploy/pathplanner/paths/Left Coral.path(2 hunks)deploy/pathplanner/paths/Right Coral Intake 2.path(1 hunks)deploy/pathplanner/paths/Right Cower.path(2 hunks)deploy/pathplanner/settings.json(2 hunks)src/main/kotlin/org/frc5183/Robot.kt(3 hunks)src/main/kotlin/org/frc5183/commands/elevator/CorrectElevatorCommand.kt(3 hunks)src/main/kotlin/org/frc5183/commands/elevator/GotoElevatorCommand.kt(1 hunks)src/main/kotlin/org/frc5183/commands/elevator/HoldElevatorCommand.kt(1 hunks)src/main/kotlin/org/frc5183/commands/elevator/LowerElevatorCommand.kt(1 hunks)src/main/kotlin/org/frc5183/commands/elevator/RaiseElevatorCommand.kt(1 hunks)src/main/kotlin/org/frc5183/constants/Config.kt(1 hunks)src/main/kotlin/org/frc5183/constants/Controls.kt(1 hunks)src/main/kotlin/org/frc5183/constants/PhysicalConstants.kt(1 hunks)src/main/kotlin/org/frc5183/subsystems/elevator/ElevatorSubsystem.kt(3 hunks)src/main/kotlin/org/frc5183/subsystems/elevator/io/RealElevatorIO.kt(1 hunks)src/test/kotlin/org/frc5183/constants/ConfigTest.kt(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: Baconing
PR: frc5183/octavian#16
File: src/main/kotlin/org/frc5183/Robot.kt:137-137
Timestamp: 2025-03-06T16:08:19.728Z
Learning: Baconing prefers using mechanism-specific simulation classes (like ElevatorSim) for proper physics modeling rather than basic motor simulations.
🪛 GitHub Check: ktlint
src/main/kotlin/org/frc5183/commands/elevator/CorrectElevatorCommand.kt
[failure] 4-4:
Unused import
src/main/kotlin/org/frc5183/commands/elevator/GotoElevatorCommand.kt
[failure] 53-53:
File must end with a newline (\n)
[failure] 41-41:
Exceeded max line length (140)
[failure] 41-41:
Line is exceeding max line length. Break line after '||' in binary expression
[failure] 32-32:
Missing { ... }
[failure] 32-32:
Expected a newline
[failure] 31-31:
No comment expected at this location
[failure] 29-29:
Missing { ... }
[failure] 29-29:
Expected a newline
🔇 Additional comments (20)
src/main/kotlin/org/frc5183/constants/PhysicalConstants.kt (1)
11-11: Mass constant now uses SI unitsLine 11’s switch to
Units.Kilograms.of(35.0)keeps the mass value in SI, which is consistent with the rest of the constants module. Looks good.deploy/pathplanner/autos/Left Cower.auto (1)
10-10: Path rename aligns with updated auto namingLine 10’s rename to “Left Cower” matches the new auto naming scheme and keeps the JSON structure untouched. 👍
deploy/pathplanner/paths/Right Coral Intake 2.path (1)
12-12: Tiny control-point tweak is harmlessLine 12’s minute Y adjustment maintains path continuity without affecting downstream tooling. All good.
src/main/kotlin/org/frc5183/constants/Config.kt (1)
16-21: No hard-coded references to the removed fifth stage found. Verified across the codebase—no.get(4)or[4]usages remain.deploy/pathplanner/autos/Right Coral.auto (2)
8-12: Regression risk: path/elevator no longer run in parallel.This replaces a parallel start with a pure path step. Elevator will now raise only after the path completes, likely slowing cycle time.
Is this intentional? If not, re-wrap the path and elevator steps in a parallel block so they execute concurrently.
14-22: Double “Raise Elevator” may overshoot or hit bounds.Two sequential raises rely on command-level bounds to avoid invalid desiredStage. Given the RaiseElevatorCommand fix, confirm this still produces the intended stage.
If you intend N-stage jumps, consider a single command that increments by N to avoid repeated events and reduce fragility.
deploy/pathplanner/autos/Left Coral.auto (2)
46-63: Race block OK; order is inconsequential.No functional impact from ordering inside a race; looks fine.
40-44: LGTM: register “Goto Elevator”Add a NamedCommands.registerCommand("Goto Elevator", …) call in your robot initialization.
deploy/pathplanner/autos/Left Full Coral.auto (1)
62-62: LGTM: “Goto Elevator” name update.Consistent with new command.
src/main/kotlin/org/frc5183/commands/elevator/CorrectElevatorCommand.kt (1)
29-29: Use unit-safe absolute comparison
elevator.stageDrift.abs(Units.Degrees)may not exist; compare on the raw Double instead:- override fun isFinished(): Boolean = elevator.stageDrift.abs(Units.Degrees) < Config.ELEVATOR_MAX_ALLOWED_DRIFT.`in`(Units.Degrees) + override fun isFinished(): Boolean = + kotlin.math.abs(elevator.stageDrift.`in`(Units.Degrees)) < + Config.ELEVATOR_MAX_ALLOWED_DRIFT.`in`(Units.Degrees)If your WPILib version offers
Angle.abs(), you can use:kotlin.math.abs(elevator.stageDrift.abs().`in`(Units.Degrees)) < Config.ELEVATOR_MAX_ALLOWED_DRIFT.`in`(Units.Degrees)Confirm the chosen API is supported.
src/test/kotlin/org/frc5183/constants/ConfigTest.kt (2)
7-17: LGTM: ordering and non-empty checksThe ascending-order assertion with strict < and the non-empty check look good.
29-52: LGTM: speed and threshold rangesValidating motor speeds within [-1, 1] and sensor threshold bounds is appropriate.
src/main/kotlin/org/frc5183/commands/elevator/GotoElevatorCommand.kt (2)
14-26: Initialize desiredStage bounds: LGTMClamping desiredStage into valid bounds is correct and defensive.
35-37: Stop on end: LGTMStopping the motor on end (both normal and interrupted) is correct.
src/main/kotlin/org/frc5183/commands/elevator/HoldElevatorCommand.kt (1)
10-12: Clarifying HoldElevatorCommand behavior helps operatorsThanks for spelling out that this command keeps running until something else cancels it; that reminder should prevent confusion when composing commands.
deploy/pathplanner/paths/Left Coral.path (1)
11-12: Control point refinement acknowledgedChanges appear to slightly tighten the curve. Please re‑preview to ensure intake/placement alignment remains correct at the end waypoint.
Also applies to: 23-24
src/main/kotlin/org/frc5183/constants/Controls.kt (1)
151-156: Update trailing comment for GotoElevatorCommand
Replace in src/main/kotlin/org/frc5183/constants/Controls.kt:- ), // First correct the elevator, then shoot the coral while holding the elevator. + ), // First go to the desired elevator stage, then shoot the coral while holding the elevator.deploy/pathplanner/autos/Right Cower.auto (1)
10-10: Rename consistent; manually confirm path references
Ensure every"pathName"indeploy/pathplanner/autos/has a corresponding.pathfile indeploy/pathplanner/paths/.deploy/pathplanner/settings.json (1)
2-28: Dimensions and drivetrain parameters stay consistentWheel radius and chassis dimensions now line up with the 2" wheels and measured bumper-to-bumper footprint, so the planner’s kinematics will remain coherent with the physical robot. Looks good.
deploy/pathplanner/autos/Right Full Coral.auto (1)
95-388: Goto integration keeps elevator stages explicitReplacing the old corrective step with an explicit “Goto Elevator” after each raise/lower stack keeps the desired stage bookkeeping clear while retaining the timing of the original auto. No concerns from my side.
| fun speedMovesUp(speed: Double): Boolean = speed < 0 && Config.ELEVATOR_MOTOR_INVERTED || speed > 0 && !Config.ELEVATOR_MOTOR_INVERTED | ||
|
|
There was a problem hiding this comment.
Direction helpers are fine, but raise/lower assume inversion = true
speedMovesUp/Down read Config.ELEVATOR_MOTOR_INVERTED, but raiseElevator() always negates speed and lowerElevator() always passes positive. If inversion is ever false, raise/lower will run the wrong direction.
Suggested update outside this hunk (to make raise/lower inversion-aware):
// Replace these:
fun raiseElevator(speed: Double) = runElevator(-1.0 * speed)
fun lowerElevator(speed: Double) = runElevator(1.0 * speed)
// With inversion-aware versions:
fun raiseElevator(speed: Double) = runElevator(if (Config.ELEVATOR_MOTOR_INVERTED) -speed else speed)
fun lowerElevator(speed: Double) = runElevator(if (Config.ELEVATOR_MOTOR_INVERTED) speed else -speed)Optionally, derive speedMovesUp/Down from the same mapping to keep a single source of truth.
🤖 Prompt for AI Agents
In src/main/kotlin/org/frc5183/subsystems/elevator/ElevatorSubsystem.kt around
lines 114-115, raiseElevator/lowerElevator assume Config.ELEVATOR_MOTOR_INVERTED
== true while speedMovesUp/Down consult that flag, causing wrong directions when
inversion is false; update raiseElevator and lowerElevator to be inversion-aware
by using conditional mapping (use -speed when inverted for raising, otherwise
use +speed, and the opposite for lowering) so the sign passed to runElevator
respects Config.ELEVATOR_MOTOR_INVERTED, and optionally refactor
speedMovesUp/Down to compute direction from the same mapping to keep a single
source of truth.
There was a problem hiding this comment.
this could actually be an issue
lol
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/kotlin/org/frc5183/subsystems/elevator/ElevatorSubsystem.kt (1)
82-88: Fix direction: raise/lower ignore inversion flagCurrent implementations always negate/positivize speed, causing reversed motion when inversion is false. Make them inversion-aware.
- fun raiseElevator(speed: Double) = runElevator(-1.0 * speed) + fun raiseElevator(speed: Double) = + runElevator(if (Config.ELEVATOR_MOTOR_INVERTED) -speed else speed) - fun lowerElevator(speed: Double) = runElevator(1.0 * speed) + fun lowerElevator(speed: Double) = + runElevator(if (Config.ELEVATOR_MOTOR_INVERTED) speed else -speed)Optionally, derive speedMovesUp/Down from the same mapping to keep one source of truth.
🧹 Nitpick comments (5)
deploy/pathplanner/autos/Middle Coral.auto (2)
8-37: Consider deadline/race semantics and duplicate “Raise Elevator”
- If “Raise Elevator” is non-terminating or slow, a plain parallel can stall the step. Prefer a deadline/race variant with the path as the deadline so the group ends when the path finishes.
- “Raise Elevator” appears twice; verify this isn’t an accidental double increment.
72-93: Verify duplicate “Lower Elevator”Two consecutive “Lower Elevator” entries may unintentionally drop two stages. If the intent is a specific target, set the desired stage explicitly and use a single “Goto Elevator” for determinism.
src/main/kotlin/org/frc5183/commands/elevator/GotoElevatorCommand.kt (2)
15-17: Scope the dependency and drop unused stateMake elevator private and remove the unused ‘invalid’ flag.
-class GotoElevatorCommand( - val elevator: ElevatorSubsystem, -) : Command() { - var invalid = false +class GotoElevatorCommand( + private val elevator: ElevatorSubsystem, +) : Command() {
27-33: Fix comment wording; keep style ktlint-friendlyClarify “desired stage” in the comment on the else-if branch.
- } else if (elevator.stageDrift.lt(Units.Degrees.zero())) { // If we're above the current stage, move down + } else if (elevator.stageDrift.lt(Units.Degrees.zero())) { // If we're above the desired stage, move downsrc/main/kotlin/org/frc5183/subsystems/elevator/ElevatorSubsystem.kt (1)
114-115: Clarify boolean precedence or reuse mappingAdd parentheses for readability or reuse the same inversion-aware mapping as raise/lower to avoid drift in logic over time.
- fun speedMovesUp(speed: Double): Boolean = speed < 0 && Config.ELEVATOR_MOTOR_INVERTED || speed > 0 && !Config.ELEVATOR_MOTOR_INVERTED + fun speedMovesUp(speed: Double): Boolean = + (speed < 0 && Config.ELEVATOR_MOTOR_INVERTED) || + (speed > 0 && !Config.ELEVATOR_MOTOR_INVERTED)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
build.gradle.kts(1 hunks)deploy/pathplanner/autos/Middle Coral.auto(1 hunks)src/main/kotlin/org/frc5183/commands/elevator/GotoElevatorCommand.kt(1 hunks)src/main/kotlin/org/frc5183/constants/Config.kt(1 hunks)src/main/kotlin/org/frc5183/constants/Controls.kt(2 hunks)src/main/kotlin/org/frc5183/subsystems/elevator/ElevatorSubsystem.kt(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/kotlin/org/frc5183/constants/Controls.kt
- src/main/kotlin/org/frc5183/constants/Config.kt
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: Baconing
PR: frc5183/octavian#16
File: src/main/kotlin/org/frc5183/Robot.kt:137-137
Timestamp: 2025-03-06T16:08:19.728Z
Learning: Baconing prefers using mechanism-specific simulation classes (like ElevatorSim) for proper physics modeling rather than basic motor simulations.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (6)
build.gradle.kts (1)
129-136: Good: enforce tests before build/deploySolid safeguard to keep CI and deployments healthy. For field-quick deploys, remember you can still bypass with
-x testif needed.deploy/pathplanner/autos/Middle Coral.auto (1)
40-68: Nice: switched hold/shoot to a race groupThis avoids deadlock from the non-terminating hold command. Looks correct.
src/main/kotlin/org/frc5183/commands/elevator/GotoElevatorCommand.kt (1)
39-54: Simplify finish logic and align comment/conditionCheck limit switches first, then drift. Avoid mixed ORs with “and” comments.
- override fun isFinished(): Boolean { - // If we're either at or above the max stage, and the top limit switch is hit, we're done no matter what. - if ((elevator.desiredStage >= Config.ELEVATOR_STAGES.size || elevator.currentStage >= Config.ELEVATOR_STAGES.size) || - elevator.topLimitSwitch - ) { - return true - } - - // If we're at or below the min stage, and the bottom limit switch is hit, we're done no matter what. - if ((elevator.desiredStage <= 0 || elevator.currentStage <= 0) || elevator.bottomLimitSwitch) { - return true - } - - // Otherwise, we're done once we're within the allowed drift of the desired stage. - return elevator.stageDrift.abs(Units.Degrees) <= Config.ELEVATOR_MAX_ALLOWED_DRIFT.`in`(Units.Degrees) - } + override fun isFinished(): Boolean { + // Hard stop on either limit switch + if (elevator.topLimitSwitch || elevator.bottomLimitSwitch) return true + + // Done once we're within allowed drift of the desired stage + return elevator.stageDrift.abs(Units.Degrees) <= + Config.ELEVATOR_MAX_ALLOWED_DRIFT.`in`(Units.Degrees) + }src/main/kotlin/org/frc5183/subsystems/elevator/ElevatorSubsystem.kt (3)
53-57: Good diagnosticsReadable stage names and drift logging will help tune and debug.
73-77: Safety checks before commanding motor look goodEarly-returns on limit switches prevent unsafe motion.
116-130: LGTM: stage name mappingClear, user-friendly labels with an “Unknown” fallback.
Trip-kun
left a comment
Trip-kun
left a comment
There was a problem hiding this comment.
Looks mostly fine from what I can see. Code bloat and boilerplate is definitely getting out of hand though.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
src/main/kotlin/org/frc5183/commands/elevator/CorrectElevatorCommand.kt (1)
28-28: Align drift threshold semantics with GotoElevatorCommand (use <=).Current check uses strict < while GotoElevatorCommand uses <=. Aligning avoids edge-case oscillation and keeps behavior consistent.
- override fun isFinished(): Boolean = elevator.stageDrift.abs(Units.Degrees) < Config.ELEVATOR_MAX_ALLOWED_DRIFT.`in`(Units.Degrees) + override fun isFinished(): Boolean = elevator.stageDrift.abs(Units.Degrees) <= Config.ELEVATOR_MAX_ALLOWED_DRIFT.`in`(Units.Degrees)src/main/kotlin/org/frc5183/commands/elevator/LowerElevatorCommand.kt (2)
21-24: Clamp with coerceAtLeast to simplify boundary logic.You can avoid the early return and guarantee non-negative desiredStage in one line.
- if (elevator.desiredStage <= 0) { - finished = true - return - } - - elevator.desiredStage -= 1 - - finished = true + elevator.desiredStage = (elevator.desiredStage - 1).coerceAtLeast(0) + finished = true
6-13: Consider making this an InstantCommand.This command is instantaneous; InstantCommand reduces boilerplate (finished flag, isFinished) and is idiomatic for “set-and-done” actions.
Proposed replacement (outside selected lines):
class LowerElevatorCommand(val elevator: ElevatorSubsystem) : InstantCommand({ elevator.desiredStage = (elevator.desiredStage - 1).coerceAtLeast(0) }, elevator)src/main/kotlin/org/frc5183/commands/elevator/GotoElevatorCommand.kt (3)
17-18: Remove unused propertyinvalid.It’s never read and can trigger warnings.
- var invalid = false -
27-33: Add an else branch to clear output in deadband and fix comment wording.Prevent carrying previous motor output when within tolerance. Also correct “current stage” → “desired stage”.
override fun execute() { - if (elevator.stageDrift.gt(Units.Degrees.zero())) { // If we're below the desired stage, move up + if (elevator.stageDrift.gt(Units.Degrees.zero())) { // If we're below the desired stage, move up elevator.raiseElevator(Config.ELEVATOR_MOVEMENT_SPEED) - } else if (elevator.stageDrift.lt(Units.Degrees.zero())) { // If we're above the current stage, move down + } else if (elevator.stageDrift.lt(Units.Degrees.zero())) { // If we're above the desired stage, move down elevator.lowerElevator(Config.ELEVATOR_MOVEMENT_SPEED) + } else { + // Within deadband; ensure we don't keep prior output + elevator.stopElevator() } }
39-54: Clarify finish checks: short-circuit on limit switches, then bounds, then drift.Current comment says “and” but code uses “or”. Also prefer simple, short-circuit checks for readability and ktlint friendliness.
- override fun isFinished(): Boolean { - // If we're either at or above the max stage, and the top limit switch is hit, we're done no matter what. - if ((elevator.desiredStage >= Config.ELEVATOR_STAGES.lastIndex || elevator.currentStage >= Config.ELEVATOR_STAGES.lastIndex) || - elevator.topLimitSwitch - ) { - return true - } - - // If we're at or below the min stage, and the bottom limit switch is hit, we're done no matter what. - if ((elevator.desiredStage <= 0 || elevator.currentStage <= 0) || elevator.bottomLimitSwitch) { - return true - } - - // Otherwise, we're done once we're within the allowed drift of the desired stage. - return elevator.stageDrift.abs(Units.Degrees) <= Config.ELEVATOR_MAX_ALLOWED_DRIFT.`in`(Units.Degrees) - } + override fun isFinished(): Boolean { + // Hard-stop if either limit switch is hit + if (elevator.topLimitSwitch) return true + if (elevator.bottomLimitSwitch) return true + + // Defensive bounds checks (should be clamped on initialize) + val last = Config.ELEVATOR_STAGES.lastIndex + if (elevator.desiredStage !in 0..last) return true + if (elevator.currentStage !in 0..last) return true + + // Done once we're within allowed drift of the desired stage + return elevator.stageDrift.abs(Units.Degrees) <= + Config.ELEVATOR_MAX_ALLOWED_DRIFT.`in`(Units.Degrees) + }As per coding guidelines
src/test/kotlin/org/frc5183/constants/ConfigTest.kt (1)
19-23: Avoid brittle size check for ELEVATOR_STAGES.Hard-coding 5 will break if configuration legitimately changes. Either reference a dedicated constant or assert a minimum/expected range.
Options:
- Replace with a lower-bound check (non-empty already covered).
- Or introduce Config.EXPECTED_ELEVATOR_STAGE_COUNT and assert equality to that constant.
- fun `ELEVATOR_STAGES size is 5`() { - assertEquals(5, Config.ELEVATOR_STAGES.size) - } + fun `ELEVATOR_STAGES size is sane`() { + assertTrue(Config.ELEVATOR_STAGES.size >= 1) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.editorconfig(1 hunks)src/main/kotlin/org/frc5183/commands/elevator/CorrectElevatorCommand.kt(3 hunks)src/main/kotlin/org/frc5183/commands/elevator/GotoElevatorCommand.kt(1 hunks)src/main/kotlin/org/frc5183/commands/elevator/LowerElevatorCommand.kt(1 hunks)src/main/kotlin/org/frc5183/commands/elevator/RaiseElevatorCommand.kt(1 hunks)src/test/kotlin/org/frc5183/constants/ConfigTest.kt(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .editorconfig
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/kotlin/org/frc5183/commands/elevator/RaiseElevatorCommand.kt
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: Baconing
PR: frc5183/octavian#16
File: src/main/kotlin/org/frc5183/Robot.kt:137-137
Timestamp: 2025-03-06T16:08:19.728Z
Learning: Baconing prefers using mechanism-specific simulation classes (like ElevatorSim) for proper physics modeling rather than basic motor simulations.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (1)
src/main/kotlin/org/frc5183/commands/elevator/LowerElevatorCommand.kt (1)
15-24: This command only updates desiredStage and immediately finishes; ensure motion gets scheduled.Unless you chain or default-schedule GotoElevatorCommand, the elevator will never move after decrementing.
Example (in Controls.kt):
- LowerElevatorCommand(elevator).andThen(GotoElevatorCommand(elevator))
well that's kinda rude |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/kotlin/org/frc5183/commands/elevator/GotoElevatorCommand.kt (1)
17-17: Drop the unusedinvalidflag.The
invalidfield is never read or written after declaration, so it just adds noise. Please remove it (or wire it up if you meant to use it).- var invalid = false
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/kotlin/org/frc5183/commands/elevator/GotoElevatorCommand.kt(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: Baconing
PR: frc5183/octavian#16
File: src/main/kotlin/org/frc5183/Robot.kt:137-137
Timestamp: 2025-03-06T16:08:19.728Z
Learning: Baconing prefers using mechanism-specific simulation classes (like ElevatorSim) for proper physics modeling rather than basic motor simulations.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
| class GotoElevatorCommand( | ||
| val elevator: ElevatorSubsystem, | ||
| ) : Command() { |
There was a problem hiding this comment.
Fix Command inheritance — current declaration won't compile.
edu.wpi.first.wpilibj2.command.Command is an interface, so Kotlin can’t treat it like a constructor (Command()). This class won’t compile until it either implements the interface (: Command) or extends a concrete base such as CommandBase(). Please fix before merge.
-class GotoElevatorCommand(
- val elevator: ElevatorSubsystem,
-) : Command() {
+class GotoElevatorCommand(
+ val elevator: ElevatorSubsystem,
+) : 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.
| class GotoElevatorCommand( | |
| val elevator: ElevatorSubsystem, | |
| ) : Command() { | |
| class GotoElevatorCommand( | |
| val elevator: ElevatorSubsystem, | |
| ) : Command { |
🤖 Prompt for AI Agents
In src/main/kotlin/org/frc5183/commands/elevator/GotoElevatorCommand.kt around
lines 14 to 16, the class currently declares ": Command()" but Command is an
interface so this doesn't compile; update the declaration to either implement
the interface (": Command") and add the required interface method
implementations, or more simply extend the concrete base class ("CommandBase()")
and import it; prefer extending CommandBase() for typical WPILib command classes
and adjust imports accordingly so the file compiles.
2 participants
This should be a better solution instead of just moving up and down as soon as we hit the button.
Instead, we set the desired stage and then we actually move to it. This should avoid queue issues along with other random stuff like that.This should be a better solution instead of just moving up and down as soon as we hit the button.
Instead, we set the desired stage and then we actually move to it. This should avoid queue issues along with other random stuff like that.
GotoElevatorCommand is basically the same as CorrectElevatorCommand, so I might just get rid of that. I like the sag solution though, so I might base GotoElevatorCommand off stageDrift instead of just comparing desiredStage and currentStage, and all I need to do is copy CorrectElevatorCommand but add support for going down.
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Chores
Documentation
Tests