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

Omer elevator #7

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Omer elevator #7

wants to merge 2 commits into from

Conversation

omeryakoby
Copy link

omer elevator

Copy link
Member

@katzuv katzuv left a comment

Choose a reason for hiding this comment

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

  • There is some lack of documentation in the code, make sure to complete it.
  • There is a bunch of code that was deleted in this branch, i.e. gripper
    code. Why is that?
    • Also, there is diff in this branch that is unrelated to it, i.e. changes to Gradle files. These changes aren't related to the elevator, so they should be made in another branch.
  • There are a few unused imports in the code, delete them. Also, run Ctrl+Alt+L on your code (which will also optimize your imports 🙃).

Good luck! I'm here for any questions 🙂

public static final double kI = 0;
public static final double kD = 0;
public static final double kF = 0;
public static final double RADIUS = 0.66;
Copy link
Member

Choose a reason for hiding this comment

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

Radius of what?

Comment on lines -4 to +5
public static class Gripper {
public static final int LEFTMOTOR = 0;
public static class Elevator {
public static final int motor = 0;
Copy link
Member

Choose a reason for hiding this comment

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

Why did you delete gripper constants?

private final UnitModel unitModel = new UnitModel(Constants.Elevator.TICKS_PER_METER);
private static Elevator INSTANCE = null;
private Elevator(){
motor.setInverted(TalonFXInvertType.Clockwise);
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't that be a constant?

public static final double kI = 0;
public static final double kD = 0;
public static final double kF = 0;
public static final double RADIUS = 0.66;
Copy link
Member

Choose a reason for hiding this comment

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

Radius of what?

Comment on lines -4 to +5
public static class Gripper {
public static final int LEFTMOTOR = 0;
public static class Elevator {
public static final int motor = 0;
Copy link
Member

Choose a reason for hiding this comment

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

Why did you delete gripper constants?

}

public double getheight(){
return unitModel.toUnits(motor.getSensorCollection().getIntegratedSensorPosition());
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 simply use getSelectedSensorPosition():

Suggested change
return unitModel.toUnits(motor.getSensorCollection().getIntegratedSensorPosition());
return unitModel.toUnits(motor.getSelectedSensorPosition());

import edu.wpi.first.wpilibj2.command.CommandBase;
import frc.robot.subsystems.elevator.Elevator;

public class Height extends CommandBase {
Copy link
Member

Choose a reason for hiding this comment

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

Command names need to start with a verb.

Comment on lines +26 to +30
@Override
public boolean isFinished() {
return Math.abs( elevator.getheight() - height);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

A few unrelated notes:

  1. isFinished() need to return a boolean, but Math.abs returns a double in this case.
  2. TalonFX.getClosedLoopError() can quite help you here 🙂.
  3. isFinished() should come before end().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants