search

joltwallet / esp_littlefs

LittleFS port for ESP-IDF
MIT License
254 stars 95 forks source link

Add CFLAG to select SPIRAM heap caps malloc #159

Closed tonyjackson-srt closed 9 months ago

tonyjackson-srt commented 9 months ago

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

BrianPugh commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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

tonyjackson-srt commented 9 months ago

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 commented 9 months ago

@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 commented 9 months ago

@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 :)