-
Notifications
You must be signed in to change notification settings - Fork 0
Initial Biscuit Subsystem #1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
mwitcpalek
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review comments
| return talonFXConfiguration; | ||
| } | ||
|
|
||
| // These are all wrong right now because we don't have any actual info |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move these above the talonFXconfig getter for consistency across constants files
| // public static Angle Level1 = ; | ||
|
|
||
| // Disables the TalonFX by setting it's voltage to zero. Not very shocking. | ||
| public static TalonFXConfiguration disableTalon() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we needed to disable the fwd limit switch as part of disabling output if it wasn't in a safe region
This could just return a VoltageConfig instead of a full talon config - if you don't change you should get your talonFX config from the one above and just modify the voltage config, not start from a blank one
|
|
||
| public class BiscuitConstants { | ||
|
|
||
| public static TalonFXConfiguration talonConfiguration() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a duplicate of line 53? If so get rid of this
| } | ||
|
|
||
| public void setPosition(Angle position) { | ||
| setPoint = position.plus(BiscuitConstants.kZero); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that we have an encoder 1:1 we can actually write to the encoder in the talonFX so we don't actually have to save off zeroed position and use that as an offset
| public void updateInputs(BiscuitIOInputs inputs) { | ||
| BaseStatusSignal.refreshAll(velocity, position, fwdLimitSwitch); | ||
| inputs.velocity = velocity.getValue(); | ||
| inputs.position = position.getValue().minus(BiscuitConstants.kZero); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be able to convert this to just getValue()
| public void zero() { | ||
| didZero = false; | ||
| if (fwdLimitSwitchOpen == true) { | ||
| Angle pos = position.getValue(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can actually write to the encoder position now so need to add subtraction logic based on zero constant and current pos from a remote encoder
Right now you have no way to get that remote encoder (we need new SW api) so just use get position for now
|
|
||
| @Override | ||
| public void periodic() { | ||
| io.updateInputs(inputs); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to add processInputs()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add logging of the current setpoint to the periodic - see example subsystem for a reference
Put them under "Biscuit/"
| return Set.of(new Measure("Is Biscuit Finished", () -> isFinished() ? 1.0 : 0.0)); | ||
| } | ||
|
|
||
| public void zero() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May want to use your "didZero" in the i/o layer for passing info up to robotState
No description provided.