Nodemcu-firmware: Polish the Lua examples

[marcelstoer]

Go through most of the examples an refresh them using the API and decent coding style.

@TerryE you once stated you had this on your TODO list. Feel free to remove the assignee.

Also, I don't see why we should have both /examples and /lua_examples.

[TerryE]

It's a bit much for one person, but yes I will try to do this and work through these. No commitments on timescales though.

[marcelstoer]

Thanks! There's no need to do it all at once. You can very well create a PR for every example you fix. Once we have a few consistent examples we can parallelize and still achieve high consistency across all examples.

[marcelstoer]

Also replace old tmr code with new OO tmr API.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[TerryE]

This is still job that needs to be done.

[galjonsfigur]

I started digging into this problem and here are my thoughts:

• The best way to ensure correctness and good quality of Lua examples is to use static analysis tool like luacheck + custom configuration for it to make it aware of all possible global variables in NodeMCU environment.

• That way it would also be possible to add it to Travis CI so every example file can be checked so adding any new examples will not involve much pain.

• Using static analysis tool requires custom configuration and for every C module that exist a configuration entry would have to be made. I wrote test configuration containing only globals from node C module and it works like it should, but even if I would write entry for every function, table, or value in every C Module, It would be another thing to remember when writing new C module (right now there is only need to write C module, documentation and add entries in user_modules.h and mkdocs.yml.)

• Also it would be possible to check examples in documentation that way and find issues like this but for now this is out of scope of this issue

• The same goes for Lua Modules but I think luacheck will be even more handy there.

I will start testing this approach and checking if correct configuration for luacheck is possible for every C module. Any suggestions are welcome.

[galjonsfigur]

Update:
I created configuration file for luacheckfor most of C modules using script to read all functions after LROT_BEGIN. The only ones that were not included were sqlite (still not updated for new C module syntax), u8g2 and ucg (there are just too many variables like fonts and displays but at some point could be added) and the results:

Total: 935 warnings / 1 error in 45 files

Configuration gist
99% of them is about using global variables and 1% is actual bugs in the examples. At this point I should ask: Are global variables necessary for those examples? Is there any performance or memory benefit over local variables in eLua or it does not matter? I know globals can be sometimes useful but most of those examples are one file scripts so making some of the variables local shouldn't matter at all and AFAIK when writing Lua scripts for Desktop there can be a performance benefit when using locals for things from standard libraries like math.

I will try to create a PR that would include a luacheck configuration and script for Travis CI that will be disabled for now so the builds won't fail. I will also try to fix those examples one by one and enable luacheck in Travis when the process would be done.

[galjonsfigur]

Update 2:
I created a new repository to configure Travis properly - https://github.com/galjonsfigur/travis-lua-nodemcu-tests (Just copied travis.yml from this repo and commented out most of it to only test luacheck) and it's working as it should (mostly). The idea is to create a PR to include this stage of test but comment it out for now and add information on how to use luacheck in CONTRIBUTING.md file.

I would be glad if anyone could give any feedback on this whole idea. I want to be sure that this is a correct approach.

[marcelstoer]

I like where you are headed with this. Looking forward to the PR.

comment it out for now and add information on how to use luacheck in CONTRIBUTING.md file.

Why not activate it? As long as it doesn't break the build when "violations" are detected I see no harm in having it active.

[galjonsfigur]

As long as it doesn't break the build

It will break the build because luacheck passes with 0 exit code only when there are no warnings. I see no reason to dig in Travis build logs to see if the warnings changed or not - the most important thing would be to set up luacheck so anyone could improve quality any NodeMCU Lua code. I will try to make this PR today and after that more PRs to start fixing the examples and maybe commenting on how they work.

[galjonsfigur]

Just progress update:
Total: 0 warnings / 0 errors in 68 files
I went through all .lua files in both lua_examples and lua_modules and fix/silence all the warnings. I will create a PR when I will be done with testing those I can test. There are still many questions regarding code style(should it be standardized in some way or not?), examples that won't work anymore because of dead third-party services(like yeelink) or when it will be a good idea to silence warnings from the code (luacheck has a neat feature called inline options so there is a way to set a fragment of code that luacheck will not check).

[TerryE]

@galjonsfigur, perhaps you might want to post a link to your branch or put up a reference example as a gist so we can discuss and reach a consensus on this. :smile:

[galjonsfigur]

@TerryE Just sent changes here - for now only ds18b20 and adc_rgb are tested - the rest is only checked. Those commits only fix luacheck warnings so there are still many things to do. I think the best reference example would be adc_rgb.lua - well commented, simple and clean. Any feedback and nitpicking very welcome 😊
EDIT:
Any feedback and nitpicking form anyone very welcome 😊

[TerryE]

@galjonsfigur, is there any way we can add luacheck pragmas so that we can suppress individual warning where there is a valid reason for this being the case?

And the answer to my Q is here.

[TerryE]

On a separate note, I've yet to work out why the Travis validation of my build is executing a full Lua source validation and failing. IMO, since this involves installing both Lua 5.3.5 and luarocks in the container and aborting the Travis check if _any_ Lua warnings are raised, then we definitely should _not_ be doing these checks by default every make.