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

Arm Brake & 2-Note Auto with Simulated Note #110

Merged
merged 8 commits into from
Feb 21, 2024
Merged

Conversation

JohnGilb
Copy link
Contributor

@JohnGilb JohnGilb commented Feb 20, 2024

Why are we doing this?

Add some manual brake control, and disabled automatic brake control, add 2-note auto
Asana task URL:

Whats changing?

  • Brake can be manually engaged/disabled via joysticks
  • Automatic brake that engages when Arm PID is satisfied
  • 2 note auto with simulated note launch

Questions/notes for reviewers

How this was tested

  • unit tests added
  • tested on robot
  • tested in simulator

@JohnGilb JohnGilb requested a review from a team as a code owner February 20, 2024 03:58
@JohnGilb JohnGilb changed the title Feature/arm upgrades Arm Brake & 2-Note Auto with Simulated Note Feb 20, 2024
Copy link
Contributor

@aschokking aschokking left a comment

Choose a reason for hiding this comment

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

Overall looks good. Next time if it's not too much overhead though please consider breaking this up into the ~3 kinda unrelated changes it is. Would make it easier to review and understand the git history.

// Drive to the middle note while collecting
queueMessageToAutoSelector("Drive to middle note");
var driveToMiddleSpike = swerveProvider.get();
driveToMiddleSpike.logic.setEnableConstantVelocity(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

⭐ I think these longer command group constructors start getting hard to read. What do you think about logic like all this stuff for setting up a particular command ending up in a function so the top level constructor is just "do these 5 commands in this sequence"

stopDrive));
}

private void queueMessageToAutoSelector(String message) {
Copy link
Contributor

Choose a reason for hiding this comment

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

⭐ fun idea! But maybe something that should live on a base class?

if (dynamicBrakingEnabled) {
// Engage the brake so we don't backdrive away from this point
// (vibrations from the shooter can cause that)
arm.setBrakeEnabled(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

⭐ just out of curiosity, why not manage the break inside the subsystem based on the power that's sent, how would it be different to disengage with non-zero power and engage with zero-ish power?

@aschokking aschokking merged commit f0be136 into main Feb 21, 2024
1 check passed
@aschokking aschokking deleted the feature/ArmUpgrades branch February 21, 2024 04:09
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