Issue with two millis() timers interfering

Two timers are interfering: digital output pin12 is set high during time static const int interval1 = 2000ms and digital output pin13 is set high during static const int interval2= 3000ms.
First digital output pin12 is set high, and after that digital output pin13 is set high, yet this second output remains high during a time interval2 - interval1 = 1000ms.
What is needed is that the interval1 is used for output pin13, and not the difference between both intervals for digital output pin13, ie digital output pin13 has to remain high during interval2 which is 3000ms.

I am stuck trying to figure out the reason. Can someone please help?

// http://forum.arduino.cc/index.php?topic=499210.msg3407044#msg3407044
// http://forum.arduino.cc/index.php?topic=396450.0

// HC12 Communication between Arduinos

/*
  This program serves to send one integer (or byte) only from two different HC12
  transmitters at one sensor each (A and B)to one HC12 receiver at the central
  controller Arduino. The receiving HC12 has to know from which transmitter A or B
  the value comes.
  The data are sent with start- and endmarkers

  v5: two integers used, one for the sensor value (integer1FromPC) and one
  for the sensor identifier (integer2FromPC). At the transmitter the sensor A or B are
  identiefied by reading two digital inputs connected to a dipswitch with
  2 switches (max: 4 different sensor/transmitters).

  // RECEIVER PART
*/

#include <SoftwareSerial.h>
const byte HC12RxdPin = 4;                  // Recieve Pin on HC12
const byte HC12TxdPin = 5;                  // Transmit Pin on HC12
SoftwareSerial HC12(HC12TxdPin, HC12RxdPin); // Create Software Serial Port
const byte numChars = 32;
char receivedChars[numChars];
char tempChars[numChars];        // temporary array for use when parsing
// variables to hold the parsed data
char messageFromPC[numChars] = {0};
int integer1FromPC = 0;
int integer2FromPC = 0;
static int previousInteger1FromPC = 0;
static int previousInteger2FromPC = 0;
float floatFromPC = 0.0;

boolean newData = false;

//============

void setup()
{
  Serial.begin(115200);
  HC12.begin(115200);       // Open serial port to HC12

  // declare pin 10, 11 & 12 as output
  pinMode (10, OUTPUT); // sensor value from A
  pinMode (11, OUTPUT); // sensor value from B
  pinMode (12, OUTPUT); // data being received
  pinMode (13, OUTPUT); // data received is different from previous data
  delay(100);
}

void loop() {

  static const int interval2 = 3000;
  static long int previousMillis2;

  recvWithStartEndMarkers();
  if (newData == true) {
    strcpy(tempChars, receivedChars);
    // this temporary copy is necessary to protect the original data
    //   because strtok() used in parseData() replaces the commas with \0
    parseData();
    useParsedData();
    newData = false;
    if (previousInteger1FromPC != integer1FromPC) {
      digitalWrite (13, HIGH); // new data different from previous data
      previousMillis2 = millis();
    }
  }
  if (digitalRead(13) == HIGH) {
    if (millis() - previousMillis2 >= interval2) {
      digitalWrite (13, LOW);
    }
  }
}

//============

void recvWithStartEndMarkers() {

  static boolean recvInProgress = false;
  static byte ndx = 0;
  char startMarker = '<';
  char endMarker = '>';
  char rc;
  static unsigned long previousMillis1; // timer
  static const int interval1 = 2000;
  boolean rxStatus = false;
  boolean ledState = false;


  while (HC12.available() > 0 && newData == false) {
    rc = HC12.read();

    if (recvInProgress == true) {
      if (rc != endMarker) {
        receivedChars[ndx] = rc;
        ndx++;
        if (ndx >= numChars) {
          ndx = numChars - 1;
        }
      }
      else {
        receivedChars[ndx] = '\0'; // terminate the string
        recvInProgress = false;
        ndx = 0;
        newData = true;
      }
    }

    else if (rc == startMarker) {
      recvInProgress = true;
      digitalWrite(12, HIGH); // new data received
      previousMillis1 = millis();
      previousInteger1FromPC = integer1FromPC;
    }
  }

  if (digitalRead(12) == HIGH) {
    if (millis() - previousMillis1 >= interval1) {
      digitalWrite(12, LOW);
    }
  }


}


//============

void parseData() {      // split the data into its parts

  char * strtokIndx; // this is used by strtok() as an index. It creates the variable
  // strkIndx as a pointer to a variable of type char. It can hold the address of a
  // variable of type char.
  /*
    strtokIndx = strtok(tempChars, ",");     // get the first part - the string
    strcpy(messageFromPC, strtokIndx); // copy it to messageFromPC

    strtokIndx = strtok(NULL, ","); // this continues where the previous call left off
    integer1FromPC = atoi(strtokIndx);     // convert this part to an integer
    /*
        strtokIndx = strtok(NULL, ",");
        floatFromPC = atof(strtokIndx);     // convert this part to a float
  */

  strtokIndx = strtok(tempChars, ",");     // get the first part - the string
  integer1FromPC = atoi(strtokIndx);     // convert this part to an integer

  strtokIndx = strtok(NULL, ","); // this continues where the previous call left off
  integer2FromPC = atoi(strtokIndx);     // convert this part to an integer

  strtokIndx = strtok(NULL, ","); // this continues where the previous call left off
  strcpy(messageFromPC, strtokIndx); // copy it to messageFromPC


}
//============

void useParsedData() {
  if (messageFromPC[0] == 'A') analogWrite (10, integer1FromPC);
  if (messageFromPC[0] == 'B') analogWrite (11, integer2FromPC);
}

Anytime you use millis, make all your variables unsigned long. Not int

jbellavance:
Anytime you use millis, make all your variables unsigned long. Not int

Right, corrected, but the problem is still there.

So, post the "corrected" code

AWOL:
So, post the “corrected” code

Correct…

// http://forum.arduino.cc/index.php?topic=499210.msg3407044#msg3407044
// http://forum.arduino.cc/index.php?topic=396450.0

// HC12 Communication between Arduinos

/*
  This program serves to send one integer (or byte) only from two different HC12
  transmitters at one sensor each (A and B)to one HC12 receiver at the central
  controller Arduino. The receiving HC12 has to know from which transmitter A or B
  the value comes.
  The data are sent with start- and endmarkers

  v5: two integers used, one for the sensor value (integer1FromPC) and one
  for the sensor identifier (integer2FromPC). At the transmitter the sensor A or B are
  identiefied by reading two digital inputs connected to a dipswitch with
  2 switches (max: 4 different sensor/transmitters).

  // RECEIVER PART
*/

#include <SoftwareSerial.h>
const byte HC12RxdPin = 4;                  // Recieve Pin on HC12
const byte HC12TxdPin = 5;                  // Transmit Pin on HC12
SoftwareSerial HC12(HC12TxdPin, HC12RxdPin); // Create Software Serial Port
const byte numChars = 32;
char receivedChars[numChars];
char tempChars[numChars];        // temporary array for use when parsing
// variables to hold the parsed data
char messageFromPC[numChars] = {0};
int integer1FromPC = 0;
int integer2FromPC = 0;
static int previousInteger1FromPC = 0;
static int previousInteger2FromPC = 0;
float floatFromPC = 0.0;

boolean newData = false;

//============

void setup()
{
  Serial.begin(115200);
  HC12.begin(115200);       // Open serial port to HC12

  // declare pin 10, 11 & 12 as output
  pinMode (10, OUTPUT); // sensor value from A
  pinMode (11, OUTPUT); // sensor value from B
  pinMode (12, OUTPUT); // data being received
  pinMode (13, OUTPUT); // data received is different from previous data
  delay(100);
}

void loop() {

  static const int interval2 = 3000;
  static unsigned long previousMillis2;

  recvWithStartEndMarkers();
  if (newData == true) {
    strcpy(tempChars, receivedChars);
    // this temporary copy is necessary to protect the original data
    //   because strtok() used in parseData() replaces the commas with \0
    parseData();
    useParsedData();
    newData = false;
    previousMillis2 = millis();
    if (previousInteger1FromPC != integer1FromPC) {
      digitalWrite (13, HIGH); // new data different from previous data
    }
  }


  if (digitalRead(13) == HIGH) {
    if (millis() - previousMillis2 >= interval2) {
      digitalWrite (13, LOW);
    }
  }
}

//============

