Wrong negative readings from printf()

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

I am using a DS18B20 temperature sensor, in external power mode with proper pullup. I get proper readings on PUTTY when the temperature is >0. When the temperature is < 0, as in a deep freezer (-21C), I get the readings a bit off. I think the raw temperature readings are proper, but the conversion by printf() may be an issue. Attached are the relevant code snippets from my file ds18b20.c and main.c. Also attached are the readings I see on the PUTTY. For reference, I have both, the raw temperature in hex and the printf() values.

I am not doing any two's complement conversion for negative readings, but leaving this to the printf() function. So the two's complement conversion is commented out.

Please assist and let me know where this is going wrong.

Once again I reiterate, all functions are proper, the DS18B20 is properly working, since the positive readings are proper.

 

ds18b20.c code snippet relevant to the problem

double ds18b20_gettemp()		// get temperature
{
	uint8_t temperature_l;
	uint8_t temperature_h;
	int16_t temperature;

	ds18b20_reset(); //reset
	ds18b20_writebyte(DS18B20_CMD_SKIPROM);		//skip ROM
	ds18b20_writebyte(DS18B20_CMD_CONVERTTEMP); //start temperature conversion

	while(!ds18b20_readbit());		//wait until conversion is complete

	ds18b20_reset(); //reset
	ds18b20_writebyte(DS18B20_CMD_SKIPROM);		//skip ROM
	ds18b20_writebyte(DS18B20_CMD_RSCRATCHPAD); //read scratchpad

	//read 2 byte from scratchpad
	temperature_l = ds18b20_readbyte();
	temperature_h = ds18b20_readbyte();
	ds18b20_reset(); //reset

//	temperature = ( ( temperature_h << 8 ) + temperature_l );
	temperature = ( ( temperature_h << 8 ) | temperature_l );

/*	//check for -ve temperature readings
	//temperature is a signed 16 bit int, bit 15 =1, so ANDing with 8000h will always be !0
	if(temperature_h & 0x80)
	{
		temperature = ~temperature + 1;	//convert using two's complement
	//	temperature = ~temperature;
	}
*/
	rawtemp = (double) temperature;		//for debugging
	return rawtemp;

	//convert the 12 bit value obtained
//	retd = ( ( temperature_h << 8 ) + temperature_l ) * 0.0625;
//	retd = (temperature *625)/10000;
//	return retd;
}

main.c code snippet relevant to the problem

while(1)
	{
		rawtemp = ds18b20_gettemp();	// function returns double temp
		printf("%.4X\n", rawtemp);
		ctemp = rawtemp/16.0;
		printf("Temp = %.2f C \n", ctemp);

		_delay_ms(5000);
	}

Below is the output of the PUITTY terminal. only a section of the positive and negative readings are shown, since there are many. 

 

 

 

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

amorawala wrote:

	rawtemp = ds18b20_gettemp();	// function returns double temp
	printf("%.4X\n", rawtemp);

The %X in printf expects an argument of unsigend int. You are giving it a double. This won't do what you want it to do. [EDIT assuming rawtemp is defined as double, which you haven't shown so i was just guessing. If you have defined rawtemp as int16_t then it will work, but with unnecessary conversions betwen integer to double and back again. The hex values in the putty output you have shown look consistent with the temperature value, for both positive and negative]

I'm not sure in fact what you want it to do.

i would probably return rawtemp as a uint16_t.

    uint16_t rawtemp = ds18b20_get_temp();
    printf("%.4X\n", rawtemp);
    /* rawtemp actually represents a signed 16 bit number in 2's complement,
     * in units of 1/16 degree C */
    int16_t signed_temp = (int16_t)rawtemp;  // cast not necessary, added for clarity
    double ctemp = signed_temp / 16.0;

I mentioned in a similar thread recently about the 'legaility' of converting uint16_t (which you know in this case represents a signed value in 2's complement) to int16_t

int16_t signed_temp = rawtemp;

In summary, in practice it is highly unlikely to not work as expected.

 

Last Edited: Sat. May 1, 2021 - 05:14 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

The %X in printf expects an argument of unsigend int. You are giving it a double. This won't do what you want it to do. [EDIT assuming rawtemp is defined as double, which you haven't shown so i was just guessing. If you have defined rawtemp as int16_t then it will work, but with unnecessary conversions between integer to double and back again. The hex values in the putty output you have shown look consistent with the temperature value, for both positive and negative]

I'm not sure in fact what you want it to do.

i would probably return rawtemp as a uint16_t.

    uint16_t rawtemp = ds18b20_get_temp();
    printf("%.4X\n", rawtemp);
    /* rawtemp actually represents a signed 16 bit number in 2's complement,
     * in units of 1/16 degree C */
    int16_t signed_temp = (int16_t)rawtemp;  // cast not necessary, added for clarity
    double ctemp = signed_temp / 16.0;

I mentioned in a similar thread recently about the 'legaility' of converting uint16_t (which you know in this case represents a signed value in 2's complement) to int16_t

int16_t signed_temp = rawtemp;

In summary, in practice it is highly unlikely to not work as expected.

 

I regret I did not show the variable types. I am confused here.

In ds18b20.c file it is defined as double rawtemp; Then we have the lines: 

rawtemp = (double) temperature;		//for debugging
	return rawtemp;

In main.c it is defined as int16_t rawtemp; double ctemp; I understand that if printf() requires %X to be unsigned, I am giving it a signed int (int16_t rawtemp). But it prints out the hex values properly as can be seen. 

You say that "The hex values in the putty output you have shown look consistent with the temperature value, for both positive and negative]". However, on calculating, the decimal values are a bit off. the integral value is proper, but the decimal value is not. I may have made some mistake, can you guide me please? What is the real issue here and how to overcome this even in future? 

Just on the side, what do you mean by " in practice it is highly unlikely to not work as expected"? Do you mean that it will work?

 

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

OK, I think what you are doing is, in gettemp() you have rawtemp initially as an int16_t, but then that value is converted to double becuase you have defined gettemp to return double.

If you then assign that returned value to an int16_t like

int16_t temp = gettemp();

it will work, that double is converted back to int16_t. But you now have completely unnecessary conversion from int16_t to double and then back to int16_t.

You may as well just have getemp return int16_t.

 

Taking an example of one of the negative values from your original post.

FED4 gives -18.75

FED4 as a uint16_t is 65236, as an int16_t it is -300.

So -300 / 16 = -18.75.

So looks correct.

amorawala wrote:

Just on the side, what do you mean by " in practice it is highly unlikely to not work as expected"? Do you mean that it will work?

Yes, it will work.

This remark is spefiically about converting a value to a signed type.

Taking the example from just above.

Let's say you have a uint16_t with value 65236 (0xFED4).

If you assign that value to a int16_t, question is what happens, since the value 65236 is outside the range of values for an int16_t (-32768 to 32767).

In practice what happens is the bit patern 0xFED4 is unchanged, it is simply now going to be viewed as a signed 16 bit value, which has value -300.

In this case, this is exactly what you want.

I say 'in practice what happens' because this behaviour is not absolutely guaranteed by the C standard (it is referred to as implementation defined), so in theory a compiler is allowed to do something different, but in reality it won't.

 

EDIT

Forgot to mention

"...I understand that if printf() requires %X to be unsigned, I am giving it a signed int (int16_t rawtemp). But it prints out the hex values properly as can be seen. "

 

That is OK, giving it signed int will work. i thought at first you were giving it a double, that wouldn't work.

 

 

Last Edited: Sun. May 2, 2021 - 08:19 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

MrKendo wrote:

OK, I think what you are doing is, in gettemp() you have rawtemp initially as an int16_t, but then that value is converted to double becuase you have defined gettemp to return double.

If you then assign that returned value to an int16_t like

int16_t temp = gettemp();

it will work, that double is converted back to int16_t. But you now have completely unnecessary conversion from int16_t to double and then back to int16_t.

You may as well just have getemp return int16_t.

 

Taking an example of one of the negative values from your original post.

FED4 gives -18.75

FED4 as a uint16_t is 65236, as an int16_t it is -300.

So -300 / 16 = -18.75.

So looks correct.

 

amorawala wrote:

 

Just on the side, what do you mean by " in practice it is highly unlikely to not work as expected"? Do you mean that it will work?

Yes, it will work.

This remark is spefiically about converting a value to a signed type.

Taking the example from just above.

Let's say you have a uint16_t with value 65236 (0xFED4).

If you assign that value to a int16_t, question is what happens, since the value 65236 is outside the range of values for an int16_t (-32768 to 32767).

In practice what happens is the bit patern 0xFED4 is unchanged, it is simply now going to be viewed as a signed 16 bit value, which has value -300.

In this case, this is exactly what you want.

I say 'in practice what happens' because this behaviour is not absolutely guaranteed by the C standard (it is referred to as implementation defined), so in theory a compiler is allowed to do something different, but in reality it won't.

 

EDIT

Forgot to mention

"...I understand that if printf() requires %X to be unsigned, I am giving it a signed int (int16_t rawtemp). But it prints out the hex values properly as can be seen. "

 

That is OK, giving it signed int will work. i thought at first you were giving it a double, that wouldn't work.

 

 

Thanks MrKendo, for the detailed explanation. I have edited rawtemp to be int16 now. I will get back to you as soon as I test and confirm the negative readings.

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

As always; it can be instructive to look at the implementation in Arduino Libraries:

In https://github.com/matmunk/DS18B20/blob/master/src/DS18B20.cpp I find:

 uint8_t sign = msb & 0x80;
 int16_t temp = (msb << 8) + lsb;

    if (sign) {
        temp = ((temp ^ 0xffff) + 1) * -1;
    }

    return temp / 16.0;

This differs slightly from your code.

 

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

 

 

Hello,

should run with signed integer 16 bit. Convert to float and then result /= 16;

 

example 

-55 °C

register 0xFC90 as signed Integer = -880 (dez). / 16 = -55

 

 

 

 

here datasheet:

 

 

 

Senior Electric Engineer
--------------------------
I use Atmega128, AS7, GNU GCC, Atmel ICE

Last Edited: Sun. May 2, 2021 - 10:12 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

N.Winterbottom wrote:

As always; it can be instructive to look at the implementation in Arduino Libraries:

This is instructive of don't trust everything on the internet :)

 int16_t temp = (msb << 8) + lsb;

msb << 8 invokes undefined behaviour of signed left shift if msb >= 0x80 (assuming 16 bit int and if msb is defined as uint8_t. And I'm referencing C language, not sure if C++ has exactly the same rules in this regard).

In practice it will work though.

 

    if (sign) {
        temp = ((temp ^ 0xffff) + 1) * -1;
    }

this is entirely pointless is it not?

Having said that, the rest of the code could be very useful for figuring out how to use the sensor.

 

I would just go with


    uint16_t rawtemp = ((uint16_t)msb << 8) | lsb;
    int16_t temp = (int16_t)rawtemp;

If concerned about the implementation defined behaviour possibly doing something different one day, you could do

int16_t temp = safe_u16_to_s16(rawtemp);

with various different ways of implementing

static inline int16_t safe_u16_to_s16(uint16_t x)

most of which will probably be optimised away by compiler.

Or you could put the msb and lsb into a union of uint8_t[2] and int16_t  if you don't mind having dependency on endianess.

Or you could memcpy the msb and lsb into a int16_t again if you don't mind having dependency on endianess.

But, honestly, i wouldn't bother for a hobby project.

 

 

 

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

MrKendo wrote:

I would just go with

    uint16_t rawtemp = ((uint16_t)msb << 8) | lsb;
    int16_t temp = (int16_t)rawtemp;

+1

 

Note that the temperature will be (rawtemp / 256.0) regardless of the number of resolution bits.

 

David.

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

MrKendo wrote:

Or you could put the msb and lsb into a union of uint8_t[2] and int16_t  if you don't mind having dependency on endianess.

An example would be like

    uint8_t msb = ...;
    uint8_t lsb = ...;
    union
    {
        uint8_t b[2];
        int16_t t;
    } u;
    /* assuming Little Endian */
    u.b[0] = lsb;
    u.b[1] = msb;
    int16_t temp = u.t;

Which way is best?

Depends how fussed you are about writing code that is completely portable across lots of different processors and compilers,  how pedantic you want to be over things that are not absolutely guaranteed by C standard and so theoretically could change but probably won't if you change compiler etc.

Lots of different ways it can be done :)

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

Go on.   The datasheet shows the format and shows which byte is msb and lsb.    You assemble the 16-bit value as shown in the datasheet.

 

As a generality,   peripheral registers, functions,  are uint8_t  which makes it easy to assemble a uint16_t

 

Then you convert it to a human or program friendly value.   e.g. f-p temperature or (scaled) signed integer.

 

I can understand someone attempting to save cycles in a guided missile e.g. unions, casting, ...

However ((msb << 8) | lsb) tends to be pretty efficient with any modern compiler.

And the One-Wire functions are incredibly slow.   Subsequent shaving a cycle or two at the expense of portability seems reckless.

 

David.

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

MrKendo wrote:

If concerned about the implementation defined behaviour possibly doing something different one day, you could do

int16_t temp = safe_u16_to_s16(rawtemp);

with various different ways of implementing

static inline int16_t safe_u16_to_s16(uint16_t x)

most of which will probably be optimised away by compiler.

Now you got me started on this :)

