Initial implementation of ArmPID subsystem - #5
Conversation
alarichunz
commented
Feb 2, 2019
alarichunz
commented
- Create the ArmPID subsystem file
- Add potentiometer to RobotMap
- Lower MaxArmSpeed to 0.75
pvu-github
left a comment
pvu-github
left a comment
There was a problem hiding this comment.
Overall this is a great start. You need to plumb the rest of the lower-level functions in this Subsystem class to control the motor for the arm.
Also, you need to 1) create the commands for controlling the arm and 2) set the OI button mapping to commands.
| /* TODO: Add digital sensors (ultrasonic/limit switch) from other subsystems */ | ||
|
|
||
| /* Analog ports */ | ||
| public static int potentiometer = 3; |
There was a problem hiding this comment.
Rename to specify what this potentiometer belongs to. (e.g. m_armPot)
| public ArmPID() { | ||
|
|
||
| // Intert a subsystem name and PID values here | ||
| super("ArmPID", 1, 2, 3); |
There was a problem hiding this comment.
I'm pretty sure the 1, 2, 3 refers to the P, I, and D values respectively.
Please create PID constants and use them instead of hardcoding them here
| // e.g. yourMotor.set(output); | ||
| } | ||
|
|
||
| public void set (double output) { |
There was a problem hiding this comment.
Coding style - Remove extra spaces between the method name and the "()" for the arguments.
| this.output = output; | ||
| } | ||
|
|
||
| public int index() { |
There was a problem hiding this comment.
rename to "getIndex()" for clarity
|
|
||
| public double zeroPoint = 0; | ||
|
|
||
| private int index = 0; |
There was a problem hiding this comment.
Add a comment to describe that this index reflects the setpoints for the arm.
| public double zeroPoint = 0; | ||
|
|
||
| private int index = 0; | ||
| public double output = 0; |
There was a problem hiding this comment.
It seems output and target aren't used in your code. These variables are likely used for setting the motor "output" and the "target" encoder position for your PID control loop to adjust the motor speeds to maintain those preset positions.
I'd suggest renaming these variables to be more clear once we figure out what they are supposed to do.
…commands, and set up OI button mapping
2 participants
