ayushsharma82 / WebSerial

A remote terminal library for wireless microcontrollers to log, monitor or debug your firmware/product
https://webserial.pro
GNU Affero General Public License v3.0
507 stars 113 forks source link

Concurrency problem when using multiple FreeRTOS tasks #102

Closed Gummy35 closed 1 day ago

Gummy35 commented 2 months ago

on ESP32, when I call WebSerial.printXXX from another FreeRTOS task, I get an incomplete output, as it seems that the loop method is called while the buffer is being written.

using the following wrapper solves the problem, but it would be nice to have it in the library.

ThreadSafeWebSerial.h :

#ifdef ESP32

#pragma once

#include "Arduino.h"
#include "stdlib_noniso.h"
#include <functional>
#include "WiFi.h"
#include "AsyncTCP.h"
#include "ESPAsyncWebServer.h"

typedef std::function<void(uint8_t *data, size_t len)> WSLMessageHandler;
typedef std::function<void(const String& msg)> WSLStringMessageHandler;

class ThreadSafeWebSerialClass : public Print {
public:
    void Lock();
    void Unlock();
    void begin(AsyncWebServer *server, const char *url = "/webserial");
    inline void setAuthentication(const char* username, const char* password) { setAuthentication(String(username), String(password)); }
    void setAuthentication(const String& username, const String& password);
    void onMessage(WSLMessageHandler recv);
    void onMessage(WSLStringMessageHandler recv);
    size_t write(uint8_t m) override;
    size_t write(const uint8_t* buffer, size_t size) override;
    void loop();
    void setBuffer(size_t initialCapacity);
private:
    SemaphoreHandle_t _lock;
};

extern ThreadSafeWebSerialClass TSWebSerial;

#endif

ThreadSafeWebSerial.c :

#ifdef ESP32

#include "ThreadSafeWebSerial.h"
#include <WebSerial.h>

void ThreadSafeWebSerialClass::Lock()
{
    xSemaphoreTake(_lock, portMAX_DELAY);
}

void ThreadSafeWebSerialClass::Unlock()
{
    xSemaphoreGive(_lock);
}

void ThreadSafeWebSerialClass::begin(AsyncWebServer *server, const char *url)
{
    _lock = xSemaphoreCreateBinary();
    WebSerial.begin(server, url);
    Unlock();
}

void ThreadSafeWebSerialClass::setAuthentication(const String &username, const String &password)
{
    WebSerial.setAuthentication(username, password);
}

void ThreadSafeWebSerialClass::onMessage(WSLMessageHandler recv)
{
    WebSerial.onMessage(recv);
}

void ThreadSafeWebSerialClass::onMessage(WSLStringMessageHandler recv)
{
    WebSerial.onMessage(recv);
}

size_t ThreadSafeWebSerialClass::write(uint8_t m)
{
    Lock();
    size_t res = WebSerial.write(m);
    Unlock();
    return res;
}

size_t ThreadSafeWebSerialClass::write(const uint8_t *buffer, size_t size)
{
    Lock();
    size_t res = WebSerial.write(buffer, size);
    Unlock();
    return res;
}

void ThreadSafeWebSerialClass::loop()
{
    Lock();
    WebSerial.loop();
    Unlock();
}

void ThreadSafeWebSerialClass::setBuffer(size_t initialCapacity)
{
    WebSerial.setBuffer(initialCapacity);
}

ThreadSafeWebSerialClass TSWebSerial;

#endif
mathieucarbou commented 2 months ago

@Gummy35 this is a known issue for this project, and caused by the use of a temporary buffer which is flushed by the loop(). I am not a fan of the current implementation, that is why I've contributed a high performance mode to this project (which comes from https://github.com/mathieucarbou/MycilaWebSerial).

High perf mode removes any intermediary buffer and requirement to call loop() and directly writes in the websocket queue.

The drawback is that only full lines can be printed: it is not possible to print characters one by one for example.

In other words, any required buffering has to be done on caller side, eventually being helped with a StreamString

High perf mode supports about 20 push per second and can be activated with a flag. See the header:

https://github.com/ayushsharma82/WebSerial/blob/master/src/WebSerial.h#L127

it will also reduce the size a little.

Gummy35 commented 2 months ago

Thank you Mathieu, I will try it tomorrow ;)

Le lun. 16 sept. 2024, 23:38, Mathieu Carbou @.***> a écrit :

@Gummy35 https://github.com/Gummy35 this is a known issue for this project, and caused by the use of a temporary buffer which is flushed by the loop().

I've contributed a high performance mode to this project (which comes from https://github.com/mathieucarbou/MycilaWebSerial).

High perf mode removes any intermediary buffer and requirement to call loop() and directly writes in the websocket queue.

The drawback is that only full lines can be printed: it is not possible to print characters one by one for example.

In other words, any required buffering has to be done on caller side, eventually being helped with a StreamString https://github.com/espressif/arduino-esp32/blob/master/cores/esp32/StreamString.h

High perf mode supports about 20 push per second https://github.com/ayushsharma82/WebSerial/blob/master/examples/HighPerf/HighPerf.ino

