timofurrer / w1thermsensor

A Python package and CLI tool to work with w1 temperature sensors like DS1822, DS18S20 & DS18B20 on the Raspberry Pi, Beagle Bone and other devices.
MIT License
493 stars 113 forks source link

Error handle 85 centigrades for DS18B20 #46

Closed el-hult closed 5 years ago

el-hult commented 6 years ago

Background

This code does not take the "reset" code 85 centigrades into account. Thus - the code claims that the sensor says 85 degrees, even though it is an error code for DS18B20

That information is stated in the rasperry pi forums and also in the data sheet.

Needed solution

Add another guard in the raw_data section so it raises the SensorNotReadyError if the readout is exactly 85000 millicentigrades, and not only handle CRC failures as is now.

https://github.com/timofurrer/w1thermsensor/blob/54d8ba8654a003e4c4624db5a0c99d81360d13e6/w1thermsensor/core.py#L193-L195

This check should possibly only be done for the model DS18B20. I have limited knowledge about the other supported variants.

timofurrer commented 6 years ago

I actually didn't know about this. Thanks for pointing it out 👍

You've already proposed a solution - would you be up for a PR?

el-hult commented 6 years ago

Have not done that before, but would love to. It'll take me some week before I have the time to verify it all on my PRi on correct hardware and so on, so hang on. But coding up the suggested fix was quick.

Untested change suggestion: https://github.com/timofurrer/w1thermsensor/compare/master...dirrelito:patch-reset-val

timofurrer commented 6 years ago

Have not done that before, but would love to.

Awesome! so let's do that! :tada:

Take your time!

el-hult commented 6 years ago

Okay. I managed to reproduce bug on my hardware. This is done by doing the wiring as on page 9 in this presentation and then unplugging the red wire of the sensor from the breadboard. I think that is the V_DD wire. In that setup no errors are thrown, but the temperature readout is always 85.000 centigrades when using the w1thermsensor library.

I hope to run the test cases on my pi this weekend. :)

timofurrer commented 6 years ago

Sounds good - keep me posted :+1:

timofurrer commented 6 years ago

@dirrelito Are you still working on this? Your patch looked pretty promising!

procule commented 3 years ago

This change is incorrectly implemented:

First, the "85 C" case is already taken into account in the w1-therm kernel driver on line 1131 here returning IOError: https://github.com/torvalds/linux/blob/master/drivers/w1/slaves/w1_therm.c#L1128-L1139

Second, reading "85.0 C" is a necessary but not sufficient condition to be considered a "power on reset" event. As you can see in the w1-therm driver code, the read temperature must indeed be 85 C (0x05, 0x55 for the two first LSB) but the 6th byte must also be 0x0C.

So, only reading "85 C" like done here creates false "power on reset" event: https://github.com/timofurrer/w1thermsensor/blob/master/src/w1thermsensor/core.py#L408-L416

I went into that situation while monitoring hot water and getting the ResetValueError while getting around 85 C.