[Ginto8]

Originally Posted by DaveFrederick

Update to the draft. About 40% complete.
Lots more to learn.

Dave Frederick, Mentor
Team 1895, Manassas, VA

I've been looking through your presentation, and it seems pretty good. I have some constructive criticism:

• On slide 11 (talking about the RobotTemplate class), you talk about the "IterativeRobot" approach as something entirely separate from the Command approach. IterativeRobot is actually part of the Command-based approach, and can be used exactly like a typical IterativeRobot. The big difference between using IterativeRobot normally (putting all robot logic in either the periodic() or continuous() functions) and using Commands is that the periodic() functions MUST call

  Scheduler.getInstance().run();

  This call handles all the details of executing and stopping Commands. In fact, you could simultaneously use the normal IterativeRobot approach and the Command approach... it would just be a terrible headache
  
• This is more of a design concern than anything being wrong, but having two different Commands (LED_Off and LED_On) for a single idea (setting the LED's state) seems unnecessary and a bit cumbersome. Having a LED_Set Command that takes a boolean in the constructor would require less code, and would only change
  

  button4.whenPressed(new LED_Off());
  button5.whenPressed(new LED_On());

  to
  

  button4.whenPressed(new LED_Set(false));
  button5.whenPressed(new LED_Set(true));

  To make this easier, IndicatorLED could have a set() method that looked like this:
  

  public void set(boolean state) {
      LED_one.set(!state); // since false == on for DigitalOutputs, negate state
  }

• In slides 18 and 19 ("Create the commands used on the subsystem"), declaring a local copy of CommandBase's static variable indicatorLED isn't necessary. Because LED_On inherits from CommandBase, it can access all protected and public members of CommandBase, so the local version isn't necessary. The constructor then can become more concise:
  

  requires(indicatorLED);

  The Scheduler only allows one running Command to require() a subsystem at a time, so that there isn't any fighting over resources. Take for example, two commands, TankDrive and ArcadeDrive. Both would require() the driveTrain Subsystem. If both were running at once, the results would be... surprising, to say the least. But since they both require() driveTrain, when one starts the other is automatically stopped so they don't cause trouble.
  
  A full example implementation of LED_Set (assuming IndicatorLED has a set() method as suggested before) would be:
  

  public class LED_Set extends CommandBase {
      private boolean chosenState;
      public LED_Set(boolean state) {
          requires(indicatorLED);
          chosenState = state;
      }
  
      protected void initialize() {
          indicatorLED.set(chosenState);
      }
  
      protected void execute() {}
  
      protected boolean isFinished() {
          return true;
      }
  
      protected void end() {}
      
      protected void interrupted() {}
  }

• On slide 22 ("Bind a joystick action to the command (within the OI Class)"), you say you don't understand what the static "instance" variable is for. This variable allows OI to be a lazy singleton, or more correctly a singleton with lazy initizialization. The basic pattern is this:
  

  public class Singleton {
      private static Singleton instance;
      private Singleton() {}
      public static Singleton getInstance() {
          if(instance == null) {
              instance = new Singleton();
          }
          return instance;
      }
  }

  This fulfills two requirements: 1) that only one instance of the class exists, and 2) that the instance isn't initialized until it's used.
  
• On slide 24 ("Final configuration of commands"), the idea of the two Commands autonomousCommand and teleopCommand is that you can easily start and stop all autonomous/teleop activity by simply start()ing and cancel()ing the corresponding command. To make a single command perform multiple actions, just inherit from CommandGroup, like this:
  

  public class TeleopTemplate extends CommandGroup {
      public TeleopTemplate() {
          addSequential(new FirstCommand()); // sequential commands run one after the other
          addParallel(new ParallelCommand()); // this command will run at the same time as SecondCommand and ThirdCommand
          addSequential(new SecondCommand());
          addSequential(new ThirdCommand());
          addSequential(new WaitForChildren()); // wait for ParallelCommand to finish
          addSequential(new FourthCommand()); // Will only run after both ThirdCommand and ParallelCommand have completed
      }
  }

  WaitForChildren is a Command provided by WPILib, the rest can be any command you create.
  
• On slide 36 (when implementing the DynamicServoAngle command), I'm not sure why you say you need to instantiate the joystick on each execute()... To me it seems that it shouldn't work, as I'm under the impression that multiple instances of the same Joystick would cause errors. Joysticks are typically supposed to remain in the OI class; in my code I don't even expose them directly, instead having functions like getDriveRightAxis() and getDriveLeftAxis(). eg:
  

  public class OI {
      // ... other stuff ...
      private Joystick joyServo = new Joystick(SERVO_JOYSTICK_PORT);
      public getServoAxis() {
          return joyServo.getRawAxis(1);
      }
  }