pkg/lua: Better Lua-RIOT integration by jcarrano · Pull Request #9153 · RIOT-OS/RIOT

jcarrano · 2018-05-18T15:48:49Z

Better integration of the Lua interpreter with RIOT

The main idea behind this PR is that interpreted code should never be allowed to crash the host application. A secondary goal is to cover the majority of use cases in a few simple API calls, to remove the required boilerplate and make it easier to use the Lua package correctly.

The lua intepreter itself, with some custom modifications:
- Patches to reduce C-stack usage.
- Modifications to the test module to enable it to work on RIOT.
- The vanilla module loader is disabled as it is too dependent on files and dynamic loading.
A new module loader intended to load source code and C-modules built-into the application.
A custom memory allocator based on TLSF. This allows to limit the memory that Lua can consume and to not have memory leaks in case of an interpreter crash.
A panic handler based on setjmp()/longjmp(). Thus, in case of a irrecoverable failure in the interpreter, control returns to the host application (by default, lua calls abort())
An example with an interactive Lua REPL.
Procedures to easily launch Lua and handle errors.

Note that only 32 bit CPUs will be supported.

Future work

The next PRs will focus on build system integration (i.e. being able to compile %.lua into an application).

About the Lua patches

I'm patching lua, in a way that is perfectly compatible with the unmodified code. I'm running the test suite, and it passes. Patches are still patches, and they will never get merged into upstream lua, simply because the Lua people don't accept outside submissions (check the FAQ for more info). This is a reality we have to accept. Other projects do the same, they apply their own modifications.

Issues/PRs references

Extends: #8847
Depends on: #9090 #9006 #9244

smlng · 2018-05-28T09:34:00Z

please rebase

jcarrano · 2018-06-04T07:45:00Z

Why is the Murdock state stuck in "Waiting for status to be reported"?

cgundogan · 2018-06-04T07:50:04Z

@jcarrano Murdock only starts the build, once the ready for CI build flag is set

jia200x · 2018-06-11T11:48:01Z

cpu/saml21/periph/pm.c

                DEBUG("pm_set(): setting STANDBY mode.\n");
                _mode = PM_SLEEPCFG_SLEEPMODE_STANDBY;
                break;
+            default:


This should be in a separate PR (gcc7 complaining?)

Yes, it is in #9244. As soon as that is merged, I will rebase and that change should disappear.

jia200x · 2018-06-11T11:48:43Z

examples/lua_REPL/Makefile

+# development process:
+DEVELHELP ?= 1
+
+CFLAGS_OPT = -Og


To make it easier to debug the lua implementation and find the exact lines where error appeared. I can remove it.

I would either remove it or add a comment on top

jia200x · 2018-06-11T11:49:46Z

examples/lua_REPL/main.c

@@ -0,0 +1,50 @@
+/*


maybe this could be in the same lua example. What do you think?

(basically the LUA Repl implementation is running LUA. So, should be enough to show how it works)

Yes, the thing is the REPL requires more resources, so it runs on less boards (part of that is because I'm enabling the lua test module).
My idea was to have the lua example showcase the basic API, and another example with a more complex app.

check the OpenThread test at #7149. It's possible to switch between Full Thread Device and Minimum Thread Device with a Makefile flag.

Are you suggesting I test the type of device and run the REPL or the Hello World accordingly?

ok, let's leave it like that then. We can discuss it later in further PRs and don't block this one

(or what @jcarrano thinks the best)

+1 for separate examples for clarity reasons. Could rename example/lua to example/lua_hello-world or something. In general I think it's more important that the examples are all concise, useful and non-overlapping than that they are simply small in number

I like @danpetry approach

This is corrected

jia200x · 2018-06-11T11:51:36Z

pkg/lua/Makefile

 PKG_NAME=lua
 PKG_URL=https://github.com/lua/lua.git
-PKG_VERSION=e354c6355e7f48e087678ec49e340ca0696725b1
+PKG_VERSION=v5-3-4


is this v5-3-4 a git identifier?

Yes, it is a tag.

The majority of packages use a commit hash for this. Some use a version number. Personally I think a version number is more readable, and don't know of any disadvantage to using that.

This has gone back to a commit hash, because tags can change.

jia200x · 2018-06-11T11:53:36Z

pkg/lua/contrib/binsearch.c

+        else /* (cmp > 0) */
+            lo = mid + 1;
+    }
+    return BINSEARCH_NOTFOUND;


if possible, prefer errno types

