Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Websockets no async #319

Merged
merged 13 commits into from
Feb 3, 2022
Merged

Websockets no async #319

merged 13 commits into from
Feb 3, 2022

Conversation

MarkusSchneider
Copy link
Collaborator

Hi Anthony,

I've added the first implementation of the new logger concept.
I've used the FreeRTOS MessageBuffer instead of the Queue. Queues allow only fixed size data structures. MessageBuffers are build for variable sized strings.

I moved away from https://github.com/Links2004/arduinoWebSockets library, I think the library is no longer maintained (162 open issues 😒) and picked https://github.com/gilmaimon/ArduinoWebsockets.

Please take a look at the implementation.

Two things are open:

  1. If we want to handle multiple web socket connection, we must extend the WebSocketsAdapter to handle this.
  2. MessageBuffers are not thread safe. We need to add a Mutex to the writev method:
    void LogHandler::writev(esp_log_level_t level, const char *format, va_list args) {
    if (_messageBufferHandle == NULL) {
    ESP_LOGE("LogHandler", "Can not send log message. Message Buffer is NULL");
    }
    const size_t buffer_size = 512;
    char buffer[buffer_size];
    vsnprintf(buffer, buffer_size, format, args);
    size_t bytesSent = xMessageBufferSend(_messageBufferHandle, buffer, strnlen(buffer, buffer_size), 0);
    if (bytesSent < strnlen(buffer, buffer_size)) {
    ESP_LOGE("LogHandler", "Can not send log message. Not enough free space left in buffer.");
    }
    }

I've disable the buffer for http logging. If we need them in future, I can build a BufferedAppender and http_server can read the content of the buffer from the appender, no need to have this inside SSKLog.

For testing I've used a chrome extension:
https://chrome.google.com/webstore/detail/simple-websocket-client/pfdhoblngboilpfeibdedpjgfnlcodoo?hl=de

@doudar
Copy link
Owner

doudar commented Jan 30, 2022

Nice @MarkusSchneider ! I see your updates to that branch. I'm going to ride test it ASAP and see if we're stable.

Looks awesome!

Do you have a preference on what port we should use for the websocket? Is there a standard? For now, I'll update the status.html to use port 8080 that you selected

@MarkusSchneider
Copy link
Collaborator Author

Nice @MarkusSchneider ! I see your updates to that branch. I'm going to ride test it ASAP and see if we're stable.

Looks awesome!

Do you have a preference on what port we should use for the websocket? Is there a standard? For now, I'll update the status.html to use port 8080 that you selected

Don't know if there is a standard port. If you have a web server who is able to talk http and ws default is 80 or 443. But Port 80 is already used by http_server and http_server is not able to talk ws. Let's go with port 8080 😉

@MarkusSchneider
Copy link
Collaborator Author

MarkusSchneider commented Jan 31, 2022

Hi @doudar I've added the mutex. Work is almost done. We can remove the HTTP-Log Buffer if no longer need and cleanup status.html (remove udp-logger hint, request debugLog, slice log messages, ...)

@doudar
Copy link
Owner

doudar commented Jan 31, 2022

Looking good!

@MarkusSchneider
Copy link
Collaborator Author

I've removed DebugInfo.
Status.html must be update, see post above.

I've also remove some macros and extended writev function to get level and module to be prepared for filtering in the future.

void LogHandler::writev(esp_log_level_t level, const char *module, const char *format, va_list args) {
if (xSemaphoreTake(_logBufferMutex, 10) == pdFALSE) {
ESP_LOGE("LogHandler", "Can not write log message. Write is blocke by other task.");
return;
}
if (_messageBufferHandle == NULL) {
ESP_LOGE("LogHandler", "Can not send log message. Message Buffer is NULL");
return;
}
char formatString[256];
sprintf(formatString, "[%6lu][%c](%s): %s", millis(), _logLevelToLetter(level), module, format);
const size_t buffer_size = 512;
char buffer[buffer_size];
int written = vsnprintf(buffer, buffer_size, formatString, args);
// Default logger -> write all to serial if connected
if (Serial) {
Serial.println(buffer);
}
size_t bytesSent = xMessageBufferSend(_messageBufferHandle, buffer, written, 0);
if (bytesSent < written) {
ESP_LOGE("LogHandler", "Can not send log message. Not enough free space left in buffer.");
}

@doudar
Copy link
Owner

doudar commented Jan 31, 2022

I'm busy with other work today but I like what you've done. I'll be troubleshooting the BLE disconnect when I get time.

Copy link
Owner

@doudar doudar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still need to fix the status.html to work with websockets. It’s close but I didn’t finish it yet. I’m having trouble getting it to only connect to the websocket when udpLogging is disabled.

@MarkusSchneider
Copy link
Collaborator Author

We still need to fix the status.html to work with websockets. It’s close but I didn’t finish it yet. I’m having trouble getting it to only connect to the websocket when udpLogging is disabled.

I don't think that websockets must disabled when UPD logging is enabled. All is running now inside the main-task.
If sending of log messages is to time consuming, some messages are skippend when message buffer is full.

With websocket every message is only sent once. In the previous implementation the message buffer was sent ever second.
The size of sent messages should be a lot smaller than before.

I'll rework status.html and we can test if performance is good.

@MarkusSchneider
Copy link
Collaborator Author

I've updated status.html.
Removed check for udp-logging.
If we need the check in future, we can put it here:

function startUpdate() {
//Update values on specified interval loading late because this tiny webserver hates frequent requests
if (updateTimer === undefined) {
updateTimer = setInterval(function () {
requestRuntimeValues();
if (websocket == null) {
setupLogging();
}
}, 2500);
}
}

in line 140.

I've limited the log to 1000 lines.
Bedtime now 😉. I'll doing a test ride tomorrow to see if BLE is running smooth.

@doudar
Copy link
Owner

doudar commented Jan 31, 2022

Thanks!!!!

@MarkusSchneider
Copy link
Collaborator Author

@doudar finished my tests. Fixed a re-connection problem. All is now running stable. Also did a test ride and BLE works great.
At the moment only one client can connect to websocket, all other clients gets queued and connected if the first client closes the connection.
I can extend the Websockets Appender to handle more than on client, let my know if I should do it.

I'm using a std::vector to handle the registered appenders:

std::vector<ILogAppender *> _appenders;

it's maybe better to use an array to avoid head defragmentation, but appenders only added once so it should not be a problem.

@doudar doudar merged commit a870ecf into develop Feb 3, 2022
@doudar doudar deleted the websockets-no-async branch December 16, 2022 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants