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

RFC: what should we do with the lua_examples/lfs ? #3200

Closed
TerryE opened this issue Jul 7, 2020 · 8 comments
Closed

RFC: what should we do with the lua_examples/lfs ? #3200

TerryE opened this issue Jul 7, 2020 · 8 comments
Assignees
Labels

Comments

@TerryE
Copy link
Collaborator

TerryE commented Jul 7, 2020

As it stands lua_examples/lfs contains four files. My initial intent was that these would be just that: examples, that is fragments of Lua code which would give developers some idea of how to integrate use of LFS into their applications. However, these seems to have morphed into tablets of stone, dogma or what you will: Don't bother to understand or to question but just include them verbatim into your application. Even to extent that we are getting issues and PRs about "breaking changes" in what is supposed to be sample code (#3168 is an example of this).

Since these seems to have assumed a weight that I hadn't intended, I feel that we need some agreement on how we should deal with them going forward.

  • My preferred approach would be to add some strong caveat comments to emphasise that these samples are templates to assist developers in creating their own init module which they should execute at startup for example by overwriting the default on boot action by executing node.startupcommand('!init')
  • We explicitly state that these are examples that may evolve and that we make no attempts to ensure backwards compatibility.

So let's confider these scripts in turn.

_init

  • This is a general init script, as an app will want to do more than this and, IMO, some of this doesn't make much sense to me. A more accurate name might be autoload here since it is only a part of initialisation,

  • With Fixed esp-lwip breaking change on dns_getserver #3195 the LFS table stuff can be stripped out and replaced by local LFS=node.LFS (or the global assignment to LFS)

  • Adding LFS modules to the require search path makes sense but we need to think about the precedence of file and LFS copies: if an LFS and a FS copy exists, then which should be loaded? Always having LFS take precedence is not right in my view.

    • During development it makes a lot of sense for the file copy to take precedence over the LFS one. This would be quite useful for debugging LFS module, since we would have the option to (temporarily) replace an LFS module with a SPIFFS debugging version.
    • For production the opposite is true: LFS should take precedence as this avoid unnecessary SPIFFS I/O.
  • loadfile and dofile. Personally these make no sense, at least to me.

    • The correct way to load an LFS modile xxx is to use LFS.xxx and the correct way to do it is LFS.xxx()
    • If you want path flexibility then use modules accessed with require and this in built in.
    • If you do want to add path flexibility to these loadfile and dofile functions, then surely the same arguments apply as for require. I suggest these also change from development to production.

dummy_strings

A quick win for LFS freeing up RAM, but the strings are different for 5.1 and 5.3. The simplest approach is to have a union of the two since a few extra ROstrt strings is hardly an overhead.

HTTP_OTA

I use this, but then again I run apache or lighttpd on most of my boxes. Anyone else use it? Should be a module, IMO.

lfs_fragments

Again this could do with being reviewed.

@HHHartmann
Copy link
Member

[...] Even to extent that we are getting issues and PRs about "breaking changes" in what is supposed to be sample code (#3168 is an example of this).

Well also examples should be up to date and executable. And right now 5.1 is not backwards compatibility.
I also would like to keep _init.lua as it is stone now and used in many HOWTOs.

  • My preferred approach would be to add some strong caveat comments to emphasise that these samples are templates to assist developers in creating their own init module which they should execute at startup for example by overwriting the default on boot action by executing node.startupcommand('!init')
  • We explicitly state that these are examples that may evolve and that we make no attempts to ensure backwards compatibility.

Maybe we need to deprecate _init.lua (for next milestone e.g.) and afterwards leave it deliberately not running error("Do your own script! this is only an example"), so Developers are forced to have a look and write their own startup. Then renaming it is a good idea too.

  • During development it makes a lot of sense for the file copy to take precedence over the LFS one. This would be quite useful for debugging LFS module, since we would have the option to (temporarily) replace an LFS module with a SPIFFS debugging version.

Agreed

  • loadfile and dofile. Personally these make no sense, at least to me.
  • If you do want to add path flexibility to these loadfile and dofile functions, then surely the same arguments apply as for require. I suggest these also change from development to production.

Because of this it does make sense to me.

dummy_strings

A quick win for LFS freeing up RAM, but the strings are different for 5.1 and 5.3. The simplest approach is to have a union of the two since a few extra ROstrt strings is hardly an overhead.

really should start using these

@TerryE
Copy link
Collaborator Author

TerryE commented Jul 7, 2020

@HHHartmann, Gregor you seem to have the most well developed views here. So can I suggest that you take the lead on coordinating feedback and define / agree the function requirement and doing the changes. This will give me more bandwidth to work on the internals.

@KT819GM
Copy link

KT819GM commented Jul 7, 2020

I'm using HTTP_OTA heavily, bit in a different approach (downloading, doing restart and just after reload, somehow found this more reliable when you floating on <20Kb RAM) so could not agree more for it to become module.

@vsky279
Copy link
Contributor

vsky279 commented Jul 8, 2020

_init

With Lua 5.3 my needs for _init.lua shrinks to

package.loaders[3] = function(module) -- loader_flash
  return LFS[module]
end

which I think I will implement directly in my init.lua. So deprecation & example warning seems ok to me. I don't use `loadfile' and 'dofile' for LFS.

dummy_strings

Union is a good solution.

HTTP_OTA

I don't use it. I use ftp and telnet instead. It is probably not so elegant but working so I had no desire to study this one. Once it is a module it will be more tempting for me to try it.

lfs_fragments

I don't use it.

@HHHartmann HHHartmann self-assigned this Jul 13, 2020
@pjsg
Copy link
Member

pjsg commented Jul 15, 2020

I think that the problem is that the https://nodemcu.readthedocs.io/en/dev/lfs/ page explicitly points to the _init.

I think that the dofile wrapper is probably useful for people converting code from a non-lfs setup to a lfs setup.

@TerryE
Copy link
Collaborator Author

TerryE commented Jul 16, 2020

I am in the process of redrafting the LFS page to be consistent with the new dual LFS setup and on-ESP LFS creation. I will update _init in the interim so that those whose apps just dofile it at least won't break. That will include dofile and loadfile pathing for now.

@stale
Copy link

stale bot commented Jul 11, 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 11, 2021
@stale stale bot closed this as completed Jul 30, 2021
@TerryE
Copy link
Collaborator Author

TerryE commented Sep 26, 2021

I can't resource this, so I think this should remain close unless one of our contributors who is primarily a Lua developer steps up. If so then reopen it on your own behalf.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants