ADC interrupt enabled causing problem

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

Hello guys.  Can I ask for your help.  You probably have the solution and answer to my question and problem. 

I am using Arduino Uno starter kit and learning PWMs and interrupts.  

I have analog input through potentiometer and I am enabling PortB PB0 and PB4.  Trigger point is between 1V to 3V.  This is working fine.

I also enable a PWM with output on OCR0A PD6 through input pin PC5.  This is also working fine.

I also enable an ADC interrupt to control the brightness of LED at output OCR2B PD3.  I can vary the brightness using potentiometer as well.

However after enabling this ADC interrupt I found out that the ADC controlled PB0 and PB4 are not working properly.

Now I could not control them.  Do you know why is the ADC interrupt causing problem to the other ADC controlled outputs PB0 and PB4?

I have the code below.  I appreciate if you can give me some hint.  Thank you.


#include <avr/io.h>
#define F_CPU 16000000
#include <util/delay.h>
#include <avr/interrupt.h>


void ADC_init(void)
{
	ADMUX|=(1<<REFS0);                                                              //use VCC as reference
	ADCSRA|=(1<<ADEN)|(1<<ADATE)|(1<<ADIE)|(1<<ADPS2)|(1<<ADPS1)|(1<<ADPS0);        //use prescaler 128  = 16000000/128 = 125k 
	sei();															
}																					

uint16_t ADC_read(uint8_t ch)
{
	ch&=0x07;
	ADMUX&=0xF8;
	ADMUX|=ch;
	ADCSRA|=(1<<ADSC);
	while(!(ADCSRA&(1<<ADIF)));
	ADCSRA|=(1<<ADIF);
	return (ADC);
}


void PWM0_init(void)
{
	TCCR0A|=(1<<COM0A1)|(1<<COM0A0)|(1<<WGM00)|(1<<WGM01);                   //Clear OC0A on Compare Match.  Set to fast PWM 0xFF as top.  Inverted.
	TCCR0B|=(1<<CS01)|(1<<CS00);						// Set prescale to 64.
}


void PWM2B_init(void)
{
	TCCR2A|=(1<<COM2A1)|(1<<COM2A0)|(1<<COM2B1)|(1<<COM2B0)|(1<<WGM20);				//Set OC2A/OC2B on Compare Match when up-counting. Clear OC2A/OC2B on
	TCCR2B|=(1<<WGM22)|(1<<CS21);									//compare Match when down-counting.  Inverted.
	OCR2A=255;											//Set to PWM, Phase Correct PWM.  Top is OCR2A.  Set prescale to 8																					 
}


uint16_t debouncer2_init(void)
{
	if(bit_is_clear(PINC, PC5))                                 //set to active low
	{
		_delay_ms(50);                                      //debounce by 50ms
		if(bit_is_clear(PINC, PC5));
		{
			return (1);
		}
	}
	return (0);
}


int main(void)
{
	DDRB=0x11;                                                  //set PB0 and PB4 as output
	DDRD=0x48;                                                  //set PD3 and PD6 as output

	ADC_init();	
	uint16_t AD_value2;
	PWM0_init();
	PWM2B_init();
	
	while(1)
	{	
		{												
			AD_value2=ADC_read(2);
			if(AD_value2>=613)                                          //set threshold to 3V
			{
				PORTB|=(1<<0);                                      //enable PortB PB0				
			}
			else if((AD_value2>204)&&(AD_value2<613))                   //set trigger between 1V and 3V
			{
				PORTB|=(1<<4);                                      //enable PortB PB4				
			}
			else
			{
				PORTB&=~(1<<4);                                      //disable PortB PB4
				PORTB&=~(1<<0);			                     //disable PortB PB0
			}			
		}
		
		{
			if(debouncer2_init())
			{
				OCR0A=64;                                                    //set to 980Hz
			}																 //set to 75%duty cycle = 0.75x255 = 191.  Inverted PWM
			else
			{
				OCR0A=255;                                                   //disable OCR0A.  Inverted PWM
			}
		}	
	}
}


ISR(ADC_vect)
{	
		OCR2B=255-(ADC_read(1)/4);						   //adjust duty cycle	
}




 

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

Why on earth are you mixing asynchronous and synchronous? Execution will arrive in ISR(ADC_vect) for the very reason that a conversion if complete, the ADIF flag has been raised and that has triggered ADC_vect to be entered. but then you do:

