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

Elevator #53

Merged
merged 10 commits into from
Mar 30, 2024
Merged

Elevator #53

merged 10 commits into from
Mar 30, 2024

Conversation

penguin212
Copy link
Contributor

#36

Copy link
Contributor Author

@penguin212 penguin212 left a comment

Choose a reason for hiding this comment

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

@6kn4eakfr4s Look at comments please. Just a couple changes.


@Override
public void initialize() {
elevatorSubsystem.setMotorPower(0.2);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should probably be -.2?

Copy link
Member

@6kn4eakfr4s 6kn4eakfr4s Mar 29, 2024

Choose a reason for hiding this comment

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

I think someone told me that the elevator motor's inversed, which means to go down u need positive power. We can just run a quick test about that.

Copy link
Member

Choose a reason for hiding this comment

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

Changed this to -0.2 for now.

import frc.robot.subsystems.elevator.ElevatorSubsystem;

/** Sends the elevator to position zero. */
public class ElevatorToLimitSwitchCommand extends Command {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you also combine this to be a part of the toGround command? Maybe you can just make a command that is a sequence of first doing the toGround and then this command and we will use that to lower the elevator.

Copy link
Member

Choose a reason for hiding this comment

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

So first do toGround command and then if it doesn't touch the limit switch I keep lowering it?

@@ -45,7 +49,7 @@ public ElevatorSubsystem() {

extensionMotor = new CANSparkMax(ElevatorConstants.EXTENSION_ID, MotorType.kBrushless);
extensionMotor.setIdleMode(IdleMode.kBrake);
extensionMotor.setInverted(false);
extensionMotor.setInverted(true);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

be careful changing this, I think it should still be false

Copy link
Member

@6kn4eakfr4s 6kn4eakfr4s Mar 29, 2024

Choose a reason for hiding this comment

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

Changed to false according to Greg


if (ElevatorConstants.LIMIT_SWITCH_ENABLED) {
if (zeroLimitSwitch != null && zeroLimitSwitch.get()) { //if limit switch is not pressed
if (Math.abs(this.getExtensionPercent()) < ElevatorConstants.EXTENSION_TOLERANCE) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

nested ifs can be ands

Copy link
Member

@6kn4eakfr4s 6kn4eakfr4s Mar 29, 2024

Choose a reason for hiding this comment

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

Removed this part since we only need to do this in toLimitSwitch command

if (zeroLimitSwitch != null && zeroLimitSwitch.get()) { //if limit switch is not pressed
if (Math.abs(this.getExtensionPercent()) < ElevatorConstants.EXTENSION_TOLERANCE) {
//if the encoder thinks the elevator is at ground
CommandScheduler.getInstance().schedule(new ElevatorToLimitSwitchCommand(this));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think this will work as you expect. Image we are trying to go to the amp height and it begins raising. If it is still within the tolerance this will override the raising and cancel going to the amp.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also this is probably unnecessary. We only really need to check that it hit the limit switch when we are actively lowering it. It won't raise on its own.

Copy link
Member

@6kn4eakfr4s 6kn4eakfr4s Mar 29, 2024

Choose a reason for hiding this comment

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

Deleted this part

if (zeroLimitSwitch != null && zeroLimitSwitch.get()) {
return true;
if (ElevatorConstants.LIMIT_SWITCH_ENABLED) {
if (zeroLimitSwitch != null && zeroLimitSwitch.get()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

you can just return the if condition

Copy link
Member

Choose a reason for hiding this comment

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

Sure

if (zeroLimitSwitch != null) {
return !zeroLimitSwitch.get();
} else {
throw new Error("Can't find limit switch!");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

probably don't do this because it will crash the code if this is called when the limit switch is unplugged. For example, if the limit switch gets unplugged in a match, the robot code will crash when this is called. You can print something and then the best thing to would be disabling the code that uses the limit switch. (if no limit switch is detected, fallback on the current code that doesn't use it).

Copy link
Member

@6kn4eakfr4s 6kn4eakfr4s Mar 29, 2024

Choose a reason for hiding this comment

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

I kept this as it allows me to see if we have the limit switch plugged in by tring and catching errors.

@MysticalApple MysticalApple merged commit a408abb into main Mar 30, 2024
1 check passed
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.

3 participants