Finished final tweaks for lift, also includes tests #78

nelly-yy · 2018-02-19T20:27:34Z

Tweaks include manip. mapping, ports, slaves, and also tests. Also added gabe's doc. commit for compatibility.

for example, outake -> outtake, adding "Button" to button names, and ensuring they all start lowercase

The 4 stages of lift are now mapped to POV for teleop. Also did tweaks like motor ports, slaves, etc. Also note that manipulator in OI is static now for default command in lift.

…otCode2018 into controller-docs

https://github.com/gabe-mitnick/RobotCode2018 Conflicts: Robot2018/src/org/usfirst/frc/team199/Robot2018/OI.java

doawelul · 2018-02-20T02:54:22Z

Robot2018/src/org/usfirst/frc/team199/Robot2018/OI.java

-		PIDMoveButton = new JoystickButton(leftJoy, getButton("PID Move", 7));
-		PIDMoveButton
+		pIDMoveButton = new JoystickButton(leftJoy, getButton("PID Move", 7));
+		pIDMoveButton


On the right track, can we get pidMoveButton instead of pIDMoveButton?

doawelul · 2018-02-20T02:55:16Z

Robot2018/src/org/usfirst/frc/team199/Robot2018/OI.java

+		//	intakeCubeButton = new JoystickButton(manipulator, getButton("Intake Button", 5));
+		//	intakeCubeButton.whenPressed(new IntakeCube());
+		//	outtakeCubeButton = new JoystickButton(manipulator, getButton("Outtake Button", 6));
+		//	outtakeCubeButton.whenPressed(new OuttakeCube());


Is there a reason you changed these comments to start with a tab rather than a space? Did you use the eclipse auto-formatter?

doawelul · 2018-02-20T02:57:42Z

Robot2018/src/org/usfirst/frc/team199/Robot2018/RobotMap.java

-	public static WPI_TalonSRX liftMotor;
+	public static WPI_VictorSPX liftMotorA;
+	public static WPI_TalonSRX liftMotorB;
+	public static WPI_TalonSRX liftMotorC;


Seems fine for now, but according to Dean, there will be a master and two slaves, so just a thing to note right now so that we can change to names for readability later.

doawelul · 2018-02-20T03:07:12Z

Robot2018/src/org/usfirst/frc/team199/Robot2018/commands/AutoLift.java

@@ -104,7 +88,7 @@ protected void execute() {
    // Make this return true when this Command no longer needs to run execute()
    protected boolean isFinished() {
    	if(liftController.onTarget()) {
-    		lift.setTargetPosition(desiredPos);
+    		lift.setCurrPosition(desiredPos);


Is there a reason why this lift.setCur.... cannot be written in end()?

doawelul · 2018-02-20T03:07:42Z

Robot2018/src/org/usfirst/frc/team199/Robot2018/RobotMap.java

+		configSRX(liftMotorB);
+		liftMotorC = new WPI_TalonSRX(getPort("2LiftTalonSRX", 7));
+		configSRX(liftMotorC);
+		liftMotors = new SpeedControllerGroup(liftMotorB, liftMotorA, liftMotorC);


Same as above, the keys are a bit weird because 1) The keys are sorted alphabetically so the keys will not appear together and 2) Viewing "LiftVictorSPX" w/o the others might make people think that that is the only lift VictorSPX. I suggest changing to "Lift Motor Master", "Lift Motor Slave 1" etc.

doawelul · 2018-02-20T18:59:08Z

Robot2018/src/org/usfirst/frc/team199/Robot2018/subsystems/Climber.java

@@ -52,7 +52,7 @@ public void attachToBar() {
 	 * stops the climber
 	 */
 	public void stopClimber() {
-		climberMotor.stopMotor();
+		//climberMotor.stopMotor();


Some commands/other people may call this and expect it to work, maybe change the javadoc?

doawelul · 2018-02-20T19:01:15Z

Robot2018/src/org/usfirst/frc/team199/Robot2018/subsystems/Lift.java

+	 * @param newPosition - the new position meant to be set
+	 */
+    public void setCurrPosition(Position newPosition) {
+    		currPosition = newPosition;
    }

    /**
 	 * Uses (insert sensor here) to detect the current lift position 


You can insert sensor here now.

Couple of issues: Validity of constants that affect acceleration as well as PID constant tuning. On a separate note, all the commented lines with parameters on top of variables is for dynamic value testing ( part of junit ) .

brettle · 2018-02-26T02:47:04Z

@nelsonychs can you resolve conflicts in your fork, commit and push, so this can be merged cleanly?

nelly-yy · 2018-02-26T06:02:52Z

@brettle yes, I'll finish by tomorrow probably. Also, sorry for ignoring your suggestions @doawelul , I wasn't notified by github, thanks for spotting those issues.

brettle · 2018-02-26T02:53:32Z

Robot2018/src/org/usfirst/frc/team199/Robot2018/commands/AutoLift.java

-	 * All distances are - 1 foot for initial height of intake and + 3 inches for wiggle room for dropping cubes
-	 * Also, acutal distances are divided by 3 because according to cad, the lift will have a 1:3 ratio from winch
+	 * All distances are measured from bottom of cube and + 3 inches for wiggle room for dropping cubes
+	 * Also, actual distances are divided by 3 because according to cad, the lift will have a 1:3 ratio from winch
 	 * to actual height.


It would be clearer to just include the factor of 3 in the encoder's distance per pulse.

brettle · 2018-02-26T20:04:56Z

Robot2018/src/org/usfirst/frc/team199/Robot2018/subsystems/Lift.java

+    	else if(ang == 270)
+    		com = new AutoLift(Position.SWITCH, this);
+    	if(com != null)
+    		setDefaultCommand(com);


I don't understand what the above code is supposed to do. Note that initDefaultCommand() is generally only called by wpilib immediately after the subsystem is created. If the goal is to use the manipulator POV to control the height of the lift, you probably want a single DefaultLift() command that continuously reads the POV angle and attempts to maintain the corresponding height. You should also think about what the DefaultLift() command should do if no POV button is pressed?

On a separate note, a POV angle of 0 (which the docs indicate is "up") seems like an odd choice for going to the ground.

That makes more sense, what do you recommend I use to achieve that? Is there a way to call AutoLift from another command so that I don't repeat functionality?

Here's what I'd do:

Move the PID control of the lift out of the commands. It should always be running to move the lift to the desired height.

Make the AutoLift() command just set the target height and wait until it is reached.

Make the DefaultLift() command set the target height based on POV angle (and perhaps other input).

brettle · 2018-02-26T21:07:12Z

Robot2018/test/org/usfirst/frc/team199/Robot2018/AutoLiftTest.java

+		//1 second?
+		for(int i = 0; i < 50; i++) {
+			liftController.calc();
+		}


I believe the PIDController's thread will also be calling calculate() periodically. This will be in addition to your calls via calc(). The combination could cause the test to be unreliable.

I recommend overriding PIDController.calculate() so that it only calls super.calculate() if Thread.currentThread().getId() matches the thread id being used by the simulator.

I assumed that would be the case as well, but after looking at the source code it doesn't seem to work that way. All enable() does is set an enabled boolean that doesn't necessarily start the thread. I'm positive the current way of calculating is only one thread.

The thread (actually a timer that periodically runs a task on a separate thread) is started in the constructor here. Still positive? :)

The reason I was positive was because I tested the loop by printing the current position rather than calculating after enabling the controller, it was zero all throughout. But the print lines were far faster than the calculating thread so I see now, I'll take your advice and implement calculate that way, thanks.

nelly-yy and others added 9 commits February 18, 2018 22:54

Added auto testing code and other minor fixes

5bfb1c5

add JavaDocs to some commands and fix naming

814628a

for example, outake -> outtake, adding "Button" to button names, and ensuring they all start lowercase

update controllers.txt with all of the buttons in OI

1879b6b

Merge branch 'master' into controller-docs

8d5328f

Mapped 4 stages of lift to manipulator POV for tele, also minor tweaks

f2a2bec

The 4 stages of lift are now mapped to POV for teleop. Also did tweaks like motor ports, slaves, etc. Also note that manipulator in OI is static now for default command in lift.

add joystick functionalities to controllers.txt

7405909

Merge branch 'controller-docs' of https://github.com/Gabe-Mitnick/Rob…

07d1fb8

…otCode2018 into controller-docs

Merge branch 'controller-docs' of

c2fba8b

https://github.com/gabe-mitnick/RobotCode2018 Conflicts: Robot2018/src/org/usfirst/frc/team199/Robot2018/OI.java

Accidentally didn't commit update doc

9f30235

doawelul reviewed Feb 20, 2018

View reviewed changes

PID Simulator for lift, currently tuning constants

8ee9a13

Couple of issues: Validity of constants that affect acceleration as well as PID constant tuning. On a separate note, all the commented lines with parameters on top of variables is for dynamic value testing ( part of junit ) .

brettle suggested changes Feb 26, 2018

View reviewed changes

brettle mentioned this pull request Mar 1, 2018

Lift should shift to low gear when holding position to reduce current draw #87

Open

brettle merged commit 8ee9a13 into DeepBlueRobotics:master Mar 11, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Finished final tweaks for lift, also includes tests #78

Finished final tweaks for lift, also includes tests #78

nelly-yy commented Feb 19, 2018 •

edited

Loading

doawelul Feb 20, 2018

doawelul Feb 20, 2018

doawelul Feb 20, 2018

doawelul Feb 20, 2018

doawelul Feb 20, 2018

doawelul Feb 20, 2018

doawelul Feb 20, 2018

brettle commented Feb 26, 2018

nelly-yy commented Feb 26, 2018

brettle Feb 26, 2018

brettle Feb 26, 2018

nelly-yy Feb 27, 2018

brettle Feb 28, 2018

brettle Feb 26, 2018

nelly-yy Feb 27, 2018

brettle Feb 28, 2018

nelly-yy Mar 1, 2018

Finished final tweaks for lift, also includes tests #78

Finished final tweaks for lift, also includes tests #78

Conversation

nelly-yy commented Feb 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brettle commented Feb 26, 2018

nelly-yy commented Feb 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nelly-yy commented Feb 19, 2018 •

edited

Loading