ATMEGA-328p-Unable to successfully switch between ADC channels and store reading

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

Hello,

 

Any guidance on the problem below would be appreciated. I'm looking for guidance (i.e. where have I gone wrong), rather than what the fix would be, since this is for school.

 

Anywho, the project involves the ATMEGA328p and the Pololu QTR8-A Reflectance Sensor. The goal is to have all 8 IR modules on the Pololu board attached to the 8 ADC channels; read and store the value from channel 0, move to channel 1, read, etc. through channel 8. Then each of the 8 values is combined to produce a single value that is output through the UART. The combination formula for the assignment is a bit of a mess, but you can see it in the function calc_dec().

 

I believe that where I am going wrong is in the function analog(). This is for the following reasons:

  • The UART does output values; but the majority of time the value is 00000 when it should be high (it is a 5 byte transmission), and the values are inconsistent even if positioning over the sensor has not changed (and not slightly different, more like 17382 followed by 08340, and then a 00000 for good measure)
  • I have uploaded a different program with the same wiring, and same UART code, where I read only one channel and do not switch. I get a UART output between 0-243 (it seems to not like 255, and this program is transmits 3 bytes). I have changed this single channel program multiple times, to test the different sensors and channels. It works.

 

I could be completely off base though, and the error could be elsewhere.

 

Some additional details in case they are important:

  • the Pololu board outputs a high value when nothing is near the sensors; it drives low (or lower) as an item gets closer to the IR being read (this is why each value stored in the array is =255-ADCH)
  • ADC is in free-running mode
  • writing the program in Notepad ++; compiling using the make command in cmd; uploading with Xloader, and getting the UART output via Tera Term

 

Thanks in advance for any guidance.

 

#include <avr/io.h>
#define F_CPU 16000000UL
#define BAUD 9600
#include <util/delay.h>

void initUART(unsigned int baud); //sets up UART
void initADC(void); //sets up ADC
void transmitByte (int  []);//sends array to UART
void transmitNewLine (void);//sends newline and carriage return to UART
void analog (void); //goes through ADC channels and assigns each channels value to sensor_array location
void printDec (unsigned int data);//converts ADC_value so that each digit is in an array, so that it can go out UART
int ADC_value; //value of all sensors
void delay_ms (uint16_t ms);
int dec_val[5]; //array that will be output through UART
int sensor_array[8]; //each array location holds a value from one of the 8 ADC channels
void calc_dec (void);//calculates ADC_value

int main (void){
	DIDR0=1; //make ADC7 pin analog input rather than digital
	initUART(BAUD);
	initADC();
	while(1){
		analog();
		calc_dec();
		printDec(ADC_value);
		transmitByte ( dec_val);
		transmitNewLine();
		delay_ms(2000);
	}
		
	return(0);
}

void initADC( ) {
	//enable ADC
    ADCSRA |= (1 << ADEN);
    
	 // Vref connect internally to AVcc
    //ADLAR is set to 1, so value is left adjusted (8 bit precion, can read ADCH only)
    ADMUX |= (1 << REFS0) | (1 << ADLAR);
   
	//Set prescale to 8 (16000000/128 = 125Khz)
    ADCSRA |= (1 << ADPS2) | (1 << ADPS1) | (1 << ADPS0);
    

}

void initUART(unsigned int baud) {
	//Normal mode UBRR formula
   unsigned int ubrr = F_CPU/16/BAUD-1;
   
	// shift MSB and store in UBRR0H
   UBRR0H = (unsigned char) (ubrr >> 8);
   
// store LSB in UBRR0L
   UBRR0L = (unsigned char) ubrr;
   
   // Enable transmitter/receiver
   UCSR0B = (1 << RXEN0) | (1 << TXEN0);

   //8-Bit Characters, 1 Stop bits, No parity
   UCSR0C = (1 <<UCSZ00) | (1 <<UCSZ01);

   }
 
void analog (void) {
ADCSRA |= (1 << ADEN);
 ADMUX |= (0 << MUX3) | (0 << MUX2 )| (0 << MUX1) | (0 << MUX0);//	channel 0
 ADCSRA |= (1 << ADSC); //Start conversion process
 while ( ADCSRA & (1 << ADSC ) );
 sensor_array[0] = 255-ADCH;
 ADCSRA |= (0 << ADEN);
 
 ADMUX |= (0 << MUX3) | (0 << MUX2) | (0 << MUX1) | (1 << MUX0);//	channel 1
 ADCSRA |= (1 << ADEN);
 ADCSRA |= (1 << ADSC); //Start conversion process
 while ( ADCSRA & (1 << ADSC ) );
 sensor_array[1] = 255-ADCH;
 ADCSRA |= (0 << ADEN);
 
 ADMUX |= (0 << MUX3) | (0 << MUX2) | (1 << MUX1) | (0 << MUX0); //	channel 2
 ADCSRA |= (1 << ADEN);
 ADCSRA |= (1 << ADSC); //Start conversion process
 while ( ADCSRA & (1 << ADSC ) );
 sensor_array[2] = 255-ADCH;
 ADCSRA |= (0 << ADEN);
 
 ADMUX |= (0 << MUX3) | (0 << MUX2) | (1 << MUX1) | (1 << MUX0);//	channel 3
 ADCSRA |= (1 << ADEN);
 ADCSRA |= (1 << ADSC); //Start conversion process
 while ( ADCSRA & (1 << ADSC ) );
 sensor_array[3] = 255- ADCH;
 ADCSRA |= (0 << ADEN);
 
 ADMUX |= (0 << MUX3) | (1 << MUX2) | (0 << MUX1) | (0 << MUX0); //	channel 4
 ADCSRA |= (1 << ADEN);
 ADCSRA |= (1 << ADSC); //Start conversion process
 while ( ADCSRA & (1 << ADSC ) );
 sensor_array[4] = 255-ADCH;
 ADCSRA |= (0 << ADEN);
 
 ADMUX |= (0 << MUX3) | (1 << MUX2) | (0 << MUX1) | (1 << MUX0); //	channel 5
 ADCSRA |= (1 << ADEN);
 ADCSRA |= (1 << ADSC); //Start conversion process
 while ( ADCSRA & (1 << ADSC ) );
 sensor_array[5] = 255-ADCH;
 ADCSRA |= (0 << ADEN);
 
 ADMUX |= (0 << MUX3) | (1 << MUX2) | (1 << MUX1) | (0 << MUX0);   //	channel 6
 ADCSRA |= (1 << ADEN);
 ADCSRA |= (1 << ADSC); //Start conversion process
 while ( ADCSRA & (1 << ADSC ) );
 sensor_array[6] = 255-ADCH;
 ADCSRA |= (0 << ADEN);
 
 ADMUX |= (0 << MUX3) | (1 << MUX2) | (1 << MUX1) | (1 << MUX0);   //	channel 7
 ADCSRA |= (1 << ADEN);
 ADCSRA |= (1 << ADSC); //Start conversion process
 while ( ADCSRA & (1 << ADSC ) );
 sensor_array[7] = 255-ADCH;
 ADCSRA |= (0 << ADEN);
 }

 void calc_dec (void){
	 ADC_value= ((8000*sensor_array[7])+(7000*sensor_array[6])+(6000*sensor_array[5])+(5000*sensor_array[4])+(4000*sensor_array[3])+(3000*sensor_array[2] + (2000*sensor_array[1])+(1000*sensor_array[0]))/( sensor_array[7]+ sensor_array[6]+ sensor_array[5]+ sensor_array[4]+ sensor_array[3]+ sensor_array[2]+ sensor_array[1]+ sensor_array[0]));
	 
 }
 
void delay_ms (uint16_t ms) {
	uint16_t i;
	for (i = 0; i < ms; i++)
		_delay_ms(1);		
}
 
void printDec (unsigned int data){
	int i=0;
	int holder = data;
	dec_val[0]=0;
	dec_val[1]=0;
	dec_val[2]=0;
	dec_val[3]=0;
	dec_val[4]=0;


	while (i<5){
		
		if (holder>9){
			dec_val[i]= holder%10;
			holder = holder/10;
		}
		else if (holder >0 ){
			dec_val[i]=holder ;
			holder=holder - 10;
		}
		else {
			dec_val[i]=0;
		}
		i++;
		}
	}
 
void transmitByte (int dec_array []) {
   
   int i = 5;
  
   while (i !=0){
	   while ( !(UCSR0A & (1 << UDRE0)) );
	   // Wait for empty transmit buffer
		//Send dec_array[] to UART
				i--;
				UDR0 =(dec_array[i] +48);	
	}
	
	
} 

void transmitNewLine (void) {
   
   while ( !(UCSR0A & (1 << UDRE0)) );
   // Wait for empty transmit buffer
			
			UDR0 =13;	

   while ( !(UCSR0A & (1 << UDRE0)) );
   // Wait for empty transmit buffer
			
			UDR0 =10;	
} 

 

 

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

0 shifted n times is still 0. Anything ‘or’ed with 0 has no change. Simply write a function that takes a channel number and assign that to ADMUX. It will work a whole lot better and you won’t have to repeat the same code over and over.

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

Thanks for that tip. It has helped shorten the code substantially.

analog() now reads:

void analog (void) {
 while (current_channel <8){
	ADCSRA |= (1 << ADEN);
	ADMUX |= current_channel;
	ADCSRA |= (1 << ADSC); //Start conversion process
	while ( ADCSRA & (1 << ADSC ) );
		sensor_array[current_sensor_array_position] = 255-ADCH;
		ADCSRA |= (0 << ADEN);
		current_channel++;
		current_sensor_array_position++;
	}
	current_channel = 0;
	current_sensor_array_position=0;

	}

Not shown are the two new global variables created: current_channel and current_sensor_array.

 

Unfortunately, the values are still wonky.

 

I am toggling ADEN so that the channel does not switch mid conversion (reading suggests that will screw up the value).  Could that be messing it up?

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

There is no need to turn-off the ADC after every conversion.

You will have integer-overflow issues in calc_dec() because the intermediate results might not 'fit' into a 16-bit int.
ie., if sensor_array[7] is greater than 4 you will have problems because
(8000*sensor_array[7]) will either produce a negative result or truncate the most-significant bits.

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

As Kartman said, ORing with zero will not change anything.
If you want to set a bit to zero you will need to do something of the form ADCSRA = ADCSRA & ( ~ ( 1 << ADEN ) );

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

Thanks for pointing that out. The length before the calculation was completely done did not even enter my mind. I never thought about where it holds the number while it works through the calculation.

 

I think I followed what you said. I have altered calc_dec() so that the highest it goes at any stage is 36000 (if my math is correct), which is 16 bits.

 

void calc_dec (void){
	 ADC_value= ((8*sensor_array[7])+(7*sensor_array[6])+(6*sensor_array[5])+(5*sensor_array[4])+(4*sensor_array[3])+(3*sensor_array[2] + (2*sensor_array[1])+(1*sensor_array[0]))/( sensor_array[7]+ sensor_array[6]+ sensor_array[5]+ sensor_array[4]+ sensor_array[3]+ sensor_array[2]+ sensor_array[1]+ sensor_array[0]));
	 ADC_value=ADC_value*1000;
 }

I have tried the new calc_dec() both with the ADC turning off after every conversion, and without.

 

Unfortunately, the numbers are still wonky (ex. alternating between 30000 and 00000, with nothing near the sensors).

 

On the plus side though, new considerations and cleaner code with every tip :)

 

EDIT: Realized I still have it as ORing through the channels; will try to fix that next.

Last Edited: Sun. Dec 17, 2017 - 04:45 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Thanks mikech for catching that I was still ORing without having cleared the last four bits of ADMUX (which is where the channel is written). analog() now reads:

