[BUG] / [improvement] PID loop windup guards are fundamentally broken

[mikeshub]

Description

I had a print come off my bed so I wanted to use a PID loop to keep the power to the bed constant. The PID autotune is useless for the bed in its current form of tune + bugged loop, so I turned on PID debug and wow was I surprised. The I term winds up and stays wound up. Even when the setpoint is returned to 0. Not good.

Steps to Reproduce

Use a PID loop. This is more of a discussion of how a PID loop should work and the difference in my results.

Expected behavior:

What we expect out of a PID loop is it to climb to a target then maybe over shoot and settle out with no or minimal ringing.

Actual behavior:

Oscillations. Horrible underdamped control behavior. Ringing like a giant gong was struck.

Additional Information

This is because the outputs of the PID loop are completely unreasonable. Why are they unreasonable? Lines 920 and 924 of temperature.cpp

if (pid_output > MAX_BED_POWER) {
  // if (pid_error > 0) temp_iState -= pid_error; // conditional un-integration
  pid_output = MAX_BED_POWER;
}
else if (pid_output < 0) {
  // if (pid_error < 0) temp_iState -= pid_error; // conditional un-integration
  pid_output = 0;
}

Take a moment to consider what a PID loop is doing. The P is the heavy lifter. The I lets it track in steady state. The D rejects disturbances and reduces overshoot. How is the I term going to get to a steady state if it is constantly being wound up or down? This makes absolutely no sense whatsoever and is not mathematically valid.

Here's the basic fix along with the above code.

float max_power_over_i_gain = (float)MAX_BED_POWER / temp_bed.pid.Ki;
if (temp_iState > max_power_over_i_gain)
  temp_iState = max_power_over_i_gain;
if (temp_iState < 0)
  temp_iState = 0;
work_pid.Kp = temp_bed.pid.Kp * pid_error;
work_pid.Ki = temp_bed.pid.Ki * temp_iState;
work_pid.Kd = PID_K2 * temp_bed.pid.Kd * (temp_bed.current - temp_dState) + PID_K1 * work_pid.Kd;

Here is what I propose for the fix. A definition called NEW_WINDUP_GAURD or something is added to configiration_adv.h. That definition will control whether the old or new method is used. This will prevent people from having to retune PID loops. I'm sure if you use your imagination you can guess at the kind of havoc it would unleash if this changed was just dropped into the code and no one's PIDs were valid anymore. The tuning will no longer be valid once this change is made. This same stuff applies to the hot end PIDs as well.

I have already coded up these changes and will make a pull request once my other open one gets closed out. From my initial results this is going to be a massive improvement to the performance of the PID loops. The worst part about that windup guard is it wasn't actually working. The I term winds up and stays wound up even when the setpoint is 0 with the current windup guard.

I haven't dug into the PID autotune just yet, but I am skeptical about the results. If you think about it the bed tunings on printers that use PID beds are comically wrong. What is a P gain of 100 doing when the output of the PID loop maxes out at 255 and errors can be easily in the 5 range. This is a recipe for oscillation. I'm not sure I'm motivated to look at this mathematically, but intuitively this is all wildly wrong.

That said I ran the autotune after changing the windup guards. Recall I said the gains and performance were absolutely terrible using autotune and the old windup guard? The new one worked perfectly the first time, even if the gains do seem very high to me. I'm looking at half a degree or so oscillation about the set point. This is clearly due to the auto tune getting far too high of an i gain as the i term is oscillating back and forth between 90 and 130 for the output.

Taking the I gain down to 13 from 33 seems to have gotten rid of the ringing. The filter coefficients on the D term seem to be too high as it is often doing the opposite of what it should due to the filter's very low cut off frequency.

The main thing is HOLY !!!!! THE BED PIDs WORK NOW! In just the time to type this up it has settled in and is tracking 90c to +/- 0.01c. No more models popping off because of temp cycling bed warping. And I'm going to mess with some stuff, but this should allow the hotend PID loops to be greatly simplified, that is to say we will likely be able to get rid of the PID_FUNCTIONAL_RANGE define. Why? Because we're not winding the I term up to the max value allowed by a float on the way up to the setpoint. Getting rid of the improper windup kludge makes the P term act as a bang bang controller up until the error is small enough that the P term stops saturating the output.

I dropped the tunings all a good bit. 167 to 100, 33 to 10, D from 300 to 215 and it had minimal overshoot and the ringing has settled out about a couple of minutes. The amplitude of the ringing during the settling is +/- 0.5c about the setpoint.

To test disturbance rejection I poured a good deal of 90% rubbing alcohol on the bed with the part fan blowing over it. The bed didn't drop below 89.25 and had less than 0.25 overshoot once the alcohol completely evaporated.

My plan for some PID improvements

• First is the above discussion with the main thing being a define so that people running the firmware can transition to the new loops on their own time

• Next is to evaluate the PID autotune function and improve it. I have a sneaky suspicion that if the system being tuned is bounded in a way these are (ie max of 255 on the output) that the math behind the PID autotune is no longer valid, but I'm rusty on controls and wasn't ever that good at it anyway.

• The last thing is to just use one PID loop. There is no reason to have one function for the bed and one for the hotends.

Let me know what you think about all this? I'm pretty excited about it because who isn't sick of having models pop off the bed due to heat cycle warping.

For the hotend loop the code change looks like this:

float max_power_over_i_gain = (float)PID_MAX/ PID_PARAM(Ki, HOTEND_INDEX);
if (temp_iState[HOTEND_INDEX] > max_power_over_i_gain)
  temp_iState[HOTEND_INDEX] = max_power_over_i_gain;
if (temp_iState[HOTEND_INDEX] < 0)
  temp_iState[HOTEND_INDEX] = 0;

and this

if (pid_output > PID_MAX) {
  // if (pid_error > 0) temp_iState[HOTEND_INDEX] -= pid_error; // conditional un-integration
  pid_output = PID_MAX;
}
else if (pid_output < 0) {
  // if (pid_error < 0) temp_iState[HOTEND_INDEX] -= pid_error; // conditional un-integration
  pid_output = 0;
}

I'm not going to mess with the PR until my open one is closed, but if someone feels like adding this in the meantime go for it. The above code is all you need along with an appropriate definition in config adv.

One last thing, I'd wager with the PID loops fixed the PID_EXTRUSION_SCALING functionality can be gotten rid of as well based on how well the bed loop rejected the rubbing alcohol disturbance.

[mikeshub]

I've messed with this a little bit more this morning before I have to head into the office. The default PID exponentially weighted LPF term PID_K1 is too high at it's default. If you enable PID_DEBUG you can see that its sign does not match what it should be doing in most cases and it is still adjusting the output of the PID loop far after whatever disturbance has passed. I'm guess this is what leads to such massive D terms out of the tuning algorithm. More to come this weekend.

I seem to be getting better results by resetting the D term when the PID reset flag is set.

I wish I had more time to test this before work today. Is there a reason the d term is subtracted at the output of the loop? Shouldn't it be added to the output?

EDIT: I figured out why it is subtracted. In the calculation of the difference it is the current minus the previous temp as opposed to error minus previous error for derivative kick. It would be more clear to swap the sign in the calculation instead of the output of the PID loop. That way the signs will be right when looking at the PID debug as well as more clear in the code.

Also this would be less computationally expensive by just a little:

work_pid[HOTEND_INDEX].Kd = work_pid[HOTEND_INDEX].Kd + PID_K2 * ( PID_PARAM(Kd, HOTEND_INDEX) * (temp_dState[HOTEND_INDEX] - temp_hotend[HOTEND_INDEX].current) - work_pid[HOTEND_INDEX].Kd);

Now that I've messed with this some more I am not sure if the definition for the new wind up guards is necessary. I will try updating my CR10 later with the code change and will see if its gains still work right. If so than this fix can reside entirely in temperature.cpp

[mikeshub]

I think it might be good to add defines or a parameter to M303 to select between which level of tuning (classic, some overshoot, no overshoot) to use out of PID autotune. I am finding the classic tunings to be quite a bit overtuned.

[mikeshub]

Crickets...

Since my commit is getting handled I'll make a PR once my power is back up. I've used these changes on both my CR10 and Creator Pro with great results. I think a lot more people are going to use bed PIDs after this fix.

@mikeshub Using it myself already as well

[mikeshub]

Great! Things have been hectic, but I'm still planning the PR.

[github-actions]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.