Regarding running multiple functions using millis

Question

I have been writing code for measuring when the digital signal comes and goes (ON and OFF respectively). I have done the part calculating a single system's downtime (difference in timings between ON and OFF from the system), but when I'm not able to scale up to multiple systems when I try to copy the same code which worked above. So it would be helpful if anyone could provide some insight into how it can be done plus explaining with the code would help.

My code:

unsigned long a=0, b=0, difference;  
void setup()   
{    
  Serial.begin(9600);  
  pinMode(2, INPUT);  
  Serial.print("start");  
  Serial.print(",");  
  Serial.print("stop");  
  Serial.print(",");  
  Serial.print("minutes");  
  Serial.print(",");  
  Serial.println("seconds");  
}

void loop()   
{  
 //delay(1000);  
 int A = digitalRead(2);  
 if(A == LOW)  
 {  
  b = millis();  
 }  
 if(A == HIGH)  
  {  
     a = millis();  
     display_output();  
   }  
 delay(3000);  
}  
void display_output()  
{  
 int minutes, seconds, sec;  
 difference = a - b;  
 seconds = int(difference/1000);  
 if(seconds >= 60)  
 {  
  //sec = seconds;  
  seconds = seconds%60;  
  minutes = minutes+1;  
 }  
 //minutes = int(seconds/60);  
 Serial.print((a/1000)+1);  
 Serial.print(",");  
 Serial.print((b/1000)+1);  
 Serial.print(",");  
// Serial.print(difference);  
 //Serial.print(",");  
 Serial.print(minutes);  
 Serial.print(",");  
 Serial.println(seconds);  
 //Serial.println(",");  
}  

Answer 1

First of all, you should notice there is a problem with your logic:

 int A = digitalRead(2);
 if(A == LOW)  
 {  
  b = millis();  
 }

Here you are testing the state of the pin. If it's LOW you take a record of the time. This is wrong. What you want to record is the time at which the inputs falls, not when it's LOW. This is called state change detection. In order to implement it, you have to compare the pin state that you just read with the previous state. The correct condition is “if the pin was HIGH last time I read it and is now LOW”:

// Global variable:
int pin_state = HIGH;

// Within loop():
int new_pin_state = digitalRead(2);
if (pin_state == HIGH && new_pin_state == LOW) {  // it just fell
    time_of_fall = millis();
    pin_state = new_pin_state;
}

Note that this assumes that the default state is HIGH. If the pin turns out to be LOW at power-up, then the code will assume it just fell when the program started.

Next, you have to remove any occurrence of delay() from your code. That will only make your timings wrong. At this point you have non-blocking code that can easily be adapted to handle multiple input channels.

Then you have to identify the variables that are channel-specific, and have a per-channel version of each variable (arrays are useful here). Then your code only needs to loop through the arrays.

Instead of having multiples arrays, you can make the code cleaner by grouping all the channel-specific data into a single data structure:

struct Channel {
    byte pin;
    byte state;
    unsigned long fell, rose;  // times of fall and rise
};

This way you can have a single array that holds all the data for all the channels:

const int CHANNEL_COUNT = 5;

Channel channels[CHANNEL_COUNT] = {
    { 2, HIGH, 0, 0 },
    { 3, HIGH, 0, 0 },
    { 4, HIGH, 0, 0 },
    { 5, HIGH, 0, 0 },
    { 6, HIGH, 0, 0 }
};

The code now becomes quite simple: you just loop through the array and handle one channel at a time:

void setup()
{
    for (int i = 0; i < CHANNEL_COUNT; ++i) {
        pinMode(channels[i].pin, INPUT);
    }
    Serial.begin(9600);
    Serial.println("pin,fell,rose,low time");
}

void loop()
{
    for (int i = 0; i < CHANNEL_COUNT; ++i) {
        Channel &c = channels[i];
        byte new_state = digitalRead(c.pin);
        if (c.state == HIGH && new_state == LOW) {  // input fell
            c.fell = millis();
        }
        else if (c.state == LOW && new_state == HIGH) {  // input rose
            c.rose = millis();
            display_channel(c);
        }
        c.state = new_state;
    }
}

The function display_channel() is left as an exercise to the reader ;-) (hint: it's prototype should be void display_channel(const Channel &c); in order to avoid unnecessarily copying the data structure.

Next step in code modularity would be to turn the Channel structure into a class with a proper constructor, and make display_channel() a method of that class.