Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new utility class, Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a new utility class ClockAndHubStatus for game-specific logic related to match time and hub status. A critical security vulnerability was identified in the isGameDataValid method, where an empty game data string could cause a StringIndexOutOfBoundsException, leading to a denial-of-service by crashing the robot's control program. Beyond this, a significant logic flaw in getTargetLocation was found, which could prevent compilation or cause incorrect behavior. There are also opportunities to improve code clarity and maintainability by removing unused code, defining magic numbers as constants, and improving coordinate point declarations.
I am having trouble creating individual review comments. Click here to see my feedback.
src/main/java/frc/robot/util/ClockAndHubStatus.java (18-50)
The logical structure of getTargetLocation is flawed and will likely result in a compilation error (e.g., unreachable code) or incorrect runtime behavior. The if (AllianceUtils.isRed()) block is followed by a separate if (AllianceUtils.isBlue()) block, instead of being chained with else if. This means after returning a value in the red alliance case, the code would attempt to continue execution, which is not valid. The method should be restructured to use a proper if-else if-else chain for checking the alliance.
public Translation2d getTargetLocation(CommandSwerveDrivetrain drivetrain){
if(isHubActive(0)){
return AllianceUtils.getHubTranslation2d();
}
else {
if(AllianceUtils.isRed()){
if(drivetrain.getState().Pose.getX() <= Units.inchesToMeters(158.6)){
return AllianceUtils.getHubTranslation2d();
}
else if(drivetrain.getState().Pose.getY() >= Units.inchesToMeters(317.7/2)){
return pointLeftFieldTop;
}
else{
return pointLeftFieldBottom;
}
} else if (AllianceUtils.isBlue()){
if(drivetrain.getState().Pose.getX() >= Units.inchesToMeters(651.2-158.6)){
return AllianceUtils.getHubTranslation2d();
}
else if(drivetrain.getState().Pose.getY() >= Units.inchesToMeters(317.7/2)){
return pointRightFieldBottom;
}
else{
return pointRightFieldTop;
}
} else {
return AllianceUtils.getHubTranslation2d();
}
}
}src/main/java/frc/robot/util/ClockAndHubStatus.java (111-124)
The method isGameDataValid is vulnerable to a StringIndexOutOfBoundsException because it calls gameData.charAt(0) without checking if gameData is empty. If DriverStation.getGameSpecificMessage() returns an empty string (e.g., if game data isn't received from FMS), this will crash the robot's control program, leading to a denial of service during a match. A guard clause is needed to prevent this, and the logic can also be simplified for better readability.
public boolean isGameDataValid(){
String gameData = DriverStation.getGameSpecificMessage();
if (gameData.isEmpty()) {
return false;
}
switch (gameData.charAt(0)) {
case 'R' :
return true;
case 'B' :
return true;
default : {
return false;
}
}
}src/main/java/frc/robot/util/ClockAndHubStatus.java (4)
This import edu.wpi.first.wpilibj2.command.Command is not used in the file and can be removed.
src/main/java/frc/robot/util/ClockAndHubStatus.java (11)
The field matchLength is not used anywhere in the class and can be removed to clean up the code.
src/main/java/frc/robot/util/ClockAndHubStatus.java (13-16)
These Translation2d fields should be declared as private static final constants since their values do not change. Additionally, their names (pointLeftFieldTop, etc.) are ambiguous. Consider renaming them to reflect which alliance they are for, to improve clarity (e.g., RED_ALLIANCE_FALLBACK_TOP). The indentation for these lines is also inconsistent with the rest of the class and should be fixed.
private static final Translation2d pointLeftFieldTop = new Translation2d(2,2);
private static final Translation2d pointLeftFieldBottom = new Translation2d(2,6);
private static final Translation2d pointRightFieldTop = new Translation2d(14,2);
private static final Translation2d pointRightFieldBottom = new Translation2d(14,6);src/main/java/frc/robot/util/ClockAndHubStatus.java (90-108)
The isHubActive method uses several "magic numbers" for match time thresholds (e.g., 130, 105, 80, 55, 30). These numbers make the code harder to read and maintain. It's better to define them as private static final constants with descriptive names at the top of the class. This also applies to the field dimensions used in getTargetLocation (e.g., 158.6, 317.7, 651.2).
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new utility class "ClockAndHubStatus" to manage game-specific logic, determining target locations and tracking hub status based on game time and alliance. A security review identified a potential Denial of Service (DoS) vulnerability in the "isGameDataValid" method, where an unhandled empty string from the Field Management System (FMS) could cause the robot code to crash. Additionally, my review suggests improving code clarity and maintainability by refactoring confusing control flow and extracting magic numbers into named constants.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces HubShiftUtil to manage game clock and hub status during different phases of the match. It also updates LauncherConstants to provide necessary time-of-flight data. The overall logic is sound, and the identified areas in the new HubShiftUtil class for simplification and maintainability have been retained as they align with general best practices and do not contradict any specific repository rules.
…ClockAndHubStatus
…ClockAndHubStatus
| if (!winOverride.isEmpty()) { | ||
| return winOverride.get() | ||
| ? (alliance == Alliance.Blue ? Alliance.Red : Alliance.Blue) | ||
| : (alliance == Alliance.Blue ? Alliance.Blue : Alliance.Red); |
There was a problem hiding this comment.
Isn't this line redundant? Just return alliance in this case.
Removed unnecessary merge conflict markers from imports.
…ClockAndHubStatus
…ILT2026 into ClockAndHubStatus
| setHardwareColor(color); | ||
|
|
||
| currentPatternPub.set("SOLID"); | ||
| currentPatternPub.set(currentPattern.name()); |
There was a problem hiding this comment.
[nit] I don't think you need the currentPattern variable. You can just do:
currentPatternPub.set(LEDPattern.SOLID.name());
| import lombok.Setter; | ||
|
|
||
| public class ClockAndHubStatus { | ||
| public double matchLength = 2.5; |
There was a problem hiding this comment.
Please add a comment with units. Is this minutes?
| public class ClockAndHubStatus { | ||
| public double matchLength = 2.5; | ||
|
|
||
| private Translation2d pointLeftFieldTop = new Translation2d(2, 6); |
There was a problem hiding this comment.
Should the field variables be in a Field.java file? Will other subsystems want or need this info?
| private double allianceLineX = Units.inchesToMeters(158.6); | ||
| private double robotOffset = Units.inchesToMeters(15); | ||
|
|
||
| private static final double TRANSITION_PERIOD_END_TIME = 130; |
There was a problem hiding this comment.
Add a comment with units. (seconds?)
| } | ||
|
|
||
| if (AllianceUtils.isBlue()) { | ||
| if (drivetrain.getState().Pose.getX() <= allianceLineX + robotOffset) { |
There was a problem hiding this comment.
Call drivetrain.getState().Pose once. It can change each time you call it in this if() tree.
Pose2d currentPose = drivetrain.getState().Pose;
if (currentPose.getX() <= allianceLineX + robotOffset) {
...
} else if (currentPost.getY() >= (fieldWidth / 2)) {
| } | ||
| } | ||
|
|
||
| /** Starts the timer at the begining of teleop. */ |
There was a problem hiding this comment.
[nit] spelling: "beginning"
| // If we have invalid game data, assume hub is active. | ||
| return true; | ||
| } | ||
| } |
There was a problem hiding this comment.
Code beyond this point is not reachable as the routine will return from this switch statement regardless of the value of gameData.charAt(0).
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the ClockAndHubStatus utility for match-time-based logic and refactors HubShiftUtil to use a more dynamic shift adjustment system. It also implements an autoShoot trigger in Controls and centralizes NetworkTables state publishing within LEDSubsystem. Review feedback highlights a critical logic error in ClockAndHubStatus where premature returns bypass shift calculations, along with suggestions to remove an unnecessary utility class instantiation and redundant code in the LED and alliance logic.
| if (message.length() > 0) { | ||
| char character = message.charAt(0); | ||
| if (character == 'R') { | ||
| return redInactiveFirst = true; | ||
| } else if (character == 'B') { | ||
| return redInactiveFirst = false; | ||
| } | ||
| } |
There was a problem hiding this comment.
| // continue until interrupted by another command, remove | ||
| // this line or comment it out. | ||
|
|
||
| HubShiftUtil instance = new HubShiftUtil(); |
| currentPatternPub.set(currentPattern.name()); | ||
| currentColorPub.set(color.toHexString()); | ||
| alternatingColorsPub.set("A:None | B:None"); |
There was a problem hiding this comment.
| if (!winOverride.isEmpty()) { | ||
| return winOverride.get() | ||
| ? (alliance == Alliance.Blue ? Alliance.Red : Alliance.Blue) | ||
| : (alliance == Alliance.Blue ? Alliance.Blue : Alliance.Red); |
…ClockAndHubStatus
…ClockAndHubStatus
No description provided.