fix for ds18b20 negative decimals

[peturdainn]

Fixes ds18b20 negative decimals bug by changing the way temp and temp_dec are calculated, taking care not to lose the sign of the decimals part

Fixes #2313

• This PR is for the dev branch rather than for master.
• This PR is compliant with the other contributing guidelines as well (if not, please describe why).
• I have thoroughly tested my contribution.

ds18b20 decimals do not take into account the sign bit. Since the original calculation was not so readable, rewritten in readable way that also fixes the bug. Same code as PR against master.

[marcelstoer]

@fetchbot you're the original author of this module, could you please verify this fix?

[TerryE]

I've just taken a look at this code and some of the logic here is plain bizarre. I am talking about the original module, though @peturdainn's mods don't help.

• First, I would have expected some conditional logic based on LUA_NUMBER_INTEGRAL. The CB should only return a single value for floating point builds.
• In the case of integer build, yes it might make sense to return an integral part and a decimal fraction part, but what these are is not well defined, and this confusion lies at the heart of this issue / PR.
• whether or not, the build is integral, internally within the module the calculations should be done in floating point.

So taking an example what should a reading of - 1.4°C return on integer builds?

• The number of decimal places isn't explicitly defined or documented. Should this be API configurable, or fixed at 3? Let's assume 3 for the following.
• -2, 600 - valid and the maths convention but not easy to use for Lua programmers.
• -1, 400 - definitely not because 1000*(-1)+400 is the wrong answer.
• -1, - 400 - this is the one that makes the most sense to me. This is also easy to use in printing so long as you use maths.abs() on the decimal component.

This PR doesn't do this so -1 from me

[peturdainn]

I don't see how 1, -400 is a good representation for -1.4

The patch makes it return -1, -400 so you can do something like
t * 10 + tdec / 100
to generate a value in 0.1 units

before the patch it would return -1, 600

[TerryE]

@peturdainn Petur if you are going to start fixing things then why no do this properly? For example:

• Basic 101 stuff like variables are local in scope to one function (e.g. ds18b20_device_scratchpad_* should be stack-allocated and not static.

• In make absolutely no sense IMO to return 1.5, 500 on a floating point build, as the temperature is not 1000*temp+temp_dec)/1000. OK, we can't drop the TEMP_DEC parameter because of the sensor parasitic flag following and this would break backwards compatibility, but we should set it to zero on FP builds.

• Your evaluation isn't KISS. In the case of -1.125 or 0xFFEE this should return either -1.125, 0 or -1,-125 depending on build type so how about

lua_pushnumber(L, ((lua_Number)ds18b20_raw_temp) / 16);
#ifdef LUA_NUMBER_INTEGRAL
lua_pushinteger(L, ((ds18b20_raw_temp&0xf)*1000)/16 - (ds18b20_raw_temp>0 ? 0: 1000));
#else
lua_pushinteger(L, 0);
#endif

though why the OP needed the ds18b20_ prefix for a local variable declared a few line above loses me. :)

And either way can you update the ds18b20.md file to explain the *1000 on the TEMP_DEC part.

[peturdainn]

I am not familiar with the nodemcu codebase (or much of lua), I just happened to stumble over this bug and offered something that fixed just that.

I don't have the time right now to go fix the other problems in this module (just because I offered this fix).

[TerryE]

@peturdainn,

I don't have the time right now to go fix the other problems in this module (just because I offered this fix)

That's life, I guess, but what your PR is doing is to fix only a part of the problem, the other parts being the lack of complete integer vs. float support and the confusion in the documentation.

Is it worth a PR so solve 33% of the related issues. My feeling is -½, but I will defer to the other committers. @pjsg what are your thoughts, since you are tracking this?

[peturdainn]

The doc will need edit anyway, without the fix the user should be warned that negative temps are wrong and particularly bad around 0 when trying to calibrate sensors with ice water, temps between 0 and -1 are reported positive.

I understand you want the code perfect, but it pushes away incremental patch contributors. I'm a big fan of Pieter Hintjens in that regard.

[marcelstoer]

Is it worth a PR so solve 33% of the related issues...I will defer to the other committers.

My answer is clear; I'm quite pragmatic. Any PR that solves a problem, or even a fraction thereof, is worth merging if

a) it is in line with our guidelines
b) we can be reasonably certain that it doesn't introduce new problems

83% complete is better than 82%, isn't it? I see no reason why we would want to miss out on that 1%. Any developer who devotes time and energy to get from 82 to 83 does a lot more for our project than all the others who couldn't be bothered.

[peturdainn]

Thanks Marcel, you're actually motivating me to work on this in a later time-frame

[TerryE]

As I said I will defer to the other committers, but once committed I will then raise a new issue to cover the other outstanding items so that they are logged and not forgotten.

[marcelstoer]

Thanks Marcel, you're actually motivating me to work on this in a later time-frame

I can't deny this being part of the motivation behind my comment 😜 You're welcome around here anytime.

[marcelstoer]

@pjsg / @devsaurus you've also been involved in the discussions on the issue. Would you be willing to approve this?

[TerryE]

@marcelstoer the fix does fix the bug. I can confirm this. The issue isn't of approval, but of policy: should we accept PR which makes a module less flawed rather then fixes all know related issues?

I suggest that we decouple the policy discussion from this point issue.