Add CFLAG to select SPIRAM heap caps malloc

[tonyjackson-srt]

Allows including project to select SPIRAM for mallocs with add_compile_definitions(LFS_MALLOC_SPIRAM)

[BrianPugh]

Thanks for the PR! I think this is something appropritate to add to the Kconfig. Could we add a field with variable name CONFIG_LITTLEFS_MALLOC_LOCATION with options for internal or spiram?

[tonyjackson-srt]

I was pondering whether to do that or not. The reason I didn't add it to the Kconfig was because in the lfs_config.h there is no other Kconfig setting, only CFLAGs, EG LFS_NO_MALLOC and LFS_NO_ASSERT.

These could both also be Kconfig, and that's a change I'm happy to make - if it's in fitting with the rest of the code?

[BrianPugh]

The LFS_* flags are for the main upstream littlefs. The CONFIG_LITTLEFS_* flags are for this esp-idf specific port.

I'd say for now, just adding CONFIG_LITTLEFS_MALLOC_LOCATION to Kconfig would be nice; if you need other elements exposed we can tackle them in another PR. Don't forget to include "sdkconfig.h"!

[tonyjackson-srt]

Although lfs_config.h overrides the upstream lfs_util.h, some of the CFLAGS are also used in the body of upsteam littlefs modules.

Particularly, lfs_file_open( ) is only included in the absence of LFS_NO_MALLOC - as esp_littlefs master stands setting LFS_NO_MALLOC will result in a compilation failure; I've reflected this in these changes by throwing an #error to state that static buffers are not currently supported by the wrapper.

Also this vfs wrapper is strictly not static anyway, even if LFS_NO_MALLOC or the new Kconfig is set - there are a number of callocs in esp_littlefs.c which I've also wrapped to reflect the selected allocation strategy.

A question - a comment against lfs_malloc states:

// Allocate memory, only used if buffers are not provided to littlefs
// Note, memory must be 64-bit aligned

However, the previous code was not ensuring 64-bit alignment. I have changed this to use heap_caps_aligned_alloc to ensure that 64-bit alignment is met. But is this necessary?

[BrianPugh]

git-blaming the 64-bit alignment comment in the littlefs repo yields the following commit message:

In v2, the lookahead_buffer was changed from requiring 4-byte alignment
to requiring 8-byte alignment. This was not documented as well as it
could be, and as FabianInostroza noted, this also implies that
lfs_malloc must provide 8-byte alignment.

To protect against this, I've also added an assert on the alignment of
both the lookahead_size and lookahead_buffer.

However, looking at the assert:

    // setup lookahead, must be multiple of 64-bits, 32-bit aligned
    LFS_ASSERT(lfs->cfg->lookahead_size > 0);
    LFS_ASSERT(lfs->cfg->lookahead_size % 8 == 0 &&
            (uintptr_t)lfs->cfg->lookahead_buffer % 4 == 0);

We see that the lookahead needs to be a multiple of 64-bits, but only required to be 32-bit aligned. I think this is a slight error in the documentation @geky.

Overall, I think this PR is pretty good now, just a few minor clarifying comments. I also need to give this a test on some hardware.

[BrianPugh]

thanks for all the commits! I'm going to try and run this on hardware tonight, and then merge 👍

[tonyjackson-srt]

thanks for all the commits! I'm going to try and run this on hardware tonight, and then merge 👍

Awesome. If this goes well I'll have a go at some of the other things that could do with being in Kconfig (I'm looking at you, debug levels!)

[BrianPugh]

@tonyjackson-srt are you looking to make more PRs over the next few days? Or would you prefer me to cut a new minor release now?

[tonyjackson-srt]

@BrianPugh definitely more to come, I see the version has been bumped - thanks for this as I will be able to use that in my project in the meantime :)