Skip to content

WIP -Make SPIFFS and LittleFS stay out of link when not needed #6697

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

Closed

Conversation

earlephilhower
Copy link
Collaborator

@earlephilhower earlephilhower commented Nov 1, 2019

Fixes #6691

Idea is to use same principal of weak references to allow the call to
SPIFFS.end() only to ever be linked in (along with all of SPIFFS) if
there is a call somewhere in the app to SPIFFS.begin().

The biggest use case is httpUpdateServer which includes both LittleFS
and SPIFFS code because it has embedded SPIFFS/LittleFS.end() calls.

Standard apps need no changes and continue using standard calls.

Users of the HttpUpdateServer will automatically save an extra 10-40K
of code if they do not use one or both filesystems, or will be
unaffected if they use both.

Fixes esp8266#6691

Idea is to use same principal of weak references to allow the call to
SPIFFS.end() only to ever be linked in (along with all of SPIFFS) if
there is a call somewhere in the app to SPIFFS.begin().

The biggest use case is httpUpdateServer which includes both LittleFS
and SPIFFS code because it has embedded SPIFFS/LittleFS.end() calls.

Standard apps need no changes and continue using standard calls.

Users of the HttpUpdateServer will automatically save an extra 10-40K
of code if they do not use one or both filesystems, or will be
unaffected if they use both.
@earlephilhower earlephilhower changed the title WIP - Make SPIFFS and LittleFS stay out of link when not needed Make SPIFFS and LittleFS stay out of link when not needed Nov 1, 2019
@earlephilhower
Copy link
Collaborator Author

Using objdump I see this PR removing the littlefs/spiffs code when I don't have an explicit call to SPIFFS.begin() in my user app.

HOWEVER This needs a runtime verification by @TD-er or others to make sure that spiffs.end/littlefs.end are still being called.

You'll need to add a Serial.printf("SPIFFS_end called\n"); to cores/esp8266/spiffs_api.h line 201 and see it print that out in the case when you use the ESP8266HttpUpdateServer to do an upload.

I don't have the HW handy now so can't run on real chip myself, and will be unable to try it for a good week or so...

@earlephilhower earlephilhower changed the title Make SPIFFS and LittleFS stay out of link when not needed WIP -Make SPIFFS and LittleFS stay out of link when not needed Nov 1, 2019
@earlephilhower
Copy link
Collaborator Author

Marking as WIP because I believe it still to be broken.

I added a printf("UNIQUESTRING"); to the weak function that does the LITTLEFS.end() call (littlefs_weak_end_redefinable) but I'm not finding that string in the generated binary which indicates the weak function is not being overridden properly and it's just never calling xxx.end() in the weak overrides.

@TD-er
Copy link
Contributor

TD-er commented Nov 1, 2019

As I said, I didn't know how this strong/weak link stuff works in C++, so I started Googl'ing and came across this very elaborate reply: StackOverflow - attribute((weak)) and static libraries
The stackoverflow question is about linking against static libraries, but I guess it does apply also here.

The thing I'm wondering about is, are we trying to fix it in the right place?
Does the updater class need to know all supported file system types?
I would say it just needs to call (if it is considered responsibility of this class) some very generic FS.end() and in there we should deal with all available filesystems.

In traditional coding, you will have a void pointer or a pointer to some base class, which does have to be checked for nullptr. If it is a valid pointer, it should be either casted to the right class, or the end() function should be strictly virtual in the base class.

Another way could be to have some factory design pattern to deliver a valid new instance of the object. The presence of such factory design pattern does suggest to destruct the FS class and thus end() it and re-create it of the type needed when we need access to the filesystem again and the pointer holding it is nullptr.
This factory class would then be the single place handling all the magic of having the includes and creating the right object type.
The most simple way of including/excluding unused code would be to simply use defines, but I do realize that does require quite some maintenance and also is for sure not the most pretty code out there.

The approach displayed in this PR is doing some hocus-pocus in juggling with multiple weak implementations of the same function, which does seem to be order dependent on the files processed during compile.
Please correct me if I'm wrong, since I really don't understand how this is supposed to work.
For example, how do you guarantee what order these two are being processed:

  • libraries/LittleFS/src/littlefs_weak_internal.cpp - Implements the weak littlefs_weak_end_redefinable which does call nothing.
  • libraries/LittleFS/src/littlefs_weak.cpp - implements the strong littlefs_weak_end_redefinable which does call LittleFS.end();

If the compile/link step does already have littlefs_weak_end_redefinable resolved by using the weak one, then the strong one will not be looked for.
But I would expect it will use the strong one, since there is a strong one present which will be chosen instead of the weak one.

Given you do see the compiled binary has been decreased in size, I guess the weak one is used.
I don't know how GCC will process intermediate object files if the files they refer to have not been changed.
I would expect these kind of compile sessions must have a clean build if you would change what filesystem is used.

@d-a-v
Copy link
Collaborator

d-a-v commented Nov 2, 2019

I would say it just needs to call (if it is considered responsibility of this class) some very generic FS.end() and in there we should deal with all available filesystems.

That's true, I though of that too, I'll update #6699

In traditional coding, you will have a void pointer or a pointer to some base class, which does have to be checked for nullptr. If it is a valid pointer, it should be either casted to the right class, or the end() function should be strictly virtual in the base class.

That's right too. There'd be code to check which FS is used. A bit of complexity.
OTOH using weak functions does the same job automagically at link time.

@TD-er about complexity discussion please head to #6699 .
This PR has been made from the WPS model in which there are unrelated coding tricks (custom_function, "weakref") which are after all unnecessary in this case.

@devyte
Copy link
Collaborator

devyte commented Nov 4, 2019

Closing in favor of #6699 .

@devyte devyte closed this Nov 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LittleFS library linked when not used
4 participants