Subsystem Wrapper #54
Conversation
src/main/java/org/trigon/hardware/subsystems/elevator/ElevatorConfiguration.java
Outdated
Show resolved
Hide resolved
src/main/java/org/trigon/hardware/subsystems/flywheel/SimpleMotorConfiguration.java
Outdated
Show resolved
Hide resolved
src/main/java/org/trigon/hardware/subsystems/flywheel/SimpleMotorSubsystem.java
Outdated
Show resolved
Hide resolved
src/main/java/org/trigon/hardware/subsystems/flywheel/SimpleMotorSubsystem.java
Show resolved
Hide resolved
src/main/java/org/trigon/hardware/subsystems/flywheel/SimpleMotorSubsystem.java
Outdated
Show resolved
Hide resolved
src/main/java/org/trigon/hardware/subsystems/flywheel/SimpleMotorSubsystem.java
Outdated
Show resolved
Hide resolved
src/main/java/org/trigon/hardware/subsystems/flywheel/SimpleMotorSubsystem.java
Outdated
Show resolved
Hide resolved
src/main/java/org/trigon/hardware/subsystems/flywheel/SimpleMotorSubsystem.java
Outdated
Show resolved
Hide resolved
src/main/java/org/trigon/hardware/subsystems/flywheel/SimpleMotorSubsystem.java
Outdated
Show resolved
Hide resolved
|
Approved by me as long as you've tested everything |
src/main/java/org/trigon/hardware/subsystems/arm/ArmConfiguration.java
Outdated
Show resolved
Hide resolved
Trigon5990
left a comment
Trigon5990
left a comment
There was a problem hiding this comment.
Needs a ton of jdocs. Make motor subsystem more abstract for this to work with it
Strflightmight09
left a comment
Strflightmight09
left a comment
There was a problem hiding this comment.
A couple things:
- You forgot to add in javadocs for the actual subsystems themselves
- Each of the javadocs for the configs should be a full sentence, and should start with "the" in most cases and shouldn't be in point form
| /** | ||
| * Maximum velocity of the motor. | ||
| */ | ||
| public double maximumVelocity = 1; | ||
|
|
||
| /** | ||
| * Maximum acceleration of the motor. | ||
| */ | ||
| public double maximumAcceleration = 1; | ||
|
|
||
| /** | ||
| * Maximum jerk of the motor. | ||
| */ | ||
| public double maximumJerk = 1; |
| /** | ||
| * Visual offset for display in Mechanism2D. | ||
| */ | ||
| public double visualizationOffset = 1; |
There was a problem hiding this comment.
This needs more than just a rewording, this needs an explanation
| /** | ||
| * Acceptable angular tolerance for reaching the target. | ||
| */ | ||
| public Rotation2d angleTolerance = Rotation2d.kZero; |
There was a problem hiding this comment.
for reaching the target isn't precise enough
|
|
||
| public boolean shouldSimulateGravity = true; | ||
|
|
||
| public Color mechanismColor = Color.kBlue; |
There was a problem hiding this comment.
| public Color mechanismColor = Color.kBlue; | |
| public Color mechanism2DColor = Color.kBlue; |
Rn it sounds like the physical colour of the mechanism
| public Color mechanismColor = Color.kBlue; | ||
|
|
||
| /** | ||
| * The type and number of motors used in the simulation. |
There was a problem hiding this comment.
| * The type and number of motors used in the simulation. | |
| * The type and number of motors used for the simulation. |
| /** | ||
| * An interface for an arm state. | ||
| * getTargetAngle represents the target angle of the state. | ||
| * getSpeedScalar represents the speed scalar of the state. | ||
| */ | ||
| public interface ArmState { |
There was a problem hiding this comment.
You still don't explain what it's for
| /** | ||
| * Acceptable position error in meters. | ||
| */ | ||
| public double positionToleranceMeters = 1; |
There was a problem hiding this comment.
This isn't consistent with how you described the arm one
|
|
||
| public double gearRatio = 1; | ||
| /** | ||
| * Mass of the Elevator in kg. |
| */ | ||
| maximumDisplayableVelocity = 1, | ||
| /** | ||
| * The acceptable tolerance between the target velocity and current velocity. |
| /** | ||
| * should the motor control mode be Voltage as opposed to Velocity. | ||
| */ | ||
| public boolean shouldUseVoltageControl = false; |
There was a problem hiding this comment.
Explain what each one is
WalkthroughAdds an abstract MotorSubsystem base class with a central registry, global disabled-trigger braking, async brake setting, periodic update hooks, SysId routine integration, extensible logging, and a stop() contract. Implements three concrete subsystems: ArmSubsystem, ElevatorSubsystem, and SimpleMotorSubsystem, each with corresponding Configuration and Commands classes. Each subsystem wires motor control, simulation, SysId configuration, mechanism visualization, state-targeting APIs, logging hooks, and command factory utilities for debugging and state transitions. 🚥 Pre-merge checks | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 12
♻️ Duplicate comments (1)
hardware/subsystems/arm/ArmConfiguration.java (1)
15-28: Units still missing in velocity/acceleration/jerk JavaDocs.These fields lack unit specifications. Based on the context (arm angular motion), these should likely be in radians/s, radians/s², and radians/s³ respectively.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
hardware/subsystems/arm/ArmConfiguration.javahardware/subsystems/arm/ArmSubsystem.javahardware/subsystems/elevator/ElevatorConfiguration.javahardware/subsystems/elevator/ElevatorSubsystem.javahardware/subsystems/flywheel/SimpleMotorConfiguration.javahardware/subsystems/flywheel/SimpleMotorSubsystem.java
🧰 Additional context used
📓 Path-based instructions (1)
**/*
⚙️ CodeRabbit configuration file
**/*: ## Review Style
- Explain issues clearly, but don't over-explain. Don't write anything unnecessary or repetetive
- Suggest a solution when helpful
- No code blocks unless they genuinely add clarity - GitHub already shows the diff
- Don't describe what the code does - focus on what could be improved
Naming & Readability
- Variable, function, and class names must be descriptive and intention-revealing
- No abbreviations unless universally understood (e.g., id, url)
- Boolean names should read as questions (isValid, hasPermission, canExecute)
- Functions should be verbs, classes should be nouns
- Magic numbers and strings should be named constants
- Code should be self-documenting; comments should explain "why", not "what"
Clean Code Principles
- Functions should do one thing and be small
- Too many parameters? Suggest grouping into an object
- Flag deep nesting - suggest early returns or extraction
- DRY: flag duplicated logic
- Functions with non-obvious side effects
- Single responsibility for classes and modules
- Proper abstraction levels - no mixing high and low level logic
Logic & Correctness
- Off-by-one errors, null/undefined handling, race conditions
- Unhandled edge cases
- Incorrect boolean logic or conditions
- Resource leaks, missing cleanup
- Error handling gaps
- Performance issues when significant
Code Structure
- File and folder organization
- Proper separation of concerns
- Dependency management and coupling
Files:
hardware/subsystems/flywheel/SimpleMotorConfiguration.javahardware/subsystems/elevator/ElevatorConfiguration.javahardware/subsystems/arm/ArmConfiguration.javahardware/subsystems/elevator/ElevatorSubsystem.javahardware/subsystems/flywheel/SimpleMotorSubsystem.javahardware/subsystems/arm/ArmSubsystem.java
🧬 Code graph analysis (2)
hardware/subsystems/flywheel/SimpleMotorSubsystem.java (3)
hardware/phoenix6/talonfx/TalonFXMotor.java (1)
TalonFXMotor(20-209)hardware/simulation/SimpleMotorSimulation.java (1)
SimpleMotorSimulation(12-81)utilities/mechanisms/SpeedMechanism2d.java (1)
SpeedMechanism2d(12-129)
hardware/subsystems/arm/ArmSubsystem.java (4)
hardware/RobotHardwareStats.java (1)
RobotHardwareStats(6-78)hardware/phoenix6/talonfx/TalonFXMotor.java (1)
TalonFXMotor(20-209)hardware/simulation/SingleJointedArmSimulation.java (1)
SingleJointedArmSimulation(12-81)utilities/mechanisms/SingleJointedArmMechanism2d.java (1)
SingleJointedArmMechanism2d(14-87)
🔇 Additional comments (4)
hardware/subsystems/elevator/ElevatorSubsystem.java (2)
89-96: Mechanism update may use wrong units.
getPositionRotations()returns motor rotations, butElevatorMechanism2dwas constructed with height limits in meters. Verify whether the mechanism expects meters or rotations; if meters, usegetPositionMeters()instead.
49-53: SysId timeout of 1000 seconds is unusually long.This allows tests to run for ~16 minutes. If intentional, consider adding a comment explaining the rationale. If not, a more typical value might be 10-30 seconds.
hardware/subsystems/arm/ArmSubsystem.java (1)
46-50: SysId timeout of 1000 seconds is very long.Same as ElevatorSubsystem. Consider documenting if intentional, or reducing to a more typical value.
hardware/subsystems/flywheel/SimpleMotorSubsystem.java (1)
103-110:atTargetState()always returns true in voltage control mode.Line 107-108 immediately returns
truefor voltage control. This means any voltage target is considered "reached" instantly, which may be intentional but differs from velocity mode behavior. Document this design choice if intentional.
| motor.setPhysicsSimulation( | ||
| new SingleJointedArmSimulation( | ||
| config.gearbox, | ||
| config.gearRatio, | ||
| config.massKilograms, | ||
| config.lengthMeters, | ||
| config.minimumAngle, | ||
| config.maximumAngle, | ||
| config.shouldSimulateGravity | ||
| )); |
There was a problem hiding this comment.
Parameter order swapped: mass and length are reversed.
The SingleJointedArmSimulation constructor expects (gearbox, gearRatio, armLengthMeters, armMassKilograms, ...) but the code passes massKilograms before lengthMeters. This will cause incorrect physics simulation.
🔎 Proposed fix
motor.setPhysicsSimulation(
new SingleJointedArmSimulation(
config.gearbox,
config.gearRatio,
- config.massKilograms,
config.lengthMeters,
+ config.massKilograms,
config.minimumAngle,
config.maximumAngle,
config.shouldSimulateGravity
));| public boolean atTargetState() { | ||
| if (targetState == null) | ||
| return false; | ||
| final double currentToTargetStateDifferenceDegrees = Math.abs(targetState.getTargetAngle().minus(getAngle()).getDegrees()); | ||
| return currentToTargetStateDifferenceDegrees < angleTolerance.getDegrees(); | ||
| } |
There was a problem hiding this comment.
Edge case: zero angleTolerance causes atTargetState() to never return true.
If angleTolerance is Rotation2d.kZero (the default), getDegrees() returns 0, and the comparison difference < 0 is always false. Consider using <= or documenting that tolerance must be non-zero.
| /** | ||
| * Maximum velocity of the elevator in meters per second. | ||
| */ | ||
| public double maximumVelocity = 1; | ||
|
|
||
| /** | ||
| * Maximum acceleration of the elevator in meters per second squared. | ||
| */ | ||
| public double maximumAcceleration = 1; | ||
|
|
||
| /** | ||
| * Maximum jerk of the elevator in meters per second cubed. | ||
| */ | ||
| public double maximumJerk = 1; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Missing units in JavaDocs for velocity, acceleration, and jerk.
The ElevatorConfiguration specifies units in JavaDocs for position (meters) but omits units for velocity (meters/second), acceleration (meters/second²), and jerk (meters/second³). This inconsistency could lead to confusion. Consider adding units like in the position tolerance field.
| public void updateLog(SysIdRoutineLog log) { | ||
| log.motor(name) | ||
| .linearPosition(Units.Meters.of(getPositionRotations())) | ||
| .linearVelocity(Units.MetersPerSecond.of(motor.getSignal(TalonFXSignal.VELOCITY))) |
There was a problem hiding this comment.
Velocity also needs unit conversion.
TalonFXSignal.VELOCITY returns rotations per second, but linearVelocity expects meters per second. Apply the same drum radius conversion used in rotationsToMeters.
| public double | ||
| gearRatio = 1, | ||
| /** | ||
| * The moment of inertia of the motor | ||
| */ | ||
| momentOfInertia = 0.003, | ||
| /** | ||
| * Maximum velocity of the motor. | ||
| */ | ||
| maximumVelocity = 1, | ||
| /** | ||
| * Maximum acceleration of the motor. | ||
| */ | ||
| maximumAcceleration = 1, | ||
| /** | ||
| * Maximum jerk of the motor. | ||
| */ | ||
| maximumJerk = 1, | ||
| /** | ||
| * Maximum velocity displayed by the mechanism 2D | ||
| */ | ||
| maximumDisplayableVelocity = 1, | ||
| /** | ||
| * The acceptable tolerance between the target velocity and current velocity. | ||
| */ | ||
| velocityTolerance = 1, | ||
| /** | ||
| * Ramp rate used in system identification. | ||
| * See <a href="https://docs.wpilib.org/en/stable/docs/software/advanced-controls/system-identification/introduction.html"> | ||
| * WPILib System Identification Introduction</a>. | ||
| */ | ||
| sysIDRampRate = 1, | ||
| /** | ||
| * Step voltage used in system identification. | ||
| * See <a href="https://docs.wpilib.org/en/stable/docs/software/advanced-controls/system-identification/introduction.html"> | ||
| * WPILib System Identification Introduction</a>. | ||
| */ | ||
| sysIDStepVoltage = 1; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Unconventional multi-variable declaration with inline JavaDocs.
While functional, declaring multiple fields in one statement with interleaved JavaDocs is unusual and harder to maintain. Consider separating each field for clarity and consistency with other configuration classes.
| * WPILib System Identification Introduction</a>. | ||
| */ | ||
| sysIDStepVoltage = 1; | ||
| public boolean focEnabled = true; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Missing documentation for focEnabled.
Unlike other fields, focEnabled has no JavaDoc. Add a brief explanation of what FOC (Field-Oriented Control) does and when to enable/disable it.
| public void updateLog(SysIdRoutineLog log) { | ||
| log.motor(name) | ||
| .angularPosition(Units.Degrees.of(motor.getSignal(TalonFXSignal.POSITION))) | ||
| .angularVelocity(Units.RotationsPerSecond.of(motor.getSignal(TalonFXSignal.VELOCITY))) | ||
| .voltage(Units.Volts.of(motor.getSignal(TalonFXSignal.MOTOR_VOLTAGE))); | ||
| } |
There was a problem hiding this comment.
Unit mismatch: position is in rotations, not degrees.
TalonFXSignal.POSITION returns rotations, but line 73 wraps it with Units.Degrees.of(). Should be Units.Rotations.of().
🔎 Proposed fix
public void updateLog(SysIdRoutineLog log) {
log.motor(name)
- .angularPosition(Units.Degrees.of(motor.getSignal(TalonFXSignal.POSITION)))
+ .angularPosition(Units.Rotations.of(motor.getSignal(TalonFXSignal.POSITION)))
.angularVelocity(Units.RotationsPerSecond.of(motor.getSignal(TalonFXSignal.VELOCITY)))
.voltage(Units.Volts.of(motor.getSignal(TalonFXSignal.MOTOR_VOLTAGE)));
}📝 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.
| public void updateLog(SysIdRoutineLog log) { | |
| log.motor(name) | |
| .angularPosition(Units.Degrees.of(motor.getSignal(TalonFXSignal.POSITION))) | |
| .angularVelocity(Units.RotationsPerSecond.of(motor.getSignal(TalonFXSignal.VELOCITY))) | |
| .voltage(Units.Volts.of(motor.getSignal(TalonFXSignal.MOTOR_VOLTAGE))); | |
| } | |
| public void updateLog(SysIdRoutineLog log) { | |
| log.motor(name) | |
| .angularPosition(Units.Rotations.of(motor.getSignal(TalonFXSignal.POSITION))) | |
| .angularVelocity(Units.RotationsPerSecond.of(motor.getSignal(TalonFXSignal.VELOCITY))) | |
| .voltage(Units.Volts.of(motor.getSignal(TalonFXSignal.MOTOR_VOLTAGE))); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 28
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
subsystems/MotorSubsystem.javasubsystems/arm/ArmSubsystem.javasubsystems/arm/ArmSubsystemCommands.javasubsystems/arm/ArmSubsystemConfiguration.javasubsystems/elevator/ElevatorSubsystem.javasubsystems/elevator/ElevatorSubsystemCommands.javasubsystems/elevator/ElevatorSubsystemConfiguration.javasubsystems/flywheel/SimpleMotorSubsystem.javasubsystems/flywheel/SimpleMotorSubsystemCommands.javasubsystems/flywheel/SimpleMotorSubsystemConfiguration.java
🧰 Additional context used
📓 Path-based instructions (1)
**/*
⚙️ CodeRabbit configuration file
**/*: ## Review Style
- Explain issues clearly, but don't over-explain. Don't write anything unnecessary or repetetive
- Suggest a solution when helpful
- No code blocks unless they genuinely add clarity - GitHub already shows the diff
- Don't describe what the code does - focus on what could be improved
Naming & Readability
- Variable, function, and class names must be descriptive and intention-revealing
- No abbreviations unless universally understood (e.g., id, url)
- Boolean names should read as questions (isValid, hasPermission, canExecute)
- Functions should be verbs, classes should be nouns
- Magic numbers and strings should be named constants
- Code should be self-documenting; comments should explain "why", not "what"
Clean Code Principles
- Functions should do one thing and be small
- Too many parameters? Suggest grouping into an object
- Flag deep nesting - suggest early returns or extraction
- DRY: flag duplicated logic
- Functions with non-obvious side effects
- Single responsibility for classes and modules
- Proper abstraction levels - no mixing high and low level logic
Logic & Correctness
- Off-by-one errors, null/undefined handling, race conditions
- Unhandled edge cases
- Incorrect boolean logic or conditions
- Resource leaks, missing cleanup
- Error handling gaps
- Performance issues when significant
Code Structure
- File and folder organization
- Proper separation of concerns
- Dependency management and coupling
Files:
subsystems/arm/ArmSubsystemConfiguration.javasubsystems/arm/ArmSubsystemCommands.javasubsystems/flywheel/SimpleMotorSubsystemCommands.javasubsystems/flywheel/SimpleMotorSubsystemConfiguration.javasubsystems/elevator/ElevatorSubsystem.javasubsystems/elevator/ElevatorSubsystemConfiguration.javasubsystems/elevator/ElevatorSubsystemCommands.javasubsystems/arm/ArmSubsystem.javasubsystems/MotorSubsystem.javasubsystems/flywheel/SimpleMotorSubsystem.java
🧬 Code graph analysis (5)
subsystems/arm/ArmSubsystemCommands.java (3)
commands/ExecuteEndCommand.java (1)
ExecuteEndCommand(9-21)commands/GearRatioCalculationCommand.java (1)
GearRatioCalculationCommand(20-183)commands/NetworkTablesCommand.java (1)
NetworkTablesCommand(16-139)
subsystems/flywheel/SimpleMotorSubsystemCommands.java (2)
commands/ExecuteEndCommand.java (1)
ExecuteEndCommand(9-21)commands/NetworkTablesCommand.java (1)
NetworkTablesCommand(16-139)
subsystems/elevator/ElevatorSubsystem.java (5)
hardware/phoenix6/talonfx/TalonFXMotor.java (1)
TalonFXMotor(20-209)hardware/simulation/ElevatorSimulation.java (1)
ElevatorSimulation(11-84)subsystems/MotorSubsystem.java (1)
MotorSubsystem(24-171)utilities/Conversions.java (1)
Conversions(8-258)utilities/mechanisms/ElevatorMechanism2d.java (1)
ElevatorMechanism2d(13-84)
subsystems/elevator/ElevatorSubsystemCommands.java (2)
commands/ExecuteEndCommand.java (1)
ExecuteEndCommand(9-21)commands/NetworkTablesCommand.java (1)
NetworkTablesCommand(16-139)
subsystems/arm/ArmSubsystem.java (5)
hardware/RobotHardwareStats.java (1)
RobotHardwareStats(6-78)hardware/phoenix6/talonfx/TalonFXMotor.java (1)
TalonFXMotor(20-209)hardware/simulation/SingleJointedArmSimulation.java (1)
SingleJointedArmSimulation(12-81)subsystems/MotorSubsystem.java (1)
MotorSubsystem(24-171)utilities/mechanisms/SingleJointedArmMechanism2d.java (1)
SingleJointedArmMechanism2d(14-87)
🔇 Additional comments (8)
subsystems/elevator/ElevatorSubsystemCommands.java (1)
13-13: VerifyrunPeriodically=falseis intentional.Setting
runPeriodicallytofalsemeanssetTargetStateis called only once duringinitialize(), not continuously duringexecute(). If the NetworkTables values change during the command, they won't be applied. If this is intentional for one-time debugging updates, consider renaming the method to reflect that (e.g.,getOneTimeDebuggingCommand).subsystems/arm/ArmSubsystemCommands.java (1)
16-16: VerifyrunPeriodically=falseis intentional.Setting
runPeriodicallytofalsemeans the target state is set only once duringinitialize(), not continuously duringexecute(). If NetworkTables values change during the command, they won't be re-applied. If this one-time behavior is intended, consider a more descriptive method name.subsystems/MotorSubsystem.java (2)
73-78: LGTM!The
periodic()method correctly delegates toupdatePeriodically()and conditionally callsupdateMechanism(). Making itfinalensures consistent lifecycle behavior across all subclasses.
157-170: LGTM!
createSysIDRoutinecorrectly handles the null config case and constructs the routine with proper voltage conversion and logging callbacks.subsystems/elevator/ElevatorSubsystem.java (1)
118-127: LGTM!State checking logic is clean. Uses meters consistently for tolerance comparison.
subsystems/flywheel/SimpleMotorSubsystemCommands.java (1)
19-41: LGTM!The state command factories are clean and follow a consistent pattern. Using
ExecuteEndCommandensures the motor stops when the command ends.subsystems/flywheel/SimpleMotorSubsystem.java (1)
114-121: atTargetState returns true immediately for voltage control.When
shouldUseVoltageControlis true,atTargetState()returns true unconditionally (line 119). This meansatState()will return true as soon as a target is set, not when the motor actually reaches the target voltage.If this is intentional (voltage control has no "at target" concept), document it. Otherwise, compare actual vs target voltage.
subsystems/arm/ArmSubsystem.java (1)
179-185: Remove this comment -getPrepareState()is used inArmSubsystemCommands.java.The method is called at line 51 of
ArmSubsystemCommands.javaas part of a standard pattern used consistently across subsystems (flywheel, elevator). No issue exists.Likely an incorrect or invalid review comment.
| public boolean atState(ArmState targetState) { | ||
| return this.targetState == targetState && atTargetState(); | ||
| } | ||
|
|
||
| public boolean atTargetState() { | ||
| if (targetState == null) | ||
| return false; | ||
| final double currentToTargetStateDifferenceDegrees = Math.abs(targetState.getTargetAngle().minus(getAngle()).getDegrees()); | ||
| return currentToTargetStateDifferenceDegrees < angleTolerance.getDegrees(); | ||
| } |
There was a problem hiding this comment.
Reference equality check for state may not work as intended.
atState uses == to compare this.targetState == targetState. If ArmState implementations are not singletons (e.g., enums), this check will fail even for logically equal states.
Consider using .equals() or documenting that states must be enum constants.
| public Rotation2d getAngle() { | ||
| return Rotation2d.fromRotations(motor.getSignal(TalonFXSignal.POSITION) + (RobotHardwareStats.isSimulation() ? 0 : visualizationOffsetFromGravityOffset)); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Inconsistent offset application between getAngle and setTargetAngle.
In getAngle() (line 127), the offset is added when NOT in simulation.
In setTargetAngle() (line 159), the offset is subtracted when NOT in simulation.
This is likely correct (read vs write compensation), but the dual ternary logic is error-prone and hard to verify. Consider extracting the offset logic into named helper methods for clarity.
| subsystem.getName() + "TargetAngleDegrees", | ||
| subsystem.getName() + "SpeedScalar" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
NetworkTables keys lack separator - could be ambiguous.
Concatenating subsystem.getName() + "TargetAngleDegrees" creates keys without clear boundaries. Add a separator like "/" to improve clarity:
subsystem.getName() + "/TargetAngleDegrees",
subsystem.getName() + "/SpeedScalar"| ); | ||
| } | ||
|
|
||
| public static Command getGearRatioCalculationCommand(DoubleConsumer runVoltage, double backlashAccountabilityTimeSeconds, ArmSubsystem subsystem) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Parameter name runVoltage is misleading.
The parameter is a DoubleConsumer that applies voltage to the motor, not a voltage value. Name it to reflect its purpose: applyVoltage, voltageController, or setMotorVoltage.
| import edu.wpi.first.wpilibj.util.Color; | ||
|
|
||
| public class ArmSubsystemConfiguration { | ||
| public String name = ""; |
There was a problem hiding this comment.
Empty name default hinders debugging.
An empty string for name makes logging and NetworkTables keys unclear. Default to "Arm" or require it in a constructor.
| gearRatio = 1, | ||
| /** | ||
| * The moment of inertia of the motor | ||
| */ | ||
| momentOfInertia = 0.003, | ||
| /** | ||
| * Maximum velocity of the motor. | ||
| */ | ||
| maximumVelocity = 1, | ||
| /** | ||
| * Maximum acceleration of the motor. | ||
| */ | ||
| maximumAcceleration = 1, | ||
| /** | ||
| * Maximum jerk of the motor. | ||
| */ | ||
| maximumJerk = 1, | ||
| /** | ||
| * Maximum velocity displayed by the mechanism 2D | ||
| */ | ||
| maximumDisplayableVelocity = 1, | ||
| /** | ||
| * The acceptable tolerance between the target velocity and current velocity. | ||
| */ | ||
| velocityTolerance = 1, | ||
| /** | ||
| * Ramp rate used in system identification. | ||
| * See <a href="https://docs.wpilib.org/en/stable/docs/software/advanced-controls/system-identification/introduction.html"> | ||
| * WPILib System Identification Introduction</a>. | ||
| */ | ||
| sysIDRampRate = 1, | ||
| /** | ||
| * Step voltage used in system identification. | ||
| * See <a href="https://docs.wpilib.org/en/stable/docs/software/advanced-controls/system-identification/introduction.html"> | ||
| * WPILib System Identification Introduction</a>. | ||
| */ | ||
| sysIDStepVoltage = 1; |
There was a problem hiding this comment.
Placeholder defaults risk misconfiguration.
All numeric fields default to 1 (except momentOfInertia), which are clearly placeholder values. If a user forgets to override these, the subsystem will run with nonsensical parameters. Consider either:
- Using
Double.NaNor throwing on uninitialized values to fail fast - Documenting prominently that these must be configured
- Providing sensible defaults based on actual hardware
| * If a subsystem doesn't need to ever brake (i.e. shooter, flywheel, etc.), then it should override the {@link #setBrake(boolean)} method and do nothing. | ||
| */ | ||
| public abstract class MotorSubsystem extends edu.wpi.first.wpilibj2.command.SubsystemBase { | ||
| public static boolean IS_BRAKING = true; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Public mutable static field is a code smell.
IS_BRAKING is public and mutable, which breaks encapsulation. Any code can modify this without going through the proper setAllSubsystemsBrakeAsync flow.
Consider making it private with a getter, or at least private static with package-private accessor.
🔎 Proposed fix
- public static boolean IS_BRAKING = true;
+ private static boolean isBraking = true;
+
+ public static boolean isBraking() {
+ return isBraking;
+ }Then update line 35:
- IS_BRAKING = true;
+ isBraking = true;Committable suggestion skipped: line range outside the PR's diff.
| static { | ||
| DISABLED_TRIGGER.onTrue(new InstantCommand(() -> forEach(MotorSubsystem::stop)).ignoringDisable(true)); | ||
| DISABLED_TRIGGER.onFalse(new InstantCommand(() -> { | ||
| setAllSubsystemsBrakeAsync(true); | ||
| IS_BRAKING = true; | ||
| }).ignoringDisable(true)); | ||
| } |
There was a problem hiding this comment.
Race condition: IS_BRAKING set before async brake completes.
Line 35 sets IS_BRAKING = true immediately, but setAllSubsystemsBrakeAsync runs on a thread pool. The flag will be true before motors actually finish braking.
If external code checks IS_BRAKING to determine motor state, it will get incorrect information.
| public MotorSubsystem() { | ||
| REGISTERED_SUBSYSTEMS.add(this); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Subsystems are never removed from REGISTERED_SUBSYSTEMS.
If a subsystem is ever destroyed or recreated (e.g., in tests), the old reference remains in the list, causing memory leaks and duplicate operations in forEach.
Consider providing an unregister() method or using WeakReference.
| public void changeDefaultCommand(Command newDefaultCommand) { | ||
| final Command currentDefaultCommand = getDefaultCommand(); | ||
| if (currentDefaultCommand != null) | ||
| currentDefaultCommand.cancel(); | ||
| setDefaultCommand(newDefaultCommand); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Canceling running default command may cause unexpected behavior.
changeDefaultCommand cancels the current default command before setting a new one. If the command is actively running, this interruption could leave the subsystem in an inconsistent state. The WPILib setDefaultCommand already handles this gracefully.
Consider removing the explicit cancel or documenting why it's necessary.
There was a problem hiding this comment.
Actionable comments posted: 13
♻️ Duplicate comments (20)
hardware/subsystems/elevator/ElevatorConfiguration.java (1)
57-57: Add JavaDoc forgearRatio.Other fields are documented. Briefly explain what this represents (e.g., motor rotations per output rotation, or output rotations per motor rotation).
hardware/subsystems/flywheel/SimpleMotorConfiguration.java (2)
10-47: Separate field declarations for readability.The multi-variable declaration with interleaved JavaDocs is hard to maintain. Consider separating each field onto its own line, matching the style in
ElevatorConfigurationandArmConfiguration.
48-48: Add JavaDoc forfocEnabled.Explain what FOC (Field-Oriented Control) does and when to enable/disable it.
hardware/subsystems/arm/ArmConfiguration.java (1)
15-28: Specify units for velocity, acceleration, and jerk.These fields lack units in their JavaDocs. Are they in rotations/second, radians/second, or degrees/second? Add units for clarity.
subsystems/elevator/ElevatorSubsystemCommands.java (1)
15-16: Add separator to NetworkTables keys.Keys like
"ElevatorTargetPositionMeters"can be ambiguous. Use a separator for clarity:subsystem.getName() + "/TargetPositionMeters", subsystem.getName() + "/SpeedScalar"hardware/subsystems/elevator/ElevatorSubsystem.java (1)
82-87: Bug: Unit mismatch inupdateLog.Two issues:
- Line 84:
getPositionRotations()returns rotations, butlinearPositionexpects meters. UsegetPositionMeters().- Line 85:
TalonFXSignal.VELOCITYreturns rotations/second, butlinearVelocityexpects meters/second. Apply drum radius conversion.🔎 Proposed fix
public void updateLog(SysIdRoutineLog log) { log.motor(name) - .linearPosition(Units.Meters.of(getPositionRotations())) - .linearVelocity(Units.MetersPerSecond.of(motor.getSignal(TalonFXSignal.VELOCITY))) + .linearPosition(Units.Meters.of(getPositionMeters())) + .linearVelocity(Units.MetersPerSecond.of(rotationsToMeters(motor.getSignal(TalonFXSignal.VELOCITY)))) .voltage(Units.Volts.of(motor.getSignal(TalonFXSignal.MOTOR_VOLTAGE))); }hardware/subsystems/arm/ArmSubsystem.java (2)
51-60: Bug: ParametersmassKilogramsandlengthMetersare swapped.Per
SingleJointedArmSimulationconstructor signature(gearbox, gearRatio, armLengthMeters, armMassKilograms, ...), length should come before mass. Current order passes mass first, causing incorrect physics simulation.🔎 Proposed fix
motor.setPhysicsSimulation( new SingleJointedArmSimulation( config.gearbox, config.gearRatio, - config.massKilograms, config.lengthMeters, + config.massKilograms, config.minimumAngle, config.maximumAngle, config.shouldSimulateGravity ));
108-113: Edge case: ZeroangleTolerancecausesatTargetState()to always return false.Default
angleToleranceisRotation2d.kZero(0 degrees). The comparisondifference < 0is never true. Use<=or require non-zero tolerance.🔎 Proposed fix
public boolean atTargetState() { if (targetState == null) return false; final double currentToTargetStateDifferenceDegrees = Math.abs(targetState.getTargetAngle().minus(getAngle()).getDegrees()); - return currentToTargetStateDifferenceDegrees < angleTolerance.getDegrees(); + return currentToTargetStateDifferenceDegrees <= angleTolerance.getDegrees(); }subsystems/arm/ArmSubsystemCommands.java (2)
18-19: Add separator to NetworkTables keys.Use
"/"separator for clarity:subsystem.getName() + "/TargetAngleDegrees".
23-23: RenamerunVoltageparameter.The parameter is a
DoubleConsumerthat applies voltage, not a voltage value. ConsiderapplyVoltageorvoltageApplier.subsystems/arm/ArmSubsystem.java (2)
52-61: Constructor parameter order mismatch with SingleJointedArmSimulation.Per the
SingleJointedArmSimulationconstructor signature inhardware/simulation/SingleJointedArmSimulation.java, the parameter order is(gearbox, gearRatio, armLengthMeters, armMassKilograms, ...). You're passingmassKilogramsbeforelengthMeters, which will cause incorrect physics simulation.Swap lines 56-57.
115-117: Reference equality check for state may not work as intended.
atStateuses==to compare states. IfArmStateimplementations are not enums or singletons, this comparison will fail for logically equivalent states. Use.equals()or document that states must be enum constants.subsystems/elevator/ElevatorSubsystem.java (2)
89-95: Unit mismatch: logging rotations as meters.
getPositionRotations()returns rotations, but you're wrapping it withUnits.Meters.of()on line 92. Same issue on line 93 where velocity in rotations/sec is logged asMetersPerSecond. UsegetPositionMeters()androtationsToMeters()for proper conversions.
101-104: Mechanism update uses wrong units.
ElevatorMechanism2dexpects meters (based onmaximumHeight,minimumHeightconstructor params), butgetPositionRotations()andCLOSED_LOOP_REFERENCEare in rotations. Convert both to meters.subsystems/flywheel/SimpleMotorSubsystemCommands.java (1)
10-17: Debugging command runs only once, not continuously.
runPeriodicallyisfalse, sosetTargetVelocityis called only ininitialize(). For a debugging command that should continuously read NetworkTables values, set this totrue.subsystems/flywheel/SimpleMotorSubsystem.java (3)
19-22: Unused fields: maximumAcceleration and maximumJerk.These fields are assigned but never used.
VelocityVoltagedoesn't have acceleration/jerk parameters likeDynamicMotionMagicVoltage. Remove them or use a control request that supports these parameters.Also applies to: 35-37
78-84: Inconsistent units in updateLog.Line 81 logs position as
Units.Degrees.of(motor.getSignal(TalonFXSignal.POSITION)), butPOSITIONreturns rotations, not degrees. UseUnits.Rotations.of(...)for consistency withArmSubsystem.updateLog.
86-96: Potential NullPointerException when targetState is null.In velocity control mode, line 94 calls
targetState.getTargetUnit()without a null check. IfupdateMechanism()is invoked before any target is set, this throws NPE.Add a null guard:
if (shouldUseVoltageControl) { mechanism.update(getVoltage()); return; } + if (targetState == null) { + mechanism.update(getVelocityRotationsPerSecond()); + return; + } mechanism.update( getVelocityRotationsPerSecond(), targetState.getTargetUnit() );hardware/subsystems/flywheel/SimpleMotorSubsystem.java (2)
71-76: Unit mismatch: position in rotations, not degrees.
TalonFXSignal.POSITIONreturns rotations. Line 73 wraps it withUnits.Degrees.of()which is incorrect. UseUnits.Rotations.of().
78-87: Potential NullPointerException when targetState is null.Line 85 accesses
targetState.getTargetUnit()without null check in velocity control mode. Add a guard before accessing.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (12)
hardware/subsystems/arm/ArmConfiguration.javahardware/subsystems/arm/ArmSubsystem.javahardware/subsystems/elevator/ElevatorConfiguration.javahardware/subsystems/elevator/ElevatorSubsystem.javahardware/subsystems/flywheel/SimpleMotorConfiguration.javahardware/subsystems/flywheel/SimpleMotorSubsystem.javasubsystems/arm/ArmSubsystem.javasubsystems/arm/ArmSubsystemCommands.javasubsystems/elevator/ElevatorSubsystem.javasubsystems/elevator/ElevatorSubsystemCommands.javasubsystems/flywheel/SimpleMotorSubsystem.javasubsystems/flywheel/SimpleMotorSubsystemCommands.java
🧰 Additional context used
📓 Path-based instructions (1)
**/*
⚙️ CodeRabbit configuration file
**/*: ## Review Style
- Explain issues clearly, but don't over-explain. Don't write anything unnecessary or repetetive
- Suggest a solution when helpful
- No code blocks unless they genuinely add clarity - GitHub already shows the diff
- Don't describe what the code does - focus on what could be improved
Naming & Readability
- Variable, function, and class names must be descriptive and intention-revealing
- No abbreviations unless universally understood (e.g., id, url)
- Boolean names should read as questions (isValid, hasPermission, canExecute)
- Functions should be verbs, classes should be nouns
- Magic numbers and strings should be named constants
- Code should be self-documenting; comments should explain "why", not "what"
Clean Code Principles
- Functions should do one thing and be small
- Too many parameters? Suggest grouping into an object
- Flag deep nesting - suggest early returns or extraction
- DRY: flag duplicated logic
- Functions with non-obvious side effects
- Single responsibility for classes and modules
- Proper abstraction levels - no mixing high and low level logic
Logic & Correctness
- Off-by-one errors, null/undefined handling, race conditions
- Unhandled edge cases
- Incorrect boolean logic or conditions
- Resource leaks, missing cleanup
- Error handling gaps
- Performance issues when significant
Code Structure
- File and folder organization
- Proper separation of concerns
- Dependency management and coupling
Files:
subsystems/flywheel/SimpleMotorSubsystemCommands.javasubsystems/elevator/ElevatorSubsystemCommands.javasubsystems/arm/ArmSubsystem.javahardware/subsystems/arm/ArmSubsystem.javahardware/subsystems/arm/ArmConfiguration.javahardware/subsystems/elevator/ElevatorSubsystem.javasubsystems/arm/ArmSubsystemCommands.javahardware/subsystems/flywheel/SimpleMotorConfiguration.javasubsystems/flywheel/SimpleMotorSubsystem.javasubsystems/elevator/ElevatorSubsystem.javahardware/subsystems/flywheel/SimpleMotorSubsystem.javahardware/subsystems/elevator/ElevatorConfiguration.java
🧬 Code graph analysis (7)
subsystems/flywheel/SimpleMotorSubsystemCommands.java (2)
commands/ExecuteEndCommand.java (1)
ExecuteEndCommand(9-21)commands/NetworkTablesCommand.java (1)
NetworkTablesCommand(16-139)
subsystems/elevator/ElevatorSubsystemCommands.java (2)
commands/ExecuteEndCommand.java (1)
ExecuteEndCommand(9-21)commands/NetworkTablesCommand.java (1)
NetworkTablesCommand(16-139)
subsystems/arm/ArmSubsystem.java (5)
hardware/RobotHardwareStats.java (1)
RobotHardwareStats(6-78)hardware/phoenix6/talonfx/TalonFXMotor.java (1)
TalonFXMotor(20-209)hardware/simulation/SingleJointedArmSimulation.java (1)
SingleJointedArmSimulation(12-81)subsystems/MotorSubsystem.java (1)
MotorSubsystem(24-171)utilities/mechanisms/SingleJointedArmMechanism2d.java (1)
SingleJointedArmMechanism2d(14-87)
hardware/subsystems/arm/ArmSubsystem.java (4)
hardware/RobotHardwareStats.java (1)
RobotHardwareStats(6-78)hardware/phoenix6/talonfx/TalonFXMotor.java (1)
TalonFXMotor(20-209)hardware/simulation/SingleJointedArmSimulation.java (1)
SingleJointedArmSimulation(12-81)utilities/mechanisms/SingleJointedArmMechanism2d.java (1)
SingleJointedArmMechanism2d(14-87)
subsystems/arm/ArmSubsystemCommands.java (3)
commands/ExecuteEndCommand.java (1)
ExecuteEndCommand(9-21)commands/GearRatioCalculationCommand.java (1)
GearRatioCalculationCommand(20-183)commands/NetworkTablesCommand.java (1)
NetworkTablesCommand(16-139)
subsystems/flywheel/SimpleMotorSubsystem.java (4)
hardware/phoenix6/talonfx/TalonFXMotor.java (1)
TalonFXMotor(20-209)hardware/simulation/SimpleMotorSimulation.java (1)
SimpleMotorSimulation(12-81)subsystems/MotorSubsystem.java (1)
MotorSubsystem(24-171)utilities/mechanisms/SpeedMechanism2d.java (1)
SpeedMechanism2d(12-129)
hardware/subsystems/flywheel/SimpleMotorSubsystem.java (3)
hardware/phoenix6/talonfx/TalonFXMotor.java (1)
TalonFXMotor(20-209)hardware/simulation/SimpleMotorSimulation.java (1)
SimpleMotorSimulation(12-81)utilities/mechanisms/SpeedMechanism2d.java (1)
SpeedMechanism2d(12-129)
🔇 Additional comments (4)
subsystems/arm/ArmSubsystem.java (1)
174-185: LGTM - ArmState interface is well-defined.The interface provides a clear contract for arm states with target angle, prepare state, and speed scalar. This pattern enables polymorphic state handling.
subsystems/elevator/ElevatorSubsystem.java (1)
37-67: LGTM - Constructor properly initializes all subsystem components.Physics simulation, mechanism visualization, control requests, and SysId config are all correctly wired.
subsystems/flywheel/SimpleMotorSubsystemCommands.java (1)
19-41: LGTM - Target state commands correctly use ExecuteEndCommand.The pattern of setting target in execute and calling stop on end is appropriate for these commands.
hardware/subsystems/flywheel/SimpleMotorSubsystem.java (1)
42-46: : The code at lines 43-46 already usesnullfor the timeout parameter, notUnits.Second.of(1000). This is consistent with other similar subsystems (ElevatorSubsystem, ArmSubsystem). No action needed.Likely an incorrect or invalid review comment.
| */ | ||
| public double sysIDStepVoltage = 1; | ||
|
|
||
| public double gearRatio = 1; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add JavaDoc for gearRatio.
Document what this represents (e.g., motor rotations per arm rotation).
| public boolean focEnabled = true; | ||
|
|
||
| public boolean shouldSimulateGravity = true; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add JavaDocs for focEnabled and shouldSimulateGravity.
These boolean flags lack documentation explaining their purpose.
| public interface ArmState { | ||
| Rotation2d getTargetAngle(); | ||
|
|
||
| double getSpeedScalar(); | ||
| } |
There was a problem hiding this comment.
Add getPrepareState() to ArmState interface.
ArmSubsystemCommands.getPrepareTargetStateCommand() calls targetState.getPrepareState(), but this method isn't declared in the interface.
🔎 Proposed fix
public interface ArmState {
Rotation2d getTargetAngle();
double getSpeedScalar();
+
+ ArmState getPrepareState();
}📝 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.
| public interface ArmState { | |
| Rotation2d getTargetAngle(); | |
| double getSpeedScalar(); | |
| } | |
| public interface ArmState { | |
| Rotation2d getTargetAngle(); | |
| double getSpeedScalar(); | |
| ArmState getPrepareState(); | |
| } |
| public boolean focEnabled = true; | ||
|
|
||
| public boolean shouldSimulateGravity = true; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Missing documentation for focEnabled and shouldSimulateGravity.
Add brief JavaDocs explaining what FOC (Field-Oriented Control) does and when gravity simulation should be enabled/disabled.
| public void updateMechanism() { | ||
| logComponentPoses(); | ||
|
|
||
| mechanism.update( | ||
| getPositionRotations(), | ||
| motor.getSignal(TalonFXSignal.CLOSED_LOOP_REFERENCE) | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "*.java" | xargs grep -l "class ElevatorMechanism2d"Repository: Programming-TRIGON/TRIGONLib
Length of output: 119
🏁 Script executed:
cat -n ./utilities/mechanisms/ElevatorMechanism2d.java | head -150Repository: Programming-TRIGON/TRIGONLib
Length of output: 4051
🏁 Script executed:
cat -n ./hardware/subsystems/elevator/ElevatorSubsystem.java | head -120Repository: Programming-TRIGON/TRIGONLib
Length of output: 157
🏁 Script executed:
find . -type f -name "ElevatorSubsystem.java"Repository: Programming-TRIGON/TRIGONLib
Length of output: 116
🏁 Script executed:
cat -n ./subsystems/elevator/ElevatorSubsystem.java | sed -n '80,105p'Repository: Programming-TRIGON/TRIGONLib
Length of output: 992
🏁 Script executed:
grep -n "getPositionRotations" ./subsystems/elevator/ElevatorSubsystem.javaRepository: Programming-TRIGON/TRIGONLib
Length of output: 542
🏁 Script executed:
cat -n ./subsystems/elevator/ElevatorSubsystem.java | sed -n '129,170p'Repository: Programming-TRIGON/TRIGONLib
Length of output: 1972
🏁 Script executed:
grep -n "mechanism.update" ./subsystems/elevator/ElevatorSubsystem.javaRepository: Programming-TRIGON/TRIGONLib
Length of output: 101
🏁 Script executed:
cat -n ./subsystems/elevator/ElevatorSubsystem.java | sed -n '1,50p'Repository: Programming-TRIGON/TRIGONLib
Length of output: 2843
Pass position in meters to mechanism.update().
Line 102 passes getPositionRotations() (in rotations) to mechanism.update(), but the mechanism expects meters. The mechanism displays elevator height and is initialized with maximumHeight and minimumHeight (line 47), then uses position values as-is in setLength() calls (lines 73, 82). Elsewhere in the subsystem, rotations are explicitly converted to meters for display purposes (line 168). Convert the position and target:
mechanism.update(
getPositionMeters(),
rotationsToMeters(motor.getSignal(TalonFXSignal.CLOSED_LOOP_REFERENCE))
);
| * Maximum velocity of the motor. | ||
| */ | ||
| maximumVelocity = 1, | ||
| /** | ||
| * Maximum acceleration of the motor. | ||
| */ | ||
| maximumAcceleration = 1, | ||
| /** | ||
| * Maximum jerk of the motor. | ||
| */ | ||
| maximumJerk = 1, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Specify units for velocity, acceleration, and jerk.
The JavaDocs don't indicate what units these values use (rotations/second? radians/second?). Add units for consistency with other configuration classes.
| /** | ||
| * An interface for a Simple motor state. | ||
| * getTargetUnit represents the target Velocity or Voltage (depending on the control mode) of the state. | ||
| */ | ||
| public interface SimpleMotorState { | ||
| double getTargetUnit(); | ||
| } | ||
| } |
There was a problem hiding this comment.
SimpleMotorState interface missing getPrepareState() method.
The SimpleMotorState interface in subsystems/flywheel/SimpleMotorSubsystem.java includes getPrepareState(), but this version doesn't. This creates an API inconsistency. If both classes are meant to be interchangeable, align the interfaces.
| public boolean atState(ElevatorState targetState) { | ||
| return this.targetState == targetState && atTargetState(); | ||
| } |
There was a problem hiding this comment.
Reference equality check for state may fail for non-singleton implementations.
Same issue as ArmSubsystem.atState() - using == instead of .equals(). If ElevatorState implementations aren't enums, logically equal states won't match.
There was a problem hiding this comment.
Actionable comments posted: 27
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
subsystems/arm/ArmSubsystem.javasubsystems/arm/ArmSubsystemCommands.javasubsystems/arm/ArmSubsystemConfiguration.javasubsystems/elevator/ElevatorSubsystem.javasubsystems/elevator/ElevatorSubsystemCommands.javasubsystems/elevator/ElevatorSubsystemConfiguration.javasubsystems/flywheel/SimpleMotorSubsystem.javasubsystems/flywheel/SimpleMotorSubsystemCommands.javasubsystems/flywheel/SimpleMotorSubsystemConfiguration.java
🧰 Additional context used
📓 Path-based instructions (1)
**/*
⚙️ CodeRabbit configuration file
**/*: ## Review Style
- Explain issues clearly, but don't over-explain. Don't write anything unnecessary or repetetive
- Suggest a solution when helpful
- No code blocks unless they genuinely add clarity - GitHub already shows the diff
- Don't describe what the code does - focus on what could be improved
Naming & Readability
- Variable, function, and class names must be descriptive and intention-revealing
- No abbreviations unless universally understood (e.g., id, url)
- Boolean names should read as questions (isValid, hasPermission, canExecute)
- Functions should be verbs, classes should be nouns
- Magic numbers and strings should be named constants
- Code should be self-documenting; comments should explain "why", not "what"
Clean Code Principles
- Functions should do one thing and be small
- Too many parameters? Suggest grouping into an object
- Flag deep nesting - suggest early returns or extraction
- DRY: flag duplicated logic
- Functions with non-obvious side effects
- Single responsibility for classes and modules
- Proper abstraction levels - no mixing high and low level logic
Logic & Correctness
- Off-by-one errors, null/undefined handling, race conditions
- Unhandled edge cases
- Incorrect boolean logic or conditions
- Resource leaks, missing cleanup
- Error handling gaps
- Performance issues when significant
Code Structure
- File and folder organization
- Proper separation of concerns
- Dependency management and coupling
Files:
subsystems/arm/ArmSubsystemCommands.javasubsystems/elevator/ElevatorSubsystemConfiguration.javasubsystems/arm/ArmSubsystemConfiguration.javasubsystems/elevator/ElevatorSubsystemCommands.javasubsystems/arm/ArmSubsystem.javasubsystems/flywheel/SimpleMotorSubsystemCommands.javasubsystems/flywheel/SimpleMotorSubsystem.javasubsystems/elevator/ElevatorSubsystem.javasubsystems/flywheel/SimpleMotorSubsystemConfiguration.java
🧬 Code graph analysis (5)
subsystems/arm/ArmSubsystemCommands.java (3)
commands/ExecuteEndCommand.java (1)
ExecuteEndCommand(9-21)commands/GearRatioCalculationCommand.java (1)
GearRatioCalculationCommand(20-183)commands/NetworkTablesCommand.java (1)
NetworkTablesCommand(16-139)
subsystems/elevator/ElevatorSubsystemCommands.java (2)
commands/ExecuteEndCommand.java (1)
ExecuteEndCommand(9-21)commands/NetworkTablesCommand.java (1)
NetworkTablesCommand(16-139)
subsystems/flywheel/SimpleMotorSubsystemCommands.java (2)
commands/ExecuteEndCommand.java (1)
ExecuteEndCommand(9-21)commands/NetworkTablesCommand.java (1)
NetworkTablesCommand(16-139)
subsystems/flywheel/SimpleMotorSubsystem.java (4)
hardware/phoenix6/talonfx/TalonFXMotor.java (1)
TalonFXMotor(20-209)hardware/simulation/SimpleMotorSimulation.java (1)
SimpleMotorSimulation(12-81)subsystems/MotorSubsystem.java (1)
MotorSubsystem(24-171)utilities/mechanisms/SpeedMechanism2d.java (1)
SpeedMechanism2d(12-129)
subsystems/elevator/ElevatorSubsystem.java (5)
hardware/phoenix6/talonfx/TalonFXMotor.java (1)
TalonFXMotor(20-209)hardware/simulation/ElevatorSimulation.java (1)
ElevatorSimulation(11-84)subsystems/MotorSubsystem.java (1)
MotorSubsystem(24-171)utilities/Conversions.java (1)
Conversions(8-258)utilities/mechanisms/ElevatorMechanism2d.java (1)
ElevatorMechanism2d(13-84)
🔇 Additional comments (6)
subsystems/elevator/ElevatorSubsystem.java (2)
118-127: LGTM!State comparison and tolerance checking logic is correct. Using reference equality (
==) fortargetStateis intentional for enum-like state comparisons.
166-172: Verify stage indexing doesn't divide by zero.
targetStage + 1is used as a divisor. Since the loop starts ati = 0,targetStage + 1is always at least 1, so this is safe. However, the division logicgetPositionMeters() / (targetStage + 1)for multi-stage elevators may not produce expected results—stage 0 gets full position, stage 1 gets half, etc. Confirm this matches the intended kinematic model.subsystems/arm/ArmSubsystemConfiguration.java (1)
7-81: LGTM overall—configuration data class is straightforward.Public mutable fields are typical for FRC configuration classes. The defaults are documented where non-obvious. Consider adding brief Javadoc for
gearRatioandfocEnabledto match the style of other fields.subsystems/arm/ArmSubsystemCommands.java (1)
33-47: LGTM—state commands are well-structured.Clean use of
ExecuteEndCommandwith proper stop behavior on end. The overloaded methods provide good flexibility.subsystems/arm/ArmSubsystem.java (2)
34-64: Constructor is well-organized.Good separation of configuration extraction, request initialization, simulation setup, and mechanism creation. The physics simulation wiring with
SingleJointedArmSimulationlooks correct.
86-92: Logging and update methods look correct.Proper use of AdvantageKit logging with appropriate signals. The
updatePeriodicallymethod correctly updates motor state before logging.Also applies to: 104-108
| public boolean atState(ArmState targetState) { | ||
| return this.targetState == targetState && atTargetState(); | ||
| } |
There was a problem hiding this comment.
Reference equality (==) for state comparison may fail for non-enum states.
Using == works if ArmState is always an enum, but fails for other implementations. If the interface is intended for enums only, document it. Otherwise, use .equals() or compare by value.
| public boolean atTargetState() { | ||
| if (targetState == null) | ||
| return false; | ||
| final double currentToTargetStateDifferenceDegrees = Math.abs(targetState.getTargetAngle().minus(getAngle()).getDegrees()); | ||
| return currentToTargetStateDifferenceDegrees < angleTolerance.getDegrees(); | ||
| } |
There was a problem hiding this comment.
Strict < comparison with tolerance may cause false negatives.
Using < instead of <= means being exactly at the tolerance boundary returns false. Combined with angleTolerance defaulting to zero, atTargetState() is effectively always false unless tolerance is explicitly set.
Suggested fix
- return currentToTargetStateDifferenceDegrees < angleTolerance.getDegrees();
+ return currentToTargetStateDifferenceDegrees <= angleTolerance.getDegrees();📝 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.
| public boolean atTargetState() { | |
| if (targetState == null) | |
| return false; | |
| final double currentToTargetStateDifferenceDegrees = Math.abs(targetState.getTargetAngle().minus(getAngle()).getDegrees()); | |
| return currentToTargetStateDifferenceDegrees < angleTolerance.getDegrees(); | |
| } | |
| public boolean atTargetState() { | |
| if (targetState == null) | |
| return false; | |
| final double currentToTargetStateDifferenceDegrees = Math.abs(targetState.getTargetAngle().minus(getAngle()).getDegrees()); | |
| return currentToTargetStateDifferenceDegrees <= angleTolerance.getDegrees(); | |
| } |
| private void scalePositionRequestSpeed(double speedScalar) { | ||
| positionRequest.Velocity = maximumVelocity * speedScalar; | ||
| positionRequest.Acceleration = maximumAcceleration * speedScalar; | ||
| positionRequest.Jerk = maximumJerk * speedScalar; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Directly mutating positionRequest fields is not thread-safe.
While likely fine in single-threaded robot code, mutating shared request objects could cause issues if methods are called from different contexts (e.g., command scheduling). Consider creating a new request or documenting the threading assumptions.
| /** | ||
| * An interface for an arm state. | ||
| * getTargetAngle represents the target angle of the state. | ||
| * getSpeedScalar represents the speed scalar of the state. | ||
| */ | ||
| public interface ArmState { | ||
| Rotation2d getTargetAngle(); | ||
|
|
||
| ArmState getPrepareState(); | ||
|
|
||
| double getSpeedScalar(); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Interface design is clean.
The ArmState interface is minimal and well-documented. Consider whether getPrepareState() should have a default implementation returning this to avoid null issues.
| public static Command getDebuggingCommand(ArmSubsystem subsystem) { | ||
| return new NetworkTablesCommand( | ||
| (targetAngleDegrees, speedScalar) -> subsystem.setTargetState(Rotation2d.fromDegrees(targetAngleDegrees), speedScalar), | ||
| false, | ||
| Set.of(subsystem), | ||
| subsystem.getName() + "TargetAngleDegrees", | ||
| subsystem.getName() + "SpeedScalar" | ||
| ); | ||
| } |
There was a problem hiding this comment.
runPeriodically=false seems wrong for a debugging command.
With runPeriodically=false, the command reads NetworkTables values only once on initialize(), ignoring any subsequent changes. For debugging/tuning, you'd typically want continuous updates.
Suggested fix
return new NetworkTablesCommand(
(targetAngleDegrees, speedScalar) -> subsystem.setTargetState(Rotation2d.fromDegrees(targetAngleDegrees), speedScalar),
- false,
+ true,
Set.of(subsystem),📝 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.
| public static Command getDebuggingCommand(ArmSubsystem subsystem) { | |
| return new NetworkTablesCommand( | |
| (targetAngleDegrees, speedScalar) -> subsystem.setTargetState(Rotation2d.fromDegrees(targetAngleDegrees), speedScalar), | |
| false, | |
| Set.of(subsystem), | |
| subsystem.getName() + "TargetAngleDegrees", | |
| subsystem.getName() + "SpeedScalar" | |
| ); | |
| } | |
| public static Command getDebuggingCommand(ArmSubsystem subsystem) { | |
| return new NetworkTablesCommand( | |
| (targetAngleDegrees, speedScalar) -> subsystem.setTargetState(Rotation2d.fromDegrees(targetAngleDegrees), speedScalar), | |
| true, | |
| Set.of(subsystem), | |
| subsystem.getName() + "TargetAngleDegrees", | |
| subsystem.getName() + "SpeedScalar" | |
| ); | |
| } |
|
|
||
| import java.util.Set; | ||
|
|
||
| public class SimpleMotorSubsystemCommands { |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Make this a proper utility class.
Since this class contains only static methods, make it final and add a private constructor to prevent instantiation.
| public static Command getPrepareTargetStateCommand(SimpleMotorSubsystem.SimpleMotorState targetState, SimpleMotorSubsystem subsystem) { | ||
| return new ExecuteEndCommand( | ||
| () -> subsystem.setTargetState(targetState.getPrepareState()), | ||
| subsystem::stop, | ||
| subsystem | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "*.java" | head -20Repository: Programming-TRIGON/TRIGONLib
Length of output: 885
🏁 Script executed:
fd -e java | xargs grep -l "SimpleMotorState"Repository: Programming-TRIGON/TRIGONLib
Length of output: 171
🏁 Script executed:
fd -e java | xargs grep -l "class SimpleMotorSubsystem"Repository: Programming-TRIGON/TRIGONLib
Length of output: 230
🏁 Script executed:
fd -e java -path "*flywheel*" | head -20Repository: Programming-TRIGON/TRIGONLib
Length of output: 240
🏁 Script executed:
cat -n subsystems/flywheel/SimpleMotorSubsystem.javaRepository: Programming-TRIGON/TRIGONLib
Length of output: 7258
🏁 Script executed:
cat -n subsystems/flywheel/SimpleMotorSubsystemCommands.javaRepository: Programming-TRIGON/TRIGONLib
Length of output: 1917
Add null check for getPrepareState() result.
If targetState.getPrepareState() returns null, setTargetState(null) will throw NPE when it calls getTargetUnit(). The interface doesn't guarantee non-null, and other code in the subsystem handles null targetState (e.g., atTargetState(), updateMechanism()). Either:
- Add a null check here:
SimpleMotorState prepareState = targetState.getPrepareState(); if (prepareState != null) subsystem.setTargetState(prepareState); - Or document that
getPrepareState()never returns null with@Nonnull
| public class SimpleMotorSubsystemConfiguration { | ||
| public String name = ""; | ||
| public double | ||
| gearRatio = 1, | ||
| /** | ||
| * The moment of inertia of the motor | ||
| */ | ||
| momentOfInertia = 0.003, | ||
| /** | ||
| * Maximum velocity of the motor. | ||
| */ | ||
| maximumVelocity = 1, | ||
| /** | ||
| * Maximum acceleration of the motor. | ||
| */ | ||
| maximumAcceleration = 1, | ||
| /** | ||
| * Maximum jerk of the motor. | ||
| */ | ||
| maximumJerk = 1, | ||
| /** | ||
| * Maximum velocity displayed by the mechanism 2D | ||
| */ | ||
| maximumDisplayableVelocity = 1, | ||
| /** | ||
| * The acceptable tolerance between the target velocity and current velocity. | ||
| */ | ||
| velocityTolerance = 1, | ||
| /** | ||
| * Ramp rate used in system identification. | ||
| * See <a href="https://docs.wpilib.org/en/stable/docs/software/advanced-controls/system-identification/introduction.html"> | ||
| * WPILib System Identification Introduction</a>. | ||
| */ | ||
| sysIDRampRate = 1, | ||
| /** | ||
| * Step voltage used in system identification. | ||
| * See <a href="https://docs.wpilib.org/en/stable/docs/software/advanced-controls/system-identification/introduction.html"> | ||
| * WPILib System Identification Introduction</a>. | ||
| */ | ||
| sysIDStepVoltage = 1; | ||
| public boolean focEnabled = true; | ||
| /** | ||
| * should the motor control mode be Voltage as opposed to Velocity. | ||
| */ | ||
| public boolean shouldUseVoltageControl = false; | ||
| /** | ||
| * The type and amount of motors to be used in the simulation. | ||
| */ | ||
| public DCMotor gearbox = DCMotor.getKrakenX60Foc(1); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Encapsulate configuration fields.
Public mutable fields prevent validation and make it easy to create invalid configurations. Consider making fields final with a builder pattern, or use a constructor that validates inputs.
| public String name = ""; | ||
| public double | ||
| gearRatio = 1, | ||
| /** | ||
| * The moment of inertia of the motor | ||
| */ | ||
| momentOfInertia = 0.003, | ||
| /** | ||
| * Maximum velocity of the motor. | ||
| */ | ||
| maximumVelocity = 1, | ||
| /** | ||
| * Maximum acceleration of the motor. | ||
| */ | ||
| maximumAcceleration = 1, | ||
| /** | ||
| * Maximum jerk of the motor. | ||
| */ | ||
| maximumJerk = 1, | ||
| /** | ||
| * Maximum velocity displayed by the mechanism 2D | ||
| */ | ||
| maximumDisplayableVelocity = 1, | ||
| /** | ||
| * The acceptable tolerance between the target velocity and current velocity. | ||
| */ | ||
| velocityTolerance = 1, | ||
| /** | ||
| * Ramp rate used in system identification. | ||
| * See <a href="https://docs.wpilib.org/en/stable/docs/software/advanced-controls/system-identification/introduction.html"> | ||
| * WPILib System Identification Introduction</a>. | ||
| */ | ||
| sysIDRampRate = 1, | ||
| /** | ||
| * Step voltage used in system identification. | ||
| * See <a href="https://docs.wpilib.org/en/stable/docs/software/advanced-controls/system-identification/introduction.html"> | ||
| * WPILib System Identification Introduction</a>. | ||
| */ | ||
| sysIDStepVoltage = 1; |
There was a problem hiding this comment.
Questionable default values.
Several defaults appear arbitrary:
- Empty string for
namemakes debugging difficult - Value of
1for physical parameters (velocity, acceleration, jerk, tolerance, etc.) is unlikely to be correct for real hardware
Either require these values in a constructor or provide physically meaningful defaults with documentation explaining the assumptions.
| public double | ||
| gearRatio = 1, | ||
| /** | ||
| * The moment of inertia of the motor | ||
| */ | ||
| momentOfInertia = 0.003, | ||
| /** | ||
| * Maximum velocity of the motor. | ||
| */ | ||
| maximumVelocity = 1, | ||
| /** | ||
| * Maximum acceleration of the motor. | ||
| */ | ||
| maximumAcceleration = 1, | ||
| /** | ||
| * Maximum jerk of the motor. | ||
| */ | ||
| maximumJerk = 1, | ||
| /** | ||
| * Maximum velocity displayed by the mechanism 2D | ||
| */ | ||
| maximumDisplayableVelocity = 1, | ||
| /** | ||
| * The acceptable tolerance between the target velocity and current velocity. | ||
| */ | ||
| velocityTolerance = 1, | ||
| /** | ||
| * Ramp rate used in system identification. | ||
| * See <a href="https://docs.wpilib.org/en/stable/docs/software/advanced-controls/system-identification/introduction.html"> | ||
| * WPILib System Identification Introduction</a>. | ||
| */ | ||
| sysIDRampRate = 1, | ||
| /** | ||
| * Step voltage used in system identification. | ||
| * See <a href="https://docs.wpilib.org/en/stable/docs/software/advanced-controls/system-identification/introduction.html"> | ||
| * WPILib System Identification Introduction</a>. | ||
| */ | ||
| sysIDStepVoltage = 1; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Separate field declarations for readability.
Declaring multiple fields in a single compound statement with interspersed Javadoc makes the code harder to read and maintain. Declare each field separately with its own Javadoc comment.
There was a problem hiding this comment.
Actionable comments posted: 13
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
subsystems/arm/ArmSubsystem.javasubsystems/elevator/ElevatorSubsystem.javasubsystems/flywheel/SimpleMotorSubsystem.java
🧰 Additional context used
📓 Path-based instructions (1)
**/*
⚙️ CodeRabbit configuration file
**/*: ## Review Style
- Explain issues clearly, but don't over-explain. Don't write anything unnecessary or repetetive
- Suggest a solution when helpful
- No code blocks unless they genuinely add clarity - GitHub already shows the diff
- Don't describe what the code does - focus on what could be improved
Naming & Readability
- Variable, function, and class names must be descriptive and intention-revealing
- No abbreviations unless universally understood (e.g., id, url)
- Boolean names should read as questions (isValid, hasPermission, canExecute)
- Functions should be verbs, classes should be nouns
- Magic numbers and strings should be named constants
- Code should be self-documenting; comments should explain "why", not "what"
Clean Code Principles
- Functions should do one thing and be small
- Too many parameters? Suggest grouping into an object
- Flag deep nesting - suggest early returns or extraction
- DRY: flag duplicated logic
- Functions with non-obvious side effects
- Single responsibility for classes and modules
- Proper abstraction levels - no mixing high and low level logic
Logic & Correctness
- Off-by-one errors, null/undefined handling, race conditions
- Unhandled edge cases
- Incorrect boolean logic or conditions
- Resource leaks, missing cleanup
- Error handling gaps
- Performance issues when significant
Code Structure
- File and folder organization
- Proper separation of concerns
- Dependency management and coupling
Files:
subsystems/elevator/ElevatorSubsystem.javasubsystems/flywheel/SimpleMotorSubsystem.javasubsystems/arm/ArmSubsystem.java
🧬 Code graph analysis (2)
subsystems/elevator/ElevatorSubsystem.java (5)
hardware/phoenix6/talonfx/TalonFXMotor.java (1)
TalonFXMotor(20-209)hardware/simulation/ElevatorSimulation.java (1)
ElevatorSimulation(11-84)subsystems/MotorSubsystem.java (1)
MotorSubsystem(24-171)utilities/Conversions.java (1)
Conversions(8-258)utilities/mechanisms/ElevatorMechanism2d.java (1)
ElevatorMechanism2d(13-84)
subsystems/arm/ArmSubsystem.java (3)
hardware/RobotHardwareStats.java (1)
RobotHardwareStats(6-78)hardware/simulation/SingleJointedArmSimulation.java (1)
SingleJointedArmSimulation(12-81)utilities/mechanisms/SingleJointedArmMechanism2d.java (1)
SingleJointedArmMechanism2d(14-87)
🔇 Additional comments (1)
subsystems/flywheel/SimpleMotorSubsystem.java (1)
116-123: Verify voltage control mode always returns true at target.In voltage control mode (lines 120-121),
atTargetState()immediately returnstruewithout checking if the motor has reached any particular state. Confirm this is intentional—it means commands waiting onatTargetState()will complete instantly in voltage mode.
| motor.setPhysicsSimulation( | ||
| new SingleJointedArmSimulation( | ||
| config.gearbox, | ||
| config.gearRatio, | ||
| config.massKilograms, | ||
| config.lengthMeters, | ||
| config.minimumAngle, | ||
| config.maximumAngle, | ||
| config.shouldSimulateGravity | ||
| )); |
There was a problem hiding this comment.
Critical: Arguments swapped in simulation constructor.
Lines 56-57 pass config.massKilograms and config.lengthMeters in the wrong order. The SingleJointedArmSimulation constructor expects armLengthMeters as the 3rd parameter and armMassKilograms as the 4th, but you're passing mass before length. This will break the physics simulation.
🐛 Fix the argument order
motor.setPhysicsSimulation(
new SingleJointedArmSimulation(
config.gearbox,
config.gearRatio,
- config.massKilograms,
config.lengthMeters,
+ config.massKilograms,
config.minimumAngle,
config.maximumAngle,
config.shouldSimulateGravity
));| @Override | ||
| public String getName() { | ||
| return ""; | ||
| } |
There was a problem hiding this comment.
Critical: getName() returns empty string instead of name.
This method should return the name field, not an empty string. The current implementation breaks the naming contract and makes the subsystem name inaccessible.
🐛 Return the name field
@Override
public String getName() {
- return "";
+ return name;
}📝 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.
| @Override | |
| public String getName() { | |
| return ""; | |
| } | |
| @Override | |
| public String getName() { | |
| return name; | |
| } |
| @Override | ||
| public SysIdRoutine.Config getSysIDConfig() { | ||
| if (sysIDConfig != null) | ||
| return sysIDConfig; | ||
| return new SysIdRoutine.Config(); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove unnecessary null check.
The sysIDConfig field is final and always initialized in the constructor, so the null check is unnecessary. This simplifies the method.
♻️ Simplify the method
@Override
public SysIdRoutine.Config getSysIDConfig() {
- if (sysIDConfig != null)
- return sysIDConfig;
- return new SysIdRoutine.Config();
+ return sysIDConfig;
}|
|
||
| mechanism.update( | ||
| getAngle(), | ||
| Rotation2d.fromRotations(motor.getSignal(TalonFXSignal.CLOSED_LOOP_REFERENCE) + (RobotHardwareStats.isSimulation() ? 0 : visualizationOffsetFromGravityOffset)) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Extract duplicated offset calculation.
The expression RobotHardwareStats.isSimulation() ? 0 : visualizationOffsetFromGravityOffset appears three times (lines 102, 129, 161). Extract it to a private helper method to reduce duplication and improve maintainability.
♻️ Extract to helper method
Add a helper method:
private double getVisualizationOffset() {
return RobotHardwareStats.isSimulation() ? 0 : visualizationOffsetFromGravityOffset;
}Then update the three call sites:
- Rotation2d.fromRotations(motor.getSignal(TalonFXSignal.CLOSED_LOOP_REFERENCE) + (RobotHardwareStats.isSimulation() ? 0 : visualizationOffsetFromGravityOffset))
+ Rotation2d.fromRotations(motor.getSignal(TalonFXSignal.CLOSED_LOOP_REFERENCE) + getVisualizationOffset())- return Rotation2d.fromRotations(motor.getSignal(TalonFXSignal.POSITION) + (RobotHardwareStats.isSimulation() ? 0 : visualizationOffsetFromGravityOffset));
+ return Rotation2d.fromRotations(motor.getSignal(TalonFXSignal.POSITION) + getVisualizationOffset());- setControl(positionRequest.withPosition(targetAngle.getRotations() - (RobotHardwareStats.isSimulation() ? 0 : visualizationOffsetFromGravityOffset)));
+ setControl(positionRequest.withPosition(targetAngle.getRotations() - getVisualizationOffset()));Also applies to: 129-129, 161-161
| public ElevatorSubsystem(TalonFXMotor motor, ElevatorSubsystemConfiguration config, Pose3d... stagesOriginPoints) { | ||
| this.motor = motor; | ||
| this.stagesOriginPoints = stagesOriginPoints; | ||
|
|
There was a problem hiding this comment.
Missing null check on stagesOriginPoints parameter.
The constructor accepts stagesOriginPoints without validation. If null is passed, logComponentPoses() at line 164 will throw NPE when accessing .length.
🛡️ Add validation in constructor
public ElevatorSubsystem(TalonFXMotor motor, ElevatorSubsystemConfiguration config, Pose3d... stagesOriginPoints) {
+ if (stagesOriginPoints == null)
+ throw new IllegalArgumentException("stagesOriginPoints cannot be null");
this.motor = motor;
this.stagesOriginPoints = stagesOriginPoints;Alternatively, default to an empty array:
public ElevatorSubsystem(TalonFXMotor motor, ElevatorSubsystemConfiguration config, Pose3d... stagesOriginPoints) {
this.motor = motor;
- this.stagesOriginPoints = stagesOriginPoints;
+ this.stagesOriginPoints = stagesOriginPoints != null ? stagesOriginPoints : new Pose3d[0];| @Override | ||
| public void updateMechanism() { | ||
| logComponentPoses(); | ||
|
|
||
| mechanism.update( | ||
| getPositionRotations(), | ||
| motor.getSignal(TalonFXSignal.CLOSED_LOOP_REFERENCE) | ||
| ); | ||
| } |
There was a problem hiding this comment.
Unit mismatch: mechanism.update() expects meters for both parameters.
Lines 104-105 pass rotations to mechanism.update(), but ElevatorMechanism2d expects both current and target position in meters (see setCurrentPosition in ElevatorMechanism2d.java line 73).
🐛 Fix: Convert both positions to meters
@Override
public void updateMechanism() {
logComponentPoses();
mechanism.update(
- getPositionRotations(),
- motor.getSignal(TalonFXSignal.CLOSED_LOOP_REFERENCE)
+ getPositionMeters(),
+ rotationsToMeters(motor.getSignal(TalonFXSignal.CLOSED_LOOP_REFERENCE))
);
}Committable suggestion skipped: line range outside the PR's diff.
| private final double | ||
| maximumVelocity, | ||
| maximumAcceleration, | ||
| maximumJerk; |
There was a problem hiding this comment.
Remove unused fields.
The fields maximumVelocity, maximumAcceleration, and maximumJerk are initialized from config but never used. Remove them to reduce clutter.
| @Override | ||
| public String getName() { | ||
| return ""; | ||
| } |
There was a problem hiding this comment.
Return the actual name instead of empty string.
Line 55 calls setName(name), but this override returns "". Return the name field instead.
🐛 Proposed fix
@Override
public String getName() {
- return "";
+ return name;
}📝 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.
| @Override | |
| public String getName() { | |
| return ""; | |
| } | |
| @Override | |
| public String getName() { | |
| return name; | |
| } |
| @Override | ||
| public SysIdRoutine.Config getSysIDConfig() { | ||
| if (sysIDConfig != null) | ||
| return sysIDConfig; | ||
| return new SysIdRoutine.Config(); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove unnecessary null check.
sysIDConfig is always initialized in the constructor and never set to null. The null check on line 65 is unreachable.
♻️ Simplify
@Override
public SysIdRoutine.Config getSysIDConfig() {
- if (sysIDConfig != null)
- return sysIDConfig;
- return new SysIdRoutine.Config();
+ return sysIDConfig;
}| public interface SimpleMotorState { | ||
| double getTargetUnit(); | ||
|
|
||
| SimpleMotorState getPrepareState(); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Document getPrepareState() purpose.
The getPrepareState() method in the public interface has no documentation explaining when or why it's used. Add a javadoc comment.
No description provided.