tobozo / ESP32-targz

🗜️ An Arduino library to unpack/uncompress tar, gz, and tar.gz files on ESP32 and ESP8266
Other
123 stars 16 forks source link

#include should not depend on #define #30

Closed frankcohen closed 3 years ago

frankcohen commented 3 years ago

I like your library and am using it in the Reflections project (https://github.com/frankcohen/ReflectionsOS). It took me a while to debug it in my project. My target is an ESP32 using an SD card. I wrote my code as:

include

define DEST_FS_USES_SD

Using TARGZUnpacker->tarGzExpander I see the normal log messages printed to the serial monitor. It stalls after the Decompressing message.

When I wrote my code as:

define DEST_FS_USES_SD

include

it works fine.

My opinion: Included libraries should not depend on definitions. Instead the type of storage should be an input parameter when initializing the library.

What do you think?

-Frank

tobozo commented 3 years ago

hey @frankcohen thanks for your feedback

The #define must be set before the include indeed, the main reason for that constraint is the support matrix.

fs::FS SPIFFS LittleFS SD SD_MMC FFAT
ESP32 1.0 1.0.5 1.0.5 1.0 1.0
ESP8266 builtin 0.1.0 0.1.0 n/a n/a

Although there are some affinities across Arduino packages, the fs::FS implementations slightly differ between ESP32 and ESP8266.

Moreover, the APIs aren't entirely consistent between filesystem implementations (e.g. check for free space on volume before decompression), so ESP32-Targz must take care of providing a unified API to this support matrix, and this is the least worst compromise I've found.

If you have a better strategy for that I'm totally open to suggestions.

frankcohen commented 3 years ago

I hear you on the reasons why the library needs to know the file system type in advance. The whole SD thing seems like a mess to me - I'm used to Java and Python. I also spent days trying to get SDFAT to work, without success. Here's what I recommend: Update the docs to point out that the #define DEST_FS_USES_SD needs to come before the #include, OR adopt this kind of initializer: TarGzUnpacker *TARGZUnpacker = new TarGzUnpacker( DEST_FS_USES_SD ); -Frank

tobozo commented 3 years ago

I've updated the readme but I'll also start thinking of a more modular approach.

But ESP32 and ESP8266 are very different worlds, and I'm not sure I can find a solution that fits both without improving my C++ skills first (especially templates, namespaces, friend classes and the enigmatic constexpr).

So I'm not closing a door but it'll probably take some time before this library can see the results of my own upskilling.

frankcohen commented 3 years ago

Please take my post as a suggestion.... you're doing great to deal with all of these technologies and deliver a well done library. Thank you.