Internal voltage reference

I found this code a while back on the forums it works quite well for finding the voltage on the 5 volt pin of a Arduino. My only concern is it only works sometimes. 2 hours ago it was working returning a value of 4995 (4.995 volts) now it is returning -1. 20 minutes ago it was returning 1111111111
Is there anything I should know when using this? do I have to run it before other functions or something? I have no idea what the code means or how it works.

long readVcc() {

  // Read 1.1V reference against AVcc
  // set the reference to Vcc and the measurement to the internal 1.1V reference
  #if defined(__AVR_ATmega32U4__) || defined(__AVR_ATmega1280__) || defined(__AVR_ATmega2560__)
    ADMUX = _BV(REFS0) | _BV(MUX4) | _BV(MUX3) | _BV(MUX2) | _BV(MUX1);
  #elif defined (__AVR_ATtiny24__) || defined(__AVR_ATtiny44__) || defined(__AVR_ATtiny84__)
     ADMUX = _BV(MUX5) | _BV(MUX0) ;
  #else
    ADMUX = _BV(REFS0) | _BV(MUX3) | _BV(MUX2) | _BV(MUX1);
  #endif  
 
  delay(2); // Wait for Vref to settle
  ADCSRA |= _BV(ADSC); // Start conversion
  while (bit_is_set(ADCSRA,ADSC)); // measuring
 
  uint8_t low  = ADCL; // must read ADCL first - it then locks ADCH  
  uint8_t high = ADCH; // unlocks both
 
  long result = (high<<8) | low;
 
  result = 1125300L / result; // Calculate Vcc (in mV); 1125300 = 1.1*1023*1000
  return result; // Vcc in millivolts
}

...now it is returning -1. 20 minutes ago it was returning 1111111111...

How can you possibly know that? There are no prints in the code you posted.

  long result = (high<<8) | low;

Doesn't do what you think.

By default expressions in integer context involving values and variables that are not "long", will be
done as signed int (on the Arduino that's signed 16 bit). Only then is the result used in the
assignment to the long. So if high has the top bit set the result of this expression will be negative.

You should do this:

  long result = high ;
  result = (result << 8) | low ;

or

  long result = ((long)high<<8) | low;

or

  long result = ((unsigned)high<<8) | low;

then the shift will be done in a long or unsigned int context, which is what was intended (the result
can't be negative)

...or this...

// remove  uint8_t low  = ADCL; // must read ADCL first - it then locks ADCH  
// remove  uint8_t high = ADCH; // unlocks both
 
// remove  long result = (high<<8) | low;
 
// remove    result = 1125300L / result; // Calculate Vcc (in mV); 1125300 = 1.1*1023*1000

// remove    return result; // Vcc in millivolts

    return 1125300L / ADC; // Vcc in millivolts

MarkT:

  long result = (high<<8) | low;

Doesn't do what you think.

By default expressions in integer context involving values and variables that are not "long", will be
done as signed int (on the Arduino that's signed 16 bit). Only then is the result used in the
assignment to the long. So if high has the top bit set the result of this expression will be negative.

You should do this:

  long result = high ;

result = (result << 8) | low ;




or


long result = ((long)high<<8) | low;



or


long result = ((unsigned)high<<8) | low;




then the shift will be done in a long or unsigned int context, which is what was intended (the result
can't be negative)

Or play with unions. Set up result to be a union of an unsigned long and an 2 element array of unsigned ints. Then just place high and low on the shelf where they belong. (Instead of placing high where low goes, sliding it over to where it belongs, and then placing low where it goes.) Code wise it might look something like this:

union
{
  unsigned int word[2];
  long complete;
} result;

result.word[0] = ADCL; // must read ADCL first - it then locks ADCH
result.word[1] = ADCH; // unlocks both

return 1125300L / result.complete; // Calculate Vcc (in mV); 1125300 = 1.1*1023*1000

change "word" and "complete" to what ever makes sense to you.

Set up result to be a union of an unsigned long and an 2 element array of unsigned ints.

Close. You made just two mistakes and one assumption that could be a mistake. You made just one mistake and two assumptions that could be mistakes.

Please elaborate. 2.5 mistakes across 4 statements is a pretty bad error percentage. It matches my sample code (names have been changed to protect the innocent) for doing this trick... Hmm... I do see that I changed from unsigned long to signed long in the code snippet, but forgot to change my preamble... Is that one of them?

Very carefully compare the datatypes in your code with the original. (That's one mistake.)

Head-smack... (uint8_t is only 8 bits, not 16... I have to admit I'm still not used to the proper C datatype names and usually use the Arduino versions...)

Also the signed/unsigned long business, it really should be unsigned... The ADC on these chips never give negative values, but the conversion math (from the OP) to Vcc uses a signed long constant... So, two options:

union
{
  byte bytes[4]; // or uint8_t
  long complete; // not sure what the proper syntax is... uint32_t would be my guess for unsigned long... sint32_t? (trying to parse the abbreviations that make the data type name... may have failed...)
} result = 0; // In theory the declared variable should be zero, but it might just be what ever random data is there... Best to explicitly zero it out.

result.bytes[0] = ADCL; // must read ADCL first - it then locks ADCH
result.bytes[1] = ADCH; // unlocks both

return 1125300L / result.complete; // Calculate Vcc (in mV); 1125300 = 1.1*1023*1000

or possibly better:

union
{
  byte bytes[2]; // or uint8_t
  unsigned int complete; // or uint16_t?
} result;

result.bytes[0] = ADCL; // must read ADCL first - it then locks ADCH
result.bytes[1] = ADCH; // unlocks both

return 1125300L / result.complete; // Calculate Vcc (in mV); 1125300 = 1.1*1023*1000

not sure what the proper syntax is... uint32_t

Correct... stdint.h - Google Search

I made a mistake in Reply #5 (oh, the irony). It should have been... You made just one mistake# and two assumptions that could be mistakes.

Hint for the first assumption that could be a mistake....

union
{
  uint8_t bytes[4];
  uint32_t complete;
} result = 0;

result.bytes[0] = 0x55;
result.bytes[1] = 0xAA;

What is the value of result.complete?

# Which you found and corrected.

I made this code here

float temperature = 0;
long voltage = 0;
void setup() 
{
  Serial.begin(9600);
  pinMode(A14,INPUT);
}


void loop()
{
    voltage = readVcc();
    delay(50);
    temperature =  getTemp(A14);  
    Serial.println(temperature);            
delay(100);    

}


float getTemp(int pin)
{ 
  float tempred =  (map(analogRead(pin),0,1023,0,voltage)); 
   return((( tempred -500)/10)*1.8+32);
}




long readVcc() {
  // Read 1.1V reference against AVcc
  // set the reference to Vcc and the measurement to the internal 1.1V reference
  #if defined(__AVR_ATmega32U4__) || defined(__AVR_ATmega1280__) || defined(__AVR_ATmega2560__)
    ADMUX = _BV(REFS0) | _BV(MUX4) | _BV(MUX3) | _BV(MUX2) | _BV(MUX1);
  #elif defined (__AVR_ATtiny24__) || defined(__AVR_ATtiny44__) || defined(__AVR_ATtiny84__)
     ADMUX = _BV(MUX5) | _BV(MUX0) ;
  #else
    ADMUX = _BV(REFS0) | _BV(MUX3) | _BV(MUX2) | _BV(MUX1);
  #endif  
 
  delay(2); // Wait for Vref to settle
  ADCSRA |= _BV(ADSC); // Start conversion
  while (bit_is_set(ADCSRA,ADSC)); // measuring
 
  uint8_t low  = ADCL; // must read ADCL first - it then locks ADCH  
  uint8_t high = ADCH; // unlocks both
 
  long result = (high<<8) | low;
 
  result = 1125300L / result; // Calculate Vcc (in mV); 1125300 = 1.1*1023*1000
  return result; // Vcc in millivolts
//  return 1125300L / ADC;
}

it works for the first time, but once the code is executed again the readVcc() returns 0

  uint8_t low  = ADCL; // must read ADCL first - it then locks ADCH  
  uint8_t high = ADCH; // unlocks both
 
  long result = (high<<8) | low;

What happens to an 8 bit value that has been shifted 8 bits? I think you may need a cast in this statement. Mixing _t types and C types in one function is not really a good idea. Use one or the other.

   long result = (long)(high<<8) | low;

I have found that using analogRead(); makes the

long readVcc() {
  // Read 1.1V reference against AVcc
  // set the reference to Vcc and the measurement to the internal 1.1V reference
  #if defined(__AVR_ATmega32U4__) || defined(__AVR_ATmega1280__) || defined(__AVR_ATmega2560__)
    ADMUX = _BV(REFS0) | _BV(MUX4) | _BV(MUX3) | _BV(MUX2) | _BV(MUX1);
  #elif defined (__AVR_ATtiny24__) || defined(__AVR_ATtiny44__) || defined(__AVR_ATtiny84__)
     ADMUX = _BV(MUX5) | _BV(MUX0) ;
  #else
    ADMUX = _BV(REFS0) | _BV(MUX3) | _BV(MUX2) | _BV(MUX1);
  #endif  
 
  delay(2); // Wait for Vref to settle
  ADCSRA |= _BV(ADSC); // Start conversion
  while (bit_is_set(ADCSRA,ADSC)); // measuring
 
  uint8_t low  = ADCL; // must read ADCL first - it then locks ADCH  
  uint8_t high = ADCH; // unlocks both
 
//  long result = (high<<8) | low;
   long result = (long)(high<<8) | low;
 
  result = 1125300L / result; // Calculate Vcc (in mV); 1125300 = 1.1*1023*1000
  return result; // Vcc in millivolts
//  return 1125300L / ADC;
}

return the value -1

EDIT: when I do analogRead(A0); it is fine the function works. When I do analogRead(A14); the read vcc function then returns -1

The value of result.complete should be 0x0000AA55 because these AVR chips are little-endian, and all 4 bytes were initialized to zero at declaration. Is my assumption that all AVRs are little-endian because the 328 on my UNO is little-endian?

arduinoPi:
I made this code here ... it works for the first time, but once the code is executed again the readVcc() returns 0

How can you possibly know that? You never print voltage. Post the actual code you are using.

arduinoPi:
I have found that using analogRead(); makes the ... return the value -1.

EDIT: when I do analogRead(A0); it is fine the function works. When I do analogRead(A14); the read vcc function then returns -1

I notice there are no prints and no call to analogRead in the code you posted. Post the actual code you are using.

@arduinoPi appears to have wandered off so back to our off-topic discussion...

Sembazuru:
The value of result.complete should be 0x0000AA55 because these AVR chips are little-endian, and all 4 bytes were initialized to zero at declaration. Is my assumption that all AVRs are little-endian because the 328 on my UNO is little-endian?

Yes. However, you confirmed that AVR processors + AVR GCC are essentially little-endian so this assumption will not lead to any problems.

One more assumption. Want to guess?

Actually, with my 328 UNO it isn't an assumption. I verified empirically that even if it isn't little-endian, AVR GCC makes it feel little-endian at the source code level for both unions and manually creating an array of pointers. Thus this code snippet, if compiled into a sketch framework

  unsigned int integerValue = 0xD5AA;
  byte* integerPointer;
  integerPointer = (byte*)&integerValue;
  Serial.print(integerPointer[1],HEX);

will output D5 to whatever is listening to the UART (for example, the SerialMonitor). I will concede that I assumed that all the chips of the AVR family share basic framework characteristics like endianness. But this feels like such a safe assumption that I take it as wrote until there is a problem.

One more assumption. Want to guess?

Hmmm... not much left to guess with. It might have to do with "ADCL" and "ADCH". I'm not familiar with dealing with these AVR chips at the register level. I was just parroting @arduinoPi's usage that (s)he said came from an example found on the Great Internettm. Because we all know that only true things are allowed on the Great Internettm it had to be correct code. :wink: So, I don't know if this is the assumption that you meant, but I was assuming that the source where @arduinoPi got the register manipulation parts was accurate. Also partly because that bit hadn't been corrected by anyone on this forum (other than your extremely short shortcut).

Hey, was that it? I assumed the direct registry manipulation was valid because no-one else on this forum had (yet to) correct it? Do I trust you guys too much?

What else... Returning a long? But @arduinoPi's first code posting had everything he posted declared as part of "long readVcc()". I wasn't suggesting changes to that part so I didn't include it in my code samples... Though I would probably still advocate using unsigned long instead of signed long because the expected scope of the result should never be less than zero.

In my first suggestion I was using "word" as a union part name (erm... not exactly sure what the correct terminology is here, hopefully my meaning is clear even if the terminology is incorrect). I had forgotten at that point that "word" is Arduinoese for "unsigned int". That might have caused problems. Probably should have used "words" like how I was using "bytes" in my later examples. But wouldn't that be an actual mistake instead of an assumption?

I'm running out of parts of my code sample to tear apart... But as you can see it isn't for a lack of trying. I'm just not seeing it.

(I have this deep, uneasy feeling that once you point the last assumption I will again smack my self in the head. I've developed quite the flat forehead over the years. Something to do with forests and trees. I do like a nice pretty forest. ;))

Sembazuru:
It might have to do with "ADCL" and "ADCH".

Good guess.

  uint8_t low  = ADCL; // must read ADCL first - it then locks ADCH  
  uint8_t high = ADCH; // unlocks both

ADCL has to be read first. You (and several other folks) assume that it will be. However, the compiler is free to reorder those two statements. Even if it works now, a small change to readVcc could cause the compiler to reorder the statements. A future version of the compiler may reorder the statements. Changing a compiler switch could cause the compiler to reorder the statements.

In other words, two years from now, after readVcc has proven to be very reliable, when you upgrade to a newer compiler, readVcc could stop working. Imagine trying to debug that problem.

You can either include a "memory barrier" to force the compiler's hand or you can use ADC to read the value.