Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Intake #1

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
3 changes: 3 additions & 0 deletions src/main/java/frc/robot/Constants.java
Original file line number Diff line number Diff line change
Expand Up @@ -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; // [%]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CAPITALIZE

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

}
}
12 changes: 11 additions & 1 deletion src/main/java/frc/robot/RobotContainer.java
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,23 @@
import edu.wpi.first.wpilibj.GenericHID;
import edu.wpi.first.wpilibj.XboxController;
import edu.wpi.first.wpilibj2.command.Command;
import frc.robot.subsystems.ExampleSubsystem.ExampleSubsystem;
import edu.wpi.first.wpilibj2.command.button.JoystickButton;
import frc.robot.subsystems.intake.Intake;
import frc.robot.subsystems.intake.commands.Commandy;


/**
* 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 {
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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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);
public final XboxController Xbox = new XboxController(1);
public final JoystickButton a = new JoystickButton(Xbox, XboxController.Button.kA.value);
public final JoystickButton b = new JoystickButton(Xbox, XboxController.Button.kB.value);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Also, make them private)

private Intake intake;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// The robot's subsystems and commands are defined here...


Expand All @@ -37,6 +46,7 @@ public RobotContainer() {
* {@link edu.wpi.first.wpilibj2.command.button.JoystickButton}.
*/
private void configureButtonBindings() {
a.whileHeld(new Commandy(intake, Constants.Intake.power));
}


Expand Down
62 changes: 62 additions & 0 deletions src/main/java/frc/robot/subsystems/intake/Intake.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
package frc.robot.subsystems.intake;

import com.ctre.phoenix.motorcontrol.ControlMode;
import com.ctre.phoenix.motorcontrol.can.TalonSRX;
import edu.wpi.first.wpilibj.Solenoid;
import edu.wpi.first.wpilibj2.command.SubsystemBase;

/**
* Add&named motors in Intake.
*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add proper documentation!!! (everywhere)

public class Intake extends SubsystemBase {
private TalonSRX motor = new TalonSRX(Ports.Intake.MOTOR);
private Solenoid piston = new Solenoid(Ports.Intake.SOLENOID);
Copy link
Member

@katzuv katzuv Sep 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


/**
* Add motor inverted.
*/
public void TalonSRXSelenoid() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public void TalonSRXSelenoid() {
public Intake() {

this.motor.setInverted(Ports.Intake.IS_MOTOR_INVERTED);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"this" isn't necessary here

}

/**
* Set power for motor.
* @param power the speed times the torque.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong units, it's just percentage

*/
public void setMotorPower(double power) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename to setPower

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure it's just repetitive to call the method setMotorPower

motor.set(ControlMode.PercentOutput, power);
}

/**
* A check to see if piston is open or close.
* @return
Copy link
Member

@barel13 barel13 Sep 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

*/
public boolean isPistonEngaged() {
return piston.get();
}

/**
* Set piston position.
* @param engaged
*/
public void setPistonMode(boolean engaged) {
piston.set(engaged);
}

/**
* Change piston position.
*/
public void toggle() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add the component name to the function, i.e. toggleRetractor.

if (isPistonEngaged()) {
setPistonMode(false);
} else {
setPistonMode(true);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can shorten this code:

Suggested change
if (isPistonEngaged()) {
setPistonMode(false);
} else {
setPistonMode(true);
}
setPistonMode(!isPistonEngaged());


}

}




Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for all those blank lines.

14 changes: 14 additions & 0 deletions src/main/java/frc/robot/subsystems/intake/Ports.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
package frc.robot.subsystems.intake;
Copy link
Member

@barel13 barel13 Sep 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤢🤮

Copy link
Member

@katzuv katzuv Sep 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. No need to shout 🙂
  2. 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.



/**
* Data of MOTOR & SOLENOID power.
* Data of is MOTOR inverted.
*/
public class Ports {
public static class Intake {
public static final int MOTOR = 0;
public static final int SOLENOID = 0;
public static final boolean IS_MOTOR_INVERTED = true;
}
}
51 changes: 51 additions & 0 deletions src/main/java/frc/robot/subsystems/intake/commands/Commandy.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
package frc.robot.subsystems.intake.commands;

import edu.wpi.first.wpilibj2.command.CommandBase;
import frc.robot.subsystems.intake.Intake;

/**
* Add Commandy function;
*/
public class Commandy extends CommandBase {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a command that only opens and closes the intake

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

private final Intake intake;
private final double power;

/**
* Add Requirements
*
* @param intake name to Intake.
*/
public Commandy(Intake intake, double power) {
this.intake = intake;
this.power = power;
addRequirements(intake);

}


@Override
public void initialize() {
super.initialize();
Copy link
Member

@katzuv katzuv Sep 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
  }

intake.setPistonMode(false);
}


@Override
public void execute() {
super.execute();
intake.setMotorPower(power);
}

@Override
public boolean isFinished() {
return super.isFinished();
}


@Override
public void end(boolean interrupted) {
super.end(interrupted);
intake.setMotorPower(0);
intake.setPistonMode(true);
}
}