Lift PID Controller Command Questions

It looks like the execute block sets the brake state and causes drive motion of the lift in succession. Is there any concern that the drive will be commanded to move while the break is still engaged? (It takes some time for the brake to respond to the command and physically move). Have you observed this additional load during the beginning of moves when you were tuning the controller? Would it make sense to wrap the lift.drive line of code with an if statement that checks the state of the break actuator? That way if we need t add a delay or sensor we can do it at the subsystem level.
Comment your methods!@!#!
Should the interrupted method disable the controller or clear the setpoint? It looks like if another instance of this controller stole control of the subsystem, that everything would be ok. But this may not be the only command interacting with the lift/controller. So better to be safe IMO.
Can the execute logic be simplified to something like:

If (controllerIsFinished) {
    lift.enableBrake()
    lift.drive(0)
} else {
    lift.disableBrake()
    if(lift.isBrakeDisabled)
        lift.drive(controller.getControlOutput())
}

As is, the controller output will still be sent to the drive command repeatedly when it's "finished". So you could be driving the motors w/ the brake enabled?

At this point, there is no concern of the controller not working as written, however we still need to do more testing with the controller under full load to understand the dynamics/timing of everything. We have fully tested the logic in the LiftDriveWithJoystick command, and at least right now, under no load it works well. The reason the PID command is written this way is so that, at no point it ever tries to command the lift without setting the brake... My goal is after testing, to move the logic from the DriveWithJoystick command to the Lift Subsystem, such that no drive command can ever be issued without setting the proper brake state. The only reason I have not done that yet, is because I am not sure if there is every a case where I don't want to do that. So I opted to write the commands this way, and move the brake logic into the Lift subsystem at a later time. At least right now, I do not see any problem with having the Drive command in the lift subsystem control the brake, such that anything driving the lift (PID or Joystick or other) has no knowledge of the brake, and ultimately doesn't care that it exist or not. So short answer, the code right now is not the final implementation I want to go to for the final version.
meh. Ok
Right now the current implementation, if the command ever gets interrupted, it calls the end() method which pauses the controller. This implementation is correct, and does not need to be modified.
Yes we can combine the two if statements in the current command to a single if-else statement. We wrote it this way in the moment, just so we could walk through the logic. The answer to 1 will ultimately determine how this changes. If the brake control moves into the subsystem, then this will just reduce to the drive(controller.getControlOutput()); and not have to worry about the brake. We will make that determination, after the lift has been installed. For position control, isFinished, won't return true unless the error is within our error tolerance, at which point, the output to the motor controllers is zero and will not drive, so the way it is currently written, it won't try to drive the motors with the brake enabled, however, moving the logic into the subsystem will ensure nothing can ever command the lift motor with the brake being engaged, and that is the implementation I want to go too.

Team2168 / 2015_Main_Robot

Lift PID Controller Command Questions #54