calculations inside ISR, right or wrong

Go To Last Post
16 posts / 0 new
Author
Message
#1
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

hello everyone

 

I have interrupt service routine for ADC and I wrote calculations (for pwm duty cycle) inside it (things look unstable), is it wrong to do the calculations inside the ISR, I am writing my code on atmel studio 7 and I am using the chip atmega 328p.

Help please

 

Last Edited: Mon. Apr 2, 2018 - 09:13 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Show your code.

So you probably heared somewhere that it is "bad" to do "long calculations" in ISR's, but when does it start getting bad and why?

 

The reasons are a bit diffult to explain.

If you only have a single ISR, then it does not matter much, you can spend all the time you like in the ISR.

The problem is that if you have multiple ISR's, an ISR can not be interrupted (please keep it simple here) by another ISR.

Therefore the general idea is to let all your ISR's do time critical things, but keep it short, and let the background normal functions handle the rest.

Show us your code and wee will judge it smiley

Doing magic with a USD 7 Logic Analyser: https://www.avrfreaks.net/comment/2421756#comment-2421756

Bunch of old projects with AVR's: http://www.hoevendesign.com

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

If you only have a single ISR, then it does not matter much, you can spend all the time you like in the ISR.

Well not exactly, since it could be in the isr too long and miss polling the track sensor to switch the trainload of coal over to track #5.

 

It may be simpler & permissive to put some calcs in the isr...only you can determine the acceptable threshold.  If you let your ISR's get too big, you can end up painting yourself into a software corner, when other ISR's later need added, or the main code adds moderate priority items to its "to do" list.

 

When in the dark remember-the future looks brighter than ever.   I look forward to being able to predict the future!

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

avrcandies wrote:
If you let your ISR's get too big, you can end up painting yourself into a software corner
Totally agree here. Being a bit conservative and keeping track of the time in interrupts might save you from a lot of (debugging) headaches later.

Developing good habits also helps a lot in writing quality code.

 

But it can also be a good experience to follow the wrong path for a while, just to experience what sort of trouble it leads to. Personal experience is a good teacher, even if the experience itself was bad.

In this post I have made some remarks to debug software with a USD 5 logic analyser and Sigrok.

That way you can easily measure the interrupt time and lots of other stuff:

https://www.avrfreaks.net/comment/2421756#comment-2421756

Doing magic with a USD 7 Logic Analyser: https://www.avrfreaks.net/comment/2421756#comment-2421756

Bunch of old projects with AVR's: http://www.hoevendesign.com

Last Edited: Mon. Apr 2, 2018 - 04:42 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

One of the most common sources of "strange behavior" in an ISR is the failure to declare variables as "volatile" when they are shared by the ISR and the main body of code.

 

But, at the very least, you need to provide the  ISR code. We do not have the capability to guess what might be wrong with nothing to go on.

 

Jim

 

Until Black Lives Matter, we do not have "All Lives Matter"!

 

 

Last Edited: Mon. Apr 2, 2018 - 06:36 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0
/*
 * Author : Moraa
 */ 

#include <avr/io.h>
#include <avr/interrupt.h>

////////////////////////////////////////////////////////////////////////////////

volatile signed int duty=0;
volatile  char feedback=0;
volatile signed int  errorn=0;
volatile signed  int error_prev1=0;
volatile signed  int error_prev2=0;

////////////////////////////////////////////////////////////////////////////////////

volatile signed int  CA=0;
volatile  signed int ca_prev1=0;
volatile  signed int ca_prev2=0;

////////////////////////////////////////////////////////////////////////////////////

void setup();

int main(void)
{
	setup();
    while (1)
    {
	 }
}

