add LiquidCrystal lua module

[matsievskiysv]

This module provides functionality similar to Arduino LiquidCrystal library - control LCD text screens based on the Hitachi HD44780 chip. It supports 4bit and 8bit GPIO; 4bit I2C interfaces (via GPIO expander). I2C version was tested in i2c.SLOW and i2c.FAST speed modes.

Some functionality of the LCD chip was not implemented: reading text from internal memory and adding custom characters. These functions are exotic and are not needed for most applications.

• 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/*.

[nwf]

I have done limited testing with a 4x20 4-bit/I2C LCD module I have laying around and it seems workable. I've been using a forked version of https://github.com/dvv/nodemcu-thingies/blob/master/lcd1602.lua in my projects, but having something in the NodeMCU tree itself seems like a grand plan. The things I think would be welcome improvements are...

• Some notion of a four line display (even if just a table of the offsets for column 0).
• Implementing LCD_SETCGRAMADDR functionality (custom characters)
• The ability to specify which bit of the I/O port is actually LCD_RS, LCD_RW, LCD_EN, and LCD_BACKLIGHT in the bus_args table.

But I don't think any of those are blockers for this going into the tree.

[nwf]

Oh, actually, one thing that might be important before merging: documenting exactly what the busyloop parameter influences, so that callers can choose to instead provide their own timing delays rather than using tmr.delay.

[matsievskiysv]

• Added customChar function that defines custom characters.
• Changed write so that it would allow printing characters by char code.
• Added optional row argument to cursorMove.

The ability to specify which bit of the I/O port is actually LCD_RS, LCD_RW, LCD_EN, and LCD_BACKLIGHT in the bus_args table.

@nwf Do you mean that someone might want to use I2C GPIO expander other then this one?

[nwf]

Some people apparently use the same PCF8574P I2C GPIO expander with different wiring for the LCD itself; the suggested schematic in http://lcdproc.sourceforge.net/docs/lcdproc-0-5-5-user.html#hd44780-i2c, for example, uses different pinouts. The lcdproc driver thus exports, and I have had to use, configuration knobs for which pin is which; see https://github.com/lcdproc/lcdproc/blob/5ea8501240d00f3e9744f6a78c088a3446eea75f/server/drivers/hd44780-i2c.c#L150, for example. If it's straightforward, it'd be great if LiquidCrystal were similarly capable.

[matsievskiysv]

Added the following definition to i2c4bit.lua:

local LCD_RS = 0
local LCD_RW = 1
local LCD_EN = 2
local LCD_BACKLIGHT = 3
local LCD_D4 = 4
local LCD_D5 = 5
local LCD_D6 = 6
local LCD_D7 = 7

This way it is easy enough to tune for different pinouts and init function would not be too messy.

[nwf]

I was just about to commit this, because it's been languishing too long, but then I realized... img/HD44780_char_codes.png is probably not redistributable; it looks like an excerpt from a manual, and so probably copyrighted. @seregaxvm, if you still care about this patch series after all this time (sorry), can you please amend it to not include that file? Failing that, I'll get around to it myself at some point, because I really would like to have a LCD module in NodeMCU.

[nwf]

Sorry, as said, the png's gotta go or have a license indicating its redistributability.

This review is to block future-me from accidentally committing.

[matsievskiysv]

can you please amend it to not include that file

@nwf Done

[marcelstoer]

Thanks for paying attention to copyright! Instead of redistributing an image one could either link to one (e.g. https://www.google.ch/search?q=HD44780+char+codes&tbm=isch&source=univ) or link to a specific page in the PDF like so HD44780.pdf#page=35

[matsievskiysv]

Had some static analyzer warnings there...
Added link to specific datasheet page.

[nwf]

Some very minor additional requests. If you'd rather not, I think I'm happy to merge this as is (thank you again for all the work put into it!) and I'll submit a follow-up PR.

[nwf]

Some additional thoughts, around memory saving, for you to ponder. Thanks again!

[nwf]

Creating backends like this is going to result in a new object per LCD, even if they all use the same backend. That's fine, and could even be what you want, except that the backends here are coded to be immutable collections of functions. So I'd suggest either:

• switch to require-ing the backends, so that they only get loaded once (and are subsequently cached)
• redesign the backends to be factories that produce functions that close over their configuration parameters and state, rather than look them up in dictionaries. These factories can return (or directly set, if that's more your thing) functions that end up being self._command, self._backlight, self._write and friends.

Either approach should help with memory usage. I think doing both will have the maximum impact, as then the function bodies will be shared and only the closures will vary per LCD.

[matsievskiysv]

Current code works with both SPIFFS and LFS. require will not find module if it is stored on LFS

[nwf]

We really should just make package.loaders search LFS by default at this point, but thus far the guidance has been for programmers to enable that themselves: https://nodemcu.readthedocs.io/en/master/lfs/#accessing-lfs-functions-and-loading-lfs-modules

Anyway, require can be taught to search LFS and it's the recommended configuration.

Failing that, you could push the onus of finding and constructing the backend to the user. So rather than :init(bus, bus_args, ...), it'd be more like :init((require "backend")(args), ...).

[nwf]

I've made some time to draft up what I had in mind in hopes of tamping down on memory requirements by using constructors and closures rather than tables. I've also found out that it is possible to read back from these LCDs and so gone and written a first pass at an automated test harness for LiquidCrystal.

Give https://github.com/nwf/nodemcu-firmware/tree/lcd-draft a look over and take anything that's useful. Please note that it the draft there is incomplete.

[matsievskiysv]

I've made some time to draft up what I had in mind in hopes of tamping down on memory requirements by using constructors and closures rather than tables. I've also found out that it is possible to read back from these LCDs and so gone and written a first pass at an automated test harness for LiquidCrystal.

Give https://github.com/nwf/nodemcu-firmware/tree/lcd-draft a look over and take anything that's useful. Please note that it the draft there is incomplete.

@nwf I'm not very familiar with Lua internals. Does it really require less memory to have such constructors with local functions? Don't all functions internally have metatables associated with them? Or require somehow caches all that?

[nwf]

I will not pretend to be an expert at Lua internals, by any means. But, as I understand it, the function bodies themselves are constant and are shared values across all constructor invocations. The bits that aren't shared are the "closure" entries that point at the code blocks and the captured "upval" variables. These are not generic (meta)tables, but slightly specialized storage structures for each function. Section 3 of https://pdfs.semanticscholar.org/73a2/e3c03f799956aa5a3188e4eb35c90977a471.pdf seems to do an OK job describing what's going on.

Even with the juggling I did, instantiating a LCD still seems to occupy 3-4KB of RAM, which seems... high. I mean to investigate further as time permits.

[matsievskiysv]

I've got these results for (keep in mind that closure version has extra feature read from lcd implemented):

node.heap()	table	closure
initial	43056	43056
instantiated	28232	26496
garbage collected	28232	27016
Δ	14824	16040

Maybe this ought to be a C module?

[nwf]

That's a good idea, and nicely decouples the issue from this PR. Go ahead and leave it as you have, with local names for constant values.

[matsievskiysv]

Actually I suspect that it's not too hard to detect constants automatically without any keywords

[TerryE]

The whole idea of considering a c preprocessor for embedded Lua is definitely the wong way to go IMO, and just not needed. Accessing Upvals is cheap and the storage overhead is minimal. In general it takes less RAM to use scalar upvals than table ones. Also remember that binding upvals is only done once when the closure is itself bound, and the cost of accessing a upval local less than a global or table entry.

In an LFS usecase, don't worry too much about the true constants and Lua code since this is in flash anyway. If in doubt then luac -l -l is your friend, and the Lua VM Internals paper that I reference in my FAQ. 😄

[nwf]

@TerryE: the particular concern is that there isn't a way to define a symbolic name for a constant in Lua. That's the motivation for wanting either to rewrite the Lua before handing it off to luac, especially in combination with LFS or to push constants into a C rotable. Neither option is very attractive, and I would, ordinarily, agree that neither should be considered, were it not for....

In my experiment on this PR, I found that inlining all the local LCD_FOO = 0x04 definitions saved almost a kilobyte of heap, between the storage allocated for the TValues and the upvals in the closures created for the LiquidCrystal metatable methods. While those TValue structures and the function closures are indeed only created once, an entire kilobyte seemed quite heavy.

That concern then spilled out more generally: should we generally be trying to minimize the number of upvals referenced by closures by referring to tables instead? I think the answer is no, because unless there is truly an exorbitant number of closures, the overhead of creating the table outweighs the overhead of the upvals, but I don't know for sure.

[TerryE]

@TerryE: the particular concern is that there isn't a way to define a symbolic name for a constant in Lua...

Lua has some strengths for embedded use but unlike JavaScript and PHP, there isn't the concept of symbolic constants in the language.

In my experiment on this PR, I found that inlining all the local LCD_FOO = 0x04 definitions saved almost a kilobyte of heap, between the storage allocated for the TValues and the upvals in the closures created ...

I accept your figure of 1Kb, which is regrettable, but then again this is only a few % of the total heap at boot. This shouldn't be an issue with the code in LFS. The whole point of LFS is that we can move the Lua code of RAM, so we can be a little more relaxed about creating variables.

That concern then spilled out more generally: should we generally be trying to minimize the number of upvals referenced by closures by referring to tables instead? I think the answer is no, because unless there is truly an exorbitant number of closures, the overhead of creating the table outweighs the overhead of the upvals, but I don't know for sure.

I think that the main goals in any code should be correctness, clarity and simplicity.

[matsievskiysv]

This code may be used to test gpio8bit on esp32 (since sep8266 does not have enough gpio pins):

local gpio = gpio

return {
   read = gpio.read,
   write = gpio.write,
   mode = function(pin, mode)
      gpio.config{gpio={pin}, dir=mode}
   end,
   INPUT = gpio.IN,
   OUTPUT = gpio.OUT,
   HIGH = 1,
   LOW = 0,
}

[matsievskiysv]

@nwf Code and docs are updated, only tests are left. Maybe wait until #2983 before adding them?

[nwf]

Thank you for all your hard work on this! I believe this PR is good to go live, but I'll give it a day or so for others to chime in. Yes, I think the tests will have to wait until we've decided how we want to do things; for LiquidCrystal, mispec (cf. #2984) may be able to completely cover the intended functionality (esp. since there's only one ESP and no network involved), and so we wouldn't need host-driven tests (#2983).

[nwf]

Ah, one more favor to ask: can you rename your backends from (e.g.) i2c4bit.lua to lc-i2c4bit.lua?

[nwf]

Er, no, one more still, I'm afraid. While readChar can read the display RAM, there doesn't appear to be a way to read graphics RAM? I think that just means another parameter to the backend's read function?

[matsievskiysv]

@nwf added missing functionality, renamed backends, fixed some 10 dot mode bugs. When this will be ready for merge I will squash the commits.

[nwf]

LGTM!

[marcelstoer]

When this will be ready for merge I will squash the commits.

Thanks, but there's no need. GitHub will do that for us when we merge 😄

[matsievskiysv]

I've made a small program for custom character generation. I'll upload it to PyPi and update documentation soon.

[nwf]

@seregaxvm I'm ready to push the button if you are. :)

[matsievskiysv]

Merge when you are ready. I don't have anything to add