A bit of a funny one here. I have borrowed some code (many thanks to whoever supplied it) to read the RPM of a fan connected to pin2 on a Nano. This runs fine but outputting the values to the serial monitor. If I then insert the code to display on an LCD via I2C the code stops and nothing is dispalyed. If I comment out or place the cli() command at the end of the loop code (as shown) the program works and I get the readout on the LCD.
Any ideas as to why the stock code does not work without me changing the cli() statement. How will it effect further code that will need to be inserted for the final project
The code is below that works.
Apologies in advance if I am missing something but I am a newbie with this
Thanks
--
Mark
[code]
#include <Wire.h>
#include <LiquidCrystal_I2C.h>
LiquidCrystal_I2C lcd(0x3F, 2, 1, 0, 4, 5, 6, 7, 3, POSITIVE); // Set the LCD I2C address
//code by Crenn from http://thebestcasescenario.com
//project by Charles Gantt from http://themakersworkbench.com
//To disable interrupts: cli(); disable global interrupts and
//to enable them: sei(); enable interrupts
//Varibles used for calculations
int NbTopsFan;
int Calc;
//The pin location of the sensor
int hallsensor = 2;
typedef struct {
//Defines the structure for multiple fans and
//their dividers
char fantype;
unsigned int fandiv;
} fanspec;
//Definitions of the fans
//This is the varible used to select the fan and it's divider,
//set 1 for unipole hall effect sensor
//and 2 for bipole hall effect sensor
fanspec fanspace[3] = {{0, 1}, {1, 2}, {2, 8}};
char fan = 1;
void rpm ()
//This is the function that the interupt calls
{
NbTopsFan++;
}
//This is the setup function where the serial port is initialised,
//and the interrupt is attached
void setup()
{
Serial.begin(9600);
pinMode(hallsensor, INPUT_PULLUP); //enable internal resistor
lcd.begin(20, 4);
attachInterrupt(digitalPinToInterrupt(2), rpm, RISING);
}
void loop ()
//Set NbTops to 0 ready for calculations
{
NbTopsFan = 0; //Set NbTops to 0 ready for calculations
sei(); //Enables interrupts
delay(1000); //Wait 1 second
//cli(); //Disable interrupts
Calc = ((NbTopsFan * 60) / fanspace[fan].fandiv); //Times NbTopsFan (which is apprioxiamately the fequency the fan is spinning at) by 60 seconds before dividing by the fan's divider
lcd.setCursor(0, 1);
lcd.print (Calc, DEC); //Prints the number calculated above
lcd.setCursor(4, 1);
lcd.print ("RPM"); //Prints " RPM"
Serial.print(Calc, DEC);
Serial.print(" rpm\r\n");
cli();
}
Wrong interrupt handling and wrong disabling of interrupts is no fun.
Wrong disabling of Interrupts will dead-lock your program.
I don't know your library, but most likely this should work:
delay(1000); //Wait 1 second
cli(); //Disable interrupts
Calc = ((NbTopsFan * 60) / fanspace[fan].fandiv); //Times NbTopsFan (which is apprioxiamately the fequency the fan is spinning at) by 60 seconds before dividing by the fan's divider
NbTopsFan = 0; //Set NbTops to 0 ready for calculations // remove this line from top of loop
sei(); //Enables interrupts
// insert Serial.print() and lcd commands here (while interrupts are enabled)
BTW:
blocking programs by using delay() is bad in general in almost every Arduino program
using delay() to create a 1-second-timing for calculations lead to slightly wrong results
P.S.: All your variables you want to access from within interrupt handling code and from normal code have to be declared 'volatile'. So declare that variable volatile:
That is an absolutely piss-poor way of computing RPM (or whatever you are trying to calculate).
Turning on interrupts, stuffing your head in the sand, then turning off interrupts after 1 second is NOT the correct thing to do. Look at the blink without delay example.
Periodically (on every pass through loop()), see if it is time to calculate the needed value(s). If it is, turn off interrupts, copy the necessary value(s), then turn interrupts back on. Then calculate the needed value(s).
This way, interrupts are disabled for only a few clock cycles.
I just copied the code from the site in the header and inserted the LCD parts. Swapping the sei() and cli() over works as it should. Not sure why the original code was as it was?
MarkDerbyshire:
I just copied the code from the site in the header and inserted the LCD parts. Swapping the sei() and cli() over works as it should. Not sure why the original code was as it was?
The programmer of the original knows that delay(1000); would lead to slightly inaccurate timing, but instead of using an accurate timing without using delay he decided to use a weird programming logic for disabling/enabling interrupts, as in his code this would work and also provide accurate timing by fixing the problem caused by delay().
So one weird function ("delay()") used in his code lead him to other weird things like weird disabling/enabling of interrupts.
So better do not get used to weird functions like "delay()" in your code, or things might get more and more complicated and also more and more weird to make it work as expected!