void analog (void) {
 while (current_channel <8){
	ADCSRA |= (1 << ADEN);
	ADMUX  &=  0xF0; //Clearing the last 4 bits of ADMUX
	ADMUX |= current_channel;
	ADCSRA |= (1 << ADSC); //Start conversion process
	while ( ADCSRA & (1 << ADSC ) );
		sensor_array[current_sensor_array_position] = 255-ADCH;
		
		current_channel++;
		current_sensor_array_position++;
	}
	current_channel = 0;
	current_sensor_array_position=0;

	}

Values are still wonky, but that is one more problem down.

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

//
//  don't use globals unless you really need to.
//  a function should return a value
//
uint16_t analog (uint8_t chan)
{
	ADCSRA |= (1 << ADEN);//adc should already be enabled - should not need to do it again

	ADMUX  &=  0xF0; //Clearing the adc channel select - comments should add value - not say the obvious
	ADMUX |= chan;
	ADCSRA |= (1 << ADSC); //Start conversion process

	while ( ADCSRA & (1 << ADSC ) );

        return ADC;
}

The secret to writing simple code that is easy to understand and debug is to write a function that does ONE thing and does it well. Your function does a number of things and you haven't figured out the problem.

Above is a nice, simple function. Make sure that works as you expect, then add the extra magic - but not to this function! Add it elsewhere. 

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

The integer overflow problem still exists.
Working backwards, the maximum value in each element of sensor_array[] can be 255, which makes the maximum value in
ADC_value= ((8*sensor_array[7])+(7*sensor_array[6])+(6*sensor_array[5])+(5*sensor_array[4])+(4*sensor_array[3])+(3*sensor_array[2] + (2*sensor_array[1])+(1*sensor_array[0]))/( sensor_array[7]+ sensor_array[6]+ sensor_array[5]+ sensor_array[4]+ sensor_array[3]+ sensor_array[2]+ sensor_array[1]+ sensor_array[0]));
= 9180
which when multiplied by 1000 will overflow an 16-bit 'int'.
Make ADC_value a 'long' or (preferably) an 'int32_t', (which means that you will need to decode and print a 7-digit decimal value)
or skip the multiplication by 1000 (which means your displayed value is scaled by 1/1000)

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

Thank you for the writing/design tip Kartman. I am going to revise and try breaking it up.

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

Hi mikech. I am not quite following. The first part (underlined) ADC_value= ((8*sensor_array[7])+(7*sensor_array[6])+(6*sensor_array[5])+(5*sensor_array[4])+(4*sensor_array[3])+(3*sensor_array[2] + (2*sensor_array[1])+(1*sensor_array[0]))/( sensor_array[7]+ sensor_array[6]+ sensor_array[5]+ sensor_array[4]+ sensor_array[3]+ sensor_array[2]+ sensor_array[1]+ sensor_array[0])); does add to 9180, but if it did it I thought it would be divided by the second half (italicized) which would be a divisor of 2040 before it moves onto the next line with multiply by 1000.

Do the steps work differently?

 

Though now that I look at that again, if it works like I think it does, it means I have a float temporarily..... So switching to 'int32_t and changing the math up a bit.

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

:embarrassed:
I didn't 'see' that division by the sum-of-the-samples, i fixated on the multiplications.

'C' will promote intermediate calculations to whatever it finds to be the 'widest' data-type.
In your calculation there are only integer constants and int variables, therefore all the calculations will be done as 16-bit 'ints'.
I would expect the calculated value of ADC_value to be in the range of 0 to 8000 (with intermediate values in steps of 1000 because of the integer division).
At this stage I would (in calc_dec() ) print-out the values in sensor_array[] and then manually evaluate that formula and compare it with the ADC_value value.

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

I would do the ADMUX in one step, otherwise you're briefly setting the channel to 0 before changing it to the desired channel (ADMUX is volatile):

  ADMUX = (ADMUX &  0xF0) | chan;

"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

I would avoid having blocking code like this:

while ( ADCSRA & (1 << ADSC ) );

In a function.

 

Use interrupts. I personally would use a timer ISR, but it's a matter of taste and what else you need to do in the real program.