A problem with Interrupts

Hi all,

I am not a programmer just a useless Electronics Technician (Hardware). Just wondering if anyone out there would be able to sort out a Program I put together the will display Temp of a Heatsink, the PWM output to the Cooling Fan and the RPM Reading from the Hall Effect Sensor in the Fan for a Power Supply.

The Temp & Fan PWM works fine (a little bit of tweaking left to do), but when I include the Enable & Disable Interrupt lines, the Display is Blank.

Monitoring just the HES works fine on the display. It only when I try to incorporate it with the rest of the Code is where the problem lies.

Here is the Code and Many Thanks to whoever can help

#include <Wire.h>
#include <LiquidCrystal_I2C.h>
LiquidCrystal_I2C lcd(0x27, 2, 1, 0, 4, 5, 6, 7, 3, POSITIVE);  // Set the LCD I2C address

int tempPin = A1;   // the output pin of LM35
int fans = 11;       // the pin where fan is
//int led = 8;        // led pin
int temp;
int tempMin = 20;   // the temperature to start the fan
int tempLow = 30;   // the temperature where fan increases speed
int tempMax = 70;   // the maximum temperature when fan is at 100%
int fanSpeed;
int fanLCD;

int NbTopsFan; //Varibles used for calculations
int Calc;
int hallsensor = 2; typedef struct{ //The pin location of the sensor
char fantype; //Defines the structure for multiple fans and their dividers 
unsigned int fandiv; }fanspec;
//int readTemp;
fanspec fanspace[3]={{0,1},{1,2},{2,8}}; char fan = 1; // 1 for unipole hall effect sensor or 2 for bipole hall effect sensor

void rpm ()
{ NbTopsFan++; } //This is the function that the interupt calls
 
void setup() {
  pinMode(fans, OUTPUT);
  //pinMode(led, OUTPUT);
  pinMode(tempPin, INPUT);
  lcd.begin(16,2);
  pinMode(hallsensor, INPUT);
  attachInterrupt(0, rpm, RISING); }
 
void loop() {  
   
   temp = readTemp();     // get the temperature
   if(temp < tempMin) { fanSpeed = 0; digitalWrite(fan, LOW); }
   if((temp >= tempMin) && (temp < tempLow)) { fanSpeed = 50; fanLCD = 20; }
   if((temp >= tempLow) && (temp <= tempMax)) { fanSpeed = map(temp, tempLow, tempMax, 12, 255); fanLCD = map(temp, tempLow, tempMax, 0, 100); analogWrite(fan, fanSpeed); } 
   if(temp > tempMax) { digitalWrite(fan, HIGH);  // turn on led 
   } // else { digitalWrite(led, LOW); 
   //}
   lcd.setCursor(0,0);
   lcd.print("TEMP");
   lcd.setCursor(0,1);
   lcd.print(temp);      // display the temperature
   lcd.print((char)0xdf);
   lcd.print("C");
   lcd.setCursor(6,0);   // move cursor to next line
   lcd.print("FANS");
   lcd.setCursor(6,1);
   lcd.print(fanLCD);    // display the fan speed
   lcd.print("%");
   lcd.setCursor(12,0);
   lcd.print("RPM");
   
   NbTopsFan = 0; //Set NbTops to 0 ready for calculations
   
   
  //sei(); //Enables interrupts
  //delay (1000);
  //cli(); //Disable interrupts
  
  
   Calc = ((NbTopsFan * 60)/fanspace[fan].fandiv);
   
   lcd.setCursor(12,1);
   lcd.print (Calc, DEC);
   delay(200);
   lcd.clear();   
}
 
int readTemp() {  // get the temperature and convert it to celsius
  temp = analogRead(tempPin);
  return temp * 0.48828125;
}

Regards Steve

int hallsensor = 2; typedef struct{ //The pin location of the sensor
char fantype; //Defines the structure for multiple fans and their dividers
unsigned int fandiv; }fanspec;

Arewesupposedtobeabletoreadthiscrap?

ONE statement per line!

Make the comments match the code or get rid of them.

{ NbTopsFan++; } //This is the function that the interupt calls

That is NOT a function.

Variables used in interrupt service routines and in other functions MUST be volatile!

One EVERY pass through loop, you set NbTopsFan back to 0. Why?

Ok. You say that the display is OK. But why are you trying to manipulate the interrupts which anyway appear only to be used to count the pulses from the fan revolutions ?
This, in particular, if implemented would have unpredictable side effects and it is also not clear exactly what you are trying to achieve:

//sei(); //Enables interrupts
 //delay (1000);
 //cli(); //Disable interrupts

Are you intending to write code to actually change the speed of the fan. At the moment you are merely setting a variable fanSpeed ?

You are aware that an (16bit) integer can hold a relatively small number (around 65,000) in the context of fan revolutions (unless it is a very slow fan), and calculations like this may blow up:

 Calc = ((NbTopsFan * 60)/fanspace[fan].fandiv);

Using floats may be better here.

You can use the code formatter in the Arduino IDE to improve the layout of your code so it is not so confusing to read.

Using floats may be better here.

Why? So you can know that the fan speed is 600.3 RPM instead of 600? Does that REALLY matter?

Hi badcom. The way that you're doing it with "blocking code" using delay() is not ideal. It would be a lot better to just use millis() to time it for say 1 seconds between resetting the NbTopsFan count and subsequently calculating the speed. That would allow the loop to keep running while timing is in progress.

Having said that however, your code should be able to work if you correct the major problem of interrupt masking. You don't want to globally disable interrupts with cli() ! After you do that then the system timers (that handle things like millis and delay) can no longer work. So your code gets "stuck" on the subsequent delay(200) as the timers are not working. (I've included just the relevant code below to show where it gets stuck).

Instead you need to just mask the particular interrupt that you are using (INT0 in this case). For example, if you replace sti() and cli() with the following unmasking and masking statements it should work.

EIMSK |= (1<<INT0); // To replace sti()
EIMSK &= ~(1<<INT0); // To replace cli()

BTW. There might be a higher level way to do this. TBH I don't know because I'm usually using low level code when I'm using interrupts. In any case, it should work. :slight_smile:

Also make the definition of NbTopsFan volatile as suggested above. For example:

unsigned volatile NbTopsFan;

void rpm ()
{ NbTopsFan++; } //This is the function that the interupt calls
 
void setup() {
  attachInterrupt(0, rpm, RISING);
  ...
}
 
void loop() { 
   ...
   NbTopsFan = 0; //Set NbTops to 0 ready for calculations
   ...   
  //sei(); //Enables interrupts
  //delay (1000);
  //cli(); //Disable interrupts
   Calc = ((NbTopsFan * 60)/fanspace[fan].fandiv);
   ...
   delay(200);  
}

Here is a thread which address that specific piece of code which reads interrupt counts during a delay period. Surprisingly, there may only be minimal changes required if your sketch can tolerate the use of delay(1000).

https://forum.arduino.cc/index.php?topic=421005.0

My apologies PaulS, I didn't mean to upset anyone.

I guess I will stick to designing using Hardware instead of Software as it seem less volatile.

Anyway thanks for everyone help.

Regards

badcom:
I guess I will stick to designing using Hardware instead of Software as it seem less volatile.

Na, stick at it. It's the only way to learn :slight_smile:

The last two replies show how to fix the specific problem with how you were using interrupts, specifically how you were leaving interrupts globally disabled through most of the main loop. Both address the problem in a slightly different way, but both should work.

  NbTopsFan = 0;            //Set NbTops to 0 ready for calculations (Note INT0 is already masked here)
  EIMSK |= (1<<INT0);       // To replace sti(), but specific to INT0
  delay (1000);
  EIMSK &= ~(1<<INT0);      // To replace cli(), but specific to INT0

The last two replies show how to fix the specific problem with how you were using interrupts, specifically how you were leaving interrupts globally disabled through most of the main loop.

I guess I miss how that was happening. Everyone keeps pointing to these three lines:

//sei(); //Enables interrupts
 //delay (1000);
 //cli(); //Disable interrupts

apparently missing the first two non-space characters on each of them.

PaulS:
apparently missing the first two non-space characters on each of them.

The op said "but when I include the Enable & Disable Interrupt lines, the Display is Blank". So not too much of a stretch to guess that he had deliberately commented those lines out because they were causing his program to hang.

stuart0:
The op said "but when I include the Enable & Disable Interrupt lines, the Display is Blank". So not too much of a stretch to guess that he had deliberately commented those lines out because they were causing his program to hang.

Of course. I believe that is clear to most people who read it.
It appears that Mr. PaulS's heavyweight contribution to the thread has traumatised the OP to the extent that he [the OP] is considering abandoning software development. Rather sad.