void setup()
{

// Test points configuration:
//---------------------------

	DDRB|=(1<<DDB1); //configuring pin 1 in port b to be output // pin 15 //pwm
        DDRC|=(0<<DDC0); //configuring analog channel 0 to be input //PIN 23 //feedback
DDRC|=(0<<DDC3); //configuring analog channel 3 to be input //PIN 26 //feedback 2
	DDRD|=(1<<DDD4); //configuring pin 4 in port D to be output // pin 6 //test pin
	DDRD|=(1<<DDD7); //configuring pin 7 in port D to be output // pin 13 //test pin
	DDRD|=(1<<DDD6);

//////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////

//Timer 0 setup:
//---------------

//Set Initial Timer value
    TCNT0=0b00000000;
//Place TOP timer value to Output compare register
	OCR0A = 39;  //it should be 39 for 50k // compare match register {(16MHz*20 usec)/8}-1
    OCR0B=0;
//Set CTC mode
//and make toggle PD6/OC0A pin on compare match
	TCCR0A = 0;
	TCCR0A |= (1<<COM0A0)|(1 << WGM01);   // CTC mode
//Set pre-scaler 8 and start timer
	TCCR0B = 0;
    TCCR0B |=(0<<CS02)|(1<<CS01)|(0<<CS00); //8 pre-scaler

//////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
     TIMSK0 |= (1 << OCIE0A);  // enable timer compare interrupt

//Timer 1 setup:
//---------------

	TCCR1A=0b00000000;
	TCCR1B=0b00000000;
	TCCR1A=(1<<COM1A1)| (0<<COM1A0)|(1<<COM1B1)|(1<<COM1B0)|(0<<WGM11)|(0<<WGM10);
	TCCR1B=(0<<ICNC1)|(0<<ICES1)|(1<<WGM13)|(0<<CS12)|(0<<CS11)|(1<<CS10); //1 pre

	TCNT1H=0x00;
	TCNT1L=0x00;

	ICR1H=0x00;
	//ICR1L=0x64;  // 100 decimal
	ICR1L=0xFF; // for testing ADC , pwm f=1000 hz

//////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////

// ADC setup:
//-----------

// Select Vref=AVcc
//and set left adjust result
    //ADMUX=0;
	ADCSRA=0;
    ADMUX |= (1<<ADLAR)|(1<<REFS0)|(0<<REFS1);

//set pre-scaler to 8
//enable auto-triggering
//enable ADC interrupt
//and enable ADC
   ADCSRA=0;
   ADCSRA |=(1<<ADPS0)|(1<<ADPS1)| (0<<ADPS2)|(1<<ADATE)|(1<<ADIE)|(1<<ADEN);
//triggered by compare match a of timer0 compare match A
   ADCSRB=0;
   ADCSRB |=(1<<ADTS0)|(1<<ADTS1)|(0<<ADTS2);
//select ADC channel
 ADMUX|=(1<<MUX0)|(1<<MUX1)|(0<<MUX2)|(0<<MUX3); // channel 0
//DIDR0 |=(1<<DIDR0); // digital input disabled register --channel 0

//Start ADC
    ADCSRA |=(1<<ADSC);
	sei();
//////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
}

ISR(TIMER0_COMPA_vect)
{
		//empty
}

ISR(ADC_vect)          // timer compare interrupt service routine
{
//cli();
	PORTD |= (1 <<PORTD4) ;
  //clear timer compare match flag

   TIFR0=(1<<OCF0A);

   feedback=ADCH; // contains 8 bits that express the reading of voltage.

		//GAIN Controller

	errorn=128-feedback; //bits

	if(errorn >32)
{
	CA=errorn*15;
}
else
{
	//CA=(70*(errorn-error_prev1))+(5*ca_prev1); //checked 	

CA=((67*errorn)+(3*error_prev1)-(64*error_prev2)+(0.25*ca_prev1)+(0.75*ca_prev2));

}

	duty=CA;

	if(duty>230)
	{
		duty=230;
	}

	if(duty<0)
	{
		duty=0;
	}

	OCR1AL=duty;
	error_prev2=error_prev1;
	error_prev1=errorn;
	ca_prev2=ca_prev1;
	ca_prev1=CA;
	PORTD &= !(1 <<PORTD4) ;
//sei();
}

 

Last Edited: Mon. Apr 2, 2018 - 09:16 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

ka7ehk wrote:
One of the most common sources of "strange behavior" in an ISR is the failure to declare variables as "volatile" when they are shared by the ISR and the main body of code.

Indeed (although the "strange behaviour" usually shows up in the "main body" rather than the ISR itself).

 

And one of the most common sources of  "poor performance" on simple controllers is a failure to understand the impact of Floating Point:

 

Moraa wrote:

CA=((67*errorn)+(3*error_prev1)-(64*error_prev2)+(0.25*ca_prev1)+(0.75*ca_prev2));

Doing floating-point calculation is (relatively) "hard work" for a simple, 8-bit microcontroller with no hardware FP support - such as an AVR

 

As the calculations here are all quarters, it should be possible to do that with just shifts ...

 

 

Top Tips:

  1. How to properly post source code - see: https://www.avrfreaks.net/comment... - also how to properly include images/pictures
  2. "Garbage" characters on a serial terminal are (almost?) invariably due to wrong baud rate - see: https://learn.sparkfun.com/tutorials/serial-communication
  3. Wrong baud rate is usually due to not running at the speed you thought; check by blinking a LED to see if you get the speed you expected
  4. Difference between a crystal, and a crystal oscillatorhttps://www.avrfreaks.net/comment...
  5. When your question is resolved, mark the solution: https://www.avrfreaks.net/comment...
  6. Beginner's "Getting Started" tips: https://www.avrfreaks.net/comment...
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Just to say that multiply by 0.25 is the same as /4 and the latter will stick to integers. Similarly * 0.75 is the same as multiply by 3 then divide by 4 which will also avoid floating point (but always do the multiply before the divide)

 

