nodemcu-firmware icon indicating copy to clipboard operation
nodemcu-firmware copied to clipboard

Lua tidy-up to reduce delta against esp32 branch.

Open jmattsson opened this issue 3 years ago • 7 comments

  • [x] This PR is for the dev branch rather than for the release branch.
  • [x] This PR is compliant with the other contributing guidelines as well (if not, please describe why).
  • [x] 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 avatar Sep 24 '21 02:09 jmattsson

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

jmattsson avatar Sep 24 '21 04:09 jmattsson

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

jmattsson avatar Sep 28 '21 03:09 jmattsson

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

marcelstoer avatar Dec 22 '22 20:12 marcelstoer

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.

jmattsson avatar Jan 01 '23 06:01 jmattsson

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.

marcelstoer avatar Jan 01 '23 08:01 marcelstoer

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

HHHartmann avatar Jan 01 '23 10:01 HHHartmann

@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 avatar Jan 02 '23 03:01 jmattsson