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

File LFS Lua module initial commit #3332

Merged
merged 9 commits into from May 6, 2021
Merged

Conversation

vsky279
Copy link
Contributor

@vsky279 vsky279 commented Nov 15, 2020

Fixes #3329.

Make sure all boxes are checked (add x inside the brackets) when you submit your contribution, remove this sentence before doing so.

  • 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.
  • The code changes are reflected in the documentation at docs/*.

Support for arbitrary files stored in LFS

Usage

The module is intended to overlay standard file module. If file is present in SPIFFS then this version is preferred. If it is not present then read only access (of course) to the file in LFS is provided.

The following functions for LFS files are supported:

  • file.exists()
  • file.open()
  • file.getcontents()
  • file.read(), file.obj:read()
  • file.readline(), file.obj:readline()
  • file.seek(), file.obj:seek()

Other functions are just passed-through to the standard file module.

When file is embedded in LFS, no standard userdata file structure is created. Instead a Lua object is created.

Example

file = require("file_lfs")

local idx = file.open("index.html") -- file not in SPIFFS but in resource.lua in LFS
print(idx:readline())
idx:seek("set", 0)
print(file.readline(idx))

idx = file.open("favicon.ico", "r") -- file not in SPIFFS but in resource.lua in LFS
print(file.read(512))
file.seek(idx, "cur", 100)
print(idx:read(16))
idx:close()

local i = file.open("init.lua") -- files in SPIFFS are accessible using same routines
print(i:readline())

Also test file for the testing framework (#2983) is provided.

@vsky279 vsky279 force-pushed the c_file_lfs branch 2 times, most recently from e3c69df to 5bc0282 Compare November 15, 2020 19:13
@pjsg
Copy link
Member

pjsg commented Nov 15, 2020

I like this -- I built something similar (though not so general purpose) for an application that I am working on. This is definitely a better approach!

@HHHartmann
Copy link
Member

@vsky279 could you please not force push? We can squash the commits when merging and the reviewer will have to read everything over again and again.

@vsky279
Copy link
Contributor Author

vsky279 commented Nov 16, 2020

Sure, no problem. So I will pile up commits that we squash once ready to be merged.

@nwf
Copy link
Member

nwf commented Nov 17, 2020

I'm delighted to see NTest catching on! :D

Copy link
Member

@HHHartmann HHHartmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also review my comments in #3329
I feel that a file enhancement should work as usual and throw errors where normal operation can not be guaranteed.
Given the following code

if file.exists("myfile") then file.rename("myfile", "myotherfile") end
print(file.getcontents("myotherfile"))

should work, regardless of myfile being in SPIFFS or LFS.
An alternative would be to throw a meaningful error that the operation is not possible.
Atm copy from a non existing file throws an error (I hope) but with puzzeling message.

Maybe some code to smoothly access the resource like a file might be more useful without blending with file.

docs/lua-modules/file_lfs.md Show resolved Hide resolved
docs/lua-modules/file_lfs.md Outdated Show resolved Hide resolved
docs/lua-modules/file_lfs.md Outdated Show resolved Hide resolved
lua_modules/file_lfs/file_lfs.lua Outdated Show resolved Hide resolved
lua_modules/file_lfs/file_lfs.lua Outdated Show resolved Hide resolved
tests/NTest_file_lfs.lua Show resolved Hide resolved
lua_modules/file_lfs/make_resource.lua Outdated Show resolved Hide resolved
lua_modules/file_lfs/file_lfs.lua Outdated Show resolved Hide resolved
lua_modules/file_lfs/file_lfs.lua Outdated Show resolved Hide resolved
@vsky279
Copy link
Contributor Author

vsky279 commented Nov 19, 2020

Thanks Gregor for all these comments!

Please also review my comments in #3329

I'm sorry I missed these ones.

open modes

I suggest the following LFS file behavior:

  • "r": standard mode
  • "w": opens file of the same name on SPIFFS
  • "a", "r+", "w+", "a+": copy LFS file content to SPIFFS and opens with given open mode

file.getcontents()

We already have file.getcontents() - this needed to be implemented for the test suite.

file.rename()

Renaming non existing file returns false rather than raising error. So I guess an attempt to rename LFS file should return false too.
It's implemented now. But the implementation is a bit useless as calling directly file.rename with LFS filename (as if file_lfs.rename was not implemented) would return false anyway.

file.stat()

 :thumbsup:

file.open("index.html")
print(file.readline())

Oups. I didn't know this approach is still possible. Implemented.

@vsky279 vsky279 force-pushed the c_file_lfs branch 3 times, most recently from 36192c2 to 953f49a Compare February 12, 2021 21:56
Copy link
Member

@HHHartmann HHHartmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the long delay.
Now again reviewed and tested and found some issues but then we should be done I think.

mkdocs.yml Outdated Show resolved Hide resolved
lua_modules/file_lfs/file_lfs.lua Outdated Show resolved Hide resolved
lua_modules/file_lfs/file_lfs.lua Show resolved Hide resolved
lua_modules/file_lfs/file_lfs.lua Outdated Show resolved Hide resolved
lua_modules/file_lfs/file_lfs.lua Show resolved Hide resolved
lua_modules/file_lfs/file_lfs.lua Show resolved Hide resolved
lua_modules/file_lfs/file_lfs.lua Outdated Show resolved Hide resolved
lua_modules/file_lfs/file_lfs.lua Show resolved Hide resolved
tests/NTest_file_lfs.lua Outdated Show resolved Hide resolved
tests/NTest_file_lfs.lua Outdated Show resolved Hide resolved
@vsky279
Copy link
Contributor Author

vsky279 commented Mar 14, 2021

Gregor, thanks for this in-deep review. 👍
Everything should be handled now.

@vsky279
Copy link
Contributor Author

vsky279 commented Mar 14, 2021

I can't find anywhere what is the limitation for the string length in LFS.
53kB string is too long - LFS image that includes such string cannot be flashed.

@HHHartmann
Copy link
Member

Looking forward to this working. Will use it in my current project to speed up startup.

@vsky279
Copy link
Contributor Author

vsky279 commented May 5, 2021

Can we merge this? I think everything has been adressed.

@HHHartmann
Copy link
Member

Yes. Sorry for the delay

@HHHartmann HHHartmann merged commit 949875d into nodemcu:dev May 6, 2021
@HHHartmann HHHartmann added this to the Next release milestone May 6, 2021
@vsky279 vsky279 deleted the c_file_lfs branch August 27, 2021 22:10
nwf pushed a commit that referenced this pull request Dec 30, 2021
* File LFS module initial commit

* LFS file module update #1

* LFS file module update #2 - doc update and file.stat() returning read only attribute

* Implementing file.list()

* Fine-tuning `file_lfs` module

* Adding `file_lfs` to mkdocs.yml

* Implementing file.list() update #1

* Fine-tuning

* Fine-tuning #2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants