HELP: My ATMEGA8 ADC is NOT converting

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

Hi,

 

I am trying to do an ADC conversion from and LDR that gives off different voltages depending how much light is shine onto it.  I have verified the LDR to work properly via multimetter, measuring it output voltages as I shine the light or not directly at the LDR.

 

Below is my ATMEGA8 ADC code, and it is written as interupt driven.  I have verfied that the ISR gets called as it should, but the problem is that the converted ADCL and ADCH values are always the same, 0xFF and 0x03, respectively.  Looks like conversion is not taking place.

 

Would greatly appreciate for any help.

 

My ATMEGA8 runs at 8Mhz (F_CPU set to 8000000UL) using the internal RC clock, with the fuses programmed as 0xC4 (low fuse byte) and 0xD9 (high fuse byte).  The ADC is setup to be free running.  And ADC0 is the ADC channel I use.  I don't have my ATMEGA8's AREF connected to anything but have AVCC connected to VCC, which is 5V, with a decoupling 0.1uF capacitor.  I have disabled the internal AREF REFS0 and REFS1 both equal to zero.  It's actually does matter, that is, same problem occurs, if I have REFS0 & REFS1 equal zero or REFS0=1 and REFS1=0 to use AVCC as external analog reference voltage.

 

Thank you!

 

// ADC Module for the ATMEGA8.

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

// High when an ADC value is ready to be read
volatile uint8_t readFlag = 0;

// Value to store analog result
volatile uint16_t adcValue = 0;

// Initialization
void adcSetup() {

  // ADC initially not ready yet.
  readFlag = 0;

  // clear ADLAR in ADMUX to right-adjust the result
  // ADCL will contain lower 8 bits, ADCH upper 2 (in last two bits)
  ADMUX &= 0b11011111;

  // Set REFS1..0 in ADMUX to change reference voltage to the
  // proper source (01).
  // REFS = OFF and REFS1 = OFF, meaning internal AREF turned OFF
  ADMUX &= 0b00111111;  // 7th & 8th bits, REFS0 & REFS1, respectively.

  // Set the ADC channel.
  ADMUX &= 0b11110000;  // ADC0

  // Set ADEN in ADCSRA to enable the ADC.
  ADCSRA |= 0b10000000;

  // FREE RUNNING MODE:
  ADCSRA |= 0b00100000;  // FOR ATMEGA8, ADFR, the 6th bit is set
                         // for free running.

  // Set sampling frequency to be below 200KHz based on F_CPU defined.
  // Set the Prescaler to 64(8000KHz/64 = 125KHz). ADPS2 = ADPS1 = 1
  ADCSRA |= 0b00000110;

  // Set ADIE in ADCSRA to enable the ADC interrupt.
  ADCSRA |= 0b00001000;
  sei();  // enable 'I' bit in the SREG for global interrupt.

  // Set ADSC in ADCSRA to start the ADC conversion
  ADCSRA |= 0b01000000;
}

// Fetch ADC value.
uint16_t adcRead() {

  uint16_t retVal = 0;

  // Check to see if the value has been updated.
  if (readFlag == 1) {

    retVal = adcValue;

    // Perform whatever updating needed
    readFlag = 0;
  }
  return retVal;
}

// Interrupt service routine for the ADC completion. This ISR is called
// when the ADCSRA register's ADIF bit is set, signaling an ADC conversion
// is completed.
ISR(ADC_vect) {

  // Done reading
  readFlag = 1;

  // Must read low first
  adcValue = ADCL | (ADCH << 8);

  // Not needed because free-running mode is enabled.
  // Set ADSC in ADCSRA to start another ADC conversion
  // ADCSRA |= 0b01000000;

}

 

Last Edited: Mon. Sep 19, 2022 - 01:25 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Why are you using interrupts just to get things going?  Use that some other time, if at all.

 

Also why is this code 10000 miles long???? It should easily fit on 10 to 20 lines TOTAL to show all of what you are doing.  Why make a gigantic haystack to wade through???  You'll never spot an error.

// Fetch ADC value.

Ok, isn't that what ADCread does?  This increases dilution with things that add little/no information

 

Just write the lines of code you need, and it would probably already be working. You are burying yourself doing nothing--you can always write a tutorial article to go with the code.

When in the dark remember-the future looks brighter than ever.   I look forward to being able to predict the future!

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

avrcandies wrote:

