Modify Brake check command to check for both states

[NotInControl]

I would modify the following code to be 2 methods based on #38

The double solenoid can take on other values besides kForward. It can be kForward, kOff, or kReverse.

The way this method is written is such that a kOff will return brake disengaged... if you have only 1 method for this subsystem. While this code may work as is without problem, we should correct so that it is proper.

I recommend adding a check such that you also check value is kReverse before passing a Boolean that says break is disengaged,

[jcorcoran]

Are you commenting on the isBrakeEnabled() method in the Lift subsystem?

If so, I think it's written as you would want it to be. It doesn't return true unless you are positively in the engaged state. What would you have the method return when the solenoid is in the kOff state? There are only two options, so apparently you think True is the right choice... That doesn't make sense to me.

"I recommend adding a check such that you also check value is kReverse before passing a Boolean that says break is disengaged" The method has to return a value, what are you going to do differently in this extra if statement? Also, changing the logic to if {} else if {} isn't going to make the compiler happy.

"The way this method is written is such that a kOff will return brake disengaged. While this code may work as is without problem, we should correct so that it is proper. " This isn't a true statement, It returns false, which is saying that the brake is not engaged. That's not equivalent to the brake being disengaged (not engaged != disengaged for a device which can be in between states).

End of the day we're not going to have sensors on this component unless we are real lucky and the Bimba cylinders are magnetic. If there's no sensors, you need to code defensively and command the device to the position you want it to be in explicitly within each command group that has a dependency on brake state. You can't trust the returned value from the subsystem because it's literally whatever you last commanded and not necessarily reflecting the position the actuator is in.

[NotInControl]

My true recommendation is to make 2 methods for each state as I stated in #38, and your rationale is why we have done it that way in the past. I wanted the programmers who worked on this method to figure that out once they got into it without me telling them to change their implementation completely.

If we don't have sensors on the actuators, then the last command position is all we have. I can't argue with that as being less than ideal, but we can at least use it as information and assume if the last command position is x, then the current state of the piston is X, and that is what we are doing. It is better than nothing.

IT is going to be important to know when the brake is released as well as when it is engaged. Even if our sensed state is an assumption (known as an estimator) based on the last commanded signal. That is good enough for me, given the reliability we have seen with the pneumatics systems. This assumption is incorrect only if we have a catastrophic failure of the mechanical device, or the plumbing, and I can live with that as being the only real times this assumption would be wrong.

Yes if you want to be 100% technical you cant have 100% confidence, but that is exactly what an estimated state is, I am okay with not having a true sensor on everything. You can assume a time delta travel if you want to be fancy, and wait that duration of time before you call the state true, but I don't think we need that for such a small stroke small bore piston.

These estimators have worked for us in the past couple of years very reliably, what is your hesitation about using them in this way this year?

[NotInControl]

I added clarification to the issue so that it is explicitly clear what the programmer should do.

[jcorcoran]

I don't have hesitation in using methods for determining if the brake is in a position.

I just don't think this issue is valid, for the reasons stated. If #38 asks there to be two methods, that makes sense.

Adding more checks into the existing method, as originally suggested, doesn't make sense.