Placement of noInterrupts and Interrupts

Where should I enable and disable interrupts in my code? I ask because I am getting erratic results from the newframetime variable occasionally when the interrupt is triggered. I was advised that it was caused by the interrupt triggering while the variable is being updated because it is an unsigned long. I have tried enabling and disabling interrupts in various places throughout and it either makes no difference or the arduino crashes.

example of serial output
41
40
40
40
0
41
40
40
41
0
40

volatile unsigned long prevframetime = 0;//stores the first reading of millis when the interrupt is triggered
volatile unsigned long newframetime = 0;//stores the second reading of millis when the interrupt is triggered after the first time
int frametime = 0; // stores the difference between prev and newframetime
int setspeed = 40;//The target speed the camera must run at. 40 is 25 fps
int val = 95;//The current analog write value. It is initially set at 95 for quick startup
volatile bool count = 0;
volatile bool calctime = 0; //when this is true, the resulting time is calculated and displayed
volatile int frames = 0;
int feet = 0;
bool recording = 0;

#include <Adafruit_SSD1306.h>
//Define SPI Connections:
#define OLED_MOSI   9
#define OLED_CLK   10
#define OLED_DC    11
#define OLED_CS    12
#define OLED_RESET 13
Adafruit_SSD1306 display(OLED_RESET);

void setup()
{
pinMode(3,OUTPUT);
pinMode(8,INPUT);
pinMode(10,OUTPUT);
Serial.begin(9600);

display.begin(SSD1306_SWITCHCAPVCC, 0x3C);  //Initialize with the I2C address 0x3C.
display.clearDisplay();
display.setTextSize(2);
display.setTextColor(WHITE);
display.setCursor(0,0);
display.print("Feet:");
display.setCursor(71,0);
display.print(feet);
display.setCursor(0,16);
display.print("Frame:");
display.setCursor(71,16);
display.print(frames);
display.display();
attachInterrupt(digitalPinToInterrupt(2), incframe, RISING);
}

void loop() 
{
  recording = digitalRead(8);
  if (recording == 1)
    {
    if (calctime == 1) 
      {          
        frametime = Calculate_Frame_Time();  
        Set_Motor_Speed(frametime,setspeed);
        Update_Display(frames);        
      }
    analogWrite(3,val);
    }
  if (recording == 0)
    {
      analogWrite(3,0);
    }
}

void incframe()
{
  calctime = 1;
  frames +=1;
  switch (count)
    {
    case 0:
      count = 1;
      prevframetime = millis();
      break;
    case 1:
      count = 0;
      newframetime = millis();
      break;
    }
}

int Calculate_Frame_Time()
{
  
  int result;
     if (count == 0)
    {
      result = newframetime - prevframetime;   
      calctime = 0;     
    }
    if (count == 1)
    {
      result = prevframetime - newframetime;  
      calctime = 0;      
    }
    
    return result;   
}

void Set_Motor_Speed(int a, int b)//a frametime b setspeed
{ 

  if (a > b && val < 255)
    {
      val += 1;
    }
  if (a < b && val > 0)
    {
      val -= 1;
    }
    //analogWrite(3,val);
    Serial.println(a);
}

void Update_Display(int a)//a frames
{
    feet = a/40;
    display.clearDisplay();
    display.setTextSize(2);
    display.setTextColor(WHITE);
    display.setCursor(0,0);
    display.print("Feet:");
    display.setCursor(71,0);
    display.print(feet);
    display.setCursor(0,16);
    display.print("Frame:");
    display.setCursor(71,16);
    display.print(a);
    display.display();
}

Enclose any access outside an ISR to a variable, that can be changed by an interrupt, with noInterruts and interrupts. That way the variable cannot be changed while it is being read. Make the time that interrups are disabled as short as possible.

//get long_value
noInterrupts;
// access long_value
interrupts;

You should post some info about wiring and attached hardware. I'm guessing that a baby-alarm is triggering the interrupt? No? Oh.. I see.. :stuck_out_tongue:

Your interrupt handler is performing way too much work and calling millis() in the interrupt is not adviceable.

volatile byte isr_counter = 0;

void ISR_Handler()
{
  //Only increment a 8 bit counter here, the rest is handled in loop()
  isr_counter++;
}

void loop()
{
  if (isr_counter > 0)
  {
    isr_counter--;
    //Handle your ISR depend code here
  }
  //Handle the rest of your code here
}

And for best results just make a copy of your volatile variable and use the copy in the rest of your calculations or whatever. That way you can turn interrupts back on quickly.

noInterrupts();
unsigned long copyVar = whateverVolatileVar;
Interrupts();

Then use the copy for everything else.

calling millis() in the interrupt is not adviceable.

Calling it is fine. You'll get whatever value it had when the interrupt fired. If the ISR is short that will be accurate.

Just don't expect it to update during your ISR. No waiting for millis() to equal something.

That interrupt really isn't doing much. Are we looking at the same function? It's long on lines but it only assigns a few variables and does one comparison.

Deferring the majority ISR code to "loop()" will eliminate the necessity to disable interrupts in order to get the code working :slight_smile:

For any "volatile" variable, you want it to only update in one place, within the interrupt. You want the interrupt(s) to execute as fast and light as possible. elsewhere in your code where you might want to evaluate, or otherwise manipulate the value of the volatile variable, you want to use other local variables with a function call that will read the value of the volatile variable. consider how millis() is done in source code in wiring.c. An ISR (interrupt) is setup to trigger on timer0. That interrupt increments "timer0_millis" which is a volatile unsigned long. Now, the function millis() is used simply to read "timer0_millis" with "cli();" which is the same as "noInterrupts()", used to prevent "timer0_millis" from incrementing between reading the 4 bytes of the unsigned long, which would give erratic results. In my own function, I would add "sei();", same as "interrupts()" before returning "m". This is because I am not as fluent in AVR to know what would be the result if I did not add it.

Danois90:
Deferring the majority ISR code to "loop()" will eliminate the necessity to disable interrupts in order to get the code working :slight_smile:

But won't allow him to capture the time when the interrupt fired which seems to be the whole point.

If he was to do as you suggest then the interrupt itself is a bit redundant. If the loop is fast enough to accurately catch the time then you could dispense with the interrupt and just poll the pin.

I see that, I would modify/optimize some parts of the code to:

volatile unsigned long prevframetime = 0;
volatile unsigned long newframetime = 0;
volatile bool odd = false; //Name changed
volatile bool calctime = false;
volatile unsigned int frames = 0; //Modified to unsigned

void incframe()
{
  calctime = true;
  frames++;
  if (odd) newframetime = millis();
  else prevframetime = millis();
  odd ^= 1; //Invert state
}

int Calculate_Frame_Time()
{
  noInterrupts(); //cli();
  int result = odd ? (prevframetime - newframetime) : (newframetime - prevframetime);
  calctime = false;
  interrupts(); //sti();
  return result;
}

The term to remember for this is a "critical section" - your main program and your ISRs are both
trying to run at once and access some variable(s). You want each to see a consistent state and
make any changes atomically between consistent states.

The ISR has no problem making atomic changes, but the main program cannot do this without
"guard"ing the code to access the state within noInterrupts()/interrupts() - the code thus guarded
is a critical section. The body of the ISR is automatically a critical section. Any state touched by
any critical section that isn't constant, must be declared volatile so the compiler doesn't muck things
up by optimizing away reads and writes to memory.

When you have multithreading, you have to guard every critical section with an explicit lock, but for
single threaded code you only have to block interrupts.

There are situations where only a single byte is updated where the guard isn't needed, but you have
to be careful.

I tried the suggestion made by Danois90 and I am still getting the exact same results. The code is a bit neater by the way, so thanks for that.

I don’t think it is a fault with the sensor as I have another simple sketch which just sends the value of millis every time the interrupt is triggered and I get no strange values. For those of you who don’t know, this project is for a constant speed motor drive for a 16mm cine camera with features for counting the feet and frames. My programming skills are still very limited so I’m sorry to admit that I don’t fully understand the hardware I am programming for nor the C language itself.

This is the whole thing after the suggested edit. I have added explanations about the hardware.

volatile unsigned long prevframetime = 0;//stores the first reading of millis when the interrupt is triggered
volatile unsigned long newframetime = 0;//stores the second reading of millis when the interrupt is triggered after the first time
int frametime = 0; // stores the difference between prev and newframetime
int setspeed = 40;//The target speed the camera must run at. 40 is 25 fps
int val = 95;//The current analog write value. It is initially set at 95 for quick startup
volatile bool odd = false;
volatile bool calctime = false; //when this is true, the resulting time is calculated and displayed
volatile unsigned int frames = 0;
int feet = 0;
bool recording = 0;

#include <Adafruit_SSD1306.h>
//Define SPI Connections:
#define OLED_MOSI   9
#define OLED_CLK   10
#define OLED_DC    11
#define OLED_CS    12
#define OLED_RESET 13
Adafruit_SSD1306 display(OLED_RESET);

void setup()
{
pinMode(3,OUTPUT); //This pin is attached to the gate of an N channel mosfet through a 200 ohm resistor
pinMode(8,INPUT); // This pin is connected to a switch to start the motor
pinMode(10,OUTPUT);// LED to shine on the film for a few frames to make a sync point, not currently implemented.
attachInterrupt(digitalPinToInterrupt(2), incframe, RISING); Pin 2 is attached to a TCST1103 Transmissive Optical Sensor with Phototransistor Output the spinning shutter on the camera passes through the gap, triggering the interrupt
Serial.begin(9600);

display.begin(SSD1306_SWITCHCAPVCC, 0x3C);  //Initialize with the I2C address 0x3C.
display.clearDisplay();
display.setTextSize(2);
display.setTextColor(WHITE);
display.setCursor(0,0);
display.print("Feet:");
display.setCursor(71,0);
display.print(feet);
display.setCursor(0,16);
display.print("Frame:");
display.setCursor(71,16);
display.print(frames);
display.display();

}

void loop() 
{
  recording = digitalRead(8);
  if (recording == 1)
    {
    if (calctime == 1) 
      {          
        frametime = Calculate_Frame_Time();  
        Set_Motor_Speed(frametime,setspeed);
        Update_Display(frames);        
      }
    analogWrite(3,val);
    }
  if (recording == 0)
    {
      analogWrite(3,0);
    }
}

void incframe()
{
  calctime = true;
  frames++;
  if (odd) newframetime = millis();
  else prevframetime = millis();
  odd ^= 1; //Invert state
}

int Calculate_Frame_Time()
{
  noInterrupts(); //cli();
  int result = odd ? (prevframetime - newframetime) : (newframetime - prevframetime);
  calctime = false;
  interrupts(); //sti();
  return result; 
}

void Set_Motor_Speed(int a, int b)//a frametime b setspeed
{ 

  if (a > b && val < 255)
    {
      val += 1;
    }
  if (a < b && val > 0)
    {
      val -= 1;
    }
    //analogWrite(3,val);
    Serial.println(a);
}

void Update_Display(int a)//a frames
{
    feet = a/40;
    display.clearDisplay();
    display.setTextSize(2);
    display.setTextColor(WHITE);
    display.setCursor(0,0);
    display.print("Feet:");
    display.setCursor(71,0);
    display.print(feet);
    display.setCursor(0,16);
    display.print("Frame:");
    display.setCursor(71,16);
    display.print(a);
    display.display();
}

You haven't set the pinMode for pin 2. Dunno if it needs it, but this will set the internal pullup resistor as well:

  pinMode(2,INPUT_PULLUP);

Pete

Recently there was a post about a similar sensor type which also yielded some bad readings once in a while. The sollution (as I remember) was to filter away any readings which where more than 50% away from the previous reading.

int lastFrameTime = 0;

int Calculate_Frame_Time()
{
  noInterrupts(); //cli();
  int result = odd ? (prevframetime - newframetime) : (newframetime - prevframetime);
  calctime = false;
  interrupts(); //sti();
  if (lastFrameTime > 0)
  {
    int halfLast = (lastFrameTime >> 1); //Shift right once = divide by two
    if ((result < halfLast) || (result > lastFrameTime+halfLast)) result = lastFrameTime;
  }
  lastFrameTime = result;
  return result;
}

You should also consider to use “unsigned int” instead of “int” here.

If you are using the DUE or the Mega and you do not find a solution for the interrupt issues you are having try using the uMT library. The learning curve can be steep but I was able to switch all my routines to being interrupt driven. The only thing loop does is setup the interrupts and blink LED13.

I tried the edit by Danois90 and it worked perfectly, once, for about a minute and then the camera just slowed down and stopped. When I reset the arduino, it just ran up to greater than the set speed with no control and it won’t work again unless I revert the code back to my version. The Serial monitor just repeats a random number which was anywhere between 400 and 4000 when I press the button to start recording.
Here is the whole thing with his suggested edit

volatile unsigned long prevframetime = 0;//stores the first reading of millis when the interrupt is triggered
volatile unsigned long newframetime = 0;//stores the second reading of millis when the interrupt is triggered after the first time
int frametime = 0; // stores the difference between prev and newframetime
int setspeed = 40;//The target speed the camera must run at. 40 is 25 fps
int val = 95;//The current analog write value. It is initially set at 95 for quick startup
volatile bool odd = false;
volatile bool calctime = false; //when this is true, the resulting time is calculated and displayed
volatile unsigned int frames = 0;
int feet = 0;
bool recording = 0;
int lastFrameTime = 0;
#include <Adafruit_SSD1306.h>
//Define SPI Connections:
#define OLED_MOSI   9
#define OLED_CLK   10
#define OLED_DC    11
#define OLED_CS    12
#define OLED_RESET 13
Adafruit_SSD1306 display(OLED_RESET);

void setup()
{
pinMode(3,OUTPUT); //This pin is attached to the gate of an N channel mosfet through a 200 ohm resistor
pinMode(8,INPUT); // This pin is connected to a switch to start the motor
pinMode(10,OUTPUT);// LED to shine on the film for a few frames to make a sync point, not currently implemented.
attachInterrupt(digitalPinToInterrupt(2), incframe, RISING); //Pin 2 is attached to a TCST1103 Transmissive Optical Sensor with Phototransistor Output the spinning shutter on the camera passes through the gap, triggering the interrupt
Serial.begin(9600);

display.begin(SSD1306_SWITCHCAPVCC, 0x3C);  //Initialize with the I2C address 0x3C.
display.clearDisplay();
display.setTextSize(2);
display.setTextColor(WHITE);
display.setCursor(0,0);
display.print("Feet:");
display.setCursor(71,0);
display.print(feet);
display.setCursor(0,16);
display.print("Frame:");
display.setCursor(71,16);
display.print(frames);
display.display();

}

void loop() 
{
  recording = digitalRead(8);
  if (recording == 1)
    {
    if (calctime == 1) 
      {          
        frametime = Calculate_Frame_Time();  
        Set_Motor_Speed(frametime,setspeed);
        Update_Display(frames);        
      }
    analogWrite(3,val);
    }
  if (recording == 0)
    {
      analogWrite(3,0);
    }
}

void incframe()
{
  calctime = true;
  frames++;
  if (odd) newframetime = millis();
  else prevframetime = millis();
  odd ^= 1; //Invert state
}

int Calculate_Frame_Time()
{
  noInterrupts(); //cli();
  int result = odd ? (prevframetime - newframetime) : (newframetime - prevframetime);
  calctime = false;
  interrupts(); //sti();
  if (lastFrameTime > 0)
  {
    int halfLast = (lastFrameTime >> 1); //Shift right once = divide by two
    if ((result < halfLast) || (result > lastFrameTime+halfLast)) result = lastFrameTime;
  }
  lastFrameTime = result;
  return result;
}

void Set_Motor_Speed(int a, int b)//a frametime b setspeed
{ 

  if (a > b && val < 255)
    {
      val += 1;
    }
  if (a < b && val > 0)
    {
      val -= 1;
    }
    //analogWrite(3,val);
    Serial.println(a);
}

void Update_Display(int a)//a frames
{
    feet = a/40;
    display.clearDisplay();
    display.setTextSize(2);
    display.setTextColor(WHITE);
    display.setCursor(0,0);
    display.print("Feet:");
    display.setCursor(71,0);
    display.print(feet);
    display.setCursor(0,16);
    display.print("Frame:");
    display.setCursor(71,16);
    display.print(a);
    display.display();
}

#11

For your Calculate_Frame_Time() function to return 0 (as your serial output suggests) indicates that the interrupt is firing twice within 1 millisecond. ie bounce (as silly as it sounds), or other interference.
I would be inclined to use a sanity check, and disregard spurious readings.

(** This would also mean that the frame count should only get updated in Calculate_Frame_Time() )

Try incorporating the following

....
....

volatile bool interrupted = false;
volatile unsigned long interrupt_time = 0;
unsigned long last_interrupt_time = 0;

// SANITY_CHECK value. Uncomment...
// Either
// unsigned long SANITY_CHECK = 2L;
// OR
// #define SANITY_CHECK 2L
// Adjust the value to give the shutter time to trigger the interrupt, and also clear the slot. 2 - 5 ms should do it

void loop)( {
  recording = digitalRead(8);
  if (recording == 1)
  {
    if (Calculate_Frame_Time(frametime))
    {
      Set_Motor_Speed(frametime,setspeed);
      Update_Display(frames);
    }
    analogWrite(3,val);
  }
  if (recording == 0)
  {
    analogWrite(3,0);
  }
}
	
void incframe() {
  if (!interrupted)
  {
    interrupted = true;
    interrupt_time = millis();
  }
}


bool Calc_Frame_Time(int &frameTime) {
	
  unsigned long __interrupt_time;
	
  if (interrupted)
  {
    noInterrupts();
    interrupted = false;
    __interrupt_time = interrupt_time;
    interrupts();
		
    if ((__interrupt_time - last_interrupt_time) >= SANITY_CHECK)
    {
      frames +=1;
      frameTime = __interrupt_time - last_interrupt_time;
      last_interrupt_time = __interrupt_time;
      return true;
    }
  }
  return false;
}

The only issue that I can see with the above is that the initial interrupt will give a weird result.

I would (as mentioned earlier in the thread) use a pullup resistor on the button, the internal is fine, and then I would debounce the button. Remember that when you use a pullup resistor, the button will read “LOW” (or 0) when pressed and not “HIGH” (or 1) as in your sketch.

#define BUTTON_PIN 8

byte currentButtonState = HIGH; //Un-pressed state with pullup resistor

void setup()
{
  pinMode(BUTTON_PIN, INPUT_PULLUP);
}

void loop()
{
  byte buttonState = digitalRead(BUTTON_PIN);
  if (buttonState != currentButtonState)
  {
    currentButtonState = buttonState;
    if (currentButtonState == LOW)
    {
      Serial.println(F("Button was pressed"));
    }
    else
    {
      Serial.println(F("Button was released"));
    }
  }
}

The code shows how to properly handle a push-button (still not debounced, though).

Danois90:
I would (as mentioned earlier in the thread) use a pullup resistor on the button, the internal is fine, and then I would debounce the button. Remember that when you use a pullup resistor, the button will read "LOW" (or 0) when pressed and not "HIGH" (or 1) as in your sketch.

el_supremo:
You haven't set the pinMode for pin 2. Dunno if it needs it, but this will set the internal pullup resistor as well:

The thing with these devices (TCST1103 optical switch) is that they (usually) read HIGH when the beam is broken (ie active, ie the rotating shutter passes through the slot). Of course, that does depend on the circuitry around the device.

As the OP has set the trigger for the ISR to RISING, I'm working under the assumption that this is true in his case as well.

Setting the input to INPUT_PULLUP is not, I feel, appropriate for this sensor.

I have been playing with one of these things as a limit switch, and they do appear to 'bounce' on occasion. I put my experience down to it being a cheap board (with dodgy soldering), so I just added some logic to cope with the situation in addition to a pulldown on the breadboard.

Some more info on the circuit might be nice, but I think that just discarding spurious readings (ie a sanity check on the time between interrupts) would solve the issue.