2 LED's on two timers unable to blink at the same time

In this sketch I receive at fixed intervals a few byte worth of datastring over a serial link. These datastrings are sent with 2000ms intervals. What should happen at the receiver end is to let output 12 (LED1) go HIGH during interval1 as soon as new data is being received. Output 9 (LED2) needs to go HIGH during interval2 if new data has been received and if a condition on these new data is fulfilled (which for the case of testing is always true).

The issue I have is that these outputs never go HIGH at the same time. Suppose interval1 = 500 and interval2 = 600 then LED1 will light up during 500ms and only after LED1 stops will LED2 light up, and during 100ms only (interval2 - interval1).

First sketch here below is the receiver sketch. Underneath is the transmitter sketch.

// 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 receive two integers (or byte) from one (later to become two different)
  HC12 transmitter.
  The data are sent with start- and endmarkers

  // RECEIVER PART
*/
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() {

  recvWithStartEndMarkers();
  lightLEDwhenNewMessage();
  if (newData == true) {
    strcpy(tempChars, receivedChars);
    Serial.println(receivedChars); // serial monitor output
    // this temporary copy is necessary to protect the original data
    //   because strtok() used in parseData() replaces the commas with \0
    parseData();
    useParsedData();
    newData = false;
  }
}

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

void recvWithStartEndMarkers() {

  static boolean recvInProgress = false;
  static byte ndx = 0;
  char startMarker = '<';
  char endMarker = '>';
  char rc;
  boolean rxStatus = false;
  boolean ledState = false;

  while (Serial.available() > 0 && newData == false) {
    rc = Serial.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;
    }
  }
}

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

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

  previousInteger1FromPC = integer1FromPC;
  previousInteger2FromPC = integer2FromPC;

  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);
}

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

void lightLEDwhenNewMessage() {

  static unsigned long previousMillis1; // timer
  static unsigned long previousMillis2;
  static const int interval1 = 500;
  static const int interval2 = 600;

  if (newData == true) {
    digitalWrite(12, HIGH); // new data received
    previousMillis1 = millis();
  }
  if (millis() - previousMillis1 >= interval1) {
    digitalWrite(12, LOW);
  }
  if (newData == true && integer1FromPC == integer2FromPC) {
    digitalWrite (9, HIGH); // new data different from previous data
    previousMillis2 = millis();
  }
  //    if (digitalRead(9) == HIGH) {
  if (millis() - previousMillis2 >= interval2) {
    digitalWrite (9, LOW);
  }
}

Transmitter sketch:

// 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 two integers (or byte) from one (later to become two different)
  HC12 transmitter.
  The data are sent with start- and endmarkers

  // TRANSMITTER PART
*/

int analogValue5, val5;
int HC12power = 8;
const int interval1 = 100; // HC12 power-off delay time in milliseconds
const int interval3 = 2000; // HC12 fixed retransmission timings
unsigned long previousMillis1 = 0; // timer1
// unsigned long previousMillis2; // timer2
unsigned long previousMillis3 = 0; // timer3
void setup()
{
  Serial.begin(115200);     // Open serial port to computer
  // HC12.begin(9600);       // Open serial port to HC12
  pinMode(HC12power, OUTPUT);
}

void loop() {

  // read analog pin 5
  analogValue5 = analogRead(5);
  // remap values from the analogValue5 variable to 0 / 255
  val5 = map(analogValue5, 0, 1023, 0, 255);

  if (millis() - previousMillis3 >= interval3) {
    previousMillis3 = millis();

    if (digitalRead(HC12power) == LOW) {

      digitalWrite(HC12power, HIGH);
      previousMillis1 = millis();
      delay(50);
    }

    Serial.print('<');
    Serial.print(val5);
    Serial.print(',');
    Serial.print(val5);
    Serial.print(",B>");

    /*
      HC12.print('<');
      HC12.print(val5);
      HC12.print(',');
      HC12.print(val5);
      HC12.print(",B>");
    */
  }
  //start timer to switch off HC12
  if (millis() - previousMillis1 >= interval1) {
    digitalWrite(HC12power, LOW);
  }
}

Print the values parsed from the message. Are they what you expect ?
Print the values that are tested by the ifs just before you do each test. Are they what you expect ?

UKHeliBob:
Print the values parsed from the message. Are they what you expect ?

Yes, values as expected, parsing works ok.

UKHeliBob:
Print the values that are tested by the ifs just before you do each test. Are they what you expect ?

Yes, values as expected.

One thing: the function where the LED's are commanded is executed continually so I serialprinted the values tested by ifs in the newData loop.

    Serial.print('<');
    Serial.print(val5);
    Serial.print(',');
    Serial.print(val5);
    Serial.print(",B>");

Any good reason why you send twice val5 - or is that just for the test?

In lightLEDwhenNewMessage() when you do

 if (newData == true && [color=red]integer1FromPC == integer2FromPC[/color]) {

you have not parsed yet integer1FromPC and integer2FromPC from the incoming buffer, you do that after in the loop()

J-M-L:
Any good reason why you send twice val5 - or is that just for the test?

Yes, that is just for testing.

J-M-L:
In lightLEDwhenNewMessage() when you do

 if (newData == true && [color=red]integer1FromPC == integer2FromPC[/color]) {

you have not parsed yet integer1FromPC and integer2FromPC from the incoming buffer, you do that after in the loop()

Correct. But when I place the LED2 timer inside the parsing function I get the same issue. What I want to achieve here is that LED2 blinks during interval2 when the newly arrived data has identical values within the one string.
And besides, the values for integer1FromPC and integer2FromPC are always identical since I connect analog input to 0V; so the parsing should in this case not make a difference.

When I remove the inter1FromPC == integer2FromPC condition for LED2, basically making the condition to blink LED1 and LED2 identical, I still have the same issue: only after LED1 goes LOW after interval1 does LED2 blink during interval2-interval1 time. Not as wanted during interval2 time

// 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 receive two integers (or byte) from one (later to become two different)
  HC12 transmitter.
  The data are sent with start- and endmarkers

  // RECEIVER PART
*/
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
  pinMode (8, OUTPUT);// data received is different from previous data
  delay(100);
}

void loop() {

  recvWithStartEndMarkers();
  lightLEDwhenNewMessage();
  if (newData == true) {
    strcpy(tempChars, receivedChars);
    Serial.println(receivedChars); // serial monitor output
    // this temporary copy is necessary to protect the original data
    //   because strtok() used in parseData() replaces the commas with \0
    parseData();
    useParsedData();
    newData = false;
  }
}

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

void recvWithStartEndMarkers() {

  static boolean recvInProgress = false;
  static byte ndx = 0;
  char startMarker = '<';
  char endMarker = '>';
  char rc;
  boolean rxStatus = false;
  boolean ledState = false;

  while (Serial.available() > 0 && newData == false) {
    rc = Serial.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;
    }
  }
}

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

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

  previousInteger1FromPC = integer1FromPC;
  previousInteger2FromPC = integer2FromPC;

  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);
}

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

void lightLEDwhenNewMessage() {

  static unsigned long previousMillis1; // timer
  static unsigned long previousMillis2;
  static const int interval1 = 500;
  static const int interval2 = 600;

  if (newData == true) {
    digitalWrite(10, HIGH); // new data received
    previousMillis1 = millis();
  }
  if (millis() - previousMillis1 >= interval1) {
    digitalWrite(10, LOW);
  }
  if (newData == true) {
    digitalWrite (8, HIGH); // new data different from previous data
    previousMillis2 = millis();
  }
  if (millis() - previousMillis2 >= interval2) {
    digitalWrite (8, LOW);
  }
}

why would it blink? you turn it on upon receiving a new message and off until a new message arrives after interval2 ms

with the code you have both should turn on when you get a new message and then through the loop() iterating one goes off after interval1 and the other interval2

if they don't then you might have a power issue for your LED.. do you have power limiting resistors on both pins?

(would suggest you don't use explicitly 9 and 12 all over the place but use const variable names that make sense)

J-M-L:
(..)
if they don't then you might have a power issue for your LED.. do you have power limiting resistors on both pins?

(would suggest you don't use explicitly 9 and 12 all over the place but use const variable names that make sense)

:grin: I had LED (two different LED's in design) cathodes connected to one resistor to GND, and LED1 has a lower Vforward (1.2V) then LED2 (1.8V) causing LED2 not to turn on while LED1 was lit. Basic electronics stuff, I shoud have known.
Good idea to use const variable names, I will correct.
Thank you!

:smiling_imp:

rule #1 ===> 1 resistor per LED to not get magic smoke on your LEDs, PINs and possibly arduino...

(even with same FWD voltage there are enough small differences that 1 LED would be a little bit below the other one and then that LED will get all the current it can absorb, limited by the R you have added - also 1 R means not the same brightness when 1 or 2 would be lit when it works)

J-M-L:
:smiling_imp:

rule #1 ===> 1 resistor per LED to not get magic smoke on your LEDs, PINs and possibly arduino...

Shouldn't have to remind a guy with an M in electronics about that :wink:

Time to read The Checklist Manifesto.

kenwood120s:
Shouldn't have to remind a guy with an M in electronics about that :wink:

Time to read The Checklist Manifesto.

Yes yes I know, rub it in :wink:

Edit: by the way, very good article!

J-M-L:
:smiling_imp:

rule #1 ===> 1 resistor per LED to not get magic smoke on your LEDs, PINs and possibly arduino...

(even with same FWD voltage there are enough small differences that 1 LED would be a little bit below the other one and then that LED will get all the current it can absorb, limited by the R you have added - also 1 R means not the same brightness when 1 or 2 would be lit when it works)

Agree and disagree: if the resistor is choosen to limit to the maximum current to a LED there is no way anything would go up in smoke when more than one LED's cathode is connected to this same resistor. But fully agree it is a bad idea.

brice3010:
Yes yes I know, rub it in :wink:

Hell, I did add a smiley.... (to soften the blow)

kenwood120s:
Hell, I did add a smiley.... (to soften the blow)

I saw that, don't worry, no offence taken, on the contrary: being humble from time to time helps :slight_smile:

Colleague sent me a pdf of The Checklist Manifesto after he an I had a brainstorm session about some training material we were working on. If surgeons need checklists to remind them to give medicine before cutting the customer open ffs, then the rest of us are in good company and shouldn't feel bad about wiring LEDs up wrong.

I'm a big believer in job aids as takeaways from training sessions.

kenwood120s:
Colleague sent me a pdf of The Checklist Manifesto after he an I had a brainstorm session about some training material we were working on. If surgeons need checklists to remind them to give medicine before cutting the customer open ffs, then the rest of us are in good company and shouldn't feel bad about wiring LEDs up wrong.

I'm a big believer in job aids as takeaways from training sessions.

+1

brice3010:
Agree and disagree: if the resistor is choosen to limit to the maximum current to a LED there is no way anything would go up in smoke when more than one LED's cathode is connected to this same resistor.

You are right - I should have qualified more

I meant if the R is sized to be right for both LEDs on at the same time with solid brightness and not just one. Then when one fails the other asks for lots of current which might damage the pin and will kill also the second led