espressif / esp-mqtt

ESP32 mqtt component
Apache License 2.0
603 stars 256 forks source link

Feature request: Add menuconfig option to prefer PSRAM when allocating outbox buffers. (IDFGH-8993) #242

Closed not-my-real-name closed 1 year ago

not-my-real-name commented 1 year ago

Hi Everyone,

I have a feature request for the MQTT library. I am using this library with PSRAM on an ESP32 WROVER (with IDF4.4). I've noticed that the default outbox implementation utilises a linked list where messages waiting to be published are copied into a RAM buffer before being sent. The buffers are allocated using a malloc() call. Could you consider adding a config option to prefer allocating these buffers from PSRAM (e.g. heap_caps_malloc_prefer()) instead of using a basic malloc call? Even if the list objects are kept in internal RAM, just moving the payload buffers would make a huge difference.

I can shift my large message buffers (e.g. 10-50KB) to PSRAM, but I still need to have an equal sized amount of internal RAM free.

An alternative/additional solution would be to add a zero-copy publish command that takes a buffer pointer and a callback and calls the callback when the buffer is safe to release/free (ideally include a result code for the publish as well). This is obviously a much larger change, but it would execute faster if you could get rid of the extra copy step.

Link to topic for original discussion: https://esp32.com/viewtopic.php?f=13&t=31122

euripedesrocha commented 1 year ago

Hi @not-my-real-name, thanks for the suggestion. We have an ongoing work on the outbox, and we'll introduce this changes also.

not-my-real-name commented 1 year ago

I thought I would check back in and see if there as been any update on this?

I've forked this library and done my own small change that adds an option for PSRAM malloc.

2e30eb5071c892f516a696603e47e992332fa246

This also requires changes to the Kconfig file for this component but that is in the parent repository.

    config MQTT_PSRAM_OUTBOX
        bool "Allow PSRAM outbox buffers"
        default n
        help
            Set to true if you want to allocate message buffers for pending MQTT outbox items from the external
            PSRAM. This is especially useful when sending messages with large payloads as it conserves internal 
            RAM.
euripedesrocha commented 1 year ago

@not-my-real-name this was introduced to esp-mqtt a while ago in 2c71f9e69b1b9e5373c3ec937bdfcddc50d5b1c2. Recently we updated to idf master, and we are in the process to backport it to 5.1

not-my-real-name commented 1 year ago

Thanks. Sorry, I did look for it but can't have looked well enough!

not-my-real-name commented 1 year ago

Oh, I know why I missed it. I think the implementation is incorrect. The wrong memory allocation has been changed.

This only changes where the list item is held. I think it makes sense to keep this from a normal calloc in normal memory as that is probably faster and the object is small. So this should revert:

https://github.com/espressif/esp-mqtt/blob/05b347643f6679cc60a50a9664b86a24ebf3ae10/lib/mqtt_outbox.c#L44

The external malloc should be here: https://github.com/espressif/esp-mqtt/blob/05b347643f6679cc60a50a9664b86a24ebf3ae10/lib/mqtt_outbox.c#L52

item->buffer = heap_caps_malloc(message->len + message->remaining_len, MQTT_OUTBOX_MEMORY);
euripedesrocha commented 1 year ago

@not-my-real-name you are correct. I'll take another look.

bmedici commented 2 months ago

Hi, do you have any news on this ?

euripedesrocha commented 1 month ago

@bmedici the feature is implemented, and it is available. If you have any problems with it, I would ask you to please open a new issue explaining your problem.