-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Safe mode #32
base: master
Are you sure you want to change the base?
Safe mode #32
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically this code has already been tested and it works ok, but I still see a lot of problems with it that I would like to see resolved ideally before Save the Music. I can't "Request Changes" on my own branch but I do not approve this PR. @FriedLongJohns (work on arm related issues) or @BrownBear85 (work on drivetrain related issues) please fix this when you have time.
src/main/java/org/carlmontrobotics/robotcode2023/Constants.java
Outdated
Show resolved
Hide resolved
src/main/java/org/carlmontrobotics/robotcode2023/Constants.java
Outdated
Show resolved
Hide resolved
src/main/java/org/carlmontrobotics/robotcode2023/Constants.java
Outdated
Show resolved
Hide resolved
src/main/java/org/carlmontrobotics/robotcode2023/RobotContainer.java
Outdated
Show resolved
Hide resolved
src/main/java/org/carlmontrobotics/robotcode2023/subsystems/Arm.java
Outdated
Show resolved
Hide resolved
src/main/java/org/carlmontrobotics/robotcode2023/Constants.java
Outdated
Show resolved
Hide resolved
src/main/java/org/carlmontrobotics/robotcode2023/commands/TeleopDrive.java
Outdated
Show resolved
Hide resolved
src/main/java/org/carlmontrobotics/robotcode2023/commands/TeleopDrive.java
Outdated
Show resolved
Hide resolved
src/main/java/org/carlmontrobotics/robotcode2023/subsystems/Arm.java
Outdated
Show resolved
Hide resolved
src/main/java/org/carlmontrobotics/robotcode2023/subsystems/Arm.java
Outdated
Show resolved
Hide resolved
src/main/java/org/carlmontrobotics/robotcode2023/subsystems/Arm.java
Outdated
Show resolved
Hide resolved
was an unused controller rumble variable
wasn't fully implemented before +allow ArmTeleop.getRequestedSpeeds() to use baby max vel +fix Arm.arm/wristConstraints not changing the max ff accel even though we had a baby max accel
Tasks
^ Above tasks can be done in any order
Also, a review of how baby/safemode affects things (Correct me if I'm wrong, @ProfessorAtomicManiac ):
|
Yep that's correct!
|
This reverts commit cf37551.
was changeable but in constants, now moved to Roller subsystem =Also remove get/set of RollerMode.speed in favor of just making it public (if you both get and set a variable publicly without doing anything special, don't make it private)
now only check one enum instead of checking both slow and baby (baby takes priority, of course)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the looks of it, it mostly works. The code is still rough in some places so please try to clean those up before Sunday's worksession.
// TODO: Determine actual speeds/timings for roller | ||
public static class RollerMode { | ||
public static RollerMode INTAKE_CONE = new RollerMode(-0.5, .5, GameObject.CONE, conePickupColor); | ||
public static RollerMode INTAKE_CUBE = new RollerMode(0.4, .25, GameObject.CUBE, cubePickupColor); | ||
// The obj indicates which game object the roller is trying to intake | ||
// if obj == NONE, that means it is trying to outtake rather than intake | ||
public static RollerMode OUTTAKE_CONE = new RollerMode(0.5, .5, GameObject.NONE, defaultColor); | ||
public static RollerMode OUTTAKE_CUBE = new RollerMode(-0.5, .5, GameObject.NONE, defaultColor); | ||
public static RollerMode STOP = new RollerMode(0, .1, GameObject.NONE, defaultColor); | ||
public double speed, time; | ||
public GameObject obj; | ||
public Color ledColor; | ||
|
||
/** | ||
* @param speed A number between -1 and 1 | ||
* @param time Amount of time in seconds to keep the motor running after | ||
* distance sensor has detected an object | ||
* @param intake Whether the roller is outtaking or intaking | ||
*/ | ||
public RollerMode(double speed, double time, GameObject obj, Color ledColor) { | ||
this.speed = speed; | ||
this.time = time; | ||
this.obj = obj; | ||
this.ledColor = ledColor; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Originally Ryan wanted all constants put into Constants.java. Personally, I think constants that are strictly only used in subsystems should stay in their subsystem classes and the only appropriate constants that should be put in Constants.java are constants that are used between subsystems or ports. That way, there is less clutter in Constants.java and it is easier to find certain constants. Seems like you agree with me on this, but do we wanna make it convention that all subsystem specific constants (excluding ports) should stay in subsystems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other solutions include creating completely new files just to store constants to organize them better. I realize that storing goal positions in Arm.java would be extremely annoying. We should probably discuss and agree on how exactly we want to store constants.
//set mode to baby if baby was true | ||
//else but don't set mode to norm if slow was true | ||
if (SmartDashboard.getBoolean("safeMode", false)) { | ||
RobotContainer.driverMode = RobotContainer.DriverMode.BABY; | ||
} else if (RobotContainer.driverMode.isSlow()) { | ||
RobotContainer.driverMode = RobotContainer.DriverMode.NORM; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once you enable safeMode or slow the robot, how is it supposed to return back to normal mode when switched off?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also see my comment in RobotContainer.java
|
||
public static enum DriverMode { | ||
NORM, SLOW, BABY; | ||
|
||
public boolean isNorm() { | ||
return this == DriverMode.NORM; | ||
} | ||
|
||
public boolean isSlow() { | ||
return this == DriverMode.SLOW; | ||
} | ||
|
||
public boolean isBaby() { | ||
return this == DriverMode.BABY; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm really against having the logic for DriverMode snucked away in a class file meant to initialize joysticks and commands. DriverMode logic is also mixed into Robot.java. In the future if someone wants to change up DriverMode, they would have to edit code in RobotContainer.java and Robot.java, both files that I would argue would not be the first place a programmer would think of to look in. Please refactor the driver mode code (maybe make an entire subsystem class just to keep track of DriverMode, as this is used across multiple subsystems. That's just an idea though, maybe there is a less "boilerplaty" solution).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also know that Ryan implemented a lib199 implmentation of all the safe mode stuff if you want to use that instead. I forgot how it works though.
double driveMultiplier = ((DriverMode)mode.get()).isBaby() ? kBabyDriveSpeed : ( | ||
((DriverMode)mode.get()).isSlow() ? kSlowDriveSpeed : kNormalDriveSpeed | ||
); | ||
double rotationMultiplier = ((DriverMode)mode.get()).isBaby() ? kBabyTurnSpeed : ( | ||
((DriverMode)mode.get()).isSlow() ? kSlowDriveRotation : kNormalDriveRotation | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reading the code before, I realized that kBabyDriveSpeed was never actually used and baby mode was just "slow mode". This will fix that error but kinda interesting that baby mode was never used.
public static class RollerMode { | ||
public static RollerMode INTAKE_CONE = new RollerMode(-0.5, .5, GameObject.CONE, conePickupColor); | ||
public static RollerMode INTAKE_CUBE = new RollerMode(0.4, .25, GameObject.CUBE, cubePickupColor); | ||
// The obj indicates which game object the roller is trying to intake | ||
// if obj == NONE, that means it is trying to outtake rather than intake | ||
public static RollerMode OUTTAKE_CONE = new RollerMode(0.5, .5, GameObject.NONE, defaultColor); | ||
public static RollerMode OUTTAKE_CUBE = new RollerMode(-0.5, .5, GameObject.NONE, defaultColor); | ||
public static RollerMode STOP = new RollerMode(0, .1, GameObject.NONE, defaultColor); | ||
public double speed; | ||
public double time; | ||
public GameObject obj; | ||
public Color ledColor; | ||
|
||
/** | ||
* @param speed A number between -1 and 1 | ||
* @param time Amount of time in seconds to keep the motor running after | ||
* distance sensor has detected an object | ||
* @param intake Whether the roller is outtaking or intaking | ||
*/ | ||
public RollerMode(double speed, double time, GameObject obj, Color ledColor) { | ||
this.speed = speed; | ||
this.time = time; | ||
this.obj = obj; | ||
this.ledColor = ledColor; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comment in Constants.java
Don't merge into master until after everything is tested out