Do Loop failure [millis() ?]

I have a home control project that monitors switches and runs a ventilation system. I want it to exit the loop and blink the LED on pin13 every few seconds as a system “OK” indication. It performs as expected for about 30-40 (I didn’t count them) cycles, with the LED blinking every 2 seconds, then the LED starts blinking as if the Do Loop is no longer being executed, eg, the blinker flashes as quickly as possible.

Is there something about millis() that I don’t understand? (loop_start is an unsigned long) I suspected something in the called functions was causing the problem but I commented them out so that there is nothing inside the Do Loop and the behavior is the same.

// ------------Main Program Loop------------
void loop() {
  loop_start = millis() ;
  do {
//    check_clocks() ;
//    if(switch_query()) {      // is any switch line at logic 0?
//      if(valid_switch()) {    // a valid switch activation waits here for release
//        switch_action() ; } } // yes, it's valid; change the light control words 
     } while(millis() - loop_start < 2000 ) ; // 2 seconds between blinker
   blinker(50) ;   
   }                          // blink every 2 seconds; end of LOOP
  
// -----------End Main Program Loop---------------

// ------------------------------------------------
void blinker(int a) {       // blink for "2a" msec
  delay(a / 2) ;            // ensure there is a blink
  digitalWrite(led, HIGH);  // turn the LED on (HIGH is the voltage level)
  delay(a);                 // wait for "a" msec
  digitalWrite(led, LOW);   // turn the LED off by making the voltage LOW
  delay(a / 2) ; }          // ensure there is a blink, end of blinker
// -----------------------------------------------

Show us your definition of loop_start.

Arctic_Eddie:
It probably lasted 32.77 seconds because loop_start is likely a 16 bit unsigned integer. Change it to 32 bits.

No he said - loop_start is an unsigned long (but is it really we don’t know because we can’t see…)

so I tried this code and I don’t see the behavior you describe.

unsigned long   loop_start;
const int led = 13;

void setup() {
  pinMode(led, OUTPUT);
}


// ------------Main Program Loop------------
void loop() {
  loop_start = millis() ;
  do {
  } while (millis() - loop_start < 2000 ) ; // 2 seconds between blinker
  blinker(50) ;
}                          // blink every 2 seconds; end of LOOP

// -----------End Main Program Loop---------------

// ------------------------------------------------
void blinker(int a) {       // blink for "2a" msec
  delay(a / 2) ;            // ensure there is a blink
  digitalWrite(led, HIGH);  // turn the LED on (HIGH is the voltage level)
  delay(a);                 // wait for "a" msec
  digitalWrite(led, LOW);   // turn the LED off by making the voltage LOW
  delay(a / 2) ;
}          // ensure there is a blink, end of blinker
// -----------------------------------------------

can you try again with that exact code? we don’t know what your setup() looks like and if you trigger any timer or interrupt or anything… so a lot can happen in the code we don’t see.

I was suspecting it was actually a uint16_t which has a cycle time of about 33 seconds.

Arctic_Eddie:
I was suspecting it was actually a uint16_t which has a cycle time of about 33 seconds.

and you 100% might be right given the OP did not share the full setup() and variable declaration…

it has been 9 minutes now and my LED still blinks at the right pace without any pb…

Here’s the full code. No interrupts.

char ver[] =   "Fan_ver_A 10/11/16" ;
char chan1[] = "Fans w/o lights" ;
char chan2[] = "Fans off in 7 mins" ;

int led = 13 ;

int loop_count ;
byte bad_switch = 0 ;       // stores continuously pressed (bad) switch
byte sw_pressed = 0 ;       // stores which switch was pressed
byte pressed_mask = 0 ;     // ensures that only one switch is tested

// The fan automatically will switch off 7 minutes after it is turned on.
// Fans can be manually turned off with the fan switch.
unsigned long mbath_clock ; // timers for auto fan shutoff
boolean mbath_flag ;
unsigned long powder_clock ;
boolean powder_flag ;
unsigned long bsmt_clock ;
boolean bsmt_flag ;
unsigned int off_time = 420000 ;  // 420,000 is 7 minutes in msecs

// When the fan is switched by the light timers, it is generally switched
// on 2 minutes after the light is turned on and is switched off 5 minutes
// after the light is turned off. The fans can be manually turned off with
// the fan switch. The light algorithm uses the same countdown timers and
// timers are running flags as are used in the switch algorithm.
int prev_mbath_lt = B00001000 ;      // previous state of the shower lights,
int prev_bsmt_lt =  B00100000 ;      // which are assumed to be off.
unsigned int mbath_time_on  ;    // time when the shower light switched on
unsigned int bsmt_time_on   ;
boolean mbath_lt_flag = false ;  // master shower lights timer is running
boolean bsmt_lt_flag = false  ;
unsigned int lt_off_time = 300000 ; // 300,000 is 5 minutes in msecs
unsigned int lt_delay = 120000 ;    // delay for fan to turn on

unsigned int loop_start ; // timer for the "I'm OK" blinker

// ---------------SETUP---------------------  
void setup() {

  pinMode(led, OUTPUT) ;    // for test only
  digitalWrite(led, LOW);   // turn the LED off by making the voltage LOW
  
  // include IcC library and set up ports 8 & 9 for temperature measaurement
  
  DDRD = DDRD & B00000011;  // sets Port D bits 2-7 to input
  /*
  Bits 0 & 1 are reserved for serial comm
  Bit 2 = Masteer Bath Fan Switch
  Bit 3 = Master Bath Shower Light
  Bit 4 = Basesment Shower Fan Switch
  Bit 5 = Basement Shower Light
  Bit 6 = Powder Room Fan Switch
  Bit 7 = Isolation Door
  */
  digitalWrite(2, HIGH) ; // turn on the input pullup resistors
  digitalWrite(3, HIGH) ;
  digitalWrite(4, HIGH) ;
  digitalWrite(5, HIGH) ; 
  digitalWrite(6, HIGH) ;

  DDRB = DDRB | B00011100;  // sets Port B bits 2-4 to output
  PORTB = PORTB | B00010000 ; // Turn HRV off
  /*
  Bits 0 & 1 are I2C for temperature measurement
  Bit 2 = Main Fan
  Bit 3 = Main Damper
  Bit 4 = HRV on
  */
  DDRC = DDRC | B00001111;    // sets Port C bits 0-3 to output
  PORTC = PORTC | B00001111 ; // initialize all fans to off
  /*
  Bit 0 = Master Bath Fan
  Bit 1 = Powder Room Fan
  Bit 2 = Basement Bath Fan
  Bit 3 = Gym Fan and Damper
  */
  DDRC = DDRC & B11001111;  // sets Port C bits 4-5 to input
  /*
  Bit 4 = HRV ventillation on/off
  Bit 5 = Temp Balancing Function on/off
  */
  digitalWrite(18, HIGH) ; // turn on the input pullup resistors
  digitalWrite(19, HIGH) ;

  delay(100) ;              // Wait 0.1 seconds at start
  blinker(150) ;            // three quick blinks show cpu is OK
  blinker(150) ;
  blinker(150) ;
  delay (100) ;
  
  Serial.begin(9600) ; 
  Serial.println(ver) ;       // send software version
  Serial.println(chan1) ;
  Serial.println(chan2) ; 
  Serial.println(" ") ; 
  Serial.println(" ") ; 
}

// -------------End of Setup---------------


// ------------Main Program Loop------------
void loop() {
  loop_start = millis() ;
  do {
//    check_clocks() ;
//    if(switch_query()) {      // is any switch line at logic 0?
//      if(valid_switch()) {    // a valid switch activation waits here for release
//        switch_action() ; } } // yes, it's valid; change the light control words 
     } while(millis() - loop_start < 2000 ) ; // 2 seconds between blinker
   blinker(50) ;   
   }                          // blink every 2 seconds; end of LOOP
  
// -----------End Main Program Loop---------------

// ------------------------------------------------
void blinker(int a) {       // blink for "2a" msec
  delay(a / 2) ;            // ensure there is a blink
  digitalWrite(led, HIGH);  // turn the LED on (HIGH is the voltage level)
  delay(a);                 // wait for "a" msec
  digitalWrite(led, LOW);   // turn the LED off by making the voltage LOW
  delay(a / 2) ; }          // ensure there is a blink, end of blinker
// -----------------------------------------------

I like to use the 'uint**_t' notation so I know what I'm getting. Many of my sketches that started out on a 328P board end up on a Teensy 3.2 or 3.6.

unsigned int loop_start ; // timer for the "I'm OK" blinker

There it is!

Dope slap here! I caught it as you guys were replying. Thanks to all.

J-M-L, I loaded your code and it runs per your experience. In a pro mini.

unsigned int off_time = 420000 ; // 420,000 is 7 minutes in msecs

unsigned int lt_off_time = 300000 ; // 300,000 is 5 minutes in msecs
unsigned int lt_delay = 120000 ; // delay for fan to turn on

and these ones too!

On a Teensy 3.2/3.6, it probably would have worked OK.

loop_start is 16 bits. It’s max value is 65536. millis returns 32 bits. Its max value is much bigger.

This means that millis()<loop_start will always be > 2000 once the real time hits 67.5 seconds.

To fix: always use unsigned long or uint32_t for timing.

Other fix: truncate the value of millis()

if(((unsigned int)millis()) - loop_start > 2000)

This allows you to time small intervals without having to hold a full four bytes. Not usually a problem, but if it ever is - this is how you deal with it.

Oh, and I always like to suffix my variable names with unit of measure: loop_start_ms or loop_start_us if I am using micros().

Questions to forum, if I may ask.

If there is a fixed time interval, as in this case, and the app will run 24/7/ 365 wouldn’t using software defined time work better?
For example using “for” loop ?

Assuming accuracy is not an issue of course.

Of course there are other options - hardware timers , WDT.

Jim

Millis and micros are pretty accurate ways to do this with a few micros/millis seconds accuracy due to testing and looping. Assuming your loop is fast of course

If you need sub micro second accuracy, the standard 328 or 2560 arduinos are not the right platform.

If you want to do an active wait, then delay and delaymicrosecond are good tools. I suggest studying the code of delaymicrosecond to see how at the core of the function they use asssembly language level understanding of what time instructions are taking to generate the right delay

millis() is not highly accurate, note, it will sometimes jump by 2 units, not 1. micros() is as
accurate as software can be though.

delayMicroseconds (n) doesn't work if the value of n is too large, and this often catches people out,
I would suggest not trying to pass it a value larger than 8000 to be on the safe side (currently I think
the fail point is 16384, on Uno and many other boards, but best to check).

julyjim:
If there is a fixed time interval, as in this case, and the app will run 24/7/ 365 wouldn't using software defined time work better?
For example using "for" loop ?

What does the arduino do between loops? Well - it looks at the USB connections and checks for a new sketch coming down the pipe. You can write a loop that doesn't terminate, you can write your own main(), but my understanding is that if you do, you'll need to reset to upload a new sketch. It may also do other things during that pause that are important.

PaulMurrayCbr:
What does the arduino do between loops? Well - it looks at the USB connections and checks for a new sketch coming down the pipe. You can write a loop that doesn't terminate, you can write your own main(), but my understanding is that if you do, you'll need to reset to upload a new sketch. It may also do other things during that pause that are important.

Which Arduino are you referring to? The AVR Boards main() is(Arduino/main.cpp at 1.6.12 · arduino/Arduino · GitHub):

int main(void)
{
 init();

 initVariant();

#if defined(USBCON)
 USBDevice.attach();
#endif
 
 setup();
    
 for (;;) {
 loop();
 if (serialEventRun) serialEventRun();
 }
        
 return 0;
}

So if you don't ever return from loop() run the only thing you miss is that idiotic serialEventRun() thing which boils down to something like:

if (Serial.available()) {
  serialEvent();
}

Don't ask me why Arduino considered that to be an improvement over just putting that code in your sketch, it's less beginner friendly, and slows down/wastes memory on the 99.99% of sketches that don't use serialEvent().

On the AVR boards, sketch upload is initiated by the USB-serial chip pulling the MCU's reset line low(or a serial port being opened/closed at 1200 baud on the ATmega32U4 boards), which causes the bootloader to run, there's no need for the MCU to monitor for an upload while the user's application is running and not returning from loop() will have no effect on your ability to upload sketches.

julyjim:
Questions to forum, if I may ask.

If there is a fixed time interval, as in this case, and the app will run 24/7/ 365 wouldn't using software defined time work better?
For example using "for" loop ?

Assuming accuracy is not an issue of course.

Of course there are other options - hardware timers , WDT.

Jim

I assumed, wrongly as always, the outcome of the OP was overflowing variable.
That is often overlooked in types of apps I also specified.
Not sure how timing accuracy became an issue and "for" loop got interpreted as Arduino loop function.
Sorry to be bother.
Jim