add ds18b20 module #2003

fetchbot · 2017-06-11T16:00:01Z

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.
The code changes are reflected in the documentation at docs/en/*.

DS18B20 module in C rather than Lua #1996, #1883, #1820.

marcelstoer · 2017-06-11T17:08:42Z

I guess you'll want to drop /lua_modules/ds18b20 with this as well?

fetchbot · 2017-06-11T17:15:14Z

If it covers @vsky279 and @TerryE use cases?

TerryE · 2017-06-11T19:37:28Z

TBH, I don't understand the desire to use C and write C libraries where Lua ones work fine. Lua developers can always tweak a Lua module if their needs aren't 100% aligned to the current module implementation -- and can do so without having to get into C or rebuild the firmware. In this case the core One-wire C library encapsulates everything that needs to be done in C. So why reimplement DS18B20 in C?

fetchbot · 2017-06-11T20:38:15Z

My main problem is just the lack of available memory if I'll run all my scripts in pure Lua, so I try to off-load the most to C.

joysfera · 2017-06-11T23:52:36Z

I've been told that almost nothing needs to be running in memory - most if not all code can be loaded on demand, like using dofile("ds18b20.lc"). The available NodeMCU documentation covers require() use only, though. No detailed word about dofile(), IIRC. I think it would be great if NodeMCU beginners could see some large-ish example programs that would show how to keep only the necessary minimum of Lua code in memory. Then the need for offloading Lua code to C modules would disappear, perhaps.

The DS18B20 is actually a good example of a code that might not be simple to keep out of memory: it needs one timer to wait those 750+ milliseconds between decoding and reading the temperature, and usually another timer to measure temperatures say every minute. How to write the dofile() or require() in a way that ensures the memory is indeed freed immediately and completely?

devyte · 2017-06-12T00:37:05Z

If the code is linear, as in just a single pass through and no functions, then that is true: nothing is kept in mem. If there are functions declared, then each of those gets loaded into memory first, then they get called whenever. I have no idea what happens to the loaded functions after the dofile returns. Also, is it possible to pass arguments to a dofile call? If not, then parameter passing must be through some global variable scheme, which is rather fragile. Finally, if a callback needs to be setup for something, which is quite common, then you'll probably need a closure of some kind. I found that in most cases that is not viable to do, unless the code loaded with dofile is itself an entire function that gets loaded. Of course, I'm not a lua expert, so please take my comments with a grain of salt, but that has been my experience.

…

On Jun 11, 2017 7:52 PM, "Petr Stehlík" ***@***.***> wrote: I've been told that almost nothing needs to be running in memory - most if not all code can be loaded on demand, like dofile("ds18b20.lc"). The available NodeMCU documentation covers 'require()' use only, though. No detailed word about 'dofile()', IIRC. I think it would be great if NodeMCU beginners could see some large-ish example programs that would show how to keep only the necessary minimum of Lua code in memory. Then the need for offloading Lua code to C modules would disappear, perhaps. The DS18B20 is actually a good example of a code that might not be simple to keep out of memory: it needs one timer to wait those 750+ milliseconds between decoding and reading the temperature, and usually another timer to measure temperatures say every minute. How to write the dofile() or require() in a way that ensures the memory is indeed freed immediately and completely? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#2003 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AQC6BkzvXnVT_pHLdp9mGtjDfGL5XarTks5sDH3LgaJpZM4N2byy> .

marcelstoer · 2017-06-14T14:45:24Z

@joysfera I think the dofile() vs. require() discussion could be fed into #1899 for @TerryE to be addressed in the FAQ update?

TerryE · 2017-06-14T17:45:17Z

I think the dofile() vs. require() discussion could be fed into #1899 for @TerryE to be addressed in the FAQ update?

@marcelstoer Marcel, thanks for the prompt. I am coming to a checkpoint in my Lua 5.3 work -- time to migrate into the Xtensa build environment -- so I will do this refresh first. 😄

nickandrew · 2017-06-16T07:42:47Z

app/modules/ds18b20.c

+	}
+
+	// no change to th and tl setting
+	ds18b20_device_conf[0] = 0x4B;


Can you put the magic numbers into #defines?

nickandrew · 2017-06-16T07:43:55Z

docs/en/modules/ds18b20.md

+- `RES` temperature resolution
+- `TEMP` temperature
+- `TEMP_DEC` temperature decimals for integer firmware
+- `PAR` sensor parasitc flag


nickandrew · 2017-06-16T07:45:00Z

docs/en/modules/ds18b20.md

+		end
+	end);
+
+-- print if temperature is greater or less then a defined value


marcelstoer · 2017-06-28T08:11:19Z

Can we have a quick voting among the maintainers as to whether to include this module? Terry already voted against it I understand.

TerryE · 2017-06-28T08:41:05Z

@marcelstoer, my vote is more of an abstain rather than a 👎 It's just that I personally don't see the point of doing this one in C and wouldn't personally use it, but I wouldn't wish to block its inclusion if other committers think that there is value in having it.

devsaurus · 2017-07-03T20:14:39Z

My experience with this sensor is that people use it in many different ways. I guess that neither a C nor a Lua module would cover all. I regarded the old Lua module more like a template to start from and adapt to one's needs. The C module has a more "official" character and need more support effort on the long run.
We have a couple of modules in C that would also work as plain Lua code, so there's no point in blocking this one IMO.

nickandrew · 2017-07-13T06:50:59Z

The argument against a C module is that it needs to be compiled-in. Arguments for are that it may use less memory, and be faster - which is good to ensure that any timing constraints on the 1wire bus are met. I believe the timing in this module is not critical (as it uses the onewire lib). I'd probably use this module in my devices.

All up I give it a weak thumbs-up; merge it and if problems are reported in future then reconsider.

marcelstoer · 2017-07-13T06:55:52Z

How about we merge both #1996 and this one but cross-reference them in the documentation (I could do that after the merge)?

nickandrew · 2017-07-13T07:03:14Z

I just had a look at #1996 but I don't have an opinion on its value, yet. I don't think it should hold up #2003.

TerryE · 2017-07-13T13:12:44Z

OK, I've had a look at this code.

I accept that this might be a good approach for @fetchbot 's specific requirements, but it is very functionally limited and I really don't see the point of doing this in C as it will run a lot slower than my or @vsky279 Lukas' Lua version. Why? and other points:

The read() function does a broadcast convert request and then uses an OW search to enumerate the OW address space (not even limiting it to DS18B20 device types). This is a slow binary-chop scan. Surely this only needs to be once or at least optional so you can do fast reads of specified thermometers?
The documentation is misleading as the read() returns nothing. Instead it executes the supplied cb once per DS18B20 found passing the return data as input parameters.
It makes a lot more sense to return the ROM1..ROM8 bytes as a single 8-byte string. Lua strings aren't the same as C strings and can be binary. You can always use the byte and substr functions to pick them apart. Ditto the input ROM array to setting().
The callback is called using a lua_call rather than a lua_pcall which means that any errors in the calback will panic the system (though to be fair, this is a common failing of most modules).

I still will register an abstain rather than a 👎 -- so long as we get resolution / consensus on the first 3 points.

@nickandrew Nick what are your thoughts? Am I being too pedantic?

nickandrew · 2017-08-03T04:57:24Z

@TerryE I think those are fair comments. My temp code treats the device IDs as a string or array rather than individual bytes (I also munge the byte order in a way that suits me). How much slower is the discovery algorithm than reading only a specified sensor? My temp code always does a select-all; convert; then goes through the algorithm to process all responses.

I need to spend more time looking at this 😬

TerryE · 2017-08-03T15:43:36Z

BTW,

I guess you'll want to drop /lua_modules/ds18b20 with this as well?

I for one will stick a Lua version. It's faster and easier to tailor.

As to the OW enumeration algo, it's pretty slow.

marcelstoer · 2017-08-16T19:35:56Z

I believe that sometimes making the wrong decision is "less bad" than not making a decision at all. I cross-linked the C/Lua module docs: http://nodemcu.readthedocs.io/en/latest/en/modules/ds18b20/

* add ds18b20 module * add intitial eeprom value definition * adjust read() function and address handling

Fisherman48 · 2018-01-23T12:45:59Z

Missing leading zero of decimal part?
Using integer firmware and concatenate temp and tdec strings ex:
..romhex.."="..temp..","..tdec.." " I get 6,62 when I expect 6,0625
Other examples with 3 ds18b20 and decreasing temp:
18,250 18,250 18,437
18,62 18,62 18,250
18,62 18,62 18,250
18,62 18,125 18,250
18,0 18,62 18,250
Help

devsaurus · 2018-01-24T21:17:24Z

..romhex.."="..temp..","..tdec.." " I get 6,62 when I expect 6,0625

Guess you need to format tdec with leading zeros:

print(string.format("%d,%04d", temp, tdec))

Fisherman48 · 2018-02-20T14:09:03Z

Strange behaviour at 0,0 C?
Integer master modules , powered by Lua 5.1.4 on SDK 2.1.0(116b762)
adding temp + tdec in excel

Never show zero C temp- temp+tdec show -1 C when I expect zero.
Se discontinuity in graph graph

`
local ow_pin = 3
ds18b20.setup(ow_pin)

dstimer = tmr.create()
dstimer:register(60*1000, tmr.ALARM_AUTO, function()

-- read all sensors and print all measurement results
ds18b20.read(
    function(ind,rom,res,temp,tdec,par)
        print(ind,string.format("%02X:%02X:%02X:%02X:%02X:%02X:%02X:%02X",string.match(rom,"(%d+):(%d+):(%d+):(%d+):(%d+):(%d+):(%d+):(%d+)")),res,temp,tdec,par)
    end,{});
end)

dstimer:start()`

/Fisherman

Fisherman48 · 2018-02-21T11:46:50Z

After a cold night the same effect occurs at all negative x.0 temps from -2.0, -3.0 to -9.0
Is it something due to scaling and 2'nd complement for neg temps for integer version of module?
/Fisherman

TerryE · 2018-03-02T18:04:52Z

@Fisherman48, have tried extending the wait time Line 188 out to 800 or 900 mSec. 760 is only 10mSec over the datasheet minimum, and the code might just be jumping in too early before the reading is complete in cold weather. I use 1sec in my Lua version but then again I do all of my device converts in parallel then loop around the devices to do the read, so it still works out a lot faster.

add ds18b20 module

2291d2c

nickandrew reviewed Jun 16, 2017

View reviewed changes

docs/en/modules/ds18b20.md Outdated

end

end);

-- print if temperature is greater or less then a defined value

Copy link

Contributor

nickandrew Jun 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo

fetchbot added 2 commits June 16, 2017 13:39

add intitial eeprom value definition

ac65003

fix typo

39e67b6

marcelstoer added the new module label Jun 16, 2017

adjust read() function and address handling

a746c5f

marcelstoer added this to the 2.1 follow-up II milestone Aug 16, 2017

marcelstoer merged commit d079b84 into nodemcu:dev Aug 16, 2017

fetchbot deleted the dev-ds18b20 branch August 27, 2017 09:25

eiselekd pushed a commit to eiselekd/nodemcu-firmware that referenced this pull request Jan 7, 2018

add ds18b20 module (nodemcu#2003)

0e53963

* add ds18b20 module * add intitial eeprom value definition * adjust read() function and address handling

TerryE mentioned this pull request Mar 2, 2018

Module ds18b20 determining the number of detected sensors #2274

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add ds18b20 module #2003

add ds18b20 module #2003

fetchbot commented Jun 11, 2017

marcelstoer commented Jun 11, 2017

fetchbot commented Jun 11, 2017

TerryE commented Jun 11, 2017 •

edited

Loading

fetchbot commented Jun 11, 2017

joysfera commented Jun 11, 2017 •

edited

Loading

devyte commented Jun 12, 2017 via email

marcelstoer commented Jun 14, 2017

TerryE commented Jun 14, 2017 •

edited

Loading

nickandrew Jun 16, 2017

nickandrew Jun 16, 2017

nickandrew Jun 16, 2017

marcelstoer commented Jun 28, 2017

TerryE commented Jun 28, 2017

devsaurus commented Jul 3, 2017

nickandrew commented Jul 13, 2017

marcelstoer commented Jul 13, 2017

nickandrew commented Jul 13, 2017

TerryE commented Jul 13, 2017

nickandrew commented Aug 3, 2017

TerryE commented Aug 3, 2017 •

edited

Loading

marcelstoer commented Aug 16, 2017 •

edited

Loading

Fisherman48 commented Jan 23, 2018

devsaurus commented Jan 24, 2018

Fisherman48 commented Feb 20, 2018

Fisherman48 commented Feb 21, 2018

TerryE commented Mar 2, 2018

add ds18b20 module #2003

add ds18b20 module #2003

Conversation

fetchbot commented Jun 11, 2017

marcelstoer commented Jun 11, 2017

fetchbot commented Jun 11, 2017

TerryE commented Jun 11, 2017 • edited Loading

fetchbot commented Jun 11, 2017

joysfera commented Jun 11, 2017 • edited Loading

devyte commented Jun 12, 2017 via email

marcelstoer commented Jun 14, 2017

TerryE commented Jun 14, 2017 • edited Loading

nickandrew Jun 16, 2017

Choose a reason for hiding this comment

nickandrew Jun 16, 2017

Choose a reason for hiding this comment

nickandrew Jun 16, 2017

Choose a reason for hiding this comment

marcelstoer commented Jun 28, 2017

TerryE commented Jun 28, 2017

devsaurus commented Jul 3, 2017

nickandrew commented Jul 13, 2017

marcelstoer commented Jul 13, 2017

nickandrew commented Jul 13, 2017

TerryE commented Jul 13, 2017

nickandrew commented Aug 3, 2017

TerryE commented Aug 3, 2017 • edited Loading

marcelstoer commented Aug 16, 2017 • edited Loading

Fisherman48 commented Jan 23, 2018

devsaurus commented Jan 24, 2018

Fisherman48 commented Feb 20, 2018

Fisherman48 commented Feb 21, 2018

TerryE commented Mar 2, 2018

TerryE commented Jun 11, 2017 •

edited

Loading

joysfera commented Jun 11, 2017 •

edited

Loading

TerryE commented Jun 14, 2017 •

edited

Loading

TerryE commented Aug 3, 2017 •

edited

Loading

marcelstoer commented Aug 16, 2017 •

edited

Loading