Here are 5 different ways (not all tested) i just knocked up. Since the only reason to do this would be extreme paranoia about what exactly is guaranteed by C standard, you have to be careful here not to simply substitute one case of implementation defined behaviour for another. I can't guarantee i haven't done that!

You can then decide if it really seems worth it comapred to simply

int16_t temp = (int16_t)rawtemp;

I have my doubts.

 

The first 3 ways think in terms of the value, so ensure at no stage do you assign a value to the int16_t that is outside the range it can hold. The result is then absolutely guaranteed.

The last 2 ways think in terms of ensuring the compiler can't change the bit pattern.

In a quick test, all 5 ways appeared to be completely optimised away by compiler.

Remember what this is about, it is for when you have a uint16_t value but you know that value is actually representing a signed 16 bit value in 2's complement.

About the only time that is likely is where the uint16_t has come from 'outside somewhere'.

 

static inline int16_t safe_u16_to_s16(uint16_t u)
{
    int16_t s;

    if (u <= INT16_MAX)
    {
        s = u; // value in range 0 to 32767, fits in signed
    }
    else
    {
        s = u - INT16_MAX - 1; // result in range 0 to 32767, fits in signed
        s = s - INT16_MAX - 1; // result now in range -32768 to -1
    }
    return s;
}

static inline int16_t safe_u16_to_s16(uint16_t u)
{
    int16_t s;

    if (u <= INT16_MAX)
    {
        s = u; // value in range 0 to 32767, fits in signed
    }
    else
    {
        s = (int16_t)(uint16_t)~u; // result in range 32767 to 0, fits in signed
        s = -s - 1; // result in range -32768 to -1
    }
    return s;
}

static inline int16_t safe_u16_to_s16(uint16_t u)
{
    int16_t s;

    if (u <= INT16_MAX)
    {
        s = u; // value in range 0 to 32767, fits in signed
    }
    else
    {
        s = (int32_t)u - 65536; // result in range -32768 to -1
    }
    return s;
}

static inline int16_t safe_u16_to_s16(uint16_t x)
{
    union
    {
        uint16_t u;
        int16_t  s;
    } us;
    us.u = x;
    return us.s;
}

static inline int16_t safe_u16_to_s16(uint16_t x)
{
    int16_t s;
    memcpy(&s, &x, sizeof(s));
    return s;
}

 

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

I'm not sure it is realized that the values you are getting are in 1/16 of a C (0.0625). That should simplify everything.

 

int16_t ds18b20_readTemp () { return ds18b20_readbyte()<<8 | ds18b20_readbyte(); }
 

int16_t tempRaw = ds18b20_readTemp();

 

printf("raw: 0x%04X,%d  Cx100: %d  C: %d.%02u\r\n",

        tempRaw, tempRaw, tempRaw*100/16, tempRaw/16, (tempRaw&15)*100/16);

 

 

 

https://godbolt.org/z/39o6qP6Ka

 

 

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

curtvm wrote:

I'm not sure it is realized that the values you are getting are in 1/16 of a C (0.0625). That should simplify everything.

That is realized, OP is using floating point to do the divide by 16

amorawala wrote:

	ctemp = rawtemp/16.0;
	printf("Temp = %.2f C \n", ctemp);

where rawtemp is int16_t and ctemp is double.

The argument has been mainly about getting that int16_t value in the first place.

curtvm wrote:
(tempRaw&15)*100/16)

Using this for the 2 decimal places, have to watch out for signed negative values, works if the fraction is 0.5 but not generally correct

Last Edited: Mon. May 3, 2021 - 08:16 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Hello  amorawala ,

 

I do it in this way:

 

int16_t  buf = THbyte << 8;
            buf += TLbyte;
float     _temp = (float)buf;
           _temp /= 16;

 

printing with format provider:

 

"T= %4.1f"

 

In AVR GCC you have add a toolchain option to the linker options: -lprintf_flt

 

 

 

Senior Electric Engineer
--------------------------
I use Atmega128, AS7, GNU GCC, Atmel ICE

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

>Using this for the 2 decimal places, have to watch out for signed negative values, works if the fraction is 0.5 but not generally correct

 

Picking -25.5 for the negative example was a bad choice that hid the problem although had I realized earlier it would show up- saw the right answer and then ignored the problem. Also note that using a %100 if starting with a x100 value would also show the same problem on negative numbers so would also require getting to an absolute value first. Sorry for the bad info.

 

printf("raw: 0x%04X,%d  Cx100: %d  C: %d.%02u\r\n",

        tempRaw, tempRaw, tempRaw*100/16, tempRaw/16, (__builtin_abs(tempRaw)&15)*100/16;

 

I guess my point was that you only have (up to) 16 possible values in the fraction in this case, so no need to deal in floating point. The only area that presents a problem is printing, but can be split up as shown although it should be done correctly when you get to negative temps.

 

I am using a tmp117 and si7051 for temperature and double checked if I am doing negative correctly- in those I am dealing with a Fx10 value when I get to printing and doing the same type of printf, and for the decimal I am doing '(f < 0) ? -f%10 : f%10', although using __builtin_abs(f)%10 would do the same thing.