Conversation
SwerveSubsystem.java and YAGSL with custom compatible swerveSwerveSubsystem.java and YAGSL with custom AK-compatible swerve
harrislegobrick
left a comment
There was a problem hiding this comment.
First pass, but I'm not too sure how intentional deviation from the akit example is.
For example, the akit example has custom logic to run the odometry at a higher frequency than the robot code, but that is not implemented here. That functionality isn't strictly required, but since it is already implemented in the example code, it would be nice to have. Not sure if you planned on doing that in follow up PR or this one.
Overall looks fine, but some useful comments would be nice. The comments in the akit example are really good imo.
| private static SwerveModuleState optimize(SwerveModuleState desiredState, Rotation2d currentAngle) { | ||
| Rotation2d delta = desiredState.angle.minus(currentAngle); | ||
|
|
||
| if (Math.abs(delta.getDegrees()) > 90.0) { |
There was a problem hiding this comment.
the SwerveModuleState class has a method to do the optimization logic for you with the .optimize(Rotation2d) method.
| return Math.copySign(deadbanded * deadbanded, deadbanded); | ||
| } | ||
|
|
||
| private static boolean shouldFlipForAlliance() { |
There was a problem hiding this comment.
This is broken out into its own file in the akit example. I am fine with keeping it here if you'd want to keep it here, but I'd rather it be in a separate file in the module directory.
|
|
||
| private static double modifyAxis(double value) { | ||
| double deadbanded = Math.abs(value) > OperatorConstants.DEADBAND ? value : 0.0; | ||
| return Math.copySign(deadbanded * deadbanded, deadbanded); |
There was a problem hiding this comment.
WPILib also has a function for this too with MathUtil.copydirectionPow(double, double, double).
| } | ||
|
|
||
| private static double modifyAxis(double value) { | ||
| double deadbanded = Math.abs(value) > OperatorConstants.DEADBAND ? value : 0.0; |
There was a problem hiding this comment.
WPILib has a function for doing this with MathUtil.applyDeadband(double, double).
| double omega; | ||
| if (aim.getAsBoolean()) { | ||
| Translation2d toTarget = aimTarget.get().getTranslation().minus(getPose().getTranslation()); | ||
| Rotation2d targetHeading = toTarget.getAngle().plus(Rotation2d.fromDegrees(180.0)); |
There was a problem hiding this comment.
Rotation2d.fromDegrees(180.0) can be replaced with Rotation2d.k180deg. The description for the pre-made rotation values say that using them can reduce memory usage by sharing the commonly used rotation values.
| SparkUtil.tryUntilOk( | ||
| driveMotor, | ||
| 5, | ||
| () -> driveMotor.configure(driveConfig, ResetMode.kNoResetSafeParameters, |
There was a problem hiding this comment.
I think this would overwrite the configs set earlier.
|
|
||
| @Override | ||
| public void setDesiredState(SwerveModuleState state) { | ||
| if (Math.abs(state.speedMetersPerSecond) < 0.01) { |
There was a problem hiding this comment.
I wouldn't have slow velocity filtering in the module but rather in the input into the module. Also the state should be optimized with both the optimizations in the akit example.
| encoderConfig.MagnetSensor.AbsoluteSensorDiscontinuityPoint = 0.5; | ||
| absoluteEncoder.getConfigurator().apply(encoderConfig); | ||
|
|
||
| SparkMaxConfig driveConfig = new SparkMaxConfig(); |
There was a problem hiding this comment.
Should also set the signals for CAN bus optimization.
| driveMotor = new SparkMax(driveId, MotorType.kBrushless); | ||
| angleMotor = new SparkMax(angleId, MotorType.kBrushless); | ||
|
|
||
| absoluteEncoder = new CANcoder(encoderId); |
There was a problem hiding this comment.
Not calling the optimize CAN method.
| import frc.robot.constants.Constants; | ||
| import frc.robot.constants.SwerveConstants; | ||
|
|
||
| public class ModuleIOSim implements ModuleIO { |
There was a problem hiding this comment.
This doesn't have the DCMotorSim for actual sim support, so this class wouldn't really do anything right now.
harrislegobrick
left a comment
There was a problem hiding this comment.
Looks pretty good, just needs a few changes.
| linearMagnitude = linearMagnitude * linearMagnitude; | ||
|
|
||
| // Return new linear velocity | ||
| return new Pose2d(Translation2d.kZero, linearDirection) |
There was a problem hiding this comment.
This can be replaced by return new Translation2d(linearMagnitude, linearDirection);
| this, | ||
| translationX, | ||
| translationY, | ||
| () -> targetPoseSupplier.get().getTranslation().minus(getPose().getTranslation()).getAngle()); |
There was a problem hiding this comment.
Would be nice to also supply an offset to be added to the calculated angle considering the turret's angle isn't zero. I'd also probably change from accepting a Supplier<Pose2d> to just a Supplier<Translation2d>.
| return Commands.either(targetLockDrive, manualDrive, aimAtTarget).withName("DriveCommand"); | ||
| } | ||
|
|
||
| /** Convenience overload for WPILib triggers. */ |
There was a problem hiding this comment.
The Trigger class implements BooleanSupplier so this isn't actually needed.
| private final int index; | ||
|
|
||
| private final Alert driveDisconnectedAlert; | ||
| private final Alert turnDisconnectedAlert; |
There was a problem hiding this comment.
Should probably have one for the CANCoder too.
|
|
||
| public interface ModuleIO { | ||
| @AutoLog | ||
| public static class ModuleIOInputs { |
There was a problem hiding this comment.
In the example project, the absolute encoder is assumed to be plugged into the SparkMAX directly so there is no accessing it differently, but for our setup, we have a CANCoder for the absolute angle. I think the information about the absolute encoder (mainly connection and position) should be logged separately from the turn motor's information.
|
|
||
| // Create odometry queues | ||
| timestampQueue = SparkOdometryThread.getInstance().makeTimestampQueue(); | ||
| drivePositionQueue = SparkOdometryThread.getInstance().registerSignal(driveSpark, driveEncoder::getPosition); |
There was a problem hiding this comment.
This should use a copied status signal like the gyro.
| PersistMode.kNoPersistParameters)); | ||
| } | ||
|
|
||
| private void syncTurnEncoderWithAbsolute() { |
There was a problem hiding this comment.
YAGSL syncs the absolute encoder with the motor's internal encoder every 100ms as long as the wheel isn't turning/moving. I'm not sure how necessary that is, but it is a similar idea to the turret where it uses the absolute information to correct the internal encoder after some time. I don't think the swerve turn will mechanically slip so I don't think it is necessary, but is something to be aware of and is why I want both logged.
| translationX, | ||
| translationY, | ||
| () -> targetTranslationSupplier.get().minus(getPose().getTranslation()).getAngle().plus(angleOffset)); | ||
| return Commands.either(targetLockDrive, manualDrive, aimAtTarget).withName("DriveCommand"); |
There was a problem hiding this comment.
This won't work like the current system does because of how the Command.either() works. It is a ConditionalCommand where it chooses between the two commands only once when the command is initialized, and since this is the default command for the drivetrain, this will only call the initialization once at the start of teleop.
| } | ||
|
|
||
| @Override | ||
| public void updateInputs(ModuleIOInputs inputs) { |
There was a problem hiding this comment.
Should also set the two new IO variables added for the absolute CANcoder. It can probably just use the value produced in the sim from the turn position along with always being connected.
harrislegobrick
left a comment
There was a problem hiding this comment.
Probably fine for an initial implementation.
| new TrapezoidProfile.Constraints(ANGLE_MAX_VELOCITY, ANGLE_MAX_ACCELERATION)); | ||
|
|
||
| angleController.enableContinuousInput(-Math.PI, Math.PI); | ||
| boolean[] wasUsingAngle = new boolean[] { false }; |
There was a problem hiding this comment.
Why is this an array?
This PR removes the previous swerve code and the YAGSL library with custom swerve code. This is a pretty big change and I'd like additional human input, so I'm making this a PR.