Closed loop PID bug?

teg · Post by **teg** » Tue Sep 18, 2007 6:35 am

Hello!

I think PID-algorithm has a bug. I have a feeling that it always a intergral part when I was tuning it. If you look code:

Code: Select all

			    Kp = ((long)rpm_error * flash5.pwmidle_Kp);
			    Ki = ((long)pwmidle_error_sum * flash5.pwmidle_Ki);
			    Kd = ((long)pwmidle_deriv * flash5.pwmidle_Kd);

			    tmp1 = ((long)(Kp + Ki - Kd) * (flash5.pwmidle_open_duty - flash5.pwmidle_closed_duty)) /
				((long)outpc.rpm * 100);

			    tmp1 += IACmotor_pos;

			    if (tmp1 <flash5> flash5.pwmidle_open_duty) {
				tmp1 = flash5.pwmidle_open_duty;
			    }
			   
			    DISABLE_INTERRUPTS;
			    IACmotor_pos = tmp1;
			    ENABLE_INTERRUPTS;

tmp1 will always increase even if I don't have any Ki.

Shound't line:

Code: Select all

			    tmp1 += IACmotor_pos;

Be something like:

Code: Select all

			    tmp1 = IACmotor_pos;

?

Post by **muythaibxr** » Tue Sep 18, 2007 7:17 am

no, tmp1 won't always increase, it will be 0 when the error gets small enough. Then 0 + whatever the IAC is already at = whatever the IAC is already at... meaning no movement.

The += is because tmp1 contains the amount to change the duty by, not the absolute duty. You have to add that to the current duty to affect the proper change to the duty.

Ken

teg · Post by **teg** » Tue Sep 18, 2007 9:41 am

But if error (rpm set value - current rpm) is constant, tmp1 just grows and grows (integrates)?

I didn't found place where IACmotor_pos is zeroed..

Post by **muythaibxr** » Tue Sep 18, 2007 10:53 am

teg wrote:But if error (rpm set value - current rpm) is constant, tmp1 just grows and grows (integrates)?

Not exactly... the I term you set keeps this from happening on very small error.

That's why you typically use a very low "I" term. With a very low I term, the "integration" doesn't really have a huge effect on the value of "tmp1," and will just correct a little at a time... If you're at 850 rpms, and your target is 800... it won't immediately correct, but after a little while, when enough error builds up, it'll drop the duty a bit to compensate, at which point the error diminishes, and the I term stops having an effect.

IACmotor_pos is not zeroed, it's set at crank to the cranking value, then dropped from there to wherever it should be based on the PID settings and the target RPM.

Ken

teg · Post by **teg** » Tue Sep 18, 2007 11:12 am

I know how PID works...

Maybe I'm just missing something, but this is how I understand Your code works:

Lets say, that I have P = 1, I and D = 0. Actual rpm = 900 and target = 950.
I get some correction X which should be constant if error is constant.

But if I look your code:

first correction tmp1 will be stored to IACmotor_pos,

Code: Select all

			    IACmotor_pos = tmp1;

Next program loop, if error is still same, code will "integrate" even if there isn't any I-term, because it will add tmp1 to IACmotor_pos which had a value from last program loop.

Code: Select all

 tmp1 += IACmotor_pos;

Now IACmotor_pos will have tmp1 (from previous program loop)+tmp1 value? OR is IACmotor_pos zeroed somewhere between program loops?

Post by **muythaibxr** » Tue Sep 18, 2007 11:36 am

tmp1 is set by this line:

tmp1 = ((long)(Kp + Ki - Kd) * (flash5.pwmidle_open_duty - flash5.pwmidle_closed_duty)) / ((long)outpc.rpm * 100);

every time through the loop. If the error is small, tmp1 will be 0.

IACmotor_pos will have the last absolute duty... tmp1 += IACmotor_pos will be 0 += previous valve duty, which will be equal to the previous valve duty. This is the exact behavior you want... 0 error results in 0 change.

After this, the bounds will be checked using tmp1, and then assuming it is within its bounds, it'll set the new IACmotor_pos to the tmp1 value.

tmp1 is also a local (stack), non-static variable inside the idle_ctl() function, so its value is not preserved across calls to the function. Not that it matters, as again, it's being set every time through the function anyway.

Setting tmp1 = IACmotor_pos every time through the loop will just result in IACmotor_pos staying at whatever value it was first set to, instead of increasing or decreasing as it should.

Ken

Post by **muythaibxr** » Tue Sep 18, 2007 11:53 am

Also, if RPM is 950 and target is 900, you want the integration to continue until it causes an adjustment, which will cause rpm to drop, at which point error will be 0, and the I term will be 0 (some percent of 0 error sum = 0 I term).

IACmotor_pos is the actual duty that the valve is using RIGHT NOW, and is used by the rtc to PWM the valve. It's the variable the valve duty is set to.

You can't zero it, or the valve duty will go to 0, and you'll end up with a nice engine stall.

the tmp1 value is a relative correction, meaning I need tmp1 change in duty to affect the rpm change that I want. If the next time through the loop there is still rpm error, then I'll want to further correct the duty.

This is how it's supposed to work (and how it does work).

Ken

teg · Post by **teg** » Tue Sep 18, 2007 11:53 am

muythaibxr wrote: every time through the loop. If the error is small, tmp1 will be 0.

Lets say that tmp1 is 2.

IACmotor_pos will have the last absolute duty... tmp1 += IACmotor_pos will be 0 += previous valve duty, which will be equal to the previous valve duty. This is the exact behavior you want... 0 error results in 0 change.

IACmotor_pos will have last absolute duty + tmp1, IACmotor_pos will grow by 2. So lets say IACmotor_pos was 10, then tmp1 will be 12?

Setting tmp1 = IACmotor_pos every time through the loop will just result in IACmotor_pos staying at whatever value it was first set to, instead of increasing or decreasing as it should.

Now, IACmotor_pos will have value 12.

Next program loop error still exists, and tmp1 will get value of 2, and IACmotor_pos is 12, and tmp1 += IACmotor_pos will get result tmp1 = tmp1 + IACmotor_pos = 14.

Then tmp1 is stored to IACmotor_pos.

Again in next loop, when tmp1 will get again value of 2, IACmotor_pos will grow to 16 and so on..

This shouldn't happend if I is set to zero! As far I know how PID works, when it doesn't have I, the PID controller will have constant error.

Post by **muythaibxr** » Tue Sep 18, 2007 11:58 am

teg wrote:
muythaibxr wrote: every time through the loop. If the error is small, tmp1 will be 0.
Lets say that tmp1 is 2.

IACmotor_pos will have the last absolute duty... tmp1 += IACmotor_pos will be 0 += previous valve duty, which will be equal to the previous valve duty. This is the exact behavior you want... 0 error results in 0 change.
IACmotor_pos will have last absolute duty + tmp1, IACmotor_pos will grow by 2. So lets say IACmotor_pos was 10, then tmp1 will be 12?

Setting tmp1 = IACmotor_pos every time through the loop will just result in IACmotor_pos staying at whatever value it was first set to, instead of increasing or decreasing as it should.
Now, IACmotor_pos will have value 12.

Next program loop error still exists, and tmp1 will get value of 2, and IACmotor_pos is 12, and tmp1 += IACmotor_pos will get result tmp1 = tmp1 + IACmotor_pos = 14.

Then tmp1 is stored to IACmotor_pos.

Again in next loop, when tmp1 will get again value of 2, IACmotor_pos will grow to 16 and so on..

This shouldn't happend if I is set to zero! As far I know how PID works, when it doesn't have I, the PID controller will have constant error.

Incorrect... If I is 0, tmp1 won't be 2, it'll be 0, and IACmotor_pos + 0 = 0.

The I term won't stay 2 because the actual rpm error disappears as the valve gets closer to where it should be. Eventually tmp1 will be 0, and the valve will stop moving.

As long as there is error, even if I is set by the user to 0, the P and D terms will cause tmp1 to be something other than 0, and cause the valve to move... if the calculated P, I and D values are 0, then the valve will not move... this will happen once actual rpm gets close to the target rpm.

If tmp1 is staying at 2, then that means that there is some P, I or D error being calculated based on the actual rpm error, which means, that you want to correct the absolute duty.

In other words, the code is doing what it is supposed to do.

Ken

teg · Post by **teg** » Tue Sep 18, 2007 12:15 pm

Uhh.. Lets start over..

Sorry, but I'm just a simple guy from simple country and my english isn't perfect...

Lets say that I have:
P = 1, I = 0, D=0. Target_rpm = 900. current_rpm=800.
IACmotor_pos = 10.

So error is 100.

First program loop will give tmp1 here something that is != 0, because:

Code: Select all

			    Kp = ((long)rpm_error * flash5.pwmidle_Kp);

and

Code: Select all

tmp1 = ((long)(Kp + Ki - Kd) * (flash5.pwmidle_open_duty - flash5.pwmidle_closed_duty)) /
				((long)outpc.rpm * 100);

Right? Lets say tmp1 will get here value 2.

Then IACmotor_pos will get final result 12. Right?

Next program loop I still have current_rpm=800 and same target => error is still 100.

Again tmp1 will have value != 0, because:

Code: Select all

			    Kp = ((long)rpm_error * flash5.pwmidle_Kp);

and

Code: Select all

tmp1 = ((long)(Kp + Ki - Kd) * (flash5.pwmidle_open_duty - flash5.pwmidle_closed_duty)) /
				((long)outpc.rpm * 100);

Am I correct? tmp1 should be again 2.

Then you will read IACmotor_pos (which was 12 after last loop) and add 2 to it. Result: IACmotor_pos will be 14. So, if error stays in 100 rpm, tmp1 will get value of 2 after every "PID" calculation and it will be added to IACmotor_pos? IACmotor_pos will grow with value of 2 by every program loop?

Post by **muythaibxr** » Tue Sep 18, 2007 12:26 pm

Correct, if error stays at 100, and P is non-0, even if I and D are 0, you will keep getting more correction. It'll keep trying to correct until the error gets close to 0.

As long as tmp1 is non-zero, that means that the error is non-zero, and that means that the duty should be adjusted to try to make rpm change.

This is exactly how it's supposed to work.

If there is error, and tmp1 ends up non-0, the duty should be adjusted.

Ken

teg · Post by **teg** » Tue Sep 18, 2007 12:47 pm

Normally PID algorithm doesn't work like that. P (proportional part) just reacts to current error. It doesn't affect to past (as I part) or to future (as D part).

When I=0 and D=0, PID controller works as a P-controller and output should be error * proportional band.

With a proportional controller offset (deviation from set-point) is present. Integral action (and only integral!) was included in controllers to eliminate this offset.

Now your PID-controller will eliminate this offset without integral, which is incorrect.

Post by **muythaibxr** » Tue Sep 18, 2007 1:17 pm

teg wrote:Normally PID algorithm doesn't work like that. P (proportional part) just reacts to current error. It doesn't affect to past (as I part) or to future (as D part).

When I=0 and D=0, PID controller works as a P-controller and output should be error * proportional band.

Correct, and since in your example, there is still error, there will still be correction. It is reacting to current error by further changing the output, and will continue to do so until current error diminishes.

With a proportional controller offset (deviation from set-point) is present. Integral action (and only integral!) was included in controllers to eliminate this offset.

Now your PID-controller will eliminate this offset without integral, which is incorrect.

I disagree. Previous error will not be taken into account, only previous valve position. IF there is RPM error, then the P term should affect an immediate change, which it does. As the error gets smaller, it'll affect less and less of a change, but should still affect a change.

Say my error is 800 rpm, and I have the values P = 50, I = 0, D = 0.

Also, say my target rpm is 800, and I'm at 1600 rpms, and my duty is 70%, my open duty is 80%, and closed duty is 10%.

So with a P of 50, I want to make a change to duty that should affect a 400 rpm change...

I want ((Kp + Ki - Kd) * (open duty - closed duty)) / (rpm) or

((-400 + 0 - 0) * (70)) / 1600)

or

-17.5 (-17 since we're an integer-only processor)

now, my current duty is 70, so 70 + -17 = 53.

OK, so now lets say that the rpm dropped where we wanted, and our error is now 400, we go through the same thing again...

We end up with 53 - 8, and get a new duty of 45.

And so on... The proportional value comes from the current error. As long as there is error, and the calculated value that comes from the P term is not resulting in a 0 correction, (meaning as long as the error is large enough) we correct.

It works exactly as it should... I HAVE to store the previous valve position so as to correct by a certain relative amount. I'm not storing previous corrections, but making a relative change to the current duty. IACmotor_pos is the current valve duty.

If there is currently error, and the error is large enough, the P term should affect a change... once error gets small enough that P no longer has an effect, the error sum will eventually build up, and be large enough that when multiplied by the I term, then divided by 100, we'll end up with something non-zero which will affect a change.

P causes immediate change, I causes change as error builds up, D causes change based on the current rate of change (future).

The IACmotor_pos is the actual position of the valve. I'm not incorporating previous error by using that and adding to it, I'm changing the valve position by a relative amount. Basically, if you change it by 2 %, and you have 100 error, then change it another 2%, and still have 100 error, you should continue changing it until the error goes away. Most often, a change in valve position will result in a change in rpm, which will cause the position to stay where it is when error gets small enough.

It is working exactly as it should.

Ken

natesully · Post by **natesully** » Tue Sep 18, 2007 5:34 pm

The output of the idle PID should be engine acceleration up or down, like a servo PID would be motor current + or -. This is controlled by a delta in the idle DC, so the code needs to be the way it is. The trouble is, it doesn't work the way it (theoretically) should- as the RPM approaches the setpoint, the P term does not fall off, it just stops increasing as quickly, while the accumulated DC causes overshoot. In a PID servo, the motor would loose current as it approached the set point and slow down, but in the idle PID system the engine keeps accelerating as it approaches idle speed, because the engine lags behind the IAC moving. The whole thing works well in practice though, at least for me.

I guess what I'm saying is that the code is correct, but the linearity of the system isn't. I hear that fuzzy logic is better for this kind of thing, and that is what those aftermarket electronic boost controllers use. Fuzzy is complex, but could provide an asymmetric response (try to idle up harder than idle down). I think the current code is good, it just needs the right parameters. In my case, and input for "A/C on" that adds/subtracts 20 or so to the DC would make it perfect. Droop on A/C engage is the only problem I can't fix with this code.

UnaClocker · Post by **UnaClocker** » Tue Sep 18, 2007 8:22 pm

MS2E has stepper IAC closed loop now? Man I need to toss a 36-1 wheel on my engine if that's true.

teg · Post by **teg** » Wed Sep 19, 2007 2:21 am

EDIT:

After a cup of coffee and thinking a while, my opinion is that ms2extra PID works as PID contoller usually works.

Post by **muythaibxr** » Wed Sep 19, 2007 6:52 am

UnaClocker wrote:MS2E has stepper IAC closed loop now? Man I need to toss a 36-1 wheel on my engine if that's true.

No, not yet, I still haven't received a stepper from anyone.

James is working on any neon troubles too... so that should be fixed up whenever he figures it out.

Ken

Post by **muythaibxr** » Wed Sep 19, 2007 7:30 am

Actually for me, and one of the ways I tested it, the IAC actually does slow down correction as it gets close to the set point... The engine does lag a little, but I've never had any trouble getting it to go right to the target + or - about 20 rpms.

I'm not suprised that the AC engaging drops the RPM, but the PID code should recover pretty quickly assuming it's set up right.

I considered using fuzzy logic for this and boost, but back when I was doing the research, it seemed like fuzzy logic was on the way out, and PID was what most people were doing.

There are ways I could make it "overreact" to rpm being below the target, but then you might end up with oscillation and such as well.

The algorithm isn't set in stone... it works well right now, but I'm open to improving it still... not in 2.0 but for 2.x/3.x releases.

Ken

Keithg · Post by **Keithg** » Wed Sep 19, 2007 8:44 am

My experience with the Bosch 2 wire AIC is that the # of steps is the limiting factor. Mine will sit at 44% (for example) and the set point is 900 and it is idling at 1000. 43 may put it slightly below the set point, so it sits at 44. If I adjust either I or D, it oscillates. My terms are 9,3,3. I would think that more steps would help, but understand that we are limited to the 2 frequencies we currently have. More resolution would help, I feel. Would it also allow larger values of PID coefficients?

KeithG

On 9/19/07, muythaibxr wrote:

Actually for me, and one of the ways I tested it, the IAC actually does slow down correction as it gets close to the set point... The engine does lag a little, but I've never had any trouble getting it to go right to the target + or - about 20 rpms.

Ken

Post by **muythaibxr** » Wed Sep 19, 2007 9:03 am

I actually am using pretty high PID values already, like 20,2,1 or something like that.

I think I could allow higher values for P, I and D if I munged the math a bit to allow it.

The way it is now is better than when I first did the work though, when the values that worked for people were 1 or 2 for P.

I could also make P, I, and D adjustable to .1% intervals too.

I planned on doing a lot of this for 2.1 anyway.

With the features I plan on getting in, the next release might end up being 2.5 instead of 2.1. In any case we have to get the rest of the bugs worked out of 2.0, and I've been so busy at work I haven't had time to work on it much. I'll have time Friday though, so I'm going to try to get rid of all the known bugs then that I can get rid of.

Ken