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

what to use instead of lfs_file_open() when LFS_NO_MALLOC is defined #720

Open
lidavid opened this issue Aug 16, 2022 · 11 comments
Open

what to use instead of lfs_file_open() when LFS_NO_MALLOC is defined #720

lidavid opened this issue Aug 16, 2022 · 11 comments

Comments

@lidavid
Copy link

lidavid commented Aug 16, 2022

#614 made the use of lfs_file_open() not possible when compiling with LFS_NO_MALLOC defined. What function should I use instead to open files?

@sigsstock
Copy link

Take a look at lfs_file_opencfg.

    uint8_t cache[256] = { 0 };
    struct lfs_file_config fileCFG = { 0 };
    int flag = LFS_O_RDWR | LFS_O_CREAT;

    fileCFG.buffer = cache;

    if (lfs_file_opencfg(&G_lfs, &file, <FILE_NAME>, flag, &fileCFG) == LFS_ERR_OK) {

    }

@lidavid
Copy link
Author

lidavid commented Aug 17, 2022

Thank you.

I understand correctly that this cache buffer must be allocated per file ?
There is no way to allocate it ahead in struct lfs_config ?

@GaryJSAdv
Copy link

If there's potential to have multiple files open at one time then yes. I have multiple mounts, on one of my mounts, I have a buffer that's shared between files, but I have mechanisms to prevent multiple files getting opened at the same time (it's a mount that's reserved for logging).

@rockyHead
Copy link

does lfs_remove() will still work with lfs_file_opencfg?

@geky
Copy link
Member

geky commented Sep 7, 2022

Yes, lfs_file_opencfg is equivalent to lfs_file_open but can avoid malloc. lfs_remove does not care how the file was created.

EDIT: Below this line is wrong. See follow up comments. Thanks @Xywzel!

You could also provide a custom LFS_MALLOC if you don't want to use lfs_file_opencfg. Note this one only allows one file open at a time:

// -DLFS_MALLOC=my_malloc
// -DLFS_FREE=my_free

uint8_t buffer[1024];
bool buffer_in_use = false;

void *my_malloc(size_t size) {
    assert(size <= 1024);
    assert(!buffer_in_use);
    buffer_in_use = true;
    return buffer;
}

void my_free(void *p) {
    assert(p == buffer);
    buffer_in_use = false;
}

@geky geky added the question label Sep 7, 2022
@Xywzel
Copy link

Xywzel commented Sep 21, 2022

It doesn't look like LFS_MALLOC is used anywhere in the codebase. How does whatever is provided with that macro definition get used in the functions?

@geky
Copy link
Member

geky commented Sep 21, 2022

Ah! You're right. I should have double checked. That must have been a bit disorienting.

You can achieve the same effect by providing your own custom my_lfs_util.h file and defining -DLFS_CONFIG=my_lfs_util.h file during compilation. However this is a bit more tedious. If you do the functions would need to stay named lfs_malloc and lfs_free.

@Xywzel
Copy link

Xywzel commented Sep 22, 2022

Yeah, we opted to just adding our own microcontroller specific malloc and free trough our own version of the lfs_util.h, but for future versions, passing these as either function pointers in struct lfs_config or with macro definition before include of lfs.h would likely be a good option.

@geosmall
Copy link

geosmall commented May 12, 2023

I've been implementing a file system that uses LFS on SPI NOR flash, and FatFS for SD Cards for a little while now. I wanted to stick to static allocations, it took me a bit to understand lfs_file_opencfg vs lfs_file_open to avoid malloc, but seems clear enough now.

I understand the need to conserve ram on tiny targets. Might it be clearer to adopt the strategy that FatFS uses, where the buffer is part of the file descriptor and there is a "LFS_FS_TINY" flag that moves the buffer to the file system level if so desired. This way there would be only one File_Open method and possibly be less confusing. Just a suggestion.

See:
https://github.com/RIOT-OS/FatFS/blob/48078c12d2ce8dd8ec629ffa71d11c7692552cee/source/ffconf.h#L220
and
https://github.com/RIOT-OS/FatFS/blob/48078c12d2ce8dd8ec629ffa71d11c7692552cee/source/ff.h#L171

/*---------------------------------------------------------------------------/
/ System Configurations
/---------------------------------------------------------------------------*/

#define FF_FS_TINY		0
/* This option switches tiny buffer configuration. (0:Normal or 1:Tiny)
/  At the tiny configuration, size of file object (FIL) is shrinked FF_MAX_SS bytes.
/  Instead of private sector buffer eliminated from the file object, common sector
/  buffer in the filesystem object (FATFS) is used for the file data transfer. */

@geky
Copy link
Member

geky commented May 17, 2023

Merging the static buffer into lfs_file_t is an interesting idea. Though I wonder if it comes with surprises for users by potentially allocating a "large" object on the stack.

The main problem right now is that the buffer size is a runtime configuration, forcing the buffer to be a separate "allocation". Though this may change in #491 (needs a lot of work).

Looking into what ChaN's FatFS is doing with FF_FS_TINY is really interesting, but if I understand it correctly it risks being extremely costly. In theory you can do the same in LittleFS: have a single shared buffer, and flush any pending writes when another file needs the buffer, but if you have two files open for writing this could create a lot of small flushes that can't be reused due to padding bytes (#725).

But it may still be worth adding as an option when the user thinks multiple open files is unlikely. Separate read/write buffers could help for common patterns such as 1 open for reading 1 open for writing.

I've also considered having a sort of LFS_ONEFILE flag that uses an internal buffer and asserts if more than one file is opened. Maybe a theoretical LFS_TINY should assert if more than one file is opened for writing since this is where interleaved options would be the most costly/destructive.

Another option is allowing users to specify the number of files they will open and adding a simple fixed-sized allocator into littlefs itself. Though maybe that is overkill.

There is certainly room for improvement here.

@geky geky added the code size label May 17, 2023
@geky
Copy link
Member

geky commented Dec 19, 2023

It doesn't look like LFS_MALLOC is used anywhere in the codebase. How does whatever is provided with that macro definition get used in the functions?

This is improving over here: #909

Let me know if anything looks amiss.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants