Delay() doesn't work after Interrupt Service Routine - nointerrupts()

Using Arduino Programming Questions
#1

I have two PWM fans where I read their RPM with an Interrupt Service Routine. My script is based on this tutorial (Reading PC Fan RPM with an Arduino).

This works as it should, the fans' RPM are calculated correctly. However, after I have calculated the fans' RPM I would like to delay the loop for 60 sec but the delay() function doesn't seems to work.

Any suggestions on how to get this to work are greatly appreciated.

I have read Gammon's post on interrupts, but I still don't understand what I'm doing wrong.

Please let me know if you need more information.

My code:

const int fan1PwmPin = 5;
const int fan1RpmPin = 2;

const int fan2PwmPin = 9;
const int fan2RpmPin = 3;

volatile int fan1Hz;
volatile int fan2Hz;

int fan1Rpm; 
int fan2Rpm; 

int pwmVal[] = {90, 90};

const int debugMode = 0;

long delayTime = 60000;

void getFan1Hz(void){ 
  fan1Hz++; 
}

void getFan2Hz(void){ 
  fan2Hz++; 
}

void setup(void) {
  Serial.begin(9600);

  pinMode(fan1PwmPin, OUTPUT);
  pinMode(fan1RpmPin, INPUT);
  analogWrite(fan1PwmPin, pwmVal[0]);

  pinMode(fan2PwmPin, OUTPUT);
  pinMode(fan2RpmPin, INPUT);
  analogWrite(fan2PwmPin, pwmVal[0]);

  attachInterrupt(0, getFan1Hz, RISING);
  attachInterrupt(1, getFan2Hz, RISING);
}

void loop(){

   // Resetting the Hz counts
  fan1Hz = 0;
  fan2Hz = 0;

  // Counting the Hz of both fans
  interrupts();
  delay(1000);
  noInterrupts();

  // Calculating the fans' RPM
  fan1Rpm = fan1Hz / 2 * 60;
  fan2Rpm = fan2Hz / 2 * 60;

  Serial.println(fan1Hz);
  Serial.println(fan1Rpm);
  Serial.println(fan2Hz);
  Serial.println(fan2Rpm);
  Serial.println("------");

  // if running in debug mode then set the delay time to 1 sec
  if (debugMode == 1) {
    delayTime = 1000;
  }
  
  delay(delayTime);
  
}
#2

Are you aware of the techniques found in the "Blink Without Delay" sketch that is found in the examples?

#3

What happens if you change the attachInterrupt() to CHANGE?

#4

What is happening here?

interrupts();
  delay(1000);
  noInterrupts();
#5

You have the interrupts() statement in the wrong place. For fairly obvious reasons it should always FOLLOW noInterrupts()

 // Counting the Hz of both fans
  // interrupts();  //==============not here
  delay(1000);
  noInterrupts();

  // Calculating the fans' RPM
  fan1Rpm = fan1Hz / 2 * 60;
  fan2Rpm = fan2Hz / 2 * 60;
  interrupts();   //===============It should be here

It would also be better to save the rpm values to other variables for general use. For example

noInterrupts()
  newFan1 = fan1Hz;
  newFan2 = fan2Hz;
interrupts();
fan1Rpm = newFan1 * 30;
etc

...R

#6

You have the interrupts() statement in the wrong place. For fairly obvious reasons it should always FOLLOW noInterrupts()

Sorry, no, you are wrong.

That method of enabling the interrupts for a period of time, and then disabling them, is correct. It is NOWHERE near best practice.

(In any case, since loop() is called over and over, interrupts() DOES follow noInterrupts().)

#7

delay() requires interrupts from Timer 0 to work, and you are entering the body of the loop after noInterrupts() which has disabled the interrupts. Serial printing also requires interrupts to be enabled.

Using delay to collect interrupt counts for one second is not good practice and you should follow Larry D's suggestion of using the "blink without delay" template.

volatile unsigned int  count = 0;

unsigned int copyCount = 0;
unsigned int rpm = 0;

unsigned long lastRead = 0;

void setup()
{
  Serial.begin(115200);
  Serial.println("start...");

  attachInterrupt(0, isrCount, RISING); //interrupt signal to pin2
}

void loop()
{
  if (millis() - lastRead >=1000) //read interrupt count every second
  {
    lastRead = millis();

    // disable interrupts,make copy of count, re-enable interrupts
    noInterrupts();
    copyCount = count;
    count = 0;
    interrupts(); 
   

    rpm = copyCount*60;
    Serial.println(rpm);
  }
}

void isrCount()
{
  count++;
}
#8

PaulS: Sorry, no, you are wrong.

Prompted by your comment and after some more thought I see where the OP is coming from - he just wants the interrupts to work for 1 second.

It had not occurred to me to do it like that.

I would let the interrupts run continuously and read the values every second. But then I would not think of using delay() in my code.

