bug: sensor (DS18S20) is considered disconnected when reporting the lowest temperature (-55 deg C)

[bonnyr]

When a sensor is reporting -55°C, the library treats the sensor as disconnected and reports temperature readings as -127°C.

The issue appears to lie with the library since it defines the following (in DallasTemperature.h):
#define DEVICE_DISCONNECTED_RAW -7040

And has the following to say about that value in the implementation (DallasTemperature.cpp):

// returns temperature in 1/128 degrees C or DEVICE_DISCONNECTED_RAW if the
// device's scratch pad cannot be read successfully.
// the numeric value of DEVICE_DISCONNECTED_RAW is defined in
// DallasTemperature.h. It is a large negative number outside the
// operating range of the device
int32_t DallasTemperature::getTemp(const uint8_t* deviceAddress) {

	ScratchPad scratchPad;
	if (isConnected(deviceAddress, scratchPad))
		return calculateTemperature(deviceAddress, scratchPad);
	return DEVICE_DISCONNECTED_RAW;

However, -55 deg C is exactly -7040 (if you multiply it by 128 which the code does all over)

Thus, when you request a reading the next time, the value matches a DEVICE_DISCONNECTED_RAW sensor when in reality it is not.

This was tested with the latest version in master.

[milesburton]

Hmm, this issue has come up before. We had to pick a magic number, one we hoped would be unlikely to be valid in the real world - unfortunately that appears not to be the case. On the upside, it's great to see people are using the library in such extremes! To the floor, what do we feel is the best way forward? Pick a new magic number or implement a flag?

…

On Mon, 6 Mar 2023 at 09:46, bonnyr ***@***.***> wrote: When a sensor is reporting -55°C, the library treats the sensor as disconnected and reports temperature readings as -127°C. The issue appears to lie with the library since it defines the following (in DallasTemperature.h): #define DEVICE_DISCONNECTED_RAW -7040 And has the following to say about that value in the implementation ( DallasTemperature.cpp): // returns temperature in 1/128 degrees C or DEVICE_DISCONNECTED_RAW if the // device's scratch pad cannot be read successfully. // the numeric value of DEVICE_DISCONNECTED_RAW is defined in // DallasTemperature.h. It is a large negative number outside the // operating range of the device int32_t DallasTemperature::getTemp(const uint8_t* deviceAddress) { ScratchPad scratchPad; if (isConnected(deviceAddress, scratchPad)) return calculateTemperature(deviceAddress, scratchPad); return DEVICE_DISCONNECTED_RAW; However, -55 deg C is exactly -7040 (if you multiply it by 128 which the code does all over) Thus, when you request a reading the next time, the value matches a *DEVICE_DISCONNECTED_RAW* sensor when in reality it is not. This was tested with the latest version in master. — Reply to this email directly, view it on GitHub <#236>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJGHMYFQLKU4VFLZ5LQPMLW2WW5RANCNFSM6AAAAAAVQ4EBHY> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.*** com>

[bonnyr]

I would argue that the temperature should be different than the connection state.

The API already had methods for checking if the device is connected, so it's a matter of using that I think.

Btw, the tests were conducted using a simulated sensor on a platform called wokwi. I've implemented the sensor in software.

[TimMathias]

Using a flag or an error code would be the preferred method.

If there is no other option but to use a magic number, what about -100°K (44774 scaled) because the lowest temperature in the Universe is 0°K or -273.15°C ?

Certainly shouldn't be using a magic number as an error code that is within the valid range of temperatures.

[RobTillaart]

Agree with @TimMathias that an error code would be preferred, other error codes can be supported too.

• TEMP_OUT_RANGE
• TIME_OUT
• ADDRESS_NOT_FOUND
• CRC_ERROR
• ALARM_LOW
• ALARM_HIGH

wrt magic numbers:
Practical problem is that the value -127 is used in many applications.
So I would propose a define that defaults to backwards compatibility and optional use the -100K for those that need the very LOW temperatures.

my 2 cents

[milesburton]

Practical problem is that the value -127 is used in many application

That's the big issue with such a widely used library. Last time we attempted to 'fix' this issue it caused havoc. As such I agree with you Rob, we'll need a solution which enables backwards compatibility.

…

On Tue, 7 Mar 2023 at 08:52, Rob Tillaart ***@***.***> wrote: Agree with @TimMathias <https://github.com/TimMathias> that an error code would be preferred, other error codes can be supported too. - TEMP_OUT_RANGE - TIME_OUT - ADDRESS_NOT_FOUND - CRC_ERROR - ALARM_LOW - ALARM_HIGH wrt magic numbers: Practical problem is that the value -127 is used in many applications. So I would propose a define that defaults to backwards compatibility and optional use the -100K for those that need the very LOW temperatures. my 2 cents — Reply to this email directly, view it on GitHub <#236 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJGHM2B7SKZVGWSQUL6G3TW23ZNPANCNFSM6AAAAAAVQ4EBHY> . You are receiving this because you commented.Message ID: <milesburton/Arduino-Temperature-Control-Library/issues/236/1457782248@ github.com>

[bonnyr]

As I said above, a flag, error code or other separate state representation is preferable.
I like the backward compatibility idea from @RobTillaart - have your cake and eat it too :)

What's the go wrt fixes here ? @milesburton - do you expect people to submit PRs?

[milesburton]

Yeah the best way forward is to suggest a change via a pr and then let the community come to a consensus, if it's green lit we can merge

…

On Tue, 7 Mar 2023, 9:55 am bonnyr, ***@***.***> wrote: As I said above, a flag, error code or other *separate* state representation is preferable. I like the backward compatibility idea from @RobTillaart <https://github.com/RobTillaart> - have your cake and eat it too :) What's the go wrt fixes here ? @milesburton <https://github.com/milesburton> - do you expect people to submit PRs? — Reply to this email directly, view it on GitHub <#236 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJGHM6HR22PYW2DZCXUIH3W24AZ7ANCNFSM6AAAAAAVQ4EBHY> . You are receiving this because you were mentioned.Message ID: <milesburton/Arduino-Temperature-Control-Library/issues/236/1457875217@ github.com>

[Andersama]

Vaguely remember this mess when we started using these sensors for the first time, in one sense it's pretty clever to have a valid reported range and a magic number for when things go wrong. It gets a bit frustrating though when you realize that a spotty soldering job reads like a freezing cold temperature.

I'm not impacted by this, but conceptually this seems similar to my timestamps pull request. EG: in addition to returning the temperature, have it also return some error code value. Have the magic numbers not exported the library so that people can't break their source code that way.

struct temperature_result_t {
   float value; // from what I read it seems reading from scratch pad only 2 bytes are used for temp, I don't remember
// the full-precision range of a float, but I'm pretty sure 32 bits covers an absurd possible range.
   uint32_t error_code;
   //uint8_t in_celcius_kelvin_or_farenheit; // I would assume the temp_result_t by default is in celcius
   
   float get_reading_in_celcius(); // 
   float get_reading_in_farenheit(); //
   float get_reading_in_kelvin(); //
   
   operator float() {
      return value;
   }
};
// could do safe math shenanigans eg: return a celcius_result_t which must explicitly be converted to farenheit_result_t
// etc...etc..., only allow celcius_result_t to have a default cast to float for backwards compatibility?

Edit: Writing a rough draft of a pull request, I apologize for the github commit notification spam. I don't know why github changed the behavior to show all the commits in mentioned threads, really ought to only display the commits in the pull request itself.