ISR Questions

Hi, I have a heart rate sensor from Adafruit (https://www.adafruit.com/products/1093). It includes code that calculates the users heart rate - it does so using interrupt 2 that runs every 2ms. It seems to work fine for the most part…
So, inside my loop I have a ‘critical section’ that reads the BPM value from the interrupt code, and I use that… what I do is set digital pins high/low to open and close a linear actuator based on the heart rate.
It all works - sort of. In my loop I basically do: if (currentBPM < lastBPM){ openActuator… it opens for 2 seconds and then stops.

The problem is that it seems to get stuck in a loop where when the actuator is moving the heart rate keeps going down, so the actuator just keeps moving…and the heart rate is not right. It’ll go down to like 30 BPM from 80 during this ‘stuck’ phase.

Problem is I cannot see the problem, and why I am here. I am wondering - is there something that prevents the interrupt from running? Would setting a digital pin high/low affect the reading from an analog pin?

My code is like 400 lines, so I didn’t want to post it… just hoping to figure out something and see if maybe the ISR is not running, etc.

My code is like 400 lines, so I didn't want to post it

Oh, go on. Force yourself.

I would but this lame forum software won't let me - it's too long...

OK, I tried.

Good luck.

Because everyone loves code in two parts (sorry - crap forum software)

//  Variables
int pulsePin = 0;                 // Pulse Sensor purple wire connected to analog pin 0

// Volatile Variables, used in the interrupt service routine!
int currentBPM; //set inside loop to BPM - in critical section

volatile int BPM; 
volatile int Signal;                // holds the incoming raw data
volatile int IBI = 600;             // int that holds the time interval between beats! Must be seeded! 
volatile boolean Pulse = false;     // "True" when User's live heartbeat is detected. "False" when not a "live beat". 
volatile boolean QS = false;        // becomes true when Arduoino finds a beat.

const int numEntries = 10;
volatile int rate[numEntries];                    // array to hold last ten IBI values
volatile unsigned long sampleCounter = 0;          // used to determine pulse timing
volatile unsigned long lastBeatTime = 0;           // used to find IBI
volatile int P = 512;                     // used to find peak in pulse wave, seeded
volatile int T = 512;                     // used to find trough in pulse wave, seeded
volatile int thresh = 525;                // used to find instant moment of heart beat, seeded
volatile int amp = 100;                   // used to hold amplitude of pulse waveform, seeded
volatile boolean firstBeat = true;        // used to seed rate array so we startup with reasonable BPM
volatile boolean secondBeat = false;      // used to seed rate array so we startup with reasonable BPM

const int MAX_HR_JUMP = 10; //won't allow the HR to vary more than this per reading

const int buttonPin = 7;//for resetting actuator
int buttonState = 0;

const int resetPin = 4; //for resetting heart rate to baseline
int resetState = 0;

const int relay1 = 12;   //Arduino pin that triggers relay #1
const int relay2 = 8;   //Arduino pin that triggers relay #2

int baseBPM; //defaults to 0 in setup()- set to BPM when start button pressed
int startBPM; //set to BPM when start button pressed

bool actuatorIsMoving = false;
float delayTime = 0;//time actuator moves for

bool userIsPlaying = false; //false by default, true when start button pressed
//bool ledIsOn = false;
//int ledTime = 0; //time 

const float openAmountPerBeat = 2000.0f; //open for 2 seconds = 20mm - for 10 beats to full open
int openCounter = 0; //number of times hr has gone down

int waitAfterActuator;


void setup()
{
  Serial.begin(115200);             // we agree to talk fast!
  interruptSetup();                 // sets up to read Pulse Sensor signal every 2mS    
  pinMode(buttonPin, INPUT);//7 - resetting actuator
  pinMode(resetPin, INPUT);//4 - resetting heart rate
  pinMode(relay1, OUTPUT);//12
  pinMode(relay2, OUTPUT);//8

  //turn of led - internal pullups keep it lit otherwise
  pinMode(13, OUTPUT);
  digitalWrite(13, LOW);
  
   baseBPM = 0;//BPM won't be lower than this at start
}



void loop()
{   
    //critical section
    //read BPM with interrupts off so it can't be modified while it's being accessed
    cli();
    currentBPM = BPM;
    sei();

    
    if (QS == true){     // A Heartbeat Was Found - BPM and IBI have been Determined 
        QS = false;      // reset the Quantified Self flag for next time  
        doOutput(true);  
    }else{
      doOutput(false);
    }

    buttonState = digitalRead(buttonPin);//actuator reset
    resetState = digitalRead(resetPin);//BPM reset

    if(actuatorIsMoving){
      if(millis() > delayTime){
        //Serial.println("actuator time - stopping"); 
        actuatorIsMoving = false;               
        stopActuator();//write both pins low
      }
    }   

    //RESET ACTUATOR  - reset for new user
    if(buttonState == HIGH){ 
     baseBPM = 0;
     startBPM = 0;  
     userIsPlaying = false;
     actuatorIsMoving = true;
     openCounter = 0;
     extendActuator();//set the pin high
     delayTime = millis() + 26000.0f; //extend for 20 seconds - full reset - flower is closed (10mm/sec * 8in(8 * 25.4 = 203.2)) 203.2/10 = ~20
    }

    //RESET USER - START
    if(resetState == HIGH){
      baseBPM = currentBPM;
      startBPM = currentBPM;
      openCounter = 0;
      userIsPlaying = true;
    }    

    //if current bpm is less than baseline - reset baseline to current and open the flower a litle
    if(userIsPlaying && !actuatorIsMoving){
      if(currentBPM < baseBPM){         
          baseBPM = currentBPM;
          actuatorIsMoving = true;
          retractActuator();//open flower
          openCounter++;
          if(openCounter >= 10){
            //flower is fully open
          }
          
          delayTime = millis() + openAmountPerBeat;
                 
      }else if (currentBPM > baseBPM){
        //MAKE FLOWER CLOSE?
      }
    }
     
    delay(20);
}

Part two because seriously this is the worst forum software. EVER. Not only can I not post all my code, its bitching because I make too many posts in five minutes. What garbage. Come on Arduino, buy some real software.

void extendActuator() 
{
  digitalWrite(relay1, LOW);
  digitalWrite(relay2, HIGH);
}



void retractActuator() 
{  
  digitalWrite(relay1, HIGH);
  digitalWrite(relay2, LOW);
}



void stopActuator() 
{
  digitalWrite(relay1, LOW);
  digitalWrite(relay2, LOW);
}



void interruptSetup()
{     
  // Initializes Timer2 to throw an interrupt every 2mS.
  TCCR2A = 0x02;     // DISABLE PWM ON DIGITAL PINS 3 AND 11, AND GO INTO CTC MODE
  TCCR2B = 0x06;     // DON'T FORCE COMPARE, 256 PRESCALER 
  OCR2A = 0X7C;      // SET THE TOP OF THE COUNT TO 124 FOR 500Hz SAMPLE RATE
  TIMSK2 = 0x02;     // ENABLE INTERRUPT ON MATCH BETWEEN TIMER2 AND OCR2A
  sei();             // MAKE SURE GLOBAL INTERRUPTS ARE ENABLED      
} 



// THIS IS THE TIMER 2 INTERRUPT SERVICE ROUTINE. 
// Timer 2 makes sure that we take a reading every 2 miliseconds
ISR(TIMER2_COMPA_vect){                         // triggered when Timer2 counts to 124
  cli();                                      // disable interrupts while we do this
  Signal = analogRead(pulsePin);              // read the Pulse Sensor 
  sampleCounter += 2;                         // keep track of the time in mS with this variable
  int N = sampleCounter - lastBeatTime;       // monitor the time since the last beat to avoid noise

    //  find the peak and trough of the pulse wave
  if(Signal < thresh && N > (IBI/5)*3){       // avoid dichrotic noise by waiting 3/5 of last IBI
    if (Signal < T){                        // T is the trough
      T = Signal;                         // keep track of lowest point in pulse wave 
    }
  }

I wonder why this even works 'sort of'.

     delayTime = millis() + 26000.0f; //extend for 20 seconds - full reset - flower is closed (10mm/sec * 8in(8 * 25.4 = 203.2)) 203.2/10 = ~20
ISR(TIMER2_COMPA_vect){                         // triggered when Timer2 counts to 124
  cli();                                      // disable interrupts while we do this

Why do you wonder? I don't see what's wrong... I am trying to set my delay time to 26 seconds - that's how long it takes the actuator to reset - it travels at 10mm per second. Does not millis() + 26000 equal 26 seconds from now?

And I didn't write the ISR code - it came with the heart rate sensor.

However - I am jacked to know something could be wrong - and maybe I can fix it.

Thank You!

OK - seriously Arduino Forum. I posted one comment in the past five minutes and now I can't reply to my own comment for a while. That is ridiculous and so very annoying.

dmen: Does not millis() + 26000 equal 26 seconds from now?

Most of the time it does. But sometimes it equals 49 days in the past and that will certainly break your code.

After a couple of posts you wont be restricted any more (100?).

Floats and millis don't get along well and are awful slow. All timing variables should be unsigned and, if used to hold millis or micros, long.

Adding to timestamps is asking for error. Even if it is not a float that is added.

It works if you use something like

if (millis() - previousTimestamp >= 26000) {
  }

Disabling interrupts in an ISR shows that the person who wrote it does not know what he was doing. ISRs are entered with interrupts disabled.

For fuc*'s sake.

Most of the time it does. But sometimes it equals 49 days in the past and that will certainly break your code.

I’m not sure what that means at all. Is it some overflow condition? Can you please explain?

Adding to timestamps is asking for error. Even if it is not a float that is added.

Why? I feel like a total buffoon right now as I just don’t grasp this. Why is taking NOW and adding 26000 to it improper? I can do it in other languages…

Disabling interrupts in an ISR shows that the person who wrote it does not know what he was doing.

I assume the code was from the hardware vendor? And does it matter? If it’s already disabled it won’t matter right? Just two lines of code that are irrelevent? Or??

Go grab your Arduino and pop in a quick sketch to get the result of this calculation.

unsigned long num = 4294967000;
unsigned long result = num + 1000;
Serial.print(result);

Does the answer surprise you?

Yes, it does overflow. Here’s a page that explains how to avoid the overflow ‘problem’ entirely. The suggested code will work “forever” Gammon Forum : Electronics : Microprocessors : Timers and counters

You didn’t add 26000 to it. You added a floating-point approximation of 26000. Floats can’t represent most integers exactly. They usually get rounded off for display so you don’t get to see the complexity of the floating-point representation.

Then there’s the problem that an 8-bit Arduino has to do a lot of operations to add floating-point numbers. It’s something like 20 to 100 times more complex to add floats than integers. Then, in this example, it must be converted back to an integer, which also takes time.

If you have two lines of code that are irellevant, try deleting them.

dmen: Does not millis() + 26000 equal 26 seconds from now?

Suppose millis() returns 4294942296. Add 26000 to that, and you get 1000. 4294942296 - 1000 = 4294941296.

dmen: OK - seriously Arduino Forum. I posted one comment in the past five minutes and now I can't reply to my own comment for a while. That is ridiculous and so very annoying.

You are illustrating why there is such a limitation.

If you notice that your car mechanic is unable to use a wrench, would it matter to you?

ISRs should be as fast as possible, hence contain no irrelevant code.

Thank you all very much, I really appreciate the help. I’m going to clean up my code now and give it a try.

aarg:
You are illustrating why there is such a limitation.

Gotcha! Thanks!