uint16_t ADC_read(uint8_t ch)
{
	ch&=0x07;
	ADMUX&=0xF8;
	ADMUX|=ch;
	ADCSRA|=(1<<ADSC);
	while(!(ADCSRA&(1<<ADIF)));
	ADCSRA|=(1<<ADIF);
	return (ADC);
}

Where you are then changing the MUX, triggering a reading and then waiting for ADIF to signal completion. Surely your vector (assuming "free running") just wants to be:

ISR(ADC_vect)
{	
		OCR2B = 255 - (ADCH / 4);						   //adjust duty cycle	
}

If you really do want to use more than one channel then do NOT use interrupts/free running because you hit the problem of the reading lagging the mux selection.

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

clawson wrote:

Why on earth are you mixing asynchronous and synchronous? Execution will arrive in ISR(ADC_vect) for the very reason that a conversion if complete, the ADIF flag has been raised and that has triggered ADC_vect to be entered. but then you do:

uint16_t ADC_read(uint8_t ch)
{
	ch&=0x07;
	ADMUX&=0xF8;
	ADMUX|=ch;
	ADCSRA|=(1<<ADSC);
	while(!(ADCSRA&(1<<ADIF)));
	ADCSRA|=(1<<ADIF);
	return (ADC);
}

Where you are then changing the MUX, triggering a reading and then waiting for ADIF to signal completion. Surely your vector (assuming "free running") just wants to be:

ISR(ADC_vect)
{	
		OCR2B = 255 - (ADCH / 4);						   //adjust duty cycle	
}

If you really do want to use more than one channel then do NOT use interrupts/free running because you hit the problem of the reading lagging the mux selection.

 

 

Thank you for your inputs.

Using ISR is the easy way to change the duty cycle using the potentiometer so I think I have to continue with the interrupt.  What do you suggest then?  Is there a work around to avoid the problem you mentioned?

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

ryan2019 wrote:
What do you suggest then?  Is there a work around to avoid the problem you mentioned?
I suggest you pick one of either synchronous or asynchronous and use ONLY that. Do not mix the two techniques.

 

Assuming you are only using one ADC channel and there is going to be other things going on while ADC readings are being made then asynchronous (interrupts) may well be the best approach.

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

clawson wrote:

ryan2019 wrote:
What do you suggest then?  Is there a work around to avoid the problem you mentioned?
I suggest you pick one of either synchronous or asynchronous and use ONLY that. Do not mix the two techniques.

 

Assuming you are only using one ADC channel and there is going to be other things going on while ADC readings are being made then asynchronous (interrupts) may well be the best approach.

 

Hello clawson.  Good day Sir. I managed to use the interrupt scheme to control 2 ADC ports.  I saw some of your old post so I followed them.  These ports will be activated (non-dimming scheme only Hi or Lo output) depending on the ADC levels sensed at adc inputs 1 and 2 through potentiometers 1 and 2.  These are both working fine.  However I have another PWM(OCR2B) which is controlled by another potentiometer 3 at adc input 3.  I wanted to implement a dimming feature by changing the duty cycle of OCR2B.  At the moment I do not know how to correctly assign adc port 3 to control OCR2B.  Now what happens is that this OCR2B PWM is not working correctly.  Every time I change either of potentiometers 1 and 2 this OCR2B also changes.  What needs to be done so that only potentiometer 3 controls this PWM and not influenced by potentiometers 1 and 2?

#include <avr/io.h>
#define F_CPU 16000000
#include <util/delay.h>
#include <avr/interrupt.h>


void ADC_init(void)
{
	ADMUX|=(1<<REFS0);                                                   //use VCC as reference
	ADCSRA|=(1<<ADEN)|(1<<ADIE)|(1<<ADPS2)|(1<<ADPS1)|(1<<ADPS0);        //use prescaler 128  = 16000000/128 = 125k
	sei();
}

void PWM2B_init(void)
{
	TCCR2A|=(1<<COM2A1)|(1<<COM2A0)|(1<<COM2B1)|(1<<COM2B0)|(1<<WGM20);		//Set OC2A/OC2B on Compare Match when up-counting. Clear OC2A/OC2B on
	TCCR2B|=(1<<WGM22)|(1<<CS21);				                        //compare Match when down-counting.  Inverted.
	OCR2A=255;						                        //Set to PWM, Phase Correct PWM.  Top is OCR2A.  Set prescale to 8	
}


