refresh inputs logging #51
Conversation
kevinclark
left a comment
kevinclark
left a comment
There was a problem hiding this comment.
Nothing blocking, but I think the level of repetition (pulling out status signals, setting update timings) is enough that you should strongly consider making a proper class or record
| private final StatusSignal<Double> pivotVelocity = pivotMotor.getVelocity(); | ||
| private final StatusSignal<Double> pivotVoltage = pivotMotor.getMotorVoltage(); | ||
| private final StatusSignal<Double> pivotAmps = pivotMotor.getStatorCurrent(); | ||
| private final StatusSignal<Double> pivotStatorCurrentAmps = pivotMotor.getStatorCurrent(); | ||
| private final StatusSignal<Double> pivotSupplyCurrentAmps = pivotMotor.getSupplyCurrent(); | ||
| private final StatusSignal<Double> pivotTempC = pivotMotor.getDeviceTemp(); | ||
| private final StatusSignal<Double> pivotRotations = pivotMotor.getPosition(); |
There was a problem hiding this comment.
This is repeated enough you should really make it a class. A record with a custom constructor that takes the motor would really clean this up trivially.
There was a problem hiding this comment.
Nothing blocking too. Might be helpful to look into array usecases and see if they can be abstracted to an array if they aren't being used for special array use cases (iteration).
EDIT: talked to @Lewis-Seiden, gonna break into two variables then adopt @kevinclark suggestion.
| public double[] elevatorStatorCurrentAmps = new double[] {}; | ||
| public double[] elevatorSupplyCurrentAmps = new double[] {}; |
There was a problem hiding this comment.
Like Kevin's -- not blocking -- why are we using an array for this? In other languages, this is represented as a tuple, which is an indicator for abstracting to a class. (also see below relating comment).
There was a problem hiding this comment.
especially since this is not being used for iteration.
| public Command runCurrentZeroing() { | ||
| return this.run(() -> io.setVoltage(-1.0)) | ||
| .until(() -> inputs.elevatorCurrentAmps[0] > 40.0) | ||
| .until(() -> inputs.elevatorStatorCurrentAmps[0] > 40.0) |
There was a problem hiding this comment.
this syntax is very scary for me. Not sure if this is the syntax or norm for Java, but readers can be unsure of the value difference between index 0, 1, ... (even how many there are? Abstracting to a class like mentioned above should clear up your intentions). I always repeat the phrase: write once, read 10 times.
There was a problem hiding this comment.
also see this line being repeated in src/main/java/frc/robot/subsystems/shooter/ShooterSubystem.java :154, but with a different syntax of using the array. maybe clearing some of this stuff up might help clear confusion among fresh eyes.
There was a problem hiding this comment.
Yeah, I agree. Seems like a good use for a record. Additionally, the value appears to be initialized to a 0 length array but then we do array access here that would be out of bounds if the value isn't set to something else. If the size of the array changes it strongly implies that maybe array isn't the right container.
There was a problem hiding this comment.
the whole array use was a holdover from the akit examples, where they wanted you to use an array for all motor current and temperature since they consider motor count an implementation detail. i dont really agree w their view of it either, and will change it
| inputs.elevatorVelocityMetersPerSec = physicsSim.getVelocityMetersPerSecond(); | ||
| inputs.elevatorAppliedVolts = volts; | ||
| inputs.elevatorCurrentAmps = new double[] {physicsSim.getCurrentDrawAmps()}; | ||
| inputs.elevatorStatorCurrentAmps = new double[] {physicsSim.getCurrentDrawAmps()}; |
There was a problem hiding this comment.
sometimes using arrays can be confusing the total length. also since this length can be whole replaced by a new array (which looks like is what you are doing everytime -- constructing a new array instead of setting existing values).
…rRobotics/Crescendo into supply-current-logging
…rRobotics/Crescendo into supply-current-logging
adds supply current logging, standardizes on
getValueAsDouble()instead ofgetValue(), makes sure to only use arrays where needed, and makes elevator properly log both motors.