Code for claw mech. RobotContainer updated with button bindings and s… #10
Conversation
…uch to accomodate new ClawSubsystem class.
|
|
||
| public ClawSubsystem() { | ||
|
|
||
| CommandScheduler.getInstance().registerSubsystem(this); |
There was a problem hiding this comment.
this is a line in the super constructor, which according to Kevin, is already called. So I think we can get away with not having this line
| } | ||
|
|
||
| public void openClaw(){ | ||
| motor1.set(ControlMode.PercentOutput, .5); |
There was a problem hiding this comment.
I think that we should stick to our state system, in that we should have states that are public field variable (e.g. "public boolean openClaw"), and then take action in periodic() based on that field variable. same applies for lift methods.
On top of that, we should probably use encoders and run the motor until it stops moving (rate of change of encoder reading is basically 0), rather than keeping it powered. We have a high chance of burning out motors if we just set power.
| if (clawIsOpen) { | ||
|
|
||
| // Set motor | ||
| if (rightMotor.getSelectedSensorPosition() < clawOpenPosition) { |
There was a problem hiding this comment.
I've just now realized that this opening logic may not be right; if we set the encoder position to 0 in the constructor of the clawSubsystem, then we don't know if at that point the claw is open/closed/halfway/whatever. Therefore we can't rely on a constant to check if the claw is open, because that might be different every time the code runs.
|
|
||
| // If motors are barely changing position | ||
| boolean isRightStalled = Math.abs(newRightPos - rightPos) <= stallDelta; | ||
| boolean isLeftStalled = Math.abs(newLeftPos - leftPos) <= stallDelta; |
There was a problem hiding this comment.
this will probably work; for more robust code it might make sense to normalize vs time
| /** | ||
| * Set motor powers to open. | ||
| */ | ||
| public void setOpenPowers() { |
There was a problem hiding this comment.
how it makes more sense to me is a boolean clawOpen, and then handle stalling logic / powers inside each periodic loop.. for one, it abstracts what should be abstracted in my opinion (I can't imagine anyone, including autonomous, needing something besides open + closed for claw control). that means the commands would just toggle this boolean, and then in periodic we take the boolean, and see which way to power the motors. If the motors are stalling, then we stop powering them.
this also adds the benefit of not running into a scenario where having multiple claw commands 'consumes' the change in position of the encoders, making the stall return true. Essentially what I'm saying is that this logic (for when to move the motors to get to the desired position) should only happen in the subsystem anyways, cuz it's at the subsystem's level of abstraction... idk if that makes sense?
There was a problem hiding this comment.
Hmm... maybe...
Part of the reason why the code is structured like this is because we discussed last class and decided to have a button "press and hold" control the opening of the claw. So in that sense it seemed more logical to me to create a command that handles that instead of the subsystem. Because then, the subsystem would need more than just one boolean clawOpen to control the claw opening... otherwise there's no way to implement the button hold --> open claw thing.
There was a problem hiding this comment.
For the second point about consuming the position of encoders.. what exactly does that mean? Like if two commands were trying to figure out if the motors are stalling? Also according to the docs, I believe only one command of a given subsystem can be run at a time. So there should never be any conflicts... idk I might have it totally wrong
|
Closed PR so we can delete this branch. Shop project code is on a different repo now. |
…uch to accomodate new ClawSubsystem class.