First commit of DavidRobot2018 #5
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,18 @@ | ||
| /*----------------------------------------------------------------------------*/ | ||
| /* Copyright (c) 2017-2018 FIRST. All Rights Reserved. */ | ||
| /* Open Source Software - may be modified and shared by FRC teams. The code */ | ||
| /* must be accompanied by the FIRST BSD license file in the root directory of */ | ||
| /* the project. */ | ||
| /*----------------------------------------------------------------------------*/ | ||
|
|
||
| package org.usfirst.frc.team199.Robot2018; | ||
|
|
||
| import org.usfirst.frc.team199.Robot2018.commands.ShiftLowGear; | ||
| import org.usfirst.frc.team199.Robot2018.commands.ShiftDriveType; | ||
|
|
||
| import edu.wpi.first.wpilibj.Joystick; | ||
| import edu.wpi.first.wpilibj.buttons.JoystickButton; | ||
| import edu.wpi.first.wpilibj.smartdashboard.SmartDashboard; | ||
|
|
||
| /** | ||
| * This class is the glue that binds the controls on the physical operator | ||
|
|
@@ -34,4 +46,27 @@ public class OI { | |
| // Start the command when the button is released and let it run the command | ||
| // until it is finished as determined by it's isFinished method. | ||
| // button.whenReleased(new ExampleCommand()); | ||
|
|
||
| public Joystick leftJoy; | ||
| public JoystickButton shiftDrive; | ||
| public JoystickButton shiftDriveType; | ||
| public Joystick rightJoy; | ||
| public Joystick manipulator; | ||
|
|
||
|
|
||
| public int getButton(String key, int def) { | ||
| if(!SmartDashboard.containsKey(key)) { | ||
| SmartDashboard.putNumber(key, def); | ||
| } | ||
| return (int) SmartDashboard.getNumber(key, def); | ||
| } | ||
| public OI() { | ||
| leftJoy = new Joystick(0); | ||
| shiftDrive = new JoystickButton(leftJoy, getButton("Button Shift Drive", 1)); | ||
| shiftDrive.whenPressed(new ShiftLowGear()); | ||
| shiftDriveType = new JoystickButton(leftJoy, getButton("Button Shift Drive Type", 2)); | ||
| shiftDriveType.whenPressed(new ShiftDriveType()); | ||
| rightJoy = new Joystick(1); | ||
|
There was a problem hiding this comment. The names shiftDrive and shiftDriveType (and the corresponding terminology in the strings) is confusing. Would better names be something like "shift to low gear" and "toggle (or switch) drive type"? Has anyone interviewed the operators to determine how they want the controls to work. Also, can we create a picture and/or instructions which are accessible from the SmartDashboard which describes how the controls work. That picture/instructions would be maintained and committed beside the code and there should be a prominent comment in OI.java indicating that the picture/instructions need to be updated when the code is changed. There was a problem hiding this comment. No longer relevant; see 9083d16 There was a problem hiding this comment. All good on the first 2 points. Regarding the manip diagrams, please create an issue/card to make sure this doesn't get dropped. There was a problem hiding this comment. |
||
| manipulator = new Joystick(2); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| package org.usfirst.frc.team199.Robot2018.commands; | ||
|
|
||
| import org.usfirst.frc.team199.Robot2018.Robot; | ||
|
|
||
| import edu.wpi.first.wpilibj.command.Command; | ||
|
|
||
| /** | ||
| * | ||
| */ | ||
| public class PIDDrive extends Command { | ||
|
There was a problem hiding this comment. Need a comment describing what this command is supposed to do. My guess is that it is intended to use pid to drive a target distance straight forward. However, it doesn't look like it will do that since it seems to always send the same value to both motors. Mechanical and electrical differences between the left and right side of the drivetrain will cause them not to move at the same speed for the same value, and the robot will drift left or right. There was a problem hiding this comment. Drivetrain implements PIDOutput (hopefully correctly) so the value does change. Differences can/should be accounted for by the PID constants. There was a problem hiding this comment. The value changes over time due to Drivetrain's PID code, but at any given time the same value is sent directly to both motor controllers. What am I missing? There was a problem hiding this comment. I don't currently have a "correct" fix to the proposed issue; more research would be required. One possible fix could be adding coefficients in the Drivetrain class so that callers can assume that both sides run equally. |
||
|
|
||
| double target; | ||
| public PIDDrive(double targ) { | ||
| // Use requires() here to declare subsystem dependencies | ||
| // eg. requires(chassis); | ||
| target = targ; | ||
| requires(Robot.dt); | ||
|
There was a problem hiding this comment. Once you create an interface for the Drivetrain (which will be the next step), you can great a local variable in this class of the type DrivetrainInterface so you don't have to call Robot.dt every time |
||
| } | ||
|
|
||
| // Called just before this Command runs the first time | ||
| protected void initialize() { | ||
| Robot.dt.enableMovePid(); | ||
| Robot.dt.setSetMove(target); | ||
| } | ||
|
|
||
| // Called repeatedly when this Command is scheduled to run | ||
| protected void execute() { | ||
| Robot.dt.arcadeDrive(Robot.dt.getPidOut(), 0); | ||
| } | ||
|
|
||
| // Make this return true when this Command no longer needs to run execute() | ||
| protected boolean isFinished() { | ||
| return Robot.dt.onDriveTarg(); | ||
| } | ||
|
|
||
| // Called once after isFinished returns true | ||
| protected void end() { | ||
| Robot.dt.stopDrive(); | ||
| } | ||
|
|
||
| // Called when another command which requires one or more of the same | ||
| // subsystems is scheduled to run | ||
| protected void interrupted() { | ||
| end(); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| package org.usfirst.frc.team199.Robot2018.commands; | ||
|
|
||
| import org.usfirst.frc.team199.Robot2018.Robot; | ||
| import org.usfirst.frc.team199.Robot2018.RobotMap; | ||
|
|
||
| import edu.wpi.first.wpilibj.command.Command; | ||
|
|
||
| /** | ||
| * | ||
| */ | ||
| public class PIDTurn extends Command { | ||
|
There was a problem hiding this comment. Please see my comments on PIDDrive. They apply here as well. In this case the assymetries will cause the robot to move somewhat forward or backward when attempting to turn in place. |
||
|
|
||
| double target; | ||
| public PIDTurn(double targ) { | ||
| // Use requires() here to declare subsystem dependencies | ||
| // eg. requires(chassis); | ||
| target = targ; | ||
| requires(Robot.dt); | ||
| } | ||
|
|
||
| // Called just before this Command runs the first time | ||
| protected void initialize() { | ||
| Robot.dt.resetAHRS(); | ||
| Robot.dt.setSetTurn(target); | ||
| Robot.dt.enableTurnPid(); | ||
| } | ||
|
|
||
| // Called repeatedly when this Command is scheduled to run | ||
| protected void execute() { | ||
| Robot.dt.arcadeDrive(0, Robot.dt.getPidOut()); | ||
| } | ||
|
|
||
| // Make this return true when this Command no longer needs to run execute() | ||
| protected boolean isFinished() { | ||
| return Robot.dt.onTurnTarg(); | ||
| } | ||
|
|
||
| // Called once after isFinished returns true | ||
| protected void end() { | ||
| Robot.dt.stopDrive(); | ||
| } | ||
|
|
||
| // Called when another command which requires one or more of the same | ||
| // subsystems is scheduled to run | ||
| protected void interrupted() { | ||
| end(); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,43 @@ | ||
| package org.usfirst.frc.team199.Robot2018.commands; | ||
|
|
||
| import edu.wpi.first.wpilibj.Timer; | ||
| import edu.wpi.first.wpilibj.command.Command; | ||
| import edu.wpi.first.wpilibj.smartdashboard.SmartDashboard; | ||
|
|
||
| /** | ||
| * | ||
| */ | ||
| public class ShiftDriveType extends Command { | ||
|
|
||
| Timer tim; | ||
| public ShiftDriveType() { | ||
| // Use requires() here to declare subsystem dependencies | ||
| // eg. requires(chassis); | ||
| } | ||
|
|
||
| // Called just before this Command runs the first time | ||
| protected void initialize() { | ||
| tim = new Timer(); | ||
| tim.reset(); | ||
| tim.start(); | ||
| } | ||
|
|
||
| // Called repeatedly when this Command is scheduled to run | ||
| protected void execute() { | ||
| SmartDashboard.putBoolean("Arcade Drive", !SmartDashboard.getBoolean("Arcade Drive", false)); | ||
| } | ||
|
|
||
| // Make this return true when this Command no longer needs to run execute() | ||
| protected boolean isFinished() { | ||
| return tim.get() > 0.125; | ||
|
There was a problem hiding this comment. Isn't there a more reliable and clear way to do this? There was a problem hiding this comment. This command just switches the boolean value; the person who implements JUnit test will be responsible for commenting the code. There was a problem hiding this comment. After another look, I'm pretty sure this code is just plain broken. Looks like it will toggle the value of "Arcade Drive" repeatedly (~every 20ms) for at least 125ms, leaving it in a psuedorandom state. On a separate note, the person who writes the code is generally responsible for writing appropriate comments. There was a problem hiding this comment. My bad. I see it's not broken now. Still unnecessarily confusing. Moved to issue #30. |
||
| } | ||
|
|
||
| // Called once after isFinished returns true | ||
| protected void end() { | ||
| } | ||
|
|
||
| // Called when another command which requires one or more of the same | ||
| // subsystems is scheduled to run | ||
| protected void interrupted() { | ||
| } | ||
| } | ||
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.
Do these need to be public for some reason?
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.
The joysticks need to be public because their values are needed in some functions. I have no clue about the buttons, however, that's how we did it last year so that's how I did it this week.
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 agree on the Joysticks. I saw they were being used after writing my comment.
I don't see any need to make the buttons public. It just makes it easier to introduce unnecessary dependencies. I recommend making them private for now and reevaluate if you find they need to be accessed from other classes.