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.
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
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++;
}
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("------");
}
// // 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
}
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.
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?
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
}
}
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
}
}
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?
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.
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.
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
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);
}