Problems with micros() function

I am having issues using the micros() function with

the nano 33 BLE.

Below is the data on the version of IDE I am using:

Version: 2.1.0

Date: 2023-04-19T15:31:10.185Z

CLI Version: 0.32.2

Copyright © 2023 Arduino SA

My sketch is very involved.

Therefore, I have submitted a sketch which shows the

problem with minimal code.

What I am basically trying to implement is an off-delay timer

to be used as a screen saver to prevent damage to my OLEDs.

The falling edge of PB(pin 4) triggers the ISR.

It sets the “SCREEN_SAVER_ELAPSED_MICROS” =0.

This is the current value.

This variable then counts up and should turn off the

LED after ten seconds. (10000000)

Pulsing PB resets ELAPSED_MICROS=0.

The problem I am having is that the LED stays on for about 14.4 sec.

To make things even more confusing, if I unremark the delay(1)

on line 57 the time is pretty close.(10 sec)

Any code I add (print,display,etc) changes the timing error.

So I can’t really troubleshoot using the monitor.

I have scope pictures if interested.

There doesn’t seem to be any contact bounce on the PB.

I’m sure I’m doing something wrong but don’t have a clue.

Please don’t ask me to try millis().

Here is the code:

Thanks,

Larry

//2023-06-21
//Minimal Code test micros() problem



#define  irq_pin         4

#define LEDPIN 10
#define TONEPIN 9  // Used if you want audio via a small buzzer



uint32_t       SCREEN_SAVER_ELAPSED_MICROS;
const uint32_t SCREEN_SAVER_THR_MICROS = 10000000;   //10 sec threshold
uint32_t       SCREEN_SAVER_OLD_MICROS = 0;
uint32_t       SCREEN_SAVER_DELTA_MICROS;
bool           SCREEN_SAVER_ACTIVE;

void setup() {

  // put your setup code here, to run once:
pinMode(4, INPUT_PULLUP);        //IRQ PIN


pinMode(10,OUTPUT);              //LED 4



// ========================= Global data definitions =====================


  Serial.begin(57600);
  attachInterrupt(irq_pin, ps2interrupt, FALLING);

} //end setup


// The ISR for the external interrupt
void ps2interrupt()   //driven by PB
{
 
  digitalWrite(10,HIGH); // Turn on L.E.D.
  SCREEN_SAVER_ELAPSED_MICROS=0; //resets screen saver
	}
  


void loop()
{
       
     SCREEN_SAVER();  //DO  screen saver calculations

}  //END of loop


void SCREEN_SAVER(){
  //delay(1);

if (SCREEN_SAVER_ELAPSED_MICROS >SCREEN_SAVER_THR_MICROS ){
     SCREEN_SAVER_OLD_MICROS = micros(); //keep track of micros even when screen save is active
     return;  //don't do calculatioins if already in saver mode
     
}

if(SCREEN_SAVER_ACTIVE & (SCREEN_SAVER_ELAPSED_MICROS==0 ) ) {  //SCREEN_SAVER_ELAPSED_MICROS SET=0 BY ISR
  SCREEN_SAVER_ACTIVE = false;
  return;
}

//ACCUMALATE TIME
if (micros()>SCREEN_SAVER_OLD_MICROS) {
    (SCREEN_SAVER_DELTA_MICROS = micros() - SCREEN_SAVER_OLD_MICROS );
    (SCREEN_SAVER_ELAPSED_MICROS = SCREEN_SAVER_ELAPSED_MICROS + SCREEN_SAVER_DELTA_MICROS);
    (SCREEN_SAVER_OLD_MICROS = micros()  );
    }

if(SCREEN_SAVER_ELAPSED_MICROS >SCREEN_SAVER_THR_MICROS ) {
  SCREEN_SAVER_ACTIVE = true;
  digitalWrite(10,LOW); //
  return;
}

} // end of SCREEN_SAVER()


  

That needs to be volatile if it changes in an ISR. Since it is multi-byte it also needs critical sections when you read from it or you will have sporadic bugs and corrupted data.

For a 10s delay why use micros()? millis() would be better and you only need to count 10,000 of them. Also I'm not sure quite how accurate micros() is though it might not be significant for you.

I'm not going to try and figure out the logic of your SCREEN_SAVED() function, just want to comment that it would be much easier to save the current value of micros() in the ISR, then use

if ((micros() - SCREEN_SAVER_ELAPSED_MICROS) >= 10000000)

to test if 10 seconds have elapsed.

I'm not sure what you are using to trigger the ISR, but if it is a switch it would probably work fine to just poll the switch in the code and eliminate the complications of using an interrupt.

In the real project the clock pulse from a keypoard is
triggering the interrupt.

Thanks for your time.
Larry

I need to use an interrupt because in the real project the clock
pulse from the PS2 keyboard is triggering the calculations.

Thanks,
Larry

Delta_G,
Unfortunately changing to volatile did not work.
I even set a bit in the ISR and used it in SCREEN_SAVER() to
reset the elapsed counts.
That did not work either.
Thanks for your thoughts.
I was sure that that was the problem.
What are "critical sections" and how do I get me some?

Thanks,
Larry

jhaine,
Thanks for the reply.

Are you saying there is a bug in the micros() function?

Later,
Larry

Not at all. Just that measuring a non-critical 10s delay with something with a nominal resolution of 1us is like measuring 10km with a 1mm long yardstick. The millis function will do the job just fine. I'm not sure about the nano33BLE but on the Uno and Nano also the granularity of micros() is also 4 usec.

SCREEN_SAVER_ELAPSED_MICROS is a 32 bit variable. When you read from it, it takes more than one clock cycle to read it. It is possible that you read one or two of the bytes and then and interrupt occurs and changes the value while you are reading it. Then you end up with the top two bytes of one value and the bottom two bytes of a different value or something. This leads to a corrupted value.

It is important when you read a multi-byte variable that can change like that in a critical section. That means you turn off interrupts, read your variable, and then turn them back on. Usually you want to do that quickly so what you do is kill the interrupts, make a copy of the variable, and then turn interrupts back on and use the copy.

Eveywhere SCREEN_SAVER_ELAPSED_MICROS is read or modified the interrupts need to be turned off while it happens.

What is this code going to actually do? I mean what is the big picture of what you're building? Why do you need ten seconds to the microsecond?

I didn't know if I should create a new topic or add to this one.

After some investigation I've discovered that both millis() and micros() exhibit
the same problem.

In the attached code line 48 is delay(1).

If that line is included both millis() & micros() time correctly.
If it is not included both millis() & micros() time too long by about (2x).
Actually 3.92 sec vs 2.0 sec.

This was measured by a scope.

The interrupt is gone.

The extra method is gone.

Everything is done in "loop()".

Why is the timing wrong?

Thanks,
Larry

//2023-06-21
//Minimal Code test micros() problem





#define LEDPIN 10




volatile uint32_t       SCREEN_SAVER_ELAPSED_MICROS ;


const uint32_t          SCREEN_SAVER_THR_MICROS = 1000000;   //1.00 sec threshold, therefore 2 sec waveform period
uint32_t                SCREEN_SAVER_OLD_MICROS = 0;
uint32_t                SCREEN_SAVER_DELTA_MICROS = 0;
uint32_t                DUMMY;   //used to add meaningless processor steps


void setup() {

  // put your setup code here, to run once:



pinMode(10,OUTPUT);              //LED 4



// ========================= Global data definitions =====================


  Serial.begin(57600);
 

} //end setup




void loop()
{
//delayMicroseconds(5);


   delay(1);  

//ACCUMALATE TIME

    (SCREEN_SAVER_DELTA_MICROS = micros() - SCREEN_SAVER_OLD_MICROS );
    (SCREEN_SAVER_ELAPSED_MICROS = SCREEN_SAVER_ELAPSED_MICROS + SCREEN_SAVER_DELTA_MICROS);
    (SCREEN_SAVER_OLD_MICROS = micros()  );
  

if(SCREEN_SAVER_ELAPSED_MICROS >SCREEN_SAVER_THR_MICROS ) {
  digitalWrite(10,!digitalRead(10)); //
  SCREEN_SAVER_ELAPSED_MICROS = 0;
}
    

 
  
}  //END of loop






  

Looks to be related to calling micros() twice in these lines of code, the slight difference in value for micros() is causing you to lose time. The 1mS delay greatly decreases how many time this code gets executed, so minimizes the accumulated error. If you set a variable to the value of micros(), then use that in these lines of code, the timing error goes away.

    (SCREEN_SAVER_DELTA_MICROS = micros() - SCREEN_SAVER_OLD_MICROS );
    (SCREEN_SAVER_ELAPSED_MICROS = SCREEN_SAVER_ELAPSED_MICROS + SCREEN_SAVER_DELTA_MICROS);
    (SCREEN_SAVER_OLD_MICROS = micros()  );

I'll ask again, why not just use the conventional method of millis / micros timing, instead of the rather convoluted method you are using?

#define LEDPIN 10

uint32_t       SCREEN_SAVER_ELAPSED_MICROS ;

const uint32_t          SCREEN_SAVER_THR_MICROS = 1000000;   //1.00 sec threshold, therefore 2 sec waveform period

void setup() {
  // put your setup code here, to run once:
  pinMode(10, OUTPUT);             //LED 4
  // ========================= Global data definitions =====================
  Serial.begin(57600);
  SCREEN_SAVER_ELAPSED_MICROS = micros();
} //end setup

void loop()
{
  if ((micros() - SCREEN_SAVER_ELAPSED_MICROS) >= SCREEN_SAVER_THR_MICROS ) {
    digitalWrite(10, !digitalRead(10)); //
    SCREEN_SAVER_ELAPSED_MICROS += SCREEN_SAVER_THR_MICROS; //increments target time by exactly SCREEN_SAVER_THR_MICROS
    //Serial.println(millis());
  }
}  //END of loop

I don't understand what your "accumulate time" code is doing, or where you got that something should be two seconds, which the code has 1e6 microseconds as its constant.

micros() can be quite slow on mbed-based systems, because of all the layering involved in using the OS (on an rp2040, a call to micros() takes about 4us!) That could cause problems if not accounted for (ie micros() - micros() will give about 4, but takes 8us.)
I'm not sure how the 33BLE compares to an rp2040.

    (SCREEN_SAVER_DELTA_MICROS = micros() - SCREEN_SAVER_OLD_MICROS );
    (SCREEN_SAVER_ELAPSED_MICROS = SCREEN_SAVER_ELAPSED_MICROS + SCREEN_SAVER_DELTA_MICROS);
    (SCREEN_SAVER_OLD_MICROS = micros()  );

The first line is calculating the number of microseconds since the value of micros() was saved in the last iteration of loop().
The second line adds this amount to the accumulated time interval.
The last line saves the current micros() for use in the next loop.

The problem the OP is currently having is caused by the amount of time between the first line of code and the third line. There appears to be slightly less than 50% chance for the incrementing of the micros count to occur during this time, resulting in time passing but not being included in the accumulated time interval.

David,
You are a GENIUS!
I never would have thought that one instruction cycle would have made that much difference.
Your solution also solved the problem using millis().

How do I flag this topic as SOLVED?

Thanks again,
Larry

David,
I did not know there was a conventional method.
Where can I research it?

Thanks again,
Larry

David,
In my old life I spent much time programing industrial controlers.
Their operating system would load all global variables into a buffer
and use the buffered value. At the end of the scan it would then update
the global variables. It was called Load, Compute, Dump.

My mind was still working in the LoadComputeDump mode!
Thanks again,
Larry

It's one line of code. It's WAY more than one instruction.

You can look at the BlinkWithoutDelay example sketch, that is almost exactly what you are doing in your test code.

Delta_G,
You are 200% correct.
I'm sorry for posting incorrect info.
Thanks for the correction.
Larry