thefekete / LM75

Arduino library for the LM75 I2C temperature sensor
GNU General Public License v3.0
10 stars 11 forks source link

Negative temperature values calculated incorrectly #2

Closed cklam2 closed 9 years ago

cklam2 commented 9 years ago

Hi,

First of all thank you very much for this library.

I think the library shows incorrect temperature when the temp is negative. According the LM75 datasheets the most significant bit (D15) contains 1 when value is negative.

In your case the temp value should be subtracted by 0x100 to get the right (negative) value.

float LM75::regdata2floattemp (word regdata)
{
  float temp = ((float)(int)regdata / 32) / 8;
  if (regdata >> 15) temp -= 0x100;
  return temp;
}

Also think that D0..D6 (don't care bits) should be masked. Not sure though...

thefekete commented 9 years ago

Thanks! Unfortunately, I'm unable to test this for a while... Have you tested it? If so I'll patch..

cklam2 commented 9 years ago

Don't know what to say.. but I tested your code with my Arduino and your library does calculate negative values correctly. I only have no idea why.

This is your code:

float LM75::regdata2float (word regdata)
{
  return ((float)(int)regdata / 32) / 8;
}

When passing regdata = 0xC920 then it outputs -54.88 C That's correct but how can that be???

Anyway, no changes required. Sorry :(

thefekete commented 9 years ago

Hey no worries.. It's been 4 years since I wrote this and 2 since I last looked at it.. And all I can say is this belongs on a slide in a presentation on how NOT to write code! It took me 10 minutes of staring at it before I figured it out too! I'll try to explain it for both of us ;)

First of all, let me re-write regdata2float so it shows the intention a little better. It also helps to look at float2regdata, since it's just doing the inverse of that function (although maybe not, that one's pretty bad too :)

word LM75::float2regdata (float temp)
{
  // First multiply by 8 and coerce to integer to get +/- whole numbers
  // Then coerce to word and bitshift 5 to fill out MSB
  return (word)((int)(temp * 8) << 5);
}

float LM75::regdata2float (uint16_t regdata)
{
    /* regdata comes in as a 16bit word that has a bunch of 'don't care bits' in the
     * 7 LSBs. It's basically a signed integer representing +/- degrees in the upper
     * byte and 256ths in the lower byte. So first it casts regdata as an (signed)
     * integer, converting it to +/-. Then to a float, allowing us to divide and get the
     * decimals. After that, it's just a matter of "shifting" the lower byte to the other
     * side of the decimal point by dividing by 256 (not sure why I divided by 32 then
     * 8, but same result).
     */ 
    return ( (float)( (int16_t)regdata)) / 256;
}

Of course this is super convoluted and very hard to follow code... But I think the important parts are casting to an int gives you the +/- and then dividing by 256 "shifts" it down so that the units column is whole degrees. It might be possible to right shift by 8 instead, but I'm not sure how that works for floats..

Anyways, not sure if you wanted an explaination, but I figured I'd write one for you as much as myself if I ever need to use this again ;)

Hope that helps and thanks for asking the question, I know I learned something!

cklam2 commented 9 years ago

Thanks for the explanation.

I also tried to figure out how it works. What I didn't know was that on Arduino an int is a 16-bit value. I've been working on 32-bit systems where int was always 32-bit. By converting a word to an int would mark the value negative if the MSB is 1.

Reason who you divide by 32 and then by 8 is because you first eliminate the 5 don't care LSB's and then divide by 8 as the lowest temp resolution is 1/8th = 0.125C

vanbwodonk commented 5 years ago

Hi, i used this library with STM32duino. This library works perfectly, except can't show negative value. After some research i know it because casts to (int) variable in AVR/Arduino use 16 bit. But int the ARM it use 32 bit. I need to changed into int16_t and this patch works under ARM and AVR/Arduino. Need some little patch this code:

word LM75::float2regdata (float temp)
{
  // First multiply by 8 and coerce to integer to get +/- whole numbers
  // Then coerce to word and bitshift 5 to fill out MSB
  return (word)((int)(temp * 8) << 5);
}

float LM75::regdata2float (word regdata)
{
  return ((float)(int)regdata / 32) / 8;
}

to this,

uint16_t LM75::float2regdata (float temp)
{
  // First multiply by 8 and coerce to integer to get +/- whole numbers
  // Then coerce to word and bitshift 5 to fill out MSB
  return (uint16_t)((int16_t)(temp * 8) << 5);
}

float LM75::regdata2float (word regdata)
{
  return ((float)(int16_t)regdata / 32) / 8;
}
geresy commented 3 years ago

Indeed on SAMD the values rollover when the temperature is negative. Changing to uint16_t as exemplified above has fixed the problem.