Advantagekit Rewrite #4
Conversation
…Up into advantagekit
…Up into advantagekit
Lewis-Seiden
requested review from
kevinclark,
victoryforphil and
Awesomeplayer165
|
Let's try to go over it today. Looks fine though |
|
|
||
| import org.littletonrobotics.junction.AutoLog; | ||
|
|
||
| /** Add your docs here. */ |
| leader.config_kF(0, 0); | ||
| follower = new HighlanderFalcon(Constants.ElevatorConstants.elevatorFollowerID); | ||
| follower.set(ControlMode.Follower, Constants.ElevatorConstants.elevatorMotorID); | ||
| follower.configSupplyCurrentLimit(new SupplyCurrentLimitConfiguration(true, 30.0, 30.0, 0.5)); |
There was a problem hiding this comment.
Should current/voltage limits be constants?
| public double[] currentAmps = new double[] {}; | ||
| } | ||
|
|
||
| public abstract void updateInputs(ElevatorIOInputs inputs); |
There was a problem hiding this comment.
There's a few good reasons to avoid mutation methods and return a new object instead. First is that if you instead make this getInputs or something similar which returns a value, you can chain on that value vs needing to have a separate line for just updating the output variable. Second is that mutation can be a major pitfall when dealing with concurrent systems. Third is that if you want to unit test, mocking or stubbing out this result is much more complicated. Two of those three don't really apply in our codebase, but it's a useful habit to get into.
Honestly, the only good reason for mutation functions like this is if the object you're dealing with is particularly expensive to create. This doesn't look like that.
Strongly encourage you to replace this (and the analogous method in the other interfaces) to return a value instead.
| elevatorFollower.configVoltageCompSaturation(10); | ||
| LoggingWrapper.shared.add("elevatorsim", mech2d); | ||
| zeroMotor(); | ||
| io = Robot.isReal() ? new ElevatorIOFalcon() : new ElevatorIOSim(); |
There was a problem hiding this comment.
nitpick: recommend using this. for member variables. Means I don't go looking for where it's defined.
There was a problem hiding this comment.
Also, it's going to be worth considering as you add more Sim versions (assuming you do that) whether that check (Robot.isReal) should be hoisted up a level and you have a Sim subsystem vs each subsystem having to know which parts are sim'd out.
|
we've basically been using this as new main anyways so merge time |
Opening pr, probably need to go over it one more time before merging