Is this interrupt function small enough?

Hello,

I am having some problems as my arduino stucks sometimes while executing an interrupt, I tried to slim down the interrupt function (removed EEPROM write) so this is the function:

void quicfunc() {
  float timebetweenintervals;
  if(digitalRead(WATERCLOCK)==1)
    if((long)(micros() - last_micros) >= debouncing_time * 1000) {

      if(now()-LastSampleTime<20)
      {
        timebetweenintervals=  (long)(micros() - last_micros)/1000;
        last_micros = micros();

        WaterMeterCounter++;
        digitalWrite(6, 1-digitalRead(6)); 
      
        //  WaterFlow=(360*ClockSize)/(float)((WaterClockPulseTime[0]-millis()/100)/10);//divide by 10000 to calibrate it to the clock size, divide by 10 to make it seconds //1000* to make it liters, 3600* to make it per 
        WaterFlow=approxRollingAverage(WaterFlow, 10000*(float)(360*ClockSize)/timebetweenintervals);
      }
      LastSampleTime=now();  
    }


}

I know interrupts should be as small as possible, is this small enough? (the digital Write is not supposed to be there on the final code, its just for debugging)

float timebetweenintervals; :o

srusha: I know interrupts should be as small as possible, is this small enough?

Is this a question for mind-readers only?

I'm not a mind-reader and I don't know what function "now()" may be.

Is the execution time of "now()" one microsecond? Or one millisecond? One second? Or half a day?

Is the execution of "now()" interrupt-safe?

I don't know nothing at all. I'm not a mind-reader. I'm just an Arduino programmer.

So possibly this question is not for me.

P.S.: If I should act as a mind-reader and make a guess, I'd say that the call of function "now()" from within an interrupt handling routine is misplaced.

(float)(360*ClockSize)

If you simply multiply by 360.0, the cast would not be necessary.

As is, the result of the multiplication depends on the type of ClockSize. If ClockSize is an int, with a value greater than 10, you will have overflow, and the cast to a float won't solve THAT problem.

srusha:
I know interrupts should be as small as possible, is this small enough? (the digital Write is not supposed to be there on the final code, its just for debugging)

What about something like this

void quickerFunc() {
    funcTriggeredMicros = micros();
    newTrigger = true;
}

…R

srusha: I know interrupts should be as small as possible, is this small enough? (the digital Write is not supposed to be there on the final code, its just for debugging)

There is no one right answer to that question. Does the program do what you need it to do? Does the rest of the program remain responsive? If so, then you're ok.

Making ISRs as short as reasonably possible is a good guideline, not a hard and fast rule. Sometimes, ISRs have to do a lot of work. For example, my current project runs several DC servo motors concurrently, and all the PID control loops, which run much more code than you are have, are run on timer interrupts. The whole system works just fine, and is still responsive to user input at all times.

You can profile the code, to measure how long it takes for the ISR to run - use micros() to time how long it takes to run, and print the result. As long as you're not anywhere close to consuming 100% of the CPU running the ISR, you should be fine. How close is "close" depends on what else the system is doing and what the performance constraints are. In my system, the PID loops are called by a timer interrupt, run every mSec when the motors are moving, and consume about 70% of the CPU. This causes no problems whatsoever.

Regards, Ray L.

AWOL: float timebetweenintervals; :o

Whats the problem with that?

PaulS: (float)(360*ClockSize)

If you simply multiply by 360.0, the cast would not be necessary.

As is, the result of the multiplication depends on the type of ClockSize. If ClockSize is an int, with a value greater than 10, you will have overflow, and the cast to a float won't solve THAT problem.

ClockSize is float, so I guess the casting is redundant.. thanks for the comment

jurs: Is this a question for mind-readers only?

I'm not a mind-reader and I don't know what function "now()" may be.

Is the execution time of "now()" one microsecond? Or one millisecond? One second? Or half a day?

Is the execution of "now()" interrupt-safe?

I don't know nothing at all. I'm not a mind-reader. I'm just an Arduino programmer.

So possibly this question is not for me.

P.S.: If I should act as a mind-reader and make a guess, I'd say that the call of function "now()" from within an interrupt handling routine is misplaced.

No, you are not a mind reader. but now is a well defined arduino function: http://playground.arduino.cc/Code/Time

You dont have to be a jurk about it.

srusha: Whats the problem with that?

What purpose could it possibly serve to keep time in a float besides making the numbers very inexact and opening you up to some very confusing and hard to find bugs. Ever try to chase down why 12 / 4 == 2?

RayLivingston: There is no one right answer to that question. Does the program do what you need it to do? Does the rest of the program remain responsive? If so, then you're ok.

Making ISRs as short as reasonably possible is a good guideline, not a hard and fast rule. Sometimes, ISRs have to do a lot of work. For example, my current project runs several DC servo motors concurrently, and all the PID control loops, which run much more code than you are have, are run on timer interrupts. The whole system works just fine, and is still responsive to user input at all times.

