-
Notifications
You must be signed in to change notification settings - Fork 0
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
Create arm subsystem #11
Conversation
…eir respective helper methods
*/ | ||
private fun setArmPower(power: Double) { | ||
armState = ArmState.VELOCITY_CONTROL | ||
masterSRX.set(ControlMode.MotionMagic, Math.abs(power)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
velocity control != motionmagic, use velocity control mode
|
||
@Synchronized override fun stop() { | ||
movementState = MovementState.STATIONARY | ||
setArmPower(0.0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actuate brake in this situation too
when (armState) { | ||
Arm.ArmState.LOW -> { | ||
masterSRX.set(ControlMode.MotionMagic, ArmConversion.radiansToPulses(ArmState.LOW.targetPos).toDouble()) | ||
armAngle = ArmState.LOW.targetPos |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
armAngle should always track current angle, not target position -- there's some method to get that from the talon
} | ||
ArmState.VELOCITY_CONTROL -> { | ||
masterSRX.set(ControlMode.Velocity, 0.0) | ||
when (movementState) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
movement states should be set regardless of whether or not it's in velocity control.
Also, in velocity control, more than just the state will be set -- have it literally just pass
in this case, and let the setPower function be public (in velocity control mode, use that function to control movement and direction)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some issues with timing on stationary mode and braking.
} else if (masterSRX.sensorCollection.quadratureVelocity > 0) { | ||
movementState = Arm.MovementState.UP | ||
} else if (masterSRX.sensorCollection.quadratureVelocity == 0) { | ||
stationaryTime ++ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stationaryTime
is never actually set to zero, so the second time this happens it will instantly activate stationary. Also, stylistically the ++
shouldn't have a space before it.
Either way, it's probably better not to use just a counter but instead actually keep track of time elapsed; @plato2000 mentioned that there was some timing method that was used last year in the intake code if you want a reference point.
movementState = Arm.MovementState.UP | ||
} else if (masterSRX.sensorCollection.quadratureVelocity == 0) { | ||
stationaryTime ++ | ||
if (stationaryTime >= 100) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the way, the loop is on a roughly 50ms cycle, so this condition would take a full 5 seconds.
} | ||
armAngle = ArmConversion.pulsesToRadians(masterSRX.sensorCollection.pulseWidthPosition) - armBaseAngle | ||
if (movementState == Arm.MovementState.STATIONARY) { | ||
brake.set(DoubleSolenoid.Value.kReverse) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The brake is actuated in stationary, but is never actually un-actuated when the arm starts moving. Additionally, it might be a good idea to add a timer that prevents the arm from moving for some amount of time after the brake disengages - although that may be something to implement at a later date.
@Synchronized override fun stop() { | ||
movementState = MovementState.STATIONARY | ||
setArmVelocity(0.0) | ||
brake.set(DoubleSolenoid.Value.kReverse) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might have to switch all the kReverse
and kForward
settings later, but that's not that big of a deal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few more timer issues.
if (stationaryTime >= 100) { | ||
if (movementState != Arm.MovementState.STATIONARY && brake.get() == DoubleSolenoid.Value.kReverse) { | ||
brakeTime = Timer.getFPGATimestamp() | ||
stationaryTime++ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stationaryTime
is still using a counter instead of an actual timestamp.
stationaryTime++ | ||
brake.set(DoubleSolenoid.Value.kForward) | ||
} | ||
if (Timer.getFPGATimestamp() - brakeTime > 0.5) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused as to what's actually happening here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quite a few issues with state changing.
masterSRX.set(ControlMode.MotionMagic, ArmConversion.pulsesToRadians(masterSRX.sensorCollection.pulseWidthPosition)) | ||
} else { | ||
brake.set(DoubleSolenoid.Value.kForward) | ||
when (armState) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On second thought, this would probably be better off as part of the if
statement - LOW
, EXCHANGE
, and HIGH
all do the same thing, STILL
has its own action, and VELOCITY_CONTROL
doesn't need to be handled.
// after 0.5 seconds of hold, actuate the brake and prevent movement | ||
// 0.5 seconds after actuating the brake, set to stationary | ||
// once the user tries to move, un-actuate the brake | ||
if (masterSRX.sensorCollection.quadratureVelocity == 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's both this condition for quadratureVelocity
as well as the one down at the bottom. Only one is necessary.
} | ||
if (Timer.getFPGATimestamp() - holdTime == 0.5) { | ||
movementState = Arm.MovementState.STATIONARY | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this else
block supposed to follow the enclosing statement?
} | ||
} | ||
|
||
if (masterSRX.sensorCollection.quadratureVelocity > 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may be better to have the movement state not based on what the actual current velocity is, but instead the relationship between the current position and the target position. However, this isn't really that important, and can be changed later on if necessary.
brake.set(DoubleSolenoid.Value.kReverse) | ||
|
||
} | ||
if (movementState != Arm.MovementState.STATIONARY && brake.get() == DoubleSolenoid.Value.kReverse) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also essentially duplicated lower down.
movementState = Arm.MovementState.UP | ||
} else if (masterSRX.sensorCollection.quadratureVelocity == 0) { | ||
//movementState = Arm.MovementState.HOLD | ||
if (movementState != Arm.MovementState.STATIONARY && brake.get() == DoubleSolenoid.Value.kReverse) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check would fail whenever the bot is in STATIONARY
and would instantly shift into HOLD
.
return | ||
} | ||
//if current time - init time > 0.5 : skip loop: set to stationary | ||
if (movementState == Arm.MovementState.STATIONARY) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also partially duplicated down below.
not sure if right
Unused subsystem |
Resolves #9.