-
Notifications
You must be signed in to change notification settings - Fork 0
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
carson and max c #77
carson and max c #77
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are more things here but here are some stuffs to look over.
src/main/java/frc/robot/subsystems/shooter/FlywheelConstants.java
Outdated
Show resolved
Hide resolved
src/main/java/frc/robot/subsystems/shooter/FlywheelIOTalonFX.java
Outdated
Show resolved
Hide resolved
ShooterConstants.AUTO_LINEUP_ROTATION_I, | ||
ShooterConstants.AUTO_LINEUP_ROTATION_D, | ||
ShooterConstants.AUTO_LINEUP_ROTATION_CONSTRAINTS); | ||
FlywheelConstants.AUTO_LINEUP_ROTATION_P, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why did we rename this? I think ShooterConstants
was more accurate, especially for this case. The auto lineup stuff doesn't have anything to do with specifically the flywheel
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it was renamed to match the subsystem name. imo auto lineup constants should actually go in trajectory constants bc the shooter isn't the thing thats lining up w/ the amp, the swerve is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah we should move it to trajectory constants, also consider renaming trajectory constants to auto constants
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like im right so Im keeping it as flywheel
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bro did not read the conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bro did not read the conversation
src/main/java/frc/robot/subsystems/shooter/FlywheelIOTalonFX.java
Outdated
Show resolved
Hide resolved
its been so long i forgot there was a pr open for this 💀 |
@mychenezpz please make the changes requested in the pull request |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this * 60.0 should be a constant, you can put it in HardwareConstants or something
OK sure
ShooterConstants.AUTO_LINEUP_ROTATION_I, | ||
ShooterConstants.AUTO_LINEUP_ROTATION_D, | ||
ShooterConstants.AUTO_LINEUP_ROTATION_CONSTRAINTS); | ||
FlywheelConstants.AUTO_LINEUP_ROTATION_P, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like im right so Im keeping it as flywheel
No description provided.