Why are you using interrupts just to get things going?  Use that some other time, if at all.

 

Also why is this code 10000 miles long???? It should easily fit on 10 to 20 lines TOTAL to show all of what you are doing.  Why make a gigantic haystack to wade through???  You'll never spot an error.

// Fetch ADC value.

Ok, isn't that what ADCread does?  This increases dilution with things that add little/no information

 

Just write the lines of code you need, and it would probably already be working. You are burying yourself doing nothing--you can always write a tutorial article to go with the code.

 

Take it easy, man!  You overworked yourself there.  The code is with ample comments for ease of reading.  I think it's the way coloring of the text, comments and actual code statements, of the same color that makes it hard to read.  The code tag "<>" can't do code coloring, as it looks like.

 

Last Edited: Mon. Sep 19, 2022 - 02:54 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

The code is with ample comments for ease of reading. 

Good--now that you removed about 40 lines of stuff. it MUCH more tolerable-- it looked more like reading the datasheet ,sentence by sentence

Whatever mistake(s) you code up then gets buried in all the verbosity

 

Why do you need to use IRQ's right now?   Note you are not using volatile, as required.

 

and the exact code I need is exactly posted. 

ok?  

 

  ADCSRA |= 0b10000000;

Why are you configuring this register five different times?? You should be able to figure out the complete value and set it (including lots or little comments, either way)

ADCRSA = my_figured_out_value

To top it off, the inefficient 5 times, now taking 5x of the program space, and diluting your listing, leaves some bits in a potentially undesired state, since 3 of them are being ignored. Who told you to do this?    

Always use  = when configuring initializations, this has been posted about a million times.  Do it and avoid hard-to-find reset gotcha's.

 

 

 ADMUX &= 0b1101 1111;
ADMUX &= 0b0011 1111;
 ADMUX &= 0b1111 0000;

Means you simply write ADMUX = 0b000Y0000;  once, with Y set 0/1 as you want (your code leaves this bit unresolved)

Also, use the bit or bit weighted names, not 1's and 0's---big error source 

 

 

When in the dark remember-the future looks brighter than ever.   I look forward to being able to predict the future!

Last Edited: Mon. Sep 19, 2022 - 02:35 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1

Try set

 

REFS1 = 1

REFS0 = 1

 

to select internal 2.56V voltage referencing.

 

Using a 1k resistor, short the ADC reading pin to microcontroller ground to see if the conversion value is going down to zero or close to it.

 

Then we can isolate if it is the code or the external hardware problem.

I am a two-star programmer, but I prefer to be a one-star programmer. C what I did there?

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

MilkyBay wrote:

Try set

 

REFS1 = 1

REFS0 = 1

 

to select internal 2.56V voltage referencing.

 

Using a 1k resistor, short the ADC reading pin to microcontroller ground to see if the conversion value is going down to zero or close to it.

 

Then we can isolate if it is the code or the external hardware problem.

 

Great suggestion there on setting REFS0 = REFS1 = 1.   Not sure if I can do the 1k resistor thing, since the AMTEGA8 is TQFP format.

 

Thank you!

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

No worries there.

 

Also, avrcandies suggestions are correct as well if you take the "Harsh Tones" away.

 

Try to apply what avrcandies said without the "Hard Tones" plus with my suggestion. I hope you solved your problem.

I am a two-star programmer, but I prefer to be a one-star programmer. C what I did there?

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

unebonnevie wrote:

// Set REFS1..0 in ADMUX to change reference voltage to the
  // proper source (01).
  // REFS = OFF and REFS1 = OFF, meaning internal AREF turned OFF
  ADMUX &= 0b00111111;  // 7th & 8th bits, REFS0 & REFS1, respectively.

 

You are confusing AREF (pin) and Vref (voltage). For the sake of correctness, text should be:

 

// REFS = OFF and REFS1 = OFF, meaning Internal Vref turned OFF

 

Note capitalized 'Internal'. The full name of this voltage is 'Internal Vref', while AREF is just one pin (and Internal Vref may be connected to this pin).

 

 

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

unebonnevie wrote:
I don't have my ATMEGA8's AREF connected to anything

  // Set REFS1..0 in ADMUX to change reference voltage to the
  // proper source (01).
  // REFS = OFF and REFS1 = OFF, meaning internal AREF turned OFF
  ADMUX &= 0b00111111;  // 7th & 8th bits, REFS0 & REFS1, respectively.

 

Well there's your problem. Your code doesn't match the comment. You have No Reference Voltage.

 

Table 22-3. Voltage Reference Selections for ADC

REFS1 REFS0 Voltage Reference Selection
0 0 AREF, Internal Vref turned off
0 1 AVCC with external capacitor at AREF pin
1 0 Reserved
1 1 Internal 2.56V Voltage Reference with external capacitor at AREF pin

 

 

Therefore you need to connect an external capacitor to AREF Pin and use this (avrcandies compliant) code instead:

 

 

// Initialization
void adcSetup() {

  // Flag ADC Reading not ready yet.
  readFlag = 0;

  /* ADMUX BITS */
  // Clear ADLAR in ADMUX to right-adjust the result
  // ADCL will contain lower 8 bits, ADCH upper 2 (in last two bits)
  // Set REFS1:0 to 01. Vref = "AVCC with external capacitor at AREF pin"
  // Set the ADC channel to ADC0.
  ADMUX = (0b01 << REFS0) | (0 << ADLAR) | (0 << MUX0);

  /* ADCSRA BITS */
  // Set ADEN in ADCSRA to enable the ADC.
  // FREE RUNNING MODE:
  // Set sampling frequency to be below 200KHz based on F_CPU defined.
  // Set the Prescaler to 64(8000KHz/64 = 125KHz). ADPS2 = ADPS1 = 1
  // Set ADIE in ADCSRA to enable the ADC interrupt.
  // Set ADSC in ADCSRA to start the ADC conversion
  ADCSRA = (1 << ADEN) | (0 << ADSC) | (1 << ADFR) | (1 << ADIF) | (1 << ADIE) | (0b110 << ADPS0);
  ADCSRA |= (1 << ADSC);

  sei();  // Enable Global Interrupts.
}
 

 

 

Last Edited: Mon. Sep 19, 2022 - 08:38 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Reading this thread I'm reminded of the perennial rule here that if you haven't got anything positive /helpful to say then don't say anything at all. 

 

Moderator 

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

This code seems a possible cause for the problem.

// Must read low first
  adcValue = ADCL | (ADCH << 8);

 

In C there are no guarantees of sub-expression evaluation order, except (IIRC) for ',', '&&', and '||'.

Splitting the statement in two or simply

 

adcValue = ADC;

 

may fix the problem.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0
adcValue = ADC;

This. 

 

While there might be an argument for using ADCH (if using left adjust) there is no reason anyone should ever touch ADCL. Use either ADCH (8 bit) or ADC (10 bit) 

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

gatk wrote:

In C there are no guarantees of sub-expression evaluation order, except (IIRC) for ',', '&&', and '||'.

But where volatile SFR access in involved the compiler has it's hands tied, it MUST read the SFRs in left-to-right order. Therefore the original ISR code should work.

 

Despite the above, I'm still recommending the use of adcValue = ADC; however.

 

Last Edited: Tue. Sep 20, 2022 - 07:21 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

"But where volatile SFR access in involved the compiler has it's hands tied, it MUST read the SFRs in left-to-right order. Therefore the original ISR code should work."

 

A web search on "C standard volatile" yields nothing to support that, so a reference would be helpful.  It does describe the behaviour of the avr-gcc 5.4 that I have installed.  The generated code (-O3) does not inspire much confidence:

 

    result = ADCL | (ADCH << 8);

 

        in r24,0x4
        in r18,0x5
        ldi r25,0
        or r25,r18
        sts result+1,r25
        sts result,r24
 

Why use one register in an ISR when 3 will do?

 

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

gatk wrote:
a reference would be helpful.

 

This is the best I could find: https://gcc.gnu.org/onlinedocs/gcc/Volatiles.html

This StackOverlfow question also: https://stackoverflow.com/questions/54071349/volatile-and-sequence-point

 

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

I read those as confirming that you can not rely on left-to-right parsing, at least at the language definition level.  But the gcc 7.3 that comes with Arduino 1.9.3 does it too, and emits this even more baffling assembler:

 

        in r25,0x4
        in r24,0x5
        eor r24,r25
        eor r25,r24
        eor r24,r25
        sts result+1,r25
        sts result,r24
 

result= ADC;

 

is fairly sane, but still uses two registers and saves r0, r1 and SREG in the prologue although they are untouched by compiled code.  Perhaps Microchip could do themselves a favour and fund a developer to work on AVR code generation.