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

Arms_Deployable #3

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Arms_Deployable #3

wants to merge 6 commits into from

Conversation

ARMS300
Copy link
Contributor

@ARMS300 ARMS300 commented Feb 2, 2023

Pull request template

Please fill out the fields below by replacing their explaination text.


Subsystem: added Deployable Subsystem

Description: Sets the Manipulator D-Pad down to toggle the deployabe intake and rev

Describe your changes in a few sentences.

Tested: No

Checking that the code has built locally is important, but that is not testing.
If yes, please elaborate.

Collaborators: List anyone else who worked on this PR, preferably with @s


Before submitting:

  • Review that your code meets our coding standards
  • Confirm there is a green ✔️ (not a red ❌) next to your last commit's date
  • Replace all text above with appropriate descriptions
  • Remove header and footer from this template
  • Add @smccrorie and @fruzyna as reviewers

@ARMS300 ARMS300 requested review from fruzyna and smccrorie February 2, 2023 00:22
Copy link
Member

@fruzyna fruzyna left a comment

Choose a reason for hiding this comment

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

Looks pretty good, just a couple tweaks

import org.wildstang.year2023.robot.WSInputs;
import org.wildstang.year2023.robot.WSOutputs;

public class Deployable implements Subsystem {
Copy link
Member

Choose a reason for hiding this comment

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

Lets call this Intake to be more clear

// TODO Auto-generated method stub

if (source == deployRevIntake){
intakeCall = !(intakeCall);
Copy link
Member

Choose a reason for hiding this comment

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

This is functionally the same but it could just be !intakeCall

Copy link
Member

Choose a reason for hiding this comment

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

We should also have the ability to spin the intake backwards

@@ -0,0 +1,59 @@
package org.wildstang.year2023.subsystems;
Copy link
Member

Choose a reason for hiding this comment

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

Looks like you had already committed this, I'll remove it

@Override
public String getName() {
// TODO Auto-generated method stub
return "Deployable";
Copy link
Member

Choose a reason for hiding this comment

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

Rename this to match the class name and add the class to WSSubsystems

@ARMS300
Copy link
Contributor Author

ARMS300 commented Feb 9, 2023 via email

Copy link
Member

@fruzyna fruzyna left a comment

Choose a reason for hiding this comment

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

Looks functional, and sounds like it too, these comments don't need action, just thoughts

Comment on lines +68 to +86
if (intakeCall){
intake1.setValue(1);
deployable1.setValue(true);
deployable2.setValue(true);
} else {
intake1.setValue(0);
deployable1.setValue(false);
deployable2.setValue(false);
}

if (backCall){
intake1.setValue(-1);
deployable1.setValue(true);
deployable2.setValue(true);
} else {
intake1.setValue(0);
deployable1.setValue(false);
deployable2.setValue(false);
}
Copy link
Member

Choose a reason for hiding this comment

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

So what happens when both buttons have been pressed? Should backCall always take precedent or should the last one pressed take precedent? If its the last one pressed you could use an int to store -1, 0, or 1 instead of intakeCall and backCall then having to decide which is more important

backwards = (WsJoystickButton) WSInputs.MANIPULATOR_DPAD_UP.get();

deployable1 = (WsSolenoid) WSOutputs.DEPLOYABLE1.get();
deployable2 = (WsSolenoid) WSOutputs.DEPLOYABLE2.get();
Copy link
Member

Choose a reason for hiding this comment

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

This ended up being just 1 solenoid, but we can leave this for now

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