Added possibility to open multiple files with LFS_NO_MALLOC enabled #58
Conversation
d77ed32 to
118027b
Compare
|
I have rebased this PR on top of the latest master, with change in license. |
|
Hi @dpgeorge, thanks for the pr (and rebasing). I think you're right passing a buffer into open would be a better API. Tossing ideas around, one idea to make this future compatible would be to take in a sort of config struct. Similar to the lfs_mount/lfs_format functions: struct lfs_file_config {
void *buffer;
};
int lfs_file_open(lfs_t *lfs, lfs_file_t *file,
const char *path, int flags,
const struct lfs_file_config *cfg);This would let us add additional configuration to the config structure in the future without breaking compatibility or having to come up with additional function names. Thoughts? |
It might be simpler to just add a config entry to the Alternatively, a user could just set the existing |
Hmm, although wouldn't that no longer be backwards compatible since the config entry would need to be zeroed by users that don't want to use a buffer? |
Yes that's true. If 100% backwards compatibility is needed there are a few options:
|
|
Those are all good ideas. I believe the main benefit of providing a separate config struct is that it makes it clear to the user what needs to be initialized and what doesn't. And additionally the config struct could be stored in How about we go the "lfs_file_open_ext" route and add an additional open function as One thing I'm considering is the addition of custom attributes (#48). Introducing a config struct (or setting members of the file struct) would open up a good way to introduct custom attributes before file open: struct my_file {
lfs_file_t file;
struct lfs_file_config cfg;
struct lfs_attr attrs[2];
uint32_t timestamp;
uint16_t number;
};
struct my_file file;
file->attrs[0].type = MY_TYPE_TIMESTAMP;
file->attrs[0].size = sizeof(file->timestamp);
file->attrs[0].buffer = &file->timestamp;
file->attrs[1].type = MY_TYPE_NUMBER;
file->attrs[1].size = sizeof(file->number);
file->attrs[1].buffer = &file->number;
memset(&file->cfg, 0, sizeof(file->cfg));
file->cfg.attrs = &file->attrs;
file->cfg.attr_count = 2;
int err = lfs_file_opencfg(lfs, &file->file, "filename.txt", LFS_O_RDONLY, &file->cfg); |
That sounds good.
To allow to add entries to the file config struct in a backwards-compatible way, I guess you'd require the user to always do For static uint8_t my_file_buffer[128];
static const struct lfs_file_config my_file_cfg = {
.file_buffer = my_file_buffer,
// all other entries are 0 by default
};
Also sounds good. Although in your example above, if you open the file with |
|
@dpgeorge How does this look to you? I added the One other question is if these config structs should be copied into the structures during format/mount/opencfg:
I went with the second option for consistency with
It could still read the attributes, though there wouldn't be much advantage over and explicit |
Thanks, I see it.
That brings up an interesting point: for files there would be two kinds of configurations: global configuration options that are common to all files (like attributes), and local configuration options just for a file (like the cache buffer). If these are put into the same struct then it would be a waste of memory to allocate a new one of these structs for each file, since all the global config would be common and could have gone in ROM. To be explicit on this point: the Maybe the common, global file config (like configuration of attributes and their types) needs to go in the |
|
You're right that any common configuration should go into the global struct. But I think attributes fall into this category. A part of the attribute structure is the buffer that contain's the attribute's data. This would be unique for each file. There's also the minor point that you may want different attributes for different files, but I'm not sure that's important. On the plus side, you only pay the cost for the attribute structures if you use custom attributes. |
bf25c0b to
f6e3193
Compare
|
Rebased due to conflict with master and added version bump to v1.5 Hi @dpgeorge, does this change look good to you? Since it's backwards compatible I think we can go ahead and merge it without any problems (just a minor version bump). |
|
Yes it looks good to me, thanks! It does look backwards compatible. |
|
Thanks for the initial pr! |
|
No problem. There's not much left of my initial code/commit, I don't know if it's worth keeping it around or not. |
The optional config structure options up the possibility of adding file-level configuration in a backwards compatible manner. Also adds possibility to open multiple files with LFS_NO_MALLOC enabled thanks to dpgeorge Also bumped minor version to v1.5
f6e3193 to
961fab7
Compare
|
Sure thing, squashed, should be good to go once CI is done |
|
Ok, I'm happy with it. |
|
Thanks for the work to get this merged, it's much appreciated! FYI here is the change I made to support this new |
The
LFS_NO_MALLOCoption is very useful, because some systems have very particular memory allocation needs. But enabled this option doesn't allow to have multiple files open at the same time. This PR adds a quick fix for this by allowing to pass in thefile_bufferdirectly tolfs_file_open(), so the latter doesn't need to calllfs_malloc().I don't expect this to be merged as-is because it breaks the existing API. But a feature along these lines would be very useful because it gives full functionality of littlefs without the need for any
malloc()orfree()functions.Alternatives to this PR would be:
lfs_file_open_using()which accepts the 5th parameter added herelfs->cfg->file_bufferpointer before each call tolfs_file_open()so the latter could pick up a new buffer each time( Background: we are looking to integrate littlefs in MicroPython: micropython/micropython#3847 )