[Elevator] Make 'SmartElevator' subsystem

[gregmarra]

We can create duplicate subsystems, one version "Smart" that uses closed loop control based on encoders and other sensors, and a version "Dumb" that doesn't.

Two Classes

For example, we can make a Smart and Dumb elevator:

• DumbElevator: Only has a "go up" and "go down" command that drives the elevator as long as a button is being held.
• SmartElevator: Has a "go up one position" and "go down one position" command, keeps track of its own current position, and uses an encoder to decide when its reached the new desired positions.

Since we want the robot to be able to work and tested even before we have finished all the mechanical systems, this will let us quickly swap out the code by changing one line:

In Robot.java, we can change public static Elevator m_elevator = new Elevator(); to instead be public static SmartElevator m_elevator = new SmartElevator();

Keeping track of states

SmartElevator can have its own internal state for what position it was last in.

This not-quite-java illustrates

Inside of the Subsystem, we can have some variables to track the state

function getNextStateDown() {
  if (current_state == "low") { return "low" }
  if (current_state == "medium") { return "low" }
  if (current_state == "high") { return "medium" }
}

function getPositionForState(state) {
  if (state == "low") { return RobotMap.ElevatorLowPosition }
  ...
}

And then in the "goDownCommand", we'd say

execute() {
  elevator.setDesiredPosition(getPositionForState(getNextStateDown())
}

Mechanical Mockup

We can also build a mock-up of the SmartElevator mechanical system using our encoder test rig. We can hook that test rig motor up to our Talon that will drive the SmartElevator (or any Talon, if we remap things in RobotMap.java), and then demonstrate that the system works as expected.

Can we get the mechanical mockup to toggle between "low", "medium" and "high" positions?

Unknown Constants

We won't know yet what position values will correspond to "low", "medium" and "high" for the elevator, so we can make constants for now in Constants.java and reference them in the code. Then when we have the real robot built, we can move the elevator to the right positions, note down the positions, and change our constants, and the code will be all ready.

[gregmarra]

(If we have encoders added to the grabber arms first, we may want to do a SmartGrabber first)

[gregmarra]

Some pointers!

The start you have for SmartElevator looks nice! Here are a few questions and pointers.

I'm looking at https://github.com/GwhsRobotics3/Team-5507-2018/blob/master/src/org/usfirst/frc/team5507/robot/subsystems/SmartElevator.java

I've split these thoughts apart into separate Issues to make followup comments easier, and to be able to track their completion by closing the issues :)

1. [high-pri] #10 – We shouldn't make two Talon objects for the same physical Talon
2. [mid-pri] #11 - Some thoughts about state inside the SmartElevator
3. [low-pri] #12 - Moving Constants

[gregmarra]

Changing States and Naming Conventions

You've got the entire scaffolding for keeping track of the SmartElevator with currentState! It's looking really nice. You set it to the low position to start, and then have methods to step up a state and step down a state. We can rename those functions to be slightly clearer:

• getXYZ - "get" as a prefix usually means you are "reading something, and not changing anything in the process. For example, in getPositionForState, you are reading a number, perfect!
• setXYZ - "set" as a prefix usually means you are "changing something". For example, your current function getNextStateUp actually changes SmartElevator.currentState to a different value! You should rename this method to setNextStateUp to be clear it changes something. "Changing Something" is sometimes called "having a side effect".

Setting Position

The SmartElevator should use the WPI_TalonSRX "magic motion" similarly to the way we made the Subsystem for our Encoder Demo in EncoderDemo.driveToAngle. Think about the similarities between the mechanical system that the EncoderDemo and the SmartElevator are driving. How are they similar? How are they different?

Command Structure

Our DumbElevator has to be told "Go Up as long as someone is holding a button" or "Go Down as long as someone is holding a button". Our SmartElevator just needs to be told "go up one state" or "go down one state".

Our command should therefore:

1. Call SmartElevator.setNextStateUp (see note about Naming Conventions above) exactly once and then end. If it calls it more than once (the way Elevator.goUp() gets constantly called), then we'll keep incrementing state and it will be impossible to go to the middle position.
2. Look at the scaffold of a Command, and read the comments. How can we make sure a Command does something exactly once?

Challenge: Can you think about how to make a single command called SmartElevatorChangePositionBy(int direction) that takes 1 or -1 as a direction, and moves the elevator up or down, accordingly? This will avoid having nearly-identical SmartElevatorUp and SmartElevatorDown commands.

[GwhsRobotics3]

We tried coding the elevator going up and down based on the other methods but it is not working. Is there something wrong with the logic of the code?

[gregmarra]

Lets Skip Counters

It looks like you're basing the counter code on https://wpilib.screenstepslive.com/s/3120/m/7912/l/599687-using-limit-switches-to-control-behavior .

I think that this is a bit more complicated than it needs to be for now. Let's be simpler, and drop the Counters, and just based things on "Command-based program to operate until limit switch closed"

1. Remove the Counters from SmartElevator.
2. Change isSwitchSetTop and isSwitchSetBottom to return the DigitalInput's value directly via it's get() method: http://first.wpi.edu/FRC/roborio/release/docs/java/edu/wpi/first/wpilibj/DigitalInput.html#get--

Don't use while loops

We will almost never want to use while loops in our robot code. We already are calling methods over and over inside of a larger loop (the execute() method of Commands gets called continuously while the command is running, so we don't need to use a while loop to make sure something keeps happening).

Right now you have a while here: https://github.com/GwhsRobotics3/Team-5507-2018/blob/master/src/org/usfirst/frc/team5507/robot/subsystems/SmartElevator.java#L97 . You can change that while to an if, and it will run over and over thanks to the Command calling execute() repeatedly.

What will happen when getCurrentPos() > pos?

After you change https://github.com/GwhsRobotics3/Team-5507-2018/blob/master/src/org/usfirst/frc/team5507/robot/subsystems/SmartElevator.java#L97 to be an if, what will happen once the SmartElevator has lifted above the current set position, and getCurrentPos() exceeds pos?

[gregmarra]

Using the Talon's closed loop control.

The biggest thing we're going to want to change in SmartElevator is to use the Talon's built in Position Control.

Think about EncoderDemo. What's different between what EncoderDemo does, and what we want our elevator system to do?