Problems with interrupt not always working fast enough

Hi , I'm fairly new to Arduino so please forgive if my submittal procedure is not total correct.
I have (the attached sketch) where i need the closeGate() function to be very attentive to the interrupt on pin 2. this seems to work but not as fast as I would like and when it does work I need the interrupt to be checked again to ensure the gate close can recommence. I am also open to constructive criticism on the rest if anyone feels it necessary.

// lets start with the libraries
#include <SPI.h>
#include <Wire.h>
#include <Adafruit_GFX.h>
#include <Adafruit_SSD1306.h>
#include <avr/wdt.h>
#include <nRF24L01.h>
#include <RF24.h>

// Setup other stuff
uint8_t holdTime;
uint8_t openingTime;
uint8_t closingTime;
uint8_t pauseTime;
uint8_t thisVersion  = 22;
const int Relay1 = 7;
const int Relay2 = 6;
const int yellowLED = 17;    // This LED illuminates whenever the motor is running

//
// There are three trigger methods N/C , N/O and RF (via NRF2401)
//
const int NC_trigger = 4;    // Define Pin number for Normally Closed trigger
const int NO_trigger = 3;    // Define Pin number for Normally Open trigger
/*
  THE LASER.

  The laser is powered from the 24v PSU that also powers the gate motors
  It is intented that the laser is only powered during it neccessary period IE when the gate is closing.
  As such via a transistor from Digital Pin 5 the 24V will be applied.

  If this beam is broken the movement of the gate will be stopped for the duration of 'pauseTime'
  after which the beam will be checked again only after a clear period of 'pauseTime' will the gate start
  to move again.
  This uses an interrupt so its pin numberis critical and on UNO & NANO must be 2 or 3.(in this case 2)

*/

const int laserTrig = 2;     // This is for the laser safe edge and must be this pin as it uses interupt
const int laserArm = 5;      // This output controls when the laser is powered
uint8_t Command = 0 ;        // This variable defines the operational mode of the gate.

// Setup NRF2401 Radio
#define CE_PIN 10  // These are the pins my Nano breakout board has hardwired
#define CSN_PIN 9
const byte thisSlaveAddress[5] = {'R', 'x', 'A', 'A', 'A'}; // Sets the Radios unique address
RF24 radio(CE_PIN, CSN_PIN);
char dataReceived = '0';      // this must match dataToSend in the TX
bool newData = false;
bool trigger = false;
volatile bool isrCalled = false;

/*
  Declaration for an SSD1306 display connected to I2C (SDA, SCL pins)
  The pins for I2C are defined by the Wire-library.
  On an arduino UNO:       A4(SDA), A5(SCL)
*/
#define SCREEN_WIDTH 128     // OLED display width, in pixels
#define SCREEN_HEIGHT 64     // OLED display height, in pixels
#define OLED_RESET    -1     // Reset pin # (or -1 if sharing Arduino reset pin)
#define SCREEN_ADDRESS 0x3C  // < See datasheet for Address; 0x3D for 128x64, 0x3C for 128x32
Adafruit_SSD1306 display(SCREEN_WIDTH, SCREEN_HEIGHT, &Wire, OLED_RESET);


void setup() {
  motorOff();
  Serial.begin(9600);      // initialize serial communications at 9600 bps:

  wdt_enable(WDTO_4S);     // Watchdog resets system in 2 seconds unless cleared

  /*

    This section looks at the digital pins D14 - D16 to find the selected board ID

    A link will be made to ONE of these pins only and as such defines the ID

    In its absence the ID is 1 (Actually zero)

    NOTE - THIS METHOD WILL BE REPLACED BY STORING AN ADDRESS ON EEPROM ON THE
           NEXT RELEASE If I need to free up SRAM

    Yes I know I could have varied a resistor value to give me a single pin address
    but no resistor required suits me.
  */

  const int addressPin1 = 14;     // A0 used as digital pin
  const int addressPin2 = 15;     // A1 used as digital pin
  const int addressPin3 = 16;     // A2 used as digital pin
  pinMode(addressPin1, INPUT_PULLUP);
  pinMode(addressPin2, INPUT_PULLUP);
  pinMode(addressPin3, INPUT_PULLUP);
  const int IDcheck1 = digitalRead(addressPin1);
  const int IDcheck2 = digitalRead(addressPin2);
  const int IDcheck3 = digitalRead(addressPin3);

  /*

    Array structure is ....
    Dimension 1 Selects the Gate, dimension 2 Selects the timer values

    openingTime, holdTime, closingTime and pauseTime

    To know which gate this is will be defined by the pins D14 - D16 (A0 - A2)

  */

  const int gateTimers [4] [4] = {
    {15, 20, 16, 5}, // Gate A
    {15, 20, 16, 5}, // Gate B
    {15, 30, 16, 5}, // Gate C
    {18, 30, 14, 5}  // Gate D
  };

  /*
    NOTE the next part reads all three pins relevant to identifying the address
    it doesnt matter if more than one reads LOW as in this instance all except
    the last one will be overwritten.
    in the event that none are LOW then the address will be 1 (Zero really)

    So just connect the required pin to GND.
  */

  // Im using uint8_t to save SRAM as its all I need.

  uint8_t address = 0;
  uint8_t IDcheck;
  IDcheck = digitalRead(addressPin1);
  if (IDcheck == LOW) address = 1;
  IDcheck = digitalRead(addressPin2);
  if (IDcheck == LOW) address = 2;
  IDcheck = digitalRead(addressPin3);
  if (IDcheck == LOW) address = 3;

  // Based on the above pull the applicable values from the array for the gate
  // as I didnt want to have a different upload for each gate.

  openingTime = {gateTimers [address] [0]};
  holdTime    = {gateTimers [address] [1]};
  closingTime = {gateTimers [address] [2]};
  pauseTime   = {gateTimers [address] [3]};

  // Run startup script - Output to SERIAL
  Serial.println(F("Gate Controller - TLC Innovation"));
  Serial.println(F("--------------------------------"));
  Serial.println(F("Watchdog initialised"));
  Serial.println();
  Serial.print(F("Gate ID found as : "));
  Serial.println(address + 1);
  Serial.print(F("Open time        : "));
  Serial.println(openingTime);
  Serial.print(F("Hold Open time   : "));
  Serial.println(holdTime);
  Serial.print(F("Close time       : "));
  Serial.println(closingTime);
  Serial.print(F("Pause on Edge    : "));
  Serial.println(pauseTime);

  // Invoke radio to receive mode.
  radio.begin();
  radio.setDataRate( RF24_250KBPS );
  radio.openReadingPipe(1, thisSlaveAddress);
  radio.startListening();

  // Define trigger pins as inputs
  pinMode(NC_trigger, INPUT_PULLUP); // Assign NC Trigger Input Pin to use internal Pullup Resistor
  pinMode(NO_trigger, INPUT_PULLUP); // Assign NO Trigger Input Pin to use internal Pullup Resistor
  pinMode(laserTrig,  INPUT_PULLUP);

  // Setup motor LED & Relays
  pinMode(yellowLED, OUTPUT);       // Motor Run indicator
  pinMode(Relay1,    OUTPUT);          // Pin to drive Pole A of Motor
  pinMode(Relay2,    OUTPUT);          // Pin to drive Pole B of Motor
  /*
    THE RELAYS (above)

    I'm using 2 changerover relays where on
    RLY1 the N/C is + Volts and N/O is 0v
    RLY2 the N/C is 0v and N/O is + Volts

    With this method by connecting the DC motor to the commons I get one
    direction with both relays de-energised (Gate open) and with both
    energised I get the other (Gate close)

    Therefore with EITHER (but not both) relays de-energised the
    motor is off.

    As a visual indicator the Yellow LED illuminates if the motor runs
  */



  /* Setup OLED chatacteristics

    SSD1306_SWITCHCAPVCC = generate display voltage from 3.3V internally

  */

  if (!display.begin(SSD1306_SWITCHCAPVCC, SCREEN_ADDRESS)) {
    Serial.println(F("SSD1306 allocation failed"));
    for (;;); // Don't proceed, loop forever
  }
  display.setTextSize(1);    //sets text size
  display.invertDisplay(false);
  delay(200);
  display.clearDisplay();
  runCommand();

  // Activate interrupt for Pin 2 - Safe Edge.
  attachInterrupt(0, safeEdgeISR, LOW);
}

////////////////////
/// END OF SETUP ///
////////////////////

void runCommand() {
  display.clearDisplay();
  display.invertDisplay(false);
  display.setTextColor(WHITE);
  display.setCursor(0, 1);
  display.setTextSize(2);
  switch (Command) {
    case 0:
      display.print("  WAITING");
      display.setCursor(18, 18);
      break;
    case 1:
      display.print("  OPENING");
      motorOpen();
      motorOff();
      holdGate();
      motorClose();
      motorOff();
      break;

    // Cases 2 - 7 may be used when necessary

    case 9:
      display.print("  EMERGENCY");
      Serial.println(F("Case 9 - 911"));
      motorOpen();
      motorOff();
      display.clearDisplay();
      display.setCursor(0, 1);
      display.setTextSize(2);
      display.print("EMERGENCY");
      display.setCursor(18, 18);
      display.setTextSize(6);
      display.print("911");
      display.display();
      /*
        Keeps checking for Command code 8 which cancels a code 9. A code 8
        is the only way to leave code 9.
        Triggering a code 1 will NOT work otherwise the first trigger from
        normal operation would inadvertently clear the indefinite hold.

      */
      while (Command != 8) {
        if (radio.available() ) {
          radio.read( &dataReceived, sizeof(dataReceived) );
          dElay();
        }
        Command = 0;
      }
      break;
  }
  display.display();
}

void motorOpen() {
  digitalWrite(yellowLED, HIGH);
  digitalWrite(Relay1, HIGH);
  digitalWrite(Relay2, HIGH);
  for (uint8_t i = 0; i <= openingTime; i++) {
    display.clearDisplay();
    display.setCursor(0, 1);
    display.setTextSize(2);
    display.print("  OPENING");
    display.setCursor(30, 18);
    display.setTextSize(6);
    if ((openingTime - i) < 10) display.print("0"); // Add floating zero
    display.print((openingTime - i));
    display.print(" ");
    display.display();
    dElay();
  }
}

void safeEdgeISR() { // This is invoked by the interrupt for pin 2
  motorOff();
  isrCalled = true;
}

void dElay() {
  /*
    Im using millis() instead of delay so the interupt will work
    i'm only using this as one second but realise i could have used a
    value when calling the function but as I update the OLED every
    second I didn't feel it necessary.
  */

  int startMillis = millis();
  int currentMillis;
  do {
    currentMillis = millis();
    wdt_reset();
  }  while (currentMillis - startMillis <= 1000);
}

void safeEdge() {
  isrCalled = false;
  motorOff();
  Serial.println(F("Safe Edge TRIGGERED !"));
  display.clearDisplay();
  display.setCursor(10, 1);
  display.setTextSize(3);
  display.print("SAFE  EDGE");
  display.display();
  for (uint8_t ip = 0; ip <= pauseTime; ip++) dElay();
  digitalWrite(Relay1, LOW);
  digitalWrite(Relay2, LOW);
}

void holdGate() {
  digitalWrite(yellowLED, LOW);
  digitalWrite(Relay1, HIGH);
  digitalWrite(Relay2, LOW);
  for (uint8_t i = 0; i <= holdTime; i++) {
    getData();
    if (Command == 1) i = 1;
    display.clearDisplay();
    display.setCursor(1, 1);
    display.setTextSize(2);
    display.print("  HOLDING");
    display.setCursor(20, 18);
    display.setTextSize(6);
    // Next two lines add leading zeros if necessary to keep things tidy
    if ((holdTime - i) < 100) display.print("0");
    if ((holdTime - i) < 10) display.print("0");
    display.print((holdTime - i));
    display.display();
    wdt_reset();
    dElay();
  }
}

void motorClose() {
  // Turn on the safe edge laser, delay to allow it to stabalise then monitor it
  digitalWrite(laserArm, LOW);
  Serial.println(F(""));
  Serial.println(F("Laser Armed"));
  dElay();
  attachInterrupt(laserTrig, safeEdgeISR, LOW);
  digitalWrite(yellowLED, HIGH);
  digitalWrite(Relay1, LOW);
  digitalWrite(Relay2, LOW);
  for (uint8_t i = 0; i <= closingTime; i++) {
    display.clearDisplay();
    display.setCursor(0, 1);
    display.setTextSize(2);
    display.print("  CLOSING");
    display.setTextSize(1);
    display.setCursor(30, 18);
    display.setTextSize(6);
    if ((closingTime - i) < 10) display.print("0"); // add floating zero
    display.print((closingTime - i));
    display.print(" ");
    display.display();
    getData();
    if (isrCalled == true) {
      isrCalled = false;
      safeEdge();
    }
    dElay();
  }
  display.clearDisplay();
  display.setCursor(0, 1);
  display.setTextSize(2);
  display.print("  WAITING");
  display.display();
  detachInterrupt(digitalPinToInterrupt(0));
  digitalWrite(laserArm, HIGH);
  Serial.println(F("Laser Disarmed"));
  isrCalled = false;
  Command = 0;
  motorOff();
}

void motorOff() {
  digitalWrite(Relay1, HIGH);
  digitalWrite(Relay2, LOW);
  digitalWrite(yellowLED, LOW);
}

void getData() {
  Command = 0;
  int buttonState;
  dataReceived = '0';
  // I'm aware I could use OR if I have 2 buttonstate vars but in the future
  // I may do diffent things depening on which is triggered
  buttonState = digitalRead(NC_trigger);  // Read Normally Closed Trigger Input
  if (buttonState == LOW) {
    trigger = true;
    Command = 1;
    Serial.print(F("Command is : "));
    Serial.print(Command);
  }
  buttonState = digitalRead(NO_trigger);  // Read Normally Open Trigger Input
  if (buttonState == HIGH) {
    trigger = true;
    Command = 1;
  }
  if (radio.available() ) {
    radio.read( &dataReceived, sizeof(dataReceived) );
    newData = true;
  }
}

void loop() {
  wdt_reset();
  bool lock = false;
  isrCalled = false;
  display.clearDisplay();
  newData = false;
  getData();
  if (isrCalled == true) Serial.println(F(" ISR Triggered"));
  if (Command == 9) {
    lock = true;
    runCommand();
  }
  if (Command == 8) lock = false;
  if (Command == 1) runCommand();
  // add other options here
1 Like

Hello itzzzcol
The sketch uses a modified delay() function multiple.

Line 261:           dElay();
	Line 285:     dElay();
	Line 319:   for (uint8_t ip = 0; ip <= pauseTime; ip++) dElay();
	Line 343:     dElay();
	Line 352:   dElay();
	Line 374:     dElay();

The delay() function blocks the executuion and should be replaced by a timer function to get realtime behaviour of the sketch.
Have a nice day and enjoy coding in C++.

2 Likes

Thanks for the speedy response. Could you ellaborate please , I thought the modified dElay function using millis() would prevent blocking. will the interrupt not still be reading during this ?

Hello itzzzcol
Take a view into the BLINKWITHOUTDELAY example of the IDE to gain the knowledge about the usage of the millis() function.
Have a nice day and enjoy coding in C++.

The safeEdge() interrupt will occur as expected and set the mode, but nothing else will happen while you're spinning wheels in the do-while loop-

1 Like

So the motorOff() would run whilst the looping is going on ? or have i missed something , which is highly likely .

Things like delay( ) and while( ) looping can hang up a program until they are finished.

It is the programmers job to write non-blocking code.

That's why i used modified dElay it uses millis() therefore not blocking although I understand the loop but I seem to be misunderstanding the interrupt. If I'm successfully calling the ISR I was expecting the motorOff() function to be called. This is something I'm not familiar with.

Hello itzzzcol
The modified dElay() function is blocking too.
Have a nice day and enjoy coding in C++.

1 Like

Interesting point, not looking too closely, at first glance it would seem that motorOff() would be called from within the ISR, but nonetheless, dElay() is still blocking code as mentioned… the program can’t return from dElay() while that 1000mS is ticking away.

I need to look closer, but someone else may spot it before I get back.. !

isCalled is set false at the top of loop(), which may be cleared before it’s ever used… it should only be cleared once it’s been consumed.

I’ve added and if …safeEdge …. Break

I removed the line ‘ isCalled = false ‘ from the top of the loop and added an ‘ if (isrCalled == true ) break; ‘ inside the do…. Loop.

I’m still happy for constructive criticism

I’m still happy for constructive criticism

Remove the "modified delay()" function and calls to it. Replace them with the approach you will learn by studying the excellent explanation and advice in this tutorial:

2 Likes

Per your comment: Interrupts will also work with the delay() function. However, the rest of your code will be blocked until the do/while loop completes.

Look at the tutorial referenced by @jremington. It will require you to restructure your code but your code will be much more responsive. Indeed, you should be able to dispense with the interrupt altogether.

1 Like

The good news: You are not misunderstanding an interrupt ... That will appear even during delay() or any other time consuming function (unless you disable interrupts :wink: ).

However, no other part in loop() will have the chance to react on a variable changed by an interrupt because the processor is occupied with waiting.

Often the call for millis() is misunderstood as to replace delay() by a millis()-loop. What is ment is that the code has to be changed to loop() through all necessary tasks (as quick as possible - or - as quick as necessary) and perform those tasks when their time has come.

So instead of

  • loop(){
    • Wait for a certain time
    • Perform task 1
    • Wait for another time
    • Perform task 2
  • }

you design your code like this

  • loop() {
    • if (time is over for task 1 then
      • perform task 1
    • if (time is over for task 2) then
      • perform task 2
  • }

You still have to make sure that task 1 and task 2 do not block loop() too long. They may have to be cut into smaller pieces (regarding time consumption).

What you can see from the second pseudo code is, that if both times are not over loop() will run through as quick as the two if-statements allow ...

2 Likes

The repeated attaching / detaching of the interrupt is kind of sketchy. Better to attach it once -- in setup() -- and leave it that way. Then, have a volatile flag that indicates to the ISR whether it should do any processing or not.

Thank you EVERYONE for your responses. Once I get home tonight I will see what I can learn from all the comments made. I love forums like this as it perpetuates learning and hopefully steers me down the correct path.

Additionally, you have a TON of print statements, which at 9600 baud will cause 1.04ms delay between characters to print, due to the while() loops in print.cpp.

1 Like

They are only present until i no longer need to see what's going on but thank you. I intend to retire them when they are no longer necessary. In reality this sketch has nothing to do except look at the three triggers . The rest is killing time just to update the Oled counter.