BTW there is no reason why any variable in that code as it is now need be "volatile".

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I think this is one of the situations where it doesn't matter where you do the calculations and in fact it makes more sense to do it in the ISR.

 

I say this because it looks as though you are filtering the output of an ADC which is regularly sampling a signal. So you cannot miss a sample or skip a filter step or else the output will be wrong. So any calculation MUST complete in the time between ADC samples. So any attempt to move the calculation out of the ISR is not worth doing. The only exception would be if something else, in the form of another interrupt, must have a higher priority than the calculation and cannot wait until the maths is finished.

#1 Hardware Problem? https://www.avrfreaks.net/forum/...

#2 Hardware Problem? Read AVR042.

#3 All grounds are not created equal

#4 Have you proved your chip is running at xxMHz?

#5 "If you think you need floating point to solve the problem then you don't understand the problem. If you really do need floating point then you have a problem you do not understand."

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Well you might as well forget interrupts all together and just poll ADSC then do the calculations immediately. With interrupts you waste something like 12..20 cycles getting into the ISR and if float cannot be avoided it'll be considerably more for all the PUSHs.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

How fast do you need to actually sample?  I saw something referring to 50KHz, which of course is much faster than the ADC conversion limit of about 15KHz (in full res).

When in the dark remember-the future looks brighter than ever.   I look forward to being able to predict the future!

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

clawson wrote:
With interrupts you waste something like 12..20 cycles getting into the ISR

I've lost track of the objectives here.  I think we definitely need more information about what "unstable" is, and the app requirements.

 

I generally use 12 cycles as the average "overhead" to get into an out of an AVR8 ISR.  But so what -- what difference does an extra microsecond or two make, when a conversion takes about 100us, and PWM period may well be more than that? 

 

Now, OP has a gratuitous interrupt enabled for timer0.  For what purpose -- to throw off timing?

 

Back to "unstable" -- is this one of the timer setups where the TOP is not double-buffered. so OP is getting runt cycles?

 

But yes -- why add floating point at all?  Why not make a slim transfer function, and do some cycle-counting?

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Lee, maybe we've seen it but I'm not sure how fast this AVR is being run at but let's say it's somewhere down near the 1MHz but perhaps some speed chosen to run the ADC at the optimal 200kHz so the conversion time is 66us and there's therefore best case about 66 cycles to get all the calculations done. I'd say that in that realm the 12..20 cycles it "costs" to get into an ISR could be fairly significant. Of course if this is a 16MHz Arduino (328 says maybe it is?) then, yes, the ISR cost is probably irrelevant. 

 

Of course all that pales into insignificance when float is bandied about. 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

 is it wrong to do the calculations inside the ISR,

I think one of the items to keep in mind, not yet mentioned, is your "high level" project design at the beginning of the project.

What are the system inputs?

What are the system outputs?

What is time critical?

How are the various parts of the project inter-related?

 

As was already mentioned, one of the key differences between running code within an ISR and from within the Main Loop is that the ISR (generally) isn't interruptible, while the Main Loop is. 

So if you have a really long processing code segment within the ISR it can interfere with other ISRs, causing them to be missed.

Clearly whether or not this is an issue depends upon the specific project.

 

You can interrupt an ISR on the Mega or Tiny, but that can easily cause a stack overflow if one isn't careful.

 

The Xmega series have a three level priority interrupt controller, so higher priority, "can't miss this one", interrupts can interrupt a lower priority interrupt, or the Main Loop code.

 

On occasion I've had a program or two where almost everything ran within interrupts.

 

It is clearly a matter of what your program needs to accomplish, and how you have structured it, as to whether or not "excessive" ISR code is, in fact, excessive or reasonable.

 

There are often many ways to successfully accomplish a given task.

 

JC 

 

 

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

You can use interrupts with all functionality in your programs too... Main task then is sleeping or is doing unimportant things.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

The OP needs to say how fast he needs to read the ADC & react, among other things.  No need for floating point....maybe no need for signed either, with careful design.  Of course, signed is nothing like trying to use float.

 

... Main task then is sleeping or is doing unimportant things.  

I've been quite successful at it & am hoping to increasingly use this technique. laugh

When in the dark remember-the future looks brighter than ever.   I look forward to being able to predict the future!