And the OP cannot use Serial.print() while interrupts are off.

...R

#9

Thanks for all of your replies I really appreciate it.

cattledog: delay() requires interrupts from Timer 0 to work, and you are entering the body of the loop after noInterrupts() which has disabled the interrupts. Serial printing also requires interrupts to be enabled.

Using delay to collect interrupt counts for one second is not good practice and you should follow Larry D's suggestion of using the "blink without delay" template.

I as suggested by both cattledog and Larry D I have made the code based on the "blink without delay" template, which is working fine.

Maybe this is a newbie question, but what if I want to execute more code in the loop than just counting the RPM. This could for example be to change the PWM value of the fans, getting the temperature from a sensor and so on. I guess that then the time could easily exceed 1 sec? Or am I wrong? 

const int fan1PwmPin = 5;
const int fan1RpmPin = 2;

const int fan2PwmPin = 9;
const int fan2RpmPin = 3;

volatile unsigned int fan1Hz;
volatile unsigned int fan2Hz;

unsigned int copyFan1Hz = 0;
unsigned int copyFan2Hz = 0;

unsigned int fan1Rpm; 
unsigned int fan2Rpm;

int pwmVal[] = {90, 90};

const int debugMode = 0;

unsigned long previousMillis = 0;

const long interval = 1000;

void getFan1Hz(void){ 
  fan1Hz++; 
}

void getFan2Hz(void){ 
  fan2Hz++; 
}

void setup(void) {
  Serial.begin(9600);

  pinMode(fan1PwmPin, OUTPUT);
  pinMode(fan1RpmPin, INPUT);
  analogWrite(fan1PwmPin, pwmVal[0]);

  pinMode(fan2PwmPin, OUTPUT);
  pinMode(fan2RpmPin, INPUT);
  analogWrite(fan2PwmPin, pwmVal[0]);

  attachInterrupt(0, getFan1Hz, RISING);
  attachInterrupt(1, getFan2Hz, RISING);
}

void loop(){

  unsigned long currentMillis = millis();
        // Resetting the Hz counts

  if(currentMillis - previousMillis >= interval){
      previousMillis = currentMillis;

      noInterrupts();
      copyFan1Hz = fan1Hz;
      copyFan2Hz = fan2Hz;
      interrupts();

      fan1Hz = 0;
      fan2Hz = 0;
  }

  // // Do more stuff here e.g. changing the PWM signal to the fans or reading temperature sensors
  fan1Rpm = copyFan1Hz / 2 * 60;
  fan2Rpm = copyFan2Hz / 2 * 60;
  Serial.println(copyFan1Hz);
  Serial.println(fan1Rpm);
  Serial.println(copyFan2Hz);
  Serial.println(fan2Rpm);
  Serial.println("------");
  
}
#10 
  // // Do more stuff here e.g. changing the PWM signal to the fans or reading temperature sensors
  fan1Rpm = copyFan1Hz / 2 * 60;
  fan2Rpm = copyFan2Hz / 2 * 60;
  Serial.println(copyFan1Hz);
  Serial.println(fan1Rpm);
  Serial.println(copyFan2Hz);
  Serial.println(fan2Rpm);
  Serial.println("------");

This code is being executed as fast as possible for no reason, this belongs inside the previous if statement...

Try this code

void loop(){

  unsigned long currentMillis = millis();
        // Resetting the Hz counts

  if(currentMillis - previousMillis >= interval){
      previousMillis = currentMillis;

      noInterrupts();
      copyFan1Hz = fan1Hz;
      copyFan2Hz = fan2Hz;
      fan1Hz = 0;
      fan2Hz = 0;
      interrupts();
      // Do more stuff here e.g. changing the PWM signal to the fans or reading temperature sensors
      fan1Rpm = copyFan1Hz / 2 * 60;
      fan2Rpm = copyFan2Hz / 2 * 60;
      Serial.println(copyFan1Hz);
      Serial.println(fan1Rpm);
      Serial.println(copyFan2Hz);
      Serial.println(fan2Rpm);
      Serial.println("------");
  }
  
}

The question you asked above: The problem is that you are not counting your RPMs accurately, you are just asking the processor to execute that code when it gets to it. This can be inaccurate. What you will want to do instead is use a timed interrupt and calculate your RPMs in there.

Here is a simple Timer Interrupt Example...

void setup() {
  int frequency = 50; // in hz
  //Interupt Service Routine and timer setup
  noInterrupts();// kill interrupts until everybody is set up
  //We use Timer 1, its the only 16 bit timer
  TCCR1A = B00000000;//Register A all 0's since we're not toggling any pins
    // TCCR1B clock prescalers
    // 0 0 1 clkI/O /1 (No prescaling)
    // 0 1 0 clkI/O /8 (From prescaler)
    // 0 1 1 clkI/O /64 (From prescaler)
    // 1 0 0 clkI/O /256 (From prescaler)
    // 1 0 1 clkI/O /1024 (From prescaler)
  TCCR1B = B00001011;//bit 3 set for CTC mode, will call interrupt on counter match, bits 0 and 1 set to divide clock by 64, so 16MHz/64=250kHz
  TIMSK1 = B00000010;//bit 1 set to call the interrupt on an OCR1A match
  OCR1A  = (unsigned long)((250000UL / frequency) - 1UL);//our clock runs at 250kHz, which is 1/250kHz = 4us
  interrupts();//let the show begin, this lets the multiplexing start
}

void loop() {
  
}

ISR(TIMER1_COMPA_vect){ //Interrupt Service Routine, Timer/Counter1 Compare Match A
  
}
#11

Robin2: You have the interrupts() statement in the wrong place. For fairly obvious reasons it should always FOLLOW noInterrupts()

It would also be better to save the rpm values to other variables for general use. For example

noInterrupts()
  newFan1 = fan1Hz;
  newFan2 = fan2Hz;
interrupts();
fan1Rpm = newFan1 * 30;
etc

...R

I followed your advice, Robin2, and it worked. Putting the noInterrupt() before reading the rpm and then re-enabeling them again afterwards did the trick.

void loop(){

  // Resetting the counts before counting for one sec
  fan1Hz = 0;
  fan2Hz = 0;

  delay(1000);

  noInterrupts();

  copyFan1Hz = fan1Hz;
  copyFan2Hz = fan2Hz;

  interrupts();
  
  fan1Rpm = copyFan1Hz / 2 * 60;
  fan2Rpm = copyFan2Hz / 2 * 60;
  
  delay(10000);

}

And as also mentioned by cattledog delay() requires interrupts and in my previous example I had disabled interrupts for the rest of the loop, which was the reason that delay() was ignored.

But that said, it sounds that I should avoid using delay(1000) to calculate the fans' RPM and instead use the "blink without delay" template? Is that right?

#12

Thanks Ps991 - it looks rather advanced, but I will differently look into it. Just so I get it right - if I use your timer example then I don't need to use the "blink without delay" template?

Ps991: The question you asked above: The problem is that you are not counting your RPMs accurately, you are just asking the processor to execute that code when it gets to it. This can be inaccurate. What you will want to do instead is use a timed interrupt and calculate your RPMs in there.

Here is a simple Timer Interrupt Example...

void setup() {
  int frequency = 50; // in hz
  //Interupt Service Routine and timer setup
  noInterrupts();// kill interrupts until everybody is set up
  //We use Timer 1, its the only 16 bit timer
  TCCR1A = B00000000;//Register A all 0's since we're not toggling any pins
    // TCCR1B clock prescalers
    // 0 0 1 clkI/O /1 (No prescaling)
    // 0 1 0 clkI/O /8 (From prescaler)
    // 0 1 1 clkI/O /64 (From prescaler)
    // 1 0 0 clkI/O /256 (From prescaler)
    // 1 0 1 clkI/O /1024 (From prescaler)
  TCCR1B = B00001011;//bit 3 set for CTC mode, will call interrupt on counter match, bits 0 and 1 set to divide clock by 64, so 16MHz/64=250kHz
  TIMSK1 = B00000010;//bit 1 set to call the interrupt on an OCR1A match
  OCR1A  = (unsigned long)((250000UL / frequency) - 1UL);//our clock runs at 250kHz, which is 1/250kHz = 4us
  interrupts();//let the show begin, this lets the multiplexing start
}

void loop() {   }

ISR(TIMER1_COMPA_vect){ //Interrupt Service Routine, Timer/Counter1 Compare Match A   }

It is not that the RPM needs to be extremely accurate, but in the "blink without delay" example bellow reading the temperature from the sensor might take 0.5 sec, which will make the RPM calculation rather inaccurate. 

void loop(){

  unsigned long currentMillis = millis();
        // Resetting the Hz counts

  if(currentMillis - previousMillis >= interval){
      previousMillis = currentMillis;

      noInterrupts();
      copyFan1Hz = fan1Hz;
      copyFan2Hz = fan2Hz;
      interrupts();

      fan1Hz = 0;
      fan2Hz = 0;

      // Do more stuff here e.g. changing the PWM signal to the fans or reading temperature sensors

  }

  
}
#13

Just another question about the "Blink without delay" method after have reading up on it. In the documentation of millis() it says that after 50 days it will go back to zero. In such a case, I guess it will not work?

void loop(){

  unsigned long currentMillis = millis();
        // Resetting the Hz counts

  if(currentMillis - previousMillis >= interval){
      previousMillis = currentMillis;

      noInterrupts();
      copyFan1Hz = fan1Hz;
      copyFan2Hz = fan2Hz;
      interrupts();

      fan1Hz = 0;
      fan2Hz = 0;

      // Do more stuff here e.g. changing the PWM signal to the fans or reading temperature sensors

  }

  
}
#14

In such a case, I guess it will not work?

Your watch goes back to 0 twice a day. Is it broken?

#15

PaulS: Your watch goes back to 0 twice a day. Is it broken?

Good point PaulS, but if previousMillis ends up higher than currentMillis, which after 50 days, will become 0 then the IF block will not be executed, or am I wrong?

#16

Wrong.

Please read this: http://www.gammon.com.au/forum/?id=12127

.

#17

Also this: http://www.gammon.com.au/forum/?id=11488&reply=10#reply10

The concept of turning interrupts on just to time an interval is not recommended for reasons explained there.

#18

I don't think "blink without delay" really applies here. (I never liked that way of doing things anyway, since there are better ways of handling concurrency like using interrupts or even some of the scheduler libraries)

but anyway, this is a very simple task and there is no need for that kind of complexity.

All that is needed is some careful interrupt masking while you clear and collect the counters. What you have is very close.

i.e. here is some psuedo logic for the loop() function:

  • disable interrupts
  • clear your global timer counters
  • enable interrupts
  • delay your 1 second
  • disable interrupts
  • copy your global counters to local versions of the counters. (this is new and VERY important)
  • enable interrupts
  • calculate the RPM using the local counters and not the global counters (this is also VERY important)

So essentially what you need is way to create some atomicity around the collection of your counter data. You need to have local versions of the counters so you can snap shot them after your delay. The globals will start incrementing again after you enable interrupts, but that doesn't matter since you grabbed the values immediately after the delay and put them in locals.

You need to create 2 new local counters to use and then use those local values for saving your global counters for your calculations.

-- bill

#19

runevn: but if previousMillis ends up higher than currentMillis, which after 50 days, will become 0 then the IF block will not be executed, or am I wrong?

If you calculate the elapsed millis like this

if (millis() - prevMillis >= reqdInterval)

it will give the correct value even when the value of millis() rolls over - thanks to the magic of binary integer maths.

The use of millis() is illustrated in several things at a time.

If this was my project I would use millis() to figure out when every second has elapsed and then read the counter that the interrupt has been incrementing. Something like

void loop() {
   if (millis() - prevMillis >= interval) {
     prevFan1Hz = curFan1Hz;
     prevFan2Hz = curFan2Hz;
     noInterrupts();
     curFan1Hz = fan1Hz;
     curFan2Hz = fan2Hz;
    interrupts();
    prevMillis += interval;
    fan1Rpm = (curFan1Hz - prevFan1Hz) / 2 * 60;
    // etc
}

...R

#20

I think that unless there is other things to do in loop(), i'd take the simpler approach of just adding atomicity of reading the ISR counters. like this: (untested)

const int fan1PwmPin = 5;
const int fan1RpmPin = 2;

const int fan2PwmPin = 9;
const int fan2RpmPin = 3;

volatile int ISRfan1Hz;
volatile int ISRfan2Hz;

int fan1Rpm;
int fan2Rpm;

int pwmVal[] = {90, 90};

const int debugMode = 0;

long delayTime = 60000;

void getFan1Hz(void){
  ISRfan1Hz++;
}

void getFan2Hz(void){
  ISRfan2Hz++;
}

void setup(void) {
  Serial.begin(9600);

  pinMode(fan1PwmPin, OUTPUT);
  pinMode(fan1RpmPin, INPUT);
  analogWrite(fan1PwmPin, pwmVal[0]);

  pinMode(fan2PwmPin, OUTPUT);
  pinMode(fan2RpmPin, INPUT);
  analogWrite(fan2PwmPin, pwmVal[0]);

  attachInterrupt(0, getFan1Hz, RISING);
  attachInterrupt(1, getFan2Hz, RISING);
}

void loop(){
int fan1Hz, fan2Hz;

   // Resetting the ISR Hz counts
  noInterrupts();
  ISRfan1Hz = 0;
  ISRfan2Hz = 0;
  interrupts();

  // delay while counting the Hz of both fans
  delay(1000);

  // get local copies of the ISR Hz counts
  noInterrupts();
  fan1Hz = ISRfan1Hz;
  fan2Hz = ISRfan2Hz;
  interrupts();


  // Calculating the fans' RPM
  fan1Rpm = fan1Hz / 2 * 60;
  fan2Rpm = fan2Hz / 2 * 60;

  Serial.println(fan1Hz);
  Serial.println(fan1Rpm);
  Serial.println(fan2Hz);
  Serial.println(fan2Rpm);
  Serial.println("------");

  // if running in debug mode then set the delay time to 1 sec
  if (debugMode == 1) {
    delayTime = 1000;
  }

  delay(delayTime);

}