Polish Lua examples

[galjonsfigur]

Fixes #772.

• This PR is for the dev branch rather than for master.
• This PR is compliant with the other contributing guidelines
• I have thoroughly tested my contribution.
• The code changes are reflected in the documentation at docs/*.

This will be a long PR but because it only touches Lua files it shouldn't be hard to rebase if necessary.
Here I will be pushing more and more gradual polishing of the examples. This first pass is only to fix all luacheck warnings so it will be easier to work on.

And there is a lot to work on:

• Code and comment style: That one is important discuss - any suggestions/ideas/propositions welcome.
• Introduction examples: Arduino has it's own Blink sketch - I think it would be a wise idea to do similar thing - add a few examples that would be just as simple only for introduction to development model and maybe to show a proper way to do some basic things like connecting to WiFi. (wifi module documentation is great but can be unintuitive for a beginner user)
• Pattern for Lua device drivers: I think there should be some pattern to follow when writing a driver for an I2C or SPI device. There are lots of various sensors, ICs and other devices that don't require to be implemented in C so introducing some kind of code model for that seems to be a good idea.
• Not working examples or modules: yeelink module is pretty much useless now because it depends on 3rd-party service that doesn't work any more. Other example is irsend.lua that seems to do some kind of delay to be able to generate proper signal and I doubt if that's still will work after all the changes that were made. But that one is still to be tested.
• Examples that couldn't be tested because of the lack of hardware: ucglib, somfy, bh1750, dht_lib, ds3231, lm92, hdc1000are the ones I can't test for now, but testing them will be more important later.
• Probably more but for now I can't think of anything else - any additional suggestions very welcome.

I'm still testing if my fixes for luacheck warnings broke something and for now ds18b20, webap_toggle_pin, adc_rgb, u8g2, and pcm/play_file are confirmed to work - more to come.

I encourage anyone to nitpick anything here - this PR is not as important as those that mess with C part of the project and it's a low-priority PR when compared to the others.

[TerryE]

@galjonsfigur I've removed the DMN tag because I disagree, but because it does matter, IMO, I also think that we should get this right. I'll put some specific review comments inline, but I also think that there is a more general issue that we need to consider / discuss and to agree the broad principles here. This relates to the general issue of how coding in NodeMCU Lua is different and should additional quality criteria should apply? Put simply, I think that Lua for ESP devices should conform to Lua best practice, but not all code that is best practice in a luacheck sense makes good NodeMCU code.

The main resource that we are limited by in our IoT apps is RAM space, so:

• Where possible examples should be "LFS friendly" so that the code can be run out of Flash instead of RAM
• Some though should be made in keeping RAM data compact and limited in scope: data should not linger past its natural life.
• A corollary to this is that any module or function should be ephemeral in some sense: when closed or don with, nothing should be left in the environment or the registry, so that the free heap should be the same as before the routine ran.

The mark and sweep Lua GC is very good at collecting dead data, but the Achilles heal here is the Lua Registry. You will quite often create some object such as a network socket where you then establish Lua callbacks and these CBs may need to have the UData object as an upvalue, hence we have a UData in the registry which refers to a Lua function in the registry which has the UData as an upval. Because the dependency graph of collectible object involves the Registry, this frustrates the Lua GC.

You can get similar issues in Lua code, so I feel that any "good" Lua module should include some form of close then results in the complete cleanup of resources. You can see that I do this with my FTPserver and telnet examples as well as the new coroutining one that I am about to add.

[TerryE]

I know that the below seems like a knit-pikck, but if we want developers to use this facility then it should be usable and not slow down the travis check process unduly.

[TerryE]

• If Lua checking is mandatory for the linux build then IMO it should be called from ci-build-linux.sh and not by extra lines in the YML.
  You haven't changed the run-check.sh on this PR but you should:
• If luacheck is already on the path then the install components should be skipped. This facilitates developers running the script when developing as well as during Travis QA runs.
• Ditto if luarocks is already installed then the only the rocks install of luacheck should be done. As I said in a previous comment we should not be running builds of Lua and Luarocks when they are already available as package addons in the .travis,yml

[galjonsfigur]

Those additional commands lines are using luac.cross to check if the .lua files have no syntax errors and this check is used both in Linux and Windows build - that way also luac.cross binary can be tested on both platforms.
I modified run-luacheck.sh to use pre-installed LuaRocks by default, but added an standalone install option just in case. Tested both options and everything seems to work fine (at least in docker environment).

It may be a good option to make this check before building a whole firmware or even better - check if commit changes only .lua files, only C stuff or both (using TRAVIS_COMMIT_RANGE) and run firmware build or luacheck only when necessary but this is out of scope of this PR.

[HHHartmann]

It might also make sense to have another job for luacheck, just like for windows. That way luacheck could run in parallel and would not require any extra waiting time.

[galjonsfigur]

@HHHartmann I added script to run Luacheck on Windows and it seems to be working properly.

BTW I just realised that I misunderstood your comment and added Luacheck for windows instead of making it as a separate job. Sorry 😄 It runs much faster on Windows though (because only luac.cross is built in that enviroment) and the script itself can be used in normal Windows environment with Bash included in Git for Windows and wget.

[HHHartmann]

Does luacheck for linux yield difrfrent results than for windoes? If not maybe the windoes part would be enough. But then the linux version is not tested and starts rotting.

[galjonsfigur]

luacheck for both Windows and GNU/Linux should give the exact same output, so it's not really necessary to run it in both environments. The only advantage of using Windows is that it's faster to check because there is official pre-compiled binary of luacheck with built-in Lua 5.3 and other libraries + Travis can cache the binary itself. A good idea would be to look into Travis CI docs and add separate jobs for changes in Lua code and changes in C code, but that's out-of-scope for this PR.

[HHHartmann]

So should we use the windows version only here to save time and add the linux version only to a release build to detect when it is broken?
@TerryE Terry what is your opinion on this?

[marcelstoer]

@galjonsfigur can I ask you to resolve the conflicts please.

@TerryE any more changes requested from your side? @HHHartmann I think yours were properly addressed, no?

[galjonsfigur]

@marcelstoer I resolved conflicts and made some updates like adding support for the DCC module.
@vsky279 I changed your example code for DCC module. I would be thankful if you could look into it and test/review this change.

[marcelstoer]

Wow, thank you so much! I really appreciate your efforts 👏

[marcelstoer]

It might make sense to define the retry params. You don't want a network hiccup to fail your build.

[HHHartmann]

Some nitpicking from my side.

1. I would prefer to leave unused variables with their names instead of renaming them to _
   I would always ask myself what it is. Maybe add an underscore at the beginning to quickly see that they are unused.
2. I would prefer -- luacheck: std max type exceptions over using -- luacheck: ignore because then you can see what is excepted. Also individual lines are better than blocks as has been replaced in some cases already.
3. should we add a TODO refactor on exceptions like -- luacheck: new globals z T r disp lcg_rnd millis

But none of them are showstoppers in my opinion.

Another issue is what we can do to not forget about the suggestions to further improve the tests by @galjonsfigur and @TerryE. Should we create another Issue for those?

[galjonsfigur]

@HHHartmann Thanks for feedback. I will look into this again and replace rest of --luacheck: push ignore blocks where necessary.

The underscore variable is mostly just a Lua community convention to mark variable that is unused or used just as a "padding" to get the desired output from function returning multiple variables. For example: for _, value in ipairs(someTable) means that only values are being used and keys for this table are unused in code inside the loop. But I'm sure that for at least some of the .lua files I replaced too much of them. Instead of this an inline luacheck comment can be used.

About the globals usage: I think the display example where all those globals are used is pretty interesting - the example code loads and unloads some of the code to save RAM and global variables are used as shared data. I will try to find a better way to do this but for now it's not really bad.

[HHHartmann]

@galjonsfigur that sounds good. Overall this PR is an importent thing to have. In C if we break an interface the compiler immediately tells us but often we forget the Lua codebase. With LFS this has become even more important.

[TerryE]

I've scanned through the various changes, and I feel that overall the Lua code is much improved. I did come across a couple of instances where @galjonsfigur has included bugfixes in his changes and a couple where the changes also changed the semantics of the source. In one case in some of my old code changing from socket to sck would introduce a resource leak if I am not mistaken, but given that the next line was node.restart(), this wouldn't be material or easy to test.

However, the bottom line to me is that galjonsfigur has done a lot of quality work here and the examples overall will be more solid. We have a general issue that some will be dated because we've since changed APIs etc. and missed regressing this into the examples, but I think it unrealistic to expect to address all these in this PR.

One specific note which picks up an earlier point of Gregor's. Often the examples are there as a starting point for developers to build upon and to customise to their particular applications. Hence whilst variables such as sck might be unused in the example, we would expect the custom version to use them. In these circumstances it would be better to leave the variable name in place rather than changing it out to _ say.

A net 👍

[galjonsfigur]

@TerryE Thank you for the feedback! I think there is a lot more work to do aside from this PR:

• Exposing luacheck and tools like https://luac.nl/ in documentation - they can be very useful and show some bugs/problems with code (especially luacheck)
• Removing dead examples/libraries like yeelink
• Rewriting some more important examples like mqtt
• Create more examples for rest of the modules like coap

And Issues/PRs for each one of them. That's a lot of work and I will try to do them slowly as time allows.

[nwf]

This seems an excellent, solid improvement. Given @TerryE's +1, I'm going to merge this to dev. Thanks @galjonsfigur!