search

m-lundberg / simple-pid

A simple and easy to use PID controller in Python
MIT License
792 stars 216 forks source link

Auto_Mode toggling causes disturbance #3

Closed james0807 closed 5 years ago

james0807 commented 5 years ago

Hi,

Firstly, great tool, thank you!

I have been working on implementing the controller and I think I've come across an unintended bug in the code when it comes to toggling "auto_mode" off and on again.

The issue arises for two reasons:

  1. self._last_time is not updated during calls that take place while auto_mode is off, leading to a potentially huge dt term once auto_mode is re-enabled.

  2. The self._error_sum is reset to 0 when auto_mode is re-enabled. I think this should be reset to whatever the last output was set to (possibly in manual mode) to ensure the control begins again as though it never stopped being in control. Lets say in manual mode I've changed my output and my input (PV) has settled at a new value. If I re-enable auto_mode having changed the setpoint to match my stable input value, then my derivative and proportional terms will be zero (proportional because error=0, derivative because d_input reset to 0 via _last_input being reset to None). In order for the output to hold (the intended behaviour presumably), the error_sum term must be set to my previous output value thus mimicking the state of the controller if it had been responsible for the new stable input value. (Apologies if that isn't clear).

I've implemented the above changes and have successfully removed the "kick" that I was seeing upon re-enabling auto_mode. Let me know your thoughts.

Best,

James

m-lundberg commented 5 years ago

Hello James,

Thanks for the feedback! I'm glad you find it useful.

I definitely agree with both your points. I had not really toggled between auto and manual mode so I had not noticed these problems myself.

For point 2. I wonder if it would be a good idea to also update _last_input even if we are in manual mode? I guess it would make less of a difference than your other suggestions since currently the D-term will just become 0 for the first iteration after turning auto mode back on and it will then keep working as normal, but on the other hand it is very easy to change and should be more "correct" I think.

I'm struggling right now to see a way to set _error_sum to the last output without changing the current API, since the value needs to be passed from the user, right? The best way I can see right now is to remove the property setter for auto_mode and replace it with a set_auto_mode() or similar that also accepts the last output value from the user. Do you have any other suggestions?

If you want to implement a fix for this you are welcome to open a pull request with the changes and I will take a look as soon as I have time!

m-lundberg commented 5 years ago

I had some time so I started playing around with a solution. I opened #4 with my current fix. If you see any problems with my fix or have any suggestions for improvements they would be very welcome!

m-lundberg commented 5 years ago

I merged the changes I made. If you have any additional feedback it is still very welcome!

james0807 commented 5 years ago

Hi, sorry, I've had a crazy week. I'll take a look when I get a chance.