Nodemcu-firmware: Tidying up the Library coding styles

[TerryE]

I've noticed that the current libraries haven't been cut by someone who is familiar with the Lua C interface APIs and as a result the only a subset of the API is used with standard features being reimplemented inline in the library code. A good example is in the gpio.c module where:

  const char *str = luaL_checklstring( L, 2, &sl );
  if (str == NULL)
    return luaL_error( L, "wrong arg type" );

  if(sl == 2 && c_strcmp(str, "up") == 0){
    type = GPIO_PIN_INTR_POSEDGE;
  }else if(sl == 4 && c_strcmp(str, "down") == 0){
    type = GPIO_PIN_INTR_NEGEDGE;
  }else if(sl == 4 && c_strcmp(str, "both") == 0){
    type = GPIO_PIN_INTR_ANYEDGE;
  }else if(sl == 3 && c_strcmp(str, "low") == 0){
    type = GPIO_PIN_INTR_LOLEVEL;
  }else if(sl == 4 && c_strcmp(str, "high") == 0){
    type = GPIO_PIN_INTR_HILEVEL;
  }else{
    type = GPIO_PIN_INTR_DISABLE;
  }

can be better coded:

  static const char * const opts[] = {"", "up", "down", "both", "low", "high"};
  static const int opts_type[] = { 
    GPIO_PIN_INTR_DISABLE, GPIO_PIN_INTR_POSEDGE, GPIO_PIN_INTR_NEGEDGE,
    GPIO_PIN_INTR_ANYEDGE, GPIO_PIN_INTR_LOLEVEL, GPIO_PIN_INTR_HILEVEL
    };
  int type = opts_type[luaL_checkoption(L, 2, "", opts)];

This is shorter, easier to read and uses less code space.

My suggestion is that it is not worth going though the modules tidying up such loose coding as a separate exercise, but that if anyone is doing material changes to any specific module, then it does make sense doing this tidy up also at the same time if there is a code space saving.

Though given Nick's recent comments, maybe this tidy up is best done as a precursor commit.

[romanchyla]

gpio.c exemplifies one other issue (eg: serout() function) - the code for checking/reading from Lua state is mixed with the execution logic. I.e. 80% of all lines deal with getting input arguments, 20% is doing the actual work. That may be a matter of taste or necessity (a smaller footprint?)

is there any 'official' guidance? (ie: why is better to have static int lgpio_serout( lua_State* L ) and not:

• static int lgpio_gateway( lua_State* L ) which calls
• static int serout(int pin, int pin_out, int pin_pull, int delay_table[])
  )

the issue is not only about readability, functions that declare their input arguments and return values are easier to test (and the code for reading/checking values could be extracted into a macro and reused by many modules - which would be actually saving space)

[TerryE]

I agree that it is well worth while enforcing a clear split by doing all of the API argument validation first, the execute the functional bit.

There is the issue of calling overhead both in terms of extra generated lx106 code and execution time to handle the procedural call overheads, but these can effectively removed by declaring the execution function first with an static inline attribute in which case the gcc compiler effectively treats this as a macro and you lose this overhead. It the routine then subsequently needs to be exported as an external function, it's then a simple issue to remove the inline declaration.

[marcelstoer]

Terry, I agree with you but is it worth keeping this issue open? When would be a good time to close it? Clearly we don't want to keep it around until all loose coding issues in all modules are fixed?

I suggest we close this and keep an eye on the issue with every PR that contains material changes.

[marcelstoer]

Bump. Don't wanna close your issues but I still think it should be closed.

[djphoenix]

I think the best case is to use linter like cpplint, this is good enough to get readable code (consistent tab/space struct, maximum line width and more). Also configuration for e.g. Eclipse Code Formatter will be good point. Anyway I prefer automated tools more than "guidelines".

[marcelstoer]

Yuri, that's a valid input - for #1168. Look at Terry's initial example here. That's not something a linter could "fix".

[karrots]

While eslint doesn't lint C/C++ code it is an example of a linter which will fix and/or identify exactly the issue. mentioned in JavaScript. Assuming I understand your inference correctly.

http://eslint.org/docs/rules/curly

[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]

Don't wanna close your issues but I still think it should be closed.

Nah, this is still very much the case. I am going through the modules ATM as part of the Lua51 + Lua53 integration and keep coming across this crap. It just adds noise to the modules that we don't need. For example every module which returns a UData resource like a socket registers a registry entry, say tls.socket, and uses a luaL_checkudata() on method entry to check that the US is of the correct type -- that is it has a metatable with is as required. A type error is thrown if there is a mismatch so the execution never gets to the following line if the userdata is invalid. So why do stuff like:

  luaL_argcheck(L, ud, 1, "TLS socket expected");
  if(ud==NULL){
      NODE_DBG("userdata is nil.\n");
      return 0;
  }

This is all _totally_ redundant noise -- _doubly_ redundant in this case -- and should be stripped out.

Likewise in the case of

  const char *method = luaL_checklstring( L, 2, &sl );
  if (method == NULL)
    return luaL_error( L, "wrong arg type" );

luaL_checklstring() either returns a pointer to a string _or_ throws an error. It can _never_ return NULL so why have the if statement and luaL_error()?

[TerryE]

What @nwf or I will do is a debris sweep to remove this crap.

[TerryE]

Reserving luaXXX names for the Lua runtime

A bunch of modules (bmp085.c, dcc.c, tsl2561.c, bme680.c, ads1115.c, si7021.c, tcs34725.c, bme280.c) create their own variables and functions with the lua prefix. All modules should reserve luaXXX for the lua API.

Call | bmp085.c | tsl2561.c | bme680.c | ads1115.c | si7021.c | tcs34725.c | bme280.c | Total Result
---|---|---|---|---|---|---|---|---
lua_altitude() | | | | 1 | | | | 1
lua_baro() | || | | | | | 1 | 1
lua_calclux() | | 1 | | | | | | 1
lua_dewpoint() | | | | 1 | | | | 1
lua_firmware() | | | | | 1 | | | 1
lua_getchannels() | | 1 | | | | | | 1
lua_humi() | | | | | | | 1 | 1
lua_read() | | | 1 | 1 | 1 | | 1 | 4
lua_read() | | | 1 | | | | 1 | 4
lua_setting() | | | | 1 | 1 | | | 2
lua_setup() | | | 1 | | 1 | | 1 | 4
lua_startread() | | | | 1 | | | | 1
lua_startreadout() | | | | 1 | | | | 1
lua_temp() | | | 1 | | | | 1 |
lua_temperature() | 1 | | | | | | | 1
luaSetGain() | | | | | | 2 | | 2
luaSetIntegrationTime() | | | | | | 2 | | 2

The wifi modules also use lua_CB plus C variables

[TerryE]

A few other peeves of mine:

• luaL_unref patterns. We have lots of the following coding pattern. This should be collapsed into a single line with a macro.

      luaL_unref(L, LUA_REGISTRYINDEX, ud->client.cb_connect_ref);
      ud->client.cb_connect_ref = LUA_NOREF;

• luaLreref pattern. We have lots of the following coding pattern. This should be collapsed into a single new luaxlib call. lua_reffunc(L, N, &func_ref), so that a type error is thrown if stack[N] is not a function; if Reg[&func_ref] exists then this slot is reused (rather than the more expensive unref + ref) otherwise a new registry slot is used. func_ref is update with the reference index. Less source code and faster execution.

    luaL_argcheck(L, lua_isfunction(L, N), N, "not a function");
    lua_pushvalue(L, N);
    luaL_unref(L, LUA_REGISTRYINDEX, func_ref);
    func_ref = luaL_ref(L, LUA_REGISTRYINDEX);

• Userdata selfrefs. None of the Lua libraries do this. I need to think about the architectural drivers of this use with the registry. I am not sure that we are doing this the Lua way. Needs more thought.

[TerryE]

Userdata selfrefs

More thoughts on what makes me uneasy about these: any form of denormalisation (storing stuff twice) makes me uneasy. You have to add extra code to the extra store and GC, plus code to handle exception cases when the two aren't the same. Why add all of this complexity? It's not the Lua way. So why so we need to do this. OS CBs typically require a context to do their job. The standard model is to pass the &UD as this context, but if they need to clean up the registry then they need to know the parent registry slot that holds the UD. So why not just use the registry index, the slot number as the context? This makes the CB code a lot simpler.

[TerryE]

Another on on my TODO list is to make is that various lib_init entries for modules create one or more metatable references such as net.udpsocket and which the luaLcheckudata() function uses to bind a ROTable metatable to a socket-style Udata. If takes less code to do this binds lazily in the socket create function, and this remove any unused meta references from the registry.

[nwf]

@TerryE: whether we use the userdata pointer or a registry index as the callback parameter stored in the platform C layer doesn't matter. The ref in the registry needs to exist to keep either of those things meaningful; whether or not that ref is stored as a self-ref in the userdata structure is semantically immaterial. Syntactically, using the userdata pointer itself as the callback parameter means that some platform callbacks can avoid going to Lua to manipulate state (e.g., the on_sent callbacks, if there's no on:("sent") callback registered on the Lua side).

The "problem", at its core, and the reason that the Lua libraries don't make self-refs like we do, is that in a Lua program without registry-like shenanigans going on,