You can profile the code, to measure how long it takes for the ISR to run - use micros() to time how long it takes to run, and print the result. As long as you're not anywhere close to consuming 100% of the CPU running the ISR, you should be fine. How close is "close" depends on what else the system is doing and what the performance constraints are. In my system, the PID loops are called by a timer interrupt, run every mSec when the motors are moving, and consume about 70% of the CPU. This causes no problems whatsoever.

Regards, Ray L.

My problem is that my code hangs when the interrupt is called. I'm not sure why. trying to pin point the problem but it only occurs after 2 days ~ of running.

srusha: No, you are not a mind reader. but now is a well defined arduino function: http://playground.arduino.cc/Code/Time

You dont have to be a jurk about it.

You're right, now is in the Time library, but that doesn't make it an Arduino function. At no point did you indicate that you were using the Time library and now() could be defined to be something different in your sketch or in another library.

Now, don't call people jerks if you don't know what you're talking about.

srusha: My problem is that my code hangs when the interrupt is called. I'm not sure why. trying to pin point the problem but it only occurs after 2 days ~ of running.

Unless you want to let us see the rest of the code so we can help you, then all we can do is wish you the best of luck. It doesn't look like a problem in the ISR from here without seeing anything else.

Delta_G:
Unless you want to let us see the rest of the code so we can help you, then all we can do is wish you the best of luck. It doesn’t look like a problem in the ISR from here without seeing anything else.

OK

thats the thing I wanted to ask about, so you think this interrupt looks OK and it shouldn’t cause the arduino to halt?

Thats a good conclusion for me…

srusha: OK

thats the thing I wanted to ask about, so you think this interrupt looks OK and it shouldn't cause the arduino to halt?

Thats a good conclusion for me..

No, I didn't say that. I said that without seeing the rest of the code I couldn't see anything wrong with it. It still may be broken, but it is impossible to tell withotut the rest of the code.

Did you read the entire respose there or just the one like you wanted to hear?

You're also calling this function that we can't see..

WaterFlow=approxRollingAverage(WaterFlow, 10000*(float)(360*ClockSize)/timebetweenintervals);
      }

And a bunch of variables we can't see the declarations for.

srusha:
No, you are not a mind reader. but now is a well defined arduino function: http://playground.arduino.cc/Code/Time

You dont have to be a jurk about it.

I think, you are mixing something up.

The Arduino IDE has no time library with it.
So you are talking about some third-party library.

And that time library you are pointing at seems to be “by Paul Stoffregen”.

OK, let’s have a look at the “now()” function of his library:

time_t now() {
  while (millis() - prevMillis >= 1000){      
    sysTime++;
    prevMillis += 1000; 
#ifdef TIME_DRIFT_INFO
    sysUnsyncedTime++; // this can be compared to the synced time to measure long term drift     
#endif
  }
  if (nextSyncTime <= sysTime) {
    if (getTimePtr != 0) {
      time_t t = getTimePtr();
      if (t != 0) {
        setTime(t);
      } else {
        nextSyncTime = sysTime + syncInterval;
        Status = (Status == timeNotSet) ?  timeNotSet : timeNeedsSync;
      }
    }
  }  
  return (time_t)sysTime;
}

Looks as if this library will even call further external functions if they are set.

getExternalTime getTimePtr; // pointer to external sync function

Have you set up your system getting time from an external source? RTC? NTP?
It can be a relatively time consuming process, to receive time from an external source.
If so, I’d say: Calling that function within an interrupt handler is NOT INTERRUPT-SAFE.

Funktion “now()” must only be called from normal code, not from within interrupt handling routine. At least, if you have set up the time library to synchronize time with an external source (like RTC or something else).

jurs:
I think, you are mixing something up.

The Arduino IDE has no time library with it.
So you are talking about some third-party library.

And that time library you are pointing at seems to be “by Paul Stoffregen”.

OK, let’s have a look at the “now()” function of his library:

time_t now() {

while (millis() - prevMillis >= 1000){     
    sysTime++;
    prevMillis += 1000;
#ifdef TIME_DRIFT_INFO
    sysUnsyncedTime++; // this can be compared to the synced time to measure long term drift   
#endif
  }
  if (nextSyncTime <= sysTime) {
    if (getTimePtr != 0) {
      time_t t = getTimePtr();
      if (t != 0) {
        setTime(t);
      } else {
        nextSyncTime = sysTime + syncInterval;
        Status = (Status == timeNotSet) ?  timeNotSet : timeNeedsSync;
      }
    }
  } 
  return (time_t)sysTime;
}




Looks as if this library will even call further external functions if they are set.

getExternalTime getTimePtr; // pointer to external sync function

Have you set up your system getting time from an external source? RTC? NTP?
It can be a relatively time consuming process, to receive time from an external source.
If so, I'd say: Calling that function within an interrupt handler is NOT INTERRUPT-SAFE.

Funktion "now()" must only be called from normal code, not from within interrupt handling routine. At least, if you have set up the time library to synchronize time with an external source (like RTC or something else).

Thanks a bunch