I searched, but I cannot find an errno code that matches (they concern mostly external errors). Anyways, there is only one error that can be thrown by a search routine: "not found".

ENOTFOUND should do the trick :)

ENOTFOUND is not in errno.h and is not defined in RIOT either.

you can use ENOENT as seen here

Mmmhh, I disagree with that usage. The definition says it means "No such file or directory". I'll change anyways, if it is a sort of convention in RIOT to use it that way.

ENOENT stands for Error No ENTry (or Error No ENTity), as seen here. It's usually described as "No such file or directory", but can be used for more cases

jia200x · 2018-06-11T11:55:13Z

pkg/lua/contrib/lua_loadlib.c

+
+/* ======================== 'require' function ============================= */
+
+


please remove extraline

jia200x · 2018-06-11T11:55:24Z

pkg/lua/contrib/lua_loadlib.c

+    }
+}
+
+


and here :)

jia200x · 2018-06-11T11:58:21Z

sys/newlib_syscalls_default/syscalls.c

 }
 #endif /* MODULE_VFS */

+/**


can you add these functions in a separate PR?

It's in #9090

#9090 is merged now, please rebase.

This is rebased

jia200x · 2018-06-11T13:22:16Z

pkg/lua/include/lua_builtin.h

+#ifdef __cplusplus
+extern "C" {
+#endif
+


double empty line here

We really need a tool to find these.

that's the idea of #8519. Hope it gets into master soon

This is corrected

jia200x · 2018-06-11T13:32:18Z

pkg/lua/contrib/lua_run.c

+    return tlsf_realloc(tlsf, ptr, nsize);
+}
+
+LUALIB_API lua_State *luaR_newstate(void *memory, size_t mem_size,


@jcarrano @danpetry I'm not sure about these luaR_xxx functions. Are these Lua or RIOT specific? According to our Coding Conventions, RIOT code should be always snake_case( It's OK to call CamelCase functions though if they are defined in the original pkg)

What do you think?

(it's also OK to write CamelCase when it's the implementation of a function required by the pkg, but couldn't find any luaR functions reference on internet)

There is no "nice" solution. I chose those names because those are replacements for luaL_newstate, lual_openlibs, etc, and the convention in lua is to have lua_xxxx for core functions and luaY_xxx for other functions.

I tend to think this code should follow RIOT standards, because it's an adoption for RIOT.
Maybe using wrappers as seen here can do the trick.

Note is always possible to directly invoke pkg functions with their own style, but this code will live in Doxygen and strictly attached to the RIOT side

There is still the rest of the lua api, named after lua conventions. Then I would have mixed conventions in the same code (lua_riot_newstate returning a lua_State).

My opinion is that it would be best to follow the RIOT conventions as far as possible. If the API is RIOT programmer-facing, then it would avoid the case where e.g. there are a bunch of different packages with their own coding style and the programmer sees a range of different styles. I think leaving in stuff which has to be left in is acceptable (and obv necessary) but better than having a real mix of styles

I strongly agree with @jia200x and @danpetry here to follow RIOT coding conventions. mixing conventions is something that is often caused by external software. I would not wrap everything, but having at least our own contributed functions in our own convention also makes it easier for seasoned RIOT users as they know what to expect and prevents confusion with new users

Rewording our own functions to something like lua_riot_funcname is IMHO clear enough to indicate that it is our version of funcname.

(I think I accidentally rephrased @danpetry's arguments)

This has been changed

jia200x · 2018-06-11T13:36:46Z

pkg/lua/include/lua_run.h

+ */
+LUALIB_API lua_State *luaR_newstate(void *memory, size_t mem_size,
+                                    lua_CFunction panicf);
+


double empty line

jia200x · 2018-06-11T13:41:31Z

examples/lua_REPL/main.c

@@ -0,0 +1,50 @@
+/*


I like @danpetry approach

jia200x · 2018-06-11T13:42:02Z

examples/lua_REPL/README.md

@@ -0,0 +1,20 @@
+### About


Could you provide a readme with the available commands? I ran the example but don't know what to do :)

I suggest a link to the lua programming guide and a brief rundown of what's supported, in the package API reference, and then a note here linking to there?

@jcarrano pointed out that it's not our place to teach people to program in Lua (we wouldn't link to a JavaScript tutorial in the jerryscript pkg for example). Worth noting that at the moment we haven't done any bindings. The only officially supported function is print() as far as I'm aware

Link to the lua page and some brief info "works in the same way as the lua shell in the default installation" has been added

jia200x · 2018-06-11T13:43:18Z

examples/lua_REPL/Makefile

+# This has to be the absolute path to the RIOT base directory:
+RIOTBASE ?= $(CURDIR)/../..
+
+BOARD_WHITELIST := frdm-k64f iotlab-m3 mulle nrf52840dk nucleo-f411re \


this test runs on native too. You should add it to BOARD_WHITELIST

jia200x · 2018-06-11T14:15:24Z

@jcarrano I set the Waiting For Other PR label in order to focus on the blocking PRs (#9090 and #9244)

danpetry · 2018-06-11T14:49:22Z

pkg/lua/Makefile.lua

+          -DL_MAXLENNUM=50
+#    Enable these options to debug stack usage
+#          -Wstack-usage=128 -Wno-error=stack-usage=128



Note that now the examples build with warnings...
@jcarrano @jia200x don't want to do a blocking comment but wanted to highlight this

If you are referring to this:

loslib.c:(.text.os_tmpname+0x29): warning: the use of `tmpnam' is dangerous, better use `mkstemp'

There is no standard C way of fixing it, without changing the OS module's API. Asking for a filename and then creating the file is a bad idea, but that's how the lua standard library is made to work.

@jcarrano is it possible to patch that file?

Is CFLAGS += -DLUA_USE_LINUX a bad idea? eg because it provides a non-representative platform or something?

It is already patched. Here is what happens:

The os module contains a function os.tempname which should return a filename suitable for a temporary file.

tmpnam is a C function that was supposed to do this before people realized that it's not good to generate a filename without creating the file at the same time: it exposes you to a race condition, the file could be created by someone else in the meanwhile.

mkstemp is an alternative. However, it is not C-standard and not really compatible with the way os.tempname works. What unpatched lua does (if LUA_USE_LINUX is defined) is to create a file and then close it (it does not even delete it).

Conclusion: it is a bug in the Lua API.

Cool, worth documenting somewhere? Worth starting maybe a wiki page on Lua for RIOT or something which can grow?

I tend to prefer Doxygen pages (or similar), since the wiki is hard to maintain. See #8516

Ok, can we then:

remove os.tmpname using the same approach as used with os.rename and os.clock (see first patch)

record that these three functions aren't supported in doc.txt

remove os.tmpname using the same approach as used with os.rename and os.clock (see first patch)

record that these three functions aren't supported in doc.txt

Done

Great, thanks.

danpetry

Cool stuff especially with the REPL loop which is shows a few neat things. Most of the comments are about documentation and generally making the examples as useful as possible. Have done a full review of examples/, so far. Will look at pkg/ next.

danpetry · 2018-06-21T13:43:07Z

sys/newlib_syscalls_default/syscalls.c

 }
 #endif /* MODULE_VFS */

