pschatzmann / arduino-liblame

A simple mp3 encoder (not only) for Arduino using LAME
20 stars 3 forks source link

Do we really want `heap_caps_malloc_extmem_enable` implicitly set to `liblame_extmem_enable_limit`? #7

Closed zackees closed 1 year ago

zackees commented 1 year ago

Without special settings using the idf.py, all tasks/threads in the ESP32 will crash as soon as they touch psram.

In debug_calloc we currently have this:

void* debug_calloc(int count, int size){
    void* result=NULL;
#ifdef ESP32
    heap_caps_malloc_extmem_enable(liblame_extmem_enable_limit);  // <--- NOW GLOBALLY ENABLED
    //result = heap_caps_calloc(count, size, MALLOC_CAP_8BIT);  // MALLOC_CAP_SPIRAM, MALLOC_CAP_32BIT, MALLOC_CAP_8BIT
    result = calloc(count,size);
#else
    result = calloc(count,size);

This means that other code that mallocs large data (unrelated to LAME encoder) used in tasks will now crash.

In my local code repo that I'm using this library, I've patched the debug_calloc routine so that the allocation with psram is done in the allocator.

Should this patch be upstreamed?

pschatzmann commented 1 year ago

How would this look like ? Do you allocate all memory in PSRAM ?

The liblame_extmem_enable_limit should maybe go to the config as well with the following logic:

zackees commented 1 year ago

In my solution - yes. debug_calloc just simply goes to psram and it works fast enough for realtime encoding of 16-bit mono audio @ 44.1khz on the main thread.

void *debug_calloc(int count, int size)
{
    void *result = NULL;
#ifdef ESP32
    // heap_caps_malloc_extmem_enable(liblame_extmem_enable_limit);
    //  result = heap_caps_calloc(count, size, MALLOC_CAP_8BIT);  // MALLOC_CAP_SPIRAM, MALLOC_CAP_32BIT, MALLOC_CAP_8BIT
    // result = calloc(count, size);
    result = ps_calloc(count, size); // use psram
#else
    result = calloc(count, size);
#endif

But the upstream patch could simply do

void *debug_calloc(int count, int size)
{
    void *result = NULL;
#ifdef ESP32
    if (count >= liblame_extmem_enable_limit) {
       // Note - haven't tested this.
       result = heap_caps_calloc(count, size, MALLOC_CAP_8BIT | MALLOC_CAP_SPIRAM)
    } else {
       result = calloc(...)
    }
    // heap_caps_malloc_extmem_enable(liblame_extmem_enable_limit);
    //  result = heap_caps_calloc(count, size, MALLOC_CAP_8BIT);  // MALLOC_CAP_SPIRAM, MALLOC_CAP_32BIT, MALLOC_CAP_8BIT
    // result = calloc(count, size);
    result = ps_calloc(count, size); // use psram
#else
    result = calloc(count, size);
#endif
pschatzmann commented 1 year ago

Good to know that this is working with all allocations in PSRAM. How about to replace #ifdef ESP32 with #if USE_PSRAM so that it will work for those who do not have PSRAM available... And we define USE_PSRAM in config.h


void *debug_calloc(int count, int size)
{
    void *result = NULL;
#if USE_PSRAM
    result = ps_calloc(count, size); // use psram
#else
    result = calloc(count, size);
#endif
zackees commented 1 year ago

My only objection to this scheme is that psram is very slow, in comparison to sram, and my WT32 based on the wrover chip may not match the performance requirements of lower end modules. However in time this probably won't matter as much.

zackees commented 1 year ago

Here's what I suggest:

void *debug_calloc(int count, int size)
{
    void *result = NULL;
#if USE_PSRAM
    if (count * size > liblame_extmem_enable_limit) {
      result = ps_calloc(count, size); // use psram
    } else {
      result = calloc(count, size);
    }
#else
    result = calloc(count, size);
#endif

That will preserve current behavior without the global call to set the internal memory limits.

pschatzmann commented 1 year ago

How about making the limit configurable. With this we can keep the existing behaviour and would have full flexibility:

void *debug_calloc(int count, int size)
{
    void *result = NULL;
#ifdef ESP32
    if (count * size > ESP_PSRAM_ENABLE_LIMIT) {
      result = ps_calloc(count, size); // use psram
    } else {
      result = calloc(count, size);
    }
#else
    result = calloc(count, size);
#endif
zackees commented 1 year ago

Can we replace ESP_PARAM_ENABLE_LIMIT with liblame_extmem_enable_limit or create the new #define ESP_PARAM_ENABLE_LIMIT?

pschatzmann commented 1 year ago

I suggest to create the #define ESP_PARAM_ENABLE_LIMIT in config.h