Intake #1

GaiaZano05 · 2021-09-03T08:48:31Z

No description provided.

barel13 · 2021-09-03T09:50:07Z

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

@@ -16,4 +16,7 @@
 * constants are needed, to reduce verbosity.
 */
 public final class Constants {
+    public static final class Intake {
+        public static final double power = 0.8; // [%]


CAPITALIZE

Suggested change

public static final double power = 0.8; // [%]

public static final double POWER = 0.8; // [%]

barel13 · 2021-09-03T09:50:46Z

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

+  public XboxController Xbox = new XboxController(1);
+  public JoystickButton a = new JoystickButton(Xbox, XboxController.Button.kA.value);
+  public JoystickButton b = new JoystickButton(Xbox, XboxController.Button.kB.value);
+  private Intake intake;


Suggested change

private Intake intake;

private final Intake intake = new Intake();

barel13 · 2021-09-03T09:51:25Z

src/main/java/frc/robot/subsystems/intake/Intake.java

+/**
+ * Add&named motors in Intake.
+ */


Add proper documentation!!! (everywhere)

barel13 · 2021-09-03T09:51:49Z

src/main/java/frc/robot/subsystems/intake/Intake.java

+    /**
+     * Add motor inverted.
+     */
+    public void TalonSRXSelenoid() {


Suggested change

public void TalonSRXSelenoid() {

public Intake() {

barel13 · 2021-09-03T09:52:23Z

src/main/java/frc/robot/subsystems/intake/Intake.java

+
+    /**
+     * Set power for motor.
+     * @param power the speed times the torque.


Wrong units, it's just percentage

barel13 · 2021-09-03T09:53:02Z

src/main/java/frc/robot/subsystems/intake/Intake.java

+
+    /**
+     * A check to see if piston is open or close.
+     * @return


You should write what true mean (does it mean open or closed?)

A shorter solution will be renaming the function isPistonOpen or something similar.

barel13 · 2021-09-03T10:03:43Z

src/main/java/frc/robot/subsystems/intake/Ports.java

@@ -0,0 +1,14 @@
+package frc.robot.subsystems.intake;


Ports should be in frc.robot package!!!!!!!!!!!!

No need to shout 🙂

IMO ports and constants should be stored in every subsystem's packages except global stuff like we did in 2019. When they are all in the same file you have a lot of merge conflicts.

adamv17

Great job, there is really very little left to improve in this code. :)

adamv17 · 2021-09-04T04:36:31Z

src/main/java/frc/robot/subsystems/intake/Intake.java

+     * Set power for motor.
+     * @param power the speed times the torque.
+     */
+    public void setMotorPower(double power) {


Rename to setPower

I disagree, it might not be the proper naming, maybe powerWheels(...)?

Sure it's just repetitive to call the method setMotorPower

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

src/main/java/frc/robot/subsystems/intake/Intake.java

+     * Add motor inverted.
+     */
+    public void TalonSRXSelenoid() {
+        this.motor.setInverted(Ports.Intake.IS_MOTOR_INVERTED);


"this" isn't necessary here

adamv17 · 2021-09-04T04:39:07Z

src/main/java/frc/robot/subsystems/intake/commands/Commandy.java

+/**
+ * Add Commandy function;
+ */
+public class Commandy extends CommandBase {


Add a command that only opens and closes the intake

Agreed, and add another command that powers the wheels. Then, if needed, combine both commands in a command group.
Also, change the command's name because "commandy" is very not indicative.

katzuv

Great job! 😁

katzuv · 2021-09-11T10:39:10Z

src/main/java/frc/robot/subsystems/intake/Ports.java

@@ -0,0 +1,14 @@
+package frc.robot.subsystems.intake;


No need to shout 🙂

IMO ports and constants should be stored in every subsystem's packages except global stuff like we did in 2019. When they are all in the same file you have a lot of merge conflicts.

katzuv · 2021-09-11T10:41:37Z

src/main/java/frc/robot/subsystems/intake/commands/Commandy.java

+/**
+ * Add Commandy function;
+ */
+public class Commandy extends CommandBase {


Agreed, and add another command that powers the wheels. Then, if needed, combine both commands in a command group.
Also, change the command's name because "commandy" is very not indicative.

katzuv · 2021-09-11T10:41:58Z

src/main/java/frc/robot/subsystems/intake/Intake.java

+
+
+


No need for all those blank lines.

katzuv · 2021-09-11T10:42:59Z

src/main/java/frc/robot/subsystems/intake/Intake.java

+        if (isPistonEngaged()) {
+            setPistonMode(false);
+        } else {
+            setPistonMode(true);
+        }


You can shorten this code:

Suggested change

if (isPistonEngaged()) {

setPistonMode(false);

} else {

setPistonMode(true);

}

setPistonMode(!isPistonEngaged());

katzuv · 2021-09-11T10:44:00Z

src/main/java/frc/robot/subsystems/intake/Intake.java

+
+    /**
+     * A check to see if piston is open or close.
+     * @return


A shorter solution will be renaming the function isPistonOpen or something similar.

katzuv · 2021-09-11T12:44:20Z

src/main/java/frc/robot/subsystems/intake/Intake.java

+ */
+public class Intake extends SubsystemBase {
+    private TalonSRX motor = new TalonSRX(Ports.Intake.MOTOR);
+    private Solenoid piston = new Solenoid(Ports.Intake.SOLENOID);


Better to call it "retractor" or something similar, to explain what the piston's purpose is. Make sure you change the name everywhere, for example, change every function name that has "piston" inside it.
Also, "piston" is a hardware component, making it an implementation detail. Thus, users of the subsystem (i.e. commands) do not care about it, they care only about what the subsystem does, not about how it does that. Therefore, "piston" shouldn't be exposed outside of this class. This is also why you declared it private.
Please let me know if the explanation wasn't clear.

katzuv · 2021-09-12T20:29:05Z

src/main/java/frc/robot/Intake.java

+ */
+public class Intake extends SubsystemBase {
+    private TalonSRX motor = new TalonSRX(Ports.Intake.MOTOR);
+    private Solenoid piston = new Solenoid(Ports.Intake.SOLENOID);


Better to call this "retractor" or something similar, so that it's clear what is the piston's purpose.

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

src/main/java/frc/robot/Intake.java

+     * A check to see if piston is open or close, true= open, false= close.
+     * @return
+     */
+    public boolean isPistonEngaged() {


As written above, it's better to name this function isRetracted or something similar. That way, the function's purpose is clear. Make sure you do the renaming everywhere else appropriate in this class.
Also, the piston is a hardware component, which means it is an implementation detail. Thus, it shouldn't be exposed to external users of the class, such as commands. Users of the class care about what it does, not how it does it.
Please reach out if the explanation above is not clear 🙂

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

src/main/java/frc/robot/Intake.java

+
+    }
+
+}
+
+
+
+


Suggested change

}

}

}

}

No need for all these blank lines 🙂

katzuv · 2021-09-12T20:47:00Z

src/main/java/frc/robot/subsystems/intake/commands/Commandy.java

+
+    @Override
+    public void initialize() {
+        super.initialize();


No need for this call to super's methods and also the ones in the methods following. As you can see in WPILib's source code, these methods are empty in the parent class, so these calls do nothing (source code taken from here):

public interface Command { /** The initial subroutine of a command. Called once when the command is initially scheduled. */ default void initialize() {} /** The main body of a command. Called repeatedly while the command is scheduled. */ default void execute() {} /** * The action to take when the command ends. Called when either the command finishes normally, or * when it interrupted/canceled. * * <p>Do not schedule commands here that share requirements with this command. Use {@link * #andThen(Command...)} instead. * * @param interrupted whether the command was interrupted/canceled */ default void end(boolean interrupted) {} /** * Whether the command has finished. Once a command finishes, the scheduler will call its end() * method and un-schedule it. * * @return whether the command has finished. */ default boolean isFinished() { return false; }

katzuv · 2021-09-12T21:21:27Z

Just a small comment – every commit should document a single change. so 42689a0 should have been split into four different commits.

barel13 · 2021-09-15T13:58:52Z

src/main/java/frc/robot/Intake.java

+
+    /**
+     * Set percentage for motor.
+     * @param power the speed times the torque.


The units are simply %

barel13 · 2021-09-15T14:00:12Z

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

+  public XboxController Xbox = new XboxController(1);
+  public JoystickButton a = new JoystickButton(Xbox, XboxController.Button.kA.value);
+  public JoystickButton b = new JoystickButton(Xbox, XboxController.Button.kB.value);


(Also, make them private)

barel13 · 2021-09-15T14:01:50Z

src/main/java/frc/robot/subsystems/intake/commands/ToggleIntake.java

+
+    private Intake intake;
+
+    public ToggleIntake(Intake intake) {


The should also move to specific state, not only toggle.

GaiaZano05 added 5 commits August 29, 2021 17:58

Create Intake subsystem

e8dda8f

Add boolean is engaged

49433ad

Add Commandy function

5fda56e

Add documentation

759f0ff

Add Intake class to Constants, add set power, add set Piston mode false.

42689a0

GaiaZano05 requested review from adamv17, barel13 and Se-gol September 3, 2021 08:48

GaiaZano05 self-assigned this Sep 3, 2021

Merge branch 'master' into intake

e9afee6

barel13 requested changes Sep 3, 2021

View reviewed changes

barel13 reviewed Sep 3, 2021

View reviewed changes

Merge branch 'master' into intake

377e96c

adamv17 suggested changes Sep 4, 2021

View reviewed changes

GaiaZano05 added 3 commits September 10, 2021 20:46

Capitalize POWER, Add documentation, write what true mean, Rename setMot

af850d7

Add Intake function

207419e

Add toggle funcrion to ToggleIntake, fix merge

b07a4bb

katzuv requested changes Sep 12, 2021

View reviewed changes

katzuv mentioned this pull request Sep 12, 2021

Funnel #3

Open

barel13 requested changes Sep 15, 2021

View reviewed changes

Make ready for use

d16f0e3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Intake #1

Intake #1

GaiaZano05 commented Sep 3, 2021

barel13 Sep 3, 2021

barel13 Sep 3, 2021

barel13 Sep 3, 2021

barel13 Sep 3, 2021

barel13 Sep 3, 2021

barel13 Sep 3, 2021 •

edited

Loading

katzuv Sep 11, 2021

barel13 Sep 3, 2021 •

edited

Loading

Se-gol Sep 3, 2021

katzuv Sep 11, 2021 •

edited

Loading

adamv17 left a comment

adamv17 Sep 4, 2021

barel13 Sep 4, 2021

adamv17 Sep 4, 2021

adamv17 Sep 4, 2021

adamv17 Sep 4, 2021

katzuv Sep 11, 2021

katzuv left a comment

katzuv Sep 11, 2021 •

edited

Loading

katzuv Sep 11, 2021

katzuv Sep 11, 2021

katzuv Sep 11, 2021

katzuv Sep 11, 2021

katzuv Sep 11, 2021 •

edited

Loading

katzuv Sep 12, 2021

katzuv Sep 12, 2021

katzuv Sep 12, 2021

katzuv Sep 12, 2021 •

edited

Loading

katzuv commented Sep 12, 2021 •

edited

Loading

barel13 Sep 15, 2021

barel13 Sep 15, 2021

barel13 Sep 15, 2021

	public static final double power = 0.8; // [%]
	public static final double POWER = 0.8; // [%]

	private Intake intake;
	private final Intake intake = new Intake();

-    }
-}
+    }
+}

Intake #1

Are you sure you want to change the base?

Intake #1

Conversation

GaiaZano05 commented Sep 3, 2021

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

barel13 Sep 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barel13 Sep 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katzuv Sep 11, 2021 • edited Loading

Choose a reason for hiding this comment

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

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

katzuv Sep 11, 2021 • 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

katzuv Sep 11, 2021 • 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

katzuv Sep 12, 2021 • edited Loading

Choose a reason for hiding this comment

katzuv commented Sep 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barel13 Sep 3, 2021 •

edited

Loading

barel13 Sep 3, 2021 •

edited

Loading

katzuv Sep 11, 2021 •

edited

Loading

katzuv Sep 11, 2021 •

edited

Loading

katzuv Sep 11, 2021 •

edited

Loading

katzuv Sep 12, 2021 •

edited

Loading

katzuv commented Sep 12, 2021 •

edited

Loading