search

joltwallet / esp_littlefs

LittleFS port for ESP-IDF
MIT License
259 stars 97 forks source link

Use littlefs-python instead of mklittlefs #140

Closed ianlevesque closed 1 year ago

ianlevesque commented 1 year ago

This is a completely different approach, so I am curious what your thoughts are. I struggled with the, essentially total lack of, Windows support for mklittlefs in this component and came up with this alternative. Fundamentally ESP-IDF doesn't guarantee that you'll have a working host-targeting compiler present on your machine, and CMake doesn't support targeting both the host and ESP from one project either, making the previous shelling out to make with a Makefile for mklittlefs approach a bit of a hack. For SPIFFS, Espressif made a pure python 'spiffsgen.py' - probably because the ESP-IDF itself does depend upon, and guarantee a python environment. littlefs-python exists, and includes a code sample to make an image, which I adapted here. It seems to be actively maintained, and avoids the lack of host compiler situation, despite using the native littlefs, by providing prebuilt binary wheels on pypi.

I tried to keep this correct and clean by using the ESP-IDF python to create an isolated virtualenv in the cmake build directory, installing a known-compatible version of littlefs-python there, and then using it to create littlefs partition images. This avoids polluting the ESP-IDF python environment with our extra package, even though doing so would probably be harmless. I tested this on Windows 11 and macOS 13, with ESP-IDF 5.1.1.

If acceptable, this would make everything 'just work' on Windows (and not break existing platforms).

BrianPugh commented 1 year ago

I'm one of the contributors to littlefs-python and actually added some CI for some of the prebuilt wheels 😄 .

I'd be totally onboard with replacing mklittlefs with littlefs-python and a script. I'd actually prefer if we add a CLI to the littlefs-python package (seems generically useful, and would avoid another script in this repo), it'd be pretty easy to do. I wonder if @jrast would be on board.

I'm spread a little thin at the moment between several projects, but if you could open a PR, I'd be available to review/merge.

ianlevesque commented 1 year ago

Well that's a small world! This is all a hobby outside $DAYJOB so I'm not sure when, but I'll clean up / move the script part of this over to a PR against littlefs-python then and adjust the cmake changes here to use it.

BrianPugh commented 1 year ago

If making a PR to littlefs-python, I'd recommend adding a __main__.py and all that so that it can be accessed as a normal CLI app. You can see an example of that here:

https://github.com/pazzarpj/micropython-ustubby/pull/25/files

Not sure if geky is using the littlefs cli name, but it might be good to name this cli littlefs-python to prevent future colllisions.

jrast commented 1 year ago

I had some spare time and put together a minimal (and ugly coded) CLI for littlefs-python: https://github.com/jrast/littlefs-python/pull/50

Please add your ideas and inputs in the comments of the pull request.

ianlevesque commented 1 year ago

Please add your ideas and inputs in the comments of the pull request.

I added two suggestions that would make it easier to integrate here. A lot cleaner than my hack!

ianlevesque commented 1 year ago

Updated to use the (prerelease) version of https://github.com/jrast/littlefs-python/pull/50 for discussion. Not quite mergeable until there's an actual release.

BrianPugh commented 1 year ago

I like this a lot more 😄 . I don't think I have any feedback on anything that needs to change, but yeah lets wait for a new littlefs-python release.

jrast commented 1 year ago

New Version 0.7.0 of littlefs-python is released! Enjoy!

BrianPugh commented 1 year ago

@ianlevesque before merging this, I need to think about LFS_NAME_MAX a little more. I think it's actually fine if it's anything <=255, but I don't think its fully piped through in littlefs-python. My main fear/thoughts:

  1. Currently, in esp_littlefs, the default LFS_NAME_MAX is 64, which might be a bit on the short side, but we're kind of stuck with it.
  2. If we merge this PR in, this might throw off anyone who currently has an LFS_NAME_MAX of 64 who prebuilds images.
  3. We could fully pipe through LFS_NAME_MAX in littlefs-python, it seems like lfs supports a configuration making the name shorter, but not longer.
  4. Can existing littlefs systems work if a longer LFS_NAME_MAX is specified than when the filesystem was created?

Basically, I just don't want to accidentally break anyones workflow or systems.

ianlevesque commented 1 year ago

Yes I believe 3 & 4 to be true based on playing with the earlier version of this PR. I believe setting a high compile-time default on the python binaries and passing the value through will solve the issue.

BrianPugh commented 1 year ago

alright, I'll try and make an upstream littlefs-python PR for that tonight!

BrianPugh commented 1 year ago

once https://github.com/jrast/littlefs-python/pull/55 is merged/released, we'll need to perform the following actions here.

ianlevesque commented 1 year ago

Appreciate all the effort to get this right!

BrianPugh commented 1 year ago

@ianlevesque jrast has merged in the PR and released 0.7.1 that has these changes. If you could make the changes mentioned above, i'll merge and cut a release!

BrianPugh commented 1 year ago

@ianlevesque sorry for mucking up your branch a little, i'll try and merge this tomorrow; esp-idf's provided docker containers for building have some absolutely ancient code (super old git, super old python) and it's been a pain. Will try and get this sorted soon.

ianlevesque commented 1 year ago

No worries, change whatever.

BrianPugh commented 1 year ago

merged via #143

BrianPugh commented 1 year ago

included in release v1.9.0