Elevator safety - max Chen#99
Conversation
|
/format |
JacksonElia
left a comment
There was a problem hiding this comment.
I think before this is merged, a lot of testing should be done, but the way you implemented it is nice. Cooking fr
| // Called once the command ends or is interrupted. | ||
| @Override | ||
| public void end(boolean interrupted) {} | ||
|
|
There was a problem hiding this comment.
Maybe you want to set the elevator motor speed to 0 when the command ends? Ask mr ishan
| if (Math.abs(swerveDrive.getRoll()) >= ElevatorConstants.MAX_ROLL_ANGLE // gyro safety | ||
| || Math.abs(swerveDrive.getPitch()) | ||
| >= ElevatorConstants | ||
| .MAX_PITCH_ANGLE) // TODO if robot is not in climbing state, then do this (AND |
There was a problem hiding this comment.
We don't care about the state of the elevator when we are climbing right? In fact the elevator should be all the way down while climbing, so this should only help us.
There was a problem hiding this comment.
W THINKING MAX L WAS WRONG LOLLLLL
| // logic) | ||
| { | ||
| // elevatorSubsystem.setDefaultCommand(new ZeroElevator(elevatorSubsystem)); // maybe works | ||
| elevatorSubsystem.setElevatorPosition(0); |
There was a problem hiding this comment.
I haven't looked into both of these things enough to comment on whether you guys did this correctly or not....
There was a problem hiding this comment.
test it ig -maxxxxxxxxxc
| import frc.robot.subsystems.elevator.ElevatorSubsystem; | ||
|
|
||
| /* You should consider using the more terse Command factories API instead https://docs.wpilib.org/en/stable/docs/software/commandbased/organizing-command-based.html#defining-commands */ | ||
| public class ZeroElevator extends Command { |
There was a problem hiding this comment.
Make this a command factory in elevator subsystem
| // The limits for the gyro | ||
| public static final double MAX_ROLL_ANGLE = 30; // degrees | ||
| public static final double MAX_PITCH_ANGLE = 20; // degrees | ||
| // public static final double MAX_ACCEL_X = 69; |
There was a problem hiding this comment.
These should be in swerve constants and wouldn't rlly have anything to do with moving the elevator; they just limit the motion of the robot so we don't tip.
| return gyroInputs.yawDegrees; | ||
| } | ||
|
|
||
| public double getPitch() { |
There was a problem hiding this comment.
Make the docstring have a return in it, and specify what units/direction this is in. E.g. Gets the pitch of the robot in degrees, positive being when the robot leans forwards (this is just a guess, I could be wrong, so double check it)
|
I will delete all your code - Rohan |
JacksonElia
left a comment
There was a problem hiding this comment.
Nice work, this code could def save us
| // operatorController.b().whileTrue(Commands.none()); | ||
| // operatorController.y().whileTrue(Commands.none()); | ||
| // operatorController.x().whileTrue(Commands.none()); |
There was a problem hiding this comment.
| // operatorController.b().whileTrue(Commands.none()); | |
| // operatorController.y().whileTrue(Commands.none()); | |
| // operatorController.x().whileTrue(Commands.none()); |
| leaderMotor.setPosition(0);//set encoder back to 0 | ||
| followerMotor.setPosition(0); // set encoder back to 0 | ||
| //leaderMotor.setPosition(Rotations.of(0.0)); | ||
| followerMotor.set(0); | ||
| leaderMotor.set(0);} |
There was a problem hiding this comment.
do you need to set both the leader and follower? Couldn't you just do the leader?
| leaderMotor.setPosition(ElevatorConstants.MAX_HEIGHT);//set encoder to max height | ||
| followerMotor.setPosition(ElevatorConstants.MAX_HEIGHT); // set encoder to max height |
There was a problem hiding this comment.
Idk if you want to set the position here, it could lead to the elevator getting zeroed wrong I think
There was a problem hiding this comment.
dat da whole point of dis tho
There was a problem hiding this comment.
no but like we should only ever zero it at the bottom i feel. Like what if the elevator gets stuck on something while its going up, or it gets 90% of the way up and bc of fucky belt stuff can't go any further?
| public static final double MAX_ROLL_ANGLE = 30; // degrees | ||
| public static final double MAX_PITCH_ANGLE = 20; // degrees |
There was a problem hiding this comment.
We should maybe try to "tune" these values, but these are probably pretty good
There was a problem hiding this comment.
Pull Request Overview
This pull request enhances elevator safety by adding a new ZeroElevator command, incorporating homing logic into the PhysicalElevator, and introducing stator current monitoring and safety checks during teleop.
- Introduces a ZeroElevator command to reset elevator position and stop motors.
- Implements elevator homing logic based on stator current and velocity conditions in PhysicalElevator.
- Updates elevator constants, adds current measurement support, and includes additional safety checks in teleopPeriodic.
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/frc/robot/commands/elevator/ZeroElevator.java | Adds new command for zeroing the elevator. |
| src/main/java/frc/robot/subsystems/elevator/PhysicalElevator.java | Introduces stator current monitoring and homing functionality. |
| src/main/java/frc/robot/subsystems/elevator/ElevatorInterface.java | Adds default methods for stator current and homing. |
| src/main/java/frc/robot/Robot.java | Integrates ZeroElevator and homing checks in teleopPeriodic along with safety limits. |
| src/main/java/frc/robot/subsystems/swerve/gyro/GyroInterface.java, SwerveDrive.java, PhysicalGyro.java, SimulatedGyro.java | Enhances gyro input handling by including pitch and roll. |
| src/main/java/frc/robot/subsystems/elevator/ElevatorConstants.java | Updates elevator proportional gain and introduces stator current thresholds. |
Comments suppressed due to low confidence (1)
src/main/java/frc/robot/subsystems/elevator/ElevatorConstants.java:35
- MIN_STATOR_THRESHOLD is set to the same value as MAX_STATOR_THRESHOLD, which might lead to unintended logic in the homing method; please confirm that this is the desired behavior.
public static final double MIN_STATOR_THRESHOLD = 59343494;
| public static final double MAX_STATOR_THRESHOLD = 59343494; | ||
| public static final double MIN_STATOR_THRESHOLD = 59343494; | ||
|
|
There was a problem hiding this comment.
The MAX_STATOR_THRESHOLD value appears unusually high; please double-check whether this constant accurately reflects operational limits.
| public static final double MAX_STATOR_THRESHOLD = 59343494; | |
| public static final double MIN_STATOR_THRESHOLD = 59343494; | |
| public static final double MAX_STATOR_THRESHOLD = 40.0; | |
| public static final double MIN_STATOR_THRESHOLD = 0.0; |
There was a problem hiding this comment.
it said 59334343494 is not operational limits???
|
we might merge b4 pitt idk. depends on how it goes today |
|
ok so i did add docstrings and I did the eject coral thing |
No description provided.