ADC ISR won't run when conversion is complete

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

First, a little bit about my setup:

- I use an AT90CAN128

- LED is connected to PB4

- I use ADC2 which is connected to PF2

- I use a potensiometer to change the PWM duty cycle to adjust the brightness of the LED.

 

See the full code at Github.

 

My problem is that the ISR with ADC_vect is doesn't run even though the conversion is done. When the program starts, it initializes the ADC and calls the start_conversion() function which basically sets the ADSC bit in the ADCSRA register. In the datasheet it says that when the conversion is done (and interrupts are enabled, which they are), the ADIF flag will be set and the ISR will run and reset the ADIF flag. However, this does not happen.

 

I've tested a lot of testing to come to the conclusion that the ISR doesn't run. Here are my findings:

- If I set a break point in the ISR, it never breaks the program.

- If I look at the value of ADCSRA before I set the ADSC bit, it reads 1000 1110, as it should (this is just after the setup, so no anomalies here). When I first run the start_conversion() function, it sets the ADSC bit, as it should. When I pause the program after it has done one reading and look at the value of ADCSRA, it reads 1001 1110. The important thing to note here is that the ADIF (interrupt flag) is set, but not cleared by the ISR.

- If you see in the while(1) {...} loop in main(), the code there works just fine. I can read the ADC value just right. This indicates that there isn't anything wrong with the ADC.

- If I replace the ADC_vect with the catch-all BADISR_vect, it also doesn't run.

- If I connect a LED to a different pin and set that pin high in the ISR, the LED never turns on, i.e. the ISR doesn't run.

 

I find it really weird that I can't get the ISR to work with the ADC, even though it does work with the PWM. I have tried to change the chip to see if there's a problem with the other one, but the problem presists.

 

If anyone has any idea about what could be the problem here, please let me know. I've been scratching my head over this for days and not come any further. I'll happily try any suggestions/answer any question to clarify the situation.

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

You set MUX for ADC2:

ADMUX |= (1<<MUX1);				// set which adc to use (here: ADC2)

but then enable ADC1:

DIDR0 |= (1<<ADC1D);

 

 

David (aka frog_jr)

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0
int duty_cycle = 50;

duty_cycle is shared between two threads of exection (main and the ISR), so it must be volatile:

volatile int duty_cycle = 50;

 

"Experience is what enables you to recognise a mistake the second time you make it."

"Good judgement comes from experience.  Experience comes from bad judgement."

"When you hear hoofbeats, think horses, not unicorns."

"Fast.  Cheap.  Good.  Pick two."

"Read a lot.  Write a lot."

"We see a lot of arses on handlebars around here." - [J Ekdahl]

 

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

And please don't force others to visit another site to get your code.  Post it here:

 

 

/*
 * PWM.c
 *
 * Created: 15.10.2017 14.33.24
 * Author : eirik
 */ 

#define F_CPU 8000000UL

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

int duty_cycle = 50;

void setup_ADC();
void start_conversion();


int main(void)
{
	DDRB |= (1<<PB4);					// set output
	
	TCCR2A |= (1<<WGM20)|(1<<WGM21);	// set fast PWM mode
	TCCR2A |= (1<<COM2A1);				// will set pin high or low depending on where in the timer cycle it is
	TIMSK2 |= (1<<TOIE2);				// enable overflow interrupt flag
		
	setup_ADC();
	
	sei();								// enable global interrupt flag
	
	TCCR2A |= (1<<CS20);				// start timer with 1 prescalar (aka no prescalar)
	
	
    /* Replace with your application code */
    while (1) 
    {
		// This code works
		while (ADCSRA & (1<<ADSC));
		duty_cycle = ADC;
		start_conversion();
    }
}


void start_conversion()
{
	ADCSRA |= (1<<ADSC);			// start conversion
}

void setup_ADC()
{
	ADMUX |= (1<<REFS0);			// set ref. voltage
	ADMUX |= (1<<MUX1);				// set which adc to use (here: ADC2)

	ADCSRA |= (1<<ADEN);			// enable adc
	ADCSRA |= (1<<ADIE);			// enable adc interrupt
	ADCSRA |= (1<<ADPS2)|(1<<ADPS1);// set adc prescalar 64
	
	DIDR0 |= (1<<ADC1D);
	
	start_conversion();
}


ISR(ADC_vect)
{
	// The ISR is never run, even though the ADIF is set 
	duty_cycle = ADC;
	start_conversion();
}


ISR(TIMER2_OVF_vect)
{
	OCR2A = (duty_cycle/1023.0) * 255;
}

 

"Experience is what enables you to recognise a mistake the second time you make it."

"Good judgement comes from experience.  Experience comes from bad judgement."

"When you hear hoofbeats, think horses, not unicorns."

"Fast.  Cheap.  Good.  Pick two."

"Read a lot.  Write a lot."

"We see a lot of arses on handlebars around here." - [J Ekdahl]

 

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

You need to access duty_cycle in main atomically (i.e use ATOMIC_BLOCK):

ATOMIC_BLOCK(ATOMIC_FORCEON) {
    duty_cycle = ADC;
}

Since a timer interrupt could occur between the reading of the low and high bytes of the ADC.

 

Edit: (If getting the duty_cycle in main vs ADC ISR.)

David (aka frog_jr)

Last Edited: Fri. Oct 20, 2017 - 06:04 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

eirik-ff wrote:
. In the datasheet it says that when the conversion is done (and interrupts are enabled, which they are), the ADIF flag will be set and the ISR will run and reset the ADIF flag.

I kind of do not like going outside to look at code.  A test program for this is indeed a screenful or two of code; post it here.

 

frog_jr wrote:
but then enable ADC1:

Shouldn't matter.

 

=========

Anyway, you have the timer ISR firing every 256 AVR clock cycles.  And there is a floating-point calculation each time.  Now it is time to pull up the datasheet for your AVR model.  My guess:  The timer interrupt is a higher priority than the ADC interrupt, and thus is continually firing and the ADC interrupt is starved.

 

In main() it >>appears<< to work, because there is one main line instruction done between each ISR action.

 

If you really want that fast of a response, then use ADLAR and just plunk the ADCH into OCR0A.  (or just do ADCW/4)  Or have a flag, and only do the OCR0A update at some formula.  Or slow down the timer; anything over ~100 cycles/second should be OK.

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

frog_jr wrote:
You need to access duty_cycle in main atomically (i.e use ATOMIC_BLOCK):

It has to be volatile to work with the interrupts! (and atomic access not an issue when main loop is empty)

 

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

frog_jr wrote:

You need to access duty_cycle in main atomically (i.e use ATOMIC_BLOCK):

ATOMIC_BLOCK(ATOMIC_FORCEON) {
    duty_cycle = ADC;
}

Since a timer interrupt could occur between the reading of the low and high bytes of the ADC.

 

Edit: (If getting the duty_cycle in main vs ADC ISR.)

Yes, a crucial point, and a good reason to use inherently atomic variables (8 bits in the case of AVR) in these circumstances whenever possible.  

Just a -tiny- elaboration - getting OR setting in main, when there's a chance of any ISR accessing the variable.