Milkybeans - More human-friendly rewrite of Climber.java by FriedLongJohns · Pull Request #6 · DeepBlueRobotics/RobotCode2022

FriedLongJohns · 2022-09-19T03:28:35Z

Wow humans can use it now
And it doesn't look too bad
And it doesn't have ten billion no-argument functions

Also, milk.

-Put all Encoder Positions and Motor speeds into enums -Replaced all encoder set and motor speed set funcs with resetEncodersTo() and moveMotors() which use list lambdas to go fast and not check if statements -replace all boolean funcs with isMotorExtended and isMotorRetracted for all funcs using int argument called "motor", -1 left, 0 both, and 1 right motor resetEncodersTo.*() -> resetEncodersTo(EncoderPos, int motor) (?:slow)?(extend|retract).*() -> moveMotors(MotorSpeed, int motor) stop.*() -> stopMotors(int motor) is(Right|Left)(?:Reset)?(Retracted|Extended) -> isMotorExtended(int motor, boolean reset) and isMotorRetracted(int motor, boolean reset)

+Motor selection tuple +getMotorPos now maps to a lambda array =Rework isMotor(Extended|Retracted) to not use reset boolean anymore and operate on a "true until false" basis

-Fix enum system in Climber.java to actually work as intended by using dictionaries. -Go fix all the uses of Climber.java to also work correctly -It builds now Probably still horribly buggy

Co-authored-by: Kedas <m@yarn.network>

CoolSpy3

Looks pretty good. I just have a few comments.

CoolSpy3 · 2022-09-19T03:36:23Z

+    new JoystickButton(rightJoy, Constants.OI.RightJoy.slowExtendLeftClimberPort).whileHeld(new InstantCommand(()->climber.moveMotors(Climber.MotorSpeed.slowExtend,climber.leftMotor))).whenReleased(new InstantCommand(()->climber.stopMotors(climber.leftMotor)));
+    new JoystickButton(rightJoy, Constants.OI.RightJoy.slowRetractLeftClimberPort).whileHeld(new InstantCommand(()->climber.moveMotors(Climber.MotorSpeed.slowRetract,climber.leftMotor))).whenReleased(new InstantCommand(()->climber.stopMotors(climber.leftMotor)));
+    new JoystickButton(rightJoy, Constants.OI.RightJoy.slowExtendRightClimberPort).whileHeld(new InstantCommand(()->climber.moveMotors(Climber.MotorSpeed.slowExtend,climber.rightMotor))).whenReleased(new InstantCommand(()->climber.stopMotors(climber.rightMotor)));
+    new JoystickButton(rightJoy, Constants.OI.RightJoy.slowRetractRightClimberPort).whileHeld(new InstantCommand(()->climber.moveMotors(Climber.MotorSpeed.slowRetract,climber.rightMotor))).whenReleased(new InstantCommand(()->climber.stopMotors(climber.rightMotor)));


climber.leftMotor
climber.rightMotor

Static members should be accessed in a static manner
Climber.leftMotor
Climber.rightMotor

CoolSpy3 · 2022-09-19T03:41:17Z

-        super(
-            new FunctionalCommand(
-                () -> {},
-                climber::extendLeft,
-                climber::stopLeft,
-                climber::isLeftExtended
-            ),
-            new FunctionalCommand(
-                () -> {},
-                climber::extendRight,
-                climber::stopRight,
-                climber::isRightExtended
-            )
-        );


The point of using a ParallelCommandGroup with individual commands for each arm is that the climber won't malfunction if the arms become unaligned (unlikely but possible). I recommend that you restore this behavior through the use of a ParallelCommandGroup or by checking and updating the state of each arm in execute.

CoolSpy3 · 2022-09-19T03:42:03Z

+    public void end(){
+        climber.stopMotors(climber.bothMotors);
    }


Function end takes one argument boolean interrupted

CoolSpy3 · 2022-09-19T03:42:23Z

+import edu.wpi.first.wpilibj2.command.CommandBase;

-public class RetractClimber extends ParallelCommandGroup {
+public class RetractClimber extends CommandBase {


See comments on ExtendClimber

CoolSpy3 · 2022-09-19T03:47:52Z

+
+    public static enum MotorSpeed{retract,extend,slowRetract,slowExtend;};
+    private static Dictionary dMotorSpeed = new Hashtable();//given values in constructor
+
+
+    public static enum EncoderPos{extendLeft,extendRight,retractLeft,retractRight,zero;};
+    private static Dictionary dEncoderPos = new Hashtable();//given values in constructor


I recommend assigning a member variable to these enums instead of using a dictionary. For an example, see Limelight.TurnDirection. Additionally, it's convention to define enums at the end of the file.

CoolSpy3 · 2022-09-19T04:02:08Z


-public class ExtendClimber extends ParallelCommandGroup {
+public class ExtendClimber extends CommandBase {
+    private Climber climber;


Can be final

CoolSpy3 · 2022-09-19T04:16:30Z

+
+    private final Consumer[] setEncoder = new Consumer[]{ // faster to array[](inp) than a bunch of if's in a func
+      (pos) -> leftEncoder.setPosition((double) pos),
+      (pos) -> {
+        leftEncoder.setPosition((double) pos);
+        rightEncoder.setPosition((double) pos);
+      },
+      (pos) -> rightEncoder.setPosition((double) pos),
+    };
+
+    private final Consumer[] setMotor = new Consumer[]{
+      (speed) -> {
+        left.set((double) speed);
+        SmartDashboard.putString("Left Climber State", "Moving");
+      },
+      (speed) -> {
+        left.set((double) speed);
+        right.set((double) speed);
+        SmartDashboard.putString("Left Climber State", "Moving");
+        SmartDashboard.putString("Right Climber State", "Moving");
+      },
+      (speed) -> {
+        right.set((double) speed);
+        SmartDashboard.putString("Right Climber State", "Moving");
+      },
+    };
+


Please avoid raw types. Use DoubleConsumer instead. This should remove the need for a cast to double.

Oh, neat.
Didn't know that was a class.

CoolSpy3 · 2022-09-19T04:17:57Z

+        SmartDashboard.putString("Left Climber State", "Stop");
+      }
+      if (motor>-1){
        right.set(0);
-        SmartDashboard.putString("Right climber is", "Stopped");
-    }
-
-    public void stop(boolean interrupted) {
-        stop();
-    }
-
-    public void stopLeft(boolean interrupted) {
-        stopLeft();
-    }
-
-    public void stopRight(boolean interrupted) {
-        stopRight();
-    }
-
-    public double getLeftPosition() {
-        return leftEncoder.getPosition();
-    }
-
-    public double getRightPosition() {
-        return rightEncoder.getPosition();
-    }
-
-    public boolean isLeftExtended() {
-        return getLeftPosition() >= extendPositionLeft;
-    }
-
-    public boolean isRightExtended() {
-        return getRightPosition() >= extendPositionRight;
-    }
-
-    public boolean isLeftRetracted() {
-        return getLeftPosition() <= retractPositionLeft;
-    }
-
-    public boolean isRightRetracted() {
-        return getRightPosition() <= retractPositionRight;
-    }
-
-    public boolean isRightResetExtended() {
-        return getRightPosition() >= 0;
+        SmartDashboard.putString("Right Climber State", "Stop");


Should be "Stopped"

CoolSpy3 · 2022-09-19T07:42:26Z

I missed this earlier, but please change the pull request to merge into dev instead of master.

CoolSpy3 · 2022-09-21T22:58:51Z

I meant to define the enums at the end of the class definition.

…

On Wed, Sep 21, 2022 at 8:40 AM FriedLongJohns ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/main/java/org/team199/robot2022/subsystems/Climber.java <#6 (comment)> : > + + public static enum MotorSpeed{retract,extend,slowRetract,slowExtend;}; + private static Dictionary dMotorSpeed = new Hashtable();//given values in constructor + + + public static enum EncoderPos{extendLeft,extendRight,retractLeft,retractRight,zero;}; + private static Dictionary dEncoderPos = new Hashtable();//given values in constructor Do you mean to define them last out of all class variables or literally at the end of the java class? — Reply to this email directly, view it on GitHub <#6 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ANF6GTVMADXOR6ZBXMOFHA3V7MT47ANCNFSM6AAAAAAQPXKW6M> . You are receiving this because your review was requested.Message ID: ***@***.***>

mmm yes what a good push title

Reject Humanity, return to Monke

FriedLongJohns · 2022-09-22T00:43:54Z

Finished all your suggestions (I hope), will fix the new conflicts later.

CoolSpy3 · 2022-09-29T05:04:04Z

 import edu.wpi.first.wpilibj2.command.ParallelCommandGroup;

-public class ExtendClimber extends ParallelCommandGroup {
+public final class ExtendClimber extends ParallelCommandGroup {


Suggested change

public final class ExtendClimber extends ParallelCommandGroup {

public class ExtendClimber extends ParallelCommandGroup {

Sorry, my bad. The final comment was meant to refer to the member variable not the class. The commands do not have to be final.

Java throws an error that you're not allowed to use the 'final' modifier on the "public (Retract|Extend)Climber(Climber climber)"
So then what is "the member variable?"

I was talking about the line private Climber climber; and changing it to private final Climber climber; It looks like it was deleted in a previous commit, so no further changes are necessary.

CoolSpy3 · 2022-09-29T05:11:02Z

+                () -> {climber.moveMotors(Climber.MotorSpeed.extend,climber.rightMotor);},
+                (interrupted) -> {climber.stopMotors(climber.rightMotor);},
+                () -> {return climber.isMotorExtended(climber.rightMotor);}


Suggested change

() -> {climber.moveMotors(Climber.MotorSpeed.extend,climber.rightMotor);},

(interrupted) -> {climber.stopMotors(climber.rightMotor);},

() -> {return climber.isMotorExtended(climber.rightMotor);}

() -> {climber.moveMotors(Climber.MotorSpeed.extend,Climber.rightMotor);},

(interrupted) -> {climber.stopMotors(Climber.rightMotor);},

() -> {return climber.isMotorExtended(Climber.rightMotor);}

Static members should be accessed in a static manner. (i.e. use Climber instead of climber)

CoolSpy3 · 2022-09-29T05:12:02Z

+                () -> {climber.moveMotors(Climber.MotorSpeed.extend,climber.leftMotor);},
+                (interrupted) -> {climber.stopMotors(climber.leftMotor);},
+                () -> {return climber.isMotorExtended(climber.leftMotor);}


Suggested change

() -> {climber.moveMotors(Climber.MotorSpeed.extend,climber.leftMotor);},

(interrupted) -> {climber.stopMotors(climber.leftMotor);},

() -> {return climber.isMotorExtended(climber.leftMotor);}

() -> {climber.moveMotors(Climber.MotorSpeed.extend,Climber.leftMotor);},

(interrupted) -> {climber.stopMotors(Climber.leftMotor);},

() -> {return climber.isMotorExtended(Climber.leftMotor);}

Static members should be accessed in a static manner. (i.e. use Climber instead of climber)

CoolSpy3 · 2022-09-29T05:12:28Z

+                () -> {climber.moveMotors(Climber.MotorSpeed.extend,climber.rightMotor);},
+                (interrupted) -> {climber.stopMotors(climber.rightMotor);},
+                () -> {return climber.isMotorExtended(climber.rightMotor);}


Suggested change

() -> {climber.moveMotors(Climber.MotorSpeed.extend,climber.rightMotor);},

(interrupted) -> {climber.stopMotors(climber.rightMotor);},

() -> {return climber.isMotorExtended(climber.rightMotor);}

() -> climber.moveMotors(Climber.MotorSpeed.extend, climber.rightMotor),

(interrupted) -> climber.stopMotors(climber.rightMotor),

() -> climber.isMotorExtended(climber.rightMotor)

CoolSpy3 · 2022-09-29T05:12:56Z

+                () -> {climber.moveMotors(Climber.MotorSpeed.extend,climber.leftMotor);},
+                (interrupted) -> {climber.stopMotors(climber.leftMotor);},
+                () -> {return climber.isMotorExtended(climber.leftMotor);}


Suggested change

() -> {climber.moveMotors(Climber.MotorSpeed.extend,climber.leftMotor);},

(interrupted) -> {climber.stopMotors(climber.leftMotor);},

() -> {return climber.isMotorExtended(climber.leftMotor);}

() -> climber.moveMotors(Climber.MotorSpeed.extend, climber.leftMotor),

(interrupted) -> climber.stopMotors(climber.leftMotor),

() -> climber.isMotorExtended(climber.leftMotor)

CoolSpy3 · 2022-09-29T05:25:56Z

        left.set(0);
+        SmartDashboard.putString("Left Climber State", "Stopped");
+      }
+      if (motor>-1){


Suggested change

if (motor>-1){

if (motor!=leftMotor){

Readability

CoolSpy3 · 2022-09-29T05:26:27Z

+      if (motor<1 && leftEncoder.getPosition() < EncoderPos.extendLeft.value){
+        return false;
+      }
+      if (motor>-1 && rightEncoder.getPosition() < EncoderPos.extendRight.value){


Suggested change

if (motor<1 && leftEncoder.getPosition() < EncoderPos.extendLeft.value){

return false;

}

if (motor>-1 && rightEncoder.getPosition() < EncoderPos.extendRight.value){

if (motor!=rightMotor && leftEncoder.getPosition() < EncoderPos.extendLeft.value){

return false;

}

if (motor!=leftMotor && rightEncoder.getPosition() < EncoderPos.extendRight.value){

Readability

CoolSpy3 · 2022-09-29T05:26:54Z

+      if (motor<1 && leftEncoder.getPosition() > EncoderPos.retractLeft.value){
+        return false;
+      }
+      if (motor>-1 && rightEncoder.getPosition() > EncoderPos.retractRight.value){


Suggested change

if (motor<1 && leftEncoder.getPosition() > EncoderPos.retractLeft.value){

return false;

}

if (motor>-1 && rightEncoder.getPosition() > EncoderPos.retractRight.value){

if (motor!=rightMotor && leftEncoder.getPosition() > EncoderPos.retractLeft.value){

return false;

}

if (motor!=leftMotor && rightEncoder.getPosition() > EncoderPos.retractRight.value){

Readability

CoolSpy3 · 2022-09-29T05:33:46Z

+    private static final double kSlowDesiredRetractSpeedInps = 1;
+    private static final double kSlowDesiredExtendSpeedInps = 1;


Suggested change

private static final double kSlowDesiredRetractSpeedInps = 1;

private static final double kSlowDesiredExtendSpeedInps = 1;

private static final double kSlowDesiredRetractSpeedInps = 2;

private static final double kSlowDesiredExtendSpeedInps = 2;

These values should not be changed

CoolSpy3 · 2022-09-29T05:35:58Z

+      extendLeft(-5.317),
+      extendRight(5.315),
+      retractLeft(-1.151),
+      retractRight(-1.172),


Suggested change

extendLeft(-5.317),

extendRight(5.315),

retractLeft(-1.151),

retractRight(-1.172),

extendLeft(6.315),

extendRight(6.315),

retractLeft(-0.6),

retractRight(-0.6),

These values should match the previous encoder constants

-change EncoderPos constants -change kSlowDesired(Retract|Extend)SpeedInps

CoolSpy3

It's almost there. There are still a couple outstanding comments from the previous review in Climber.java. Take a look at those and then I'll approve.

FriedLongJohns · 2022-10-03T02:38:35Z

I think this should be it...?

Remove unnecessary imports. Revert kDesiredExtendSpeedInps to its original value Switch motor indexes to 0 1 2

CoolSpy3

I went ahead and applied my final suggestions in d8ec3c5d4e48b26f61dbe9434c1d7b6dc2d7ee23. Make sure you're okay with the changes and, if so, you can merge after the code is tested (hopefully Monday).

beansbeansbeansyes and others added 8 commits September 8, 2022 19:29

make

f35365b

Use new climber functions and stop extending ParallelCommandGroup

e7ac053

Remove Climber Reset

4a9bc0e

Use more tuples + bugfixes

5e84e12

+Motor selection tuple +getMotorPos now maps to a lambda array =Rework isMotor(Extended|Retracted) to not use reset boolean anymore and operate on a "true until false" basis

Lots of bugfixes + use constants instead of enums

26cfa7f

edit comments

5619f23

Fix ALL the bugs (it builds now)

7c5390a

-Fix enum system in Climber.java to actually work as intended by using dictionaries. -Go fix all the uses of Climber.java to also work correctly -It builds now Probably still horribly buggy

FriedLongJohns requested a review from CoolSpy3 September 19, 2022 03:31

K3das requested changes Sep 19, 2022

View reviewed changes

Comment thread src/main/java/org/team199/robot2022/subsystems/Climber.java Outdated

Comment thread src/main/java/org/team199/robot2022/subsystems/Climber.java Outdated

Comment thread src/main/java/org/team199/robot2022/subsystems/Climber.java Outdated

FriedLongJohns and others added 3 commits September 18, 2022 21:13

keep the SmartDashboard keys/values consistent to the rest of the code

b598f63

Co-authored-by: Kedas <m@yarn.network>

make more readable

95bb196

Co-authored-by: Kedas <m@yarn.network>

keep the SmartDashboard keys/values consistent to the rest of the code

77ea14d

Co-authored-by: Kedas <m@yarn.network>

FriedLongJohns requested a review from K3das September 19, 2022 04:17

CoolSpy3 requested changes Sep 19, 2022

View reviewed changes

K3das approved these changes Sep 19, 2022

View reviewed changes

FriedLongJohns added 6 commits September 21, 2022 17:25

Call static things more statically

eb54633

mmm yes what a good push title

Go back to using ParallelCommandGroup

6f254dd

Reject Humanity, return to Monke

"assign a member variable to these enums instead of using a dictionary"

c34f371

Make commands "final"

6b6c82c

Use DoubleConsumer instead of Consumer + Cast

3e73f72

Stop -> Stopped

7fe7869

FriedLongJohns changed the base branch from master to dev September 22, 2022 00:32

FriedLongJohns requested a review from CoolSpy3 September 22, 2022 00:44

FriedLongJohns added 3 commits September 23, 2022 20:26

Fix Conflicts

5d3148b

Move enums to EOF + fix conflicts (probably)

208ebf0

Merge branch 'milkybeans' into milkybeans-dev

d831001

FriedLongJohns and others added 3 commits September 23, 2022 20:46

merge conflicts

197c6aa

fix conflicts

37f258a

remove old code

852cb30

CoolSpy3 requested changes Sep 29, 2022

View reviewed changes

FriedLongJohns added 3 commits September 29, 2022 09:37

Simplify Lambdas + staticify

f64f14d

Better 'if' statements

a4ca2e7

change some constants

8322419

-change EncoderPos constants -change kSlowDesired(Retract|Extend)SpeedInps

FriedLongJohns requested a review from CoolSpy3 September 29, 2022 16:41

Make things not final ...?

e02564d

CoolSpy3 requested changes Sep 30, 2022

View reviewed changes

go make the rest of the IF statements better

20bff4c

Apply suggestions from code review

d8ec3c5

Remove unnecessary imports. Revert kDesiredExtendSpeedInps to its original value Switch motor indexes to 0 1 2

CoolSpy3 approved these changes Oct 3, 2022

View reviewed changes

	public final class ExtendClimber extends ParallelCommandGroup {
	public class ExtendClimber extends ParallelCommandGroup {

		private static final double kSlowDesiredRetractSpeedInps = 1;
		private static final double kSlowDesiredExtendSpeedInps = 1;

Uh oh!

Conversation

FriedLongJohns commented Sep 19, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

CoolSpy3 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CoolSpy3 commented Sep 19, 2022

Uh oh!

CoolSpy3 commented Sep 21, 2022 via email

Uh oh!

FriedLongJohns commented Sep 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FriedLongJohns Sep 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CoolSpy3 left a comment

Choose a reason for hiding this comment

Uh oh!

FriedLongJohns commented Oct 3, 2022

Uh oh!

CoolSpy3 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

FriedLongJohns commented Sep 22, 2022 •

edited

Loading

FriedLongJohns Sep 29, 2022 •

edited

Loading