void recvWithStartEndMarkers() {

  static boolean recvInProgress = false;
  static byte ndx = 0;
  char startMarker = '<';
  char endMarker = '>';
  char rc;
  static unsigned long previousMillis1; // timer
  static const int interval1 = 2000;
  boolean rxStatus = false;
  boolean ledState = false;


  while (HC12.available() > 0 && newData == false) {
    rc = HC12.read();

    if (recvInProgress == true) {
      if (rc != endMarker) {
        receivedChars[ndx] = rc;
        ndx++;
        if (ndx >= numChars) {
          ndx = numChars - 1;
        }
      }
      else {
        receivedChars[ndx] = '\0'; // terminate the string
        recvInProgress = false;
        ndx = 0;
        newData = true;
      }
    }

    else if (rc == startMarker) {
      recvInProgress = true;
      digitalWrite(12, HIGH); // new data received
      previousMillis1 = millis();
      previousInteger1FromPC = integer1FromPC;
    }
  }

  if (digitalRead(12) == HIGH) {
    if (millis() - previousMillis1 >= interval1) {
      digitalWrite(12, LOW);
    }
  }


}


//============

void parseData() {      // split the data into its parts

  char * strtokIndx; // this is used by strtok() as an index. It creates the variable
  // strkIndx as a pointer to a variable of type char. It can hold the address of a
  // variable of type char.
  /*
    strtokIndx = strtok(tempChars, ",");     // get the first part - the string
    strcpy(messageFromPC, strtokIndx); // copy it to messageFromPC

    strtokIndx = strtok(NULL, ","); // this continues where the previous call left off
    integer1FromPC = atoi(strtokIndx);     // convert this part to an integer
    /*
        strtokIndx = strtok(NULL, ",");
        floatFromPC = atof(strtokIndx);     // convert this part to a float
  */

  strtokIndx = strtok(tempChars, ",");     // get the first part - the string
  integer1FromPC = atoi(strtokIndx);     // convert this part to an integer

  strtokIndx = strtok(NULL, ","); // this continues where the previous call left off
  integer2FromPC = atoi(strtokIndx);     // convert this part to an integer

  strtokIndx = strtok(NULL, ","); // this continues where the previous call left off
  strcpy(messageFromPC, strtokIndx); // copy it to messageFromPC


}
//============

void useParsedData() {
  if (messageFromPC[0] == 'A') analogWrite (10, integer1FromPC);
  if (messageFromPC[0] == 'B') analogWrite (11, integer2FromPC);
}

The digital output on pin13 goes HIGH at the correct time (after newData = true; ie when digital output pin12 goes LOW again). But the time during which this digital output pin13 output is HIGH, gets reduced by the time used for digital output on pin12: so pin13 is not HIGH during interval2, but during (interval1 - interval2)

EDIT: grammar

Can you explain in English (not code) what the two timers are intended to do.

...R

Digital output pin12 goes HIGH as soon as new data is being received: recvInProgress = true and this during a time = interval1. This works correctly.

Digital output pin13 needs to go HIGH when the new data received differs from the previous data. This as soon as newData = true (the new data has been read from the HC12 buffer). This during a time = interval2.
Currently the output pin13 goes HIGH right after output pin12 goes LOW, but it goes HIGH only during a time = interval1 - interval2. It needs to go HIGH during a time = interval2.

EDIT: time during which output pin13 goes HIGH = interval2 - interval1 !! Sorry for mistake!

brice3010:
Digital output pin13 needs to go HIGH when the new data received differs from the previous data.

I think you have previousMillis2 = millis(); in the wrong place. Try is like this

if (previousInteger1FromPC != integer1FromPC) {
      previousMillis2 = millis();
      digitalWrite (13, HIGH); // new data different from previous data
}

The logic would probably be more obvious if you called it differentDataReceivedMillis

...R

Robin2:
I think you have previousMillis2 = millis(); in the wrong place. Try is like this

if (previousInteger1FromPC != integer1FromPC) {

previousMillis2 = millis();
      digitalWrite (13, HIGH); // new data different from previous data
}




The logic would probably be more obvious if you called it differentDataReceivedMillis

...R

Still the same: digital output pin13 goes HIGH during interval1 - interval2

EDIT: wrong, …goes HIGH during interval2 - interval1…

My apologies!!

Robin2:
I think you have previousMillis2 = millis(); in the wrong place. Try is like this

if (previousInteger1FromPC != integer1FromPC) {

previousMillis2 = millis();
      digitalWrite (13, HIGH); // new data different from previous data
}




The logic would probably be more obvious if you called it differentDataReceivedMillis

...R

Robin2, please see my "EDIT": time during which output pin13 goes HIGH = interval2 - interval1

Sorry for mistake!

Am I right in thinking that both pin12 and pin13 should go HIGH at roughly the same time, pin12 staying high for 2000ms and pin13 for 3000ms, but pin13 goes HIGH only after pin12 goes LOW?

A wiring problem perhaps?

Martin-X:
Am I right in thinking that both pin12 and pin13 should go HIGH at roughly the same time, pin12 staying high for 2000ms and pin13 for 3000ms, but pin13 goes HIGH only after pin12 goes LOW?

A wiring problem perhaps?

Due to the nature of the program pin12 only goes HIGH as soon as 1. HC12 signals data coming in and 2. a startmarker is detected. Pin12 goes HIGH during time interval1.

Equally, pin13 must go HIGH as soon as 1. the receive is complete and 2. hence newData = true, meaning that the incoming data is ready to be parsed and 3. if the just received data differs from the previously received data. And the time during which pin13 must go HIGH should equal interval2.

For some reason however, the output pin13 goes HIGH only for a time = interval2 - interval1.

And as far as wiring is concerned: there is not much of it: a LED and a scope channel for output 12, and a different color LED for output 13, so no mistake in colors either.

EDIT: it is as if the function call recWithStartEndMarkers interferes during the execution of void loop(), and causes a reset of previousMillis2

Out of desperation I moved digital output pin13 to pin9, but exactly the same result..

If you’re happy with the wiring then the likely culprits are array indices and pointers. Do some error checking on each array index and check pointers aren’t null. Try replacing each function with a version that explicitly sets values you’re expecting, not forgetting to read serial data to avoid buffer overflows.

@robin2 stated

I think you have previousMillis2 = millis(); in the wrong place. Try is like this
Code: [Select]
if (previousInteger1FromPC != integer1FromPC) {
previousMillis2 = millis();
digitalWrite (13, HIGH); // new data different from previous data
}

I agree with him. My question is did you remove the previousMillis2 = millis() statement from where it was before the if conditional?

Please post the most current code.

Martin-X:
If you're happy with the wiring then the likely culprits are array indices and pointers. Do some error checking on each array index and check pointers aren't null. Try replacing each function with a version that explicitly sets values you're expecting, not forgetting to read serial data to avoid buffer overflows.

Thanks for your reply, but the arrays in use do work correctly (the timers in the code were introduced in a good working program), "check pointers": can you please explain, "replace functions with versions that set values to what is expected",..

I am sorry but this is so vague it is not helpful to me.
I have a very specific issue, it is explained in minute detail, I am really hoping someone can try and see what in this code specificaly may cause this issue.

My post #7 and #12 explain in plain english what should happen and what is happening; so I hope someone looks into this code in detail?

EDIT: corrected factual error: arrays

cattledog:
@robin2 stated

I agree with him. My question is did you remove the previousMillis2 = millis() statement from where it was before the if conditional?

Please post the most current code.

Hi, here below you find the current code with the previousMillis2 = millis() statement placed within the if statement.
The issue is still present.

// http://forum.arduino.cc/index.php?topic=499210.msg3407044#msg3407044
// http://forum.arduino.cc/index.php?topic=396450.0

// HC12 Communication between Arduinos

/*
  This program serves to send one integer (or byte) only from two different HC12
  transmitters at one sensor each (A and B)to one HC12 receiver at the central
  controller Arduino. The receiving HC12 has to know from which transmitter A or B
  the value comes.
  The data are sent with start- and endmarkers

  v5: two integers used, one for the sensor value (integer1FromPC) and one
  for the sensor identifier (integer2FromPC). At the transmitter the sensor A or B are
  identiefied by reading two digital inputs connected to a dipswitch with
  2 switches (max: 4 different sensor/transmitters).

  // RECEIVER PART
*/

#include <SoftwareSerial.h>
const byte HC12RxdPin = 4;                  // Recieve Pin on HC12
const byte HC12TxdPin = 5;                  // Transmit Pin on HC12
SoftwareSerial HC12(HC12TxdPin, HC12RxdPin); // Create Software Serial Port
const byte numChars = 32;
char receivedChars[numChars];
char tempChars[numChars];        // temporary array for use when parsing
// variables to hold the parsed data
char messageFromPC[numChars] = {0};
int integer1FromPC;
int integer2FromPC;
static int previousInteger1FromPC;
static int previousInteger2FromPC;
float floatFromPC = 0.0;

boolean newData = false;

//============

void setup()
{
  Serial.begin(115200);
  HC12.begin(115200);       // Open serial port to HC12

  // declare pin 10, 11 & 12 as output
  pinMode (10, OUTPUT); // sensor value from A
  pinMode (11, OUTPUT); // sensor value from B
  pinMode (12, OUTPUT); // data being received
  pinMode (9, OUTPUT); // data received is different from previous data
  delay(100);
}

void loop() {

  static const int interval2 = 1100;
  static unsigned long previousMillis2;

  recvWithStartEndMarkers();
  if (newData == true) {
    strcpy(tempChars, receivedChars);
    // this temporary copy is necessary to protect the original data
    //   because strtok() used in parseData() replaces the commas with \0
    newData = false;
    parseData();
    useParsedData();
    if (previousInteger1FromPC != integer1FromPC) {
      digitalWrite (9, HIGH); // new data different from previous data
      previousMillis2 = millis();
    }
  }
  if (digitalRead(9) == HIGH) {
    if (millis() - previousMillis2 >= interval2) {
      digitalWrite (9, LOW);
    }
  }
}

//============

void recvWithStartEndMarkers() {

  static boolean recvInProgress = false;
  static byte ndx = 0;
  char startMarker = '<';
  char endMarker = '>';
  char rc;
  static unsigned long previousMillis1; // timer
  static const int interval1 = 1000;
  boolean rxStatus = false;
  boolean ledState = false;


  while (HC12.available() > 0 && newData == false) {
    rc = HC12.read();

    if (recvInProgress == true) {
      if (rc != endMarker) {
        receivedChars[ndx] = rc;
        ndx++;
        if (ndx >= numChars) {
          ndx = numChars - 1;
        }
      }
      else {
        receivedChars[ndx] = '\0'; // terminate the string
        recvInProgress = false;
        ndx = 0;
        newData = true;
      }
    }

    else if (rc == startMarker) {
      recvInProgress = true;
      digitalWrite(12, HIGH); // new data received
      previousMillis1 = millis();
      previousInteger1FromPC = integer1FromPC;
    }
  }

  if (digitalRead(12) == HIGH) {
    if (millis() - previousMillis1 >= interval1) {
      digitalWrite(12, LOW);
    }
  }


}


//============

void parseData() {      // split the data into its parts

  char * strtokIndx; // this is used by strtok() as an index. It creates the variable
  // strkIndx as a pointer to a variable of type char. It can hold the address of a
  // variable of type char.
  /*
    strtokIndx = strtok(tempChars, ",");     // get the first part - the string
    strcpy(messageFromPC, strtokIndx); // copy it to messageFromPC

    strtokIndx = strtok(NULL, ","); // this continues where the previous call left off
    integer1FromPC = atoi(strtokIndx);     // convert this part to an integer
    /*
        strtokIndx = strtok(NULL, ",");
        floatFromPC = atof(strtokIndx);     // convert this part to a float
  */

  strtokIndx = strtok(tempChars, ",");     // get the first part - the string
  integer1FromPC = atoi(strtokIndx);     // convert this part to an integer

  strtokIndx = strtok(NULL, ","); // this continues where the previous call left off
  integer2FromPC = atoi(strtokIndx);     // convert this part to an integer

  strtokIndx = strtok(NULL, ","); // this continues where the previous call left off
  strcpy(messageFromPC, strtokIndx); // copy it to messageFromPC


}
//============

void useParsedData() {
  if (messageFromPC[0] == 'A') analogWrite (10, integer1FromPC);
  if (messageFromPC[0] == 'B') analogWrite (11, integer2FromPC);
}

Let me see if I understand correctly.

In the latest code, interval1 is 1000ms and is relevant to pin 12. The on/off timing is contained within recvWithStartEndMarkers() and it is working correctly.

interval2 is 1100 ms and applies to pin 9. All this on/off timing is contained within loop() is is not working correctly. The on period for pin 9 is not 1100 ms but instead it is a brief flash of 100ms which is interval2 - interval1. pin9 was pin 13 in previous code.

cattledog:
Let me see if I understand correctly.

In the latest code, interval1 is 1000ms and is relevant to pin 12. The on/off timing is contained within recvWithStartEndMarkers() and it is working correctly.

interval2 is 1100 ms and applies to pin 9. All this on/off timing is contained within loop() is is not working correctly. The on period for pin 9 is not 1100 ms but instead it is a brief flash of 100ms which is interval2 - interval1. pin9 was pin 13 in previous code.

Entirely correct, with the added information that the output on pin9 currently flashes for 100ms after the output on pin12 has gone LOW