ADC issue - holding

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

Hi,

I am having some issues with a program using the ADC - I would greatly appreciate comments by anyone who has encountered something similar before!

The system is an AtMega 128 at 16Mhz with the UART0, ADC and Timer1 running. The timer 1 interrupt triggers every 5000 cycles (every 132.5uS) and triggers the ADC (3200sps). Within the timer interrupt I am incrementing a counter and checking how many ticks have passed since the last counter reset depending on which I change the "state" of the code (a virtual state machine). The ADC interrupt checks the state and writes to a different location depending on the state. Outside the interrupts the main is stuck in an infintie loop where according to the state some relays are switched on and others are switched off. The UART is running with interrupt-driven code that takes data from a circular buffer which is filled as appropriate by the main.

The strange behaviour is that the ADC always keeps giving the same value, namely the value of the first reading it takes. For example, if the first ADC reading is 410 (and the first reading corresponds correctly to the voltage input) all further readings are 410, even when I change the voltage on the input pin. I have observed this voltage both using JTAG debug and by dumping data out through the UART. My first suspicion was on damaged ADC hardware (for a moment I had driven the input above the supply voltage) but I swapped the processor with a known good one and the issue remained. Also using a hex file from an old project the ADC worked normally and updates the value.

ISR(ADC_vect)
	//
	{
		cli();
		if (machine==state1)
		{
			accumulator1 += ((ADCH<<8)+ADCL);
		}

		if ((machine==state2)||(machine==state3))
		{
			//Subtract old "end of buffer"
			accumulator2 -= temp_array[temp_array_pointer];
			//Get ADC value and write in old "end" / new "start"
			muqqu = ((ADCH<<8)+ADCL); //"muqqu" variable exists only for watch
			temp_array[temp_array_pointer] = muqqu;
			//Add new "start"
			accumulator2 += temp_array[temp_array_pointer];

			//Increment pointer and if the top is reached wrap around
			temp_array_pointer++;
			if (temp_array_pointer>=64)
			{
				temp_array_pointer = 0;
			}
			
		}
		sei();
	}

Any ideas?
N

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

1) You never want to do CLI and SEI in the ISR, unless you really really know how to do nested interrupts well.

2) "When ADCL is read, the ADC Data Register is not updated until ADCH is read. Consequently, if the result is left adjusted and no more than 8-bit precision is required, it is sufficient to read ADCH. Otherwise, ADCL must be read first, then ADCH." Many compilers will have a composite "word" for you to read.

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

Thanks for the comments!

Regarding 1) the behaviour is the same even without CLI and SEI - in fact I added them just on a hunch in the last builds - now removed.

Regarding 2) am I wrong in assuming that if I read both ADCL and ADCH before the next ADC interrupt the data register will still be updated? I changed to the following and nothing behaves differently!

muqqu=ADCL;
ISO_accumulator += ((ADCH<<8)+muqqu);
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0
muqqu=ADCL;
ISO_accumulator += ((ADCH<<8)+muqqu);

You're still doing this the hard way. Try:

ISO_accumulator += ADC;

As for the ADC always returning the same value, are you sure that you are restarting the ADC conversion process? We have not seen your ISR for the Timer1, nor the set up for the timer (or the ADC for that matter).

Stu

Engineering seems to boil down to: Cheap. Fast. Good. Choose two. Sometimes choose only one.

Newbie? Be sure to read the thread Newbie? Start here!

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

I guess that we'll need to see your trigger code then.

NB: I write nearly all my AVR apps to do continuous ADC conversions, and start the next conversion in the ISR after reading the "raw" value. In my typical industrial app I don't use that many conversions--typically, I then grab the "raw" values each 10ms and add to my accumulator for each channel.

Triggering as you are doing my be needed for a specific sampling frequency, especially at the high conversion rate if you need that particular rate.

I don't think that AVR model has auto-trigger--does it?

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

It's a long shot, but could the compiled code be reading the ADCH and ADCL out of order? ADCL must be read first, or there will be strange results such as multiple returns of the same value for different voltage inputs.
If this were assembly code, the read order would be the most likely culprit. But with C, it seems unlikely.

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

Here's the missing info... excuse my brevity in the first post.

