Subsystem Wrapper #27
Conversation
|
This should be done in the lib repo |
levyishai
left a comment
levyishai
left a comment
There was a problem hiding this comment.
This looks nice
Should do some testing, and show an example class in the group chat for a working arm with 2 motors
|
Move to lib! |
WalkthroughAdds three new subsystems (FlyWheel, Arm, Elevator) and their corresponding Constants/configuration classes. RobotContainer now exposes a public static FLY_WHEEL and binds a default REST command plus an INTAKE trigger command. MotorSubsystem imports moved to 🚥 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: 21
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (13)
src/main/java/frc/trigon/robot/RobotContainer.javasrc/main/java/frc/trigon/robot/commands/commandfactories/GeneralCommands.javasrc/main/java/frc/trigon/robot/subsystems/swerve/Swerve.javasrc/main/java/lib/subsystems/MotorSubsystem.javasrc/main/java/lib/subsystems/arm/ArmSubsystem.javasrc/main/java/lib/subsystems/arm/ArmSubsystemCommands.javasrc/main/java/lib/subsystems/arm/ArmSubsystemConfiguration.javasrc/main/java/lib/subsystems/elevator/ElevatorSubsystem.javasrc/main/java/lib/subsystems/elevator/ElevatorSubsystemCommands.javasrc/main/java/lib/subsystems/elevator/ElevatorSubsystemConfiguration.javasrc/main/java/lib/subsystems/flywheel/SimpleMotorSubsystem.javasrc/main/java/lib/subsystems/flywheel/SimpleMotorSubsystemCommands.javasrc/main/java/lib/subsystems/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:
src/main/java/lib/subsystems/MotorSubsystem.javasrc/main/java/lib/subsystems/flywheel/SimpleMotorSubsystemConfiguration.javasrc/main/java/lib/subsystems/elevator/ElevatorSubsystemConfiguration.javasrc/main/java/frc/trigon/robot/subsystems/swerve/Swerve.javasrc/main/java/lib/subsystems/flywheel/SimpleMotorSubsystemCommands.javasrc/main/java/lib/subsystems/arm/ArmSubsystemConfiguration.javasrc/main/java/lib/subsystems/elevator/ElevatorSubsystemCommands.javasrc/main/java/lib/subsystems/arm/ArmSubsystem.javasrc/main/java/lib/subsystems/arm/ArmSubsystemCommands.javasrc/main/java/frc/trigon/robot/commands/commandfactories/GeneralCommands.javasrc/main/java/frc/trigon/robot/RobotContainer.javasrc/main/java/lib/subsystems/elevator/ElevatorSubsystem.javasrc/main/java/lib/subsystems/flywheel/SimpleMotorSubsystem.java
🧠 Learnings (2)
📚 Learning: 2025-12-30T00:12:14.902Z
Learnt from: levyishai
Repo: Programming-TRIGON/RobotTemplate PR: 45
File: src/main/java/frc/trigon/robot/poseestimation/poseestimator/ObjectPoseEstimator.java:161-187
Timestamp: 2025-12-30T00:12:14.902Z
Learning: In the Programming-TRIGON/RobotTemplate Java codebase, follow Uncle Bob's Clean Code principles: prefer self-documenting code through descriptive naming and small, focused methods (method extraction) over adding explanatory comments. Ensure classes, methods, and variables convey intent (naming should be expressive and consistent). Use comments only to explain non-obvious decisions or workarounds, not as a substitute for readable code.
Applied to files:
src/main/java/lib/subsystems/MotorSubsystem.javasrc/main/java/lib/subsystems/flywheel/SimpleMotorSubsystemConfiguration.javasrc/main/java/lib/subsystems/elevator/ElevatorSubsystemConfiguration.javasrc/main/java/frc/trigon/robot/subsystems/swerve/Swerve.javasrc/main/java/lib/subsystems/flywheel/SimpleMotorSubsystemCommands.javasrc/main/java/lib/subsystems/arm/ArmSubsystemConfiguration.javasrc/main/java/lib/subsystems/elevator/ElevatorSubsystemCommands.javasrc/main/java/lib/subsystems/arm/ArmSubsystem.javasrc/main/java/lib/subsystems/arm/ArmSubsystemCommands.javasrc/main/java/frc/trigon/robot/commands/commandfactories/GeneralCommands.javasrc/main/java/frc/trigon/robot/RobotContainer.javasrc/main/java/lib/subsystems/elevator/ElevatorSubsystem.javasrc/main/java/lib/subsystems/flywheel/SimpleMotorSubsystem.java
📚 Learning: 2025-12-30T20:34:50.231Z
Learnt from: Strflightmight09
Repo: Programming-TRIGON/RobotTemplate PR: 48
File: src/main/java/frc/trigon/robot/subsystems/swerve/Swerve.java:180-183
Timestamp: 2025-12-30T20:34:50.231Z
Learning: In template/ scaffolding repositories (e.g., RobotTemplate), allow and preserve optional code patterns that are commented out for users to enable, rather than removing or fully implementing them. During reviews, treat commented-out blocks as intentional placeholders to be potentially enabled by users; avoid enforcing removal of such code and ensure commented sections are clearly documented as optional templates.
Applied to files:
src/main/java/lib/subsystems/MotorSubsystem.javasrc/main/java/lib/subsystems/flywheel/SimpleMotorSubsystemConfiguration.javasrc/main/java/lib/subsystems/elevator/ElevatorSubsystemConfiguration.javasrc/main/java/frc/trigon/robot/subsystems/swerve/Swerve.javasrc/main/java/lib/subsystems/flywheel/SimpleMotorSubsystemCommands.javasrc/main/java/lib/subsystems/arm/ArmSubsystemConfiguration.javasrc/main/java/lib/subsystems/elevator/ElevatorSubsystemCommands.javasrc/main/java/lib/subsystems/arm/ArmSubsystem.javasrc/main/java/lib/subsystems/arm/ArmSubsystemCommands.javasrc/main/java/frc/trigon/robot/commands/commandfactories/GeneralCommands.javasrc/main/java/frc/trigon/robot/RobotContainer.javasrc/main/java/lib/subsystems/elevator/ElevatorSubsystem.javasrc/main/java/lib/subsystems/flywheel/SimpleMotorSubsystem.java
🧬 Code graph analysis (5)
src/main/java/frc/trigon/robot/subsystems/swerve/Swerve.java (1)
src/main/java/lib/subsystems/MotorSubsystem.java (1)
MotorSubsystem(24-171)
src/main/java/frc/trigon/robot/commands/commandfactories/GeneralCommands.java (1)
src/main/java/lib/subsystems/MotorSubsystem.java (1)
MotorSubsystem(24-171)
src/main/java/frc/trigon/robot/RobotContainer.java (1)
src/main/java/lib/subsystems/MotorSubsystem.java (1)
MotorSubsystem(24-171)
src/main/java/lib/subsystems/elevator/ElevatorSubsystem.java (1)
src/main/java/lib/subsystems/MotorSubsystem.java (1)
MotorSubsystem(24-171)
src/main/java/lib/subsystems/flywheel/SimpleMotorSubsystem.java (1)
src/main/java/lib/subsystems/MotorSubsystem.java (1)
MotorSubsystem(24-171)
🪛 GitHub Actions: Java CI with Gradle
src/main/java/lib/subsystems/arm/ArmSubsystem.java
[error] 11-19: package lib.hardware does not exist; TalonFXMotor and related imports not found. Compilation cannot proceed.
src/main/java/lib/subsystems/arm/ArmSubsystemCommands.java
[error] 5-7: package lib.commands does not exist; related command imports fail to resolve.
src/main/java/lib/subsystems/elevator/ElevatorSubsystem.java
[error] 14-22: import lib.hardware.phoenix6.talonfx.TalonFXMotor; and related classes not found; motor and mechanism symbols unavailable.
src/main/java/lib/subsystems/flywheel/SimpleMotorSubsystem.java
[error] 10-32: Imports refer to missing lib.hardware/lib.utilities packages; TalonFXMotor, TalonFXSignal, SimpleMotorSimulation, SpeedMechanism2d unresolved; compilation fails.
🔇 Additional comments (9)
src/main/java/frc/trigon/robot/subsystems/swerve/Swerve.java (1)
22-22: LGTM - Import path correctly updatedThe import path change aligns with the MotorSubsystem migration to lib.subsystems.
src/main/java/frc/trigon/robot/RobotContainer.java (1)
23-23: LGTM - Import path correctly updatedsrc/main/java/lib/subsystems/MotorSubsystem.java (1)
1-1: LGTM - Package declaration correctly updatedThe move to lib.subsystems aligns with the library architecture pattern.
src/main/java/frc/trigon/robot/commands/commandfactories/GeneralCommands.java (1)
7-7: LGTM - Import path correctly updatedsrc/main/java/lib/subsystems/flywheel/SimpleMotorSubsystemConfiguration.java (1)
11-11: Verify default values are appropriate placeholdersMost physics parameters default to 1, which is unlikely to be correct for real mechanisms. If these are meant as placeholders in a template, add a comment at the class level explaining that users must configure these values for their specific subsystem.
Also applies to: 19-19, 23-23, 27-27, 31-31, 35-35, 41-41, 47-47
src/main/java/lib/subsystems/elevator/ElevatorSubsystem.java (2)
97-105: Verify mechanism expects rotations vs meters.
mechanism.update()receivesgetPositionRotations()but given this is an elevator with linear motion, confirmElevatorMechanism2dexpects rotations rather than meters. If it expects meters, usegetPositionMeters().
182-193:getPrepareState()declared but never used in this class.The
ElevatorStateinterface declaresgetPrepareState()but it's not called anywhere inElevatorSubsystem. Verify this is intentional for external command usage, or document the expected usage pattern.src/main/java/lib/subsystems/arm/ArmSubsystem.java (1)
174-185:getPrepareState()declared but unused inArmSubsystem.Same pattern as
ElevatorSubsystem- the interface method exists but isn't called within this class. Consistent withElevatorState, but verify the intended usage.src/main/java/lib/subsystems/flywheel/SimpleMotorSubsystem.java (1)
16-56: Structural consistency: Good alignment with other subsystems.The constructor pattern, field initialization, and SysID configuration mirror
ArmSubsystemandElevatorSubsystem. This consistency aids maintainability.
| public String name = ""; | ||
|
|
||
| /** | ||
| * Length of the arm in meters. | ||
| */ | ||
| public double lengthMeters = 1; | ||
|
|
||
| /** | ||
| * 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; | ||
|
|
||
| /** | ||
| * 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>. | ||
| */ | ||
| public double 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>. | ||
| */ | ||
| public double sysIDStepVoltage = 1; | ||
|
|
||
| public double gearRatio = 1; | ||
|
|
||
| /** | ||
| * Mass of the arm in kilograms. | ||
| */ | ||
| public double massKilograms = 1; | ||
|
|
||
| /** | ||
| * Visual offset for display in Mechanism2D. | ||
| */ | ||
| public double visualizationOffset = 1; | ||
|
|
||
| /** | ||
| * Maximum angle of the arm. | ||
| */ | ||
| public Rotation2d maximumAngle = Rotation2d.kZero; | ||
|
|
||
| /** | ||
| * Minimum angle of the arm. | ||
| */ | ||
| public Rotation2d minimumAngle = Rotation2d.kZero; | ||
|
|
||
| /** | ||
| * Acceptable angular tolerance for reaching the target. | ||
| */ | ||
| public Rotation2d angleTolerance = Rotation2d.kZero; | ||
|
|
||
| public boolean focEnabled = true; | ||
|
|
||
| public boolean shouldSimulateGravity = true; | ||
|
|
||
| public Color mechanismColor = Color.kBlue; | ||
|
|
||
| /** | ||
| * The type and number of motors used in the simulation. | ||
| */ | ||
| public DCMotor gearbox = DCMotor.getKrakenX60Foc(1); |
There was a problem hiding this comment.
All numeric defaults are placeholders requiring configuration.
All physics and control parameters default to 1, which won't produce realistic arm behavior. Users must configure these values (arm length, mass, motion constraints, gear ratio, etc.) based on their actual hardware before deployment.
Consider adding a class-level comment warning that these are template values.
There was a problem hiding this comment.
A warning isn't so important, but separate them into record or something so that they're not just 20 random variables one after the other in a single class
There was a problem hiding this comment.
@Strflightmight09 Good call. Grouping related parameters into nested records would make this much cleaner. Consider organizing like this:
Logical groups:
- MotionConstraints → maximumVelocity, maximumAcceleration, maximumJerk
- SysIdConfig → sysIDRampRate, sysIDStepVoltage
- AngleLimits → maximumAngle, minimumAngle, angleTolerance
- PhysicalProperties → lengthMeters, massKilograms, gearRatio, gearbox
- VisualizationConfig → visualizationOffset, mechanismColor
Then the main class becomes:
public class ArmSubsystemConfiguration {
public String name = "";
public MotionConstraints motionConstraints = new MotionConstraints();
public SysIdConfig sysIdConfig = new SysIdConfig();
public AngleLimits angleLimits = new AngleLimits();
public PhysicalProperties physicalProperties = new PhysicalProperties();
public VisualizationConfig visualizationConfig = new VisualizationConfig();
public boolean focEnabled = true;
public boolean shouldSimulateGravity = true;
// nested record definitions...
}This reduces cognitive load and makes it obvious which parameters are related. The same pattern should apply to ElevatorSubsystemConfiguration and SimpleMotorSubsystemConfiguration.
src/main/java/lib/subsystems/flywheel/SimpleMotorSubsystem.java
Outdated
Show resolved
Hide resolved
src/main/java/lib/subsystems/flywheel/SimpleMotorSubsystem.java
Outdated
Show resolved
Hide resolved
src/main/java/lib/subsystems/flywheel/SimpleMotorSubsystemCommands.java
Outdated
Show resolved
Hide resolved
| 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.
🛠️ Refactor suggestion | 🟠 Major
Use private fields with accessors or builder pattern
All fields are public, which prevents validation and makes future changes breaking. Consider making fields private with getters, or implement a builder pattern to enforce constraints (e.g., velocities > 0, gear ratio > 0).
| 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
One field per line for readability
Declaring multiple fields on the same line reduces readability. Split each field to its own line with consistent documentation.
Example:
- public double
- gearRatio = 1,
- /**
- * The moment of inertia of the motor
- */
- momentOfInertia = 0.003,
+ public double gearRatio = 1;
+
+ /**
+ * The moment of inertia of the motor
+ */
+ public double momentOfInertia = 0.003;Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 12
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (12)
.gitmodulessrc/main/java/frc/trigon/libsrc/main/java/frc/trigon/robot/RobotContainer.javasrc/main/java/frc/trigon/robot/commands/commandfactories/GeneralCommands.javasrc/main/java/frc/trigon/robot/subsystems/arm/Arm.javasrc/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.javasrc/main/java/frc/trigon/robot/subsystems/elevator/Elevator.javasrc/main/java/frc/trigon/robot/subsystems/elevator/ElevatorConstants.javasrc/main/java/frc/trigon/robot/subsystems/flyWheel/FlyWheel.javasrc/main/java/frc/trigon/robot/subsystems/flyWheel/FlyWheelConstants.javasrc/main/java/frc/trigon/robot/subsystems/swerve/Swerve.javasrc/main/python/keyboard_to_nt.py
🧰 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:
src/main/java/frc/trigon/libsrc/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.javasrc/main/java/frc/trigon/robot/commands/commandfactories/GeneralCommands.javasrc/main/java/frc/trigon/robot/subsystems/elevator/ElevatorConstants.javasrc/main/java/frc/trigon/robot/subsystems/flyWheel/FlyWheel.javasrc/main/java/frc/trigon/robot/subsystems/flyWheel/FlyWheelConstants.javasrc/main/java/frc/trigon/robot/subsystems/arm/Arm.javasrc/main/python/keyboard_to_nt.pysrc/main/java/frc/trigon/robot/RobotContainer.javasrc/main/java/frc/trigon/robot/subsystems/elevator/Elevator.javasrc/main/java/frc/trigon/robot/subsystems/swerve/Swerve.java
🧠 Learnings (3)
📚 Learning: 2025-12-30T00:12:14.902Z
Learnt from: levyishai
Repo: Programming-TRIGON/RobotTemplate PR: 45
File: src/main/java/frc/trigon/robot/poseestimation/poseestimator/ObjectPoseEstimator.java:161-187
Timestamp: 2025-12-30T00:12:14.902Z
Learning: In the Programming-TRIGON/RobotTemplate Java codebase, follow Uncle Bob's Clean Code principles: prefer self-documenting code through descriptive naming and small, focused methods (method extraction) over adding explanatory comments. Ensure classes, methods, and variables convey intent (naming should be expressive and consistent). Use comments only to explain non-obvious decisions or workarounds, not as a substitute for readable code.
Applied to files:
src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.javasrc/main/java/frc/trigon/robot/commands/commandfactories/GeneralCommands.javasrc/main/java/frc/trigon/robot/subsystems/elevator/ElevatorConstants.javasrc/main/java/frc/trigon/robot/subsystems/flyWheel/FlyWheel.javasrc/main/java/frc/trigon/robot/subsystems/flyWheel/FlyWheelConstants.javasrc/main/java/frc/trigon/robot/subsystems/arm/Arm.javasrc/main/java/frc/trigon/robot/RobotContainer.javasrc/main/java/frc/trigon/robot/subsystems/elevator/Elevator.javasrc/main/java/frc/trigon/robot/subsystems/swerve/Swerve.java
📚 Learning: 2025-12-30T20:34:50.231Z
Learnt from: Strflightmight09
Repo: Programming-TRIGON/RobotTemplate PR: 48
File: src/main/java/frc/trigon/robot/subsystems/swerve/Swerve.java:180-183
Timestamp: 2025-12-30T20:34:50.231Z
Learning: In template/ scaffolding repositories (e.g., RobotTemplate), allow and preserve optional code patterns that are commented out for users to enable, rather than removing or fully implementing them. During reviews, treat commented-out blocks as intentional placeholders to be potentially enabled by users; avoid enforcing removal of such code and ensure commented sections are clearly documented as optional templates.
Applied to files:
src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.javasrc/main/java/frc/trigon/robot/commands/commandfactories/GeneralCommands.javasrc/main/java/frc/trigon/robot/subsystems/elevator/ElevatorConstants.javasrc/main/java/frc/trigon/robot/subsystems/flyWheel/FlyWheel.javasrc/main/java/frc/trigon/robot/subsystems/flyWheel/FlyWheelConstants.javasrc/main/java/frc/trigon/robot/subsystems/arm/Arm.javasrc/main/java/frc/trigon/robot/RobotContainer.javasrc/main/java/frc/trigon/robot/subsystems/elevator/Elevator.javasrc/main/java/frc/trigon/robot/subsystems/swerve/Swerve.java
📚 Learning: 2026-01-11T12:11:54.447Z
Learnt from: ShmayaR
Repo: Programming-TRIGON/RobotTemplate PR: 45
File: src/main/java/frc/trigon/robot/misc/objectDetection/ObjectPoseEstimator.java:30-37
Timestamp: 2026-01-11T12:11:54.447Z
Learning: In the RobotTemplate codebase (Java/FRC), prefer silent failure (no-op) over throwing exceptions for certain validation scenarios. Specifically, for constructors or methods dealing with varargs (e.g., cameras), do not throw on invalid input such as zero elements; instead, perform a no-op or safe default if zero cameras are provided. This pattern should be applied to Java files under the frc/trigon/robot area (e.g., src/main/java/frc/trigon/robot/**/*.java). Document the rationale for silent failures where used, and ensure this behavior is clearly tested and reviewed to avoid hidden issues or regressions.
Applied to files:
src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.javasrc/main/java/frc/trigon/robot/commands/commandfactories/GeneralCommands.javasrc/main/java/frc/trigon/robot/subsystems/elevator/ElevatorConstants.javasrc/main/java/frc/trigon/robot/subsystems/flyWheel/FlyWheel.javasrc/main/java/frc/trigon/robot/subsystems/flyWheel/FlyWheelConstants.javasrc/main/java/frc/trigon/robot/subsystems/arm/Arm.javasrc/main/java/frc/trigon/robot/RobotContainer.javasrc/main/java/frc/trigon/robot/subsystems/elevator/Elevator.javasrc/main/java/frc/trigon/robot/subsystems/swerve/Swerve.java
🧬 Code graph analysis (3)
src/main/java/frc/trigon/robot/subsystems/flyWheel/FlyWheel.java (1)
src/main/java/frc/trigon/robot/subsystems/flyWheel/FlyWheelConstants.java (1)
FlyWheelConstants(13-126)
src/main/java/frc/trigon/robot/subsystems/arm/Arm.java (1)
src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java (1)
ArmConstants(16-171)
src/main/java/frc/trigon/robot/subsystems/elevator/Elevator.java (1)
src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorConstants.java (1)
ElevatorConstants(15-147)
🪛 Ruff (0.14.10)
src/main/python/keyboard_to_nt.py
20-20: Missing return type annotation for private function on_press
Add return type annotation: None
(ANN202)
21-21: Boolean positional value in function call
(FBT003)
23-23: Missing return type annotation for private function on_release
Add return type annotation: None
(ANN202)
24-24: Boolean positional value in function call
(FBT003)
26-26: Missing return type annotation for private function handle_key
Add return type annotation: None
(ANN202)
🔇 Additional comments (23)
.gitmodules (1)
4-4: Submodule points to a feature branch.Using
subsystem-wrapperbranch instead ofmaincould cause instability if the branch is rebased, force-pushed, or deleted. Ensure this is intentional for development and plan to update tomainonce the library changes are merged.src/main/java/frc/trigon/robot/subsystems/swerve/Swerve.java (1)
22-22: LGTM!Import path correctly updated to use the library version of
MotorSubsystem.src/main/java/frc/trigon/robot/commands/commandfactories/GeneralCommands.java (1)
7-7: LGTM!Import path correctly updated to use the library version of
MotorSubsystem, consistent with the broader lib migration.src/main/java/frc/trigon/robot/subsystems/flyWheel/FlyWheel.java (1)
1-9: LGTM!Clean wrapper implementation. Same package naming note applies (
flyWheel→flywheel).src/main/java/frc/trigon/robot/subsystems/arm/Arm.java (1)
8-11: LGTM!Clean wrapper implementation following the established pattern.
src/main/java/frc/trigon/robot/subsystems/elevator/Elevator.java (1)
8-11: Critical bug in ElevatorConstants.java:getTargetPositionMeters()always returns 0.In
ElevatorConstants.java(lines 127),ElevatorState.getTargetPositionMeters()returns0instead oftargetPosition. The elevator will never move to the intended HALF (0.7m) or FULL (1.5m) positions.Fix in
ElevatorConstants.java:@Override public double getTargetPositionMeters() { - return 0; + return targetPosition; }Likely an incorrect or invalid review comment.
src/main/java/frc/trigon/robot/RobotContainer.java (4)
12-18: LGTM!Imports for the library subsystem types are correctly added.
29-34: LGTM!Local subsystem and constants imports are correctly structured.
47-49: LGTM!Good use of interface types (
ArmSubsystem,ElevatorSubsystem,SimpleMotorSubsystem) for the public subsystem declarations, allowing for easier testing and abstraction.
81-82: The review comment is incorrect.OPERATOR_CONTROLLERis aKeyboardController(not a standard WPILibCommandXboxController), so.a()and.s()are valid methods representing the 'A' and 'S' keyboard keys. This is consistent with other methods on the controller like.g(),.f2(),.right(),.left(),.up(), and.down()shown in the same file.src/main/java/frc/trigon/robot/subsystems/arm/ArmConstants.java (7)
16-28: LGTM!Motor and encoder declarations are well-structured with clear naming and appropriate visibility modifiers.
30-47: LGTM!Configuration constants are clearly named and appropriately scoped.
49-56: LGTM!Static initializer with method extraction follows clean code principles for readability.
58-75: LGTM!
configureSubsystem()clearly maps constants to configuration fields.
77-110: LGTM!Motor configuration is comprehensive with PID, feedforward, and Motion Magic parameters properly set.
127-139: LGTM!Encoder configuration is properly linked to the master motor for simulation.
141-170: LGTM!
ArmStateenum correctly implementsArmSubsystem.ArmStatewith clear state definitions.src/main/java/frc/trigon/robot/subsystems/elevator/ElevatorConstants.java (6)
16-24: LGTM!Motor declarations are consistent with the ArmConstants pattern.
26-41: LGTM!Configuration constants are well-defined.
43-49: LGTM!Static initializer follows established pattern.
51-67: LGTM!Configuration mapping is correct.
69-100: LGTM!Master motor configuration with elevator-specific gravity type is correct.
117-130: LGTM!State definitions with target positions, prepare states, and speed scalars are well-structured.
| ARM.setDefaultCommand(ArmSubsystemCommands.getSetTargetStateCommand(ArmConstants.ArmState.REST, ARM)); | ||
| ELEVATOR.setDefaultCommand(ElevatorSubsystemCommands.getSetTargetStateCommand(ElevatorConstants.ElevatorState.REST,ELEVATOR)); | ||
| FLY_WHEEL.setDefaultCommand(SimpleMotorSubsystemCommands.getSetTargetStateCommand(FlyWheelConstants.FlyWheelState.REST, FLY_WHEEL)); |
There was a problem hiding this comment.
Missing space after comma on line 73.
Minor formatting inconsistency.
Suggested fix
- ELEVATOR.setDefaultCommand(ElevatorSubsystemCommands.getSetTargetStateCommand(ElevatorConstants.ElevatorState.REST,ELEVATOR));
+ ELEVATOR.setDefaultCommand(ElevatorSubsystemCommands.getSetTargetStateCommand(ElevatorConstants.ElevatorState.REST, ELEVATOR));📝 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.
| ARM.setDefaultCommand(ArmSubsystemCommands.getSetTargetStateCommand(ArmConstants.ArmState.REST, ARM)); | |
| ELEVATOR.setDefaultCommand(ElevatorSubsystemCommands.getSetTargetStateCommand(ElevatorConstants.ElevatorState.REST,ELEVATOR)); | |
| FLY_WHEEL.setDefaultCommand(SimpleMotorSubsystemCommands.getSetTargetStateCommand(FlyWheelConstants.FlyWheelState.REST, FLY_WHEEL)); | |
| ARM.setDefaultCommand(ArmSubsystemCommands.getSetTargetStateCommand(ArmConstants.ArmState.REST, ARM)); | |
| ELEVATOR.setDefaultCommand(ElevatorSubsystemCommands.getSetTargetStateCommand(ElevatorConstants.ElevatorState.REST, ELEVATOR)); | |
| FLY_WHEEL.setDefaultCommand(SimpleMotorSubsystemCommands.getSetTargetStateCommand(FlyWheelConstants.FlyWheelState.REST, FLY_WHEEL)); |
| import frc.trigon.lib.hardware.phoenix6.talonfx.TalonFXMotor; | ||
| import frc.trigon.lib.subsystems.arm.ArmSubsystem; | ||
| import frc.trigon.lib.subsystems.arm.ArmSubsystemConfiguration; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove unused imports.
TalonFXMotor and ArmSubsystemConfiguration are not used in this file.
Suggested fix
package frc.trigon.robot.subsystems.arm;
import edu.wpi.first.math.geometry.Pose3d;
-import frc.trigon.lib.hardware.phoenix6.talonfx.TalonFXMotor;
import frc.trigon.lib.subsystems.arm.ArmSubsystem;
-import frc.trigon.lib.subsystems.arm.ArmSubsystemConfiguration;
public class Arm extends ArmSubsystem {📝 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.
| import frc.trigon.lib.hardware.phoenix6.talonfx.TalonFXMotor; | |
| import frc.trigon.lib.subsystems.arm.ArmSubsystem; | |
| import frc.trigon.lib.subsystems.arm.ArmSubsystemConfiguration; | |
| package frc.trigon.robot.subsystems.arm; | |
| import edu.wpi.first.math.geometry.Pose3d; | |
| import frc.trigon.lib.subsystems.arm.ArmSubsystem; | |
| public class Arm extends ArmSubsystem { |
| private static void configureFollowerMotor() { | ||
| TalonFXConfiguration config = new TalonFXConfiguration(); | ||
| config.Audio.BeepOnConfig = false; | ||
| config.Audio.BeepOnBoot = false; | ||
|
|
||
| config.Feedback.SensorToMechanismRatio = GEAR_RATIO; | ||
| config.MotorOutput.Inverted = InvertedValue.Clockwise_Positive; | ||
| config.MotorOutput.NeutralMode = NeutralModeValue.Brake; | ||
|
|
||
| FOLLOWER_MOTOR.applyConfiguration(config); | ||
|
|
||
| final Follower FollowerRequest = new Follower(MASTER_MOTOR_ID, false); | ||
| FOLLOWER_MOTOR.setControl(FollowerRequest); | ||
| } |
There was a problem hiding this comment.
Variable name violates Java naming conventions.
FollowerRequest should be followerRequest (camelCase for local variables).
Suggested fix
- final Follower FollowerRequest = new Follower(MASTER_MOTOR_ID, false);
- FOLLOWER_MOTOR.setControl(FollowerRequest);
+ final Follower followerRequest = new Follower(MASTER_MOTOR_ID, false);
+ FOLLOWER_MOTOR.setControl(followerRequest);| import frc.trigon.lib.hardware.phoenix6.talonfx.TalonFXMotor; | ||
| import frc.trigon.lib.subsystems.elevator.ElevatorSubsystem; | ||
| import frc.trigon.lib.subsystems.elevator.ElevatorSubsystemConfiguration; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove unused imports.
TalonFXMotor and ElevatorSubsystemConfiguration are not used in this file.
Suggested fix
package frc.trigon.robot.subsystems.elevator;
import edu.wpi.first.math.geometry.Pose3d;
-import frc.trigon.lib.hardware.phoenix6.talonfx.TalonFXMotor;
import frc.trigon.lib.subsystems.elevator.ElevatorSubsystem;
-import frc.trigon.lib.subsystems.elevator.ElevatorSubsystemConfiguration;
public class Elevator extends ElevatorSubsystem {📝 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.
| import frc.trigon.lib.hardware.phoenix6.talonfx.TalonFXMotor; | |
| import frc.trigon.lib.subsystems.elevator.ElevatorSubsystem; | |
| import frc.trigon.lib.subsystems.elevator.ElevatorSubsystemConfiguration; | |
| package frc.trigon.robot.subsystems.elevator; | |
| import edu.wpi.first.math.geometry.Pose3d; | |
| import frc.trigon.lib.subsystems.elevator.ElevatorSubsystem; | |
| public class Elevator extends ElevatorSubsystem { |
| private static void configureFollowerMotor() { | ||
| TalonFXConfiguration config = new TalonFXConfiguration(); | ||
| config.Audio.BeepOnConfig = false; | ||
| config.Audio.BeepOnBoot = false; | ||
|
|
||
| config.Feedback.SensorToMechanismRatio = GEAR_RATIO; | ||
| config.MotorOutput.Inverted = InvertedValue.Clockwise_Positive; | ||
| config.MotorOutput.NeutralMode = NeutralModeValue.Brake; | ||
|
|
||
| FOLLOWER_MOTOR.applyConfiguration(config); | ||
|
|
||
| final Follower FollowerRequest = new Follower(MASTER_MOTOR_ID, false); | ||
| FOLLOWER_MOTOR.setControl(FollowerRequest); | ||
| } |
There was a problem hiding this comment.
Variable name violates Java naming conventions.
Same issue as in ArmConstants: FollowerRequest should be followerRequest.
Suggested fix
- final Follower FollowerRequest = new Follower(MASTER_MOTOR_ID, false);
- FOLLOWER_MOTOR.setControl(FollowerRequest);
+ final Follower followerRequest = new Follower(MASTER_MOTOR_ID, false);
+ FOLLOWER_MOTOR.setControl(followerRequest);📝 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.
| private static void configureFollowerMotor() { | |
| TalonFXConfiguration config = new TalonFXConfiguration(); | |
| config.Audio.BeepOnConfig = false; | |
| config.Audio.BeepOnBoot = false; | |
| config.Feedback.SensorToMechanismRatio = GEAR_RATIO; | |
| config.MotorOutput.Inverted = InvertedValue.Clockwise_Positive; | |
| config.MotorOutput.NeutralMode = NeutralModeValue.Brake; | |
| FOLLOWER_MOTOR.applyConfiguration(config); | |
| final Follower FollowerRequest = new Follower(MASTER_MOTOR_ID, false); | |
| FOLLOWER_MOTOR.setControl(FollowerRequest); | |
| } | |
| private static void configureFollowerMotor() { | |
| TalonFXConfiguration config = new TalonFXConfiguration(); | |
| config.Audio.BeepOnConfig = false; | |
| config.Audio.BeepOnBoot = false; | |
| config.Feedback.SensorToMechanismRatio = GEAR_RATIO; | |
| config.MotorOutput.Inverted = InvertedValue.Clockwise_Positive; | |
| config.MotorOutput.NeutralMode = NeutralModeValue.Brake; | |
| FOLLOWER_MOTOR.applyConfiguration(config); | |
| final Follower followerRequest = new Follower(MASTER_MOTOR_ID, false); | |
| FOLLOWER_MOTOR.setControl(followerRequest); | |
| } |
| @Override | ||
| public double getTargetPositionMeters() { | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
Bug: getTargetPositionMeters() returns hardcoded 0 instead of targetPosition.
This will cause all elevator states (REST, HALF, FULL) to target position 0 meters, completely breaking elevator position control.
Proposed fix
@Override
public double getTargetPositionMeters() {
- return 0;
+ return targetPosition;
}📝 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 double getTargetPositionMeters() { | |
| return 0; | |
| } | |
| @Override | |
| public double getTargetPositionMeters() { | |
| return targetPosition; | |
| } |
| final Follower FollowerRequest = new Follower(MASTER_MOTOR_ID, false); | ||
| FOLLOWER_MOTOR.setControl(FollowerRequest); |
There was a problem hiding this comment.
Variable naming convention violation.
FollowerRequest should be followerRequest (camelCase for local variables).
Suggested fix
- final Follower FollowerRequest = new Follower(MASTER_MOTOR_ID, false);
- FOLLOWER_MOTOR.setControl(FollowerRequest);
+ final Follower followerRequest = new Follower(MASTER_MOTOR_ID, false);
+ FOLLOWER_MOTOR.setControl(followerRequest);📝 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.
| final Follower FollowerRequest = new Follower(MASTER_MOTOR_ID, false); | |
| FOLLOWER_MOTOR.setControl(FollowerRequest); | |
| final Follower followerRequest = new Follower(MASTER_MOTOR_ID, false); | |
| FOLLOWER_MOTOR.setControl(followerRequest); |
src/main/python/keyboard_to_nt.py
Outdated
| def on_press(key): | ||
| handle_key(key, True) | ||
|
|
||
| def on_release(key): | ||
| handle_key(key, False) | ||
|
|
||
| def handle_key(key, is_down): |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Missing type annotations on helper functions.
Per static analysis (Ruff ANN202), the inner functions lack return type annotations. Low priority for internal helpers, but adding -> None improves clarity.
def on_press(key) -> None:
def on_release(key) -> None:
def handle_key(key, is_down: bool) -> None:🧰 Tools
🪛 Ruff (0.14.10)
20-20: Missing return type annotation for private function on_press
Add return type annotation: None
(ANN202)
21-21: Boolean positional value in function call
(FBT003)
23-23: Missing return type annotation for private function on_release
Add return type annotation: None
(ANN202)
24-24: Boolean positional value in function call
(FBT003)
26-26: Missing return type annotation for private function handle_key
Add return type annotation: None
(ANN202)
src/main/python/keyboard_to_nt.py
Outdated
| def handle_key(key, is_down): | ||
| try: | ||
| # Character keys | ||
| if key.char == "/": | ||
| return | ||
| table.putBoolean(key.char.lower(), is_down) | ||
| except AttributeError: | ||
| # Special keys (shift, space, arrows, etc.) | ||
| table.putBoolean(str(key).replace("Key.", ""), is_down) |
There was a problem hiding this comment.
key.char can be None, causing unintended exception flow.
For some keys, key.char exists but is None. The current code accidentally handles this via the AttributeError from None.lower(), but this obscures intent and is fragile.
Suggested fix
def handle_key(key, is_down):
try:
# Character keys
- if key.char == "/":
+ if key.char is None or key.char == "/":
return
table.putBoolean(key.char.lower(), is_down)
except AttributeError:
# Special keys (shift, space, arrows, etc.)
table.putBoolean(str(key).replace("Key.", ""), is_down)📝 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.
| def handle_key(key, is_down): | |
| try: | |
| # Character keys | |
| if key.char == "/": | |
| return | |
| table.putBoolean(key.char.lower(), is_down) | |
| except AttributeError: | |
| # Special keys (shift, space, arrows, etc.) | |
| table.putBoolean(str(key).replace("Key.", ""), is_down) | |
| def handle_key(key, is_down): | |
| try: | |
| # Character keys | |
| if key.char is None or key.char == "/": | |
| return | |
| table.putBoolean(key.char.lower(), is_down) | |
| except AttributeError: | |
| # Special keys (shift, space, arrows, etc.) | |
| table.putBoolean(str(key).replace("Key.", ""), is_down) |
🧰 Tools
🪛 Ruff (0.14.10)
26-26: Missing return type annotation for private function handle_key
Add return type annotation: None
(ANN202)
src/main/python/keyboard_to_nt.py
Outdated
| with keyboard.Listener( | ||
| on_press=on_press, | ||
| on_release=on_release | ||
| ) as listener: | ||
| listener.join() |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding graceful shutdown handling.
The listener blocks indefinitely with no exit mechanism. For a dev utility this is acceptable, but wrapping in a try/except for KeyboardInterrupt would provide cleaner exit behavior.
Optional: graceful shutdown
- with keyboard.Listener(
- on_press=on_press,
- on_release=on_release
- ) as listener:
- listener.join()
+ try:
+ with keyboard.Listener(
+ on_press=on_press,
+ on_release=on_release
+ ) as listener:
+ listener.join()
+ except KeyboardInterrupt:
+ print("\nShutting down...")
There was a problem hiding this comment.
Actionable comments posted: 8
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
src/main/java/frc/trigon/libsrc/main/java/frc/trigon/robot/RobotContainer.javasrc/main/java/frc/trigon/robot/constants/OperatorConstants.javasrc/main/java/frc/trigon/robot/subsystems/flyWheel/FlyWheelConstants.javasrc/main/java/frc/trigon/robot/subsystems/swerve/Swerve.javasrc/main/java/frc/trigon/robot/subsystems/swerve/SwerveConstants.javasrc/main/java/frc/trigon/robot/subsystems/swerve/swervemodule/SwerveModule.javasrc/main/java/frc/trigon/robot/subsystems/swerve/swervemodule/SwerveModuleConstants.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:
src/main/java/frc/trigon/robot/subsystems/flyWheel/FlyWheelConstants.javasrc/main/java/frc/trigon/libsrc/main/java/frc/trigon/robot/subsystems/swerve/swervemodule/SwerveModuleConstants.javasrc/main/java/frc/trigon/robot/subsystems/swerve/Swerve.javasrc/main/java/frc/trigon/robot/subsystems/swerve/swervemodule/SwerveModule.javasrc/main/java/frc/trigon/robot/RobotContainer.javasrc/main/java/frc/trigon/robot/subsystems/swerve/SwerveConstants.javasrc/main/java/frc/trigon/robot/constants/OperatorConstants.java
🧠 Learnings (3)
📚 Learning: 2025-12-30T00:12:14.902Z
Learnt from: levyishai
Repo: Programming-TRIGON/RobotTemplate PR: 45
File: src/main/java/frc/trigon/robot/poseestimation/poseestimator/ObjectPoseEstimator.java:161-187
Timestamp: 2025-12-30T00:12:14.902Z
Learning: In the Programming-TRIGON/RobotTemplate Java codebase, follow Uncle Bob's Clean Code principles: prefer self-documenting code through descriptive naming and small, focused methods (method extraction) over adding explanatory comments. Ensure classes, methods, and variables convey intent (naming should be expressive and consistent). Use comments only to explain non-obvious decisions or workarounds, not as a substitute for readable code.
Applied to files:
src/main/java/frc/trigon/robot/subsystems/flyWheel/FlyWheelConstants.javasrc/main/java/frc/trigon/robot/subsystems/swerve/swervemodule/SwerveModuleConstants.javasrc/main/java/frc/trigon/robot/subsystems/swerve/Swerve.javasrc/main/java/frc/trigon/robot/subsystems/swerve/swervemodule/SwerveModule.javasrc/main/java/frc/trigon/robot/RobotContainer.javasrc/main/java/frc/trigon/robot/subsystems/swerve/SwerveConstants.javasrc/main/java/frc/trigon/robot/constants/OperatorConstants.java
📚 Learning: 2025-12-30T20:34:50.231Z
Learnt from: Strflightmight09
Repo: Programming-TRIGON/RobotTemplate PR: 48
File: src/main/java/frc/trigon/robot/subsystems/swerve/Swerve.java:180-183
Timestamp: 2025-12-30T20:34:50.231Z
Learning: In template/ scaffolding repositories (e.g., RobotTemplate), allow and preserve optional code patterns that are commented out for users to enable, rather than removing or fully implementing them. During reviews, treat commented-out blocks as intentional placeholders to be potentially enabled by users; avoid enforcing removal of such code and ensure commented sections are clearly documented as optional templates.
Applied to files:
src/main/java/frc/trigon/robot/subsystems/flyWheel/FlyWheelConstants.javasrc/main/java/frc/trigon/robot/subsystems/swerve/swervemodule/SwerveModuleConstants.javasrc/main/java/frc/trigon/robot/subsystems/swerve/Swerve.javasrc/main/java/frc/trigon/robot/subsystems/swerve/swervemodule/SwerveModule.javasrc/main/java/frc/trigon/robot/RobotContainer.javasrc/main/java/frc/trigon/robot/subsystems/swerve/SwerveConstants.javasrc/main/java/frc/trigon/robot/constants/OperatorConstants.java
📚 Learning: 2026-01-11T12:11:54.447Z
Learnt from: ShmayaR
Repo: Programming-TRIGON/RobotTemplate PR: 45
File: src/main/java/frc/trigon/robot/misc/objectDetection/ObjectPoseEstimator.java:30-37
Timestamp: 2026-01-11T12:11:54.447Z
Learning: In the RobotTemplate codebase (Java/FRC), prefer silent failure (no-op) over throwing exceptions for certain validation scenarios. Specifically, for constructors or methods dealing with varargs (e.g., cameras), do not throw on invalid input such as zero elements; instead, perform a no-op or safe default if zero cameras are provided. This pattern should be applied to Java files under the frc/trigon/robot area (e.g., src/main/java/frc/trigon/robot/**/*.java). Document the rationale for silent failures where used, and ensure this behavior is clearly tested and reviewed to avoid hidden issues or regressions.
Applied to files:
src/main/java/frc/trigon/robot/subsystems/flyWheel/FlyWheelConstants.javasrc/main/java/frc/trigon/robot/subsystems/swerve/swervemodule/SwerveModuleConstants.javasrc/main/java/frc/trigon/robot/subsystems/swerve/Swerve.javasrc/main/java/frc/trigon/robot/subsystems/swerve/swervemodule/SwerveModule.javasrc/main/java/frc/trigon/robot/RobotContainer.javasrc/main/java/frc/trigon/robot/subsystems/swerve/SwerveConstants.javasrc/main/java/frc/trigon/robot/constants/OperatorConstants.java
🧬 Code graph analysis (1)
src/main/java/frc/trigon/robot/constants/OperatorConstants.java (1)
src/main/java/frc/trigon/robot/commands/commandclasses/IntakeAssistCommand.java (1)
IntakeAssistCommand(23-208)
🔇 Additional comments (12)
src/main/java/frc/trigon/robot/constants/OperatorConstants.java (2)
14-14: Verify the driver controller port change.The driver controller port changed from 0 to 1. Ensure this matches your actual hardware configuration—port 0 is typically the first controller.
47-48: LGTM!The
INTAKE_TRIGGERconstant is well-named and follows the established pattern for trigger definitions.src/main/java/frc/trigon/robot/subsystems/flyWheel/FlyWheelConstants.java (2)
72-78: All PID gains are zero.All closed-loop control gains (kP, kI, kD, kS, kG, kA, kV) are set to 0. This effectively disables closed-loop control. If this is intentional because
SHOULD_USE_VOLTAGE_CONTROL = true, consider adding a brief comment to clarify. Otherwise, these need tuning.
103-124: LGTM!The
FlyWheelStateenum cleanly implementsSimpleMotorStatewith REST and INTAKE states. The pattern is extensible for future states.src/main/java/frc/trigon/robot/RobotContainer.java (3)
37-39: LGTM!The FlyWheel subsystem is properly initialized and follows the established pattern. The commented-out ARM and ELEVATOR placeholders are appropriate for a template repository. Based on learnings, these serve as optional templates for users to enable.
61-64: LGTM!Default command correctly sets FlyWheel to REST state. The commented SWERVE default command serves as a template placeholder.
68-71: LGTM!The INTAKE_TRIGGER binding correctly uses
whileTrueto maintain the INTAKE state only while the trigger is held, returning to REST (default command) when released.src/main/java/frc/trigon/robot/subsystems/swerve/SwerveConstants.java (2)
23-23: LGTM!Simplified gyro constructor. Verify CANIVORE_NAME removal is intentional if these devices should be on a CANivore bus.
31-34: LGTM!Calibrated encoder offset values replacing the previous zeros. Ensure these were measured on the actual hardware.
src/main/java/frc/trigon/robot/subsystems/swerve/Swerve.java (1)
15-15: LGTM!Import path updated to use the library package, consistent with the broader refactoring in this PR.
src/main/java/frc/trigon/robot/subsystems/swerve/swervemodule/SwerveModuleConstants.java (2)
99-99: Verify: Coast mode on steer motor is intentional?Steering motors typically use
Braketo maintain wheel orientation when disabled. WithCoast, wheels may drift from their target angle. If this is intentional (e.g., for manual wheel adjustment when robot is off), consider adding a comment explaining the rationale.
109-109: Tuning adjustment acknowledged.The kP reduction from 40 to 30 for the real robot is a reasonable tuning change. Ensure this has been validated on hardware to confirm adequate steering response.
src/main/java/frc/trigon/lib
Outdated
| @@ -1 +1 @@ | |||
| Subproject commit 6732cfae4b692ecb714bd664012a4e68eaa62b43 | |||
| Subproject commit 56832d0d0f90f81245603a4f1c3f9ff98b0f1c3e | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the .gitmodules file to see the submodule configuration
cat .gitmodules 2>/dev/null || echo "No .gitmodules found"Repository: Programming-TRIGON/RobotTemplate
Length of output: 238
🏁 Script executed:
# Also check git configuration for submodule details
git config --file .gitmodules --get-regexp trigon/lib 2>/dev/null || echo "No git config found"Repository: Programming-TRIGON/RobotTemplate
Length of output: 303
🏁 Script executed:
# Search for any recent commits or references related to the submodule update
git log --oneline -20 -- src/main/java/frc/trigon/lib 2>/dev/null | head -5 || echo "No git history available"Repository: Programming-TRIGON/RobotTemplate
Length of output: 194
Submodule must not track feature branches.
The submodule is configured to track the subsystem-wrapper feature branch. This causes instability—developers pulling this code get whatever's currently on that branch rather than a fixed, tested commit.
Move this work to the lib repository as suggested in earlier PR comments, or if keeping the submodule approach, pin it to a stable branch or specific tag instead of subsystem-wrapper.
| import edu.wpi.first.wpilibj2.command.button.Trigger; | ||
| import frc.trigon.lib.hardware.misc.KeyboardController; | ||
| import frc.trigon.lib.hardware.misc.XboxController; | ||
| import frc.trigon.robot.commands.commandclasses.IntakeAssistCommand; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Unused import.
IntakeAssistCommand is imported but not referenced anywhere in this file. Remove it to keep imports clean.
| @@ -0,0 +1,125 @@ | |||
| package frc.trigon.robot.subsystems.flyWheel; | |||
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Package naming convention.
Java convention is all-lowercase for package names: flywheel instead of flyWheel. This affects consistency across the codebase.
| private final static boolean SHOULD_USE_VOLTAGE_CONTROL = true; | ||
| private final static DCMotor GEAR_BOX = DCMotor.getKrakenX60Foc(2); | ||
|
|
||
| static SimpleMotorSubsystemConfiguration FLY_WHEEL_CONFIG = new SimpleMotorSubsystemConfiguration(); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider making configuration immutable or final.
FLY_WHEEL_CONFIG is package-private and mutable. While it's configured in a static block, external code could still modify it after initialization. Consider making it final or providing an immutable configuration pattern.
Suggested fix
- static SimpleMotorSubsystemConfiguration FLY_WHEEL_CONFIG = new SimpleMotorSubsystemConfiguration();
+ static final SimpleMotorSubsystemConfiguration FLY_WHEEL_CONFIG = new SimpleMotorSubsystemConfiguration();📝 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.
| static SimpleMotorSubsystemConfiguration FLY_WHEEL_CONFIG = new SimpleMotorSubsystemConfiguration(); | |
| static final SimpleMotorSubsystemConfiguration FLY_WHEEL_CONFIG = new SimpleMotorSubsystemConfiguration(); |
|
|
||
| FOLLOWER_MOTOR.applyConfiguration(config); | ||
|
|
||
| final Follower FollowerRequest = new Follower(MASTER_MOTOR_ID, false); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Variable naming: use camelCase.
FollowerRequest should be followerRequest per Java naming conventions.
Suggested fix
- final Follower FollowerRequest = new Follower(MASTER_MOTOR_ID, false);
- FOLLOWER_MOTOR.setControl(FollowerRequest);
+ final Follower followerRequest = new Follower(MASTER_MOTOR_ID, false);
+ FOLLOWER_MOTOR.setControl(followerRequest);| driveMotor = new TalonFXMotor(moduleID, "Module" + moduleID + "Drive"); | ||
| steerMotor = new TalonFXMotor(moduleID + 4, "Module" + moduleID + "Steer"); | ||
| steerEncoder = new CANcoderEncoder(moduleID + 4, "Module" + moduleID + "SteerEncoder"); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
# Search for TalonFXMotor class definition
fd -type f -name "*.java" | xargs grep -l "class TalonFXMotor"Repository: Programming-TRIGON/RobotTemplate
Length of output: 248
🏁 Script executed:
# Search for CANcoderEncoder class definition
fd -type f -name "*.java" | xargs grep -l "class CANcoderEncoder"Repository: Programming-TRIGON/RobotTemplate
Length of output: 248
🏁 Script executed:
# Search for CANIVORE references in the codebase
rg -n "CANIVORE" --type java -B 2 -A 2Repository: Programming-TRIGON/RobotTemplate
Length of output: 716
🏁 Script executed:
# Find files containing TalonFXMotor class definition
find . -name "*.java" -exec grep -l "class TalonFXMotor" {} \;Repository: Programming-TRIGON/RobotTemplate
Length of output: 58
🏁 Script executed:
# Find files containing CANcoderEncoder class definition
find . -name "*.java" -exec grep -l "class CANcoderEncoder" {} \;Repository: Programming-TRIGON/RobotTemplate
Length of output: 58
🏁 Script executed:
# Search for CANIVORE references in Java files
rg "CANIVORE" -g "*.java" -B 2 -A 2Repository: Programming-TRIGON/RobotTemplate
Length of output: 705
🏁 Script executed:
# Check if CANIVORE_NAME is used in SwerveModule or if it's now orphaned
rg "CANIVORE_NAME" -g "*.java" -B 1 -A 1Repository: Programming-TRIGON/RobotTemplate
Length of output: 477
🏁 Script executed:
# Check imports in SwerveModule.java
rg "import" src/main/java/frc/trigon/robot/subsystems/swerve/swervemodule/SwerveModule.javaRepository: Programming-TRIGON/RobotTemplate
Length of output: 958
🏁 Script executed:
# Find all instantiations of TalonFXMotor in the codebase
rg "new TalonFXMotor" -g "*.java" -B 1 -A 1Repository: Programming-TRIGON/RobotTemplate
Length of output: 2292
🏁 Script executed:
# Find all instantiations of CANcoderEncoder in the codebase
rg "new CANcoderEncoder" -g "*.java" -B 1 -A 1Repository: Programming-TRIGON/RobotTemplate
Length of output: 920
🏁 Script executed:
# Check if there are any other motor/encoder instantiations that still use CANIVORE_NAME
rg "new.*\(" -g "*.java" | grep -i "motor\|encoder" | head -20Repository: Programming-TRIGON/RobotTemplate
Length of output: 3339
Remove the orphaned CANIVORE_NAME constant from RobotConstants.java.
The CANIVORE_NAME parameter was intentionally removed from all hardware instantiations as part of a consistent library update across the codebase (Arm, FlyWheel, Elevator, and SwerveModule all use the same (id, name) constructor signature). The constant is now unused and should be cleaned up.
| private static final double | ||
| DRIVE_MOTOR_GEAR_RATIO = 6.03,//R1: 7.03, R2: 6.03, R3: 5.27 | ||
| STEER_MOTOR_GEAR_RATIO = 287.0 / 11.0; | ||
| DRIVE_MOTOR_GEAR_RATIO = 6.03, | ||
| STEER_MOTOR_GEAR_RATIO = 26.09; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor precision loss in steer gear ratio.
The original expression 287.0 / 11.0 equals 26.090909..., while the literal 26.09 truncates this. The ~0.0009 difference could introduce minor steering inaccuracy over time. Consider keeping the fractional form for precision:
- STEER_MOTOR_GEAR_RATIO = 26.09;
+ STEER_MOTOR_GEAR_RATIO = 287.0 / 11.0;📝 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.
| private static final double | |
| DRIVE_MOTOR_GEAR_RATIO = 6.03,//R1: 7.03, R2: 6.03, R3: 5.27 | |
| STEER_MOTOR_GEAR_RATIO = 287.0 / 11.0; | |
| DRIVE_MOTOR_GEAR_RATIO = 6.03, | |
| STEER_MOTOR_GEAR_RATIO = 26.09; | |
| private static final double | |
| DRIVE_MOTOR_GEAR_RATIO = 6.03, | |
| STEER_MOTOR_GEAR_RATIO = 287.0 / 11.0; |
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
src/main/java/frc/trigon/libsrc/main/java/frc/trigon/robot/RobotContainer.javasrc/main/java/frc/trigon/robot/commands/commandfactories/GeneralCommands.javasrc/main/java/frc/trigon/robot/subsystems/swerve/SwerveConstants.javasrc/main/java/frc/trigon/robot/subsystems/swerve/swervemodule/SwerveModule.javasrc/main/java/frc/trigon/robot/subsystems/swerve/swervemodule/SwerveModuleConstants.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:
src/main/java/frc/trigon/robot/subsystems/swerve/SwerveConstants.javasrc/main/java/frc/trigon/robot/RobotContainer.javasrc/main/java/frc/trigon/robot/commands/commandfactories/GeneralCommands.javasrc/main/java/frc/trigon/robot/subsystems/swerve/swervemodule/SwerveModuleConstants.javasrc/main/java/frc/trigon/robot/subsystems/swerve/swervemodule/SwerveModule.javasrc/main/java/frc/trigon/lib
🧠 Learnings (3)
📚 Learning: 2025-12-30T00:12:14.902Z
Learnt from: levyishai
Repo: Programming-TRIGON/RobotTemplate PR: 45
File: src/main/java/frc/trigon/robot/poseestimation/poseestimator/ObjectPoseEstimator.java:161-187
Timestamp: 2025-12-30T00:12:14.902Z
Learning: In the Programming-TRIGON/RobotTemplate Java codebase, follow Uncle Bob's Clean Code principles: prefer self-documenting code through descriptive naming and small, focused methods (method extraction) over adding explanatory comments. Ensure classes, methods, and variables convey intent (naming should be expressive and consistent). Use comments only to explain non-obvious decisions or workarounds, not as a substitute for readable code.
Applied to files:
src/main/java/frc/trigon/robot/subsystems/swerve/SwerveConstants.javasrc/main/java/frc/trigon/robot/RobotContainer.javasrc/main/java/frc/trigon/robot/commands/commandfactories/GeneralCommands.javasrc/main/java/frc/trigon/robot/subsystems/swerve/swervemodule/SwerveModuleConstants.javasrc/main/java/frc/trigon/robot/subsystems/swerve/swervemodule/SwerveModule.java
📚 Learning: 2025-12-30T20:34:50.231Z
Learnt from: Strflightmight09
Repo: Programming-TRIGON/RobotTemplate PR: 48
File: src/main/java/frc/trigon/robot/subsystems/swerve/Swerve.java:180-183
Timestamp: 2025-12-30T20:34:50.231Z
Learning: In template/ scaffolding repositories (e.g., RobotTemplate), allow and preserve optional code patterns that are commented out for users to enable, rather than removing or fully implementing them. During reviews, treat commented-out blocks as intentional placeholders to be potentially enabled by users; avoid enforcing removal of such code and ensure commented sections are clearly documented as optional templates.
Applied to files:
src/main/java/frc/trigon/robot/subsystems/swerve/SwerveConstants.javasrc/main/java/frc/trigon/robot/RobotContainer.javasrc/main/java/frc/trigon/robot/commands/commandfactories/GeneralCommands.javasrc/main/java/frc/trigon/robot/subsystems/swerve/swervemodule/SwerveModuleConstants.javasrc/main/java/frc/trigon/robot/subsystems/swerve/swervemodule/SwerveModule.java
📚 Learning: 2026-01-11T12:11:54.447Z
Learnt from: ShmayaR
Repo: Programming-TRIGON/RobotTemplate PR: 45
File: src/main/java/frc/trigon/robot/misc/objectDetection/ObjectPoseEstimator.java:30-37
Timestamp: 2026-01-11T12:11:54.447Z
Learning: In the RobotTemplate codebase (Java/FRC), prefer silent failure (no-op) over throwing exceptions for certain validation scenarios. Specifically, for constructors or methods dealing with varargs (e.g., cameras), do not throw on invalid input such as zero elements; instead, perform a no-op or safe default if zero cameras are provided. This pattern should be applied to Java files under the frc/trigon/robot area (e.g., src/main/java/frc/trigon/robot/**/*.java). Document the rationale for silent failures where used, and ensure this behavior is clearly tested and reviewed to avoid hidden issues or regressions.
Applied to files:
src/main/java/frc/trigon/robot/subsystems/swerve/SwerveConstants.javasrc/main/java/frc/trigon/robot/RobotContainer.javasrc/main/java/frc/trigon/robot/commands/commandfactories/GeneralCommands.javasrc/main/java/frc/trigon/robot/subsystems/swerve/swervemodule/SwerveModuleConstants.javasrc/main/java/frc/trigon/robot/subsystems/swerve/swervemodule/SwerveModule.java
🧬 Code graph analysis (2)
src/main/java/frc/trigon/robot/commands/commandfactories/GeneralCommands.java (1)
src/main/java/frc/trigon/robot/subsystems/swerve/SwerveCommands.java (1)
SwerveCommands(15-163)
src/main/java/frc/trigon/robot/subsystems/swerve/swervemodule/SwerveModule.java (2)
src/main/java/frc/trigon/robot/poseestimation/poseestimator/PoseEstimatorConstants.java (1)
PoseEstimatorConstants(3-10)src/main/java/frc/trigon/robot/subsystems/swerve/SwerveConstants.java (1)
SwerveConstants(20-123)
🔇 Additional comments (7)
src/main/java/frc/trigon/robot/subsystems/swerve/swervemodule/SwerveModuleConstants.java (2)
17-20: Minor precision loss in gear ratio constant.
STEER_MOTOR_GEAR_RATIOchanged from a computed value (287.0/11.0 = 26.0909...) to 26.09. This loses ~0.0009 in precision. For FRC applications this is negligible, but if exact ratios matter, consider keeping the fraction or using more decimal places.
69-69: Verify intentional change from Brake to Coast mode.Switching drive motors to
Coastis a significant behavioral change. In Coast mode, the robot will drift when not actively driving, which can reduce control precision—especially during autonomous routines and quick direction changes. Brake mode is typically preferred for swerve drive.Confirm this change is intentional and not a regression.
src/main/java/frc/trigon/robot/commands/commandfactories/GeneralCommands.java (1)
17-21: Verify that open-loop drive is intentional for field-relative control.Switching from closed-loop to open-loop control removes velocity feedback, which may reduce drive precision. If this is intentional (e.g., for smoother joystick feel), consider adding a brief comment explaining the choice.
src/main/java/frc/trigon/robot/subsystems/swerve/swervemodule/SwerveModule.java (1)
42-44: Verify hardware constructors no longer need CAN bus name.The motor and encoder constructors now only take ID and name. If these devices are on a CANivore bus (CAN FD), the bus name may still be required for proper communication. Confirm this change aligns with the updated library API.
src/main/java/frc/trigon/robot/RobotContainer.java (2)
38-40: Commented subsystems and new FLY_WHEEL look fine for a template.The commented-out ARM and ELEVATOR serve as optional placeholders for users to enable. Based on learnings, this pattern is appropriate for template repositories.
68-73: Controller bindings are reasonable.Commented triggers serve as templates. The INTAKE_TRIGGER binding correctly uses the command factory pattern with the flywheel subsystem.
src/main/java/frc/trigon/robot/subsystems/swerve/SwerveConstants.java (1)
31-34: LGTM!Calibrated steer encoder offsets are now set with actual measured values instead of zeros.
| @@ -1 +1 @@ | |||
| Subproject commit 6732cfae4b692ecb714bd664012a4e68eaa62b43 | |||
| Subproject commit 6b9824d047b81a262d6d0bf83412fa779bd61624 | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check current .gitmodules configuration
echo "=== .gitmodules content ==="
cat .gitmodules
echo -e "\n=== Submodule status ==="
git submodule status src/main/java/frc/trigon/lib
echo -e "\n=== Check if submodule is at specific commit or tracking branch ==="
cd src/main/java/frc/trigon/lib 2>/dev/null && {
echo "Current HEAD: $(git rev-parse HEAD)"
echo "Is detached: $(git symbolic-ref -q HEAD && echo 'NO' || echo 'YES')"
git branch -a --contains HEAD | head -5
} || echo "Submodule not initialized or inaccessible"Repository: Programming-TRIGON/RobotTemplate
Length of output: 552
🏁 Script executed:
#!/bin/bash
# Get details about what changed in the submodule between commits
echo "=== Commits in submodule between old and new ==="
git log --oneline 6732cfae4b692ecb714bd664012a4e68eaa62b43..6b9824d047b81a262d6d0bf83412fa779bd61624 -- src/main/java/frc/trigon/lib || {
# Alternative: if submodule isn't initialized, check what we know from commit info
echo "Submodule not initialized for detailed log. Checking parent repo info..."
git log -1 --format="%B" 6b9824d047b81a262d6d0bf83412fa779bd61624 2>/dev/null || echo "Cannot access commit history"
}
echo -e "\n=== Submodule stat diff between commits ==="
git diff 6732cfae4b692ecb714bd664012a4e68eaa62b43 6b9824d047b81a262d6d0bf83412fa779bd61624 -- src/main/java/frc/trigon/lib
echo -e "\n=== Look for PR comments or issue references ==="
git log --all --grep="subsystem" --oneline | head -10
git log --all --grep="lib" --oneline | head -10Repository: Programming-TRIGON/RobotTemplate
Length of output: 709
🏁 Script executed:
#!/bin/bash
# Check what files changed in this PR context
echo "=== Files in src/main/java that might use lib imports ==="
find src/main/java -name "*.java" -type f | head -20
echo -e "\n=== Sample imports from a few files ==="
find src/main/java -name "*.java" -type f | head -5 | xargs grep -h "import.*trigon.*lib" | sort -u | head -10Repository: Programming-TRIGON/RobotTemplate
Length of output: 2026
Don't track mutable branch for submodule—use stable commit or tag.
The .gitmodules configuration specifies branch = subsystem-wrapper, which means this submodule reference will track a development branch. This introduces reproducibility issues and makes it difficult to pin the library to a known-good state, especially since the codebase has multiple dependencies on lib imports.
Update the submodule to reference a specific commit hash or tagged release instead of a branch.
| import frc.trigon.robot.subsystems.flyWheel.FlyWheel; | ||
| import frc.trigon.robot.subsystems.flyWheel.FlyWheelConstants; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Package name flyWheel should be lowercase.
Java package naming convention uses all lowercase. Consider renaming to flywheel for consistency.
| private static final int GYRO_ID = 0; | ||
| private static final String GYRO_NAME = "SwerveGyro"; | ||
| static final Pigeon2Gyro GYRO = new Pigeon2Gyro(GYRO_ID, GYRO_NAME, RobotConstants.CANIVORE_NAME); | ||
| static final Pigeon2Gyro GYRO = new Pigeon2Gyro(GYRO_ID, GYRO_NAME); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the Pigeon2Gyro class definition
find . -type f -name "*.java" | xargs grep -l "class Pigeon2Gyro"Repository: Programming-TRIGON/RobotTemplate
Length of output: 60
🏁 Script executed:
# Find SwerveConstants.java to see the actual code
find . -type f -name "SwerveConstants.java" | head -5Repository: Programming-TRIGON/RobotTemplate
Length of output: 147
🏁 Script executed:
# Search for Pigeon2Gyro usage and constructor patterns
rg "new Pigeon2Gyro" -A 2 -B 2Repository: Programming-TRIGON/RobotTemplate
Length of output: 627
🏁 Script executed:
cat -n src/main/java/frc/trigon/robot/subsystems/swerve/SwerveConstants.java | head -50Repository: Programming-TRIGON/RobotTemplate
Length of output: 3036
🏁 Script executed:
cat -n src/main/java/frc/trigon/robot/subsystems/swerve/SwerveConstants.java | sed -n '100,115p'Repository: Programming-TRIGON/RobotTemplate
Length of output: 986
🏁 Script executed:
# Search for MountPose usage and configuration
rg "MountPose" -B 2 -A 2Repository: Programming-TRIGON/RobotTemplate
Length of output: 978
🏁 Script executed:
find . -type f -name "*.java" -path "*pigeon2*" | head -20Repository: Programming-TRIGON/RobotTemplate
Length of output: 58
🏁 Script executed:
# Look for Pigeon2Gyro source in the lib directory
fd "Pigeon2Gyro" --type fRepository: Programming-TRIGON/RobotTemplate
Length of output: 58
🏁 Script executed:
find . -name "build.gradle*" -o -name "pom.xml" | head -5Repository: Programming-TRIGON/RobotTemplate
Length of output: 90
🏁 Script executed:
# Search for any other MountPose or Pigeon2Configuration usage
rg "Pigeon2Configuration" -B 3 -A 3Repository: Programming-TRIGON/RobotTemplate
Length of output: 1577
🏁 Script executed:
# Check if there are any comments or documentation about the MountPose fields
rg "MountPose" -B 5 -A 5Repository: Programming-TRIGON/RobotTemplate
Length of output: 1742
🏁 Script executed:
# Check the build.gradle to understand dependencies
cat build.gradleRepository: Programming-TRIGON/RobotTemplate
Length of output: 5261
🏁 Script executed:
# Search for any other Pigeon2Gyro constructor calls to see patterns
rg "new Pigeon2Gyro" -B 3 -A 3Repository: Programming-TRIGON/RobotTemplate
Length of output: 829
🏁 Script executed:
# Look for any CANbus or bus configuration references
rg "CANbus|CANivore|getCANbus|setBus" -iRepository: Programming-TRIGON/RobotTemplate
Length of output: 201
🏁 Script executed:
# Check if there are any other gyro initialization patterns or comments
rg "GYRO|Pigeon2" | grep -v "test" | head -30Repository: Programming-TRIGON/RobotTemplate
Length of output: 2206
🏁 Script executed:
# Check RobotConstants to see CANIVORE_NAME usage
rg "CANIVORE_NAME" -B 2 -A 2Repository: Programming-TRIGON/RobotTemplate
Length of output: 705
🏁 Script executed:
# Search for any git history or comments that might indicate why CANIVORE_NAME was removed
rg "CANIVORE|CANivore" -B 3 -A 3Repository: Programming-TRIGON/RobotTemplate
Length of output: 935
🏁 Script executed:
# Check if there are any other motor/sensor initializations to see CAN bus patterns
rg "new.*\(.*CANIVORE_NAME\)" -B 2 -A 2Repository: Programming-TRIGON/RobotTemplate
Length of output: 58
🌐 Web query:
CTRE Pigeon2Configuration MountPose field units degrees rotations
💡 Result:
MountPoseYaw / MountPosePitch / MountPoseRoll are doubles expressed in degrees — they are the Yaw‑Pitch‑Roll rotation angles that describe how the Pigeon2 was rotated to reach its current mounting orientation (i.e., mounting-calibration rotations). Example docs: pointing directly right = yaw -90°, pointing up = pitch -90°. [1][2][3]
Sources:
- CTRE Phoenix Pigeon2Configuration / MountPoseConfigs (fields are doubles — yaw/pitch/roll). [1][2]
- Pigeon2 configMountPose() examples (angles described in degrees; example mappings). [3]
Fix MountPose unit conversion — should be degrees, not rotations.
The MountPose fields in Pigeon2Configuration expect values in degrees. The current code converts degrees to rotations via .getRotations(), making the mount pose values incorrect by a factor of ~360.
Remove the .getRotations() conversion:
config.MountPose.MountPoseYaw = Rotation2d.fromDegrees(-11.623535).getDegrees();
Or simpler, assign the degree values directly:
config.MountPose.MountPoseYaw = -11.623535;
Also verify: The Pigeon2Gyro constructor change removed the CANIVORE_NAME parameter. Confirm the gyro is on the RIO CAN bus or that Pigeon2Gyro auto-detects the bus internally, since CANIVORE_NAME is still defined in RobotConstants but now unused.
| config.MountPose.MountPoseYaw = Rotation2d.fromDegrees(-11.623535).getRotations(); | ||
| config.MountPose.MountPosePitch = Rotation2d.fromDegrees(-0.395508).getRotations(); | ||
| config.MountPose.MountPoseRoll = Rotation2d.fromDegrees(-0.043945).getRotations(); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
CTRE Phoenix 6 Pigeon2Configuration MountPose units documentation
💡 Result:
MountPoseYaw / MountPosePitch / MountPoseRoll use degrees (units = deg). Allowed range: -360 to 360, default 0. [1][2]
Sources:
[1] MountPoseConfigs (CTRE Phoenix 6 source).
[2] Pigeon2Configuration (CTRE Phoenix 6 API docs).
🌐 Web query:
WPILib Rotation2d getRotations method degrees to rotations conversion
💡 Result:
getRotations() = angle in degrees ÷ 360 (equivalently radians ÷ 2π).
Example: Rotation2d.fromDegrees(90).getRotations() → 0.25. [1][2]
Sources: WPILib Rotation2d API and WPILib geometry docs. [1] Rotation2d API. [2] WPILib Rotation/pose documentation.
Unit mismatch: MountPose expects degrees, not rotations.
Rotation2d.fromDegrees(...).getRotations() converts degrees to rotations (divides by 360). CTRE's MountPose fields expect values in degrees. This results in mount pose compensation being ~360x smaller than intended.
If the source values are already in degrees, set them directly:
Suggested fix
- config.MountPose.MountPoseYaw = Rotation2d.fromDegrees(-11.623535).getRotations();
- config.MountPose.MountPosePitch = Rotation2d.fromDegrees(-0.395508).getRotations();
- config.MountPose.MountPoseRoll = Rotation2d.fromDegrees(-0.043945).getRotations();
+ config.MountPose.MountPoseYaw = -11.623535;
+ config.MountPose.MountPosePitch = -0.395508;
+ config.MountPose.MountPoseRoll = -0.043945;📝 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.
| config.MountPose.MountPoseYaw = Rotation2d.fromDegrees(-11.623535).getRotations(); | |
| config.MountPose.MountPosePitch = Rotation2d.fromDegrees(-0.395508).getRotations(); | |
| config.MountPose.MountPoseRoll = Rotation2d.fromDegrees(-0.043945).getRotations(); | |
| config.MountPose.MountPoseYaw = -11.623535; | |
| config.MountPose.MountPosePitch = -0.395508; | |
| config.MountPose.MountPoseRoll = -0.043945; |
| private boolean | ||
| willOptimize(SwerveModuleState state) { | ||
| final Rotation2d angularDelta = state.angle.minus(getCurrentSteerAngle()); | ||
| return Math.abs(angularDelta.getRadians()) > Math.PI / 2; | ||
| return true; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for all references to willOptimize in the codebase
rg -n "willOptimize" --type javaRepository: Programming-TRIGON/RobotTemplate
Length of output: 203
🏁 Script executed:
#!/bin/bash
# Check the setTargetState method to see context around where willOptimize might have been intended
sed -n '50,60p' src/main/java/frc/trigon/robot/subsystems/swerve/swervemodule/SwerveModule.javaRepository: Programming-TRIGON/RobotTemplate
Length of output: 469
Remove the unused willOptimize method.
The method computes angularDelta but never uses it and unconditionally returns true. Since it's not called anywhere and setTargetState already handles optimization directly via targetState.optimize(), this method is dead code.
No description provided.