-
Notifications
You must be signed in to change notification settings - Fork 9
AutoBuilder now builds upon selection rather than auto init #200
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request aims to optimize autonomous mode initialization by pre-building the auto command when it's selected on Shuffleboard. While this is a good optimization, the current implementation introduces critical bugs. Specifically, the autonomous command field (auton) is not initialized at startup, which will cause a NullPointerException if the match starts without an auto being selected. The onChange handler for auto selection can also cause a NullPointerException. Furthermore, the configurable delay for autonomous mode has been unintentionally removed. My review provides suggestions to fix these critical issues while preserving the intended optimization.
src/main/java/frc/robot/Robot.java
Outdated
| if (SubsystemConstants.DRIVEBASE_ENABLED && AutoLogic.getSelectedAuto() != null) { | ||
| AutoLogic.getSelectedAuto().schedule(); | ||
| if (SubsystemConstants.DRIVEBASE_ENABLED) { | ||
| AutoLogic.auton.schedule(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This direct call to AutoLogic.auton.schedule() introduces two critical issues:
-
NullPointerExceptionRisk: As implemented in this PR, theAutoLogic.autonfield is not initialized at startup. If autonomous mode starts without any Shuffleboard interaction,autonwill benull, causing aNullPointerException. -
Missing Auto Delay: The previous implementation using
AutoLogic.getSelectedAuto()incorporated a delay configured from Shuffleboard. This new implementation bypasses that logic, so the auto delay will no longer work. This is a functional regression.
I've suggested a corresponding fix in AutoLogic.java to initialize auton. To restore the delay functionality, you should add a null check here and prepend the delay to the command before scheduling. This will require a new public getAutoDelay() method in AutoLogic.
| AutoLogic.auton.schedule(); | |
| if (AutoLogic.auton != null) { | |
| var autoCommand = edu.wpi.first.wpilibj2.command.Commands.waitSeconds(AutoLogic.getAutoDelay()).andThen(AutoLogic.auton); | |
| autoCommand.schedule(); | |
| } |
| availableAutos.onChange((dummyVar) -> auton = AutoBuilder.buildAuto(namesToAuto.get(availableAutos.getSelected()).getAutoName())); | ||
|
|
||
| filterAutos(gameObjects.getSelected()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation for pre-building the autonomous command has a couple of critical issues:
-
NullPointerExceptionon startup: Theautoncommand is only built when the auto selection changes on Shuffleboard. It is not initialized at startup, so it will benullif no selection is made before autonomous mode begins. This will lead to aNullPointerExceptioninRobot.java. -
NullPointerExceptioninonChange: TheonChangehandler is not safe and can throw aNullPointerExceptionif an auto is selected that is not in thenamesToAutomap (like the default "do nothing" auto).
To fix this, auton should be initialized with a default command after the choosers are set up, and the onChange handler needs to be robust against missing paths.
I've also suggested a change in Robot.java to handle the auto delay, which was missed in this PR. That change will require a new public getter method in this class:
public static double getAutoDelay() {
if (autoDelayEntry != null) {
return autoDelayEntry.getDouble(0);
}
return 0.0;
} availableAutos.onChange(
(selectedName) -> {
AutoPath path = namesToAuto.get(selectedName);
if (path == null) {
path = defaultPath; // Fallback to default path if not found
}
auton = AutoBuilder.buildAuto(path.getAutoName());
});
filterAutos(gameObjects.getSelected());
// Initialize auton with the default auto command
{
AutoPath path = namesToAuto.get(availableAutos.getSelected());
if (path == null) {
path = defaultPath;
}
auton = AutoBuilder.buildAuto(path.getAutoName());
}
Optimization improvements to the AutoBuilder. The Autos would be preloaded and ready as soon as a new Auto is selected, rather than having to do all that at the start of Auto.