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

jmattsson · 2021-09-24T02:01:10Z

This PR is for the dev branch rather than for the release branch.
This PR is compliant with the other contributing guidelines as well (if not, please describe why).
I have thoroughly tested my contribution.
[n/a] The code changes are reflected in the documentation at docs/*.

These should hopefully be completely uncontentious changes, as there
are no functional changes in this commit.

There is still a delta against the esp32 branch, but to harmonise that fully
requires a bit more work.

This commit includes:

Some LUA_USE_xxx #ifdef changes, since in many ways the ESP32 is
closer to a host build than an ESP8266 build.
A bunch of compiler warnings tidy-ups.
A couple of readability/maintainability improvements (e.g. LROT_END
definition using named member initialisation, prototype declarations
moved to header file(s)).
Some 64bit correctness for 64bit host builds.

jmattsson · 2021-09-24T04:31:35Z

MSVC strikes again. I'll push a fix when I have a some uninterrupted dev time.

These should hopefully be completely uncontentious changes. There is still a delta against the esp32 branch, but to harmonise that requires a bit more work. This commit includes: - Some LUA_USE_xxx #ifdef changes, since in many ways the ESP32 is closer to a host build than an ESP8266 build. - A bunch of warnings tidy-ups. - A couple of readability/maintainability improvements (e.g. LROT_END definition using named member initialisation, prototype declarations moved to header file(s)).

TerryE

Johny, just a quit read through. I leave it to you to wildcard the use of the UNUSED() macro. I need to understand the extra meta fields on the ROTable declarations, or are this just a keyed recoding of the positional initialiser?

TerryE · 2021-09-25T17:01:05Z

app/lua53/lutf8lib.c

@@ -258,6 +258,7 @@ LROT_END(utf8, LROT_TABLEREF(utf8_meta), 0)


 LUAMOD_API int luaopen_utf8 (lua_State *L) {
+  (void)L;


There's actually an UNUSED(v) macro in llimits.h

Ah, I hadn't noticed that. Happy to swap over to that both here and on the esp32 branch then. Stay tuned... :)

Yup, I think that UNUSED(var) is more obvious to the reader.

TerryE · 2021-09-25T17:15:14Z

app/lua53/lua.h

@@ -465,6 +465,8 @@ struct lua_Debug {
  struct CallInfo *i_ci;  /* active function */
 };

+int lua_main(void);


I hummed and harred about where to export lua_main(). This is roughly equivalent to main() in std lua.c. lua.h is the public interface to the Lua API, and this main entry is not part of the API and in general if and module writer were to attempt to call this, it would be a disaster. This is why I left the entry hidden , and declared is extern in the file that requires it.

Hmm, on the other hand, letting others declare things extern elsewhere is a recipe for subtle breakage if the actual prototype does change (been there, done that 😞). Maybe it should simply be dumped into its own header file that gets included by lua.c to ensure the declaration and definition match?

In many ways I'd be more happy with that. Or at least a non-"Lua public API" section of lua.h that is gated by an #ifdef clause. I did that in lua_nodemcu.h IIRC because I came across a case where a module developer was trying to poke around the Lua engine intervals to implement the module.

TerryE · 2021-09-25T17:18:40Z

app/lua53/lstate.c

@@ -42,12 +42,15 @@
 ** created; the seed is used to randomize hashes.
 */
 #if !defined(luai_makeseed)
-#if defined(LUA_USE_ESP)
+#if defined(LUA_USE_ESP8266)


Isn't the ccount register also available on the ESP32? If so, then this should be LUA_USE_ESP

ccount is an Xtensa thing. The ESP32-C3 (and upcoming ESP32-H2) is a RISC-V core.

Yup but wouldn't we be introducing a new LUA_USE_RISCV category or at least review this hierarchy of USE source tailoring macros?

TerryE · 2021-09-25T17:20:03Z

app/lua53/lobject.h

@@ -80,7 +80,7 @@
 ** wo is the offset of aligned word in bytes 0,4,8,..
 ** bo is the field within the word in bits 0..31
 */
-#ifdef LUA_USE_ESP
+#if defined(LUA_USE_ESP8266)


Another one that also applies to the ESP32?

Nope. RISC-V does not take kindly to Xtensa assembly :)

As previous comment. Perhaps we need to do that review sooner rather than later.

TerryE · 2021-09-25T17:22:57Z

app/lua53/lobject.c

@@ -84,6 +78,8 @@ int luaO_ceillog2 (unsigned int x) {
  x--;
  while (x >= 256) { l += 8; x >>= 8; }
  return l + log_2[x];
+#else
+  return (x == 1) ? 0 : 32 - __builtin_clz(x - 1);


Is this builtin also available for the ESP8266? If so then we should be using it rather than a more cludgy asm construct. Where can I get a list of builtins that apply to the ESP32 and ESP8266 compiler?

Yeah, it's part of the standard builtins provided by gcc/clang. See e.g. gcc docs. I did run into an issue where on x86_64 compiled without optimisations it would error at runtime, so I left the host builds with the slow implementation.

TerryE · 2021-09-25T17:38:48Z

app/lua/lobject.c

-  * 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);


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

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

TerryE · 2021-09-25T17:41:30Z

app/lua/lobject.h

@@ -28,7 +28,7 @@
 #define LUA_TUPVAL	(LAST_TAG+2)
 #define LUA_TDEADKEY	(LAST_TAG+3)

-#ifdef LUA_USE_ESP
+#ifdef LUA_USE_ESP8266


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?

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 :)

TerryE · 2021-09-25T17:44:35Z

app/lua53/lnodemcu.c

@@ -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) {


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.

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?

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

TerryE · 2021-09-25T17:48:31Z

app/lua53/lnodemcu.c

@@ -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


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.

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)

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.

TerryE · 2021-09-25T17:52:01Z

app/lua53/lnodemcu.c

@@ -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(".");


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

jmattsson · 2021-09-28T03:39:01Z

I need to understand the extra meta fields on the ROTable declarations, or are this just a keyed recoding of the positional initialiser?

That is purely a change from array-style initialisation ({ 1, 2, 3 }) to named member initialisation ({ .x = 1, .y = 2, .z = 3}). I had a hard time working out what got set to what when I was chasing something through this code, and the ROTable struct being composed through two levels of indirect #defines was doing my head in. Aside from being a bit more readable, longer term the named member initialisation is a bit safer in the face of changing structures too (also been there, done that, have the scars...).

marcelstoer · 2022-12-22T20:52:01Z

Johny, any chance to revive this old PR? Reducing deltas between our branches would certainly be desirable I guess.

jmattsson · 2023-01-01T06:39:59Z

Honestly, I think the ship has sailed at this point Marcel. Both personally and at $work we have moved on from the venerable 8266. It's been a couple of years since I last actively touched an 8266 and even then it was only in maintenance mode. Even when I prepared this PR I wasn't working with the 8266 any longer. Between that, my limited time availability, and Terry not having (had?) time/interest to skill up on the ESP32 ecosystem I don't think this will happen. We could leave the PR sitting around in the hope that changes, or we could close it. I'm happy to leave the source branch there should it ever become relevant again.

marcelstoer · 2023-01-01T08:38:31Z

I think the ship has sailed at this point Marcel [..] We could leave the PR sitting around in the hope that changes, or we could close it.

I fully agree and I don't mind closing this one. I asked because I see a third option: merging what's already there.

HHHartmann · 2023-01-01T10:29:38Z

Especially not sure about Terry's comment in app/lua/lnodemcu.h
So would rather not like to see at least that part in the esp8266 branch

jmattsson · 2023-01-02T03:32:48Z

@HHHartmann I had a re-read of that comment and it does not make sense to me. There is no re-ordering, it is merely a change to using designated initalization to avoid bugs creeping in if there is reordering of actual structure declaration at some point.

jmattsson requested a review from TerryE September 24, 2021 02:01

jmattsson force-pushed the lua-harmonisation-1 branch from eddac2f to 4e56a1b Compare September 25, 2021 01:14

jmattsson force-pushed the lua-harmonisation-1 branch from 4e56a1b to 80a1278 Compare September 25, 2021 01:27

TerryE requested changes Sep 25, 2021

View reviewed changes

TerryE self-requested a review September 25, 2021 17:59

nwf added this to the 2022 rel 1 milestone Jan 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jmattsson commented Sep 24, 2021

jmattsson commented Sep 24, 2021

TerryE left a comment

TerryE Sep 25, 2021

jmattsson Sep 28, 2021

TerryE Sep 28, 2021

TerryE Sep 25, 2021

jmattsson Sep 28, 2021

TerryE Sep 28, 2021

TerryE Sep 25, 2021

jmattsson Sep 28, 2021

TerryE Sep 28, 2021

TerryE Sep 25, 2021

jmattsson Sep 28, 2021

TerryE Sep 28, 2021

TerryE Sep 25, 2021

jmattsson Sep 28, 2021

TerryE Sep 25, 2021

jmattsson Sep 28, 2021

TerryE Sep 25, 2021

jmattsson Sep 28, 2021

TerryE Sep 25, 2021

jmattsson Sep 28, 2021

TerryE Sep 28, 2021

TerryE Sep 25, 2021

jmattsson Sep 28, 2021

TerryE Sep 28, 2021

TerryE Sep 25, 2021

jmattsson commented Sep 28, 2021

marcelstoer commented Dec 22, 2022

jmattsson commented Jan 1, 2023

marcelstoer commented Jan 1, 2023

HHHartmann commented Jan 1, 2023

jmattsson commented Jan 2, 2023

		@@ -258,6 +258,7 @@ LROT_END(utf8, LROT_TABLEREF(utf8_meta), 0)


		LUAMOD_API int luaopen_utf8 (lua_State *L) {
		(void)L;

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

Are you sure you want to change the base?

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

Conversation

jmattsson commented Sep 24, 2021

jmattsson commented Sep 24, 2021

TerryE left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmattsson commented Sep 28, 2021

marcelstoer commented Dec 22, 2022

jmattsson commented Jan 1, 2023

marcelstoer commented Jan 1, 2023

HHHartmann commented Jan 1, 2023

jmattsson commented Jan 2, 2023