Messages in this thread | | | Date | Thu, 26 Feb 2015 17:52:53 +0000 | From | Javi Merino <> | Subject | Re: [PATCH v1 4/7] thermal: introduce the Power Allocator governor |
| |
On Wed, Feb 25, 2015 at 07:00:38PM +0000, Eduardo Valentin wrote: > On Wed, Feb 25, 2015 at 02:48:49PM +0000, Javi Merino wrote: > > Hi Eduardo, > > > > On Tue, Feb 24, 2015 at 06:21:26PM +0000, Eduardo Valentin wrote: > > > On Wed, Jan 28, 2015 at 05:00:35PM +0000, Javi Merino wrote: > > > > + > > > > +k_d > > > > +--- > > > > + > > > > +`k_d` configures the PID loop's derivative term constant. It's > > > > +recommended to leave it as the default: 0. > > > > + > > > > > > I know we are considering K_d = 0. However, ... > > > > > > <yet another big cut> > > > > > > > + /* > > > > + * Calculate the derivative term > > > > + * > > > > + * We do err - prev_err, so with a positive k_d, a decreasing > > > > + * error (i.e. driving closer to the line) results in less > > > > + * power being applied, slowing down the controller) > > > > + */ > > > > + d = mul_frac(tz->tzp->k_d, err - params->prev_err); > > > > > > > > > ... Shouldn't the above d component consider the rate of changes over time of the error? > > > > > > I would expect you should do: > > > d = k_d * (dE / dt) > > > > > > or > > > > > > d = K_d * ((err - params->prev_err) / sampling_period) > > > > > > in plain C: > > > > > > + d = mul_frac(tz->tzp->k_d, err - params->prev_err); > > > + d /= tz->passive_polling; /* might require fixed point division */ > > > > Could do. To be honest, both k_d and passive_polling are constants so > > Yes, I agree that they are constants. But if you deploy the thermal zone > with different sampling period on different devices, then the behavior > will change. > > > I don't think you get anything by doing this other than the added > > complexity of the fixed point division. As you said, the default k_d > > is 0, so I'm not strongly against it. > > OK. Then I would prefer to add the division, as it makes the code aligned > to the concept.
Fair enough, I've added it to the code. We will send a v2 later today if all regressions pass.
Cheers, Javi
| |