Read ADC value atomically

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

Good day!

I'd like to ask for advice on reading ADC value. My environment:
- ATmega8A @ 8Mhz internal osc
- WinAVR 20100110
- 10k pot on ADC0 pin

Here is and init code:

	ADCSRA |= (1 << ADPS2) | (1 << ADPS1); // Set ADC prescalar to 64 - 125KHz sample rate @ 8MHz
	ADMUX |= (1 << REFS0); // Set ADC reference to AVcc
	// AMDUX = ... 	// No changes to select channel, we use channel 0

	ADCSRA |= (1 << ADFR);  // Set ADC to Free-Running Mode
	ADCSRA |= (1 << ADEN);  // Enable ADC
	ADCSRA |= (1 << ADSC);  // Start A2D Conversions 
	ADCSRA |= (1 << ADIE);  // Enable interrupt
	
volatile static uint16_t adc;

ISR(ADC_vect)
{
	uint8_t temp = ADCL;
	adc = ADCH;
	adc = (adc << 8) + temp;
}

Questions:

1. Can I safely replace my ISR code with this (can I assume arv-libc will read ADCL and ADCH in the right order as described in datasheet):

ISR(ADC_vect)
{
	adc = ADC;
}

2. Should I disable interrupts while reading my adc variable? Like this:

uint16_t get_adc(void)
{
	uint8_t oldSREG = SREG;

	cli();
	uint16_t temp = adc;

	SREG = oldSREG;
	return temp;
}

...or I can access my uint16_t variable directly?

Thanks in advance!

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

1) Yes, in your 1st code you're doin' the compiler's job. It knows how to access an N byte-sized variable without all that hoop jumpin'. No need to declare that var like that, volatile ought to be enough.

2) That'll work, and you could access directly if there's no access path that could corrupt it.

1) Studio 4.18 build 716 (SP3)
2) WinAvr 20100110
3) PN, all on Doze XP... For Now
A) Avr Dragon ver. 1
B) Avr MKII ISP, 2009 model
C) MKII JTAGICE ver. 1

Last Edited: Thu. Jan 20, 2011 - 08:33 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Thanks for the response!

Why I can access my 16bit variable which is updated in ISR without disabling interrupts?

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

Quote:
Why I can access my 16bit variable which is updated in ISR without disabling interrupts?

I'm sorry i was thinking about direct accessing of the var in the ISR not get_adc(), but generally:
You only have to disable if your accessed var is shared by multiple functions AND if one or more of those functions can be called from within the adc_ISR, as an interrupt.

For the get_adc(), it would need the disable as the adc value may get corrupted if the ISR() fires within get_adc().

1) Studio 4.18 build 716 (SP3)
2) WinAvr 20100110
3) PN, all on Doze XP... For Now
A) Avr Dragon ver. 1
B) Avr MKII ISP, 2009 model
C) MKII JTAGICE ver. 1

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

Just to make it clear to me -- if I need to access my variable

volatile static uint16_t adc; 

Inside my main(), should I use function get_adc() and not access it directly, because the value can be corrupted if reading it inside main() will be interrupted by interrupt?

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

Yes, you need to use get_adc().

1) Studio 4.18 build 716 (SP3)
2) WinAvr 20100110
3) PN, all on Doze XP... For Now
A) Avr Dragon ver. 1
B) Avr MKII ISP, 2009 model
C) MKII JTAGICE ver. 1

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

Quote:
Why I can access my 16bit variable which is updated in ISR without disabling interrupts?
Interrupts are automatically disabled inside an ISR.

Regards,
Steve A.

The Board helps those that help themselves.

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

Thanks for the responses, it seems my question was not clear. I was asking about accessing value not inside ISR, but in main loop, here is a code.

volatile static uint16_t adc;

ISR(ADC_vect)
{
   adc = ADC;
} 

uint16_t get_adc(void)
{
   uint8_t oldSREG = SREG;

   cli();
   uint16_t temp = adc;

   SREG = oldSREG;
   return temp;
} 

