millis and 8 relays that turns on and off random

Hi there!
I have 8 relays that I need to turn on and off when 4 different PIR-sensors is active.

The relays turn on/off in pairs controlled by one PIR sensor.

I wrote a function that works fine when I just run it once. But the problem starts when I use the function twice. I can understand why it does not work but how should I tackle this problem?
Should I have more arguments in the function? Maybe one argument for the "previousMillis" and "interval"?

Here is the code:

int relayState = HIGH;             // relayState used to set the relay
int relayState2 = HIGH;  

unsigned long previousMillis = 0;  
const long interval = random(1000, 3000); 
  
unsigned long previousMillis2 = 0;  
const long interval2 = random(1000, 3000);  

void setup() {
Serial.begin(9600);
// Sensors
 pinMode(2, INPUT);    
 pinMode(3, INPUT);
 pinMode(4, INPUT);    
 pinMode(5, INPUT);

 //Relays
 pinMode(8, OUTPUT);
 pinMode(9, OUTPUT);
 pinMode(10, OUTPUT);
 pinMode(11, OUTPUT);
  pinMode(12, OUTPUT);
   pinMode(13, OUTPUT);
   pinMode(A0, OUTPUT);
    pinMode(A1, OUTPUT);
   
    
    
digitalWrite(8, relayState);   // I know that I can use a loop here but I do not remeber how...
digitalWrite(9, relayState);
digitalWrite(10, relayState);
digitalWrite(11, relayState);
digitalWrite(12, relayState);
digitalWrite(13, relayState);
digitalWrite(A0, relayState);
digitalWrite(A1, relayState);

}

void loop() {


minFunkMillis2(5, A0, A1);    // This function runs fine if I run it once in the void loop
minFunkMillis2(4, 12, 13);    // But it does not work when I run the function twice or more. I can understand why but how to solve the problem?


delay(10); 

}


void minFunkMillis2(int sensor, int relay, int relay2) {
     unsigned long currentMillis = millis();
 int sensorState = digitalRead(sensor);
  if (sensorState == HIGH){

 //This is the code for the int relay:
 if (currentMillis - previousMillis >= interval) {
    // save the last time you blinked the relay
    previousMillis = currentMillis;

    // if the relay is off turn it on and vice-versa:
    if (relayState == HIGH) {
      relayState = LOW;
       Serial.print("sensor:");
       Serial.println(sensor);
      Serial.print("relay:");
      
       Serial.print(relay);
        Serial.println(" is high");
    } else {
      relayState = HIGH;
       Serial.print("relay:");
       Serial.print(relay);
        Serial.println(" is LOW");
    }

    // set the relay with the relayState of the variable:
    digitalWrite(relay, relayState);
    
   
  }
  //This is the code for the int relay2:
    if (currentMillis - previousMillis2 >= interval2) {
    // save the last time you blinked the relay
    previousMillis2 = currentMillis;

    // if the relay is off turn it on and vice-versa:
    if (relayState2 == HIGH) {
      relayState2 = LOW;
      Serial.print("relay:");
       Serial.print(relay2);
        Serial.println(" is high");
    } else {
      relayState2 = HIGH;
       Serial.print("relay:");
       Serial.print(relay2);
        Serial.println(" is LOW");
    }

    // set the relay with the relayState of the variable:
    digitalWrite(relay2, relayState2);
  
  
  }
  }  else{
   digitalWrite(relay, HIGH);
   digitalWrite(relay2, HIGH);
   
  }
  }

I hope that I can get some advices here.
/Joel

You are on the right track. You will need a previous millis for each sensor. You can pass a reference to each previous millis into the function and that will work; however, if you use arrays for everything you can greatly reduce your code.

Here is an implementation using structure and arrays. It is much more compact code:

struct _relayControl_s
{
  const int sensorPin;
  const int relayPins[2];
  unsigned long interval[2];
  unsigned long prevMillis[2];
};

_relayControl_s relayControl[4] = 
{
  // Initialize pins.  Rest will be initialized in setup()
  {2, {8, 9}},
  {3, {10, 11}},
  {4, {12, 13}},
  {5, {A0, A1}}
};
  

void setup() {
  Serial.begin(9600);
  unsigned long interval0 = random(1000, 3000);
  unsigned long interval1 = random(1000, 3000);
  
  for (int i = 0; i < 4; i++)
  {
    pinMode(relayControl[i].sensorPin, INPUT);
    pinMode(relayControl[i].relayPins[0], OUTPUT);
    pinMode(relayControl[i].relayPins[1], OUTPUT);
    digitalWrite(relayControl[i].relayPins[0], HIGH);   
    digitalWrite(relayControl[i].relayPins[1], HIGH);   
    relayControl[i].interval[0] = interval0;
    relayControl[i].interval[1] = interval1;
    relayControl[i].prevMillis[0] = millis();
    relayControl[i].prevMillis[1] = millis();
  }
}

void loop() 
{
  static int sensorCnt = 0;
  
  if (digitalRead(relayControl[sensorCnt].sensorPin) == HIGH)
  {
    unsigned long currentMillis = millis();
    if (currentMillis - relayControl[sensorCnt].prevMillis[0] > relayControl[i].interval[0])
    {
      int relayState = digitalRead(relayControl[sensorCnt].relayPins[0]);
      digitalWrite(relayControl[sensorCnt].relayPins[0], !relayState);
    }
    if (currentMillis - relayControl[sensorCnt].prevMillis[1] > relayControl[i].interval[1])
    {
      int relayState = digitalRead(relayControl[sensorCnt].relayPins[1]);
      digitalWrite(relayControl[sensorCnt].relayPins[1], !relayState);
    }
  }
  else
  {
    digitalWrite(relayControl[sensorCnt].relayPins[0], HIGH);   
    digitalWrite(relayControl[sensorCnt].relayPins[1], HIGH);   
  }
  
  sensorCnt++;
  sensorCnt %= 4;
  delay(10);
}

Thanks a lot ToddL1962!

I will definetly learn more about using structure and arrays.
I had a look at your great looking code but it is to advanced for me at the moment. I have to go into study mode.

So I ended up duplicating my function 5 times with renaming all the arguments. I know that this is the bad way to do it. It worked for a while but then Arduino just stopped/crashed the program and I had to restart the board. Maybe because the program gets way to long.

Well, the deadline for the application where 1st of October so I just used the simplest way of controlling the relays with sensors. One relay per sensor and and the relay where HIGH when the sensor where HIGH.

Simple but effective.

Thanks again.

Simple but effective.

Does not sit well with

It worked for a while but then Arduino just stopped/crashed the program and I had to restart the board.

Maybe because the program gets way to long.

No, it is because you made some sort of mistake. If you are interested in finding what sort then post your code and show your circuit.

I know I'm just yelling into the wind here, but use classes. I'll use Todd's solution as a base, and put classes around things to encapsulate them:

class Flasher {
  const byte pin;
  unsigned long interval;
  unsigned long prevmillis;

public:
  Flasher(const byte pin): pin(pin) {}

  void setup(long interval) {
    this.interval = interval;
    this.prevmillis = millis() - interval;
    pinMode(pin, OUTPUT);
    digitalWrite(pin, HIGH);
  }

  void flash() {
    if (millis() - prevmillis > interval) {
      int relayState = digitalRead(pin);
      digitalWrite(pin, !relayState);
      prevmillis = millis();
    }
  }
 
  void dontFlash() {
    digitalWrite(pin, HIGH);
    prevmillis = millis() - interval;
  } 
}

class RelayControl
{
  const byte sensorPin;
  Flasher flasherA, flasherB;

public:
  RelayControl(const byte spin, const byte apin, const byte bpin) :
    sensorPin(spin), flasherA(apin), flasherB(bpin) {
  }
 
  void setup(unsigned long interval0, unsigned long interval1) {
    pinMode(sensorPin, INPUT); // not INPUT_PULLUP ?
    flasherA.setup(interval0);
    flasherB.setup(interval1);
  }

  void loop() {
    if (digitalRead(sensorPin) == HIGH)
    {
      flasherA.flash();
      flasherB.flash();
    }
    else
    {
      flasherA.dontFlash();
      flasherB.dontFlash();
    }
  }

};

RelayControl relayControl[] =
{
  // Initialize pins.  Rest will be initialized in setup()
  RelayControl(2,  8,  9),
  RelayControl(3, 10, 11),
  RelayControl(4, 12, 13),
  RelayControl(5, A0, A1)
};
 
cont int RELAYS = sizeof(relayControl)/sizeof(*relayControl);

void setup() {
  Serial.begin(9600);
 
  for (int i = 0; i < RELAYS; i++)
  {
    relayControl[i].setup(random(1000, 3000), random(1000, 3000));
  }
}

void loop()
{
  static int sensorCnt = 0;
  relayControl[sensorCnt].loop();
  sensorCnt++;
  sensorCnt %= RELAYS; // better to do an if>= because % does a division
  delay(10); // why?
}

Notice how now, with 'Flasher' encapsulated, out would be reasonably easy to make a RelayControl that takes an array of flashers and a length - so instead of switches controlling pairs, they'd control any number of flashers.