Made convenience functions for tuning a CustomPIDController on SmartDashboard #197
Conversation
| SmartDashboard.putNumber(prefix + "_F", getD()); | ||
| return; | ||
| } | ||
| setPIDF(SmartDashboard.getNumber(prefix + "_P", getP()), |
There was a problem hiding this comment.
Do you want to use the two-argument version with a default value? We've done this in the past while testing (with default 0).
There was a problem hiding this comment.
I think this is a good way of doing it, it will default to the previous P/I/D/F value.
There was a problem hiding this comment.
That is what I'm doing...?
There was a problem hiding this comment.
Ah yes -- I have... room to improve.
| * @param prefix | ||
| * The prefix to use when putting things on SmartDashboard. | ||
| */ | ||
| public void putToSmartDashboard(String prefix) { |
There was a problem hiding this comment.
This should be added as an abstract function to MotionController (same for the other functions).
|
@carterturn Yes, perhaps, except that the values themselves are implementation-specific. |
|
@ajnadel Yes. I think we should only add abstract functions to MotionController. Otherwise we will have to declare all MotionControllers to actually be a certain non-abstract type. |
| SmartDashboard.putNumber(prefix + "_Error", getError() + noise); | ||
| SmartDashboard.putNumber(prefix + "_Setpoint", getSetpoint() + noise); | ||
| SmartDashboard.putNumber(prefix + "_Sensor", getSensorValue() + noise); | ||
| SmartDashboard.putNumber(prefix + "_Output", get() + noise); |
There was a problem hiding this comment.
Also, did we decide that the mutating effects of calling get() (of the I and D) calculations are not big enough to matter?
There was a problem hiding this comment.
I think we have decided to fix that in the future. It should not make too much of a difference, as it does calculate time deltas every time.
There was a problem hiding this comment.
Cool -- and the sensor updates fast enough that each few ticks will probably have a different value.
There was a problem hiding this comment.
That is on the list to be fixed. (In fact I have a list of PID changes we need to make that various programmers will be doing before SVR)
| SmartDashboard.putNumber(prefix + "_P", getP()); | ||
| SmartDashboard.putNumber(prefix + "_I", getI()); | ||
| SmartDashboard.putNumber(prefix + "_D", getD()); | ||
| SmartDashboard.putNumber(prefix + "_F", getD()); |
|
@carterturn better? |
| @@ -18,6 +19,7 @@ | |||
| protected double A; | |||
| protected double F; | |||
| protected double threshold; | |||
| protected static final String DEFAULT_SMARTDASHBOARD_PREFIX = "!!"; | |||
There was a problem hiding this comment.
If this actually works, I think it is great. I would watch out for improper nonalphanumeric character parsing in NetworkTables though.
There was a problem hiding this comment.
Why did we change it to this? It's pretty jenk
There was a problem hiding this comment.
This did not exist before. It is for the bang (!) bang (!) controller. We should make a shebang controller at some point.
There was a problem hiding this comment.
As funny as that is it should probably be BangBang or BANGBANG
|
I'm still not convinced we want to REQUIRE every controller to implement this. I think we should either have a default implementation or make this an interface. |
|
I think it is trivial enough to implement in most cases that a default implementation is not really going to make much of a difference. This is a convenient interface for tuning that should probably be preferred over the reupload with new constants method. |
Include our PID tuning code as an optional module in
CustomPIDController.