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

Adding LITTLEFS w/o rebuild the IDF #4096

Closed
wants to merge 17 commits into from
Closed

Adding LITTLEFS w/o rebuild the IDF #4096

wants to merge 17 commits into from

Conversation

lorol
Copy link
Contributor

@lorol lorol commented Jun 17, 2020

based on https://github.com/joltwallet/esp_littlefs

This will be my first PR, hope will do it right.
Details in README

@lorol lorol mentioned this pull request Jun 17, 2020
lorol added a commit to lorol/arduino-esp32littlefs-plugin that referenced this pull request Jun 18, 2020
lorol added a commit to lorol/LITTLEFS that referenced this pull request Jun 18, 2020
Copy link
Collaborator

@atanisoft atanisoft left a comment

Choose a reason for hiding this comment

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

since this is embedding https://github.com/joltwallet/esp_littlefs it would be good to have sign-off from @joltwallet / @BrianPugh since some files are showing mixed/missing licensing.

This PR will also need to be updated for the esp32s2 branch which is based on IDF v4.2 (master) as there are VFS API changes which may break this functionality.

}
]
}
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

reformatting of an existing file (such as this one) is often discouraged and will often lead to the PR sitting for a very long time (if it ever gets merged).

If you do reformat the file, please ensure it is consistent as it doesn't appear to be consist after reformatting (looks like a mix of spaces and tabs)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@joltwallet and @BrianPugh
Could you please help in this?

//esp_err_t esp_littlefs_format(const char* partition_label);
//esp_err_t esp_littlefs_info(const char* partition_label, size_t *total_bytes, size_t *used_bytes);

#define LFS_NAME "spiffs"
Copy link
Collaborator

Choose a reason for hiding this comment

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

instead of a hardcode via DEFINE can you try:

static constexpr const char LFS_NAME[] = "spiffs";

This will prevent global namespace pollution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

CMakeLists.txt Outdated
libraries/LITTLEFS/src/esp_littlefs.c
libraries/LITTLEFS/src/lfs.c
libraries/LITTLEFS/src/lfs_util.c
libraries/LITTLEFS/src/littlefs_api.c
Copy link
Collaborator

Choose a reason for hiding this comment

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

please correct formatting on these additions.


using namespace fs;

class LITTLEFSImpl : public VFSImpl
Copy link
Collaborator

Choose a reason for hiding this comment

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

@me-no-dev would it be feasible to provide a default impl for VFSImpl that can be used by the various FS impls rather than each one copying/pasting the same but with a different name?

Copy link
Member

Choose a reason for hiding this comment

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

what is the same?

Copy link
Collaborator

Choose a reason for hiding this comment

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

nvm, this class can be dropped entirely.

@@ -0,0 +1,130 @@
// Copyright 2015-2016 Espressif Systems (Shanghai) PTE LTD
Copy link
Collaborator

Choose a reason for hiding this comment

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

adjust copyright year to 2020 (for all files with similar header)

@lorol
Copy link
Contributor Author

lorol commented Jun 18, 2020

since this is embedding https://github.com/joltwallet/esp_littlefs it would be good to have sign-off from @joltwallet / @BrianPugh since some files are showing mixed/missing licensing.

Asking Brian to review...

This PR will also need to be updated for the esp32s2 branch which is based on IDF v4.2 (master) as there are VFS API changes which may break this functionality.

Someone else should help here please to adapt the code to match older IDF, maybe @BrianPugh is the best to help with such branch of his library.

@BrianPugh
Copy link

BrianPugh commented Jun 18, 2020

Hey guys,

