climb pivot + algae flywheel #35
Conversation
…tgpt plz review it :(
| return ticksToDegrees(climbPivotMotor.getSelectedSensorPosition()); | ||
| } | ||
|
|
||
| public void stop() { |
There was a problem hiding this comment.
wait I'm not sure what the mechanism looks like, but what is stop doing? If we have one method used for setting a position, it seems weird having another method for just like turning off the motor.
Rather than that it would make more sense to me that the commands that call this would just use setPivotPosition to set it to its like inactive position.
|
Lowkey Idk enough about our design to review this effectively |
|
Abigail doesn't want this to be reviewed yet lol |
|
Should make this a draft or smthn |
|
|
||
| public ClimbPivot(frc.robot.subsystems.pivot.ClimbPivot climbPivot, double targetPosition) { | ||
| this.climbPivot = climbPivot; | ||
| this.targetPosition = targetPosition; | ||
| // Use addRequirements() here to declare subsystem dependencies. |
There was a problem hiding this comment.
addRequirements(climbPivot); plz
| // Use addRequirements() here to declare subsystem dependencies. | ||
| } | ||
|
|
||
| // Called when the command is initially scheduled. | ||
| @Override | ||
| public void initialize() {} | ||
| public void initialize() { | ||
| climbPivot.setPivotPosition(targetPosition); |
There was a problem hiding this comment.
i might consider using a command factory for this @Abigail-27
This is a very simple command and it could use it.
check out #53
|
|
||
| private double targetPosition = 0; | ||
|
|
||
| public ClimbPivot(frc.robot.subsystems.pivot.ClimbPivot climbPivot, double targetPosition) { |
There was a problem hiding this comment.
you shouldn't be passing ur target position into your command, it should probably be a constant for the climb pivot angle.
There was a problem hiding this comment.
uhhh ur gonna help me with that later
There was a problem hiding this comment.
if u need help with stuff during meetings please come and ask me :/ I will gladly help you.
|
|
||
| private double targetPosition = 0; | ||
|
|
||
| public ClimbPivot(frc.robot.subsystems.pivot.ClimbPivot climbPivot, double targetPosition) { |
There was a problem hiding this comment.
I would probably not name the subsystem and command files the same so you can avoid weird import things like this.
We have restarted doing <Subsystem>Subsystem, so you should rename your ClimbPivot in you subsystems file to ClimbPivotSubsystem. You could also rename this file to ClimbPivotCommand.
There was a problem hiding this comment.
but delete it, right? like the whole command?
There was a problem hiding this comment.
if you do the command factory then yes
| public static final double FLYWHEEL_I = 0 - 9; | ||
| public static final double FLYWHEEL_D = 0 - 9; | ||
|
|
||
| public static final double JKG_METERS_SQUARED = 0 - 9; |
There was a problem hiding this comment.
rename this to moment of inertia
| private double currentVolts; | ||
|
|
||
| public PhysicalFlywheel() { | ||
| PID = |
There was a problem hiding this comment.
Yeah also for the Physical<Subsystem>'s we generally don't use wpilib's pid, because it runs on the RoboRIO itself, which is slower than running it directly on the motors. You also need to have a flywheel motor in this impl to control, update and configure.
There was a problem hiding this comment.
making a note so i can ask u later
|
|
||
| /** Add your docs here. */ | ||
| public class SimulatedFlywheel implements FlywheelInterface { | ||
| private FlywheelSim simulatedFlywheel = new FlywheelSim(null, DCMotor.getFalcon500(2), 0); |
There was a problem hiding this comment.
you can add these constants in :/
| } | ||
|
|
||
| public void setVolts(double volts) { | ||
| currentVolts = simPID.calculate(volts); |
There was a problem hiding this comment.
if you are setting straight voltage i wouldn't use pid.
| } | ||
|
|
||
| public void setFlywheelSpeed(double speed) { | ||
| setVolts(simPID.calculate(getFlywheelSpeed(), speed)); |
There was a problem hiding this comment.
make sure your units are and will be consistent.
also your setVolts method uses the pid to calculate the pid calculating speed. That's kinda not good.
| public class SimulatedFlywheel implements FlywheelInterface { | ||
| private FlywheelSim simulatedFlywheel = new FlywheelSim(null, DCMotor.getFalcon500(2), 0); | ||
| private PIDController simPID; | ||
| private double currentVolts; |
There was a problem hiding this comment.
id log this value to akit by adding something like flywheelAppliedVolts to the interface, and then setting this value equal to that in updateInputs.
REVIEW REVIEWWWW