int main(void)
{
	DDRB=0x17;                                                  //set PB0, PB1, PB2 and PB4 as output
	DDRD=0x08;                                                  //set PD3 as output
	
	ADC_init();
	PWM2B_init();
	
	ADCSRA|=(1<<ADSC);						
	
	while(1)
	{				
	}
}

ISR(ADC_vect)
{
    {
		OCR2B=255-(ADC/4);                                  //this line is not working!
    }                                                               //what is the right code to control it using ADC input 3?
    
	uint16_t ADCvalue=ADC;
	
		if(ADMUX&(1<<MUX0))
		{
			if(ADCvalue>=613)                                       //set threshold to 3V
			{
				PORTB|=(1<<1);                                  //enable PortB PB1
			}
			else if((ADCvalue>204)&&(ADCvalue<613))                 //set trigger between 1V and 3V
			{
				PORTB|=(1<<2);                                  //enable PortB PB2
			}
			else
			{
				PORTB&=~(1<<1);                                 //disable PortB PB1
				PORTB&=~(1<<2);			                //disable PortB PB2
			}
		}
		
		else if (ADMUX&(1<<MUX1))
		{
			if(ADCvalue>=818)                                       //set threshold to 4V
			{
				PORTB|=(1<<0);                                  //enable PortB PB0
			}
			else if((ADCvalue>512)&&(ADCvalue<818))                 //set trigger between 2.5V and 4V
			{
				PORTB|=(1<<4);                                  //enable PortB PB4
			}
			else
			{
				PORTB&=~(1<<0);                                  //disable PortB PB0
				PORTB&=~(1<<4);			                 //disable PortB PB4
			}
		}
		
		if (ADMUX&(1<<MUX0))				// SETUP NEXT CHANNEL
		{
			                                        // channel 1 current so switch to 2
			ADMUX &= 0xF8;			        // clear current channel
			ADMUX |= (1<<MUX1);
		}
		else
		{
			                                        // channel 2 current so switch to 1
			ADMUX &= 0xF8;			        // clear current channel
			ADMUX |= (1<<MUX0);
		}
		
		ADCSRA |= (1<<ADSC);
	
}




 

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

You need to check ADMUX and only set OCR2B when it's set to channel 3. Also, your code to setup the next channel just alternates between channels 1 and 2; it need to be extended to cycle thru all three.

 

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

balisong42 wrote:

You need to check ADMUX and only set OCR2B when it's set to channel 3. Also, your code to setup the next channel just alternates between channels 1 and 2; it need to be extended to cycle thru all three.

 

 

Hello balisong42.  Thank you for the suggestion.  I modified according to your comment.  Still not working and the rest of the ADCs are affected now they are not working.  See my code what is wrong.

if(ADMUX & (1<<MUX2))
	{
		OCR2B=255-(ADCvalue/4);                     //this line is not working!
	}

	if (ADMUX & (1<<MUX0))							// SETUP NEXT CHANNEL
	{
		// channel 1 current so switch to 2
		ADMUX &= 0xF8;							// clear current channel
		ADMUX |= (1<<MUX1);
	}
	else if (ADMUX & (1<<MUX1))
	{
		// channel 2 current so switch to 3
		ADMUX &= 0xF8;							// clear current channel
		ADMUX |= (1<<MUX2);
	}
	else if (ADMUX & (1<<MUX2))
	{
		// channel 3 current so switch to 1
		ADMUX &= 0xF8;							// clear current channel
		ADMUX |= (1<<MUX0);
	}

	ADCSRA |= (1<<ADSC);

 

Last Edited: Mon. Dec 16, 2019 - 09:53 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0
#include <avr/io.h>
#define F_CPU 16000000
#include <util/delay.h>
#include <avr/interrupt.h>


void ADC_init(void)
{
    
    //why |=? just set the bits to what you want
	ADMUX = (1<<REFS0);                                                   //use VCC as reference
	ADCSRA = (1<<ADEN)|(1<<ADIE)|(1<<ADPS2)|(1<<ADPS1)|(1<<ADPS0);        //use prescaler 128  = 16000000/128 = 125k
	sei();
}

void PWM2B_init(void)
{
	TCCR2A = (1<<COM2A1)|(1<<COM2A0)|(1<<COM2B1)|(1<<COM2B0)|(1<<WGM20);		//Set OC2A/OC2B on Compare Match when up-counting. Clear OC2A/OC2B on
	TCCR2B = (1<<WGM22)|(1<<CS21);				                        //compare Match when down-counting.  Inverted.
	OCR2A=255;						                        //Set to PWM, Phase Correct PWM.  Top is OCR2A.  Set prescale to 8	
}


int main(void)
{
	DDRB=0x17;                                                  //set PB0, PB1, PB2 and PB4 as output
	DDRD=0x08;                                                  //set PD3 as output
	
	ADC_init();
	PWM2B_init();
	
	ADCSRA |= (1<<ADSC);						
	
	while(1)
	{				
	}
}

ISR(ADC_vect)
{
    uint16_t ADCvalue = ADC;
    
    switch(ADMUX & 0x07)
    {
        case 0:
            break;
            
        case 1:
        	if(ADCvalue >= 613)                                       //set threshold to 3V
			{
				PORTB |= (1<<1);                                  //enable PortB PB1
			}
			else if( (ADCvalue>204) && (ADCvalue<613) )                 //set trigger between 1V and 3V
			{
				PORTB |= (1<<2);                                  //enable PortB PB2
			}
			else
			{
				PORTB &= ~(1<<1);                                 //disable PortB PB1
				PORTB &= ~(1<<2);			                //disable PortB PB2
			}
            break;
            
        case 2:
            if(ADCvalue>=818)                                       //set threshold to 4V
			{
				PORTB|=(1<<0);                                  //enable PortB PB0
			}
			else if( (ADCvalue>512) && (ADCvalue<818) )                 //set trigger between 2.5V and 4V
			{
				PORTB |= (1<<4);                                  //enable PortB PB4
			}
			else
			{
				PORTB &= ~(1<<0);                                  //disable PortB PB0
				PORTB &= ~(1<<4);			                 //disable PortB PB4
			}
            break;
            
        case 3:
            OCR2B = 255 - (ADC/4);
            break;
    }   
       
    ADMUX = ( (ADMUX & 0x07) + 1) & 0x03; //select next channel. but only 0..3
    ADMUX |= (1<<REFS0);
	ADCSRA |= (1<<ADSC);
	
}


maybe something like this?

 

Note - I haven't checked it or tested it - it's straight out of the oven.

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

Kartman wrote:

#include <avr/io.h>
#define F_CPU 16000000
#include <util/delay.h>
#include <avr/interrupt.h>


void ADC_init(void)
{
    
    //why |=? just set the bits to what you want
	ADMUX = (1<<REFS0);                                                   //use VCC as reference
	ADCSRA = (1<<ADEN)|(1<<ADIE)|(1<<ADPS2)|(1<<ADPS1)|(1<<ADPS0);        //use prescaler 128  = 16000000/128 = 125k
	sei();
}

void PWM2B_init(void)
{
	TCCR2A = (1<<COM2A1)|(1<<COM2A0)|(1<<COM2B1)|(1<<COM2B0)|(1<<WGM20);		//Set OC2A/OC2B on Compare Match when up-counting. Clear OC2A/OC2B on
	TCCR2B = (1<<WGM22)|(1<<CS21);				                        //compare Match when down-counting.  Inverted.
	OCR2A=255;						                        //Set to PWM, Phase Correct PWM.  Top is OCR2A.  Set prescale to 8	
}


int main(void)
{
	DDRB=0x17;                                                  //set PB0, PB1, PB2 and PB4 as output
	DDRD=0x08;                                                  //set PD3 as output
	
	ADC_init();
	PWM2B_init();
	
	ADCSRA |= (1<<ADSC);						
	
	while(1)
	{				
	}
}

ISR(ADC_vect)
{
    uint16_t ADCvalue = ADC;
    
    switch(ADMUX & 0x07)
    {
        case 0:
            break;
            
        case 1:
        	if(ADCvalue >= 613)                                       //set threshold to 3V
			{
				PORTB |= (1<<1);                                  //enable PortB PB1
			}
			else if( (ADCvalue>204) && (ADCvalue<613) )                 //set trigger between 1V and 3V
			{
				PORTB |= (1<<2);                                  //enable PortB PB2
			}
			else
			{
				PORTB &= ~(1<<1);                                 //disable PortB PB1
				PORTB &= ~(1<<2);			                //disable PortB PB2
			}
            break;
            
        case 2:
            if(ADCvalue>=818)                                       //set threshold to 4V
			{
				PORTB|=(1<<0);                                  //enable PortB PB0
			}
			else if( (ADCvalue>512) && (ADCvalue<818) )                 //set trigger between 2.5V and 4V
			{
				PORTB |= (1<<4);                                  //enable PortB PB4
			}
			else
			{
				PORTB &= ~(1<<0);                                  //disable PortB PB0
				PORTB &= ~(1<<4);			                 //disable PortB PB4
			}
            break;
            
        case 3:
            OCR2B = 255 - (ADC/4);
            break;
    }   
       
    ADMUX = ( (ADMUX & 0x07) + 1) & 0x03; //select next channel. but only 0..3
    ADMUX |= (1<<REFS0);
	ADCSRA |= (1<<ADSC);
	
}