ISR(TIMER1_COMPA_vect)
	{
	//Should use timer in WGM4 mode. will give an interrupt every 5000 clock
	//cycles thus amounting to 64 ticks per 20mS.
	
	start_adc();
	//Increment global counting variables
	ticks++;
.
.
.
.

The start_adc(); command is a pre-defined macro that sets ADSC and clears a flag.

I am using timer 1 at 64 ticks per 20ms (3200 ticks/s) so that I can average the readings over one mains cycle (50Hz - 20ms) with simple maths (accumulate and then shift at the end to divide by 64) Timer and ADC registers are as follows.

TCCR1A = 0b00000000 = no output compare on any physical pin, WGM4 (clear timer on compare using OCR1A)
TCCR1B = 0b00001010 = no ICP, not used, WGM4 (clear timer on compare using OCR1A), prescale by 8
TCCR1C = 0b00000000 = no forcing of output compare, not used
TCNT1H = TCNT1L = not set
OCR1AH = 
OCR1AL =  together = 624 since the f_comp = f_clk/(prescale*(1+OCR1A)) = 16000000/(8(1+624)) = 3200Hz which is the required rate.
ADMUX = 0b00000000 = external AREF, data right adjusted, channel 0 single ended
ADCSRA = 0b10001111 = ADC enable, ADC not started, single shot mode, do not directly trigger interrupt, enable interrupt, prescale by 128

The prescaler of 128 should give me 200us for the longest conversion (25 ADC clock cycles) leaving 112.5us until the next timer interrupt.

I'm starting to think that the timer is interrupting before the timer ISR is complete. I'll try to get hold of a scope tomorrow morning and toggle a pin in each interrupt to get a timing diagram.

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

Quote:

The start_adc(); command is a pre-defined macro that sets ADSC and clears a flag.


So why not show it? And the timer setup?

We are getting pretty close to: "Show the smallest complete program that exhibits the behaviour."

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

I'll copy the project and remove all the code not related to the timer/adc systems - then I'll give it a try on the dev board to see what happens - unfortunately no access to the board at the moment.

Timer setup is as per the following macro

T1_init(0b00000000, 0b00001010, 0b00000000);
#define		T1_init(myTCCR1A, myTCCR1B, myTCCR1C)						\
	do{/*Write timer/counter 1 control registers*/							\
		TCCR1A = myTCCR1A;													\
		TCCR1B = myTCCR1B;													\
		TCCR1C = myTCCR1C;													\
		TCNT1 = 0;															\
	}while(0)

ADC setup is as follows

adc_init(0b10001111, 0b00000000);
#define adc_init(myADCSRA,myADMUX)										\
	do{																		\
		ADMUX = myADMUX;													\
		ADCSRA = myADCSRA;													\
		adc_idle = 255;														\
	}while(0)

and the ADC start macro is

#define start_adc()			do{ADCSRA|=(1<<ADSC);\
									adc_idle=0;}while(0)

I'm getting this uneasy feeling that when I strip away all the other code tomorrow morning I'll find one of those dumb mistakes that make me :evil:

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

So finally I'm getting to the bottom of this! I started by removing everything from the code except for the timer and ADC initialization and ISRs. Everything worked fine. I've been slowly adding stuff and have reached the point where the only part of the original code that's not being used is the switch-case statement that handles the state machine in the timer ISR. The ADC updates correctly! :)

Is there any other neat way to implement a state machine change in an ISR that doesn't make the interrupt take too long?

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

Quote:

Is there any other neat way to implement a state machine change in an ISR that doesn't make the interrupt take too long?

Are your case statements consecutive or disparate? If consecutive then the compiler probably does an ICALL via an indexed table so all cases take the same (short) time. Otherwise it may end up hopping down a load of if/then/elseif tests until it hits the right condition

But if this is a state machine presumably it switches on an enum{} state variable and that is using consecutive values?

You may want to look at the C/Asm listing to see what code structure has been generated by the switch()

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

To be honest I also found a case of CLI/SEI I hadn't removed in the Timer ISR going against theusch's recommendation - I corrected that too now and everything is working.

In fact I am using an enum as a state variable, and yes the values are consecutive. I'm not fluent in Asm but I looked at the listing and these are snippets from it with -O0 and -O2. I'm not sure of the details but it looks like a sequence of "if....else if....". Would there be a penalty if one of the cases of the state variable was not used (in the sense, would a missing value prevent the compiler from using a table)?

	switch (test_machine)
     c96:	80 91 36 02 	lds	r24, 0x0236
     c9a:	28 2f       	mov	r18, r24
     c9c:	30 e0       	ldi	r19, 0x00	; 0
     c9e:	3c 83       	std	Y+4, r19	; 0x04
     ca0:	2b 83       	std	Y+3, r18	; 0x03
     ca2:	8b 81       	ldd	r24, Y+3	; 0x03
     ca4:	9c 81       	ldd	r25, Y+4	; 0x04
     ca6:	84 30       	cpi	r24, 0x04	; 4
     ca8:	91 05       	cpc	r25, r1
     caa:	09 f4       	brne	.+2      	; 0xcae <__vector_12+0x8c>
     cac:	a7 c0       	rjmp	.+334    	; 0xdfc <__vector_12+0x1da>
     cae:	2b 81       	ldd	r18, Y+3	; 0x03
     cb0:	3c 81       	ldd	r19, Y+4	; 0x04
     cb2:	25 30       	cpi	r18, 0x05	; 5
     cb4:	31 05       	cpc	r19, r1
     cb6:	ec f4       	brge	.+58     	; 0xcf2 <__vector_12+0xd0>
switch (test_machine)
 4bc:	80 91 36 02 	lds	r24, 0x0236
 4c0:	84 30       	cpi	r24, 0x04	; 4
 4c2:	09 f4       	brne	.+2      	; 0x4c6 <__vector_12+0x42>
 4c4:	a6 c0       	rjmp	.+332    	; 0x612 <__vector_12+0x18e>
 4c6:	85 30       	cpi	r24, 0x05	; 5
 4c8:	d0 f0       	brcs	.+52     	; 0x4fe <__vector_12+0x7a>

Thanks for all the help!
N

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

How many cases are there? I'm not sure when the code generator makes the decision that it would be more efficient to use an indexed ICALL table rather than a whole bunch of if...else's but, as you say, that code seems to be of the latter type.

Last Edited: Tue. Oct 13, 2009 - 11:27 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I have nine cases total.
N