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
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 
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.
If you let your ISR's get too big, you can end up painting yourself into a software corner
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:
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
/*
* 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();
}
awneil 
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:
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 ...
clawson 
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".
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.
clawson 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.
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).
theusch 
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?
clawson 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.
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
You can use interrupts with all functionality in your programs too... Main task then is sleeping or is doing unimportant things.
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. 