LiquidCrystal robustness and test (#3369)

* LiquidCrystal I2C 4-bit robustness

- Fix up some formatting
- Initialization is now more conformant with the datasheet.
- Read-backs don't needlessly (or erroneously!) store back

While here, document some unexpected behaviour of read-back commands.

* liquidcrystal i2c 4bit NTest

docs/lua-modules/liquidcrystal.md

@@ -237,6 +237,12 @@ liquidcrystal:blink(true)
 ## liquidcrystal.busy
 Get busy status of the LCD. When using GPIO backend without `rw` argument specification function does nothing.
 
+!!! note
+  At least some HD44780s and/or interfaces have been observed to count polling
+  the busy flag as grounds for incrementing their position in memory.  This is
+  mysterious, but software should restore the position after observing that the
+  busy flag is clear.
+
 #### Syntax
 `liquidcrystal.busy(self)`
 
@@ -429,6 +435,11 @@ liquidcrystal:leftToRight()
 ## liquidcrystal.position
 Get current position of the cursor. Position is 0 indexed. When using GPIO backend without `rw` argument specification function does nothing.
 
+!!! note
+  At least some HD44780s and/or interfaces have been observed to count reading
+  the position as grounds for incrementing their position in memory.  This is
+  mysterious, but software likely intends to restore the position anyway.
+
 #### Syntax
 `liquidcrystal.position(self)`
 

lua_modules/liquidcrystal/lc-i2c4bit.lua

@@ -22,27 +22,27 @@ return function(bus_args)
    -- The onus is on us to maintain the backlight state
    local backlight = true
 
-   local function send4bitI2C(value, rs_en, rw_en, read)
+   local function exchange(data, read)
-      local function exchange(data, unset_read)
+      local rv = data
-         local rv = data
+      i2c.start(busid)
+      i2c.address(busid, busad, i2c.TRANSMITTER)
+      i2c.write(busid, bit.set(data, en)) -- set data with en
+      if read then
+         i2c.start(busid) -- read 1 byte and go back to tx mode
+         i2c.address(busid, busad, i2c.RECEIVER)
+         rv = i2c.read(busid, 1):byte(1)
          i2c.start(busid)
          i2c.address(busid, busad, i2c.TRANSMITTER)
-         i2c.write(busid, bit.set(data, en))
-         if read then
-            i2c.start(busid)
-            i2c.address(busid, busad, i2c.RECEIVER)
-            rv = i2c.read(busid, 1):byte(1)
-            i2c.start(busid)
-            i2c.address(busid, busad, i2c.TRANSMITTER)
-            if unset_read then data = bit.bor(bit.bit(rs),
-                                              bit.bit(rw),
-                                              backlight and bit.bit(bl) or 0) end
-	    i2c.write(busid, bit.set(data, en))
-         end
-         i2c.write(busid, bit.clear(data, en))
-         i2c.stop(busid)
-         return rv
       end
+      i2c.write(busid, data) -- lower en
+      i2c.stop(busid)
+      return rv
+   end
+
+   local function send4bitI2C(value, rs_en, rw_en)
+      local meta = bit.bor(rs_en and bit.bit(rs) or 0,
+                          rw_en and bit.bit(rw) or 0,
+                          backlight and bit.bit(bl) or 0)
       local lo = bit.bor(bit.isset(value, 0) and bit.bit(d4) or 0,
                          bit.isset(value, 1) and bit.bit(d5) or 0,
                          bit.isset(value, 2) and bit.bit(d6) or 0,
@@ -51,11 +51,8 @@ return function(bus_args)
                          bit.isset(value, 5) and bit.bit(d5) or 0,
                          bit.isset(value, 6) and bit.bit(d6) or 0,
                          bit.isset(value, 7) and bit.bit(d7) or 0)
-      local cmd = bit.bor(rs_en and bit.bit(rs) or 0,
+      hi = exchange(bit.bor(meta, hi), rw_en)
-                          rw_en and bit.bit(rw) or 0,
+      lo = exchange(bit.bor(meta, lo), rw_en)
-                          backlight and bit.bit(bl) or 0)
-      hi = exchange(bit.bor(cmd, hi), false)
-      lo = exchange(bit.bor(cmd, lo), true)
       return bit.bor(bit.lshift(bit.isset(lo, d4) and 1 or 0, 0),
                      bit.lshift(bit.isset(lo, d5) and 1 or 0, 1),
                      bit.lshift(bit.isset(lo, d6) and 1 or 0, 2),
@@ -66,36 +63,45 @@ return function(bus_args)
                      bit.lshift(bit.isset(hi, d7) and 1 or 0, 7))
    end
 
-   -- init sequence from datasheet
+   -- init sequence from datasheet (Figure 24)
-   send4bitI2C(0x33, false, false, false)
+   local function justsend(what)
-   send4bitI2C(0x32, false, false, false)
+     i2c.start(busid)
+     i2c.address(busid, busad, i2c.TRANSMITTER)
+     i2c.write(busid, bit.set(what, en))
+     i2c.write(busid, what)
+     i2c.stop(busid)
+   end
+   local three = bit.bor(bit.bit(d4), bit.bit(d5))
+   justsend(three)
+   tmr.delay(5)
+   justsend(three)
+   tmr.delay(1)
+   justsend(three)
+   tmr.delay(1)
+   justsend(bit.bit(d5))
+   -- we are now primed for the FUNCTIONSET command from the liquidcrystal ctor
 
    -- Return backend object
    return {
       fourbits  = true,
       command   = function (_, cmd)
-         return send4bitI2C(cmd, false, false, false)
+         return send4bitI2C(cmd, false, false)
       end,
       busy      = function(_)
-         local rv = send4bitI2C(0xff, false, true, true)
+         return bit.isset(send4bitI2C(0xff, false, true), 7)
-         send4bitI2C(bit.bor(0x80, bit.clear(rv, 7)), false, false, false)
-         return bit.isset(rv, 7)
       end,
       position  = function(_)
-         local rv = bit.clear(send4bitI2C(0xff, false, true, true), 7)
+         return bit.clear(send4bitI2C(0xff, false, true), 7)
-         send4bitI2C(bit.bor(0x80, rv), false, false, false)
-         return rv
       end,
       write     = function(_, value)
-         return send4bitI2C(value, true, false, false)
+         return send4bitI2C(value, true, false)
       end,
       read      = function(_)
-         return send4bitI2C(0xff, true, true, true)
+         return send4bitI2C(0xff, true, true)
       end,
       backlight = function(_, on)
          backlight = on
-         local rv = bit.clear(send4bitI2C(0xff, false, true, true), 7)
+         send4bitI2C(0, false, false) -- No-op
-         send4bitI2C(bit.bor(0x80, rv), false, false, false)
          return on
       end,
    }

tests/NTest_lcd_i2c4bit.lua

@@ -0,0 +1,83 @@
+-- Run LiquidCrystal through some basic tests.  Requires `liquidcrystal.lua`
+-- and `l2-i2c4bit.lua` available available to `require`.
+--
+-- This file ought to be named "NTest_liquidcrystal_i2c4bit" or something,
+-- but it has its current name due to our default SPIFFS filename length limit.
+
+local N = ...
+N = (N or require "NTest")("liquidcrystal-i2c4bit")
+
+local metalcd
+local metaback
+local backend
+local lcd
+
+collectgarbage()
+print("HEAP init", node.heap())
+
+metalcd = require "liquidcrystal"
+collectgarbage() print("HEAP constructor imported ", node.heap())
+
+metaback = require "lc-i2c4bit"
+collectgarbage() print("HEAP backend imported ", node.heap())
+
+backend = metaback({
+ address = 0x27,
+ id  = 0,
+ speed = i2c.SLOW,
+ sda = 2,
+ scl = 1,
+})
+collectgarbage() print("HEAP backend built", node.heap())
+
+lcd = metalcd(backend, false, true, 20)
+collectgarbage() print("HEAP lcd built", node.heap())
+
+print("waiting for LCD to be unbusy before testing...")
+while lcd:busy() do end
+
+N.test("custom character", function()
+  local glyph = { 0x1F, 0x15, 0x1B, 0x15, 0x1F, 0x10, 0x10, 0x0 }
+  lcd:customChar(0, glyph)
+  ok(eq(glyph,lcd:readCustom(0)), "read back")
+end)
+
+N.test("draw and readback", function()
+  lcd:cursorMove(0)
+  lcd:write("abc")
+  lcd:cursorMove(10,1)
+  lcd:write("de")
+  lcd:cursorMove(10,2)
+  lcd:write("fg")
+  lcd:cursorMove(12,3)
+  lcd:write("hi\000")
+  lcd:cursorMove(18,4)
+  lcd:write("jk")
+
+  lcd:home()           ok(eq(0x61, lcd:read()), "read back 'a'")
+                       ok(eq(0x62, lcd:read()), "read back 'b'")
+  lcd:cursorMove(11,1) ok(eq(0x65, lcd:read()), "read back 'e'")
+  lcd:cursorMove(11,2) ok(eq(0x67, lcd:read()), "read back 'g'")
+  lcd:cursorMove(13,3) ok(eq(0x69, lcd:read()), "read back 'i'")
+  lcd:cursorMove(14,3) ok(eq(0x00, lcd:read()), "read back  0" )
+  lcd:cursorMove(19,4) ok(eq(0x6B, lcd:read()), "read back 'k'")
+
+end)
+
+N.test("update home", function()
+  lcd:home() lcd:write("l")
+  lcd:home() ok(eq(0x6C, lcd:read()))
+end)
+
+N.testasync("clear", function(next)
+  -- clear and poll busy
+  lcd:clear()
+  tmr.create():alarm(5, tmr.ALARM_SEMI, function(tp)
+    if lcd:busy() then tp:start() else next() end
+  end)
+  lcd:home() -- work around busy polling incrementing position (XXX)
+  ok(eq(0x20, lcd:read()), "is space")
+  ok(eq(1, lcd:position())) -- having just read 1 from home, we should be at 1
+end)
+
+