Need a double-check of my ADC code...

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

I'm working on modifying a PID-controller for a reflow oven to use two thermocouples, and I'm hoping that someone would be good enough to check my code and make sure that I haven't messed up somewhere. :D Since I'm still new to programming in C, a second opinion would be most welcome.

The original controller was built around an ATMega32U running at 16MHz. I've adjusted the inputs to use ADC channels 8, 9, and 10 instead of 0 and 1... plus made the actual ADC read a function call instead of doing it directly each time.

For reference, here is the original code...

#include "thcouple.h"

// 12-bit temperature (0.25 deg/div)
int16_t thcouple_read(void)
{
	ADCSRB &= ~(1<<MUX5); // ADC0 channel - thermocouple
	ADCSRA |= (1<<ADSC); // start conversion
	do {}
	while ( !(ADCSRA & (1<<ADIF)) );
	ADCSRA |= (1<<ADIF); // clear ADIF flag
	
	// 41 uV/*C, K = 196 -> 41*196 = 8.036 mV/*C / 2.5 mV/div -> 3.2144 div/*C -> 0.311 *C/div
	uint16_t th_temp = ADC;
	
	th_temp *= 31;
	th_temp /= 100; // 1 *C/div
	th_temp <<= 2; // 1/4 *C/div
	

	ADCSRB |= (1<<MUX5); // ADC8 channel - TMP37 (cold junction)
	ADCSRA |= (1<<ADSC); // start conversion
	do {}
	while ( !(ADCSRA & (1<<ADIF)) );
	ADCSRA |= (1<<ADIF); // clear ADIF flag
	
	// 20 mV/*C, 0V at 0*C -> 1/8 *C/div
	uint16_t tmp37_temp = ADC;
	tmp37_temp >>= 1; // 1/4 *C/div
	
	th_temp += tmp37_temp;
	
	return th_temp;
}

void adc_setup(void)
{
	// ADC @ 250 kHz single conversion mode, 2.56V ARef
	DIDR0 = (1<<ADC0D); // thermocouple
	DIDR2 = (1<<ADC8D); // TMP37 cold junction sensor
	ADMUX = (1<<REFS0) | (1<<REFS1);
	ADCSRA = (1<<ADEN) | (1<<ADPS1) | (1<<ADPS2);
	ADCSRB = (1<<ADHSM);
	
	ADCSRA |= (1<<ADSC); // dummy conversion
	do {}
	while ( !(ADCSRA & (1<<ADIF)) );
	ADCSRA |= (1<<ADIF); // clear ADIF flag
}

And my modified version...

#include "thcouple.h"

// 12-bit temperature (0.25 deg/div)
int16_t thcouple_read(void)
{
  // Get the cold-junction reading first...
	// Channel ADC8 = TMP37 (cold junction)  
	// 20 mV/*C, 0V at 0*C -> 1/8 *C/div
  
	uint16_t tmp37_temp = adc_read();
	tmp37_temp >>= 1; // 1/4 *C/div

  ADMUX |= (1<<MUX0); // Channel ADC9 = thermocouple #1
		
  
	uint16_t th_temp = adc_read();  // Get the first temperature reading...

  ADMUX |= (1<<MUX1); // Channel ADC10 = thermocouple #2
  
  th_temp += adc_read(); //  Add the second temperature reading to the first...
  th_temp <<= 1 ;  // Divide by 2 for average

  // Now convert to temperature in *C
  // 41 uV/*C, K = 196 -> 41*196 = 8.036 mV/*C / 2.5 mV/div -> 3.2144 div/*C -> 0.311 *C/div

  th_temp *= 31;
	th_temp /= 100; // 1 *C/div
	th_temp <<= 2; // 1/4 *C/div
	
	th_temp += tmp37_temp;  // Add in the cold-junction temp for true reading...
	
	return th_temp;
}

void adc_setup(void)
{
	// ADC @ 250 kHz single conversion mode, 2.56V ARef, using channels 8-10
  // ADC8  = TMP37 cold junction sensor
  // ADC9  = Thermocouple #1
  // ADC10 = Thermocouple #2
  
	DIDR2 = (1<<ADC8D) | (1<<ADC9D) | (1<<ADC10D); 
  ADMUX = (1<<REFS0) | (1<<REFS1);
	ADCSRA = (1<<ADEN) | (1<<ADPS1) | (1<<ADPS2);
	ADCSRB = (1<<ADHSM) | (1<<MUX5) ;
	
	adc_read(); // Dummy read to finish initialization
}

uint16_t adc_read(void)
{
ADCSRA |= (1<<ADSC); // start conversion

	while (ADSRA & (1<<ADSC)) ;  // Wait for the conversion to finish...
	
  ADMUX &= ~(1<<MUX0) | ~(1<<MUX1) ; // Reset ADC Mux to Channel 8 (TMP_37)
  
return (ADC) ;
}

Any advice about functionality, as well as readability, would be greatly appreciated.

Last Edited: Tue. Aug 2, 2011 - 02:12 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Quote:

The original controller was built around an ATMega32U running at 16MHz.

What AVR model are you using now?

I'd guess there are no MUXnn bits in ADCSRB except MUX5 if you are still using 'U4.

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

Couple of notes

1) <<= 1 doesn't divide by 2, it multiplies by 2. >>= 1 divides by 2.

2) The /100 is expensive on an AVR (and most any other chip without some divide hardware). Much, much better to use >>=, that is, to divide by powers of 2. In your case, 31.1 / 100 * 128 = 39.8. 40 is only off by 2 parts in 400. So the code would be

  th_temp *= 40; 
  th_temp >>= 5; // 1/4 *C/div 

Note that the >>= 5 combines the /128 (>>=7) and the <<=2 into one operation.

3) My personal preference, based on problems I've had and others have had, is to switch to the next-to-read MUX channel immediately after reading an ADC value. This gives the voltage through the MUX the longest time to settle.

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

Quote:

This gives the voltage through the MUX the longest time to settle.


That has been an age-old debate here. Are you sure that the voltage is presented to the sample circuit at any time other than the sample period? In past threads people have made partial believers out of me--but only for signals that don't have any "drive" to them.

This must be some kind of a packaged thermocouple module with those voltages, IME. (Right?)

Quote:
Using a dissimilar metal to complete the circuit creates a circuit in which the two legs generate different voltages, leaving a small difference in voltage available for measurement. That difference increases with temperature, and is between 1 and 70 microvolts per degree Celsius (µV/°C) for standard metal combinations.

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

theusch wrote:
Quote:

This gives the voltage through the MUX the longest time to settle.


That has been an age-old debate here. Are you sure that the voltage is presented to the sample circuit at any time other than the sample period? In past threads people have made partial believers out of me--but only for signals that don't have any "drive" to them.

I don't doubt that there are cases where it's not an issue. OTOH, I have seen it be an issue, and so have others, and it costs nothing to do things in the order I suggested, so it's just become second nature for me. I don't want to have to re-think it for every input source, when I can't see any benefit on the back end.

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

Quote:

it costs nothing to do things in the order I suggested,

Well, I also do things in the order you suggest. Like for a microsecond...

//
// **************************************************************************
// *
// *		A D C _ I S R
// *
// **************************************************************************
//
// ADC interrupt service routine
// with auto input scanning
interrupt [ADC_INT] void adc_isr(void)
{
// Read the AD conversion result
	adc_raw[ad_index] = ADCW;

// Select next ADC input
	if (++ad_index >= ad_count)
		{
		ad_index=0;
		}

	ADMUX = ADC_VREF_TYPE + ad_index;

// Start the AD conversion
	ADCSRA |= (1 << ADSC);

}

While it might vary a bit, above is the normal guts for my continuous round-robin conversions in my production apps.

And my e.g. thermistor channels won't have a lot of "drive". But we do have a series R to protect the AVR's input, and a small cap next to the pin...

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

May I suggest that you modify your adc_read routine to include setting the mux. That way you can have a simple call that does what it says eg:

#define THERM1 8
#define THERM2 9
#define COLDJUNC 10

val = adc_read(THERM1);
val = adc_read(THERM2);
val = adc_read(COLDJUNC);

it pretty obvious what is happening without extra comments. As for integer vs float calcs, floating point is expensive in terms of execution time and code space. On a 32K device code space is less of an issue and since we're dealing with thermocouples, speed isn't an issue. So if using floats makes your job easier then by all means use them.

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

Kartman wrote:
...As for integer vs float calcs, floating point is expensive in terms of execution time and code space. On a 32K device code space is less of an issue and since we're dealing with thermocouples, speed isn't an issue. So if using floats makes your job easier then by all means use them.

@OP: A middle ground is to use long ints (signed or unsigned, as required) to do the *X, >>N scaling technique. This will be slower/larger than using 16-bit ints, but faster/smaller than using floats.

Using the 31.1/25 ratio found in the code, with a 10-bit ADC result we could only multiply by a number <= 63 to guarantee that the intermediate result would fit into 16 bits. But with 32 bit longs you can multiply by any number up to 2^22-1, which is some 4 million. That gives you enormously more leeway in getting the ratio exactly right. For example, if we arbitrarily decide to shift the result >>16 (meaning we'll just toss out the low 16 bits of the long, and use the hi 16 bits), our multiplier becomes 31.1*65536/25 or 81527 to about 1 part in 300000. So our calculation becomes

unsigned long val = ((unsigned long) adc_val * 81527) >> 16;
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Thanks for the input - tackling all the comments at once...

@theusch...

This is only a paper design for now, as I can't afford to buy the hardware. When I *do* get to working on it, I'll probably stick with the Mega32U, as I'm not touching the USB support.

"packaged thermocouple module"

Actually, it's a K-type thermocouple sent to an op-amp that is configured as a differential amplifier with a gain of 196.

"giving the voltage through MUX the longest time to settle"

Should I build a small delay between setting the MUX and starting the conversion...? Given that Thermocouple #1 and Thermocouple #2 are essentially being read back-to-back...

@kk6gm...

"1) <<= 1 doesn't divide by 2, it multiplies by 2. >>= 1 divides by 2."

Soo... the *original* code was wrong...? But it (apparently) worked properly.

For #2 - thanks... I wouldn't have even thought of that.

@Kartman...

I was thinking of doing that eventually - this is, after all, a first-draft rework of the code... making relatively minor changes and letting sophistication come later.

I suppose the modified adc_read routine would be along the lines of...

uint16_t adc_read(th_channel)
{
  ADMUX = (1<<REFS0) | (1<<REFS1) ; // 2.56V internal reference used

  switch (th_channel) {
        case 8:
             ADMUX &= ~(1<<MUX0) | ~(1<<MUX1);
             break;

        case 9:
             ADMUX |= (1<<MUX0);
             break;

        case 10:
             ADMUX |= (1<<MUX1);
             break;
}

ADCSRA |= (1<<ADSC); // start conversion

   while (ADSRA & (1<<ADSC)) ;  // Wait for the conversion to finish...
   
return (ADC) ;
} 

... or something similar.

Regarding the use of 32-bit integers... I don't think that would be necessary for this application. After all, it's going to be used to drive the heater elements of a modified toaster-oven - there's not a lot of precision involved in the baseline design. :D

(I just noticed that I made a few other errors, when it comes to which registers I'm setting. I'll make corrections shortly.)

Last Edited: Tue. Aug 2, 2011 - 02:21 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

The external amp with the hi gain needs to be low drift with temperature, good tempco on the resistors. Maybe look at the MAX6675 thermocouple preamp?

Imagecraft compiler user

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

Bob - that's V2 of the circuit... using a bit-banging bit of code for the SPI read routine. (Easier to route the board that way... :) ) I'm currently in a debate with myself over which one to choose.

Bear in mind that this isn't going to be used in a commercial or an industrial setting - it's a hobby project, where a bit of drift isn't critical.

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

RocketMan_Len wrote:
@kk6gm...

"1) <<= 1 doesn't divide by 2, it multiplies by 2. >>= 1 divides by 2."

Soo... the *original* code was wrong...? But it (apparently) worked properly.


I don't see where the original code used it. Maybe I missed it.

Quote:
For #2 - thanks... I wouldn't have even thought of that.

It's a fundamental technique with smaller micros, and it's not particularly obvious (unless you're writing in assembly language :))

Quote:
Regarding the use of 32-bit integers... I don't think that would be necessary for this application. After all, it's going to be used to drive the heater elements of a modified toaster-oven - there's not a lot of precision involved in the baseline design. :D

I agree, doesn't seem to be necessary in your case. At most your scaling would be off 1/2 bit out of 6 bits, or 1 part in 128. But it's good to be aware of the option.

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

Quote:

I suppose the modified adc_read routine would be along the lines of...

Only if you ALWAYS do the channels in order 8-9-10. If you don't see why, ask.

[Why do people insist on doing complex logic to rebuild ADMUX instead of just building the proper value?

#define  ADC_REF_BITS ((1<<REFS0) | (1<<REFS1))
...
ADMUX = ADC_REF_BITS | th_channel;
...

Now it always works regardless of the order of conversions, assuming "th_channel" is passed as the correct MUXn value. Admittedly in this case the MUX5 bit needs to be addressed but since you are sticking to the high channels (and have not even picked an AVR model) it is doable.]

Lee

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

@kk6gm...

Looking at the code again, and the conversion result is returned as a value of *quarter* degrees, rather than whole degrees. So he did the math to convert the ADC value to degrees C, then multiplied by 4 to make it quarter-degrees. (Why he didn't simply divide by 25, or find a conversion factor that calculates quarter-degree values directly, I don't know.)

@theusch...

Quote:
Why do people insist on doing complex logic to rebuild ADMUX instead of just building the proper value?

Well... it was late at night, I was tired, I'm not very proficient in C programming, and took a simple 'brute-force' approach. I likely would have gotten there *eventually*. :)

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

Okay... I've been doing some tweaking of the ADC code, based on the responses I've received on this thread, and have come up with the following...

File thcouple.h

#ifndef THCOUPLE_H
#define THCOUPLE_H

#include 
#include 

#define ADC_REF_BITS ((1<<REFS0) | (1<<REFS1))  // 2.56V ADC Reference Voltage
#define THERM1 0
#define THERM2 1
#define COLDJUNC 2

// 12-bit temperature (0.25 deg/div)
int16_t thcouple_read(void);
void adc_setup(void);
uint16_t adc_read(void);

#endif

File thcouple.c

#include "thcouple.h"

// 12-bit temperature (0.25 deg/div)
int16_t thcouple_read(void)
{
  // Get the cold-junction reading first...
	// Channel ADC8 = TMP37 (cold junction)  
	// 20 mV/*C, 0V at 0*C -> 1/8 *C/div
  
	tmp37_temp = adc_read(COLDJUNC);
	tmp37_temp <<= 1; // 1/4 *C/div

  th_temp = adc_read(THERM1);  
  th_temp += adc_read(THERM2); 
  th_temp >>= 1 ;  

  // Now convert to temperature in 0.25*C increments
  // 41 uV/*C, K = 196 -> 41*196 = 8.036 mV/*C / 2.5 mV/div -> 3.2144 div/*C -> 0.311 *C/div

   th_temp *= 40;
   th_temp >>= 5; // 1/4 *C/div
   
	th_temp += tmp37_temp;  // Add in the cold-junction temp for true reading...
	
	return th_temp;
}

void adc_setup(void)
{
	// ADC @ 250 kHz single conversion mode, 2.56V ARef, using channels 8-10
  // ADC8  = TMP37 cold junction sensor
  // ADC9  = Thermocouple #1
  // ADC10 = Thermocouple #2
  
	DIDR2 = (1<<ADC8D) | (1<<ADC9D) | (1<<ADC10D); 
  ADMUX = (1<<REFS0) | (1<<REFS1);
	ADCSRA = (1<<ADEN) | (1<<ADPS1) | (1<<ADPS2);
	ADCSRB = (1<<ADHSM) | (1<<MUX5);
	
	uint16_t tmp37_temp = adc_read(COLDJUNC); // Dummy read to finish initialization
}

uint16_t adc_read(th_chan)
{ 
ADMUX = ADC_REF_BITS | th_chan;   // Select Channel
ADCSRA |= (1<<ADSC); // start conversion

	while (ADSRA & (1<<ADSC)) ;  // Wait for the conversion to finish...
	
return (ADC) ;
}

Now... one thing I'm not sure of, and would like a second opinion on, is a change to the cold-junction reading code.

Would this line of code...

tmp37_temp = (adc_read(COLDJUNC) <<1 ); // 1/4 *C/div

... perform the same function as the two lines in the existing code...? I *think* that it will... :)

Also, can I combine the lines that read and average the two thermocouples into this...

th_temp = (adc_read(THERM1) + adc_read(THERM2)) >>1;