+/**


#9090 is merged now, please rebase.

danpetry · 2018-06-21T13:47:38Z

examples/lua/Makefile

@@ -29,12 +29,10 @@ DEVELHELP ?= 1
 # Change this to 0 show compiler invocation lines by default:
 QUIET ?= 1



Could you please rename this example to lua_hello-world or something similar, as discussed below?

OK. I'm thinking "lua-basic".

sounds good

has been changed

danpetry · 2018-06-21T13:59:12Z

examples/lua/Makefile

  # various lengths. Other functions untested.
-  CFLAGS += -DTHREAD_STACKSIZE_MAIN=4096
+  CFLAGS += -DTHREAD_STACKSIZE_MAIN=3000
 endif


Just wondering where the number came from and whether there is planned any quantifying of how much memory is required for which functions? Might be good to start a set of general guidelines for how much memory is required for certain applications or something and refer to this variable.

It's pretty empirical, trial and error .I don't like that magic number either. I also wanted to have a nice power-of-two number. Right now there's no easy way to get this number, and it's architecture-dependent, which means testing in one CPU doesn't tell you anything about the others. In the future we can instrument the lua functions to record the maximum stack usage, and then the user can choose a decent safety margin.

A complementary solution is to just reduce usage so much that it is no longer a problem.

Would it be possible to rewrite it to something like
CFLAGS += '-DTHREAD_STACKSIZE_MAIN=(THREAD_STACKSIZE_DEFAULT + 2048)'
To keep it somewhat architecture dependent?

@bergzand Good one, I didn't know about that define.

danpetry · 2018-06-21T14:28:23Z

examples/lua_REPL/Makefile

+DEVELHELP ?= 1
+
+# Uncomment the following lines to enable debugging features.
+#CFLAGS_OPT = -Og


This optimizes for debugging right? Just curious as to what exactly it does?

It tries to achieve a decent level of optimization, but it leaves out any optimization which could interfere with debugging. If you compile with -O2 and then try to debug, you get all kinds of weird jumps.

danpetry · 2018-06-21T14:32:45Z

examples/lua_REPL/README.md

@@ -0,0 +1,20 @@
+### About


I suggest a link to the lua programming guide and a brief rundown of what's supported, in the package API reference, and then a note here linking to there?

danpetry · 2018-06-21T16:37:46Z

examples/lua_REPL/repl.lua

+        maybe_code, compile_err = load(ln)
+    end
+
+    if maybe_code == nil then


if not maybe_code then ?

danpetry · 2018-06-21T16:40:34Z

examples/lua_REPL/repl.lua

@@ -0,0 +1,91 @@
+--[[


As you mentioned it is probably not necessary to be thorough with the Lua style guide. One thing I did notice, though, is that indents are generally two spaces. This would also reduce the array size a bit.

Another note for the future: write a "partial minifier" that preserves line numbers and variable names.

Two-space indents make me want to rip off my eyes.

ok. That's a fair reaction tbh

danpetry · 2018-06-21T16:44:03Z

examples/lua_REPL/main.c

+
+#define MAIN_LUA_MEM_SIZE (40000)
+
+char lua_memory[MAIN_LUA_MEM_SIZE] __attribute__ ((aligned (__BIGGEST_ALIGNMENT__)));


also, space between aligned and (__BIGGEST_ALIGNMENT__)))

Sorry, remove the space I meant (looked at the wrong diff from uncrustify)

danpetry · 2018-06-21T16:45:06Z

examples/lua_REPL/main.c

+
+char lua_memory[MAIN_LUA_MEM_SIZE] __attribute__ ((aligned (__BIGGEST_ALIGNMENT__)));
+
+#define BARE_MINIMUM_MODS (LUAR_LOAD_BASE|LUAR_LOAD_IO|LUAR_LOAD_CORO)


space between these constants, i.e. (LUAR_LOAD_BASE | LUAR_LOAD_IO | LUAR_LOAD_CORO)

danpetry · 2018-06-21T16:47:20Z

examples/lua_REPL/main.c

+                                BARE_MINIMUM_MODS, &value);
+
+        printf("Exited. status: %s, return code %d\n", luaR_strerror(status),
+                                                        value);


Does this need a newline? if so, should be aligned with the opening bracket

danpetry

Have reviewed the binsearch functionality and package makefiles

danpetry · 2018-06-22T13:18:59Z

pkg/lua/Makefile

 PKG_NAME=lua
 PKG_URL=https://github.com/lua/lua.git
-PKG_VERSION=e354c6355e7f48e087678ec49e340ca0696725b1
+PKG_VERSION=v5-3-4


The majority of packages use a commit hash for this. Some use a version number. Personally I think a version number is more readable, and don't know of any disadvantage to using that.

danpetry · 2018-06-22T13:19:46Z

pkg/lua/Makefile

 .PHONY: all

-all:
+all: Makefile.lua


Just wondering why this is added/what it does?

It forces a rebuild if the lua makefile changes.

danpetry · 2018-06-22T13:37:22Z

pkg/lua/Makefile.lua

+          -DL_MAXLENNUM=50
+#    Enable these options to debug stack usage
+#          -Wstack-usage=128 -Wno-error=stack-usage=128



Ok, can we then:

remove os.tmpname using the same approach as used with os.rename and os.clock (see first patch)

record that these three functions aren't supported in doc.txt

danpetry · 2018-06-22T14:30:28Z

pkg/lua/contrib/binsearch.c

@@ -0,0 +1,48 @@
+/*


For the record we just talked about this being generic and potentially going into /sys. However, it's not really clear how useful it would be outside of pkg/lua and there are issues re implementation discussed here #8974 which basically mean it's unclear how to proceed when making it part of /sys. So, it's not worth the effort and the decision is to keep it here.

danpetry · 2018-06-22T15:53:36Z

pkg/lua/contrib/binsearch.c

+ * directory for more details.
+ */
+
+/**


please run uncrustify, there are a few things here. also in the corresponding .h file

danpetry

Ok cool I have reviewed this. Good work. A few really neat bits in here. The stuff to do with changing the allocator and the library loading will be particularly relevant IMO.

Most of the review comments fall into three broad categories:

Could you please describe how the changes were tested. Ideally providing the means (instructions, code if possible) to replicate the test. If this is just verified by compiling or not crashing, this is fine if it's described
Could you please fill out the documentation, in particular the overview in doc.txt (incl a high level rundown of changes that were made, esp Lua API changes); and also please add Doxygen comments to any functions/structs/etc that were added to the Lua C API (i.e., the luaR_ stuff).
Please clean up the code somewhat, including making it easier to scan to get to know what's going on, and also run uncrustify, which should come out with zero changes.

After these things are done (and the described tests have passed) it should be ready to merge.

danpetry · 2018-06-25T14:10:12Z

pkg/lua/contrib/lua_loadlib.c

+
+static const luaL_Reg ll_funcs[] = {
+    { "require", ll_require },
+    { NULL, NULL }


As with os.tmpname etc, could we document, in one place, the difference between the original Lua API and our API? This would make it easier for people to pick up and use out-of-the-box.

Done (in doc.txt). Of course, it's a coarse readme, we need proper docs, with sections and multiple pages and code snippets and all.

Yes. the doc.txt is great for now, thanks.

danpetry · 2018-06-25T14:36:13Z

pkg/lua/contrib/lua_loadlib.c

+}
+
+/* ======================== 'require' function ============================= */
+


Think this is supposed to go below searcher_preload? Ok to tidy up a bit the grouping/commenting of the functions here in general (e.g. The "search in the list of c lua modules" could go above _ll_searcher_builtin_c, luaR_getloader could go at the bottom)

Do we have a specific rule for function placement?

Not really, fine with your implementation.

danpetry · 2018-06-26T12:26:26Z

pkg/lua/contrib/untitled

@@ -0,0 +1,3 @@
+MODULE := lua-contrib


Should this file be removed?

danpetry · 2018-06-26T12:41:06Z

pkg/lua/contrib/lua_run.c

+                            const char *modname, void *memory, size_t mem_size,
+                            uint16_t modmask, int *retval)
+{
+    jmp_buf jump_buffer;


This function could be cleaned up, I think. It could be much easier to see at first glance what's going on

What do you mean by cleanup?

It's fine now. Things look better having uncrustified

danpetry · 2018-06-26T12:43:53Z

pkg/lua/contrib/lua_run.c

+    }
+
+    switch (compilation_result) {
+    case LUAR_MODULE_NOTFOUND:


Is there a cleaner way to do this? E.g. if you match the numbers of the Lua errors to the LuaR errors or something?

It would be more like moving the dirtiness to somewhere else. I don't like to rely on specific values of enums, and I'd rather treat them as symbols.

danpetry · 2018-06-26T12:55:30Z

pkg/lua/patches/0001-Remove-luaL_newstate.patch

+index 7b14ca4d..018a4347 100644
+--- a/lauxlib.c
+++ b/lauxlib.c
+@@ -1005,31 +1005,6 @@ LUALIB_API const char *luaL_gsub (lua_State *L, const char *s, const char *p,


As discussed, could you please remove this patch after rebasing?

Done (for some reason this comment is placed in the wrong file).

danpetry · 2018-06-26T13:11:34Z

pkg/lua/contrib/lua_loadlib.c

+    }
+}
+
+int luaR_getloader(lua_State *L, const char *name)


Please could you add Doxygen comments to all the functions that have been added to the Lua C API?

See the header file.

danpetry · 2018-06-26T13:13:07Z

pkg/lua/patches/0006-Cleanup-test-module.patch

+This patch makes several improvements to the test module intended
+to allow testing inside embedded environments:
+
+- It does not rely in the standalone intepreter (lua.c).


Could you describe how to test the changes (even better, provide test snippets)? If, for any of the changes, this is simply something like "the interpreter doesn't crash" or "it compiles" then that's ok too but please describe.

Maybe I should add a link to the repo in which I have the modified Lua? I can be downloaded in a desktop and compiled just like normal lua (only better, because mine builds the debug and non-debug versions simultaneously), and can run the test suite just like normal lua.

danpetry · 2018-06-26T13:23:33Z

pkg/lua/patches/0007-Add-a-proper-makefile.patch

+index 00000000..2735db8d
+--- /dev/null
+++ b/Makefile
+@@ -0,0 +1,116 @@


Might be worth checking with Gaetan whether these changes are in line with the work he's doing on the build system. In other words, whether they incorporate the work he's doing or do things in a different way

This makefile is not being used (it is overwritten). The patch is there because it's useful if one wants to compile the standalone interpreter.

danpetry · 2018-06-26T13:28:22Z

pkg/lua/contrib/lua_run.c

