joltwallet / esp_littlefs

LittleFS port for ESP-IDF
MIT License
254 stars 95 forks source link

Accept pull request to add library.json in root ? #164

Closed EvEggelen closed 8 months ago

EvEggelen commented 8 months ago

To support PlatformIO ini files with IDF framework, adding a library.json would help. With the following line you could add the library to the project : lib_deps = https://github.com/joltwallet/esp_littlefs.git

To my knowledge this can be done by adding a library.json in the root of the esp_littlefs repository. The content would like something like this:

{
  "name": "esp_littlefs",
  "version": "0.0.0+20231217184442",
  "build": {
    "srcFilter": "+<*> -<littlefs/runners> -<littlefs/benches> -<littlefs/tests>",
    "flags": [
      "-I ./src/littlefs/"
    ]
  }
}

When successfully tested, would you accept a pull request ?

BrianPugh commented 8 months ago

I don't use PlatformIO, but sounds like it'd be useful to many people! I'd merge that PR.

Just make sure to also update .bumpversion.cfg!

EvEggelen commented 8 months ago

O.K. I will clone the repository and give it a try.

EvEggelen commented 8 months ago

I cloned the repository and added the library.json file. You can now use it within PlatformIO by added the following line the to platform.ini file. lib_deps = https://github.com/EvEggelen/esp_littlefs.git

I also updated the README.md to help use with PlatformIO. I found that IDF support still has limitations regarding the Kconfig files. The tickets are mentioned in the README.md.

I needed to make one "nasty change" that you maybe are not willing to take in. I had to make a link from Kconfig.projbuild to Kconfig. So far I did not manage to get platformIO read the Kconfig ( without the extension ). But maybe this is a limitation of my knowledge.

Kconfig.projbuild

Let me know if you have ideas to improve what I have done.

P.s. I could this not get to work:

get_filename_component(configName "${CMAKE_BINARY_DIR}" NAME) list(APPEND kconfigs "${CMAKE_SOURCE_DIR}/.pio/libdeps/${configName}/esp32-camera/Kconfig")

BrianPugh commented 8 months ago

So I don't have any real experience with PlatformIO, so I cannot add much input/assistantce.

EvEggelen commented 8 months ago

Would it be a problem for your project to rename the Kconfig to Kconfig.projbuild ? Or can you live with both Kconfig and Kconfig.projbuild ?

BrianPugh commented 8 months ago

so long as it still works with esp-idf, I'm fine with Kconfig.projbuild.

EvEggelen commented 8 months ago

I just fixed the issue. Now only the original Kconfig is sufficient. So please review my additions in the the README.md file. I tested the changes and everything seems to work O.K. with the latest PlatformIO and your lib.

The only 'issue' is a static version number in the library.json. Currently I set it to : "version": "0.0.0". Do you know a trick to automatically set this to the official version number ( so without editing the file every time ) ?

BrianPugh commented 8 months ago

Can you open up a PR for me to review?

For versioning, this is what I was referring to here about the bumpversion config. Basically just reference it in that file, and set the current version in the file so that bumpversion will automatically update it.

EvEggelen commented 8 months ago

Never use the bumpversion tool ( used others) before. But indeed this is what I was looking for. Hope that I made the changes correctly.

I created a pull request. Please review before merging. Let me know any remarks or comments.

BrianPugh commented 8 months ago

merged! this is a part of v1.12.0

EvEggelen commented 8 months ago

Great. However I noticed that you changed the command for running the menuconfig. If you use a standard install ( install Visual Studio Code, within Visual Studio code install PlatformIO extension. En then within PlatformIO install IDF framework) , pio is NOT put in the search path ( and triggers an advice to install it). IDF within PlatformIO has it quirks. So I recommend that you mention in the README.md that with a default install you need to use the following command in the terminal within PlatformIO: ~/.platformio/penv/bin/pio run -t menuconfig.

BrianPugh commented 8 months ago

Aren't you supposed to add ~/.platformio/penv/bin/ to your PATH?

EvEggelen commented 8 months ago

I normally expect that the installer takes care of this so you do not need to manually add it ( as the rest is automated). As I wanted to lower the threshold for using this lib, I added it. If you do not know better, and follow the message to install pio with apt-get, you run the risk ending up version conflicts.

I do not add it to the PATH as the directory is managed by PlatformIO platform installer. The developers willing to add this to the PATH also know to remove the directory from the command. When uninstalling the platform from PlatformIO you do not need to forget to manually remove it.

Anyway, this is what I though was best to do. It is not black and white.

BrianPugh commented 8 months ago

Looking into it, this is PlatformIO's virtual environment, so you're really supposed to activate it (source ~/.platformio/penv/bin/activate), which appropriately add its to the path. I think the CLI/terminal button does this for you (but I've never used it, so I don't personally know). I don't believe it's appropriate to directly execute ~/.platformio/penv/bin/pio.

EvEggelen commented 8 months ago

You are right. If you use the terminal button it is indeed activated. However if you do terminal -> new it is not. Also the default terminal that is created is also not activated. So I learned something today. Thanks ! Maybe we should mention this as a note.

I just noticed that there 2 types of terminals. A bash ( the default ) and a PlatformIO CLI one. If you press the terminal button you get a PlatformIO CLI that had the environment activated. Good that you mentioned this. So far I never noticed this ( as my windows indicating the type was so small and therefore dropping this info )