— Reply to this email directly, view it on GitHub https://github.com/ayushsharma82/WebSerial/issues/102#issuecomment-2354072394, or unsubscribe https://github.com/notifications/unsubscribe-auth/AARWUND2X7FYRV24LTXJGZLZW5FT3AVCNFSM6AAAAABOKDO23WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNJUGA3TEMZZGQ . You are receiving this because you were mentioned.Message ID: @.***>

Gummy35 commented 2 months ago

@mathieucarbou Hi Mathieu, I managed to do some testing and I have strange results : As expected serializeJson(doc, WebSerial) doesn't work with the WSL_HIGH_PERF mode. print and printf work. printf("xxxxx \n") works as well. However, println send an extra empty entry.

I use the webserial as a log and control console for my project. I have the following code :

WebSerial.onMessage([&](uint8_t *data, size_t len)
                      {
                        unsigned long ts = millis();
                        debug(Serial.printf("Received %u bytes from WebSerial: ", len));
                        debug(Serial.write(data, len));
                        String d(data, len);
                        WebSerial.println(d);
                        if (d.equals("help"))
                        {
                          WebSerial.println("keymap.[default|save|load|clearconfig]");
                          WebSerial.println("hardpoints.[default|save|load]");
                          WebSerial.println("macros.[load|explain]");
                          WebSerial.println("slave.[ping|reset|getupdates|getalldata]");
                          WebSerial.println("test.[leds|i2clarge]");
                          WebSerial.println("freeram");
                          WebSerial.println("reboot");
                        }
                        else if 
                        ...
                        WebSerial.printf("%d ms\n", millis() - ts);
                        });

if I send the "help" command, it takes less than 1ms in standard mode, and 4ms in WSL_HIGH_PERF.

Do you have any idea ?

Thank you, Jérémy

mathieucarbou commented 2 months ago

As expected serializeJson(doc, WebSerial) doesn't work with the WSL_HIGH_PERF mode.

You need to use an intermediary StreamString to serialise into a StreamString buffer, that you send to WebSerial after. If your json payload is too big to fit in ESP32 memory, you would need to split it to make sure it is less than half the free heap memory at least

StreamString buffer;
buffer.reserve(measureJson(doc));
serializeJson(doc, buffer);
WebSerial.write(buffer);

This buffering can even be improved by using a direct WS buffer to avoid an internal copy again of the data to create the websocket message that will be added to an internal queue:

    AsyncWebSocketMessageBuffer* wsBuffer = WebSerial.makeBuffer(measureJson(doc));
    serializeJson(doc, wsBuffer->get());
    WebSerial.send(wsBuffer);

With that, you can send messages that are more than half your remaining heap size.

Note that this feature is available in MycilaWebSerial but not yet in this repo - I will send a PR.

High perf mode does not have concurrency issues, but requires the caller to be responsible for any required buffering.

println works by sending the message with print(), then it does a second call println() to send \r\n. So that's one example of such method that should not be used with high perf mode. Use instead write(buffer).

mathieucarbou commented 2 months ago

if I send the "help" command, it takes less than 1ms in standard mode, and 4ms in WSL_HIGH_PERF.

High perf more is sending straight in the queue so it should be faster. Look at the code ;-)

Gummy35 commented 2 months ago

As expected serializeJson(doc, WebSerial) doesn't work with the WSL_HIGH_PERF mode.

You need to use an intermediary StreamString to serialise into a StreamString buffer, that you send to WebSerial after. If your json payload is too big to fit in ESP32 memory, you would need to split it to make sure it is less than half the free heap memory at least

StreamString buffer;
buffer.reserve(measureJson(doc));
serializeJson(doc, buffer);
WebSerial.write(buffer);

This buffering can even be improved by using a direct WS buffer to avoid an internal copy again of the data to create the websocket message that will be added to an internal queue:

    AsyncWebSocketMessageBuffer* wsBuffer = WebSerial.makeBuffer(measureJson(doc));
    serializeJson(doc, wsBuffer->get());
    WebSerial.send(wsBuffer);

With that, you can send messages that are more than half your remaining heap size.

Note that this feature is available in MycilaWebSerial but not yet in this repo - I will send a PR.

High perf mode does not have concurrency issues, but requires the caller to be responsible for any required buffering.

println works by sending the message with print(), then it does a second call println() to send \r\n. So that's one example of such method that should not be used with high perf mode. Use instead write(buffer).

Those are precious advises :) Thanks very much. It could be nice to have it in the docs.

About the perf issue, as you said, it should be much faster. I will do some more testing to see where the problem is, but there is a good chance it is between my chair and my keyboard ;)

mathieucarbou commented 2 months ago

I have sent the PR if you want to grab the changes locally to improve your heap usage: https://github.com/ayushsharma82/WebSerial/pull/103

github-actions[bot] commented 1 month ago

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

github-actions[bot] commented 6 days ago

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

github-actions[bot] commented 1 day ago

This issue was closed because it has been stalled for 5 days with no activity.