br3ttb / Arduino-PID-Library

1.93k stars 1.11k forks source link

Is there an Overflow in PID_RelayOutput.ino? #130

Open rtek1000 opened 1 year ago

rtek1000 commented 1 year ago

Hello,

I noticed that the sum of the value in the windowStartTime variable occurs, but it seems to me that it is not restarted, so in a moment it seems to overflow.

PID_RelayOutput.ino:

  if (millis() - windowStartTime > WindowSize)
  { //time to shift the Relay Window
    windowStartTime += WindowSize;
  }

Based on the Blink code, the variable can be set using the value of millis(), this way it must always have a value within the operating range:

  if (millis() - windowStartTime > WindowSize)
  { //time to shift the Relay Window
    windowStartTime = millis();
  }

Ref.:

  unsigned long currentMillis = millis();

  if (currentMillis - previousMillis >= interval) {
    // save the last time you blinked the LED
    previousMillis = currentMillis;
   [...]
  }

https://docs.arduino.cc/built-in-examples/digital/BlinkWithoutDelay

rtek1000 commented 1 year ago

Another thing I noticed in this example:

PID_RelayOutput.ino (line29): int WindowSize = 5000;

PID_v1.cpp (line 28):

PID::SetOutputLimits(0, 255);  //default output limit corresponds to the arduino pwm limits

Therefore, it is not possible to use the entire range of window values:

PID_RelayOutput.ino (lines 58-59):

  if (Output < millis() - windowStartTime) digitalWrite(RELAY_PIN, HIGH);
  else digitalWrite(RELAY_PIN, LOW);

P.S.: I see now that it has been resolved on line 40 of PID_RelayOutput.ino:

//tell the PID to range between 0 and the full window size
  myPID.SetOutputLimits(0, WindowSize);
drf5n commented 1 year ago

windowStartTime is the same size as the millis() result, so it will roll over at about the same time as millis() itself rolls over

https://github.com/br3ttb/Arduino-PID-Library/blob/9b4ca0e5b6d7bab9c6ac023e249d6af2446d99bb/examples/PID_RelayOutput/PID_RelayOutput.ino#L30

The math for differencing unsigned longs works well for intervals.

Using the windowStartTime += windowSize; form makes it so any jitter in the window getting triggered is absorbed by the following window. The triggers will stay synchronized with every 5000ms for as long as the code runs. If you use the windowStartTime = millis(); form, if a window starts a little late, it pushes all the following windows later too.