Conversation
For some reason all the files were not in lib199 but only in carlmontrobotics
Removed state enum cause did not able to work nicely how I thought
Created a detailed plug and play common mechs that allow programming not to waste time reinventing the wheel each year
Hopefully it works
timtogan
left a comment
timtogan
left a comment
There was a problem hiding this comment.
This looks pretty good! I mostly skimed over the code, so I'll prob do another review when I have time. Also fyi, some comments I left only for one mech but it can be applied to (all) other mechs as well
| * @param armKPID double array with 3 values, kP, kI, kD | ||
| * @param bottomLimit lowest angle in degrees the arm can safely achieve with 0 being horizontal | ||
| * @param topLimit highest angle in degrees the arm can safely achieve with 0 being horizontal | ||
| * @throws IllegalArgumentException if MotorType is not one of the 3 expected {@link MotorConfig}s |
There was a problem hiding this comment.
5 as we added solo vortexes and neo 2.0
| armKP = 0; | ||
| armKI = 0; | ||
| armKD = 0; | ||
| DriverStation.reportWarning("ArmPID is off", true); |
There was a problem hiding this comment.
I would print why ArmPID is off, ie ArmPID is off because KP is less than 0
| armKG = 0; | ||
| armKV = 0; | ||
| armKA = 0; | ||
| DriverStation.reportWarning("ArmFeedForward is off", true); |
| * @throws IllegalArgumentException if the bottom limit is higher or equal to the top limit | ||
| */ | ||
| public static ArmConfig motorWithPID(int armMasterMotorId, MotorConfig armMasterMotorType, boolean armMasterInversed, double[] armKPID, double bottomLimit, double topLimit) { | ||
| return new ArmConfig(1, 1, 14, null, null, null, armMasterMotorId, armMasterMotorType, armMasterInversed, armKPID, defaultArmPIDTolerance, false, null, null, -1, 0, bottomLimit, topLimit); |
There was a problem hiding this comment.
| return new ArmConfig(1, 1, 14, null, null, null, armMasterMotorId, armMasterMotorType, armMasterInversed, armKPID, defaultArmPIDTolerance, false, null, null, -1, 0, bottomLimit, topLimit); | |
| return new ArmConfig(1, 1, 12, null, null, null, armMasterMotorId, armMasterMotorType, armMasterInversed, armKPID, defaultArmPIDTolerance, false, null, null, -1, 0, bottomLimit, topLimit); |
| } | ||
|
|
||
| /** | ||
| * Checks PID to have 3 values, updates {@link #armPIDExists} and makes sure the values are legitimate |
There was a problem hiding this comment.
what is armPIDExists, I can't find it refrenced anywhere but in here
| /** | ||
| * @deprecated Use {@link #resetFieldOrientation()} instead | ||
| */ | ||
| @Deprecated | ||
| public void resetHeading() { | ||
| gyro.reset(); | ||
|
|
||
| } |
There was a problem hiding this comment.
| /** | |
| * @deprecated Use {@link #resetFieldOrientation()} instead | |
| */ | |
| @Deprecated | |
| public void resetHeading() { | |
| gyro.reset(); | |
| } |
| /** | ||
| * @deprecated Use {@link #setMode(Mode)} instead | ||
| * Changes between IdleModes | ||
| */ | ||
| @Deprecated | ||
| public void toggleMode() { | ||
| for (SwerveModule module : modules) | ||
| module.toggleMode(); | ||
| } | ||
| /** | ||
| * @deprecated Use {@link #setMode(Mode)} instead | ||
| */ | ||
| @Deprecated | ||
| public void brake() { | ||
| for (SwerveModule module : modules) | ||
| module.brake(); | ||
| } | ||
| /** | ||
| * @deprecated Use {@link #setMode(Mode)} instead | ||
| */ | ||
| @Deprecated | ||
| public void coast() { | ||
| for (SwerveModule module : modules) | ||
| module.coast(); | ||
| } |
There was a problem hiding this comment.
| /** | |
| * @deprecated Use {@link #setMode(Mode)} instead | |
| * Changes between IdleModes | |
| */ | |
| @Deprecated | |
| public void toggleMode() { | |
| for (SwerveModule module : modules) | |
| module.toggleMode(); | |
| } | |
| /** | |
| * @deprecated Use {@link #setMode(Mode)} instead | |
| */ | |
| @Deprecated | |
| public void brake() { | |
| for (SwerveModule module : modules) | |
| module.brake(); | |
| } | |
| /** | |
| * @deprecated Use {@link #setMode(Mode)} instead | |
| */ | |
| @Deprecated | |
| public void coast() { | |
| for (SwerveModule module : modules) | |
| module.coast(); | |
| } |
| if (armFollowId != null) { | ||
| for (MotorConfig config: armFollowMotorType) { | ||
| if (config != MotorConfig.NEO && config != MotorConfig.NEO_VORTEX && config != MotorConfig.NEO_550) { | ||
| throw new IllegalArgumentException("What is this config??? If null pls change, if not null you're cooked"); |
There was a problem hiding this comment.
maybe it's a neo 2.0 or a solo vortex :P
There was a problem hiding this comment.
also I feel like this is not really needed, as I don't nesseceraly see how this is better than a null check and it adds burden, as you need to modify this whenever a motor is added to motorConfig
CoolSpy3
left a comment
CoolSpy3
left a comment
There was a problem hiding this comment.
Hey guys! Sorry for the delay on this. I haven't gotten a chance to read through all the code yet, and my schedule's just getting busier, so I wanted to send over my in-progress comments so as to not keep you waiting for to long. Like Tim's comments, some are likely applicable to multiple subsystems. IIRC, I got part way through SimpleArm. I'll try to remember to circle back here at some point. Otherwise, feel free to ping me.
There was a problem hiding this comment.
Please use the version of Elastic from vendorLibs (see #113)
| if (armFollowId != null) { | ||
| if (armFollowId.length == armFollowInversed.length | ||
| && armFollowId.length == armFollowMotorType.length) { | ||
| return; | ||
| } | ||
| throw new IllegalArgumentException("Motors must have matching IDs, MotorTypes and Inverse Configs"); | ||
| } | ||
| if (armMasterMotorId != -1) { | ||
| if (armMasterMotorType != null) { | ||
| return; | ||
| } | ||
| throw new IllegalArgumentException("Master motor needs a MotorConfig"); | ||
| } | ||
| throw new IllegalArgumentException("Master motor needs to exist"); |
There was a problem hiding this comment.
This logic doesn't work. If armFollowId != null and the corresponding values are set appropriately, armMasterMotorId is never checked (because line 211 is hit). In fact, I'd rewrite this to avoid the use of returns as they aren't suited for this type of checking.
| } | ||
| throw new IllegalArgumentException("Motors must have matching IDs, MotorTypes and Inverse Configs"); | ||
| } | ||
| if (armMasterMotorId != -1) { |
There was a problem hiding this comment.
Why are we checking if this is -1. The only time that happens is when someone explicitly calls new ArmConfig (or an equivalent helper) and chooses to pass -1 as the motor id (as lib199 never injects a -1 value as a stand-in for null). This will basically never happen in-production. The much more common error would be to pass an invalid id, which there isn't a good test for.
Similar thing with the motor types
I'm not saying it's wrong to perform this test, but I don't think it's that useful, and it makes it less obvious which checks are important.
| * Check that arguments are good and set some of the parameters of the config. | ||
| * @param armMotorOfBackupRelativeEncoderId give motor Id | ||
| */ | ||
| private void checkRequirements(int armMotorOfBackupRelativeEncoderId) { |
There was a problem hiding this comment.
This doesn't need to take a param. armMotorOfBackupRelativeEncoderId is stored as an instance variable.
| * @throws IllegalArgumentException if the bottom limit is higher or equal to the top limit | ||
| */ | ||
| public static ArmConfig motorWithPID(int armMasterMotorId, MotorConfig armMasterMotorType, boolean armMasterInversed, double[] armKPID, double bottomLimit, double topLimit) { | ||
| return new ArmConfig(1, 1, 14, null, null, null, armMasterMotorId, armMasterMotorType, armMasterInversed, armKPID, defaultArmPIDTolerance, false, null, null, -1, 0, bottomLimit, topLimit); |
There was a problem hiding this comment.
The doc for this method (and the others) should explain these default values.
| @@ -0,0 +1,599 @@ | |||
| // Copyright (c) FIRST and other WPILib contributors. | |||
There was a problem hiding this comment.
Why is there a WPILib copyright header on this file? Did we copy it from WPILib? If so, I recommend linking back to the original file and the copyright license or just using the WPILib version.
| * {@link #AUTO} For automatically controling the arm to go to a certain goal | ||
| * {@link #MANUAL} For manually controlling the arm by providing percentage of voltage | ||
| * {@link #BRAKE} For holding the arm in a rigidish state, not moving | ||
| * {@link #COAST} For setting the arm to not resist movement, allowing accessibility when the robot is disabled. |
There was a problem hiding this comment.
These annotations should be moved to the enum values rather than the class description.
| * @return current elevator height in meters | ||
| */ | ||
| public double getElevatorHeight() { | ||
| return feedbackEncoder.getPosition(); |
There was a problem hiding this comment.
Really, this will depend on the set conversion factor (same for the other comment). So we can't say with certainty what the units are.
Edit: Actually, if you only account for the gear ratio, it will be rotations. I have to read some more of the code to figure out what's actually happening, but I'm too tired to do that now, so I'm writing this as a placeholder. If you see this text, it means I forgot to fix it.
| /** Creates a new SimpleArm. */ | ||
| private final ArmConfig armConfig; |
| protected boolean isSysIdTesting() { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Not sure if this makes sense as a method. It's only evaluated in the constructor, so overriding its behavior isn't super helpful if you want it to have any complex behavior, which could make it confusing for users. I recommend just asking people to call sysIdSetup() themselves instead.
3 participants
Please merge this tim, it works