Why millis() is priting out the same value

Projects General Guidance
1 
#include <Wire.h>
#include <ArduinoJson.h>
#include <LiquidCrystal.h>   //to use LCD function download this library from arduino site

LiquidCrystal lcd(2,3,4,5,6,7);  //create an object for LCD
int redLightPin = 13;

void setup() {
  Wire.begin(8);                /* join i2c bus with address 8 */
  Wire.onReceive(receiveEvent); /* register receive event */
  Serial.begin(115200);           /* start serial for debug */
  lcd.begin(16,2);       //initialize LCD
  lcd.print("hello!");
  pinMode(redLightPin, OUTPUT); 
}

void loop() {
  delay(100);
}

// function that executes when data is received from master
void receiveEvent(int numBytes) {
 String data="";
 while (0 <Wire.available()) {
    char c = Wire.read();      /* receive byte as a character */
    data += c;
    
  }
    Serial.println(data);           /* print the request data */
    processCall(data);         /* to newline */
}


void processCall(String command){
      DynamicJsonDocument jsonDoc(1024);         
      auto error = deserializeJson(jsonDoc, command);
      if (error) {
        Serial.print(F("deserializeJson() failed with code "));
        Serial.println(error.c_str());
      return;
      }
      else
      {
        if (jsonDoc.containsKey("redlight")) {
          int state = jsonDoc["redlight"];
          Serial.println(state);
          digitalWrite(redLightPin, state);
        } else if (jsonDoc.containsKey("timer")) {
          int durationInSeconds = jsonDoc["timer"];
          Serial.println(durationInSeconds);
          writeTimerToLCD(durationInSeconds);
        }
      }
}

void writeTimerToLCD(int durationInSeconds) {
  unsigned long lastTime = 0;
  lastTime = millis();
  Serial.println(lastTime);
  lcd.clear();
  String s = "Timer: ";
  lcd.print(s+String(durationInSeconds));
  while (durationInSeconds > 0)
  {
    Serial.println(millis());
    if (millis() - lastTime >= 1000)
    {
      lastTime=millis();
      --durationInSeconds;
      lcd.clear();
      lcd.print(s+String(durationInSeconds));
    }
  }
  lcd.clear();
  lcd.print("Timer is up");
}

I have my arduino to receive a json to set the timer in the lcd by calling method writeTimerToLCD(int durationInSeconds). However, it seems that millis() is the same in side writeTimerToLCD

2

You declare lastTime inside writeTimerToLCD( ) .
And you set it to millis every time.
So millis() - lastTime is always 0.

3

Your loop does nothing. The only happenings come only once, from Setup().Why this empty loop()?

4

ieee488:
You declare lastTime inside writeTimerToLCD( ) .
And you set it to millis every time.
So millis() - lastTime is always 0.

I understand that part but what I dont understand why millis() is the same value there

Railroader:
Your loop does nothing. The only happenings come only once, from Setup().Why this empty loop()?

My setup is the arduino will receive a json (from esp8266) and it will set the timer accordingly. It only triggers by this line Wire.onReceive(receiveEvent); in setup(). That's why loop() is empty.

5

millis() does not increment inside an Interrupt Service Routine.
Since the function it is used in, writeTimerToLcd(), is called indirectly from a callback routine defined by Wire.onReceive, I believe this has the context of an ISR.
Anyway, it is not good practice to loop in such an asynchronous task for what appears to be a number of second.

6

In the receiveEvent() function you better not:

  • Use String objects, because it takes time for example to deal with the heap.
  • Use Serial functions, because the Serial library uses interrupts itself.
  • Use a library that takes times and handles large amount of data, for example the JSON library.
  • Delays or waiting while loops.
  • I prefer not use the lcd functions there as well. They do not belong in a interrupt function.

Can you remove all of that out of the receiveEvent() function ? Then it will be almost empty, I know.

volatile bool newData;
volatile byte buffer[40];

void receiveEvent(int howMany)
{
  if( howMany > 0 && howMany < sizeof( buffer))  // safety check
  {
    // Instead of Wire.readBytes(), it is also possible to use a for-loop with Wire.read.
    // I prefer the for-loop.
    Wire.readBytes( buffer, howMany);
    newData = true;
  }
}

Then you can check in the loop() if there is 'newData'. Don't forget to reset the 'newData' to false in the loop().

Do you send JSON text over I2C ? Why ? Is it possible to do that in a other way ?

7

6v6gt:
millis() does not increment inside an Interrupt Service Routine.
Since the function it is used in, writeTimerToLcd(), is called indirectly from a callback routine defined by Wire.onReceive, I believe this has the context of an ISR.
Anyway, it is not good practice to loop in such an asynchronous task for what appears to be a number of second.

Thank you very much for your explanation. That makes sense so much. I was so confused.

Koepel:
In the receiveEvent() function you better not:

  • Use String objects, because it takes time for example to deal with the heap.
  • Use Serial functions, because the Serial library uses interrupts itself.
  • Use a library that takes times and handles large amount of data, for example the JSON library.
  • Delays or waiting while loops.
  • I prefer not use the lcd functions there as well. They do not belong in a interrupt function.

Can you remove all of that out of the receiveEvent() function ? Then it will be almost empty, I know.

volatile bool newData;

volatile byte buffer[40];

void receiveEvent(int howMany)
{
 if( howMany > 0 && howMany < sizeof( buffer))  // safety check
 {
   // Instead of Wire.readBytes(), it is also possible to use a for-loop with Wire.read.
   // I prefer the for-loop.
   Wire.readBytes( buffer, howMany);
   newData = true;
 }
}




Then you can check in the loop() if there is 'newData'. Don't forget to reset the 'newData' to false in the loop().

Do you send JSON text over I2C ? Why ? Is it possible to do that in a other way ?

Thank you for all of your suggestions. I am really appreciated of your hint to refactor the code, especially newData. I don't really have to use JSON. My updated code uses a string separated by '-'.

I leave my updated code here for anyone interested in

#include <Wire.h>
#include <ArduinoJson.h>
#include <LiquidCrystal.h>   //to use LCD function download this library from arduino site

LiquidCrystal lcd(2, 3, 4, 5, 6, 7); //create an object for LCD
int redLightPin = 13;
bool newData = false;
int data[2];

void setup() {
  Wire.begin(8);                /* join i2c bus with address 8 */
  Wire.onReceive(receiveEvent); /* register receive event */
  Serial.begin(115200);           /* start serial for debug */
  lcd.begin(16, 2);      //initialize LCD
  lcd.print("hello!");
  pinMode(redLightPin, OUTPUT);
}

void loop() {
  delay(100);
  if (newData) {
    newData = false;
    processData();
  }
}

// function that executes when data is received from master
void receiveEvent(int howMany) {
  int numBytes = 50;
  char data_buffer[numBytes];
  
  if ( howMany > 0 && howMany < numBytes) // safety check
  {
    for (int i = 0; i < howMany; ++i)
    {
      data_buffer[i] = Wire.read();
    }
    data_buffer[howMany] = '\0';
    newData = true;
    char * token ;
    token  = strtok(data_buffer, "-");
    data[0] = atoi(token);
    token = strtok(NULL, "-");
    data[1] = atoi(token);
  }  
}


void processData() {
  if (data[0] == 0) {
    digitalWrite(redLightPin, data[1]);
  } else if (data[0] == 1) {
    writeTimerToLCD(data[1]);
  }
}

void writeTimerToLCD(int durationInSeconds) {
  unsigned long lastTime = 0;
  lastTime = millis();
  lcd.clear();
  String s = "Timer: ";
  lcd.print(s + String(durationInSeconds));
  while (durationInSeconds > 0)
  {
    if (millis() - lastTime >= 1000)
    {
      lastTime = millis();
      --durationInSeconds;
      lcd.clear();
      lcd.print(s + String(durationInSeconds));
    }
  }
  lcd.clear();
  lcd.print("Timer is up");
}
8

The receiveEvent() is still processing data. That does not belong there. That should be done in the loop().
In the receiveEvent(), you may put a zero-terminator at the end of the data, but that's all.

Using strtok() in a interrupt is not a good idea anyway, since it is a very weird function that remembers the position in the string. When you use it in a interrupt and in the loop(), it will go wrong :o

No one uses readable ASCII text for the I2C bus. We all use binairy data. It is often a single variable or an array or a struct. I end up using a struct because when I add new data, the data package becomes a combination of different types of variables.
The Arduino example uses text, but that is a bad example :smiling_imp:

Can you remove the delay() in the loop() ? Without delay(), everything runs faster and smoother. Let the code check 'newData' thousands of times per second. The more that variable is checked, the better 8)

In writeTimerToLCD() is a 'while' with millis(). You can use a normal delay() there. Have you seen code that someone uses millis() in that way ? When using millis() the code should not be kept in a 'while', but the Arduino loop() should run as fast and as often as possible and millis() should be used to check if it is time to do something.

Did you notice that I put 'volatile' in front of variables that are used both in a interrupt and in the loop() ?
The 'volatile' tells the compiler to keep that variable in a memory location. Without it, the 'newData' could be copied into a register and the code could check the value in the register. Meanwhile, the interrupt changes its original memory location, but the loop() only checks the register value.