Relay example seems backwards for digitalWrite(relayPin,HIGH) == On

[drf5n]

Arduino-PID-Library/examples/PID_RelayOutput/PID_RelayOutput.ino

Lines 51 to 59 in 9b4ca0e

	/************************************************
	* 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, HIGH);
	else digitalWrite(RELAY_PIN, LOW);

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.