Feature/shift UI display

[y0shi]

No description provided.

[github-actions]

AI Code Review

Hey team!

This PR, "Feature/shift UI display", looks like a fantastic addition to our robot's driver feedback system! Providing clear visual cues and haptic feedback based on match shifts and scoring windows will be incredibly helpful for our drivers and operators, especially with the complex game dynamics we anticipate.

Positive Highlights

• Command Factory Methods: I love seeing the timedRumbleCommand and manualRumbleCommand in UISubsystem.java. This is a great example of exposing behavior as commands rather than public setters, which aligns perfectly with our command-based best practices.
• State Machine Pattern: The FeedbackState enum in UISubsystem is a clear and effective way to manage the UI's different visual and haptic states. This makes the logic easy to follow and robust.
• Clear Constants and Preferences: The new UIConstants.java and UIPreferences.java files are well-organized, using UPPER_SNAKE_CASE for constants and DoublePreference for tunable values. This promotes readability and makes it easy to adjust feedback timings.

Suggestions

ShiftState.java

1. FRC Match Timing Consistency:

   • Observation: The constant TELEOP_START_TIME is set to 140.0, but the comment mentions "20s auto per 2026 REBUILT". Standard FRC matches have a 15-second autonomous period. If DriverStation.getMatchTime() starts at 150 seconds (total match duration), then a 15-second auto means teleop starts when getMatchTime() is 135.0 seconds. If it's a 20-second auto, teleop would start at 130.0 seconds.
   • Suggestion: Could we clarify what TELEOP_START_TIME = 140.0 represents? If it's a custom team rule for a shorter auto (10 seconds), that's perfectly fine, but it might be helpful to update the comment to reflect that, or adjust the value to 130.0 if a 20-second auto is the actual target. Consistent timing is crucial for the UI to be accurate!
   • Why it matters: Inaccurate timing constants could lead to the UI feedback (colors, rumble) being out of sync with the actual match phases, potentially confusing drivers.

2. getShiftCountdownString() Logging:

   • Observation: The getShiftCountdownString() method is annotated with @Logged(name = "Shift Countdown"). This is great for debugging!
   • Suggestion: Consider adding a SmartDashboard.putString("Shift/Countdown", getShiftCountdownString()); call in the periodic() method of ShiftState as well. While @Logged handles AdvantageKit logging, explicitly putting it on SmartDashboard makes it immediately visible during live matches without needing to open the AdvantageScope log viewer.

UISubsystem.java

1. getTimeUntilNextTransition() - Handling AUTONOMOUS and ENDGAME:

   • Observation: In getTimeUntilNextTransition(), for AUTONOMOUS, you return matchTime - ShiftState.TELEOP_START_TIME. For ENDGAME, you return -1.
   • Suggestion: The AUTONOMOUS calculation is correct for time remaining until teleop starts. However, for ENDGAME, returning -1 is a good sentinel value, but it might be worth considering what feedback the UI should give during endgame. The getEndgameRumbleIntensity() already handles this, so the -1 is fine for shift transitions. Perhaps a comment in the ENDGAME case could explicitly state "No further shift transitions expected" to clarify the -1 return.
   • Why it matters: Clarity in edge cases helps future maintainers understand the system's intended behavior.

2. Rumble Intensity Constants:

   • Observation: In timedRumbleCommand, the rumble intensity is hardcoded as 0.5.
   • Suggestion: Consider defining a constant in UIConstants.java for TIMED_RUMBLE_INTENSITY (or similar) and using that here. This would make it easier to adjust all timed rumble effects from a central location.
   • Why it matters: Centralizing constants improves maintainability and consistency.

3. Javadoc for manualRumble and stopManualRumble:

   • Observation: The manualRumbleCommand and timedRumbleCommand have excellent Javadoc. The private manualRumble and stopManualRumble methods also have Javadoc, which is good.
   • Suggestion: While these are private, sometimes adding Javadoc for private methods can be overkill. However, since they are directly called by the public command factories and describe specific rumble behavior, the existing Javadoc is probably fine and helpful here! Just a thought if you ever want to reduce verbosity.

Questions

• Regarding the TELEOP_START_TIME in ShiftState.java, is the 140.0 a specific team decision for a 10-second auto, or was the "20s auto" comment meant to imply a 130.0 value?
• Are there any other specific UI elements (beyond the scoring status color and rumble) that you envision being controlled by this subsystem in the future?

This is a really solid implementation, and I'm excited to see how it enhances our driver's experience on the field! Keep up the great work!

---

This review was automatically generated by AI. Please use your judgment and feel free to discuss any suggestions!

[github-actions]

✓ Build successful and code formatting check passed!

[y0shi]

closing. want to make changes.