Some questions/feedback:

  1. Instead of copying source files from my repo, could we submodule them (or even just include arduino-related files in my repo, so long as they don't conflict with esp-idf compilation). This way upstream bug-fixes and features are easily maintained.

My action plans:

  • Update the LICENSE in my repo (could have swore i had MIT licensing on it). The LittleFS project is BSD3 licensed, which seems to play nicely so thats no big deal. Looping back to point 1 in questions/feedback, some action like that will need to be taken so that these upstream changes apply to this project.
  • As for VFS API changes, can @atanisoft link me either a file/commit/pr/doc that specifies the changes? I should be able to accomodate fairly easily (if it requires changes on my side).

edit 1: Added MIT license to my repo

@atanisoft
Copy link
Collaborator

  • As for VFS API changes, can @atanisoft link me either a file/commit/pr/doc that specifies the changes? I should be able to accomodate fairly easily (if it requires changes on my side).

IDF v4.0 changed a few things: https://github.com/espressif/esp-idf/releases/tag/v4.0, there are likely more changes beyond v4.0 that would need to be handled. You can detect the IDF version via a few compile time defines.

@BrianPugh
Copy link

  • As for VFS API changes, can @atanisoft link me either a file/commit/pr/doc that specifies the changes? I should be able to accomodate fairly easily (if it requires changes on my side).

IDF v4.0 changed a few things: https://github.com/espressif/esp-idf/releases/tag/v4.0, there are likely more changes beyond v4.0 that would need to be handled. You can detect the IDF version via a few compile time defines.

ah gotcha, I've been regularly using my repo off of esp-idf master for esp32 targets, so it should be good to go. I thought there may be even newer or esp32-s2 specific changes.

@atanisoft
Copy link
Collaborator

I've been regularly using my repo off of esp-idf master for esp32 targets, so it should be good to go.

Great! I don't know of any changes specific to the ESP32-S2 that will prevent this from working.

@lorol
Copy link
Contributor Author

lorol commented Jun 18, 2020

@BrianPugh Hi Brian,

It will be difficult for me to re-wrap the LITTLEFS.cpp and .h in such a way that your library is kept as it is and just fork the appropriate revision of it.
As I said earlier, i just dropped the files and tried until it works with minimal spread around.
I know it is an ugly hack - but I didn't find existing implementation.
@me-no-dev is busy with other priorities and LittleFS is even not in the main focus of ESP32 (at all).
Otherwise, someone may follow the entire procedure of re-component-ing Arduino under IDF then re-packing it and dropping all new *.a files plus includes where they should be.
Personally I prefer independent than embedded through *.a files way, especially if it works same way.
Maybe I'm getting too old to learn new things.

No problem if you rewrite it entirely around your wrap, especially if it can be brought back to Arduino w/o painful recompiling and IDF syncing.

LL

@atanisoft
Copy link
Collaborator

No problem if you rewrite it entirely around your wrap, especially if it can be brought back to Arduino w/o painful recompiling and IDF syncing.

The best way to avoid that would be have @BrianPugh submit a PR to ESP-IDF to have LittleFS included as a component there. Then it can be wrapped in arduino-esp32 in a clean manner (like other FS types)

@BrianPugh
Copy link

I'll make a PR, no harm in trying. I fear that there might be a lot of other requirements (writing up a lot of documentation, etc)

@lorol
Copy link
Contributor Author

lorol commented Jun 18, 2020

I wish you a patience and good luck with that :)
Will go silent for a while.

@BrianPugh
Copy link

Here's a link to my PR for littlefs in esp-idf:

espressif/esp-idf#5469

@lbernstone
Copy link
Contributor

If you've used the package, you know documentation is not always most thorough...
Having it as a separate module until is really robust is better than trying to get this in ASAP. Register it with Arduino and it will get just as much exposure as having it baked in.

@BrianPugh
Copy link

For registering, is that targeted at me or @lorol ? I don't really use arduino (and am unfamiliar with its package managers, library structures, and all that). I'm just here to help on actions requiring changes in my repo.

@lbernstone
Copy link
Contributor

Targeted at OP (@lorol), whom wants it in arduino-esp32.


using namespace fs;

class LITTLEFSImpl : public VFSImpl
Copy link
Collaborator

Choose a reason for hiding this comment

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

nvm, this class can be dropped entirely.

return (f == true) && !f.isDirectory();
}

LITTLEFSFS::LITTLEFSFS() : FS(FSImplPtr(new LITTLEFSImpl()))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Change this to:

LITTLEFSFS::LITTLEFSFS() : FS(FSImplPtr(new VFSImpl()))

and remove the definition of class LITTLEFSImpl. The customization is not necessary for LITTLEFS as the default behavior from VFSImpl() will work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@lorol
Copy link
Contributor Author

lorol commented Jun 19, 2020

Targeted at OP (@lorol), whom wants it in arduino-esp32.

Thanks but ,,,
Please let's wait first IDF team to determine the strategy there.
Meanwhile, whoever desperately needs to test sketches can use from here or drop in libraries.
Hope we will have somehow IDF version dependency resolved (buy #define or so)
BTW, If .utime_p = &vfs_littlefs_utime, is removed, then it builds and works with core release 1.0.4 (IDF v.3.2) too

@lorol lorol changed the title Adding LITTLEFS w/o rebuild the IDF. Compatible with 3.3. Adding LITTLEFS w/o rebuild the IDF. Compatible with IDF v3.3 Jun 19, 2020
- In esp_littlefs.c, added a way to build for older IDF too (IDF 3.2 is used on release 1.0.4)
#define CONFIG_LITTLEFS_FOR_IDF_3_2
@lorol lorol changed the title Adding LITTLEFS w/o rebuild the IDF. Compatible with IDF v3.3 Adding LITTLEFS w/o rebuild the IDF Jul 30, 2020
@lorol
Copy link
Contributor Author

lorol commented Sep 2, 2020

If you've used the package, you know documentation is not always most thorough...
Having it as a separate module until is really robust is better than trying to get this in ASAP. Register it with Arduino and it will get just as much exposure as having it baked in.

Request done: arduino/Arduino#10719

@me-no-dev
Copy link
Member

Here is how we will go for 2.0.0:

  • I will add to the build system joltwallet/esp_littlefs so that it will be available
  • @lorol you can then change this PR to not have the lfs files and be internal as opposed to submodule (that have proven problematic in the past)
  • we will have LittleFS in ESP Arduino 2.0.0

@me-no-dev
Copy link
Member

@lorol esp_littlefs has been added to the esp32s2 branch. You can try it there.

@lorol
Copy link
Contributor Author

lorol commented Nov 2, 2020

Ok, great! I see the lib.a and headers. Will try. I'll let you or Brian know if I need help to make it correct.

@lorol
Copy link
Contributor Author

lorol commented Nov 3, 2020

@me-no-dev
I created a folder in libraries and added the 2 files for test at a local clone.
It looks it may work as you requested :) just annoying warnings like:

hardware\espressif\esp32/tools/sdk/esp32/include/newlib/platform_include/sys/termios.h:97: warning: "B110" redefined
#define B110 3

hardware\espressif\esp32\cores\esp32/binary.h:65: note: this is the location of the previous definition
#define B110 6

hardware\espressif\esp32/tools/sdk/esp32/include/driver/include/driver/adc_common.h:368:11: note: declared here
esp_err_t adc2_vref_to_gpio(gpio_num_t gpio) attribute((deprecated));

Actually it builds w/o adding lfs files as submodule. I guess already built in through esp_littlefs?
I will test a bit more.
Any help to clean these warnings if possible?
Also since the branches are different, I don't see how to reuse this PR - maybe new one s2 to s2 ?

@me-no-dev
Copy link
Member

@lorol lfs is already in. you only need the two LITTLEFS source files :)

to fix the redefine warning, I need the full error trace, so we can see where they are both defined. ADC I'll fix later.

@me-no-dev
Copy link
Member

@lorol I'll suggest you open a pull request to the esp32s2 branch so we can give it a go (will need to add the tools, etc as well)

@lorol
Copy link
Contributor Author

lorol commented Nov 3, 2020

@me-no-dev @BrianPugh
Below is the Bxxxx redefinning chain.
If I comment the termios.h inclusion at esp_littlefs.h, warnings disapear. I asked Brian if termios is really necessary.

In file included from --\hardware\espressif\esp32/tools/sdk/esp32/include/esp_littlefs/include/esp_littlefs.h:16,
from --\hardware\espressif\esp32\libraries\LITTLEFS\src\LITTLEFS.cpp:23:
--\hardware\espressif\esp32/tools/sdk/esp32/include/newlib/platform_include/sys/termios.h:97: warning: "B110" redefined
#define B110 3

In file included from --\hardware\espressif\esp32\cores\esp32/Arduino.h:40,
from --\hardware\espressif\esp32\libraries\FS\src/FS.h:25,
from --\hardware\espressif\esp32\libraries\FS\src/vfs_api.h:18,
from --\hardware\espressif\esp32\libraries\LITTLEFS\src\LITTLEFS.cpp:17:
--\hardware\espressif\esp32\cores\esp32/binary.h:65: note: this is the location of the previous definition
#define B110 6

In file included from --\hardware\espressif\esp32/tools/sdk/esp32/include/esp_littlefs/include/esp_littlefs.h:16,
from --\hardware\espressif\esp32\libraries\LITTLEFS\src\LITTLEFS.cpp:23:
--\hardware\espressif\esp32/tools/sdk/esp32/include/newlib/platform_include/sys/termios.h:118: warning: "B1000000" redefined
#define B1000000 23

In file included from --\hardware\espressif\esp32\cores\esp32/Arduino.h:40,
from --\hardware\espressif\esp32\libraries\FS\src/FS.h:25,
from --\hardware\espressif\esp32\libraries\FS\src/vfs_api.h:18,
from --\hardware\espressif\esp32\libraries\LITTLEFS\src\LITTLEFS.cpp:17:
--\hardware\espressif\esp32\cores\esp32/binary.h:277: note: this is the location of the previous definition
#define B1000000 64

Also you need to apply:
882b12c
Otherwise dirctories cannot be removed Arduino-wise.

I will cleanup the LITTLEFS examples and try to do a PR at esp32s2 branch (with some hacky tools addition proposal to work with this https://github.com/lorol/arduino-esp32fs-plugin - of course you may need to do it in better way)

Observed huge RAM heap difference of my sketch between latest esp32s2 and master branches - about 70K more taken on s2. Board is TTGO T1

@BrianPugh
Copy link

@lorol I don't believe we need termioh.h, including it must have just been a mistake.

@me-no-dev
Copy link
Member

@lorol I am aware of the ~70k issue. Need to figure out where those bytes went.

I also agree with @BrianPugh

@me-no-dev
Copy link
Member

@lorol rmdir fix is applied. pull the changes

@me-no-dev
Copy link
Member

closing now :)

@me-no-dev me-no-dev closed this Apr 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: For reference Common questions & problems
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants