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

Major upgrades #88

Closed
wants to merge 16 commits into from
Closed

Major upgrades #88

wants to merge 16 commits into from

Conversation

rflood07
Copy link
Collaborator

Added better odometry so it works with autos, tuned speaker shooting for the comp bot, Tuned Amp shooting, added operator controller to do climber controls and ground intake. Added ground intake, added backup subwoofer and podium angles, autos now work for 1 note (the "flee" autos)

odometer.resetPosition(new Rotation2d(), getModulePositions(), new Pose2d(getPose().getTranslation(), new Rotation2d()));
zeroGyroscope(180);
}
zeroGyroscope(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like this zero should be inside the else part of this statement... This always sets the gyroscope to 0 regardless of the alliance.

} else {
angleShooterSubsystem.setDesiredShooterAngle(angleShooterSubsystem.calculateSpeakerAngle());
if (POVSupplier.getAsDouble() == 90 || POVSupplier.getAsDouble() == 45 || POVSupplier.getAsDouble() == 135) {
angleShooterSubsystem.setDesiredShooterAngle(SmartDashboard.getNumber("amp angle", Field.AMPLIFIER_ANGLE)/*Field.AMPLIFIER_ANGLE*/);
Copy link
Contributor

Choose a reason for hiding this comment

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

This SmartDashboard.getNumber call seems dangerous... Even if there is no longer an "amp angle" field on the SmartDashboard, it would put my mind at ease if we just changed the Field.AMPLIFIER_ANGLE to be the value you liked best.

If your testing earlier discovered that there was a particular angle that worked best, we need to make sure that the Field.AMPLIFIER_ANGLE constant gets updated appropriately, instead of leaving that change in the SmartDashboard widget.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree we should change this,but to set ur mind at ease, when the Boolean is initialized on SmartDashboard (whenever robot code starts) it is set to whatever the constant is. So if you never change anything on SmartDashboard, you will just use the constant

angleShooterSubsystem.setDesiredShooterAngle(ShooterConstants.GROUND_INTAKE_SHOOTER_ANGLE);
} else {
angleShooterSubsystem.setDesiredShooterAngle(angleShooterSubsystem.calculateSpeakerAngle());
}
Copy link
Contributor

Choose a reason for hiding this comment

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

A note on this whole execute function-- There's no reason to have the if/else statements increasingly nested... It just makes the code harder to read. This functionality is equivalent to:

if(SubwooferSupplier1.getAsBoolean()) {
...
}
else if(StageAngleSupplier.getAsBoolean()) {
...
}
else if(POVSupplier.getAsDouble() ...) {
...
}
else if(humanPlayerSupplier.getAsBoolean()) {
...
}
else if(intakeSupplier.getAsBoolean()) {
...
}
else {
...
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Your right... the nesting is just how i was taught to make if/else statements nested like that, but it is harder to read

public static final double COMP_MAXIMUM_SHOOTER_ANGLE = 101;//still has to be found
public static final double PRAC_MAXIMUM_SHOOTER_ANGLE = 108;//still has to be found
public static final double COMP_MAXIMUM_SHOOTER_ANGLE = 108;//still has to be found
public static final double PRAC_MAXIMUM_SHOOTER_ANGLE = 101;//still has to be found
Copy link
Contributor

Choose a reason for hiding this comment

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

These numbers seem like they have been found-- can the comments be deleted? They seem misleading.

}
public void resetOdometry() {
odometer.resetPosition(getGyroscopeRotation(), getModulePositions(), DRIVE_ODOMETRY_ORIGIN);
resetOdometry(DRIVE_ODOMETRY_ORIGIN);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't quite understand how the odometry reset works... Is there a need for this to be different between red and blue alliances?

@rflood07 rflood07 closed this Feb 28, 2024
@rflood07
Copy link
Collaborator Author

closed becuase I updated the code after thomass comments and the pull request wouldnt update

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