Conversation
…ostly works besides turning
kevinfrei
left a comment
kevinfrei
left a comment
There was a problem hiding this comment.
Added a bunch of comments on just the changes from the initial (not-working) 'Swedro21' stuff (so only the code you and your not quite competent AI friend wrote 😆)
@explosivegamer25 Please make updates and push the fixes. (I should explain how Git works to you some time...)
| subsystem.updateValues(xvalue, yvalue, rvalue); | ||
| } | ||
|
|
||
| double xvalue = x.getAsDouble(); |
There was a problem hiding this comment.
Be consistent about the negation: Either do it all at the '.getAsDouble()' line, or do it all in the "updateValues" line. But have one in both places is a weird code smell.
| if (gamepad1.crossWasPressed() && flsteer != null){ | ||
| flsteer.setPower(1); | ||
| } else if (gamepad1.crossWasReleased() && flsteer != null){ | ||
| } else |
There was a problem hiding this comment.
I think removing the .crossWasRelease() thing is fine, but you also removevd the "!= null" which will cause a NullPointerException if you don't have SWERVEDRIVE enabled... (Ditto for all the changes in this file...)
| @@ -0,0 +1,148 @@ | |||
| package org.firstinspires.ftc.swervebot.opmodes; | |||
There was a problem hiding this comment.
This whole file is awful. We don't use state-based code. We use Command-based code. Please stop doing this. Schedule the SwerveDriveCmd as a joystick command. If you needed this for validation, that's fine, but I really don't want this in the 'final' code, because it's duplicate work that you've done elsewhere, and it will become either unmaintainable or confusing to other programmers very quickly.
| public void uponStart() { | ||
| robot.prepForStart(); | ||
| } | ||
|
|
There was a problem hiding this comment.
Whitespace-only changes should be eliminated. The auto-formatter is supposed to take care of this. Not sure why this snuck through. Delete it.
| import static org.firstinspires.ftc.swervebot.opmodes.Tuning.stopRobot; | ||
| import static org.firstinspires.ftc.swervebot.opmodes.Tuning.telemetryM; | ||
|
|
||
|
|
There was a problem hiding this comment.
See previous comment about whitespace...
| } | ||
|
|
||
| // Calculate the desired state for each swerve module | ||
| // FIX (Bug 1 & 2): Read encoders FIRST in the module angle loop so that: |
There was a problem hiding this comment.
Can you characterize what the bug was that this fixes?
| public static double strafeInput = 0; | ||
| @Log.Number(name = "R: ") | ||
| public static double rotationInput = 0; | ||
| @Log.Number(name = "rNeg: ") |
There was a problem hiding this comment.
Way.Too.Many.@log's.
Delete them all. Also, names like "A, B, C, E, F, G" are garbage. Name them something real, and put the in a single summary line. (This is AI Slop. Code reviewing already stinks. This makes it worse.)
| // drive[i].setPower(wheelPowers[i]); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
This is just a waste. We already have an array called "wheelPowers". Why are you copying it to new local variables? Just so they get updated in the log. I'm done looking at bad AI Slop, now. Fix this file. I'm going to the next one, ignoring everything else in here.
| flswervoenc = new AbsoluteAnalogEncoder(hwmap.get(AnalogInput.class, Setup.HardwareNames.FL_SWERVO_ENCODER)); | ||
| rlswervoenc = new AbsoluteAnalogEncoder(hwmap.get(AnalogInput.class, Setup.HardwareNames.RL_SWERVO_ENCODER)); | ||
| rrswervoenc = new AbsoluteAnalogEncoder(hwmap.get(AnalogInput.class, Setup.HardwareNames.RR_SWERVO_ENCODER)); | ||
| frswervoenc = new AbsoluteAnalogEncoder(hwmap.get(AnalogInput.class, Setup.HardwareNames.FR_SWERVO_ENCODER)) |
There was a problem hiding this comment.
AbsoluteAnalogEncoder belongs in TechnoLib (under RobotLibrary) and, when added to TL, should only require the name. (This is leftover from KoolPoool's stuff, but it still should be done. I'm not sure what added value AbsoluteAnalogEncoder provides over ExternalEncoder, since we don't seem to be setting the range.
| rlswervoenc = new AbsoluteAnalogEncoder(hwmap.get(AnalogInput.class, Setup.HardwareNames.RL_SWERVO_ENCODER)); | ||
| rrswervoenc = new AbsoluteAnalogEncoder(hwmap.get(AnalogInput.class, Setup.HardwareNames.RR_SWERVO_ENCODER)); | ||
| frswervoenc = new AbsoluteAnalogEncoder(hwmap.get(AnalogInput.class, Setup.HardwareNames.FR_SWERVO_ENCODER)) | ||
| .zero(-2.925); |
There was a problem hiding this comment.
Hard coded constants are a big ol' no-no. Document these things and make them static's in Setup or somewhere sensible.
|
@explosivegamer25 if you have questions, start your own review. Click the "+" in the review to add comments. |
2 participants
This is to have a single place to see all the code that Colin coerced out of some poor unsuspecting AI agent...