SetSampleTime(0) doesn't change the internal SampleTime class member

[paynterf]

Passing in a value of '0' for SetSampleTime() doesn't actually do anything, as the 'if' statement is coded incorrectly.

void PID::SetSampleTime(int NewSampleTime)
{
   if (NewSampleTime > 0)
   {
      double ratio  = (double)NewSampleTime
                      / (double)SampleTime;
      ki *= ratio;
      kd /= ratio;
      SampleTime = (unsigned long)NewSampleTime;
   }
}

should be

void PID::SetSampleTime(int NewSampleTime)
{
   if (NewSampleTime >= 0)
   {
      double ratio  = (double)NewSampleTime
                      / (double)SampleTime;
      ki *= ratio;
      kd /= ratio;
      SampleTime = (unsigned long)NewSampleTime;
   }
}

Frank

[br3ttb]

Nor should it. The tuning parameters are scaled based on the sample time. A value of 0 would break them.

…

On Wed, Apr 7, 2021, 3:25 PM Frank Paynter ***@***.***> wrote: Passing in a value of '0' for SetSampleTime() doesn't actually do anything, as the 'if' statement is coded incorrectly. void PID::SetSampleTime(int NewSampleTime) { if (NewSampleTime > 0) { double ratio = (double)NewSampleTime / (double)SampleTime; ki *= ratio; kd /= ratio; SampleTime = (unsigned long)NewSampleTime; } } should be void PID::SetSampleTime(int NewSampleTime) { if (NewSampleTime >= 0) { double ratio = (double)NewSampleTime / (double)SampleTime; ki *= ratio; kd /= ratio; SampleTime = (unsigned long)NewSampleTime; } } Frank — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#110>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACYX4Q7D3SGGQHS3RXM5LTTHSWUBANCNFSM42RMNAXA> .

[paynterf]

Hmm, you are correct - a value of zero should make Kd go to infinity (unless it is zero to start with, and then it is just 'undefined' -- oops!

I was trying to use the timing supplied by an external timer interrupt, and it actually seems to work; no idea now.

Looking a little deeper, it seems like there is no way to use an external timing source - is this correct?

TIA,

Frank

[br3ttb]

If you set this sample time equal to that of your external source, it should match up fine, provided that source fires at a regular interval. If you know that compute will only be called when it's ready to eval, you could remove the time check within that function to avoid skipped evals if there are slight time mismatches.

…

On Wed, Apr 7, 2021, 9:29 PM Frank Paynter ***@***.***> wrote: Hmm, you are correct - a value of zero should make Kd go to infinity (unless it is zero to start with, and then it is just 'undefined' -- oops! I was trying to use the timing supplied by an external timer interrupt, and it actually seems to work; no idea now. Looking a little deeper, it seems like there is no way to use an external timing source - is this correct? TIA, Frank — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#110 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACYX4XMDIKQDICPRBNSKNTTHUBFXANCNFSM42RMNAXA> .

[paynterf]

I generally do know that the function will only be called when eval is necessary. I was hoping that setting the sample time to zero would be accomplish that without having to modify PID.cpp but as you pointed out, that would result in Ki = 0 and Kd = infinity

What about doing it this way? This way SetSampleTime(0) turns off time checking entirely, without modifying Ki or Kd in the process. If PID.cpp were implemented this way, users wouldn't have to modify the library file in order to use interrupt timing.

Thoughts?

Frank

/* SetTunings(...)*************************************************************
 * This function allows the controller's dynamic performance to be adjusted. 
 * it's called automatically from the constructor, but tunings can also
 * be adjusted on the fly during normal operation
 ******************************************************************************/ 
void PID::SetTunings(double Kp, double Ki, double Kd)
{
   if (Kp<0 || Ki<0 || Kd<0) return;
 
   dispKp = Kp; dispKi = Ki; dispKd = Kd;
   
   double SampleTimeInSec = ((double)SampleTime)/1000;

   if(SampleTimeInSec > 0)
   { 
     kp = Kp;
     ki = Ki * SampleTimeInSec;
     kd = Kd / SampleTimeInSec;
   }
  if(controllerDirection ==REVERSE)
   {
      kp = (0 - kp);
      ki = (0 - ki);
      kd = (0 - kd);
   }
}
  
  
/* SetSampleTime(...) *********************************************************
 * sets the period, in Milliseconds, at which the calculation is performed	
 ******************************************************************************/
void PID::SetSampleTime(int NewSampleTime)
{
   if (NewSampleTime > 0)
   {
      double ratio  = (double)NewSampleTime
                      / (double)SampleTime;
      ki *= ratio;
      kd /= ratio;
      //SampleTime = (unsigned long)NewSampleTime;
   }

    SampleTime = (unsigned long)NewSampleTime;
}

[br3ttb]

Ki and Kd need to be modified. they have time units of 1/sec and sec respectively. (they don't NEED to, but there are good usability reasons to do this, which is why I did) the modification doesn't need to happen here, it could also happen each time the pid is evaluated, but I felt that that was a waste of floating-point math. another option might be to call compute with an interrupt that's x times faster than the sample time you want. set the sample time to 100mSec, but hit compute every 20 mSec. if your actual compute is delayed by 20mSec every once in a while due to a time mismatch, it won't be as big of a deal.

…

On Thu, Apr 8, 2021 at 1:50 PM Frank Paynter ***@***.***> wrote: I generally do know that the function will only be called when eval is necessary. I was hoping that setting the sample time to zero would be accomplish that without having to modify PID.cpp but as you pointed out, that would result in Ki = 0 and Kd = infinity What about doing it this way? This way SetSampleTime(0) turns off time checking entirely, without modifying Ki or Kd in the process. If PID.cpp were implemented this way, users wouldn't have to modify the library file in order to use interrupt timing. Thoughts? Frank /* SetTunings(...)************************************************************* * This function allows the controller's dynamic performance to be adjusted. * it's called automatically from the constructor, but tunings can also * be adjusted on the fly during normal operation ******************************************************************************/ void PID::SetTunings(double Kp, double Ki, double Kd) { if (Kp<0 || Ki<0 || Kd<0) return; dispKp = Kp; dispKi = Ki; dispKd = Kd; double SampleTimeInSec = ((double)SampleTime)/1000; if(SampleTimeInSec > 0) { kp = Kp; ki = Ki * SampleTimeInSec; kd = Kd / SampleTimeInSec; } if(controllerDirection ==REVERSE) { kp = (0 - kp); ki = (0 - ki); kd = (0 - kd); } } /* SetSampleTime(...) ********************************************************* * sets the period, in Milliseconds, at which the calculation is performed ******************************************************************************/ void PID::SetSampleTime(int NewSampleTime) { if (NewSampleTime > 0) { double ratio = (double)NewSampleTime / (double)SampleTime; ki *= ratio; kd /= ratio; //SampleTime = (unsigned long)NewSampleTime; } SampleTime = (unsigned long)NewSampleTime; } — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#110 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACYX4X2VBLHSMPM6HT4JFDTHXUHRANCNFSM42RMNAXA> .

-- Brett

[paynterf]

I understand there are very good reasons why Ki & Kd should be modified when/if the sample time is changed. However, since you already skip over this code in the case of SampleTime == 0, it seems to me that the changes I suggested won't alter functionality at all, but will allow knowledgeable users to control update timing externally by setting SampleTime to zero.

Isn't what I'm suggesting functionally equivalent to manually editing my copy of PID.cpp to removing the

if(timeChange>=SampleTime)

line from PID::Compute()?

If I manually modify PID::Compute(), the modifications will be overwritten the next time the library is updated, but if you make the changes I suggested, the functionality before and after are unchanged except in the special case of SetSampleTime(0), which you disallow now anyways.

Frank

[br3ttb]

i also skip over the SampleTime assignment when you call it with (0). So no, that isn't the same as removing the if statement in compute. calling it with 0 is the same as not calling it all. ultimately, allowing a 0 value for the sampletime, regardless of how the tuning parameters were managed, would feel like a hack. I'm not comfortable doing that here. you can always create a fork. in my opinion the robust solution to this problem is to allow for different scheduling methods, either with compiler flags or with an enum of some kind. 1: call the pid as much as possible and it schedules itself (current method), 2: you commit to calling the pid at an interval, and you tell the pid the interval so the tuning parameters are correct (what you want) 3: pid can be evaluated at an irregular interval, and the deltaT is computed each time (what I probably should have done, because despite the extra math operations, it would have caused less hassle for end-users) 4: profit?

…

On Thu, Apr 8, 2021 at 2:40 PM Frank Paynter ***@***.***> wrote: I understand there are very good reasons why Ki & Kd should be modified when/if the sample time is changed. However, since you already skip over this code in the case of SampleTime == 0, it seems to me that the changes I suggested won't alter functionality at all, but will allow knowledgeable users to control update timing externally by setting SampleTime to zero. Isn't what I'm suggesting functionally equivalent to manually editing my copy of PID.cpp to removing the if(timeChange>=SampleTime) line from PID::Compute()? If I manually modify PID::Compute(), the modifications will be overwritten the next time the library is updated, but if you make the changes I suggested, the functionality before and after are unchanged except in the special case of SetSampleTime(0), which you disallow now anyways. Frank — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#110 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACYX4RTCGU4MHT6NNFM3TDTHX2BPANCNFSM42RMNAXA> .

-- Brett

[paynterf]

i also skip over the SampleTime assignment when you call it with (0). So
no, that isn't the same as removing the if statement in compute. calling
it with 0 is the same as not calling it all.

Uh, that's why I moved the SampleTime assignment line out of the 'if()' statement in SetSampleTime() - so it would be assigned even if the calling value was == 0. My thinking was that way we get the best of both worlds; a SampleTime value of zero skips any Ki, Kd adjustments in SetSampleTime() and in SetTunings(), while also causing Compute() to update every time it is called.

As you also pointed out, I can always fork the library, but I really don't want to do that; that's what happened to the Arduino I2C after Arduino refused to fix the I2C hangup bug for over a decade; dozens of alternate libraries got spawned, all with their own quirks and idiosyncracies and the whole thing was a huge mess. Fortunately Arduino finally came to their senses and fixed it last summer. The PID library is used in many different applications and I would rather not confuse programmers with 'yet another PID library'.

To turn this argument around, can you tell me what the downside would be to implementing these changes? It seems to me there would be zero functionality change for any positive non-zero argument for SetSampleTime(). However, a user who deliberately set the SampleTime to zero would get exactly what they were after - external control over Compute() timing without having to modify PID.cpp; as long as the external timing is reasonably regular, the output from Compute() would be identical to the output using an internal time interval with the same value. Obviously the final values for Ki, & Kd would be different for an external time interval different from the default 100mSec SampleTime, but that is a necessary part of the tuning procedure anyway.

Frank

[br3ttb]

So the main reasoning here is that regardless of sample time, the pid pushes about as hard over time. If the sample is 1000 and the pid pushes a certain amount, when you change it to 100, it will push about a tenth was hard 10x more often. This is fundamental to my initial desire to create an industrial-quality algorithm. If you go through the issue history here or discussions on diy-pid-control you'll see numerous requests for things like this. Where it's not a big change, and it would probably be fine. But if you stacked all those together the library would be garbage. I get your issue, and it's a real concern. But the library is elegant for a reason. This change would mean saying: "the tuning parameters have units of seconds, unless you set the sample to 0, at which point they're based on how often you decide to call it. And if you double your calc rate, remember to halve your tuning parameters to get a similar amount of oomph. Also, all those tuning rules you've read on the internet will no longer work" What's your resistance you having a separate constructor for interrupt scenarios? Feels more bloated? Less likely to be implemented? Something else?

…

On Thu, Apr 8, 2021, 3:56 PM Frank Paynter ***@***.***> wrote: i also skip over the SampleTime assignment when you call it with (0). So no, that isn't the same as removing the if statement in compute. calling it with 0 is the same as not calling it all. Uh, that's why I moved the SampleTime assignment line out of the 'if()' statement in SetSampleTime() - so it would be assigned even if the calling value was == 0. My thinking was that way we get the best of both worlds; a SampleTime value of zero skips any Ki, Kd adjustments in SetSampleTime() and in SetTunings(), while also causing Compute() to update every time it is called. As you also pointed out, I can always fork the library, but I really don't want to do that; that's what happened to the Arduino I2C after Arduino refused to fix the I2C hangup bug for over a decade; dozens of alternate libraries got spawned, all with their own quirks and idiosyncracies and the whole thing was a huge mess. Fortunately Arduino finally came to their senses and fixed it last summer. The PID library is used in many different applications and I would rather not confuse programmers with 'yet another PID library'. To turn this argument around, can you tell me what the downside would be to implementing these changes? It seems to me there would be zero functionality change for any positive non-zero argument for SetSampleTime(). However, a user who deliberately set the SampleTime to zero would get exactly what they were after - external control over Compute() timing without having to modify PID.cpp; as long as the external timing is reasonably regular, the output from Compute() would be identical to the output using an internal time interval with the same value. Obviously the final values for Ki, & Kd would be different for an external time interval different from the default 100mSec SampleTime, but that is a necessary part of the tuning procedure anyway. Frank — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#110 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACYX4SVIBJDX3Q5G6VMSCDTHYC5PANCNFSM42RMNAXA> .

[paynterf]

Oh! Sorry if I missed the option of a separate constructor - that would be great!

Frank

[paynterf]

How will the new constructor work? I'm a bit hazy on how it would change PID behavior to be interrupt controlled. Anything you can share would be appreciated ;-)

Frank

[br3ttb]

Enum allows the internals to switch functionality, overloaded constructor allows you to select the timing method, similar to the way proportional on measurement is done

…

On Fri, Apr 9, 2021, 5:27 PM Frank Paynter ***@***.***> wrote: How will the new constructor work? I'm a bit hazy on how it would change PID behavior to be interrupt controlled. Anything you can share would be appreciated ;-) Frank — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#110 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACYX4VI5EOLVFQSUWCTOMLTH5WLJANCNFSM42RMNAXA> .

[paynterf]

Great! Do you have something already coded up that I could try, and is there anything I can do to help? TIA, Frank

…

On Sat, Apr 10, 2021 at 5:58 AM br3ttb ***@***.***> wrote: Enum allows the internals to switch functionality, overloaded constructor allows you to select the timing method, similar to the way proportional on measurement is done On Fri, Apr 9, 2021, 5:27 PM Frank Paynter ***@***.***> wrote: > How will the new constructor work? I'm a bit hazy on how it would change > PID behavior to be interrupt controlled. Anything you can share would be > appreciated ;-) > > Frank > > — > You are receiving this because you commented. > Reply to this email directly, view it on GitHub > < #110 (comment) >, > or unsubscribe > < https://github.com/notifications/unsubscribe-auth/AACYX4VI5EOLVFQSUWCTOMLTH5WLJANCNFSM42RMNAXA > > . > — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#110 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA6T325RXFFTCKXB2ONU2GLTIAOKVANCNFSM42RMNAXA> .

-- G.Frank Paynter, PhD OSU ESL Research Scientist (ret) EM Workbench LLC 614 638-6749 (cell)

[paynterf]

BTW, I just noticed something a little strange in the comment section for SetOutputLimits(): /* SetOutputLimits(...)**************************************************** * This function will be used far more often than *SetInputLimits*. while * the input to the controller will generally be in the 0-1023 range (which is * the default already,) the output will be a little different. maybe they'll * be doing a time window and will need 0-8000 or something. or maybe they'll * want to clamp it from 0-125. who knows. at any rate, that can all be done * here. **************************************************************************/ As far as I can tell, there is no function called 'SetInputLimits()' associated with PID.cpp. Is that some historical holdover? Frank

…

On Sat, Apr 10, 2021 at 8:45 AM Frank Paynter ***@***.***> wrote: Great! Do you have something already coded up that I could try, and is there anything I can do to help? TIA, Frank On Sat, Apr 10, 2021 at 5:58 AM br3ttb ***@***.***> wrote: > Enum allows the internals to switch functionality, overloaded constructor > allows you to select the timing method, similar to the way proportional on > measurement is done > > On Fri, Apr 9, 2021, 5:27 PM Frank Paynter ***@***.***> wrote: > > > How will the new constructor work? I'm a bit hazy on how it would change > > PID behavior to be interrupt controlled. Anything you can share would be > > appreciated ;-) > > > > Frank > > > > — > > You are receiving this because you commented. > > Reply to this email directly, view it on GitHub > > < > #110 (comment) > >, > > or unsubscribe > > < > https://github.com/notifications/unsubscribe-auth/AACYX4VI5EOLVFQSUWCTOMLTH5WLJANCNFSM42RMNAXA > > > > . > > > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > <#110 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AA6T325RXFFTCKXB2ONU2GLTIAOKVANCNFSM42RMNAXA> > . > -- G.Frank Paynter, PhD OSU ESL Research Scientist (ret) EM Workbench LLC 614 638-6749 (cell)

-- G.Frank Paynter, PhD OSU ESL Research Scientist (ret) EM Workbench LLC 614 638-6749 (cell)

[br3ttb]

good get! That's a holdover from the pid_beta6 which I released 2-3 years prior to this one. In terms of timeline... No idea. The changes aren't particularly hard, or risky. Just time consuming. I can commit to this being in the next release, though. In the meantime... Fork? On Sat, Apr 10, 2021, 10:07 AM Frank Paynter ***@***.***> wrote:

…

BTW, I just noticed something a little strange in the comment section for SetOutputLimits(): /* SetOutputLimits(...)**************************************************** * This function will be used far more often than *SetInputLimits*. while * the input to the controller will generally be in the 0-1023 range (which is * the default already,) the output will be a little different. maybe they'll * be doing a time window and will need 0-8000 or something. or maybe they'll * want to clamp it from 0-125. who knows. at any rate, that can all be done * here. **************************************************************************/ As far as I can tell, there is no function called 'SetInputLimits()' associated with PID.cpp. Is that some historical holdover? Frank On Sat, Apr 10, 2021 at 8:45 AM Frank Paynter ***@***.***> wrote: > Great! Do you have something already coded up that I could try, and is > there anything I can do to help? > > TIA, > > Frank > > On Sat, Apr 10, 2021 at 5:58 AM br3ttb ***@***.***> wrote: > >> Enum allows the internals to switch functionality, overloaded constructor >> allows you to select the timing method, similar to the way proportional on >> measurement is done >> >> On Fri, Apr 9, 2021, 5:27 PM Frank Paynter ***@***.***> wrote: >> >> > How will the new constructor work? I'm a bit hazy on how it would change >> > PID behavior to be interrupt controlled. Anything you can share would be >> > appreciated ;-) >> > >> > Frank >> > >> > — >> > You are receiving this because you commented. >> > Reply to this email directly, view it on GitHub >> > < >> #110 (comment) >> >, >> > or unsubscribe >> > < >> https://github.com/notifications/unsubscribe-auth/AACYX4VI5EOLVFQSUWCTOMLTH5WLJANCNFSM42RMNAXA >> > >> > . >> > >> >> — >> You are receiving this because you authored the thread. >> Reply to this email directly, view it on GitHub >> < #110 (comment) >, >> or unsubscribe >> < https://github.com/notifications/unsubscribe-auth/AA6T325RXFFTCKXB2ONU2GLTIAOKVANCNFSM42RMNAXA > >> . >> > > > -- > G.Frank Paynter, PhD > OSU ESL Research Scientist (ret) > EM Workbench LLC > 614 638-6749 (cell) > -- G.Frank Paynter, PhD OSU ESL Research Scientist (ret) EM Workbench LLC 614 638-6749 (cell) — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#110 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACYX4WEVGVIEYEEQ56FH3TTIBLS3ANCNFSM42RMNAXA> .