Skip to content

rumble modifications as per kenta's request#135

Open
codingduck106 wants to merge 8 commits into
mainfrom
jazl/rumble-modifs
Open

rumble modifications as per kenta's request#135
codingduck106 wants to merge 8 commits into
mainfrom
jazl/rumble-modifs

Conversation

@codingduck106

Copy link
Copy Markdown
Contributor

The teleopPeriodic in Robot.java was modified as per kenta's request to have a different type of rumble depending on the shift ending.

When an active shift is about to end, at the five second mark, the controller rumbles for one second.
When an inactive shift is about to end, the controller rumbles for the last 2 seconds of the shift.

I also added a link to the Controls Documentation in configureButtonBindings in RobotContainer.java

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Updates driver/operator controller rumble behavior in teleopPeriodic() to differentiate alerts for active vs inactive hub shifts, and adds a link to the controls documentation in the button binding Javadoc.

Changes:

  • Adjust rumble timing logic in Robot.teleopPeriodic() for active/inactive shift end warnings.
  • Add a “Controls Documentation” link in RobotContainer.configureButtonBindings() Javadoc.
  • Reformat navgrid.json into a single-line JSON file (appears unrelated to the PR description).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
src/main/java/com/team2813/RobotContainer.java Adds controls documentation link; introduces an unused import that breaks compilation.
src/main/java/com/team2813/Robot.java Replaces prior end-of-phase rumble behavior with new active/inactive shift rumble windows.
src/main/deploy/pathplanner/navgrid.json Large formatting-only change (minified), likely conflicting with repo JSON formatting rules.
Comments suppressed due to low confidence (1)

src/main/java/com/team2813/RobotContainer.java:43

  • Unused import edu.wpi.first.units.measure.Time causes a Java compilation failure (and also violates Spotless removeUnusedImports()). Please remove the import or use it.
import edu.wpi.first.wpilibj.DriverStation;

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/main/java/com/team2813/RobotContainer.java
Comment thread src/main/java/com/team2813/Robot.java
Comment thread src/main/java/com/team2813/Robot.java Outdated
@codingduck106

Copy link
Copy Markdown
Contributor Author

@spderman3333 @anonymes-axolotole please review

@spderman3333 spderman3333 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks okay, though you should consider moving the rumble logic to util/HubStatus, as it would be a cleaner fit there. I will approve this PR if you acknowledge this potential change.

Comment thread src/main/java/com/team2813/Robot.java Outdated
Comment thread src/main/java/com/team2813/Robot.java Outdated
Comment thread src/main/java/com/team2813/Robot.java Outdated
robotContainer.setRumbleOperator();
} else {
robotContainer.stopRumble();
// TODO: mayhaps add a unit test to cover this(?)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If a unit test is to be added, then it likely should be done in this PR, as it will most likely be forgotten otherwise. Plus abstracting out the rumble logic may be more trouble than it is worth for something that is easily testable in sim, and remains relatively static.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We technically shouldn't need to extract out any logic for testing rumble stuff.
To make the timeLeftInCurrentPhase what we want it to be, we can set the match time with DriverStationSim. Then, to verify if rumble is in the state we want, we can use XboxControllerSim in the test to get the value of the rumble. So, all we really need to do is run this method on a new Robot instance1.

Footnotes

  1. Even better, we could create a handleRumble() method with the current contents of teleopPeriodic(), so that we can still independently test the rumble stuff, and making it more clear what should happen when in teleopPeriodic(). We could also make the handleRumble() method take in time left in the current phase, which would simplify testing, and allow us to still only need one query of HubStatusUtil#timeLeftInCurrentPhase() if we add more things relying on it in robotPeriodic()

@spderman3333 spderman3333 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Run spotless please

@cuttestkittensrule cuttestkittensrule left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good (assuming this is in fact the behavior Kenta wants)

Comment thread src/main/java/com/team2813/util/HubStatusUtil.java

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Like how there is a unit test!

Co-authored-by: cuttestkittensrule <mangoiscute95@gmail.com>
@spderman3333

Copy link
Copy Markdown
Member

@codingduck106, please see my comment.

@cuttestkittensrule

Copy link
Copy Markdown
Contributor

@codingduck106, please see my comment.

Which comment are you referring to, @spderman3333?

@spderman3333

Copy link
Copy Markdown
Member

The comment where i asked him to respond

@cuttestkittensrule

Copy link
Copy Markdown
Contributor

Looks okay, though you should consider moving the rumble logic to util/HubStatus, as it would be a cleaner fit there. I will approve this PR if you acknowledge this potential change.

@spderman3333 If you are referring to this message, than I'm pretty sure @codingduck106 already moved the rumble logic to HubStatusUtil.

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.

4 participants