Added LED stuff #67
Added LED stuff #67
Conversation
WalkthroughThis pull request updates various constants and restructures branch position calculations in the robot’s configuration. The LED subsystem is enhanced by adding animation capabilities and new commands. Modifications in the robot code now trigger LED animations during disabled periods. New files introduce the Lights subsystem, an LED interface, and LED command classes, as well as a utility Color class. Build constants are refreshed with updated Git and build timestamp information, and the LED subsystem is integrated into the RobotContainer. Changes
Sequence Diagram(s)sequenceDiagram
participant R as Robot
participant LEDSeg as LEDSegment
R->>LEDSeg: setFadeAnimation(Lights.red, 0.5)
LEDSeg-->>R: Animation configured
sequenceDiagram
participant RC as RobotContainer
participant LEDSub as LEDSubsystem
participant CAN as CANdle
RC->>LEDSub: Instantiate LEDSubsystem(CANDLE_ID)
LEDSub->>CAN: Initialize CANdle with LED settings
RC->>LEDSub: setDefaultCommand(SetLEDColorCommand)
SetLEDColorCommand->>LEDSub: setColor(r, g, b)
LEDSub->>CAN: setLEDs(r, g, b)
CAN-->>LEDSub: LED color set
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Note 🎁 Summarized by CodeRabbit FreeYour organization has reached its limit of developer seats under the Pro Plan. For new users, CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please add seats to your subscription by visiting https://app.coderabbit.ai/login.If you believe this is a mistake and have available seats, please assign one to the pull request author through the subscription management page using the link above. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
src/main/java/org/team5924/frc2025/Robot.java (1)
147-149: Move LED animation setup to disabledInit to prevent animation restartSetting the fade animation in
disabledPeriodic()will restart the animation on every periodic call (likely 50 times per second), which may cause flickering or inconsistent behavior.Consider moving this code to
disabledInit()instead:@Override public void disabledInit() { + LEDSegment.MainStrip.setFadeAnimation(Lights.green, 0.5); } @Override public void disabledPeriodic() { - LEDSegment.MainStrip.setFadeAnimation(Lights.green, 0.5); }This will set up the animation once when entering disabled mode rather than restarting it every loop.
src/main/java/org/team5924/frc2025/subsystems/Lights/Lights.java (4)
35-37: Consider adding error handling for hardware initializationThe CANdle instance is statically initialized with no error handling. If the hardware is disconnected or the CAN ID is incorrect, this could lead to runtime errors.
Add error handling and connection status tracking:
private static CANdle candle; private boolean isConnected = false; public Lights() { try { candle = new CANdle(Constants.CANDLE_ID); // Configuration code... isConnected = true; } catch (Exception e) { isConnected = false; System.err.println("Error initializing CANdle: " + e.getMessage()); } // Rest of constructor... } // Add a method to check connection status public boolean isConnected() { return isConnected; }
94-96: Extract LED strip configuration to constantsThe LED strip configuration (size 300) is hardcoded in the enum. This makes it difficult to adjust if the physical configuration changes.
Move these values to the Constants class:
// In Constants.java public static final int MAIN_STRIP_SIZE = 300; public static final int MAIN_STRIP_START_INDEX = 0; public static final int MAIN_STRIP_ANIMATION_SLOT = 2; // In Lights.java public static enum LEDSegment { - MainStrip(0, 300, 2); + MainStrip(Constants.MAIN_STRIP_START_INDEX, Constants.MAIN_STRIP_SIZE, Constants.MAIN_STRIP_ANIMATION_SLOT);
129-170: Add documentation for animation parametersThe animation methods contain numeric parameters whose meanings aren't immediately clear (e.g., speed values, direction, bounce modes).
Add JavaDoc comments to explain the parameters and their expected ranges:
/** * Sets a color flow animation on this segment. * @param color The color to display * @param speed Animation speed (0.0 to 1.0, with 1.0 being fastest) */ public void setFlowAnimation(Color color, double speed) { // Method implementation... }Also consider normalizing speed parameters to a consistent range (e.g., 0.0-1.0) with clear documentation on what the values mean.
17-172: Add unit tests for LED animationsThere are no visible tests for the LED functionality, which makes it difficult to verify if the animations work as expected.
Consider adding unit tests for the LED animations to verify that they behave as expected, especially given the uncertainty expressed in the PR description ("Does it work? ¯_(ツ)_/¯").
I can help generate test code that simulates the LEDs for verification if you'd like.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
build.gradleis excluded by none and included by nonevendordeps/Phoenix5-5.35.1.jsonis excluded by none and included by none
📒 Files selected for processing (5)
src/main/java/org/team5924/frc2025/Constants.java(1 hunks)src/main/java/org/team5924/frc2025/Robot.java(2 hunks)src/main/java/org/team5924/frc2025/subsystems/Lights/Lights.java(1 hunks)src/main/java/org/team5924/frc2025/subsystems/Lights/LightsIO.java(1 hunks)src/main/java/org/team5924/frc2025/util/PolynomialRegression.java(0 hunks)
💤 Files with no reviewable changes (1)
- src/main/java/org/team5924/frc2025/util/PolynomialRegression.java
🔇 Additional comments (3)
src/main/java/org/team5924/frc2025/Constants.java (1)
119-121: Verify the CANDLE_ID constant before deploymentThe TODO comment indicates uncertainty about the CAN ID for the CANdle device. Using an incorrect CAN ID could cause communication conflicts with other devices or prevent the LED controller from functioning.
Before deploying this code, verify that:
- The CANdle device is physically configured with ID 50
- No other device is using CAN ID 50 on the same CAN bus
- The ID is documented in your team's CAN ID allocation table/document
Also consider specifying which CAN bus this device is on (like other devices in this file have a BUS constant).
src/main/java/org/team5924/frc2025/Robot.java (1)
36-37: Ensure the Lights subsystem is properly instantiatedThe code uses the LEDSegment class, but there's no visible initialization of the Lights subsystem in this file.
Verify that the Lights subsystem is properly instantiated in RobotContainer or elsewhere before it's used in the disabledPeriodic method. Static access to LEDSegment without proper initialization could lead to null pointer exceptions.
src/main/java/org/team5924/frc2025/subsystems/Lights/Lights.java (1)
19-34: Using Phoenix5 for CANdle as stated in PR descriptionThe imports correctly use Phoenix5 (
com.ctre.phoenix.led) as mentioned in the PR description that CANdle doesn't have Phoenix6 integration yet. This is aligned with the PR objectives.
| public class LightsIO { | ||
| /* Pfft I have no clue what goes here :D */ | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement a proper interface for LED hardware abstraction
The comment "Pfft I have no clue what goes here :D" indicates uncertainty about implementation. Based on typical FRC code architecture, this IO class should likely:
- Define methods for hardware interaction (e.g.,
setLEDs(),configureHardware()) - Follow the team's IO pattern (methods to read/write hardware state)
- Support simulation if your codebase uses it
Consider implementing an interface like:
public interface LightsIO {
public static class LightsIOInputs {
public boolean isConnected = false;
// Add any sensor feedback if available
}
/** Updates the inputs with the latest data from hardware */
public void updateInputs(LightsIOInputs inputs);
/** Configures the LED hardware */
public void configureLEDs();
/** Sets LED color */
public void setLEDs(int r, int g, int b, int startIndex, int count);
/** Clears all animations */
public void clearAllAnimations();
// Add other methods needed for LED control
}| public Lights() { | ||
| CANdleConfiguration candleConfiguration = new CANdleConfiguration(); | ||
| candleConfiguration.statusLedOffWhenActive = true; | ||
| candleConfiguration.disableWhenLOS = false; | ||
| candleConfiguration.stripType = LEDStripType.RGB; | ||
| candleConfiguration.vBatOutputMode = VBatOutputMode.Modulated; | ||
| candle.configAllSettings(candleConfiguration, 100); | ||
|
|
||
| setDefaultCommand(defaultCommand()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider implementing proper integration with LightsIO
The Lights class doesn't integrate with the LightsIO class that was created, missing an opportunity for proper hardware abstraction.
Consider refactoring to use the IO pattern consistently:
public class Lights extends SubsystemBase {
private final LightsIO io;
private final LightsIOInputs inputs = new LightsIOInputs();
public Lights(LightsIO io) {
this.io = io;
io.configureLEDs(); // Initial configuration
setDefaultCommand(defaultCommand());
}
@Override
public void periodic() {
io.updateInputs(inputs);
// Any logging or telemetry
}
// Rest of the methods would use io to control hardware...
}This pattern would make testing easier and follows the separation of concerns principle.
| @Override | ||
| public void disabledPeriodic() {} | ||
| public void disabledPeriodic() { | ||
| LEDSegment.MainStrip.setFadeAnimation(Lights.green, 0.5); |
There was a problem hiding this comment.
I just fixed this 👍
| // Called every time the scheduler runs while the command is scheduled. | ||
| @Override | ||
| public void execute() { | ||
| switch (RobotState.getInstance().getElevatorState()) { |
There was a problem hiding this comment.
Get rid of the default command and move this into the periodic call of LED subsytem
| public static final Color black = new Color(0, 0, 0); | ||
|
|
||
| /* Team-specific Colors */ | ||
| public static final Color international_orange = new Color(255, 79, 0); |
There was a problem hiding this comment.
we have actual hex values we can pull from branding standards if this doesn't look quite right.
There was a problem hiding this comment.
I just fixed this 👍
| public static final Color international_orange = new Color(255, 79, 0); | ||
|
|
||
| public Lights() { | ||
| CANdleConfiguration candleConfiguration = new CANdleConfiguration(); |
There was a problem hiding this comment.
Hardware specific code should never be in the hardware abstract file. Listen to the code rabbit
| } | ||
|
|
||
| public Command defaultCommand() { | ||
| return runOnce( |
There was a problem hiding this comment.
Don't use a default command function here. Just set the default behavior to be part of the "DISABLED" robot state and have all the LED transition logic be based on RobotState
| package org.team5924.frc2025.subsystems.Lights; | ||
|
|
||
| public class LightsIO { | ||
| /* Pfft I have no clue what goes here :D */ |
There was a problem hiding this comment.
Put the functions here that your hardware abstract file needs to call. Any LED strip needs a function to "set color", "set brightness", etc. Also those state fields should be in your auto logged inputs.
I added in code for the LEDs. Does it work? ¯_(ツ)_/¯
Also added Phoenix5 to the vendordep because ain't no Phoenix6 integration for CANdle.
Summary by CodeRabbit
New Features
Refactor