3
\$\begingroup\$

I'm trying to configure my ATMEGA328p to measure temperature using the TMP36GZ temperature sensor. However, the 10-bit ADC keeps returning 1023. I've tried disconnecting the temperature sensor (it should return 0 at this point for the ADC reading), but it still returns 1023. I'm not exactly sure what I'm doing wrong in my code:

volatile uint16_t TMP36_VoutADCReading = 0U;

void TMP36_Init( void )
{
   // Set the ADC prescaler to 128 (i.e., 16MHz/128 = 125KHz)
   ADCSRA |= ( 1 << ADPS2 ) | ( 1 << ADPS1 ) | ( 1 << ADPS0 );

   // Set the voltage reference from AVcc (i.e., 5V).
   ADMUX |= ( 1 << REFS0 );

   // Turn on the ADC.
   ADCSRA |= ( 1 << ADEN );

   // Do the initial conversion (i.e., the slowest conversion)
   // to ensure that everything is up and running.
   ADCSRA |= ( 1 << ADSC );
}

void TMP36_ADCRead( uint8_t channel )
{
   // Clear the previously read channel.
   // ADCSRA &= 0xf0;                      <-- This was causing the bug.
   ADMUX &= 0xf0;                          // <-- This is the fix. Thanks IgnacioVazquez-Abrams!

   // Select the ADC channel to be read.
   ADMUX |= channel;

   // Start a new conversion. By default, this conversion will
   // be performed in single conversion mode.
   ADCSRA |= ( 1 << ADSC );

   // Wait until the conversion is complete.
   // while( ADCSRA & ( 1 << ADSC ) );        <-- This is probably also correct.
   while( ADCSRA & ( 1 << ADIF ) );           // <-- Added. Thanks Adithya!

   // Obtain the ADC reading from Vout.
   TMP36_VoutADCReading = ADC;

   // Clear the ADIF flag.
   ADCSRA |= ( 1 << ADIF );                    // <-- Just added. Thanks Adithya!
}

int main( void )
{
   // Disable all interrupts for the time being.
   cli();

   // Initialize the TMP36GZ temperature sensor.
   TMP36_Init();

   // Enable all interrupts.
   sei();

   while( 1 )
   {
      // Compute the distance.
      uint8_t channel = 1;
      TMP36_ADCRead( channel ); // At this point, TMP36_VoutADCReading = 1023
   }
}

In terms of connections, I'm trying to read the output voltage from pin PC1 on the ATMEGA328p and I have AVcc connected to VCC and pin 22 (i.e., GND) connected to pin 8's GND. I have disabled setting the voltage reference from AREF, so AREF isn't connected.

Any help would be greatly appreciated.

UPDATE: IgnacioVazquez-Abrams pointed out that ADCSRA &= 0xf0; should have been ADMUX &= 0xf0;. I've since modified the code above. Also, thanks Adithya for the suggestion you made about checking the ADIF flag! I'll take it into consideration.

\$\endgroup\$
  • 1
    \$\begingroup\$ "// Clear the previously read channel." What? \$\endgroup\$ – Ignacio Vazquez-Abrams Oct 26 '14 at 3:49
  • \$\begingroup\$ @IgnacioVazquez-Abrams: The lower 4 bits of ADMUX appear to be used towards channel selection. That operation just zeros out those lower 4 bits and the line after that sets those 4 bits to another channel. \$\endgroup\$ – Justin Oct 26 '14 at 4:30
  • 1
    \$\begingroup\$ Perhaps you should be applying it to ADMUX then. \$\endgroup\$ – Ignacio Vazquez-Abrams Oct 26 '14 at 4:33
  • \$\begingroup\$ @IgnacioVazquez-Abrams: Yeah. That's a mistake. I'll change it to ADMUX to see if that changes the situation. When I used the new ATMEGA328p, I disabled that line of code. Maybe the ADC isn't fried after all...let me check. \$\endgroup\$ – Justin Oct 26 '14 at 6:40
  • \$\begingroup\$ @IgnacioVazquez-Abrams: You were right! I was setting the wrong register! How should I post this as an answer? \$\endgroup\$ – Justin Oct 26 '14 at 7:02
4
\$\begingroup\$

The solution provided by IgnacioVazquez-Abrams was correct. Apparently, I was incorrectly clearing the lower 4 bits in ADCSRA when I should have been doing it to ADMUX. I've updated the code above to reflect this and the errors I've made in the code have been commented out (they are still there for reference).

\$\endgroup\$
2
\$\begingroup\$

// Wait until the conversion is complete. while( ADCSRA & ( 1 << ADSC ) )

This is wrong. You should be using ADIF to check if the conversion is complete. So,the code would be

while(!(ADCSRA&(1<<ADIF)); 
 or
while(ADCSRA&(1<<ADIF)==0);

This waits for the ADIF flag to be set once the conversion is complete. Then you have to clear the flag manually by setting the ADIF flag in order for the next conversion to start:

ADCSRA|=(1<<ADIF);

That said,why don't you stick on to ISRs? That would make life a hell lot easier.

\$\endgroup\$
  • 1
    \$\begingroup\$ "ADSC will read as one as long as a conversion is in progress. When the conversion is complete, it returns to zero." \$\endgroup\$ – Ignacio Vazquez-Abrams Oct 26 '14 at 4:55
  • 1
    \$\begingroup\$ Edit: But according to the data sheet, checking for ADIF to be set not only indicates the completion of conversion but also updates the data registers. Is it the same with ADSC becoming zero? Do the data registers get updated automatically as soon as ADSC is set to zero? \$\endgroup\$ – Adithya Oct 26 '14 at 5:13
  • \$\begingroup\$ I don't know. The datasheet doesn't specify either way. \$\endgroup\$ – Ignacio Vazquez-Abrams Oct 26 '14 at 5:17
  • 1
    \$\begingroup\$ Yeah! But,checking for ADIF has never failed me and hence I've never bothered to check for ADSC becoming zero.Thank you,anyway! \$\endgroup\$ – Adithya Oct 26 '14 at 5:26
  • \$\begingroup\$ @Adithya: Thank-you Adithya! I had no idea that this was the way it was supposed to be done, as I'm fairly new to AVR programming. \$\endgroup\$ – Justin Oct 26 '14 at 7:09

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.