Is the relay example intended to turn the relay on during the first part of the window and off for the remainder?
With a windowSize of 5000, and an Output of 100, for the first 100ms of the window this conditional won't trigger:
if (Output < millis() - windowStartTime) digitalWrite(RELAY_PIN, HIGH);
so it's else clause will:
else digitalWrite(RELAY_PIN, LOW);
and for after 100ms this will trigger:
if (Output < millis() - windowStartTime) digitalWrite(RELAY_PIN, HIGH);
so this one won't:
else digitalWrite(RELAY_PIN, LOW);
So for a 5000 ms WindowSize and an Output of 100, RELAY_PIN will be LOW for 100ms followed by RELAY_PIN = HIGH for 4900ms. It would make sense with an active-low RELAY, and if you depend on using the else clause first in the window, but the double-negative logic seems awkward to explain.
If I were writing this up as an example intended for a beginner, I'd add a couple variables/consts to disambiguate HIGH vs OFF, add curly braces to the if/else and switch the logic of the if match the "turn the output pin on/off based on pid output" comment.
#define RELAY_ON LOW
#define RELAY_OFF HIGH
...
/************************************************
* turn the output pin on/off based on pid output
************************************************/
if (millis() - windowStartTime > WindowSize)
{ //time to shift the Relay Window
windowStartTime += WindowSize;
}
if (Output > millis() - windowStartTime)
{
digitalWrite(RELAY_PIN, RELAY_ON);
} else {
digitalWrite(RELAY_PIN, RELAY_OFF);
}
Switching the '<' for '>' makes the if() conditional into positive logic rather than inverse, so the ON/OFF caluses match the on/off comment, and it is clear how to handle active LOW versus active HIGH relays, rather than assuming active LOW.
Similar to #10, #30 and #61, but this issue proposes a different change to deal with both active-HIGH and active-LOW relays.
https://github.com/br3ttb/Arduino-PID-Library/blob/9b4ca0e5b6d7bab9c6ac023e249d6af2446d99bb/examples/PID_RelayOutput/PID_RelayOutput.ino#L51-L59
Is the relay example intended to turn the relay on during the first part of the window and off for the remainder?
With a windowSize of 5000, and an Output of 100, for the first 100ms of the window this conditional won't trigger:
so it's else clause will:
else digitalWrite(RELAY_PIN, LOW);
and for after 100ms this will trigger:
so this one won't:
else digitalWrite(RELAY_PIN, LOW);
So for a 5000 ms WindowSize and an Output of 100, RELAY_PIN will be LOW for 100ms followed by RELAY_PIN = HIGH for 4900ms. It would make sense with an active-low RELAY, and if you depend on using the else clause first in the window, but the double-negative logic seems awkward to explain.
If I were writing this up as an example intended for a beginner, I'd add a couple variables/consts to disambiguate HIGH vs OFF, add curly braces to the if/else and switch the logic of the if match the "turn the output pin on/off based on pid output" comment.
Switching the '
<' for '>' makes the if() conditional into positive logic rather than inverse, so the ON/OFF caluses match the on/off comment, and it is clear how to handle active LOW versus active HIGH relays, rather than assuming active LOW.Similar to #10, #30 and #61, but this issue proposes a different change to deal with both active-HIGH and active-LOW relays.