PaulStoffregen / LittleFS

72 stars 20 forks source link

memory leak? #26

Open raphaelvalentin opened 2 years ago

raphaelvalentin commented 2 years ago

Hi Paul, I have the interest to use LittleFS with an STM32F7 and a NOR FLASH chip (SST26FV). My project application is about to serve web pages. Due to some code incompatibilities, I am adapting a wrapping library (e.g. File class does not exist, etc.). I am using your library as a great source of inspiration for its clarity and simplicity of use. However:

(1) I am not sure. Is it possible that in method File open(const char *filepath, uint8_t mode = FILE_READ) {, a free is missing at line 231?

                } // else FILE_WRITE_BEGIN
                return File(new LittleFSFile(&lfs, file, filepath));
            }
        }
        return File();
    }

(2) Accessory, does this call to malloc can possibly generate memory fragmentation? If so, is there a way to move the call to lfs_file_open into LittleFSFile(...) and remove the memory allocations (or at least offer the possibility to pre-allocate at run-time with a memory buffer)?

Thanks for your support, Raphael.

FrankBoesing commented 2 years ago

Might be better to use the original: https://github.com/littlefs-project/littlefs

FrankBoesing commented 2 years ago

This is a fork (although it's no mentioned anywhere)

PaulStoffregen commented 2 years ago

The memory is freed here: https://github.com/PaulStoffregen/cores/blob/6ceb275a17e579fddafba9f428ad7da03b6d0ffb/teensy4/FS.h#L249

As far as I know it's not leaking memory when all File references are closed or go out of scope. But I try to never say my code is 100% bug free. If there is a bug, I'm not aware of it yet.

Fragmentation may be possible depending on usage.

I am not aware of any way to support the Arduino File API across multiple libraries without dynamic memory allocation. If some way exists, I would very much like to learn of it. As far as I know, ESP8266 and ESP32 are the only other boards offering a similar cross-library Arduino File abstraction at this time. They too use dynamic allocation managed by std::shared_ptr.

The original littlefs project is C-only. It does not have any Arduino-like API.

This repository's readme file says "This is a wrapper for the LittleFS File System for the Teensy family of microprocessors", so I would not agree with the claim that it's not mentioned anywhere.

raphaelvalentin commented 2 years ago

Hi Paul, I appreciate your reply. Thank you for your explanations. I understand the purpose of the wrapper to Teensy MCUs. (1) At this moment, for what is worst, I have this (not sure of 100% compatibility with Arduino API; it can be optimized better):

#define FILE_READ       (O_READ)
#define FILE_WRITE      (O_RDWR | O_CREAT)

class File
{
private:
    lfs_t *lfs;
    lfs_file_t file;
    lfs_dir_t dir;
    char fullpath[LFS_NAME_MAX+1];
    int type;

private:
    File(lfs_t *lfsin, const char *name, int mode)
     : lfs(lfsin), dir({0}), type(LFS_TYPE_REG) {
        if (lfs_file_open(lfs, &file, name, mode) < 0) {
            type = 0;
        }
        strlcpy(fullpath, name, sizeof(fullpath));
    }
    File(lfs_t *lfsin, const char *name)
     : lfs(lfsin), file({0}), type(LFS_TYPE_DIR) {
        if (lfs_dir_open(lfs, &dir, name) < 0) {
            type = 0;
        }
        strlcpy(fullpath, name, sizeof(fullpath));
    }
    friend class LittleFS;

public:
    File() : lfs(nullptr), file({0}), dir({0}), type(0), fullpath("") {
    }
    File open(const char *filepath, unsigned int mode = FILE_READ) {
        if (!mounted) return File();

        if (mode == FILE_WRITE) {
            return File(&lfs, filepath, O_RDWR | O_CREAT);
        }
        else {
            struct lfs_info info;
            if (lfs_stat(&lfs, filepath, &info) < 0) {
                return File();
            }

            if (info.type == LFS_TYPE_REG) {
                return File(&lfs, filepath, O_READ);
            }
            else {  // LFS_TYPE_DIR
                if ((mode & O_WRITE) == O_WRITE) return File();
                return File(&lfs, filepath);
            }
        }
        return File();
    }

However, a declaration of a File consumes a bit more memory (~struct lfs_file_t + lfs_dir_t sizes), but, I get rid of any malloc calls. In my case, with the "basic environment", I have seen (without digging out too much in the HAL and config files) that malloc calls can induce the STM32F7 to crash. Issues of malloc calls are discussed over the internet.

(2) Concerning fragmentation, a typical case could be: you create instantiations of new (linked) objects inside a (recursive) list dir.

(3) Accessory, I have seen an interesting library static_malloc from https://github.com/luni64/static_malloc. I have not taken too much time to test it in detail; however, I like the idea for constraint memory MCUs: allocate statically a number of bytes at instantiation, then use this block to store structured data. I do agree that it looks like the malloc function, but you gain better controls with pre-defined boundaries. Higher consumption of memory is a drawback but stability is a must.

Friendly,

PaulStoffregen commented 2 years ago

Unless there's reason to believe a bug exists in this particular library meant for use on Teensy, I believe it's time to close this issue.

There are many places to discuss development of code for STM32 and other non-Teensy platforms. This isn't the right place.