wpilibsuite / frc-docs

Official FRC Documentation powered by Read the Docs
https://docs.wpilib.org
Other
149 stars 264 forks source link

Document enable/disable functionality for PIDController widget #833

Closed sciencewhiz closed 3 years ago

sciencewhiz commented 4 years ago

The PIDController widget for SmartDashboard/Shuffleboard has an enabled boolean property that isn't supported by the new PIDController class. While it was very important to be able to disable the old PIDController since it automatically wrote to the motor, it is still helpful to pass the enabled flag to user code for while tuning with LiveWindow.

cttdev commented 4 years ago

With the new PIDController, the user is left with calling the calculate() method in their robot loop. I think it should be left to the user to implement an enable/disable toggle for the controller(perhaps using a SmartDashboard boolean) as they are the ones calling it, transitively they are the ones declaring when the output will be written too. This would also allow for the user to customize what the happens to the output when the controller is enable/disabled.

Starlight220 commented 4 years ago

If this is settled, this issue should be closed.

calcmogul commented 4 years ago

We could add an example of the SmartDashboard boolean thing to frc-docs.

It's worth noting motors automatically disable in disabledPeriodic(). Users could just disable when changing the PID constants then reenable. Integrators would still run if the controller is in robotPeriodic(), but that was an issue with the old PIDController too.

Daltz333 commented 4 years ago

Transferring to frc-docs. Should document how users can implement this.

jasondaming commented 3 years ago

I am confused as to what documentation is needed here. Is it why the boolean went away or how it can be added back? or what?

Daltz333 commented 3 years ago

The new PIDController is ran via the calculate() method. We should provide an example of how to replicate the old functionality (enabling/disabling via dashboard) with the new PIDController. This can be done via a simple boolean toggle on the dashboard.

There are options that we have discussed and decided against:

I don't completely remember on the decision making for this as it was in one of our meetings several months back, so @calcmogul may be able to weigh in a bit more.

This issue is open to document replicating the enable/disable functionality via a NT boolean toggle box.