ArmElevator #21
ArmElevator #21
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughReplaces separate Arm and Elevator subsystems with a unified ArmElevator and adds a new EndEffector subsystem. Updates RobotContainer, command factories, ReefChooser, and SimulationFieldHandler to use ArmElevator/EndEffector APIs; removes legacy Arm/Elevator classes, commands, and constants. Changes
Sequence Diagram(s)sequenceDiagram
auton->>ArmElevatorCommands: getSetTargetStateCommand(REEF_CHOOSER::getArmElevatorState, shouldReverse)
activate ArmElevatorCommands
ArmElevatorCommands->>ArmElevator: getPrepareForStateCommand(...)
activate ArmElevator
ArmElevator->>ArmElevator: move arm angle & elevator position (constraints)
ArmElevator-->>ArmElevatorCommands: atPreparedTargetState
ArmElevatorCommands->>ArmElevator: setTargetState(targetState)
deactivate ArmElevator
deactivate ArmElevatorCommands
par Parallel scoring
auton->>EndEffectorCommands: getSetTargetStateCommand(SCORE_CORAL)
EndEffectorCommands->>EndEffector: setTargetState(SCORE_CORAL)
and
auton->>ArmElevator: maintain at target
end
sequenceDiagram
driver->>CoralCollectionCommands: getLoadCoralCommand()
activate CoralCollectionCommands
CoralCollectionCommands->>ArmElevatorCommands: set target LOAD_CORAL
CoralCollectionCommands->>EndEffectorCommands: set target LOAD_CORAL
Note over EndEffector: wait until END_EFFECTOR.hasGamePiece()
EndEffector-->>CoralCollectionCommands: hasGamePiece true
CoralCollectionCommands->>EndEffector: stop
CoralCollectionCommands->>ArmElevator: hold target
deactivate CoralCollectionCommands
sequenceDiagram
driver->>CoralEjectionCommands: getCoralEjectionCommand()
activate CoralEjectionCommands
CoralEjectionCommands->>ArmElevatorCommands: prepare angle/position for ejection
ArmElevator->>CoralEjectionCommands: atTargetState
CoralEjectionCommands->>EndEffectorCommands: set target EJECT
EndEffector->>SimulationFieldHandler: update ejection velocity/state
deactivate CoralEjectionCommands
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
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: 2
🧹 Nitpick comments (7)
src/main/java/frc/trigon/robot/subsystems/arm/Arm.java (1)
127-145: Reduce duplicated routing between setTargetState/setPrepareState/setArmStateThese methods share near-identical branching for reversed/non‑reversed angles. Consider consolidating to a single private helper to reduce duplication and drift.
src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java (2)
176-199: Remove stale end‑effector parameter from ArmStateArmState still carries a third constructor parameter that’s unused after the end effector split. Dropping it simplifies the API and avoids confusion.
Apply this diff:
- public enum ArmState { - REST(Rotation2d.fromDegrees(0), Rotation2d.fromDegrees(0), 0), - REST_WITH_CORAL(Rotation2d.fromDegrees(180), Rotation2d.fromDegrees(180), 0), - REST_FOR_CLIMB(Rotation2d.fromDegrees(0), Rotation2d.fromDegrees(0), 0), - LOAD_CORAL(Rotation2d.fromDegrees(0), Rotation2d.fromDegrees(0), -4), - HOLD_ALGAE(Rotation2d.fromDegrees(90), Rotation2d.fromDegrees(90), -4), - EJECT(Rotation2d.fromDegrees(60), Rotation2d.fromDegrees(60), 4), - SCORE_L1(Rotation2d.fromDegrees(110), Rotation2d.fromDegrees(110), 4), - SCORE_L2(Rotation2d.fromDegrees(90), Rotation2d.fromDegrees(100), 4), - SCORE_L3(Rotation2d.fromDegrees(90), Rotation2d.fromDegrees(100), 4), - SCORE_L4(Rotation2d.fromDegrees(100), Rotation2d.fromDegrees(120), 4), - SCORE_NET(Rotation2d.fromDegrees(160), Rotation2d.fromDegrees(160), 4), - SCORE_PROCESSOR(Rotation2d.fromDegrees(90), Rotation2d.fromDegrees(90), 4), - COLLECT_ALGAE_L2(Rotation2d.fromDegrees(90), Rotation2d.fromDegrees(90), -4), - COLLECT_ALGAE_L3(Rotation2d.fromDegrees(90), Rotation2d.fromDegrees(90), -4); + public enum ArmState { + REST(Rotation2d.fromDegrees(0), Rotation2d.fromDegrees(0)), + REST_WITH_CORAL(Rotation2d.fromDegrees(180), Rotation2d.fromDegrees(180)), + REST_FOR_CLIMB(Rotation2d.fromDegrees(0), Rotation2d.fromDegrees(0)), + LOAD_CORAL(Rotation2d.fromDegrees(0), Rotation2d.fromDegrees(0)), + HOLD_ALGAE(Rotation2d.fromDegrees(90), Rotation2d.fromDegrees(90)), + EJECT(Rotation2d.fromDegrees(60), Rotation2d.fromDegrees(60)), + SCORE_L1(Rotation2d.fromDegrees(110), Rotation2d.fromDegrees(110)), + SCORE_L2(Rotation2d.fromDegrees(90), Rotation2d.fromDegrees(100)), + SCORE_L3(Rotation2d.fromDegrees(90), Rotation2d.fromDegrees(100)), + SCORE_L4(Rotation2d.fromDegrees(100), Rotation2d.fromDegrees(120)), + SCORE_NET(Rotation2d.fromDegrees(160), Rotation2d.fromDegrees(160)), + SCORE_PROCESSOR(Rotation2d.fromDegrees(90), Rotation2d.fromDegrees(90)), + COLLECT_ALGAE_L2(Rotation2d.fromDegrees(90), Rotation2d.fromDegrees(90)), + COLLECT_ALGAE_L3(Rotation2d.fromDegrees(90), Rotation2d.fromDegrees(90)); @@ - ArmState(Rotation2d targetAngle, Rotation2d prepareAngle, double targetEndEffectorVoltage) { + ArmState(Rotation2d targetAngle, Rotation2d prepareAngle) { this.targetAngle = targetAngle; this.prepareAngle = prepareAngle; }
3-14: Prune unused imports carried over from the end‑effector eraBooleanEvent and CommandScheduler aren’t used in ArmConstants anymore.
-import edu.wpi.first.wpilibj.event.BooleanEvent; @@ -import edu.wpi.first.wpilibj2.command.CommandScheduler;src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java (1)
28-29: Avoid scheduling from inside a sequence unless necessarySpawning getLoadCoralCommand() via InstantCommand adds concurrency. If not intentional, inline the command into the sequence.
- new InstantCommand( - () -> getLoadCoralCommand().schedule() - ) + getLoadCoralCommand()src/main/java/frc/trigon/robot/commands/commandfactories/CoralEjectionCommands.java (1)
33-37: Add an end condition/timeout for the ejection stageThe StartEndCommand will run indefinitely; add a sensor-based end or a timeout to avoid spinning forever on sensor faults.
private static Command getEjectCoralFromEndEffectorCommand() { return new SequentialCommandGroup( ArmCommands.getSetTargetStateCommand(ArmConstants.ArmState.EJECT).until(RobotContainer.ARM::atTargetAngle), - EndEffectorCommands.getSetTargetStateCommand(EndEffectorConstants.EndEffectorState.EJECT) + EndEffectorCommands.getSetTargetStateCommand(EndEffectorConstants.EndEffectorState.EJECT) + .until(() -> !RobotContainer.END_EFFECTOR.hasGamePiece()) + .withTimeout(2.0) ); }src/main/java/frc/trigon/robot/subsystems/endEffector/EndEffector.java (1)
53-55: Consider making voltage threshold configurableThe hardcoded threshold of 2V for ejection detection should be a named constant for better maintainability.
+ private static final double EJECTION_VOLTAGE_THRESHOLD = 2.0; + public boolean isEjecting() { - return endEffectorMotor.getSignal(TalonFXSignal.MOTOR_VOLTAGE) > 2; + return endEffectorMotor.getSignal(TalonFXSignal.MOTOR_VOLTAGE) > EJECTION_VOLTAGE_THRESHOLD; }src/main/java/frc/trigon/robot/subsystems/endEffector/EndEffectorConstants.java (1)
75-79: Consider if 100Hz is necessary for all signalsRegistering all signals at 100Hz may be unnecessary and could impact CAN bus utilization. Consider if some signals like POSITION could use lower rates.
- END_EFFECTOR_MOTOR.registerSignal(TalonFXSignal.POSITION, 100); + END_EFFECTOR_MOTOR.registerSignal(TalonFXSignal.POSITION, 50); END_EFFECTOR_MOTOR.registerSignal(TalonFXSignal.VELOCITY, 100); END_EFFECTOR_MOTOR.registerSignal(TalonFXSignal.MOTOR_VOLTAGE, 100); - END_EFFECTOR_MOTOR.registerSignal(TalonFXSignal.CLOSED_LOOP_REFERENCE, 100); + END_EFFECTOR_MOTOR.registerSignal(TalonFXSignal.CLOSED_LOOP_REFERENCE, 50); END_EFFECTOR_MOTOR.registerSignal(TalonFXSignal.STATOR_CURRENT, 100);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/main/java/frc/trigon/robot/RobotContainer.java(3 hunks)src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java(2 hunks)src/main/java/frc/trigon/robot/commands/commandfactories/CoralEjectionCommands.java(3 hunks)src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java(3 hunks)src/main/java/frc/trigon/robot/subsystems/arm/Arm.java(2 hunks)src/main/java/frc/trigon/robot/subsystems/arm/ArmCommands.java(3 hunks)src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java(4 hunks)src/main/java/frc/trigon/robot/subsystems/endEffector/EndEffector.java(1 hunks)src/main/java/frc/trigon/robot/subsystems/endEffector/EndEffectorCommands.java(1 hunks)src/main/java/frc/trigon/robot/subsystems/endEffector/EndEffectorConstants.java(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-18T22:46:49.269Z
Learnt from: ShmayaR
PR: Programming-TRIGON/RobotCodeOffSeason2025#16
File: src/main/java/frc/trigon/robot/subsystems/arm/Arm.java:86-88
Timestamp: 2025-09-18T22:46:49.269Z
Learning: In the arm subsystem for this robot, angles above 90 degrees bypass the isArmAboveSafeAngle() logic, making the Rotation2d wrap-around issue less critical in practice according to the team's current system design.
Applied to files:
src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.javasrc/main/java/frc/trigon/robot/subsystems/arm/Arm.java
🧬 Code graph analysis (9)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (3)
src/main/java/frc/trigon/robot/subsystems/endEffector/EndEffectorCommands.java (1)
EndEffectorCommands(10-27)src/main/java/frc/trigon/robot/subsystems/endEffector/EndEffectorConstants.java (1)
EndEffectorConstants(18-101)src/main/java/frc/trigon/robot/subsystems/arm/ArmCommands.java (1)
ArmCommands(14-82)
src/main/java/frc/trigon/robot/RobotContainer.java (3)
src/main/java/frc/trigon/robot/subsystems/endEffector/EndEffector.java (1)
EndEffector(12-69)src/main/java/frc/trigon/robot/subsystems/endEffector/EndEffectorCommands.java (1)
EndEffectorCommands(10-27)src/main/java/frc/trigon/robot/subsystems/endEffector/EndEffectorConstants.java (1)
EndEffectorConstants(18-101)
src/main/java/frc/trigon/robot/subsystems/arm/ArmCommands.java (1)
src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(49-130)
src/main/java/frc/trigon/robot/subsystems/endEffector/EndEffectorCommands.java (2)
src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(49-130)src/main/java/lib/commands/NetworkTablesCommand.java (1)
NetworkTablesCommand(16-139)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java (3)
src/main/java/frc/trigon/robot/subsystems/endEffector/EndEffectorCommands.java (1)
EndEffectorCommands(10-27)src/main/java/frc/trigon/robot/subsystems/endEffector/EndEffectorConstants.java (1)
EndEffectorConstants(18-101)src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(49-130)
src/main/java/frc/trigon/robot/subsystems/arm/Arm.java (1)
src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java (1)
ArmConstants(23-200)
src/main/java/frc/trigon/robot/subsystems/endEffector/EndEffectorConstants.java (5)
src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationFieldHandler.java (1)
SimulationFieldHandler(17-204)src/main/java/lib/hardware/misc/simplesensor/SimpleSensor.java (1)
SimpleSensor(16-139)src/main/java/lib/hardware/phoenix6/talonfx/TalonFXMotor.java (1)
TalonFXMotor(20-209)src/main/java/lib/hardware/simulation/SimpleMotorSimulation.java (1)
SimpleMotorSimulation(12-81)src/main/java/lib/utilities/mechanisms/SpeedMechanism2d.java (1)
SpeedMechanism2d(12-129)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralEjectionCommands.java (4)
src/main/java/frc/trigon/robot/subsystems/endEffector/EndEffectorCommands.java (1)
EndEffectorCommands(10-27)src/main/java/frc/trigon/robot/subsystems/endEffector/EndEffectorConstants.java (1)
EndEffectorConstants(18-101)src/main/java/frc/trigon/robot/subsystems/arm/ArmCommands.java (1)
ArmCommands(14-82)src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(49-130)
src/main/java/frc/trigon/robot/subsystems/endEffector/EndEffector.java (5)
src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(49-130)src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationFieldHandler.java (1)
SimulationFieldHandler(17-204)src/main/java/frc/trigon/robot/subsystems/MotorSubsystem.java (1)
MotorSubsystem(25-173)src/main/java/lib/hardware/phoenix6/talonfx/TalonFXMotor.java (1)
TalonFXMotor(20-209)src/main/java/frc/trigon/robot/subsystems/endEffector/EndEffectorConstants.java (1)
EndEffectorConstants(18-101)
🪛 GitHub Actions: Java CI with Gradle
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java
[error] 54-55: ')' expected at line 54 and ';' expected at line 55 while parsing method calls: ArmCommands.getSetTargetStateCommand(REEF_CHOOSER::getArmState, CoralPlacingCommands::shouldReverseScore) and EndEffectorCommands.getSetTargetStateCommand(EndEffectorConstants.EndEffectorState.SCORE_CORAL).
🪛 Gitleaks (8.28.0)
src/main/java/frc/trigon/robot/subsystems/endEffector/EndEffector.java
[high] 32-32: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (15)
src/main/java/frc/trigon/robot/subsystems/arm/Arm.java (1)
147-149: LGTM: clear single‑angle entry pointThe Rotation2d overload keeps the API focused on arm-only control after decoupling the end effector.
src/main/java/frc/trigon/robot/RobotContainer.java (3)
36-39: LGTM: EndEffector integration importsImports align with the new subsystem and command wiring.
61-61: LGTM: EndEffector subsystem declarationStatic singleton pattern matches the existing subsystems.
90-91: LGTM: Default REST command for EndEffectorConsistent with other subsystems; end effector will idle at 0 V by default.
src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java (2)
37-40: LGTM: Parallelize elevator + end effector and finish on hasGamePieceGood shift of the completion predicate to END_EFFECTOR.
39-40: Confirm asProxy() necessity to preserve requirementsProxying removes outer requirements; ensure default EndEffector REST won’t conflict during LOAD_CORAL. If not needed, drop asProxy().
src/main/java/frc/trigon/robot/commands/commandfactories/CoralEjectionCommands.java (1)
21-22: LGTM: Route ejection through the EndEffector pathBranch switch reflects the new subsystem responsibilities.
src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java (1)
25-35: Critical: CANcoder ID collides with Arm master motor IDANGLE_ENCODER_ID = 13 duplicates ARM_MASTER_MOTOR_ID = 13. CAN device IDs must be unique; this will break CAN comms and sensor fusion.
Also, FeedbackRemoteSensorID should reference the CANcoder ID, not the motor ID.
Proposed fix:
- Assign a unique ANGLE_ENCODER_ID (per your wiring map).
- Use ANGLE_ENCODER.getID() when configuring FeedbackRemoteSensorID.
Apply this diff to bind to the encoder ID (adjust the encoder ID value as per your map):
- config.Feedback.FeedbackRemoteSensorID = ARM_MASTER_MOTOR.getID(); + config.Feedback.FeedbackRemoteSensorID = ANGLE_ENCODER.getID();You can scan for duplicate CAN IDs across the repo:
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (1)
41-45: LGTM! Good separation of concernsThe addition of EndEffectorCommands for scoring coral in the autonomous path properly separates the end effector control from the arm subsystem.
src/main/java/frc/trigon/robot/subsystems/endEffector/EndEffectorCommands.java (1)
10-27: Well-structured command factory implementationThe command factory follows established patterns with proper use of StartEndCommand for state management and NetworkTablesCommand for debugging. The stop action on command end ensures safe subsystem state.
src/main/java/frc/trigon/robot/subsystems/arm/ArmCommands.java (2)
36-40: Good refactoring to ExecuteEndCommandThe replacement of FunctionalCommand with ExecuteEndCommand simplifies the implementation while maintaining the same behavior. The consistent stop action on command end is a good safety practice.
75-80: All hasGamePiece references updated to END_EFFECTOR::hasGamePiece No remaining occurrences of ARM.hasGamePiece were found.src/main/java/frc/trigon/robot/subsystems/endEffector/EndEffector.java (2)
12-30: Clean subsystem implementation following established patternsThe EndEffector subsystem properly extends MotorSubsystem and correctly implements all required lifecycle methods. The sensor updates and mechanism visualization are well integrated.
42-51: No null-check needed for RobotContainer.ARM.getAngle()
getAngle() always returns a non-null Rotation2d and ARM is initialized at startup; no additional checks required.Likely an incorrect or invalid review comment.
src/main/java/frc/trigon/robot/subsystems/endEffector/EndEffectorConstants.java (1)
86-100: Well-designed state enum with clear voltage mappingsThe EndEffectorState enum provides a clean abstraction for different operational states with appropriate voltage values. The naming is descriptive and the voltage polarities correctly indicate direction.
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java
Show resolved
Hide resolved
| config.CurrentLimits.StatorCurrentLimitEnable = true; | ||
| config.CurrentLimits.StatorCurrentLimit = 80; |
There was a problem hiding this comment.
🧩 Analysis chain
Consider if 80A stator current limit is appropriate
The stator current limit of 80A seems high for an end effector mechanism. Verify this is intentional and won't damage the motor or mechanism.
🌐 Web query:
What is the recommended stator current limit for Kraken X60 FOC motors in intake/end effector applications?
💡 Result:
Recommended: about 20–30 A stator current for intake / end-effector Kraken X60 applications (CTRE docs show a 20 A example for an intake; community guidance suggests keeping mechanism/stall currents down to ~20–30 A to avoid overheating and excessive supply draw). [1][2]
Sources:
[1] CTRE Phoenix 6 — "Improving Performance with Current Limits" (example: intake = 20 A stator).
[2] Chief Delphi discussion — CTRE engineer / users recommending ~20–30 A for held/stall mechanisms and noting higher 80–120 A use for drives.
Lower stator current limit to 20–30 A
CTRE docs and community guidance recommend ~20–30 A for Kraken X60 in intake/end-effector roles to avoid overheating and excessive draw.
EndEffectorConstants.java (lines 69–70)
🤖 Prompt for AI Agents
In
src/main/java/frc/trigon/robot/subsystems/endEffector/EndEffectorConstants.java
around lines 69–70, the stator current limit is set to 80 A which is too high
for a Kraken X60 in an intake/end-effector role; change
config.CurrentLimits.StatorCurrentLimitEnable = true to remain enabled and set
config.CurrentLimits.StatorCurrentLimit to a value in the 20–30 A range (e.g.,
25) to follow CTRE guidance and avoid overheating/excessive draw; update any
nearby comment to note the chosen value and rationale.
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 (1)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java (1)
25-27: Avoid scheduling child commands from an InstantCommand; compose themScheduling breaks cancellation/ownership semantics. If the parent is canceled (e.g., button released), the scheduled child keeps running. Compose the load command in the sequence instead.
Apply:
- new InstantCommand( - () -> getLoadCoralCommand().schedule() - ) + getLoadCoralCommand()
🧹 Nitpick comments (10)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java (2)
41-43: Compose align/feedback instead of schedulingSame concern: scheduling align decouples lifecycle from the parent command. Compose it to preserve cancellation semantics.
- new InstantCommand(() -> getAlignCoralCommand().schedule()).alongWith(getCollectionConfirmationCommand()) + getAlignCoralCommand().alongWith(getCollectionConfirmationCommand())
36-36:.asProxy()likely unnecessary hereYou’re directly returning a top-level command, not embedding it where requirement proxying is needed. Dropping
.asProxy()reduces indirection.- ).until(RobotContainer.END_EFFECTOR::hasGamePiece).asProxy(); + ).until(RobotContainer.END_EFFECTOR::hasGamePiece);src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorCommands.java (1)
11-18: Make debugging command periodic for real-time tuning.Using runPeriodically=false applies the NT value only once on initialize. For live tuning from the dashboard, set it to true.
Apply:
- return new NetworkTablesCommand( - RobotContainer.END_EFFECTOR::setTargetState, - false, - Set.of(RobotContainer.END_EFFECTOR), - "Debugging/EndEffectorTargetVoltage" - ); + return new NetworkTablesCommand( + RobotContainer.END_EFFECTOR::setTargetState, + true, + Set.of(RobotContainer.END_EFFECTOR), + "Debugging/EndEffectorTargetVoltage" + );src/main/java/frc/trigon/robot/subsystems/arm/ArmElevator.java (2)
149-157: Use encoder-based angular velocity for correct arm linear velocity.Motor VELOCITY is likely rotor speed; converting via gear ratio or using the CANcoder yields mechanism speed. Using the CANcoder’s velocity removes ambiguity.
Apply:
- public Translation3d calculateLinearArmVelocity() { - double velocityRotationsPerSecond = armMasterMotor.getSignal(TalonFXSignal.VELOCITY); - double velocityMagnitude = velocityRotationsPerSecond * 2 * Math.PI * ArmElevatorConstants.ARM_LENGTH_METERS; + public Translation3d calculateLinearArmVelocity() { + // Mechanism angular velocity from CANcoder (rotations/sec) + double angularVelocityRotPerSec = angleEncoder.getSignal(CANcoderSignal.VELOCITY); + double velocityMagnitude = angularVelocityRotPerSec * 2 * Math.PI * ArmElevatorConstants.ARM_LENGTH_METERS; return new Translation3d( getCurrentArmAngle().getCos() * velocityMagnitude, 0, getCurrentArmAngle().getSin() * velocityMagnitude ); }
187-189: Apply per-state elevator speed scaling.speedScalar exists on the state but isn’t applied. Scale the motion request before setting the position.
Apply:
- void setTargetElevatorState(ArmElevatorConstants.ArmElevatorState targetState) { - setTargetElevatorPositionMeters(targetState.targetPositionMeters, targetState.ignoreConstraints); - } + void setTargetElevatorState(ArmElevatorConstants.ArmElevatorState targetState) { + scaleElevatorPositionRequestSpeed(targetState.speedScalar); + setTargetElevatorPositionMeters(targetState.targetPositionMeters, targetState.ignoreConstraints); + }src/main/java/frc/trigon/robot/subsystems/arm/ArmElevatorConstants.java (1)
110-121: SysId configs: clarify intent or expose both.Only ARM_SYSID_CONFIG is returned. If you intend to characterize the elevator too, consider a switch or a parameterized binding.
Example:
- static final SysIdRoutine.Config ARM_SYSID_CONFIG = ... + static final SysIdRoutine.Config ARM_SYSID_CONFIG = ... ... - static final SysIdRoutine.Config ELEVATOR_SYSID_CONFIG = ... + static final SysIdRoutine.Config ELEVATOR_SYSID_CONFIG = ...And in ArmElevator.getSysIDConfig(), select based on which mechanism you’re characterizing.
src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffector.java (1)
42-51: Wheel speed likely in rotor units; verify gear conversion.endEffectorMotor VELOCITY is typically rotor rotations/sec. If so, multiply by 1/gearRatio to get wheel rotations/sec before converting to m/s.
Since END_EFFECTOR_GEAR_RATIO is private in constants, either:
- expose a getter for the gear ratio, or
- compute mechanism velocity via a sensor or a configured signal that already includes the gear ratio.
Proposed change after exposing a getter:
- double velocityMetersPerSecond = endEffectorMotor.getSignal(TalonFXSignal.VELOCITY) * 2 * Math.PI * EndEffectorConstants.WHEEL_RADIUS_METERS; + double rotorRps = endEffectorMotor.getSignal(TalonFXSignal.VELOCITY); + double wheelRps = rotorRps / EndEffectorConstants.getGearRatio(); + double velocityMetersPerSecond = wheelRps * 2 * Math.PI * EndEffectorConstants.WHEEL_RADIUS_METERS;src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (1)
135-154: Tighten docs to match new unified subsystem.Javadoc still mentions “elevator and gripper” states. Consider updating to “ArmElevator and EndEffector” to avoid confusion.
src/main/java/frc/trigon/robot/subsystems/arm/ArmElevatorCommands.java (2)
46-56: Snapshot suppliers to avoid mid-run target drift.Repeated targetState.get() / isStateReversed.get() calls can change mid-execution (e.g., operator changes selection), leading to inconsistent behavior between onlyIf, until, and setTargetState. Snapshot once when building the command.
Apply this diff:
public static Command getSetTargetStateCommand(Supplier<ArmElevatorConstants.ArmElevatorState> targetState, Supplier<Boolean> isStateReversed) { - return new SequentialCommandGroup( - getPrepareForStateCommand(targetState, isStateReversed) - .onlyIf(() -> targetState.get().ignoreConstraints && targetState.get().prepareState != null) - .until(() -> RobotContainer.ARM_ELEVATOR.atState(targetState.get().prepareState, isStateReversed.get())), - new ExecuteEndCommand( - () -> RobotContainer.ARM_ELEVATOR.setTargetState(targetState.get(), isStateReversed.get()), - RobotContainer.ARM_ELEVATOR::stop, - RobotContainer.ARM_ELEVATOR - ) - ); + final ArmElevatorConstants.ArmElevatorState capturedState = targetState.get(); + final boolean capturedReversed = isStateReversed.get(); + return new SequentialCommandGroup( + getPrepareForStateCommand(() -> capturedState, () -> capturedReversed) + .onlyIf(() -> capturedState.ignoreConstraints && capturedState.prepareState != null) + .until(() -> RobotContainer.ARM_ELEVATOR.atState(capturedState.prepareState, capturedReversed)), + new ExecuteEndCommand( + () -> RobotContainer.ARM_ELEVATOR.setTargetState(capturedState, capturedReversed), + RobotContainer.ARM_ELEVATOR::stop, + RobotContainer.ARM_ELEVATOR + ) + ); }
29-36: Fix typo in public API: Calulation → Calculation.Rename the method to avoid a long‑lived misspelling in the API. Add a deprecated alias temporarily if referenced elsewhere.
Apply this diff:
- public static Command getArmGearRatioCalulationCommand() { + public static Command getArmGearRatioCalculationCommand() { return new GearRatioCalculationCommand( ArmElevatorConstants.ARM_MASTER_MOTOR, ArmElevatorConstants.ANGLE_ENCODER, 0.5, RobotContainer.ARM_ELEVATOR ); }Add this alias elsewhere in the class to ease migration:
@Deprecated(forRemoval = true) public static Command getArmGearRatioCalulationCommand() { return getArmGearRatioCalculationCommand(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
src/main/java/frc/trigon/robot/RobotContainer.java(3 hunks)src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java(2 hunks)src/main/java/frc/trigon/robot/commands/commandfactories/CoralEjectionCommands.java(3 hunks)src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java(7 hunks)src/main/java/frc/trigon/robot/misc/ReefChooser.java(2 hunks)src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationFieldHandler.java(5 hunks)src/main/java/frc/trigon/robot/subsystems/arm/Arm.java(0 hunks)src/main/java/frc/trigon/robot/subsystems/arm/ArmCommands.java(0 hunks)src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java(0 hunks)src/main/java/frc/trigon/robot/subsystems/arm/ArmElevator.java(1 hunks)src/main/java/frc/trigon/robot/subsystems/arm/ArmElevatorCommands.java(1 hunks)src/main/java/frc/trigon/robot/subsystems/arm/ArmElevatorConstants.java(1 hunks)src/main/java/frc/trigon/robot/subsystems/elevator/Elevator.java(0 hunks)src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorCommands.java(0 hunks)src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorConstants.java(0 hunks)src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffector.java(1 hunks)src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorCommands.java(1 hunks)src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorConstants.java(1 hunks)
💤 Files with no reviewable changes (6)
- src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java
- src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorCommands.java
- src/main/java/frc/trigon/robot/subsystems/arm/Arm.java
- src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorConstants.java
- src/main/java/frc/trigon/robot/subsystems/arm/ArmCommands.java
- src/main/java/frc/trigon/robot/subsystems/elevator/Elevator.java
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-11T09:14:51.023Z
Learnt from: levyishai
PR: Programming-TRIGON/RobotCodeOffSeason2025#15
File: src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java:163-166
Timestamp: 2025-09-11T09:14:51.023Z
Learning: FlippablePose2d is a custom class in this codebase that can be used directly with the Swerve subsystem's atPose method, without needing to call .get() to convert it to a standard Pose2d.
Applied to files:
src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationFieldHandler.java
📚 Learning: 2025-09-18T22:46:49.269Z
Learnt from: ShmayaR
PR: Programming-TRIGON/RobotCodeOffSeason2025#16
File: src/main/java/frc/trigon/robot/subsystems/arm/Arm.java:86-88
Timestamp: 2025-09-18T22:46:49.269Z
Learning: In the arm subsystem for this robot, angles above 90 degrees bypass the isArmAboveSafeAngle() logic, making the Rotation2d wrap-around issue less critical in practice according to the team's current system design.
Applied to files:
src/main/java/frc/trigon/robot/subsystems/arm/ArmElevator.java
🧬 Code graph analysis (12)
src/main/java/frc/trigon/robot/misc/ReefChooser.java (1)
src/main/java/frc/trigon/robot/subsystems/arm/ArmElevatorConstants.java (1)
ArmElevatorConstants(28-364)
src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationFieldHandler.java (3)
src/main/java/frc/trigon/robot/subsystems/arm/ArmElevatorConstants.java (1)
ArmElevatorConstants(28-364)src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(46-125)src/main/java/frc/trigon/robot/subsystems/transporter/TransporterConstants.java (1)
TransporterConstants(20-117)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralEjectionCommands.java (5)
src/main/java/frc/trigon/robot/subsystems/arm/ArmElevatorCommands.java (1)
ArmElevatorCommands(15-78)src/main/java/frc/trigon/robot/subsystems/arm/ArmElevatorConstants.java (1)
ArmElevatorConstants(28-364)src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorCommands.java (1)
EndEffectorCommands(10-27)src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorConstants.java (1)
EndEffectorConstants(18-101)src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(46-125)
src/main/java/frc/trigon/robot/subsystems/arm/ArmElevatorCommands.java (4)
src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(46-125)src/main/java/lib/commands/ExecuteEndCommand.java (1)
ExecuteEndCommand(9-21)src/main/java/lib/commands/NetworkTablesCommand.java (1)
NetworkTablesCommand(16-139)src/main/java/frc/trigon/robot/subsystems/arm/ArmElevatorConstants.java (1)
ArmElevatorConstants(28-364)
src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffector.java (5)
src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(46-125)src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationFieldHandler.java (1)
SimulationFieldHandler(17-204)src/main/java/frc/trigon/robot/subsystems/MotorSubsystem.java (1)
MotorSubsystem(25-173)src/main/java/lib/hardware/phoenix6/talonfx/TalonFXMotor.java (1)
TalonFXMotor(20-209)src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorConstants.java (1)
EndEffectorConstants(18-101)
src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorConstants.java (5)
src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationFieldHandler.java (1)
SimulationFieldHandler(17-204)src/main/java/lib/hardware/misc/simplesensor/SimpleSensor.java (1)
SimpleSensor(16-139)src/main/java/lib/hardware/phoenix6/talonfx/TalonFXMotor.java (1)
TalonFXMotor(20-209)src/main/java/lib/hardware/simulation/SimpleMotorSimulation.java (1)
SimpleMotorSimulation(12-81)src/main/java/lib/utilities/mechanisms/SpeedMechanism2d.java (1)
SpeedMechanism2d(12-129)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java (5)
src/main/java/frc/trigon/robot/subsystems/arm/ArmElevatorCommands.java (1)
ArmElevatorCommands(15-78)src/main/java/frc/trigon/robot/subsystems/arm/ArmElevatorConstants.java (1)
ArmElevatorConstants(28-364)src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorCommands.java (1)
EndEffectorCommands(10-27)src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorConstants.java (1)
EndEffectorConstants(18-101)src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(46-125)
src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorCommands.java (2)
src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(46-125)src/main/java/lib/commands/NetworkTablesCommand.java (1)
NetworkTablesCommand(16-139)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (5)
src/main/java/frc/trigon/robot/subsystems/arm/ArmElevatorCommands.java (1)
ArmElevatorCommands(15-78)src/main/java/frc/trigon/robot/subsystems/arm/ArmElevatorConstants.java (1)
ArmElevatorConstants(28-364)src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorCommands.java (1)
EndEffectorCommands(10-27)src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorConstants.java (1)
EndEffectorConstants(18-101)src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(46-125)
src/main/java/frc/trigon/robot/subsystems/arm/ArmElevator.java (6)
src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(46-125)src/main/java/frc/trigon/robot/subsystems/MotorSubsystem.java (1)
MotorSubsystem(25-173)src/main/java/lib/hardware/phoenix6/cancoder/CANcoderEncoder.java (1)
CANcoderEncoder(20-193)src/main/java/lib/hardware/phoenix6/talonfx/TalonFXMotor.java (1)
TalonFXMotor(20-209)src/main/java/lib/utilities/Conversions.java (1)
Conversions(8-258)src/main/java/frc/trigon/robot/subsystems/arm/ArmElevatorConstants.java (1)
ArmElevatorConstants(28-364)
src/main/java/frc/trigon/robot/RobotContainer.java (5)
src/main/java/frc/trigon/robot/subsystems/arm/ArmElevator.java (1)
ArmElevator(19-282)src/main/java/frc/trigon/robot/subsystems/arm/ArmElevatorCommands.java (1)
ArmElevatorCommands(15-78)src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffector.java (1)
EndEffector(12-69)src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorCommands.java (1)
EndEffectorCommands(10-27)src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorConstants.java (1)
EndEffectorConstants(18-101)
src/main/java/frc/trigon/robot/subsystems/arm/ArmElevatorConstants.java (9)
src/main/java/lib/hardware/RobotHardwareStats.java (1)
RobotHardwareStats(6-78)src/main/java/lib/hardware/misc/simplesensor/SimpleSensor.java (1)
SimpleSensor(16-139)src/main/java/lib/hardware/phoenix6/cancoder/CANcoderEncoder.java (1)
CANcoderEncoder(20-193)src/main/java/lib/hardware/phoenix6/talonfx/TalonFXMotor.java (1)
TalonFXMotor(20-209)src/main/java/lib/hardware/simulation/ElevatorSimulation.java (1)
ElevatorSimulation(11-84)src/main/java/lib/hardware/simulation/SingleJointedArmSimulation.java (1)
SingleJointedArmSimulation(12-81)src/main/java/lib/utilities/Conversions.java (1)
Conversions(8-258)src/main/java/lib/utilities/mechanisms/ElevatorMechanism2d.java (1)
ElevatorMechanism2d(13-84)src/main/java/lib/utilities/mechanisms/SingleJointedArmMechanism2d.java (1)
SingleJointedArmMechanism2d(14-87)
🪛 Gitleaks (8.28.0)
src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffector.java
[high] 32-32: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (14)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java (1)
34-36: LGTM: unified load via ArmElevator + EndEffector looks correctParallelizing ArmElevator LOAD_CORAL with EndEffector LOAD_CORAL and ending on END_EFFECTOR::hasGamePiece matches the new subsystem model.
src/main/java/frc/trigon/robot/misc/simulatedfield/SimulationFieldHandler.java (7)
6-6: LGTM: constants import aligned with ArmElevatorImporting ArmElevatorConstants matches the refactor.
81-82: LGTM: algae collection pose now sourced from ArmElevatorUsing ARM_ELEVATOR.calculateGamePieceCollectionPose() is consistent with the new kinematic source.
127-128: LGTM: algae collection state checksCOLLECT_ALGAE_L2/L3 gate collection correctly.
131-132: LGTM: loading detection via ArmElevator stateLOAD_CORAL state as the load indicator is coherent with EndEffector logic.
138-141: LGTM: algae ejection via EndEffector with linear velocityEndEffector-based ejection path and velocity calc are consistent. Verify units expected by SimulatedGamePiece.release match calculateLinearEndEffectorVelocity (m/s).
152-154: LGTM: coral ejection from EndEffector with correct release pointUsing the ArmElevator collection pose translation as the release origin is sensible.
163-165: LGTM: held poses unified on ArmElevator’s collection poseKeeps both coral-in-endeffector and algae tracking coherent.
src/main/java/frc/trigon/robot/misc/ReefChooser.java (1)
47-49: LGTM: single ArmElevatorState getterThe new getter aligns with the unified state model.
src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorConstants.java (1)
69-70: Lower stator current limit from 80A for intake/end-effector use80A is high for this mechanism; 20–30A is typical to avoid overheating/excessive draw.
- config.CurrentLimits.StatorCurrentLimitEnable = true; - config.CurrentLimits.StatorCurrentLimit = 80; + config.CurrentLimits.StatorCurrentLimitEnable = true; + config.CurrentLimits.StatorCurrentLimit = 25; // typical 20–30A for end-effectorsrc/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorCommands.java (1)
20-26: LGTM – clean encapsulation for default/teleop use.The StartEndCommand usage is appropriate; start sets the target and end stops. Works well as a default command since it never ends.
src/main/java/frc/trigon/robot/RobotContainer.java (1)
81-88: LGTM – default commands wired to new subsystems.Defaults for ArmElevator and EndEffector match the new command factories and state model.
src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffector.java (1)
42-51: Verify coordinate frame for ejection vector.Arm linear velocity uses Z for vertical, but the wheel contribution here uses Y. Confirm axis conventions so release vectors match the robot frame used in SimulationFieldHandler.
If Y is intended, ignore. If not, consider aligning with arm velocity layout (e.g., add to X/Z).
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (1)
39-42: Comma fix in ParallelCommandGroup looks good.The argument separation is correct in both parallel groups; prior syntax error is resolved.
Also applies to: 49-52
src/main/java/frc/trigon/robot/commands/commandfactories/CoralEjectionCommands.java
Outdated
Show resolved
Hide resolved
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java
Show resolved
Hide resolved
src/main/java/frc/trigon/robot/subsystems/arm/ArmElevatorConstants.java
Outdated
Show resolved
Hide resolved
src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorConstants.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
src/main/java/frc/trigon/robot/subsystems/arm/ArmElevator.java (2)
63-72: Elevator SysId telemetry commented out.If elevator SysId will be used, enable its logging similarly to the arm to get complete data.
207-215: Avoid static RobotContainer reference inside instance method.Use instance methods to reduce coupling and ease testing.
Apply this diff:
- final double armCos = RobotContainer.ARM_ELEVATOR.getCurrentArmAngle().getCos(); + final double armCos = getCurrentArmAngle().getCos(); ... - final double minimumSafeHeightMeters = (RobotContainer.ARM_ELEVATOR.isArmAboveSafeAngle() + final double minimumSafeHeightMeters = (isArmAboveSafeAngle() ? 0 : elevatorHeightFromArm) + minimumElevatorSafeZone;src/main/java/frc/trigon/robot/subsystems/arm/ArmElevatorCommands.java (2)
29-36: Typo in API: Calulation → Calculation.Public method name has a typo; rename to avoid future confusion.
Apply this diff (update callers accordingly):
- public static Command getArmGearRatioCalulationCommand() { + public static Command getArmGearRatioCalculationCommand() { return new GearRatioCalculationCommand( ArmElevatorConstants.ARM_MASTER_MOTOR, ArmElevatorConstants.ANGLE_ENCODER, 0.5, RobotContainer.ARM_ELEVATOR ); }
63-69: Null-safety for supplier-based commands.If targetState supplier returns null, the lambdas will NPE. Add an onlyIf(() -> targetState.get() != null).
Apply this diff:
public static Command getPrepareForStateCommand(Supplier<ArmElevatorConstants.ArmElevatorState> targetState, Supplier<Boolean> isStateReversed) { return new ExecuteEndCommand( () -> RobotContainer.ARM_ELEVATOR.prepareToState(targetState.get(), isStateReversed.get()), RobotContainer.ARM_ELEVATOR::stop, RobotContainer.ARM_ELEVATOR - ).onlyIf(() -> targetState.get().prepareState != null); + ).onlyIf(() -> targetState.get() != null && targetState.get().prepareState != null); }Also consider wrapping the SequentialCommandGroup in getSetTargetStateCommand(...) with .onlyIf(() -> targetState.get() != null).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/frc/trigon/robot/subsystems/arm/ArmElevator.java(1 hunks)src/main/java/frc/trigon/robot/subsystems/arm/ArmElevatorCommands.java(1 hunks)src/main/java/frc/trigon/robot/subsystems/arm/ArmElevatorConstants.java(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-18T22:46:49.269Z
Learnt from: ShmayaR
PR: Programming-TRIGON/RobotCodeOffSeason2025#16
File: src/main/java/frc/trigon/robot/subsystems/arm/Arm.java:86-88
Timestamp: 2025-09-18T22:46:49.269Z
Learning: In the arm subsystem for this robot, angles above 90 degrees bypass the isArmAboveSafeAngle() logic, making the Rotation2d wrap-around issue less critical in practice according to the team's current system design.
Applied to files:
src/main/java/frc/trigon/robot/subsystems/arm/ArmElevator.java
🧬 Code graph analysis (3)
src/main/java/frc/trigon/robot/subsystems/arm/ArmElevatorConstants.java (9)
src/main/java/lib/hardware/RobotHardwareStats.java (1)
RobotHardwareStats(6-78)src/main/java/lib/hardware/misc/simplesensor/SimpleSensor.java (1)
SimpleSensor(16-139)src/main/java/lib/hardware/phoenix6/cancoder/CANcoderEncoder.java (1)
CANcoderEncoder(20-193)src/main/java/lib/hardware/phoenix6/talonfx/TalonFXMotor.java (1)
TalonFXMotor(20-209)src/main/java/lib/hardware/simulation/ElevatorSimulation.java (1)
ElevatorSimulation(11-84)src/main/java/lib/hardware/simulation/SingleJointedArmSimulation.java (1)
SingleJointedArmSimulation(12-81)src/main/java/lib/utilities/Conversions.java (1)
Conversions(8-258)src/main/java/lib/utilities/mechanisms/ElevatorMechanism2d.java (1)
ElevatorMechanism2d(13-84)src/main/java/lib/utilities/mechanisms/SingleJointedArmMechanism2d.java (1)
SingleJointedArmMechanism2d(14-87)
src/main/java/frc/trigon/robot/subsystems/arm/ArmElevator.java (6)
src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(46-125)src/main/java/frc/trigon/robot/subsystems/MotorSubsystem.java (1)
MotorSubsystem(25-173)src/main/java/lib/hardware/phoenix6/cancoder/CANcoderEncoder.java (1)
CANcoderEncoder(20-193)src/main/java/lib/hardware/phoenix6/talonfx/TalonFXMotor.java (1)
TalonFXMotor(20-209)src/main/java/lib/utilities/Conversions.java (1)
Conversions(8-258)src/main/java/frc/trigon/robot/subsystems/arm/ArmElevatorConstants.java (1)
ArmElevatorConstants(28-364)
src/main/java/frc/trigon/robot/subsystems/arm/ArmElevatorCommands.java (5)
src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(46-125)src/main/java/lib/commands/ExecuteEndCommand.java (1)
ExecuteEndCommand(9-21)src/main/java/lib/commands/GearRatioCalculationCommand.java (1)
GearRatioCalculationCommand(20-183)src/main/java/lib/commands/NetworkTablesCommand.java (1)
NetworkTablesCommand(16-139)src/main/java/frc/trigon/robot/subsystems/arm/ArmElevatorConstants.java (1)
ArmElevatorConstants(28-364)
🔇 Additional comments (9)
src/main/java/frc/trigon/robot/subsystems/arm/ArmElevatorConstants.java (3)
30-33: Fix duplicate CAN IDs for arm motor and angle encoder.
ARM_MASTER_MOTOR_IDandANGLE_ENCODER_IDare both set to 13. CTRE devices must have unique IDs; duplicates prevent both devices from responding reliably, so the arm will lose its absolute position reference. Pick an unused CAN ID for the encoder (and update the hardware configuration to match).
195-197: Point the fused CANcoder at the encoder, not the motor.
FeedbackRemoteSensorIDis currently set to the arm master motor’s device ID. ForFeedbackSensorSourceValue.FusedCANcoderthis must be the CANcoder’s ID; otherwise the Talon never reads the external sensor and all closed-loop control runs on raw rotor rotations. Update this line to useANGLE_ENCODER.getID().Apply this diff:
- config.Feedback.FeedbackRemoteSensorID = ARM_MASTER_MOTOR.getID(); + config.Feedback.FeedbackRemoteSensorID = ANGLE_ENCODER.getID();
306-307: Follower motor is configured to follow itself.The elevator follower is told to follow
ELEVATOR_FOLLOWER_MOTOR.getID(), so it never mirrors the master and just idles. Have it followELEVATOR_MASTER_MOTORinstead so both motors stay synchronized.Apply this diff:
- final Follower followerRequest = new Follower(ELEVATOR_FOLLOWER_MOTOR.getID(), SHOULD_ELEVATOR_FOLLOWER_OPPOSE_MASTER); + final Follower followerRequest = new Follower(ELEVATOR_MASTER_MOTOR.getID(), SHOULD_ELEVATOR_FOLLOWER_OPPOSE_MASTER);src/main/java/frc/trigon/robot/subsystems/arm/ArmElevator.java (3)
45-48: SysId config selects only Arm; confirm intent.You return ARM_SYSID_CONFIG and drive/log only the arm. If you intend to characterize the elevator too, expose a switch or a dedicated command path.
110-112: Safe-angle cutoff matches team practice.Using ≥ 90° to bypass safety-vs-elevator constraints aligns with the prior subsystem design.
Based on learnings
186-196: Angle clamp logic: confirm domain assumptions.You clamp with Math.max(targetRot, minSafeRot). This assumes angles are in [0, 1) rotations and that higher rotation equals higher angle. Given soft limits and your 0–270° range, this is OK; just confirm no targets wrap negative or >1 rotation.
src/main/java/frc/trigon/robot/subsystems/arm/ArmElevatorCommands.java (3)
16-27: Debug command looks good.Accepting angle/height from NT and fanning out to the subsystem is clean for testing.
46-57: This sequence never finishes; verify intended usage.The second ExecuteEndCommand has no termination; in a sequential group it will run indefinitely. If this is used as a default command, fine. If used for one-shot moves, add an until() on reaching the target.
Optionally apply:
new ExecuteEndCommand( () -> RobotContainer.ARM_ELEVATOR.setTargetState(targetState.get(), isStateReversed.get()), RobotContainer.ARM_ELEVATOR::stop, RobotContainer.ARM_ELEVATOR - ) + ).until(() -> RobotContainer.ARM_ELEVATOR.atState(targetState.get(), isStateReversed.get()))
47-51: Prepare-phase gating may be inverted.You only run the prepare state when ignoreConstraints is true. Confirm that’s intended; often prepare is used to ensure safe sequencing when constraints are enforced.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralEjectionCommands.java (1)
36-36: Optional: End the EJECT when the coral leaves or after a short timeoutPrevent running EJECT indefinitely by stopping when the end effector no longer detects coral (or by adding a small timeout as a fallback).
- EndEffectorCommands.getSetTargetStateCommand(EndEffectorConstants.EndEffectorState.EJECT) + EndEffectorCommands.getSetTargetStateCommand(EndEffectorConstants.EndEffectorState.EJECT) + .until(() -> !RobotContainer.END_EFFECTOR.hasGamePiece()) + //.withTimeout(0.75) // optional fallbacksrc/main/java/frc/trigon/robot/subsystems/arm/ArmElevator.java (1)
215-223: Avoid cross-referencing self via RobotContainer inside the subsystemUse this.getCurrentArmAngle()/isArmAboveSafeAngle() instead of RobotContainer.ARM_ELEVATOR to reduce coupling and improve testability.
- final double armCos = RobotContainer.ARM_ELEVATOR.getCurrentArmAngle().getCos(); + final double armCos = getCurrentArmAngle().getCos(); ... - final double minimumSafeHeightMeters = (RobotContainer.ARM_ELEVATOR.isArmAboveSafeAngle() + final double minimumSafeHeightMeters = (isArmAboveSafeAngle() ? 0 : elevatorHeightFromArm) + minimumElevatorSafeZone;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralEjectionCommands.java(3 hunks)src/main/java/frc/trigon/robot/subsystems/arm/ArmElevator.java(1 hunks)src/main/java/frc/trigon/robot/subsystems/arm/ArmElevatorCommands.java(1 hunks)src/main/java/frc/trigon/robot/subsystems/arm/ArmElevatorConstants.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/frc/trigon/robot/subsystems/arm/ArmElevatorCommands.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-18T22:46:49.269Z
Learnt from: ShmayaR
PR: Programming-TRIGON/RobotCodeOffSeason2025#16
File: src/main/java/frc/trigon/robot/subsystems/arm/Arm.java:86-88
Timestamp: 2025-09-18T22:46:49.269Z
Learning: In the arm subsystem for this robot, angles above 90 degrees bypass the isArmAboveSafeAngle() logic, making the Rotation2d wrap-around issue less critical in practice according to the team's current system design.
Applied to files:
src/main/java/frc/trigon/robot/subsystems/arm/ArmElevator.java
🧬 Code graph analysis (3)
src/main/java/frc/trigon/robot/subsystems/arm/ArmElevator.java (6)
src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(46-125)src/main/java/frc/trigon/robot/subsystems/MotorSubsystem.java (1)
MotorSubsystem(25-173)src/main/java/lib/hardware/phoenix6/cancoder/CANcoderEncoder.java (1)
CANcoderEncoder(20-193)src/main/java/lib/hardware/phoenix6/talonfx/TalonFXMotor.java (1)
TalonFXMotor(20-209)src/main/java/lib/utilities/Conversions.java (1)
Conversions(8-258)src/main/java/frc/trigon/robot/subsystems/arm/ArmElevatorConstants.java (1)
ArmElevatorConstants(28-364)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralEjectionCommands.java (5)
src/main/java/frc/trigon/robot/subsystems/arm/ArmElevatorCommands.java (1)
ArmElevatorCommands(15-78)src/main/java/frc/trigon/robot/subsystems/arm/ArmElevatorConstants.java (1)
ArmElevatorConstants(28-364)src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorCommands.java (1)
EndEffectorCommands(10-27)src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorConstants.java (1)
EndEffectorConstants(18-101)src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(46-125)
src/main/java/frc/trigon/robot/subsystems/arm/ArmElevatorConstants.java (9)
src/main/java/lib/hardware/RobotHardwareStats.java (1)
RobotHardwareStats(6-78)src/main/java/lib/hardware/misc/simplesensor/SimpleSensor.java (1)
SimpleSensor(16-139)src/main/java/lib/hardware/phoenix6/cancoder/CANcoderEncoder.java (1)
CANcoderEncoder(20-193)src/main/java/lib/hardware/phoenix6/talonfx/TalonFXMotor.java (1)
TalonFXMotor(20-209)src/main/java/lib/hardware/simulation/ElevatorSimulation.java (1)
ElevatorSimulation(11-84)src/main/java/lib/hardware/simulation/SingleJointedArmSimulation.java (1)
SingleJointedArmSimulation(12-81)src/main/java/lib/utilities/Conversions.java (1)
Conversions(8-258)src/main/java/lib/utilities/mechanisms/ElevatorMechanism2d.java (1)
ElevatorMechanism2d(13-84)src/main/java/lib/utilities/mechanisms/SingleJointedArmMechanism2d.java (1)
SingleJointedArmMechanism2d(14-87)
🔇 Additional comments (9)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralEjectionCommands.java (2)
21-23: Switch to end-effector path looks goodDelegating to getEjectCoralFromEndEffectorCommand aligns with the new subsystem design.
35-36: Blocker: .until(atTargetState) can short-circuit immediately and eject earlyuntil(RobotContainer.ARM_ELEVATOR::atTargetState) can be true at schedule time (e.g., when the subsystem is already at its current target), causing the prepare-and-move to EJECT to be interrupted before motion begins, and the end-effector will eject prematurely.
Gate explicitly on the EJECT state:
- ArmElevatorCommands.getSetTargetStateCommand(ArmElevatorConstants.ArmElevatorState.EJECT).until(RobotContainer.ARM_ELEVATOR::atTargetState), + ArmElevatorCommands.getSetTargetStateCommand(ArmElevatorConstants.ArmElevatorState.EJECT) + .until(() -> RobotContainer.ARM_ELEVATOR.atState(ArmElevatorConstants.ArmElevatorState.EJECT)),src/main/java/frc/trigon/robot/subsystems/arm/ArmElevatorConstants.java (4)
320-322: LGTM: Encoder signals registered correctlyRegistering POSITION and VELOCITY at 100 Hz is appropriate.
30-35: Critical: Duplicate CAN IDs (ARM motor and angle encoder both 13)CAN devices must have unique IDs; this will break fused CANcoder and bus comms.
- ANGLE_ENCODER_ID = 13, + ANGLE_ENCODER_ID = 15, // choose an unused ID and match Phoenix TunerAlso update the physical device ID to match.
195-198: Critical: Fused CANcoder remote sensor ID points to the motor, not the encoderFeedbackRemoteSensorID must be the CANcoder’s device ID.
- config.Feedback.FeedbackRemoteSensorID = ARM_MASTER_MOTOR.getID(); + config.Feedback.FeedbackRemoteSensorID = ANGLE_ENCODER.getID();
306-307: Critical: Elevator follower follows itselfFollower should target the master motor ID.
- final Follower followerRequest = new Follower(ELEVATOR_FOLLOWER_MOTOR.getID(), SHOULD_ELEVATOR_FOLLOWER_OPPOSE_MASTER); + final Follower followerRequest = new Follower(ELEVATOR_MASTER_MOTOR.getID(), SHOULD_ELEVATOR_FOLLOWER_OPPOSE_MASTER);src/main/java/frc/trigon/robot/subsystems/arm/ArmElevator.java (3)
147-155: Fix arm tip velocity: use tangential direction, not radialCurrent uses (cos, sin) which points along the arm. Tangential velocity is (-sin, cos).
public Translation3d calculateLinearArmVelocity() { double velocityRotationsPerSecond = armMasterMotor.getSignal(TalonFXSignal.VELOCITY); double velocityMagnitude = velocityRotationsPerSecond * 2 * Math.PI * ArmElevatorConstants.ARM_LENGTH_METERS; return new Translation3d( - getCurrentArmAngle().getCos() * velocityMagnitude, + -getCurrentArmAngle().getSin() * velocityMagnitude, 0, - getCurrentArmAngle().getSin() * velocityMagnitude + getCurrentArmAngle().getCos() * velocityMagnitude ); }
157-160: Null-safety: prepareToState() can NPE when prepareState is nullIf targetState or targetState.prepareState is null, setTargetState(null) leads to downstream NPEs.
- void prepareToState(ArmElevatorConstants.ArmElevatorState targetState) { - setTargetState(targetState.prepareState); - } + void prepareToState(ArmElevatorConstants.ArmElevatorState targetState) { + if (targetState == null || targetState.prepareState == null) + return; + setTargetState(targetState.prepareState); + }
173-179: Guard setTargetState against nullCalling with null will dereference in setTargetArmState/setTargetElevatorState.
- void setTargetState(ArmElevatorConstants.ArmElevatorState targetState, boolean isStateReversed) { - this.isStateReversed = isStateReversed; - this.targetState = targetState; - - setTargetArmState(targetState, isStateReversed); - setTargetElevatorState(targetState); - } + void setTargetState(ArmElevatorConstants.ArmElevatorState targetState, boolean isStateReversed) { + if (targetState == null) + return; + this.isStateReversed = isStateReversed; + this.targetState = targetState; + setTargetArmState(targetState, isStateReversed); + setTargetElevatorState(targetState); + }
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java (2)
22-28: Avoid side‑effect scheduling; chain the load step in the sequence.Scheduling inside an InstantCommand ends the sequence immediately and decouples requirements. Prefer sequencing the load command directly for clearer lifecycle and requirement management.
- public static Command getCoralCollectionCommand() { - return new SequentialCommandGroup( - getIntakeSequenceCommand(), - new InstantCommand( - () -> getLoadCoralCommand().schedule() - ) - ); - // new IntakeAssistCommand(OperatorConstants.DEFAULT_INTAKE_ASSIST_MODE) - } + public static Command getCoralCollectionCommand() { + return new SequentialCommandGroup( + getIntakeSequenceCommand(), + getLoadCoralCommand() + ); + // new IntakeAssistCommand(OperatorConstants.DEFAULT_INTAKE_ASSIST_MODE) + }
32-37: Remove redundant termination guard (until vs onlyWhile).
.until(END_EFFECTOR::hasGamePiece)and.onlyWhile(() -> !END_EFFECTOR.hasGamePiece())are functionally duplicative. Keep theuntilfor clarity.If
hasGamePiece()is not already debounced in the EndEffector, consider a short debounce to prevent flicker end conditions.- ).until(RobotContainer.END_EFFECTOR::hasGamePiece).onlyWhile(() -> !RobotContainer.END_EFFECTOR.hasGamePiece()); + ).until(RobotContainer.END_EFFECTOR::hasGamePiece);src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (2)
41-46: Add a finish condition for the scoring phase.The ParallelCommandGroup contains StartEnd/ExecuteEnd commands that never finish on their own. Add a termination (e.g., until coral is released) to let the sequence complete without relying solely on trigger release.
- new ParallelCommandGroup( + new ParallelCommandGroup( ArmElevatorCommands.getSetTargetStateCommand(REEF_CHOOSER::getArmElevatorState, CoralPlacingCommands::shouldReverseScore), EndEffectorCommands.getSetTargetStateCommand(EndEffectorConstants.EndEffectorState.SCORE_CORAL) - ) + ).until(() -> !RobotContainer.END_EFFECTOR.hasGamePiece())
53-56: Mirror scoring termination in the manual flow.Same rationale as autonomous: add a clear end condition for the scoring phase.
- new ParallelCommandGroup( + new ParallelCommandGroup( ArmElevatorCommands.getSetTargetStateCommand(REEF_CHOOSER::getArmElevatorState, CoralPlacingCommands::shouldReverseScore), EndEffectorCommands.getSetTargetStateCommand(EndEffectorConstants.EndEffectorState.SCORE_CORAL) - ) + ).until(() -> !RobotContainer.END_EFFECTOR.hasGamePiece())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java(2 hunks)src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java (5)
src/main/java/frc/trigon/robot/subsystems/arm/ArmElevatorCommands.java (1)
ArmElevatorCommands(15-78)src/main/java/frc/trigon/robot/subsystems/arm/ArmElevatorConstants.java (1)
ArmElevatorConstants(28-364)src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorCommands.java (1)
EndEffectorCommands(10-27)src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorConstants.java (1)
EndEffectorConstants(18-101)src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(46-125)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (6)
src/main/java/frc/trigon/robot/subsystems/arm/ArmElevatorCommands.java (1)
ArmElevatorCommands(15-78)src/main/java/frc/trigon/robot/subsystems/arm/ArmElevatorConstants.java (1)
ArmElevatorConstants(28-364)src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorCommands.java (1)
EndEffectorCommands(10-27)src/main/java/frc/trigon/robot/subsystems/endeffector/EndEffectorConstants.java (1)
EndEffectorConstants(18-101)src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java (1)
CoralCollectionCommands(20-66)src/main/java/frc/trigon/robot/RobotContainer.java (1)
RobotContainer(46-125)
🔇 Additional comments (3)
src/main/java/frc/trigon/robot/commands/commandfactories/CoralCollectionCommands.java (1)
10-13: Imports migration to ArmElevator/EndEffector looks good.Aligned with the new subsystems/command factories.
src/main/java/frc/trigon/robot/commands/commandfactories/CoralPlacingCommands.java (2)
16-19: Imports updated to unified ArmElevator/EndEffector APIs.Looks correct.
111-114: Fix potential NPE when prepareState is null (L1).
REEF_CHOOSER.getArmElevatorState().prepareStatemay be null (e.g., L1), causing an NPE inatState(...).- private static boolean isReadyToScore(boolean shouldScoreRight) { - return RobotContainer.ARM_ELEVATOR.atState(REEF_CHOOSER.getArmElevatorState().prepareState, shouldReverseScore()) - && RobotContainer.SWERVE.atPose(calculateClosestScoringPose(shouldScoreRight)); - } + private static boolean isReadyToScore(boolean shouldScoreRight) { + final var state = REEF_CHOOSER.getArmElevatorState(); + final var stateToCheck = (state.prepareState == null) ? state : state.prepareState; + return RobotContainer.ARM_ELEVATOR.atState(stateToCheck, shouldReverseScore()) + && RobotContainer.SWERVE.atPose(calculateClosestScoringPose(shouldScoreRight)); + }
Summary by CodeRabbit
New Features
Refactor
Chores