Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rewrite of the Lua DS18B20 module #1883

Closed
TerryE opened this issue Mar 26, 2017 · 13 comments
Closed

Rewrite of the Lua DS18B20 module #1883

TerryE opened this issue Mar 26, 2017 · 13 comments

Comments

@TerryE
Copy link
Collaborator

TerryE commented Mar 26, 2017

I am not sure at this stage whether this is a bugrep or a missing feature. I am using DS18B20s in anger and decided that I'd rewrite the ds18b20 lua module. My reasons were as follows:

  • I wanted to make the module tmr compatible, and to this end I've split the read function into a convert_T() call which issues the convert command and a separate read() which collects the response from a single thermometer which and be collected on a ALARM_SEMI callback.
  • The module now supports multiple DS18B20s correctly in that a convert_(true) does an ow.skip() instead of an ow.select() and starts all DS18B20s converting in parallel.
  • The module saves the search output to ds18b20_save.lc and reloads this subsequently on first use so that you don't need to do the search each waking after deep sleep.
  • I've also used Lua best practice for locals and upvalues to minimise size / runtime.

Here is my test harness which shows the difference at an application level:

local ds = require("ds18b20")
local to_string, print, gpio2 = ds.to_string, print, 4
local t1, addrs

ds.init(gpio2)
addrs = ds.get_addrs()

if addrs and #addrs > 0 then
  print("Total DS18B20 sensors: ")
  for k,v in ipairs(addrs) do print (' ',to_string(v)) end
  local tmr1,tmr2 = tmr.create(), tmr.create()
  tmr1:register(1000, tmr.ALARM_SEMI, function()
    local t = {}
    for i = 1, #addrs do t[i] =  ("%8.3f"):format(ds.read(addrs[i])) end
    print(unpack(t))
  end)
  tmr2:alarm(30000, tmr.ALARM_AUTO, function()
    ds.convert_T(true)
    tmr1:start()
  end)
else
  print("No OW thermometers found")
end

You can happily crank this up to collect half a dozen probe temperatures each second.

@TerryE TerryE changed the title Rewrite of the Lua DS8B20 module Rewrite of the Lua DS18B20 module Mar 26, 2017
@TerryE
Copy link
Collaborator Author

TerryE commented Mar 31, 2017

@devsaurus @pjsg @jmattsson I've added a gist copy here. I'd be interested in your views. I am rewriting a few of these for my own use anyway, and I'd like your views on whether it is worth replacing the existing libraries as we do so.

What I've done here is to make the code a lot more runtime efficient as well as address a few functional holes.

@karrots
Copy link
Contributor

karrots commented Mar 31, 2017

Gist link needs to be fixed. Github is improperly prepending github.com instead of gist.github.com

@TerryE
Copy link
Collaborator Author

TerryE commented Mar 31, 2017

Sorry Jonathan. Done 😃

@jmattsson
Copy link
Member

Haven't used any DS18B20s with NodeMCU, so have nothing to compare against, but this seems good?

@TerryE
Copy link
Collaborator Author

TerryE commented Apr 3, 2017

Why the ? 😃

@jmattsson
Copy link
Member

Because I don't feel like I'm an authoritative source on such matters? :)

@johndoe71rus
Copy link

try test your code. But get

dofile("ds18b20_terry.lua")
No OW thermometers found

I use nodencu pin3(GPIO0)
old example, is work for me.

@vsky279
Copy link
Contributor

vsky279 commented Apr 22, 2017

@TerryE I have upgraded my module using your suggestions. It should fix majority of flaws discussed in #1820.

Please have a look here: https://github.com/vsky279/nodemcu-firmware/tree/DS18B20/lua_modules/ds18b20

Enhancements:

  • primarily targetted to float rather than integer version of Lua
  • upvalues are used much more than in previous version - it should lead to lower memory usage and more efficient code
  • ow.skip() now supported - when only non-parasit powered sensors are present on the bus the conversion is started for all sensors at once
  • bus search performed in tasks - no long loop - no need to use tmr.wdclr() (though I'm not sure it was even needed - this hase been taken it from the previous version of the module)
  • no more use of encoder module
  • enable_debug method implemented for debugging purposes
  • no need to perform search before each readout (can be invoked optionally by search parameter). Sensor addresses are not stored on the flash - I find it more easier to perform search rather than writing to flash (though your method is very elegant). Storing ds18b20.sens should be done by the application if deep sleep friendliness is requested.

Possible enhancements:

  • still using the 750 ms delay for conversion - I have no issues with this in my usecase (even mixing parasitic and powered sensors)
  • only centigrade implemented for the time being
  • no family search supported

If there is no opposition I would submit a PR as this implementation is better than the existing module. I still think the module should support both parasit and non-parasit powered sensors (which is not supported by your module).

@TerryE
Copy link
Collaborator Author

TerryE commented Apr 25, 2017

@vsky279 Lukáš, I thimk that it would be a good thing to converge on a single implementation that we both support. If we can do that, then it will be a good solution. Things that I would prefer to add:

  • Search
  • Conversions other than C. Our US colleagues seem wedded to °F and inches, etc.
  • Optional storing and retrieval of address to flash
  • I don't like commented-out code (the integer stuff). I need to suggest a better solution.

Give me a few days. I am up to my neck in my core ro TSring stuff. :)

@vsky279
Copy link
Contributor

vsky279 commented Apr 26, 2017

@TerryE I've started to implement your suggestion (except the integer stuff :-) ) so don't spend your time on it. I will come with a proposal that you can review afterwards.

@vsky279
Copy link
Contributor

vsky279 commented May 22, 2017

@TerryE I need to restart. What I miss is the optional storing and retrieval of address to flash.
As for the integer stuff I let you suggest better solution when I'm done.

@vsky279
Copy link
Contributor

vsky279 commented May 26, 2017

@TerryE Please have a look at the latest version of updated module: https://github.com/vsky279/nodemcu-firmware/blob/DS18B20/lua_modules/ds18b20/ds18b20.lua

It has all the functionalities discussed. The documentation is not yet updated. Please see the example in the same directory.
But I think it could be more efficient. It should avoid probably addressing the sens table by indexes addr, parasite and status and use a bit different approach.

Please let me know your thoughts about it.

@vsky279
Copy link
Contributor

vsky279 commented May 30, 2017

Done. I need to update documentation and will submit a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants