Colorwheel #2

AlonSchwierz · 2021-09-03T10:45:44Z

אמאשך

src/main/java/frc/robot/subsystems/colorwheel/commands/ColorReach.java

src/main/java/frc/robot/subsystems/colorwheel/ColorWheel.java

src/main/java/frc/robot/subsystems/colorwheel/commands/RotationControl.java

adamv17

This code is really good and concise, I think this would already work on the robot.

src/main/java/frc/robot/subsystems/colorwheel/ColorWheel.java

adamv17 · 2021-09-04T04:54:26Z

src/main/java/frc/robot/subsystems/colorwheel/ColorWheel.java

+    private String lastcolor = "";
+
+    public String whatcolor() {
+        Color color = sensor.getColor();


This is the right idea using the color matcher, but you don't want the match.addColorMatch here. Instead create a constructor (פעולה בונה) and put the code there, that way it will run once and not every time you run the function.

adamv17 · 2021-09-04T04:59:42Z

src/main/java/frc/robot/subsystems/colorwheel/commands/RotationControl.java

+
+    @Override
+    public void execute() {
+        colorWheel.setpower(0.5);


This code looks very good, I would just make the 0.5 value a constant

src/main/java/frc/robot/subsystems/colorwheel/commands/RotationControl.java

adamv17 · 2021-09-04T05:03:19Z

src/main/java/frc/robot/RobotContainer.java

 /**
 * This class is where the bulk of the robot should be declared.  Since Command-based is a
 * "declarative" paradigm, very little robot logic should actually be handled in the {@link Robot}
 * periodic methods (other than the scheduler calls).  Instead, the structure of the robot
 * (including subsystems, commands, and button mappings) should be declared here.
 */
 public class RobotContainer {
+


Add buttons and implement the commands

src/main/java/frc/robot/subsystems/colorwheel/ColorWheel.java

barel13

Nice work, You just need to document the functions. And please don't write in the description of this PR אמאשך.

src/main/java/frc/robot/subsystems/colorwheel/ColorWheel.java

barel13 · 2021-09-04T14:19:09Z

src/main/java/frc/robot/subsystems/colorwheel/commands/ColorReach.java

+    public ColorReach(ColorWheel colorWheel, String requestedcolor) {
+        this.colorwheel = colorWheel;
+        this.requestedcolor = requestedcolor;
+    }


addRequirements()

barel13 · 2021-09-04T14:20:00Z

src/main/java/frc/robot/subsystems/colorwheel/commands/ColorReach.java

+        if (currentcolor.equals(requestedcolor)) {
+            return true;
+        } else return false;


Suggested change

if (currentcolor.equals(requestedcolor)) {

return true;

} else return false;

return currentcolor.equals(requestedcolor);

barel13 · 2021-09-04T14:20:29Z

src/main/java/frc/robot/subsystems/colorwheel/commands/RotationControl.java

+
+    public RotationControl(ColorWheel colorWheel) {
+        this.colorWheel = colorWheel;
+    }


addRequirements()

barel13 · 2021-09-04T14:21:11Z

src/main/java/frc/robot/subsystems/colorwheel/commands/ColorReach.java

+    @Override
+    public void execute() {
+        colorwheel.setpower(0.5);
+        currentcolor = colorwheel.whatcolor()


Suggested change

currentcolor = colorwheel.whatcolor()

currentcolor = colorwheel.whatcolor();

…ach.java

…nControl.java

katzuv

Keep up the good work! 😄

katzuv · 2021-09-12T20:54:32Z

src/main/java/frc/robot/Constants.java

@@ -16,4 +16,8 @@
 * constants are needed, to reduce verbosity.
 */
 public final class Constants {
+
+    public final class ColorWheel {
+        public static final double power = 0.5;


Constant should be written in ALL_CAPS_WITH_UNDERSCORES.

katzuv · 2021-09-12T20:54:59Z

src/main/java/frc/robot/Pot.java

@@ -0,0 +1,8 @@
+package frc.robot;
+
+public class Pot {


Did you mean "ports"?

katzuv · 2021-09-12T20:55:13Z

src/main/java/frc/robot/Pot.java

+
+public class Pot {
+    public static class ColorWheel {
+        public static final int PORT_MOTOR = 0;


"motor port" is better.

katzuv · 2021-09-12T20:55:40Z

src/main/java/frc/robot/subsystems/colorwheel/ColorWheel.java

+    VictorSPX motor = new VictorSPX(PORT_MOTOR);
+    ColorSensorV3 sensor = new ColorSensorV3(I2C.Port.kMXP);
+    ColorMatch match = new ColorMatch();


All of these should be private.

katzuv · 2021-09-12T20:56:41Z

src/main/java/frc/robot/subsystems/colorwheel/ColorWheel.java

+        match.addColorMatch(Color.kYellow); // yellow
+        match.addColorMatch(Color.kGreen); // green
+        match.addColorMatch(Color.kRed); // red
+        match.addColorMatch(Color.kBlue); // blue


The comments are redundant, if you use kYellow you don't need to comment that means the color "yellow".

katzuv · 2021-09-12T21:05:04Z

src/main/java/frc/robot/subsystems/colorwheel/commands/ColorReach.java

+
+    @Override
+    public void execute() {
+        colorWheel.setPower(0.5);


You've declared a constant with this value, why not use it? 🙂

katzuv · 2021-09-12T21:06:05Z

src/main/java/frc/robot/subsystems/colorwheel/commands/ColorReach.java

+        if (currentColor.equals(requestedColor)) {
+            return true;
+        } else return false;


This can be shortened to one line. If you don't have an idea, here's a clue: What type of value does equals() return?

katzuv · 2021-09-12T21:07:18Z

src/main/java/frc/robot/subsystems/colorwheel/commands/RotationControl.java

+
+
+    }
+
+


Suggested change

}

}

No need for all this whitespace.

katzuv · 2021-09-12T21:11:10Z

src/main/java/frc/robot/subsystems/colorwheel/commands/RotationControl.java

+            if (currentColor.equals(startingColor)) {
+                counter += 0.5;
+            }


Why are incrementing in only 0.5? In isFinished you check whether the counter >= 3. This means you'll stop only when the Control Panel reaches six full rotations, and that's too high.

In every rotation the same color appears twice.

Right, I forgot, I thought there are only four.

katzuv · 2021-09-12T21:14:20Z

src/main/java/frc/robot/subsystems/colorwheel/commands/RotationControl.java

+        if (counter >= 3) {
+            return true;
+        } else
+            return false;


This can be shortened to one line too, in the same way as I commented on above.

AlonSchwierz added 5 commits August 29, 2021 18:32

Auto stash before merge of "master" and "origin/master"

5cb554f

Merge branch 'master' into colorwheel

21f6456

Fix conflicts

ea3566d

create motor sensor, read sensor

1012f34

done

39c6457

AlonSchwierz requested review from Se-gol, adamv17 and barel13 and removed request for Se-gol and adamv17 September 3, 2021 10:45

Se-gol reviewed Sep 3, 2021

View reviewed changes

adamv17 suggested changes Sep 4, 2021

View reviewed changes

barel13 requested changes Sep 4, 2021

View reviewed changes

barel13 assigned AlonSchwierz Sep 4, 2021

Se-gol and others added 5 commits September 12, 2021 17:50

Update src/main/java/frc/robot/subsystems/colorwheel/commands/ColorRe…

3e6e192

…ach.java

Update src/main/java/frc/robot/subsystems/colorwheel/ColorWheel.java

923851d

Update src/main/java/frc/robot/subsystems/colorwheel/commands/Rotatio…

d3c4a65

…nControl.java

Sunday

de04d8a

Sunday

9976b8b

AlonSchwierz force-pushed the colorwheel branch from 5f7a259 to 9976b8b Compare September 12, 2021 16:17

documentation added

f628e1a

katzuv requested changes Sep 12, 2021

View reviewed changes

AlonSchwierz and others added 3 commits September 14, 2021 16:54

added color unknown

2943fca

Merge remote-tracking branch 'origin/colorwheel' into colorwheel

504c65a

use commands to return the seen color

ce613c7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Colorwheel #2

Colorwheel #2

AlonSchwierz commented Sep 3, 2021

adamv17 left a comment

adamv17 Sep 4, 2021

adamv17 Sep 4, 2021

adamv17 Sep 4, 2021

barel13 left a comment

barel13 Sep 4, 2021

barel13 Sep 4, 2021

barel13 Sep 4, 2021

barel13 Sep 4, 2021

katzuv left a comment

katzuv Sep 12, 2021

katzuv Sep 12, 2021

katzuv Sep 12, 2021

katzuv Sep 12, 2021

katzuv Sep 12, 2021

katzuv Sep 12, 2021

katzuv Sep 12, 2021

katzuv Sep 12, 2021

katzuv Sep 12, 2021

barel13 Sep 15, 2021

katzuv Sep 23, 2021

katzuv Sep 12, 2021

	currentcolor = colorwheel.whatcolor()
	currentcolor = colorwheel.whatcolor();

Colorwheel #2

Are you sure you want to change the base?

Colorwheel #2

Conversation

AlonSchwierz commented Sep 3, 2021

adamv17 left a 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

barel13 left a 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

katzuv left a 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

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