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 5.1 to 5.3 Realignment Phase 4 #3077

Open
TerryE opened this issue Apr 27, 2020 · 10 comments
Open

Lua 5.1 to 5.3 Realignment Phase 4 #3077

TerryE opened this issue Apr 27, 2020 · 10 comments

Comments

@TerryE
Copy link
Collaborator

TerryE commented Apr 27, 2020

Summary

Now that we have merged #3075 unto dev, I want will close the historic issues which largely scoped the work completed in this PR, and to use this issue as a general discussion thread to track the general points that we can clean up with any following commits. Where any point requires detailed discussion it its own right then we can track this with its own issue and reference it here.

  1. Lua C API realignment. All C code in app/modulesand the separate app successfully compiles using both the (default) LUA=51 and the new LUA=53 targets. Testers and Modules maintainers are encourage to check the modules that maintain or use against the Lua53 environment and raise any issues that are found separately. I have also done a review of the lua.h, lauxlib.h and lnodemcu.h headers and my incorporate some further internal tidying in a subsequent PR.

  2. Extra Documentation. Impacts of migrating to Lua 5.3 #2808 and my Lua 53 Whitepaper are WIP documentation of this change.

    • IMO we need to have some form of Lua 5.3 Whitepaper in the core document set, but this could probably drop all of the internals and historic discussion and focus solely on the how and what for Lua application developers.
    • We also need a C modules developers guide.
    • Possibly ditto an LFS provisioning guide.
  3. ECG review and adjustment. Replaces Stack integrity during EGC #2881. See RFC - Emergency GC setting for Lua 5.3 #2783

  4. Review of SPIFFS use and spiffsimg. spiffsimg should generate loadable images. SPIFFS mounting should be bypassed if the SPIFFS partition size is zero (LFS-based deep-sleep applications might wish to do this to reduce startup times.)

  5. debug.debug() should work as expected on NodeMCU builds. See standard Lua documentation. Calling this from a panic exception handler is really good way to debug panics.

  6. Lua ESP test harness. Though touched on in Create unit tests in Lua #2145 which discusses a generic framework for use with new developed modules, this is very much more a narrow one of regressing a subset of the Standard Lua Test Suite for on ESP execution within the NodeMCU environment.

  7. Rename and restructure app directory hierarchy to align this to the esp32 hierarchy. As well as the split of modules into the core, common and esp8266 modules directories, most of this work is Makefile magic and file moves. This initial version will focus on changes to the core modules.

Future enhancements

@TerryE
Copy link
Collaborator Author

TerryE commented Jul 8, 2020

#3193 should address the first 3 items above. We need to drop the debug.debug() idea for now as there are a few architectural issues to sort if we want to do this and I am not sure that this is worth the effort.

@nwf @HHHartmann @marcelstoer, I would appreciate your thoughts on what I should tackle next post #3193. Possible options are:

  • Update Lua test harness to work both on host and on ESP
  • Investigation into SPIFSS instability and updates, though my current thinking is that I should leave one of the other committers to do this drill-down.
  • Add second LFS region support and on ESP generation of LFS images.

@HHHartmann
Copy link
Member

I'd go for the second LFS image. Allthough I still think that a linktime addition of a fixed LFS part containing at least the Lua part of our (yet to come) hybrid modules would be at least as nice.
We got this great LFS technology and should use it to allow Lua modules to be first class citizens in our firmware, just as C modules.
Now we always need a second step of installing the LFS image.

@nwf
Copy link
Member

nwf commented Jul 9, 2020

As the person perhaps most noisily agitating for hybrid modules, I agree with @HHHartmann's sense of priority. :)

The SPIFFS thing seems like it's going to lead to an infinite well of sadness, and while I love testing, I think having a baseline design for the New World Order takes priority, given our very limited resources (which is to say, mostly you, @TerryE).

@TerryE
Copy link
Collaborator Author

TerryE commented Jul 10, 2020

Re: the SPIFFS issue see my comment #3148 (comment). Hopefully @pjsg Philip will take a look at this. I'll get started on the LFS stuff when stuck in review cycles on #3193.

@HHHartmann
Copy link
Member

@nwf, Did you prefer the 2nd LFS partition or the LFS builtin to a dev image?
@TerryE or will we be able to include one LFS image in the build already?

@TerryE
Copy link
Collaborator Author

TerryE commented Jul 12, 2020

@HHHartmann, we already have defines for two LFS partitions and nodemcu-partition.py includes the code to read/process the RCR. The startup PT is just an RCR record that can be written to. Ditto any startup option. We can also load any partitions the same way. So we just need to extend this Python code a little.

An alternative would be to stay in the file world (ignoring esptool.py entirely) and generate a single combined .bin file that includes the RCR suitably configured, and any LFS / SPIFFS partitions needed. This single file could then be downloaded to the ESP by esptool.py. Note that esptool downloads a bootstrap which expands gzip format, and so any content sent to the ESP is in GZIP format, and it it doesn't really if there are large gaps in the content. It would be nice if esptool understood when a file was already gzipped so that esptool.py write_flash 0x00000 bin/build.img.gz would just work, but that's a nit.

@nwf
Copy link
Member

nwf commented Jul 12, 2020

Honestly, I think I prefer a story with one LFS partition with the ability to build LFS images from .lc files on the device SPIFFS, as I outlined in #3128 (comment) . If people want to add support for two LFS partitions, I won't stop them, but I am not sure I understand its necessity and it seems like it would add complexity to allow either image to be replaced. (If there's hierarchy, so that the "first" image being updated also deletes the second, then this seems simpler.)

ETA: I see that @TerrryE has written at length in #3206 . I should read and understand that and see if it changes my understanding.

@nwf
Copy link
Member

nwf commented Jul 12, 2020

OK, it seems the dual-LFS story is well underway (sorry, I've been buried in a new day job, so this is perhaps more news to me than it should have been). Given that story, I think we can have our build infrastructure build in the "sysLFS" into the .bin it generates and, so long as the default LFS updated by flashreload and its replacement is the "appLFS", I think more or less everything will just work as it does now. Nice.

@stale
Copy link

stale bot commented Jul 8, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jul 8, 2021
@stale stale bot closed this as completed Jul 30, 2021
@TerryE
Copy link
Collaborator Author

TerryE commented Sep 27, 2021

I will kill or cure this one. In the meantime it remains an open issue.

@TerryE TerryE reopened this Sep 27, 2021
@stale stale bot removed the stale label Sep 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants