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

[drf5n]

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:

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.

[drf5n]

Additionally, the RelayOutput.ino example needs to set:

 pinMode(RELAY_PIN,OUTPUT);

The code currently does not set the pinMode:

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

In https://wokwi.com/projects/410423413759806465

To make the Wokwi simulation seem natural, I swapped the sense of the relay setting conditional so large outputs energize the relay, set the pinMode, moved the setpoint to the middle of the Sensor/Pot, and set kP so you get a full-scale output at Input=0:

// https://wokwi.com/projects/410423413759806465
// Arduino_PID_Example_Relay
// Code from 
// https://github.com/br3ttb/Arduino-PID-Library/blob/master/examples/PID_RelayOutput/PID_RelayOutput.ino
// Modifications:
// Add pinMode(RELAY_PIN,OUTPUT);
// Switch sense of output conditional per https://github.com/br3ttb/Arduino-PID-Library/issues/136

// Tune for mid-range sensor-pot with Setpoint=512
// Tune Kp = 5000/512=9.97 for full on at Pot=0

/********************************************************
 * PID RelayOutput Example
 * Same as basic example, except that this time, the output
 * is going to a digital pin which (we presume) is controlling
 * a relay.  the pid is designed to Output an analog value,
 * but the relay can only be On/Off.
 *
 *   to connect them together we use "time proportioning
 * control"  it's essentially a really slow version of PWM.
 * first we decide on a window size (5000mS say.) we then
 * set the pid to adjust its output between 0 and that window
 * size.  lastly, we add some logic that translates the PID
 * output into "Relay On Time" with the remainder of the
 * window being "Relay Off Time"
 ********************************************************/

#include <PID_v1.h>

#define PIN_INPUT 0
#define RELAY_PIN 6

//Define Variables we'll be connecting to
double Setpoint, Input, Output;

//Specify the links and initial tuning parameters
//double Kp=2, Ki=5, Kd=1;
double Kp=9.76, Ki=5, Kd=1;
PID myPID(&Input, &Output, &Setpoint, Kp, Ki, Kd, DIRECT);

int WindowSize = 5000;
unsigned long windowStartTime;

void setup()
{
  pinMode(RELAY_PIN,OUTPUT);
  windowStartTime = millis();

  //initialize the variables we're linked to
//  Setpoint = 100;
  Setpoint = 512;

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

  //turn the PID on
  myPID.SetMode(AUTOMATIC);
}

void loop()
{
  Input = analogRead(PIN_INPUT);
  myPID.Compute();

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

}

Thinking about the conditional more, if the Output is pegged at the upper 5000 limit, then when millis() - windowStartTime = 5000, it still takes the second branch of this conditional:

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