Skip to content
This repository has been archived by the owner on Sep 22, 2020. It is now read-only.

Climb improvements #222

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Climb improvements #222

wants to merge 2 commits into from

Conversation

orel-david
Copy link
Collaborator

@orel-david orel-david commented Mar 2, 2020

This PR adds a command that would allow the driver to manually adjust the height of which the system releases the rods. It would set this command as a default command at the beginning of the Endgame.

@orel-david orel-david requested a review from pklito March 2, 2020 18:15
@Override
public void execute() {
setpoint = -joystickInput.get() * Constants.Climber.MODIFY_JOYSTICK_RATE;
if (Math.abs((climber.getLeftHeight() + climber.getRightHeight()) / 2 - setpoint) < Constants.Climber.ALLOWED_HEIGHT_TOLERANCE) {
Copy link
Member

Choose a reason for hiding this comment

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

This line is to long. Expand its components into a (few) variable(s).

* This command would allow the driver to modify the robot's angle manually
* with the value of the left xbox's joystick.
*/
public class JoystickRelease extends CommandBase {
Copy link
Member

Choose a reason for hiding this comment

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

Does this command release the joysticks?

@@ -35,6 +36,7 @@
private final UnitModel unitModel = new UnitModel(Constants.Climber.TICKS_PER_METER);
private DoubleSolenoid stopperA = null;
private Solenoid stopperB = null;
private boolean addDefault = false;
Copy link
Member

Choose a reason for hiding this comment

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

This flag's name is not clear enough.
But, why do you need it?

}

if (addDefault) {
this.setDefaultCommand(new JoystickRelease(this));
Copy link
Member

Choose a reason for hiding this comment

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

Subsystem shouldn't know about their default commands.

Copy link
Collaborator

@pklito pklito left a comment

Choose a reason for hiding this comment

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

There is a serious lack of documentation which makes it hard to see what changes the code brings. I'd suggest you add some lines so it would be easier to review

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants