Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lua tidy-up to reduce delta against esp32 branch. #3468

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/lua/lbaselib.c
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
** model but changing `fputs' to put the strings at a proper place
** (a console window or a log file, for instance).
*/
#ifdef LUA_CROSS_COMPILER
#if defined(LUA_USE_ESP8266)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this a host macro?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it is

#undef puts
#define puts(s) printf("%s",s)
#endif
Expand Down
3 changes: 2 additions & 1 deletion app/lua/ldebug.c
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,7 @@ static int stripdebug (lua_State *L, Proto *f, int level) {
f->packedlineinfo = luaM_freearray(L, f->packedlineinfo, sizepackedlineinfo, unsigned char);
len += sizepackedlineinfo;
}
// fall-through
case 1:
f->locvars = luaM_freearray(L, f->locvars, f->sizelocvars, struct LocVar);
f->upvalues = luaM_freearray(L, f->upvalues, f->sizeupvalues, TString *);
Expand Down Expand Up @@ -501,7 +502,7 @@ static Instruction symbexec (const Proto *pt, int lastpc, int reg) {
case OP_FORLOOP:
case OP_FORPREP:
checkreg(pt, a+3);
/* go through */
/* fall-through */
case OP_JMP: {
int dest = pc+1+b;
/* not full check and jump is forward and do not skip `lastpc'? */
Expand Down
33 changes: 19 additions & 14 deletions app/lua/lflash.c
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
* process.
*/

static char *flashAddr;
static const char *flashAddr;
static uint32_t flashSize;
static uint32_t flashAddrPhys;
static uint32_t flashSector;
Expand Down Expand Up @@ -106,20 +106,20 @@ LUA_API void dumpStrings(lua_State *L) {
* writes are suppressed if the global writeToFlash is false. This is used in
* phase I where the pass is used to size the structures in flash.
*/
static char *flashPosition(void){
static const char *flashPosition(void){
return flashAddr + curOffset;
}


static char *flashSetPosition(uint32_t offset){
static const char *flashSetPosition(uint32_t offset){
NODE_DBG("flashSetPosition(%04x)\n", offset);
curOffset = offset;
return flashPosition();
}


static char *flashBlock(const void* b, size_t size) {
void *cur = flashPosition();
static const char *flashBlock(const void* b, size_t size) {
const void *cur = flashPosition();
NODE_DBG("flashBlock((%04x),%p,%04x)\n", curOffset,b,size);
lua_assert(ALIGN_BITS(b) == 0 && ALIGN_BITS(size) == 0);
platform_flash_write(b, flashAddrPhys+curOffset, size);
Expand All @@ -137,6 +137,10 @@ static void flashErase(uint32_t start, uint32_t end){
platform_flash_erase_sector( flashSector + i );
}

static int loadLFS (lua_State *L);
static int loadLFSgc (lua_State *L);
static void procFirstPass (void);

/* =====================================================================================
* luaN_init() is exported via lflash.h.
* The first is the startup hook used in lstate.c and the last two are
Expand Down Expand Up @@ -171,7 +175,7 @@ LUAI_FUNC void luaN_init (lua_State *L) {
}

if ((fh->flash_sig & (~FLASH_SIG_ABSOLUTE)) != FLASH_SIG ) {
NODE_ERR("Flash sig not correct: 0x%08x vs 0x%08x\n",
NODE_ERR("LFS sig not correct: 0x%x vs expected 0x%x\n",
fh->flash_sig & (~FLASH_SIG_ABSOLUTE), FLASH_SIG);
return;
}
Expand Down Expand Up @@ -208,6 +212,7 @@ LUALIB_API void luaL_lfsreload (lua_State *L) {
return;
}


/*
* Do a protected call of loadLFS.
*
Expand Down Expand Up @@ -346,13 +351,13 @@ static void put_byte (uint8_t value) {
}


static uint8_t recall_byte (unsigned offset) {
static uint8_t recall_byte (uint32_t offset) {
if(offset > DICTIONARY_WINDOW || offset >= out->ndx)
flash_error("invalid dictionary offset on inflate");
/* ndx starts at 1. Need relative to 0 */
unsigned n = out->ndx - offset;
unsigned pos = n % WRITE_BLOCKSIZE;
unsigned blockNo = out->ndx / WRITE_BLOCKSIZE - n / WRITE_BLOCKSIZE;
uint32_t n = out->ndx - offset;
uint32_t pos = n % WRITE_BLOCKSIZE;
uint32_t blockNo = out->ndx / WRITE_BLOCKSIZE - n / WRITE_BLOCKSIZE;
return out->block[blockNo]->byte[pos];
}

Expand Down Expand Up @@ -386,7 +391,7 @@ void procFirstPass (void) {
fh->flash_size > flashSize ||
out->flagsLen != 1 + (out->flashLen/WORDSIZE - 1) / BITS_PER_WORD)
flash_error("LFS length mismatch");
out->flags = luaM_newvector(out->L, out->flagsLen, unsigned);
out->flags = luaM_newvector(out->L, out->flagsLen, uint32_t);
}

/* update running CRC */
Expand All @@ -412,7 +417,7 @@ void procSecondPass (void) {
(out->flashLen % WRITE_BLOCKSIZE) / WORDSIZE :
WRITE_BLOCKSIZE / WORDSIZE;
uint32_t *buf = (uint32_t *) out->buffer.byte;
uint32_t flags = 0;
uint32_t flags = 0;
/*
* Relocate all the addresses tagged in out->flags. This can't be done in
* place because the out->blocks are still in use as dictionary content so
Expand All @@ -423,7 +428,7 @@ void procSecondPass (void) {
if ((i&31)==0)
flags = out->flags[out->flagsNdx++];
if (flags&1)
buf[i] = WORDSIZE*buf[i] + cast(uint32_t, flashAddr);
buf[i] = WORDSIZE*buf[i] + cast(uint32_t, flashAddr); // mapped, not phys
}
/*
* On first block, set the flash_sig has the in progress bit set and this
Expand Down Expand Up @@ -468,7 +473,7 @@ static int loadLFS (lua_State *L) {
in->len = vfs_size(in->fd);
if (in->len <= 200 || /* size of an empty luac output */
vfs_lseek(in->fd, in->len-4, VFS_SEEK_SET) != in->len-4 ||
vfs_read(in->fd, &out->len, sizeof(unsigned)) != sizeof(unsigned))
vfs_read(in->fd, &out->len, sizeof(uint32_t)) != sizeof(uint32_t))
flash_error("read error on LFS image file");
vfs_lseek(in->fd, 0, VFS_SEEK_SET);

Expand Down
1 change: 1 addition & 0 deletions app/lua/llex.c
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ static void save (LexState *ls, int c) {


void luaX_init (lua_State *L) {
(void)L;
}


Expand Down
1 change: 1 addition & 0 deletions app/lua/lmathlib.c
Original file line number Diff line number Diff line change
Expand Up @@ -359,5 +359,6 @@ LROT_END(math, NULL, 0)
** Open math library
*/
LUALIB_API int luaopen_math (lua_State *L) {
(void)L;
return 0;
}
1 change: 1 addition & 0 deletions app/lua/lnodemcu.c
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@ LUALIB_API int luaL_posttask( lua_State* L, int prio ) { // [-1, +0, -]
}
#else
LUALIB_API int luaL_posttask( lua_State* L, int prio ) {
(void)L; (void)prio;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again use UNUSED macro. Actually can I make this a blanket comment relating to all (void)var uses in order to suppress compile warnings? I'll drop following explicit mark-ups

return 0;
} /* Dummy stub on host */
#endif
Expand Down
11 changes: 6 additions & 5 deletions app/lua/lnodemcu.h
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,11 @@
static ROTable_entry LOCK_IN_SECTION(s) rt ## _entries[] = {
#define LROT_END(rt,mt,f) {NULL, LRO_NILVAL} }; \
const ROTable rt ## _ROTable = { \
(GCObject *)1, LUA_TROTABLE, LROT_MARKED, \
cast(lu_byte, ~(f)), (sizeof(rt ## _entries)/sizeof(ROTable_entry)) - 1, \
cast(Table *, mt), cast(ROTable_entry *, rt ## _entries) };
.next = (GCObject *)1, .tt = LUA_TROTABLE, .marked = LROT_MARKED, \
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is quite a change to the ROTable layout. The current code uses exploit the fact that a common subset of Table and ROTable fields are effectively a superset union, so the code doesn't need messy conditional selectors. This change breaks this assumption. I need to think through the implications.

.flags = cast(lu_byte, ~(f)), \
.lsizenode = (sizeof(rt ## _entries)/sizeof(ROTable_entry)) - 1, \
.metatable = cast(Table *, mt), \
.entry = cast(ROTable_entry *, rt ## _entries) };
#define LROT_BREAK(rt) };

#define LROT_MASK(m) cast(lu_byte, 1<<TM_ ## m)
Expand All @@ -65,10 +67,9 @@
#define LROT_MASK_NEWINDEX LROT_MASK(NEWINDEX)
#define LROT_MASK_GC_INDEX (LROT_MASK_GC | LROT_MASK_INDEX)

/* Maximum length of a rotable name and of a string key*/
#define LUA_MAX_ROTABLE_NAME 32 /* Maximum length of a rotable name and of a string key*/

#ifdef LUA_CORE

#endif
#endif

1 change: 1 addition & 0 deletions app/lua/loadlib.c
Original file line number Diff line number Diff line change
Expand Up @@ -619,6 +619,7 @@ static int ll_seeall (lua_State *L) {

static void setpath (lua_State *L, const char *fieldname, const char *envname,
const char *def) {
(void)envname;
const char *path = NULL; /* getenv(envname) not used in NodeMCU */;
if (path == NULL) /* no environment variable? */
lua_pushstring(L, def); /* use default */
Expand Down
5 changes: 1 addition & 4 deletions app/lua/lobject.c
Original file line number Diff line number Diff line change
Expand Up @@ -68,10 +68,7 @@ int luaO_log2 (unsigned int x) {
while (x >= 256) { l += 8; x >>= 8; }
return l + log_2[x];
#else
/* Use Normalization Shift Amount Unsigned: 0x1=>31 up to 0xffffffff =>0
* See Xtensa Instruction Set Architecture (ISA) Refman P 462 */
asm volatile ("nsau %0, %1;" :"=r"(x) : "r"(x));
return 31 - x;
return 31 - __builtin_clz(x);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same point about use of builtins vs asm() as below.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah on Xtensa __builtin_clz() translates to a nsau instruction.

#endif
}

Expand Down
2 changes: 1 addition & 1 deletion app/lua/lobject.h
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
#define LUA_TUPVAL (LAST_TAG+2)
#define LUA_TDEADKEY (LAST_TAG+3)

#ifdef LUA_USE_ESP
#ifdef LUA_USE_ESP8266
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the ESP32 support unaligned data fetches from IRAM and IFLASH?. If so then how many extra clocks? The access macros add one extra register instruction to use an aligned read. If this is still faster then why have this difference between the ESP8266 and ESP32 versions?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, this is predominantly an Xtensa vs RISC-V issue here. But yes, data memory regions are byte accessible. Some optimisation may be possible, but has not been a priority for me at this point - getting NodeMCU to run on all the different ESP32 SoCs has been :)

/*
** force aligned access to critical fields in Flash-based structures
** wo is the offset of aligned word in bytes 0,4,8,..
Expand Down
2 changes: 2 additions & 0 deletions app/lua/ltable.c
Original file line number Diff line number Diff line change
Expand Up @@ -336,6 +336,7 @@ static Node *find_prev_node(Node *mp, Node *next) {
** (colliding node is in its main position), moving node goes to an empty position.
*/
static int move_node (lua_State *L, Table *t, Node *node) {
(void)L;
Node *mp = mainposition(t, key2tval(node));
/* if node is in it's main position, don't need to move node. */
if (mp == node) return 1;
Expand Down Expand Up @@ -374,6 +375,7 @@ static int move_node (lua_State *L, Table *t, Node *node) {


static int move_number (lua_State *L, Table *t, Node *node) {
(void)L;
int key;
lua_Number n = nvalue(key2tval(node));
lua_number2int(key, n);
Expand Down
1 change: 1 addition & 0 deletions app/lua/ltablib.c
Original file line number Diff line number Diff line change
Expand Up @@ -277,5 +277,6 @@ LROT_BEGIN(tab_funcs, NULL, 0)
LROT_END(tab_funcs, NULL, 0)

LUALIB_API int luaopen_table (lua_State *L) {
(void)L;
return 1;
}
9 changes: 5 additions & 4 deletions app/lua53/lauxlib.h
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,9 @@ LUALIB_API void (luaL_requiref) (lua_State *L, const char *modname,
#define luaL_loadbuffer(L,s,sz,n) luaL_loadbufferx(L,s,sz,n,NULL)


#define luaL_checkfunction(L,n) luaL_checktype(L, (n), LUA_TFUNCTION);
#define luaL_checktable(L,n) luaL_checktype(L, (n), LUA_TTABLE);

/*
** {======================================================
** Generic Buffer manipulation
Expand Down Expand Up @@ -227,7 +230,8 @@ LUALIB_API void (luaL_openlib) (lua_State *L, const char *libname,

/* print a string */
#if !defined(lua_writestring)
#ifdef LUA_USE_ESP8266
#ifdef LUA_USE_ESP
void output_redirect(const char *str, size_t l);
#define lua_writestring(s,l) output_redirect((s),(l))
#else
#define lua_writestring(s,l) fwrite((s), sizeof(char), (l), stdout)
Expand Down Expand Up @@ -290,11 +294,8 @@ LUALIB_API int (luaL_pushlfsdts) (lua_State *L);
LUALIB_API void (luaL_lfsreload) (lua_State *L);
LUALIB_API int (luaL_posttask) (lua_State* L, int prio);
LUALIB_API int (luaL_pcallx) (lua_State *L, int narg, int nres);

#define luaL_pushlfsmodule(l) lua_pushlfsfunc(L)

/* }============================================================ */

#endif


1 change: 1 addition & 0 deletions app/lua53/ldblib.c
Original file line number Diff line number Diff line change
Expand Up @@ -464,6 +464,7 @@ LROT_BEGIN(dblib, NULL, 0)
LROT_END(dblib, NULL, 0)

LUAMOD_API int luaopen_debug (lua_State *L) {
(void)L;
return 0;
}

1 change: 1 addition & 0 deletions app/lua53/ldump.c
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,7 @@ static int stripdebug (lua_State *L, Proto *f, int level) {
f->lineinfo = luaM_freearray(L, f->lineinfo, f->sizelineinfo);
len += f->sizelineinfo;
}
// fall-through
case 1:
for (i=0; i<f->sizeupvalues; i++)
f->upvalues[i].name = NULL;
Expand Down
1 change: 1 addition & 0 deletions app/lua53/lmathlib.c
Original file line number Diff line number Diff line change
Expand Up @@ -394,6 +394,7 @@ LROT_END(mathlib, NULL, 0)
** Open math library
*/
LUAMOD_API int luaopen_math (lua_State *L) {
(void)L;
return 0;
}

20 changes: 12 additions & 8 deletions app/lua53/lnodemcu.c
Original file line number Diff line number Diff line change
Expand Up @@ -296,7 +296,7 @@ LUA_API void lua_getlfsconfig (lua_State *L, int *config) {
}
}

LUA_API int (lua_pushlfsindex) (lua_State *L) {
LUA_API int lua_pushlfsindex(lua_State *L) {
lua_lock(L);
setobj2n(L, L->top, &G(L)->LFStable);
api_incr_top(L);
Expand All @@ -309,7 +309,7 @@ LUA_API int (lua_pushlfsindex) (lua_State *L) {
* one upvalue that must be the set to the _ENV variable when its closure is
* created, and as such this parallels some ldo.c processing.
*/
LUA_API int (lua_pushlfsfunc) (lua_State *L) {
LUA_API int lua_pushlfsfunc(lua_State *L) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The standard pattern is to add the parentheses where a define macro might be encoded to force sensible compile errors here. Why change this?

Ditto on following cases.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The standard pattern is to only parenthesise function names only when typedef'ing function pointer types. Why would these be different? If someone #defines a function name to something else, surely that is on them?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just feel that we should follow the standard Lua coding standards where practical

lua_lock(L);
const TValue *t = &G(L)->LFStable;
if (ttisstring(L->top-1) && ttistable(t)) {
Expand Down Expand Up @@ -341,7 +341,7 @@ LUA_API int (lua_pushlfsfunc) (lua_State *L) {
/*
* Return an array of functions in LFS
*/
LUALIB_API int (luaL_pushlfsmodules) (lua_State *L) {
LUALIB_API int luaL_pushlfsmodules(lua_State *L) {
int i = 1;
if (lua_pushlfsindex(L) == LUA_TNIL)
return 0; /* return nil if LFS not loaded */
Expand All @@ -357,7 +357,7 @@ LUALIB_API int (luaL_pushlfsmodules) (lua_State *L) {
return 1;
}

LUALIB_API int (luaL_pushlfsdts) (lua_State *L) {
LUALIB_API int luaL_pushlfsdts(lua_State *L) {
int config[5];
lua_getlfsconfig(L, config);
lua_pushinteger(L, config[4]);
Expand Down Expand Up @@ -427,7 +427,9 @@ static const char *readF (lua_State *L, void *ud, size_t *size) {

static void eraseLFS(LFSflashState *F) {
lu_int32 i;
#ifdef LUA_USE_ESP
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Surely, the use of bare printf() statements for ESP code is generally deprecated. We should use the appropriate equivalent. Whatever we need a clear coding standard here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The printf()s aren't mine. I merely added the #ifdefs because having host-side LFS image generation tell me it's erasing flash was misleading at best.
(Okay, technically I believe I moved one of the printfs to reduce the number of ifdefs I needed)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking about this, we should move towards POSIX compliant source wherever practical. The real issue here is that printing should be deprecated, except for very specific circumstances, as it will be unusual for a human or logging system to be 'listening' to the serial output, mainly development debugging. This .... progress bar is really a development tool.

printf("\nErasing LFS from flash addr 0x%06x", F->addrPhys);
#endif
unlockFlashWrite();
for (i = 0; i < F->size; i += FLASH_PAGE_SIZE) {
size_t *f = cast(size_t *, F->addr + i/sizeof(*f));
Expand All @@ -436,11 +438,13 @@ static void eraseLFS(LFSflashState *F) {
#ifdef LUA_USE_ESP
if (*f == ~0 && !memcmp(f, f + 1, FLASH_PAGE_SIZE - sizeof(*f)))
continue;
printf(".");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above re use of print. The code skips the erase where it is not need and this makes flashing of smaller LFS images a lot faster. The dots are a pretty useful visual progress bar

#endif
platform_flash_erase_sector(s);
printf(".");
}
#ifdef LUA_USE_ESP
printf(" to 0x%06x\n", F->addrPhys + F->size-1);
#endif
flush_icache(F);
lockFlashWrite();
}
Expand Down Expand Up @@ -623,7 +627,6 @@ LUALIB_API void luaL_lfsreload (lua_State *L) {


#ifdef LUA_USE_ESP
extern void lua_main(void);
/*
** Task callback handler. Uses luaN_call to do a protected call with full traceback
*/
Expand All @@ -634,7 +637,7 @@ static void do_task (platform_task_param_t task_fn_ref, uint8_t prio) {
return;
}
if (prio < LUA_TASK_LOW|| prio > LUA_TASK_HIGH)
luaL_error(L, "invalid posk task");
luaL_error(L, "invalid post task");
/* Pop the CB func from the Reg */
lua_rawgeti(L, LUA_REGISTRYINDEX, (int) task_fn_ref);
luaL_checktype(L, -1, LUA_TFUNCTION);
Expand Down Expand Up @@ -662,14 +665,15 @@ LUALIB_API int luaL_posttask ( lua_State* L, int prio ) { // [-1, +0, -]
}
return task_fn_ref;
} else {
return luaL_error(L, "invalid posk task");
return luaL_error(L, "invalid post task");
}
}
#else
/*
** Task execution isn't supported on HOST builds so returns a -1 status
*/
LUALIB_API int luaL_posttask( lua_State* L, int prio ) { // [-1, +0, -]
(void)L; (void)prio;
return -1;
}
#endif
Loading