+        goto luaR_do_error;
+    case LUA_ERRMEM: /* fallthrough */
+    case LUA_ERRGCMM:
+    default:


please also uncrustify. here and throughout the rest of the submitted code.

Ok. One thing I'll keep, though, is the indentation of the code between #ifdefs. I find it quite hard to follow conditionals if they are not indented, and I don't see how a compile-time conditional would make a difference.

tcschmidt · 2018-06-27T19:31:15Z

@danpetry are comments of @jia200x addressed? Can we remove the blocking?

bergzand

@jcarrano Some comments on readability, conventions, code duplications and other questions that I had when reading the code. I didn't test the code here and I did not check the changes from the new patch files.

In the future I would strongly recommend splitting PR's like these as it looks to me like a number of distinct changes that could be supplied in separate PR's. This to make reviewing easier (a PR of +2763, -73 spanning 30 different files is imho not very nice to review)

bergzand · 2018-06-27T20:01:40Z

examples/lua_REPL/Makefile

+
+	xxd -i $< | sed 's/^unsigned/const/g' > $@
+
+$(RIOTBUILD_CONFIG_HEADER_C): $(LUA_H)


This looks like duplicate code from examples/lua_basic/. Could you please deduplicate it in a makefiles/lua.mk or something like that?

Yes, but in a future PR. Duplication is only one of the issues. That way of generating code is not very robust, but this change I CAN split, so I will.

bergzand · 2018-06-27T20:02:40Z

examples/lua_REPL/Makefile

+# Change this to 0 show compiler invocation lines by default:
+QUIET ?= 1
+
+# This is a lot of stack, should be fixed in the future.


Could you please clarify this comment a bit, what does "fixed" mean here, is the large stack size a problem that should be resolved in the future or do you mean that it is something of fixed size.

See the package's doc.txt

bergzand · 2018-06-27T20:03:15Z

examples/lua_REPL/Makefile

+# Change this to 0 show compiler invocation lines by default:
+QUIET ?= 1
+
+# This is a lot of stack, should be fixed in the future.


Maybe reword the beginning to "LUA requires a huge stack,..."

bergzand · 2018-06-27T20:04:54Z

examples/lua_REPL/Makefile

+DEVELHELP ?= 1
+
+# Uncomment the following lines to enable debugging features.
+#CFLAGS_OPT = -Og


Is this only useful in this example, if not, would you mind reworking it to a PR to a more generic solution where it is for example enabled with a variable, similar how DEVELHELP enables a number of features at the moment.

I want to keep it separate. Running the full test suite requires -DLUA_DEBUG but that doesn't mean I want to compile with -Og because it may take too much flash.

It looks like my comment was not completely clear here. What I meant is that to me, the -Og option is not only useful here, but could be useful for debugging other examples as well. So what I would like to propose is an option along the lines of the current DEVELHELP, maybe called OPTIMIZE_DEBUG to enable -Og for a random example.

bergzand · 2018-06-27T20:05:35Z

examples/lua_REPL/Makefile

+RIOTBASE ?= $(CURDIR)/../..
+
+BOARD_WHITELIST := frdm-k64f iotlab-m3 mulle native nrf52840dk nucleo-f411re \
+				   stm32f4discovery


Is this list based on something? Would you mind adding a comment to clarify why these boards qualify for this example?

Those are boards with enough flash to run Lua. Maybe other boards can run Lua too, but I don't know.

Could you at least add this information as a comment here. If you like, maybe you could quickly find all boards that have enough flash and ram to fit the example here.

As this is now, I don't think it is clear for an end user why these specific boards are whitelisted and when a board qualifies

Now I'm thinking on whitelisting only the boards that I can try myself. I could run the CI on all boards and then blacklist all that fail because of low memory, but that's not very robust: a later change to riot may cause the build to fail. How should I proceed?

Check back with @cladmi .

bergzand · 2018-06-27T21:09:40Z

pkg/lua/contrib/lua_loadlib.c

+#include "lua_builtin.h"
+#include "lua_loadlib.h"
+
+#define LUAR_MAX_MODULE_NAME 64


As I understand it this limits the size of a LUA module name. As this information is relevant to the end user (the person developing a LUA module), maybe you could move it to lua_builtin.h with some relevant doxygen comment.

bergzand · 2018-06-27T21:12:54Z

pkg/lua/include/lua_builtin.h

+                                  *  table at the top of the lua stack.
+                                  *  @todo Add better docs.
+                                  */
+};


In the spirit of @jia200x's comment on style conventions, I would prefer to have these structs typedef't as for example lua_riot_builtin_c_t

Those structs make up tables that have to be redefined. I don't do the user any favour by hiding the fact that they are structs.

I don't think you have to hide the fact that they are structs, I'm just asking you to adhere to the RIOT coding conventions on types

The use of typedefs for structs and pointers is allowed.

"allowed" != "required"

Ah you're right there. I still don't see the harm in adding a typedef here, users are not forced to use it, but if you really have a strong opinion here I won't block it.

bergzand · 2018-06-27T21:17:36Z

pkg/lua/include/lua_builtin.h

+/** Table containing all built in c lua modules */
+extern const struct luaR_builtin_c *const luaR_builtin_c_table;
+/** Number of elements of luaR_builtin_c_table */
+extern const size_t luaR_builtin_c_table_len;


Is there a reason to expose these variables (luaR_builtin_c_table_len and luaR_builtin_lua_table_len)? If they are relevant for the application, why not provide a simple inline getter for them (static inline size_t lua_riot_get_c_table_len())?

Those are defined as weak symbols,where the default value is an empty table. This allows the code to compile and work even of there are no lua modules. The tables can be overridden by the application to provide additional functionality in the form of modules.
So, they are relevant for the application to define, but not to read.

Okay, would you mind adding a short explanation about this above these declarations?

bergzand · 2018-06-27T21:18:34Z

pkg/lua/include/lua_loadlib.h

+extern "C" {
+#endif
+
+#define LUAR_MODULE_NOTFOUND 50


Why this specific value here? Maybe add a comment to clarify?

To ensure we don't collide with lua's error codes.

bergzand · 2018-06-27T21:19:25Z

pkg/lua/include/lua_run.h

+ * - Easy to use routines for executing modules as scrips.
+ * - Control over which modules get loaded.
+ *
+ * In a future versio, support will be added for an allocator that works with


danpetry · 2018-06-28T09:35:22Z

@tcschmidt I'm waiting for some test information and results from @jcarrano . After these are here I'll review the details at the same time, i.e. do it all together. Is that ok? I prefer to do reviewing monolithically rather than piecemeal as it's more efficient.

danpetry · 2018-06-28T09:44:50Z

Strongly agree with the splitting PRs. This will make it a lot easier to get future contributions merged

jcarrano · 2018-06-28T13:42:34Z

@danpetry @bergzand I understand that the PR is big, but how do you propose to split it? The only split that could work is the package and the examples.

danpetry · 2018-06-28T13:50:58Z

Let's keep it as it is for now, we're almost there. Let's talk about the granularity that works best for upcoming PRs.

bergzand · 2018-06-28T14:06:09Z

@danpetry @bergzand I understand that the PR is big, but how do you propose to split it? The only split that could work is the package and the examples.

While I agree with @danpetry that it is a bit late for a split, I think having the examples split from this PR already helps. I'm not sure if it is possible, but having (some of) the patches in a separate PR makes also for easier reviewing.

danpetry · 2018-06-28T14:08:29Z

I'd really prefer if we didn't split this now. I've got a good handle on everything that's going on here now, and I'm almost there with reviewing. It wouldn't make sense at this stage.

bergzand · 2018-06-28T14:09:45Z

I'd really prefer if we didn't split this now. I've got a good handle on everything that's going on here now, and I'm almost there with reviewing. It wouldn't make sense at this stage.

Yes, I completely agree with you here, just wanted to give @jcarrano a few pointers on how to split PRs like these in the future

danpetry · 2018-06-28T14:42:13Z

Ah ok cool :)

All issues were addressed.

- Remove file related functions from loader. * All packages must be builtin. - Remove os.tmpname. - Interface with TLSF. - Don't abort() when out of memory.

danpetry · 2018-07-02T13:32:01Z

Thanks for making changes. However, lua_REPL example now doesn't compile, please could you debug?

jcarrano · 2018-07-02T13:34:52Z

@danpetry Fixed. It seems that the "ä" from Universität was causing trouble, and I had to change function signatures to accept uint8 instead of of char to avoid future problems.

This example add a module thats starts an interactive READ-EVAL- PRINT-LOOP written in Lua.

danpetry · 2018-07-02T13:58:14Z

All comments addressed. ACK

danpetry

All changes addressed. ACK

All comments discussed live and addressed earlier on today.

bergzand · 2018-07-02T14:04:29Z

@danpetry I could really appreciate it if you let me just ACK the PR next time instead of dismissing my review. I still had one or two open discussions... :(

danpetry · 2018-07-02T14:27:08Z

Ok, apologies! More of a n00b github/reviewer process issue than a deliberate snub. As mentioned just now in person, I will do that next time, thanks for feedback.
@jia200x on a similar note: we took care to address your review points above and wanted to merge this before you came back from holiday, hence dismissing your review. Please give us a shout if you have open issues with either the code or this approach.

jia200x added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: pkg Area: External package ports labels May 18, 2018

jia200x self-assigned this May 18, 2018

jcarrano force-pushed the lua-on-riot branch 2 times, most recently from a06b7a5 to 77e183d Compare May 22, 2018 09:25

danpetry self-assigned this May 28, 2018

jcarrano force-pushed the lua-on-riot branch from 77e183d to 5aa6441 Compare May 30, 2018 15:08

danpetry mentioned this pull request May 31, 2018

WIP: Lua tests #9256

Closed

jcarrano mentioned this pull request May 31, 2018

sys/newlib: Split stubs and make modules provide non-stubs #9258

Closed

cgundogan added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jun 4, 2018

jcarrano force-pushed the lua-on-riot branch from 5aa6441 to 3ef6d47 Compare June 5, 2018 10:43

jia200x requested changes Jun 11, 2018

View reviewed changes

jia200x previously requested changes Jun 11, 2018

View reviewed changes

jia200x added the State: waiting for other PR State: The PR requires another PR to be merged first label Jun 11, 2018

danpetry reviewed Jun 11, 2018

View reviewed changes

jcarrano force-pushed the lua-on-riot branch from 0e12f6f to 932286f Compare June 12, 2018 08:05

danpetry removed the State: waiting for other PR State: The PR requires another PR to be merged first label Jun 21, 2018

danpetry suggested changes Jun 21, 2018

View reviewed changes

jcarrano force-pushed the lua-on-riot branch from 932286f to 8178ed1 Compare June 22, 2018 15:53

danpetry suggested changes Jun 22, 2018

View reviewed changes

danpetry suggested changes Jun 26, 2018

View reviewed changes

bergzand previously requested changes Jun 27, 2018

View reviewed changes

jcarrano force-pushed the lua-on-riot branch 2 times, most recently from f1c0e04 to eb33f77 Compare July 2, 2018 12:49

pkg/lua: Provide better integration with RIOT

ed44116

- Remove file related functions from loader. * All packages must be builtin. - Remove os.tmpname. - Interface with TLSF. - Don't abort() when out of memory.

jcarrano force-pushed the lua-on-riot branch from eb33f77 to f195bb8 Compare July 2, 2018 13:13

examples/lua: Add REPL.

87496f3

This example add a module thats starts an interactive READ-EVAL- PRINT-LOOP written in Lua.

jcarrano force-pushed the lua-on-riot branch from f195bb8 to 87496f3 Compare July 2, 2018 13:39

danpetry approved these changes Jul 2, 2018

View reviewed changes

danpetry merged commit f47bbfe into RIOT-OS:master Jul 2, 2018

cladmi added this to the Release 2018.07 milestone Jul 10, 2018

jcarrano mentioned this pull request Apr 25, 2019

Lua support #8434

Closed


		/* ======================== 'require' function ============================= */

		@@ -29,12 +29,10 @@ DEVELHELP ?= 1
		# Change this to 0 show compiler invocation lines by default:
		QUIET ?= 1


		#define MAIN_LUA_MEM_SIZE (40000)

		char lua_memory[MAIN_LUA_MEM_SIZE] __attribute__ ((aligned (__BIGGEST_ALIGNMENT__)));


		char lua_memory[MAIN_LUA_MEM_SIZE] __attribute__ ((aligned (__BIGGEST_ALIGNMENT__)));

		#define BARE_MINIMUM_MODS (LUAR_LOAD_BASE\|LUAR_LOAD_IO\|LUAR_LOAD_CORO)


		xxd -i $< \| sed 's/^unsigned/const/g' > $@

		$(RIOTBUILD_CONFIG_HEADER_C): $(LUA_H)

Conversation

jcarrano commented May 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Better integration of the Lua interpreter with RIOT

Future work

About the Lua patches

Issues/PRs references

Uh oh!

smlng commented May 28, 2018

Uh oh!

jcarrano commented Jun 4, 2018

Uh oh!

cgundogan commented Jun 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcarrano Jun 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcarrano commented May 18, 2018 •

edited

Loading

jcarrano Jun 11, 2018 •

edited

Loading