maybe something like this?

 

Note - I haven't checked it or tested it - it's straight out of the oven.

 

Hello Kartman you made a magic now it's working :)

 

I just have some questions on your code.

 

1. switch(ADMUX & 0x07) { case 0: break;       Why do you need to put a break on case 0?  Why switch ADMUX&0x07? 

 

2. ADMUX = ( (ADMUX & 0x07) + 1)                Why is there +1 on this code?  What does it do?  And why did you repeat this at the last part of the code?  Why not ADMUX |= ( (ADMUX & 0x07) + 1)

 

3. ADMUX |= (1<<REFS0);                              Why did you  repeat this line at the last part of the code when initially it was already declared?

 

Thank you for the code.  Appreciate your time Sir!

 

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

1) surely that means "I recognise that case 0 is a possibility but don't want to actively do anything in this case" ?

 

2) the comment tells you why:

 //select next channel.

3) Look at the previous line. It is "=" which means "anything already in ADMUX is lost" so this is just putting back the REFs bits.

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

clawson wrote:

1) surely that means "I recognise that case 0 is a possibility but don't want to actively do anything in this case" ?

 

2) the comment tells you why:

 //select next channel.

3) Look at the previous line. It is "=" which means "anything already in ADMUX is lost" so this is just putting back the REFs bits.

Thank you sir :)

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1
ADMUX = ( (ADMUX & 0x07) + 1) & 0x03; //select next channel. but only 0..3

 

That line is a bit clumsy - how would you rewrite it? Mind you, the compiler has probably figured out what I actually wanted. 

 

I'd question why you would use interrupts for the ADC in the first place - it really isn't justified. Maybe you wanted to try them? If this is the case, I'd suggest you get your C skills up to scratch before you get into more complex stuff.

 

 

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

Kartman wrote:

ADMUX = ( (ADMUX & 0x07) + 1) & 0x03; //select next channel. but only 0..3

 

That line is a bit clumsy - how would you rewrite it? Mind you, the compiler has probably figured out what I actually wanted. 

 

I'd question why you would use interrupts for the ADC in the first place - it really isn't justified. Maybe you wanted to try them? If this is the case, I'd suggest you get your C skills up to scratch before you get into more complex stuff.

 

 

Hello kartman,

Thank you always for giving me good hints and suggestions and even the code itself.

I can also write it like:
ADMUX=(ADMUX&(0x07)+1);
ADMUX&=0×03;
This is really clever way how you did it and it took me a while to realize what you wanted to do.
The reason I used interrupts is because I wanted to control the duty cycle of OCR2B and I could not think of other way how to do it besides interrupt. Is there another way of doing it if not the interrupt scheme? Also why do you say it is not justified?

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

The solution was to only use one &. Sit down with a pencil and paper.

Interrupts are a doubled edged sword - used well they solve a class of problems, used poorly, they can cause a lot of problems. I would simply write a function that takes a channel number, sets the admux to the channel, start the conversion, wait for completion and return the result. Then just call this function and store the result into the pwm. Rinse and repeat. No interrupts and no screwing around.
How fast do you really need to update the pwm value? 10,000 times a second? Probably not.

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

A normal ADC conversion take about 100us, so what else will your cpu be doing during this short time? 

Some times its best just to spin wait, unless you have something that needs to be done during this wait time.

 

Jim

 

 

(Possum Lodge oath) Quando omni flunkus, moritati.

"I thought growing old would take longer"

 

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

Kartman wrote:
The solution was to only use one &. Sit down with a pencil and paper.

Interrupts are a doubled edged sword - used well they solve a class of problems, used poorly, they can cause a lot of problems. I would simply write a function that takes a channel number, sets the admux to the channel, start the conversion, wait for completion and return the result. Then just call this function and store the result into the pwm. Rinse and repeat. No interrupts and no screwing around.
How fast do you really need to update the pwm value? 10,000 times a second? Probably not.

Thank you for the hints Sir :)