expose WebSocket makeBuffer() method to be publically available

[vortigont]

Nice work done here refactoring AsyncWebServer, really like it!
Want to suggest exposing AsyncWebSocketClient::makeBuffer() method to be public. The origin AsyncWebServer has this feature and sometimes it really helps to avoid extra data copy or mem allocations.
I.e. I use it like this with ArduinoJson

void send(const JsonObject& data){
    size_t length = measureJson(data);
    AsyncWebSocketMessageBuffer buffer = ws->makeBuffer(length);
    if (!buffer) return;

    serializeJson(data, (char*)buffer->data() , length);
    ws->textAll(buffer);
};

[vortigont]

@avillacis any feedback?

[poulch74]

makebuffer not needed. for example in my code serialize json with ArduinoJson and send over websocket:

std::shared_ptr<std::vector<uint8_t>> buffer;

buffer = std::make_shared<std::vector<uint8_t>>(len);

serializeJson(*oroot, (char *)buffer->data(),len);

if (client) { client->text(std::move(buffer));

[vortigont]

@poulch74 sure, this could be done via bare std::shared_ptr, but for the sake of code style similar to the original ESPAsyncWebServer it would be nice to hide all of this pointer declare/create/move stuff under the hood of common methods. What my PR is actually meant for. Could save time on typing, could save less experienced coders to deep dive into lib's code to uncover it's features.

[mathieucarbou]

Hello, I'm curious to understand how this library compare to the ESPHome maintained fork (https://github.com/esphome/AsyncTCP) and what it brings compare to AsyncTCP ?
I am using a fork of yubox-node-org/ESPAsyncWebServer also, but with the ESPHome maintained AsyncTCP.
Thanks!

[vortigont]

Who knows... Do not see much activity in this fork recently.
How is it works yubox-node-org/ESPAsyncWebServer with esphome/AsyncTCP?
Is it more stable? My impression was that yubox-node-org/ESPAsyncWebServer must be used with it's own version of AsyncTCPSock.
Have you tried esphome/ESPAsyncWebServer? They have added some fixes there, but yubox fork is almost a full rework.

[mathieucarbou]

FYI I am maintaining a more up to date version of the yubox-node-org fork, which is deployed in PlatformIO registry and Arduino registry.

• It is compatible, but cleaned up regarding CI, spdiff editor, etc.
• It is also compatible Arduino Json 7
• It contains the few improvements made by ESPHome

The ESPHome folks decided to fork the original repo, which is IMO wrong because the yubox-node-org introduces a LOT of fixes regarding concurrency issues when using tasks especially on dual core systems. So they miss all that, especially on the WebSocket part.

I know a lot of projects depending on the yubox-node-org fork instead of the ESPHome one or original one for this reason. It adds a lot of stability and prevents corrupted heap causes by concurrent access to queues in the lib.

The version I maintain is using esphome/AsyncTCP because the ESPHome team does a great job IMO to maintain it. They fixed a couple of issues and introduced IPv6.

[vortigont]

Thanks! I'll take a look into your fork also! If you like I can rebase this PR onto your fork.
I also have some improvements for async webserver that I can share.

[mathieucarbou]

Thanks! I'll take a look into your fork also! If you like I can rebase this PR onto your fork. I also have some improvements for async webserver that I can share.

Yes! An adapted version of this change would definitely be interesting because sadly the api change in the buffer is not compatible with the original API. But making is compatible is really harder I think.

For the moment I have to workaround that by using feature detection like here:

https://github.com/ayushsharma82/ESP-DASH/pull/195/files#diff-b22c24b3d761e54f8997b5313d563e3b2b58217a99f7e9e95beb68ad19ba6e13R343-R351

// this fork (originally from yubox-node-org), uses another API with shared pointer that better support concurrent use cases then the original project
 #if defined(ASYNCWEBSERVER_FORK_mathieucarbou)
   auto buffer = std::make_shared<std::vector<uint8_t>>(len);
   assert(buffer);
   serializeJson(doc, buffer->data(), len);
 #else
   AsyncWebSocketMessageBuffer* buffer = _ws->makeBuffer(len);
   assert(buffer);
   serializeJson(doc, buffer->get(), len);
 #endif

The problem is that the original API returns a pointer to a buffer, which requires it to be responsible of its destruction:

https://github.com/esphome/ESPAsyncWebServer/blob/master/src/AsyncWebSocket.h#L333-L334

The change using yubox fork with a shared ptr allows a buffer created by the caller to still be referenced until all the clients have used it. His change is well explained in his commit here:

me-no-dev@4963ce9

I don't know how to make the original API compatible with the use of shared ptr.

[mathieucarbou]

@proddy : did you find out ? is it possible that this increase is caused by the difference values for the queue sizes ?

[proddy]

to be honest I can't remember! I'm pretty sure it's in AsyncTCP. I'm still using the ESPHome fork with IPv6 support with CONFIG_ASYNC_TCP_STACK_SIZE set to 5120 which is enough for ESP32 (2.3k) and ESP32S3 (3.5k)

[mathieucarbou]

to be honest I can't remember! I'm pretty sure it's in AsyncTCP. I'm still using the ESPHome fork with IPv6 support with CONFIG_ASYNC_TCP_STACK_SIZE set to 5120 which is enough for ESP32 (2.3k) and ESP32S3 (3.5k)

Ok. Because I ran some tests this morning:

• 9h57 - 10h05: AsyncTCPSock with default stack size
• 10h06 - 10h14 : AsyncTCPSock with 4k stack size
• Before and after: AsyncTCP with 4k stack size.

If you look from 10h35 and after: this is typically what I like to see. There is no big allocations / deallocation on heap, which avoids fragmentation. This is quite stable.

Spikes are app reloads (ESP-DASH PRo + WebSerial Pro).
This app also has mqtt publications each 5 seconds, dashboard refresh each 500ms, and log streamed to web console (websocket) in debug mode.

[mathieucarbou]

I am running with my forks:

• https://github.com/mathieucarbou/AsyncTCP
• https://github.com/mathieucarbou/ESPAsyncWebServer

My AsyncTCP fork is based on ESPHome, but supports Arduino 3, Ipv6 for Arduino 3 also, and fixes some issus in the ESPHome implementation of Ipv6 (I've PR-ed them if I remember). It also include a workaround of a bug introduced in Arduino 3 in the IpAddress implementation.

[proddy]

I am running with my forks:

• https://github.com/mathieucarbou/AsyncTCP
• https://github.com/mathieucarbou/ESPAsyncWebServer

My AsyncTCP fork is based on ESPHome, but supports Arduino 3, Ipv6 for Arduino 3 also, and fixes some issus in the ESPHome implementation of Ipv6 (I've PR-ed them if I remember). It also include a workaround of a bug introduced in Arduino 3 in the IpAddress implementation.

nice. I'll do some benchmarking too. Having local copies of the libraries is a nightmare to maintain so it'll be good if I switch to public libraries.

[mathieucarbou]

Please see my comment