int main(void)
{
	// stdout is connected to UART, some UART code skipped...

	ADCSRA |= (1 << ADPS2) | (1 << ADPS1); // Set ADC prescalar to 64 - 125KHz sample rate @ 8MHz
	ADMUX |= (1 << REFS0); // Set ADC reference to AVcc
	// AMDUX = ...    // No changes to select channel, we use channel 0

	ADCSRA |= (1 << ADFR);  // Set ADC to Free-Running Mode
	ADCSRA |= (1 << ADEN);  // Enable ADC
	ADCSRA |= (1 << ADSC);  // Start A2D Conversions
	ADCSRA |= (1 << ADIE);  // Enable interrupt 

	// Enable interrupts
	sei();
    	 
	while(1)
	{
		// which is correct?
	
		printf(PRIu16, adc); 
	
		// or 
		
		printf(PRIu16, get_adc());
	
	}  
}   

Both variants works in my environment, but I assume that is just because there are no other interrupts.

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

Generally when you have a question like this:

Quote:
can I assume arv-libc will read ADCL and ADCH in the right order as described in datasheet)

you can verify what the compiler did by opening the *.LSS file and find that code section and see what assembly gets generated.
Quote:

// which is correct?
NEITHER,if using other ISR()s, for the same reason that you need to use get_adc(). I've never used printf in a MCU, but I assume it DOESN'T handle interrupt system. If that's true, atomic.h should have macros to handle printf atomically. BOTH work NOW because the time it takes to printf is < time between ISR getting called.

http://www.nongnu.org/avr-libc/u...

printf(PRIu16, get_adc());

IS the most correct, generally, since you need to access adc atomically. Seems like you really don't need get_adc() for this app. 'cause it "takes so long" for the ISR() to get called and you MIGHT be able to get away w/ direct access. Also your direct access works now with a minimally coded app., what happens if you add more code, but no more ISR()s ( which may be coded to interact with adc value )?

If you put your adc_code into its own adc.c file and use its functions in an app. with the right ( wrong?) mix of ISR()s and for SOME reason your adc_values aren't coming out right... and you've forgotten that you're using direct access method... and time's marching on...

1) Studio 4.18 build 716 (SP3)
2) WinAvr 20100110
3) PN, all on Doze XP... For Now
A) Avr Dragon ver. 1
B) Avr MKII ISP, 2009 model
C) MKII JTAGICE ver. 1

Last Edited: Fri. Jan 21, 2011 - 09:05 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Quote:

you can verify what the compiler did by opening the *.LSS file and find that code section and see what assembly gets generated.

Thanks, I've already checked this after you response :)

Quote:

NEITHER,if using other ISR()s, for the same reason that you need to use get_adc(). I've never used printf in a MCU, but I assume it DOESN'T handle interrupt system.

Printing something to UART is the only debug way for me now :(. The UART lib that I use uses circular buffer and interrupts to transmit data via UART. Do you think that if interrupt occur between printf call it will break anything? The stack is stored, so it just continues execution. Is this right?

Quote:

Also your direct access works now with a minimally coded app., what happens if you add more code, but no more ISR()s ?

I will definitely use "atomic" access using get_adc() function, I just want to make sure that I'm not overengineered it :)

Edit: Thanks for atomic.h it seems that it does the same as my manual way of storing SREG.

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

For the sake of atomicity this is a great example, but if this is all your code does, it is a very complex way of doing things.

The ADC runs free converting all the time as often as it can, and you are spewing out that value as often as you can.

Why do it like this when you can just wait for conversion to complete, read the value for printing, start new conversion and print out the read value?

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

Jepael, this is a part of bigger application, I just isolated ADC code for this question. And I'd like to do it in more general\right way from the beginning...

Thanks!

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

atomic.h also makes it easy to make any function atomic w/o having to dig into the function.

Quote:
Do you think that if interrupt occur between printf call it will break anything?
NO, you're good all the way home, as adc has been transferred into temp. This is all making me think more: I said NEITHER and i was wrong ( 'cause of what i just wrote ). The get_adc() is the CORRECT one.

Quote:
Is this right?

Yes,printf() will be ok 'cause it will continue correctly after an ISR(). The DATA it's handling is the only issue that has to be looked at.

1) Studio 4.18 build 716 (SP3)
2) WinAvr 20100110
3) PN, all on Doze XP... For Now
A) Avr Dragon ver. 1
B) Avr MKII ISP, 2009 model
C) MKII JTAGICE ver. 1

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

Thanks a lot!