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

Robot shooter #39

Merged
merged 9 commits into from
Sep 16, 2024
Merged

Robot shooter #39

merged 9 commits into from
Sep 16, 2024

Conversation

avidraccoon
Copy link
Contributor

No description provided.

@avidraccoon avidraccoon requested a review from jkleiber August 26, 2024 22:44
Copy link
Member

@jkleiber jkleiber left a comment

Choose a reason for hiding this comment

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

First pass review of the shooter

src/main/java/frc/robot/RobotContainer.java Outdated Show resolved Hide resolved
src/main/java/frc/robot/Robot.java Outdated Show resolved Hide resolved
src/main/java/frc/robot/Robot.java Outdated Show resolved Hide resolved
src/main/java/frc/robot/Robot.java Outdated Show resolved Hide resolved
@avidraccoon
Copy link
Contributor Author

I'm having problems with solving the conflicting files can you help me?

@jkleiber
Copy link
Member

jkleiber commented Sep 7, 2024

@avidraccoon what I do when I see merge conflicts is try to preserve the code that is in conflict with my code while still making sure my feature works. In this case it probably makes sense to "Accept Both" changes when doing the rebase and then refactor the code as needed to make things work.

The rebase process can be found here: https://team401.github.io/Docs/Programming/Software-Workflow.html#keep-branch-updated

If there's a specific problem I can help with just let me know!

Copy link
Member

@jkleiber jkleiber left a comment

Choose a reason for hiding this comment

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

Second pass, this looks pretty clean now. We just need to get the merge conflicts resolved and then it'll be ready to merge.

After we merge this, we need to get the hardware IO made so we can test on the robot. Can you create a ticket to do this if you haven't already?

Comment on lines 8 to 11
// Move to constants later
double intakeTargetRPM = -100.0;
double shootingTargetRPM = 300.0;
//
Copy link
Member

Choose a reason for hiding this comment

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

Let's move these to constants

We can store constants in src/main/java/frc/robot/constants and eventually JSONify everything

public class ShooterIntakeIOSim implements ShooterIntakeIO {

FlywheelSim flywheelSim = new FlywheelSim(DCMotor.getCIM(2), 1.0, 1.0);
PIDController controller = new PIDController(0.75, 0.1, 0.0);
Copy link
Member

Choose a reason for hiding this comment

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

If these are going to be the control gains, we should move them to constants as well

@avidraccoon
Copy link
Contributor Author

avidraccoon commented Sep 12, 2024

I moved the constants and prepared for merge but now it can't build because it can find classes for import, how do I fix this?

Edit: It can build but simulation will not run.

Edit 2: Fixed it just needed to clear the build cache

Copy link
Member

@jkleiber jkleiber left a comment

Choose a reason for hiding this comment

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

There's some refactoring to do here, but let's do that in a separate issue. Approved - please merge when you are able

@avidraccoon avidraccoon merged commit 92fb160 into main Sep 16, 2024
3 checks 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.

2 participants