Stage1b CTRE Solution - #94
Conversation
|
🌐 Preview URL: https://pr-94.frcsoftware.pages.dev |
|
This is marked as a draft for now as we're still waiting on alpha 7 to change |
Does this have to be a blocker? I'd rather merge this in early and go back to make code changes once alpha 7 is out |
DylanB5402
left a comment
There was a problem hiding this comment.
I like this a lot, thank you for picking it up! Left a couple small comments, apologies if I'm jumping the gun on reviewing here
| @Override | ||
| public void start() { | ||
| Scheduler.getDefault() | ||
| .schedule(robot.drivetrain.arcadeDrive(() -> 0.5, () -> 0.0).withTimeout(Seconds.of(4))); |
There was a problem hiding this comment.
Can we break this out into its own command in the Drive subsystem, like the turn in place one? I think it'd be good to use this as an example of how to compose commands for more complex behaviors (Ex: drive, turn, drive again)
| robot.drivetrain.setDefaultCommand( | ||
| robot.drivetrain.arcadeDrive(() -> -xbox.getLeftY(), () -> xbox.getRightX())); | ||
|
|
||
| xbox.leftBumper().whileTrue(robot.intakeLauncher.intake()).whileTrue(robot.feeder.intake()); |
There was a problem hiding this comment.
Might just be a commands v3, but I find having two whileTrue() calls in a row like that is a bit weird to read? commands v2 had the alongWith() helper, does v3 have anything similar we could use?
There was a problem hiding this comment.
The commands v3 docs mention that you can chain whileTrue calls together, as well as running 2 commands in parallel inside of a parent command with awaitAll:
Command.noRequirements(coro -> {
System.out.println("Hi!");
coro.awaitAll(command1, command2);
})In V3, you can also use alongWith like this:
robot.launcher.intake().alongWith(robot.feeder.intake()).withAutomaticName()But IMO the withAutomaticName is a lot less intuitive, and I didn't mention alongWith since it does a very similar thing to awaitAll.
There was a problem hiding this comment.
I kinda like awaitAll since it mirrors more typical async language feature (ex: Javascript's Promise.all). @zachwaffle4 as our resident commands v3 expert do you have opinions?
There was a problem hiding this comment.
I would prefer to use awaitAll where appropriate, and I think it's more easily readable than chaining whileTrues.
|
waiting on #108 |
# Conflicts: # copy.bara.sky
| private TalonFX leftLeader = new TalonFX(leftLeaderID, CANBus.systemcore(0)), | ||
| leftFollower = new TalonFX(1, CANBus.systemcore(0)), | ||
| rightLeader = new TalonFX(rightLeaderID, CANBus.systemcore(0)), | ||
| rightFollower = new TalonFX(3, CANBus.systemcore(0)); |
There was a problem hiding this comment.
explicitly declare the types
There was a problem hiding this comment.
Personally, I find this a lot cleaner (since theres less boilerplate) but I'm ok with changing it if everyone else wants to.
There was a problem hiding this comment.
I think it's much more prone to errors, and readability is the same. Very little upside to doing it this way.
| import org.wpilib.hardware.imu.OnboardIMU.MountOrientation; | ||
|
|
||
| public class Drivetrain extends Mechanism { | ||
| private static final int leftLeaderID = 0, rightLeaderID = 2; |
There was a problem hiding this comment.
Because they're constants. I could make them UPPER_SNAKE_CASE but the stage 1A solution doesn't do that, so I didn't either
There was a problem hiding this comment.
You could argue that all of the fields could/should be static here. I personally would remove it just so that when someone is looking at the solutions they don't feel like they made a mistake by not having a static variable
| public Command rotateInPlace(double angleDegrees, DoubleSupplier rotationThrottle) { | ||
| return run(coroutine -> { | ||
| double targetAngle = imu.getRotation2d().getDegrees() + angleDegrees; | ||
| while (imu.getRotation2d().getDegrees() < targetAngle) { |
There was a problem hiding this comment.
This will fail if targetAngle is outside of the bounds returned by getDegrees (-180-180).
| .named("Drive"); | ||
| } | ||
|
|
||
| public Command rotateInPlace(double angleDegrees, DoubleSupplier rotationThrottle) { |
There was a problem hiding this comment.
I feel like this is a pretty bad example and not something anyone should write in real life
There was a problem hiding this comment.
In real life, you're probably using a path following library so that isn't really relevant.
Furthermore, PID is going to be taught in stage 1C and not 1B.
There was a problem hiding this comment.
Is there an example that could be used that would be useful on a real robot but simple enough to not need the later courses? If not should it just be removed without a replacement?
| import org.wpilib.networktables.NetworkTableInstance; | ||
| import org.wpilib.networktables.StructArrayPublisher; | ||
|
|
||
| public class FuelSim { |
There was a problem hiding this comment.
Why is this class all static?
There was a problem hiding this comment.
To make life easier for the student doing the exercise. Creating a new FuelSim() and passing it into every subsystem might get annoying.
There was a problem hiding this comment.
Maybe all of the sims should be set up in Robot.java, and the sims are what gets passed into each subsystem, rather than them creating their own? Or FuelSim should be created in Robot.java with the subsystems passed in to it
There was a problem hiding this comment.
The former might be possible, but the latter would force the simulation classes to be public which isn't great practice in general.
Changing FuelSim might have to be in a separate PR as well, since it's already merged in for the stage 1 templates and stage 1A solutions.
There was a problem hiding this comment.
Yeah this being static is fine here
No description provided.