Lua tidy-up to reduce delta against esp32 branch.

nodemcu / nodemcu-firmware

Lua based interactive firmware for ESP8266, ESP8285 and ESP32

https://nodemcu.readthedocs.io

MIT License

7.64k stars 3.12k forks source link

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

Open jmattsson opened 2 years ago

jmattsson commented 2 years ago

[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 commented 2 years ago

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

jmattsson commented 2 years ago

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 commented 1 year ago

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

jmattsson commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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