tmr.create():alarm(..., function() ... end)

would create an object, update some properties of it, and then immediately drop the reference on the floor and let the object be GC'd. Now, maybe in Lua one would have tmr.create() secret away some internal table of things it's created, but that's just... the registry in a different guise. This is what I was getting at in #3062.

The appropriate point of comparison would not be the Lua standard libraries, but rather other embeddings of Lua into larger systems with persistent state. Maybe some game engine such as Love. But I'd be willing to wager $1 that they either manage objects' lifecycles outside the Lua GC or create self-refs for precisely this reason.

[nwf]

I'd like to propose that luaL_reref leave the stack management to the caller, or at least, that there be a variant that does. Otherwise, code like this

  switch (luaL_checkoption(L, 1, NULL, names)) {
    case 0:
      luaL_reref(L, 2, &ud->cb0_ref);
      break;
    case 1:
      luaL_reref(L, 2, &ud->cb1_ref);
      break;
    // ...

risks duplicating all the luaL_argcheck and luaL_pushvalue calls if the optimizer can't see through the luaL_checkoption call.

Obviously I wasn't awake when I wrote that. The correct thing to do is

  int *refp;
  switch (luaL_checkoption(L, 1, NULL, names)) {
    case 0: refp = &ud->cb0_ref; break;
    case 1: refp = &ud->cb1_ref; break;
    // ...
  }
  luaL_reref(L, 2, refp);

[TerryE]

The correct thing to do is ...

_Very_ few of our calls can set more that one CB depending on parameters and if we only have two options then surely the simplest way to code this is:

int optn = luaL_checkoption(L, 1, NULL, name);
luaL_reref(L, 2, optn == 0 ? &ud->cb0_ref : &ud->cb1_ref);

[TerryE]

The "problem", at its core, and the reason that the Lua libraries don't make self-refs like we do ...

@nwf, I agree that the architectural difference is that we have SDK-triggered CBs.

In general my view is that we should only denormalise when there is a compelling reason to do so, for example there is a material performance benefit or we need to denormalise because this is the only way to access the parameter on some code paths. The downside of denormalising is that we need extra code to handle set-up, clean-up and we also need to maintain transactional integrity on update. The issue of potential loss of integrity between the two copies means that the two approaches are _not_ semantically the same on some on _error paths_. We avoid this risk / code complexity if we don't denormalise.

PS Edit: My comment below gives the compelling reason in this case. I am therefore striking out this example.

In terms of avoiding "going to Lua", off the top of my head, I can't think of any that don't subsequently drop into the Lua work to make a Lua CB -- at least on main path -- so we will be doing a lua_getstate() anyway to return L. Take the case of the tmr module, in the case of its OS CB, alarm_timer_common(): is the following any worse than the current version?

(Example removed)

PS. This version also uses the luaL_unref2() macro to simplify the source and the luaL_pcallx() variant to provide error handling instead of panic around the Lua CB. It also passes the tmr as an argument to the CB as all CBs should, so the CB doesn't need to use an upval to access the tmr object.

[TerryE]

Another point:

• All Lua CBs should include the socket / instance object as arg 1. This is in general the case, but there are a few exceptions. for example if you want a tmr alarm to restart the timer then the Lua function must use a upval reference to the timer. Why? This just creates the potential for a resource leak in the registry. In most cases this does not introduce a backwards compatibility code break.

[nwf]

Holding on to an int for the callback state makes me a little jittery, I admit, but maybe you're right that it's OK... I suppose if we get the lifecycles wrong, tmr_t t = (tmr_t)luaL_checkudata(L, -1, "tmr.timer"); in your example will cause a panic if the registry entry isn't a "tmr.timer"? That's probably good and makes debugging easier.

I'll see what it does to mqtt when I have time.

[TerryE]

Holding on to an int for the callback state makes me a little jittery,

What we are really saying is that the state will _always_ be in the Registry at slot n where n is this state handle.

... will cause a panic if the registry entry isn't a "tmr.timer" ...

The CB is an internal routine to the tmr module and it should only point to a "tmr.timer" UData. The only other option that I could think of is to replace the !lua_isnil(L, -1) test with a lua_isuserdata(L, -1) one. If it is a Udata != "tmr.timer" then we should hard error.

[TerryE]

@nwf, re my above comment

is the following any worse than the current version?

I am being dumb, in that in this usage tmr.create():alarm(2000,0,nextStage) there isn't any reference to the tmr object in the calling Lua code so UD must be in the registry to prevent the Lua GC will collect it whilst still active, and calls like t:unregister() need to remove the registry entry from the UD conext and so it _must_ contain the self reference. Sorry for the mindfart.

[TerryE]

One obvious feature of the C API compared to how our module use it is that while many API calls are type void because there is nothing sensible that they can return, a lot are type int because they do return something sensible; for example all of the table access functions which push a table value onto the stack also return the _type_ of that value, so perhaps 50% of the times in the code where we do a lua_type() call are avoidable.

I am guilty of this one myself 😟

[TerryE]

Some modules use "non-public" C APIs, that is other than lua.h andlauxlib.h`, for example

• lmem.h This isn't strictly part of the public API but I have previously advocated its use in preference to the standard malloc. Lua provided the userdata API for this. Modules which import this are color_utils, cron, crypto, encoder, enduser_setup, file, gpio, gpio_pulse, hx711, net, node, somfy, tls, websocket, ws2812 and ws2812_effects
• node.c includes a couple, but I can tidy these up.

What I have done with a few API calls added to support NodeMCU, and which are genuinely part of the public API is to give these a lua_ or luaL_ prefix and add them to lua.h andlauxlib.h`.

My inclination is to add lua_malloc, lua_realloc, and lua_free to lua.h and remove lmem.h use. These correspond to malloc, realloc, and free (excepting the addition L argument), but will trigger the Lua GC and retry if the allocation fails.

[nwf]

I believe some of the use of lmem is removed in recent PRs, esp. #3158 (ws2812) and #3159 (cron). tls's use of luaM_malloc is in the gross flash-based cert store that I've been trying to eliminate (#3032). ws2812_effects is hopefully not long for the world, either.

You're welcome to leave those offences stand and let them get fixed in time, if you like.

In the interest of differentiating "core" and "NodeMCU" lua, maybe name them luaN_malloc and friends?

[TerryE]

Another placeholder that I want to on record, but one that probably only @nwf and @jmattsson is interested in -- and hence this comment rather than a separate issue.

• Modules can include a "open" function as a hook in their NODEMCU_MODULE declaration and the Lua RTS will call this hook at start-up.
• The Lua RTS has a "take only pictures; leave only footprints" approach to resource management in that the RTS close-down will attempt to release all Lua resources used. Any tests will fail when running (host) test build, if this isn't the case.
• Any module's open procedure which only creates Lua resources (e.g. just doing luaL_rometatable() calls) will comply with this.
• The fly in the ointment here is that some modules (cron, dcc, file, gpio, gpio, gpio_pulse, hx711, net, node, pcm, pwm2, pwm, rotary, sjson, sntp, softuart, somfy, switec, tmr, wifi) do initialisation. Most do stateful initialisation and allocate RAM resources. What I haven't had time to do is to analyse these on a case by case basis to establish whether repeating the startup hook is (a) safe; (b) non-erroring but results in RAM resource leakage or (c) can crash the restarted system.
• An example here is that if a module function registers an SDK CB, then this is bound to a Lua CB through a registry entry. This entry is lost when the Lua VM is restarted, but the SDK is still running so the SDK CB could still be called but be unable to hook to a valid Lua CB. This might or might not cause a hard error.
• What this Lua open + close cycle provides is a relatively lightweight method of restarting the Lua VM without having to restart the CPU itself, which could be useful during LFS provisioning for example. Such a lightweight "bounce" also enables context to be passes from one load in RAM rather than RCR, and I have taken advantage of this in Lua53. However on reflection, I have decided that assuming that we can do a lightweight bounce is too unreliable, and I therefore decided to rework this and to do a CPU restart using the RCR for context.

As an corollary, I note that many users will include a range of modules for their firmware build 'just in case' but then only use a subset for any specific application usecase. Given this, I now think that it is a better practice for ESP applications to do _just-in-time_ initialisation, wherever this is practical, so we should discourage using the initialisation hook. For example most modules have an entry init or open call that the application calls to return a resource object. If we adopt the practice of using an "on first call" test (e.g. if the module uses task.post task ID then do the initialisation if this ID is zero), then unused library modules will have a zero RAM footprint and only use flash resources.

@nwf Nathaniel, as an example of this last point using TLS incurs a high RAM footprint. Is this still the case if no TLS connections are initiated? If so, then this would severely limit the app developer's ability to generate on-ESP LFS images.

[nwf]

The tls module does nothing during open except to install the tls.socket rotable. The high persistent overhead mostly comes from the TLS fragment buffers for a connection (2 * 4K) and the very high peak occupancy comes from the buffers necessary to cache and process X.509 certificates. My hope is that #3032 and the use of DER-encoded certs in LFS can drive the latter down a bit (by keeping the public cert(s) and private key in flash), but that's a separable concern. Anyway, no, just having tls present doesn't terribly exacerbate the memory crunch.