wpilibsuite / allwpilib

Official Repository of WPILibJ and WPILibC
https://wpilib.org/
Other
1.08k stars 611 forks source link

Unclear Exception when trying to execute a SequentialCommandGroup periodically #5226

Closed jonathandao0 closed 1 year ago

jonathandao0 commented 1 year ago

Describe the bug I was helping as a CSA over the weekend with a team that did something improper with running autonomous commands.It took me a while to figure out the root cause, but once I did, it was easy to fix. The StackTrace reported from the error wasn't very clear as to what was causing the issue, which prevented me from resolving it more quickly.

To Reproduce Use the following code:

Robot.java

public class Robot extends TimedRobot {
  private Command m_autonomousCommand;

  private RobotContainer m_robotContainer;

  @Override
  public void robotInit() {
    m_robotContainer = new RobotContainer();
  }

  @Override
  public void robotPeriodic() {
    CommandScheduler.getInstance().run();
  }

  @Override
  public void autonomousInit() {
    m_autonomousCommand = m_robotContainer.getAutonomousCommand();

    if (m_autonomousCommand != null) {
      m_autonomousCommand.schedule();
    }
  }

  @Override
  public void autonomousPeriodic() {
    // Invalid way of running autonomous commands
    m_autonomousCommand.execute();
  }

RobotContainer.java

public class RobotContainer {
  private final SendableChooser<Command> m_autoChooser = new SendableChooser<>();

  public RobotContainer() {
    m_autoChooser.setDefaultOption("Do Nothing", new WaitCommand(0));
    m_autoChooser.addOption("Auto 1", new PrintCommand("Auto 1"));
    m_autoChooser.addOption("Auto 2", new PrintCommand("Auto 2A").andThen(new PrintCommand("Auto 2B")));

    SmartDashboard.putData(m_autoChooser);
  }

  public Command getAutonomousCommand() {
    return m_autoChooser.getSelected();
  }
}

Steps to reproduce the behavior When running this code, you can select and run the "Do Nothing" and "Auto 1" autonomous commands with no issues. However, when you select and run "Auto 2". the auto will execute successfully, but crash the code immedately afterwards with the following StackTrace:

Auto 2A
Auto 2B
Error at java.base/jdk.internal.util.Preconditions.outOfBounds(Preconditions.java:64): Unhandled exception: java.lang.IndexOutOfBoundsException: Index -1 
out of bounds for length 2
        at java.base/jdk.internal.util.Preconditions.outOfBounds(Preconditions.java:64)
        at java.base/jdk.internal.util.Preconditions.outOfBoundsCheckIndex(Preconditions.java:70)
        at java.base/jdk.internal.util.Preconditions.checkIndex(Preconditions.java:266)
        at java.base/java.util.Objects.checkIndex(Objects.java:359)
        at java.base/java.util.ArrayList.get(ArrayList.java:427)
        at edu.wpi.first.wpilibj2.command.SequentialCommandGroup.execute(SequentialCommandGroup.java:71)
        at frc.robot.Robot.autonomousPeriodic(Robot.java:47)
        at edu.wpi.first.wpilibj.IterativeRobotBase.loopFunc(IterativeRobotBase.java:344)
        at edu.wpi.first.wpilibj.TimedRobot.startCompetition(TimedRobot.java:130)
        at edu.wpi.first.wpilibj.RobotBase.runRobot(RobotBase.java:343)
        at edu.wpi.first.wpilibj.RobotBase.lambda$startRobot$0(RobotBase.java:413)
        at java.base/java.lang.Thread.run(Thread.java:833)

The error is due to "Auto 2" being a SequentialCommandGroup and trying to execute again once it has completed. When the SequentialCommandGroup ends, the list index resets to -1. Looking into this error more, it is similar to a previous bug report (https://github.com/wpilibsuite/allwpilib/issues/2302).

I'm not exactly sure how this should be handled. Ideally, this should silently fail or provide a warning as the SequentialCommandGroup does run successfully in this case. Otherwise, perhaps a more clear exception should be thrown in this case or the code should crash/error out when the autonomous command is executed again outside of the CommandScheduler when run in autonomousPeriodic? Normally, users should never put m_autonomousCommand.execute() in autonomousPeriodic, so I'm not sure this really warrents a change in the first place.

Starlight220 commented 1 year ago

Calling command lifecycle methods out of order is undefined behavior and shouldn't be done. Checking for this in command impls (some of which are in team code!) is a significant burden, and isn't feasible to do everywhere.