Migration PsychicHttp → ESPAsyncWebServer#2
Migration PsychicHttp → ESPAsyncWebServer#2thedemoncat wants to merge 3 commits intoewowi:PhysicHTTP-to-AsynWebserverfrom
Conversation
📝 WalkthroughWalkthroughThis pull request performs a comprehensive migration from PsychicHttpServer to AsyncWebServer (ESPAsyncWebServer) across the entire framework and application codebase, updating constructor signatures, request/response handling patterns, WebSocket implementations, and HTTP endpoint registrations to use the asynchronous web server framework. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AsyncWebServer
participant Handler
participant SecurityManager
participant AsyncJsonResponse
Client->>AsyncWebServer: HTTP GET /api/status
AsyncWebServer->>Handler: onRequest(AsyncWebServerRequest*)
Handler->>SecurityManager: authenticateRequest(request)
SecurityManager-->>Handler: Authentication{authenticated, user}
alt Authenticated
Handler->>AsyncJsonResponse: new AsyncJsonResponse()
Handler->>AsyncJsonResponse: getRoot() → JsonObject
Handler->>JsonObject: populate JSON data
Handler->>AsyncJsonResponse: setLength()
Handler->>AsyncWebServer: request->send(response)
AsyncWebServer->>Client: HTTP 200 + JSON
else Unauthorized
Handler->>AsyncWebServer: request->send(401)
AsyncWebServer->>Client: HTTP 401
end
sequenceDiagram
participant WebClient
participant AsyncWebServer
participant AsyncWebSocket
participant EventSocket
participant StatefulService
WebClient->>AsyncWebServer: WebSocket /ws/events
AsyncWebServer->>AsyncWebSocket: addHandler()
AsyncWebSocket->>EventSocket: onWSEvent(WS_EVT_CONNECT)
EventSocket->>EventSocket: onWSOpen(client)
EventSocket->>StatefulService: read() → JSON state
EventSocket->>AsyncWebSocket: client->text(stateJson)
AsyncWebSocket->>WebClient: State update
WebClient->>AsyncWebSocket: text message (updated state)
AsyncWebSocket->>EventSocket: onWSEvent(WS_EVT_DATA)
EventSocket->>EventSocket: onWSFrame(client, data)
EventSocket->>StatefulService: update(state)
EventSocket->>AsyncWebSocket: textAll(broadcastJson)
AsyncWebSocket->>WebClient: Broadcast update
WebClient->>AsyncWebSocket: disconnect
AsyncWebSocket->>EventSocket: onWSEvent(WS_EVT_DISCONNECT)
EventSocket->>EventSocket: onWSClose(client)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip You can disable the changed files summary in the walkthrough.Disable the |
|
When compared with the original master branch, memory savings are not great when using ESPAsyncWebServer. 
|
|
@coderabbitai, please do an in-depth review of all the commits made, make a summary, a recommendation to merge and a list of possible future actions. |
🧩 Analysis chainThe PR description image shows the ESP32 SvelteKit web UI running successfully on an ESP32-S3 Rev 2, displaying the System Status page with: Memory at 45.9% of 344 KB (186 KB free, Max alloc 139 KB), Sketch at 54.8% of 3,342 KB (1,511 KB free), Firmware v0.6.0, SDK ESP-IDF v5.5.1, Arduino v3.3.2, running at 240 MHz Dual Core. The Comment image (from Let me gather full details about the PR commits and changed files before writing my review. 🏁 Script executed: Length of output: 19871 Let me now read the key files to do a thorough code review. 🏁 Script executed: Length of output: 25076 🏁 Script executed: Length of output: 35911
📋 PR Summary — Migration PsychicHttp → ESPAsyncWebServerAuthor: This is a well-structured, methodical migration covering all 9 planned phases. The API mapping table in the PR description is accurate and fully executed. All PsychicHttp surface areas have been replaced correctly. Below is the detailed analysis. ✅ What Was Done Well1. Security Layer — Correct & CleanThe new typedefs in typedef std::function<void(AsyncWebServerRequest *)> ArHttpRequestCallback;
typedef std::function<void(AsyncWebServerRequest *, JsonVariant &)> ArJsonRequestCallback;
typedef std::function<bool(AsyncWebServerRequest *)> ArRequestFilterFunc;The 2. WebSocket Authentication — Correctly AppliedBoth 3. HttpEndpoint POST — Correct Handler PatternUsing 4. UploadFirmwareService — Significantly ImprovedThis is actually better than the original. It now adds:
5. ESP32SvelteKit.cpp — Cleaner Server StartupRemoval of 6. Build Configuration
|
| Priority | Action | File |
|---|---|---|
| 🔴 High | Fix mutex scope in emitEvent() — release before client->text() calls |
EventSocket.cpp |
| 🔴 High | Fix iterator invalidation — collect stale IDs before removing | EventSocket.cpp |
| 🟡 Medium | Fix Update.begin(fsize) — remove - sizeof(esp_image_header_t) |
UploadFirmwareService.cpp |
| 🟡 Medium | Wire handleEarlyDisconnect() or remove dead code |
UploadFirmwareService.h/.cpp |
| 🟡 Medium | Benchmark heap under concurrent connections (2–4 clients) to validate the async model benefit | — |
| 🟢 Low | Add cleanupClients() call periodically on AsyncWebSocket to flush stale entries |
EventSocket.cpp, WebSocketServer.h |
| 🟢 Low | Consider UPDATE_SIZE_UNKNOWN in Update.begin() for robustness |
UploadFirmwareService.cpp |
| 🟢 Low | Add fragmented WebSocket frame reassembly to onFrame() / onWSFrame() (currently silently dropped) |
EventSocket.cpp, WebSocketServer.h |
| 🟢 Low | Resolve Windows SCons/ranlib linking issue to enable CI on Windows hosts | CI environment |
✅ Actions performed
Full review triggered.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
lib/framework/EventSocket.cpp (1)
142-205:⚠️ Potential issue | 🟠 MajorDeadlock risk: Mutex held during potentially blocking network I/O.
The mutex
clientSubscriptionsMutexis held from line 142 through line 205, including duringclient->text()/client->binary()calls (lines 177, 179, 197, 199). These network operations may block if the client's send buffer is full, potentially causing deadlock if another task attempts to acquire the same mutex.🔧 Recommended fix: Copy targets under mutex, release before sending
void EventSocket::emitEvent(String event, JsonObject &jsonObject, const char *originId, bool onlyToSameOrigin) { // ... validation code ... int originSubscriptionId = originId[0] ? atoi(originId) : -1; + + // Collect client IDs under mutex + std::vector<uint32_t> targetClients; xSemaphoreTake(clientSubscriptionsMutex, portMAX_DELAY); auto &subscriptions = client_subscriptions[event]; if (subscriptions.empty()) { xSemaphoreGive(clientSubscriptionsMutex); return; } + + if (onlyToSameOrigin && originSubscriptionId > 0) + { + targetClients.push_back((uint32_t)originSubscriptionId); + } + else + { + for (int subscription : subscriptions) + { + if (subscription != originSubscriptionId) + targetClients.push_back((uint32_t)subscription); + } + } + xSemaphoreGive(clientSubscriptionsMutex); // ... serialize message ... - if (onlyToSameOrigin && originSubscriptionId > 0) - { - // ... send under mutex ... - } - else + // Send outside mutex + for (uint32_t clientId : targetClients) { - // ... iterate and send under mutex ... + auto *client = _socket.client(clientId); + if (client) + { +#if FT_ENABLED(EVENT_USE_JSON) + client->text(output, len); +#else + client->binary(output, len); +#endif + } } delete[] output; - xSemaphoreGive(clientSubscriptionsMutex); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/framework/EventSocket.cpp` around lines 142 - 205, The code currently holds clientSubscriptionsMutex while performing potentially blocking network I/O (client->text/binary), risking deadlock; fix by copying necessary send targets and the prepared message while holding the mutex, then release clientSubscriptionsMutex before iterating and calling _socket.client(...) and client->text/binary; specifically: build the JsonDocument and allocate/serialize the output buffer (measureJson/serializeJson or measureMsgPack/serializeMsgPack) and copy client IDs from client_subscriptions[event] into a local vector (respecting onlyToSameOrigin and originSubscriptionId) while holding clientSubscriptionsMutex, then xSemaphoreGive(clientSubscriptionsMutex) before calling _socket.client(...) and sending; if you detect missing/dead clients during sending, collect those IDs and re-take clientSubscriptionsMutex to remove them from client_subscriptions[event]; ensure output is deleted after use. Reference symbols: clientSubscriptionsMutex, client_subscriptions, JsonDocument/doc, measureJson/measureMsgPack, serializeJson/serializeMsgPack, output, onlyToSameOrigin, originSubscriptionId, _socket.client, client->text, client->binary, subscriptions.lib/framework/EthernetSettingsService.cpp (1)
19-31:⚠️ Potential issue | 🟠 MajorDefer Ethernet reinitialization until after the async request completes.
This update hook calls
reconfigureEthernet()directly, which eventually invokesETH.end()/ETH.config()/ETH.begin(). With AsyncWebServer, request handlers run on the AsyncTCP task (not the main loop), and calling blocking network reinitialization inline can stall the response transmission, drop the TCP connection carrying the response, or trigger a watchdog reset. Queue the restart via a flag and execute it fromloop()or a separate worker task after the response has been flushed.Suggested direction
- addUpdateHandler([&](const String &originId) - { reconfigureEthernet(); }, + addUpdateHandler([&](const String &originId) + { _pendingEthernetReconfigure = true; }, false);Then drain
_pendingEthernetReconfigurefromloop()(or a dedicated worker) and callreconfigureEthernet()there.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/framework/EthernetSettingsService.cpp` around lines 19 - 31, The update handler in EthernetSettingsService's constructor currently calls reconfigureEthernet() inline; change the lambda passed to addUpdateHandler to set a boolean flag (e.g. _pendingEthernetReconfigure = true) instead of calling reconfigureEthernet() directly, and ensure the flag (_pendingEthernetReconfigure) is declared on the class; then drain that flag from the main loop() (or a dedicated worker task) and call reconfigureEthernet() there so the ETH.end()/ETH.config()/ETH.begin() sequence runs off the AsyncWebServer request task after the response is flushed.lib/framework/APSettingsService.cpp (1)
17-30:⚠️ Potential issue | 🟠 MajorAdd proper synchronization for the AP reconfigure handoff.
With the AsyncWebServer migration,
reconfigureAP()now executes on the async networking task whilemanageAP()on the Arduino loop task clears_reconfigureApat line 84. Thevolatilequalifier does not prevent race conditions—a pending reconfigure or recovery-mode request can be lost when one task sets the flag and the other clears it. Use a proper cross-task synchronization primitive: a queue, task notification, semaphore, event group, or mutex/critical section to protect the shared state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/framework/APSettingsService.cpp` around lines 17 - 30, Replace the volatile flag with a proper cross-task sync primitive so reconfigure requests from the AsyncWebServer task are not lost by the Arduino loop task: add a member like a FreeRTOS binary semaphore or task notification (e.g., _reconfigureSemaphore or use taskNotify with a target task handle), initialize it in the APSettingsService constructor, signal it from the addUpdateHandler lambda / reconfigureAP() caller (use xSemaphoreGiveFromISR or xTaskNotifyFromISR if called from ISR context), and in manageAP() replace the direct _reconfigureAp check/clear with a blocking/polling semaphore take or notification check (xSemaphoreTake or ulTaskNotifyTake) to atomically consume the request; update reconfigureAP(), manageAP(), and the constructor to use these new symbols and remove the volatile _reconfigureAp usage.lib/framework/ESP32SvelteKit.cpp (1)
190-193:⚠️ Potential issue | 🟡 MinorSleep callback deletes the loop task but doesn't shut down the AsyncWebServer.
The code calls
vTaskDelete(_loopTaskHandle)and logs "Server stopped", but it never calls_server->end()to actually stop the server. Add_server->end()to properly shut down the server before sleep.Proposed fix
_sleepService.attachOnSleepCallback([&]() { ESP_LOGI(SVK_TAG, "Attempting to stop server"); vTaskDelete(_loopTaskHandle); + _server->end(); ESP_LOGI(SVK_TAG, "Server stopped"); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/framework/ESP32SvelteKit.cpp` around lines 190 - 193, The sleep callback currently deletes the loop task via vTaskDelete(_loopTaskHandle) but never shuts down the AsyncWebServer; update the lambda passed to _sleepService.attachOnSleepCallback to call _server->end() before deleting the task so the server is properly stopped (reference the attachOnSleepCallback lambda, _server->end(), vTaskDelete and _loopTaskHandle).
🧹 Nitpick comments (4)
lib/framework/NTPStatus.h (1)
23-25: Remove implementation-only includes from this header.
AsyncJson.his not referenced in any declarations here—it's only needed in the.cppimplementation forAsyncJsonResponse. Additionally,SecurityManageris used only as a pointer parameter and member, so it can be forward-declared instead of fully included. KeepESPAsyncWebServer.hsinceAsyncWebServerandAsyncWebServerRequestare part of the public interface.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/framework/NTPStatus.h` around lines 23 - 25, In NTPStatus.h remove the implementation-only include AsyncJson.h and replace the full include of SecurityManager.h with a forward declaration (e.g., `class SecurityManager;`) since SecurityManager is only used as a pointer parameter/member; keep `#include` <ESPAsyncWebServer.h> because AsyncWebServer and AsyncWebServerRequest are part of the public interface. Update any declarations in NTPStatus.h to rely on the forward-declared SecurityManager and move the AsyncJson include into the corresponding .cpp where AsyncJsonResponse is actually used.lib/framework/WebSocketServer.h (1)
77-84: Fragmented WebSocket frames are silently dropped.The condition
info->final && info->index == 0 && info->len == lenonly processes complete single-frame messages. Multi-frame or fragmented messages will be ignored without warning.This is acceptable for typical JSON payloads that fit in a single frame, but consider adding a log message for dropped fragments to aid debugging:
📝 Optional: Log dropped fragments
if (info->final && info->index == 0 && info->len == len) { onWSFrame(client, info, data, len); } + else + { + ESP_LOGW(SVK_TAG, "ws[%u] dropped fragmented frame (index=%u, len=%u, total=%u)", + client->id(), info->index, len, info->len); + } break;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/framework/WebSocketServer.h` around lines 77 - 84, The WS_EVT_DATA handler currently only calls onWSFrame when info->final && info->index == 0 && info->len == len, which silently drops fragmented frames; update the handler around WS_EVT_DATA/AwsFrameInfo to either accumulate fragments into a buffer until info->final is true and then call onWSFrame with the reassembled payload, or at minimum log a warning (including info->final, info->index, info->len) when a fragment is received and you choose to drop it; ensure you reference the AwsFrameInfo fields (final, index, len) and the onWSFrame call so the fix is applied in the same control branch where fragments are currently ignored.lib/framework/ESP32SvelteKit.cpp (1)
77-77: Consider moving_server->begin()after route registration.The server is started before routes are registered via
WWWData::registerRoutes()on line 82. While ESPAsyncWebServer allows dynamic route registration, the conventional pattern is to register all routes first, then callbegin(). This ensures all handlers are ready before accepting connections.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/framework/ESP32SvelteKit.cpp` at line 77, Move the call to _server->begin() so it executes after WWWData::registerRoutes() has registered all handlers: locate the sequence where _server->begin() is called and WWWData::registerRoutes() is invoked, remove or relocate the _server->begin() call to just after WWWData::registerRoutes() returns (ensuring all route registration happens first) so the server starts accepting connections only after handlers are set up.lib/framework/DownloadFirmwareService.cpp (1)
60-60: Remove unused static variableotaURL.The
otaURLvariable is declared but never used. The download URL is now passed to the update task via a heap-allocatedString*(lines 236-242), making this declaration dead code.Proposed fix
static EventSocket *_socket = nullptr; static int previousProgress = 0; -static String *otaURL = nullptr; JsonDocument doc;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/framework/DownloadFirmwareService.cpp` at line 60, Remove the dead static variable otaURL: find and delete the declaration "static String *otaURL = nullptr;" in DownloadFirmwareService.cpp since the download URL is now passed via a heap-allocated String* to the update task (see the code that allocates/pushes the String pointer around lines that create the update task). Ensure no other code references otaURL; if any references remain, update them to use the passed-in String* (the update task parameter) before removing the declaration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/framework/AuthenticationService.cpp`:
- Around line 27-42: The SIGN_IN_PATH JSON POST handler uses a raw lambda
instead of the project's standard JSON wrapper; update the endpoint registration
to call _securityManager->wrapCallback with the existing lambda and pass
AuthenticationPredicates::ALLOW_ALL so the handler signature matches other
services (e.g., NTPSettingsService.cpp); keep the inner logic (reading
json["username"], json["password"], calling _securityManager->authenticate and
generateJWT) unchanged and return/send the same responses.
In `@lib/framework/CoreDump.cpp`:
- Around line 52-64: The chunked callback silently returns 0 on esp_flash_read
failure (in the lambda passed to request->beginChunkedResponse), which signals
EOF and yields a truncated 200 OK to the client; instead, read the known-size
coredump up-front and use a non-chunked response so the client can validate
Content-Length: allocate a buffer of coredump_size, call
esp_flash_read(esp_flash_default_chip, buffer, coredump_addr, coredump_size),
and if that read fails call request->send(statusCode 500, "text/plain",
"coredump read failed") (or similar) and free the buffer; on success create a
response with the full payload and Content-Length (e.g.,
request->beginResponse("application/octet-stream", coredump_size, buffer) or the
appropriate AsyncWebServerResponse constructor) and send it, ensuring buffer
cleanup on error and success.
In `@lib/framework/EventSocket.cpp`:
- Around line 185-201: The range-for over client_subscriptions[event] mutates
the same container by calling subscriptions.remove(subscription) which
invalidates the iterator; instead, in the function containing the loop (the
block referencing client_subscriptions, subscriptions, originSubscriptionId and
_socket.client), collect subscription IDs to remove in a temporary vector/list
while iterating (skip originSubscriptionId as before), and after the loop
iterate that temporary collection to call subscriptions.remove(...) once; ensure
the emission logic (ESP_LOGV and client->text/client->binary) remains unchanged
and use the same client lookup via _socket.client((uint32_t)subscription).
In `@lib/framework/RestartService.cpp`:
- Around line 32-36: The current RestartService::restart calls
request->send(200) then restartNow(), risking a restart before the async
response is transmitted; modify RestartService::restart to register a
disconnect/completion callback on the AsyncWebServerRequest (e.g., use
request->onDisconnect(...) or equivalent completion callback) that calls
restartNow(), and then call request->send(200) so restartNow() runs only after
the client connection closes and the response is delivered.
In `@lib/framework/UploadFirmwareService.cpp`:
- Around line 360-369: The handleEarlyDisconnect method in UploadFirmwareService
is defined but never used; either register it as the onDisconnect handler for
the upload endpoint or remove it. If wiring, attach
UploadFirmwareService::handleEarlyDisconnect to the upload endpoint's
onDisconnect callback (the same endpoint that starts the Update flow) so
Update.end(true)/Update.abort() is invoked on client disconnect; if your
framework doesn't support onDisconnect, delete handleEarlyDisconnect and any
Update.abort() expectations in UploadFirmwareService to avoid dead code. Ensure
references are to UploadFirmwareService::handleEarlyDisconnect, Update.end and
Update.abort, and the upload endpoint's onDisconnect registration point.
- Line 180: The call to Update.begin currently subtracts
sizeof(esp_image_header_t) from fsize which underreports the total firmware
size; change the Update.begin call in the upload flow (where Update.begin(...)
is invoked before Update.write(...)) to use the actual total size
(Update.begin(fsize)) or UPDATE_SIZE_UNKNOWN instead so the Update library can
correctly validate space and compute remaining(); keep validateChipType(), the
esp_image_header_t handling, and Update.write(...) unchanged—only update the
Update.begin(...) argument to either fsize or UPDATE_SIZE_UNKNOWN.
In `@platformio.ini`:
- Around line 119-132: Remove hard-coded factory AP/UI credentials from the
tracked defaults (the -D defines FACTORY_AP_SSID, FACTORY_AP_PASSWORD,
FACTORY_ADMIN_USERNAME, FACTORY_ADMIN_PASSWORD, FACTORY_GUEST_USERNAME,
FACTORY_GUEST_PASSWORD, etc.) and move them into an untracked override (e.g., a
local or environment-specific platformio override file) or gate them behind a
required build-time secret so a build fails if overrides are not provided;
update build logic to read those values from the override/environment and ensure
the defaults in the committed platformio.ini are non-sensitive placeholders or
absent.
- Line 36: Remove the insecure default by deleting the
DOWNLOAD_OTA_SKIP_CERT_VERIFY flag from the shared defaults in platformio.ini
(the global DEFAULTS block) so TLS certificate verification remains enabled for
OTA firmware downloads; if you need to disable verification for local testing
only, add -D DOWNLOAD_OTA_SKIP_CERT_VERIFY to a local/dev override profile
instead (do not keep it in the shared defaults and ensure the firmware
download-enabled profile referenced at Line 160 does not import this flag).
- Around line 135-136: FACTORY_NTP_TIME_ZONE_FORMAT currently encodes UK GMT/BST
while FACTORY_NTP_TIME_ZONE_LABEL is Europe/Berlin; update the TZ format string
to the correct CET/CEST encoding so factory defaults use Berlin DST rules:
change the value of FACTORY_NTP_TIME_ZONE_FORMAT to the Europe/Berlin form
(i.e., CET-1CEST,M3.5.0,M10.5.0/3) alongside the existing
FACTORY_NTP_TIME_ZONE_LABEL to ensure device local time and DST are correct.
- Line 64: The platform_packages entry currently references the Xtensa toolchain
("platform_packages = platformio/toolchain-xtensa-esp-elf@14.2.0+20241119")
which is wrong for the ESP32-C3; update that platform_packages value to use the
RISC-V toolchain "platformio/toolchain-riscv32-esp" (replace
"toolchain-xtensa-esp-elf" with "toolchain-riscv32-esp" and choose the
appropriate version specifier) so the esp32-c3-devkitm-1 environment uses the
correct RISC-V toolchain.
---
Outside diff comments:
In `@lib/framework/APSettingsService.cpp`:
- Around line 17-30: Replace the volatile flag with a proper cross-task sync
primitive so reconfigure requests from the AsyncWebServer task are not lost by
the Arduino loop task: add a member like a FreeRTOS binary semaphore or task
notification (e.g., _reconfigureSemaphore or use taskNotify with a target task
handle), initialize it in the APSettingsService constructor, signal it from the
addUpdateHandler lambda / reconfigureAP() caller (use xSemaphoreGiveFromISR or
xTaskNotifyFromISR if called from ISR context), and in manageAP() replace the
direct _reconfigureAp check/clear with a blocking/polling semaphore take or
notification check (xSemaphoreTake or ulTaskNotifyTake) to atomically consume
the request; update reconfigureAP(), manageAP(), and the constructor to use
these new symbols and remove the volatile _reconfigureAp usage.
In `@lib/framework/ESP32SvelteKit.cpp`:
- Around line 190-193: The sleep callback currently deletes the loop task via
vTaskDelete(_loopTaskHandle) but never shuts down the AsyncWebServer; update the
lambda passed to _sleepService.attachOnSleepCallback to call _server->end()
before deleting the task so the server is properly stopped (reference the
attachOnSleepCallback lambda, _server->end(), vTaskDelete and _loopTaskHandle).
In `@lib/framework/EthernetSettingsService.cpp`:
- Around line 19-31: The update handler in EthernetSettingsService's constructor
currently calls reconfigureEthernet() inline; change the lambda passed to
addUpdateHandler to set a boolean flag (e.g. _pendingEthernetReconfigure = true)
instead of calling reconfigureEthernet() directly, and ensure the flag
(_pendingEthernetReconfigure) is declared on the class; then drain that flag
from the main loop() (or a dedicated worker task) and call reconfigureEthernet()
there so the ETH.end()/ETH.config()/ETH.begin() sequence runs off the
AsyncWebServer request task after the response is flushed.
In `@lib/framework/EventSocket.cpp`:
- Around line 142-205: The code currently holds clientSubscriptionsMutex while
performing potentially blocking network I/O (client->text/binary), risking
deadlock; fix by copying necessary send targets and the prepared message while
holding the mutex, then release clientSubscriptionsMutex before iterating and
calling _socket.client(...) and client->text/binary; specifically: build the
JsonDocument and allocate/serialize the output buffer (measureJson/serializeJson
or measureMsgPack/serializeMsgPack) and copy client IDs from
client_subscriptions[event] into a local vector (respecting onlyToSameOrigin and
originSubscriptionId) while holding clientSubscriptionsMutex, then
xSemaphoreGive(clientSubscriptionsMutex) before calling _socket.client(...) and
sending; if you detect missing/dead clients during sending, collect those IDs
and re-take clientSubscriptionsMutex to remove them from
client_subscriptions[event]; ensure output is deleted after use. Reference
symbols: clientSubscriptionsMutex, client_subscriptions, JsonDocument/doc,
measureJson/measureMsgPack, serializeJson/serializeMsgPack, output,
onlyToSameOrigin, originSubscriptionId, _socket.client, client->text,
client->binary, subscriptions.
---
Nitpick comments:
In `@lib/framework/DownloadFirmwareService.cpp`:
- Line 60: Remove the dead static variable otaURL: find and delete the
declaration "static String *otaURL = nullptr;" in DownloadFirmwareService.cpp
since the download URL is now passed via a heap-allocated String* to the update
task (see the code that allocates/pushes the String pointer around lines that
create the update task). Ensure no other code references otaURL; if any
references remain, update them to use the passed-in String* (the update task
parameter) before removing the declaration.
In `@lib/framework/ESP32SvelteKit.cpp`:
- Line 77: Move the call to _server->begin() so it executes after
WWWData::registerRoutes() has registered all handlers: locate the sequence where
_server->begin() is called and WWWData::registerRoutes() is invoked, remove or
relocate the _server->begin() call to just after WWWData::registerRoutes()
returns (ensuring all route registration happens first) so the server starts
accepting connections only after handlers are set up.
In `@lib/framework/NTPStatus.h`:
- Around line 23-25: In NTPStatus.h remove the implementation-only include
AsyncJson.h and replace the full include of SecurityManager.h with a forward
declaration (e.g., `class SecurityManager;`) since SecurityManager is only used
as a pointer parameter/member; keep `#include` <ESPAsyncWebServer.h> because
AsyncWebServer and AsyncWebServerRequest are part of the public interface.
Update any declarations in NTPStatus.h to rely on the forward-declared
SecurityManager and move the AsyncJson include into the corresponding .cpp where
AsyncJsonResponse is actually used.
In `@lib/framework/WebSocketServer.h`:
- Around line 77-84: The WS_EVT_DATA handler currently only calls onWSFrame when
info->final && info->index == 0 && info->len == len, which silently drops
fragmented frames; update the handler around WS_EVT_DATA/AwsFrameInfo to either
accumulate fragments into a buffer until info->final is true and then call
onWSFrame with the reassembled payload, or at minimum log a warning (including
info->final, info->index, info->len) when a fragment is received and you choose
to drop it; ensure you reference the AwsFrameInfo fields (final, index, len) and
the onWSFrame call so the fix is applied in the same control branch where
fragments are currently ignored.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ae835cfd-221c-4dbc-8f75-5531cd3f6f92
📒 Files selected for processing (56)
lib/framework/APSettingsService.cpplib/framework/APSettingsService.hlib/framework/APStatus.cpplib/framework/APStatus.hlib/framework/AuthenticationService.cpplib/framework/AuthenticationService.hlib/framework/CoreDump.cpplib/framework/CoreDump.hlib/framework/DownloadFirmwareService.cpplib/framework/DownloadFirmwareService.hlib/framework/ESP32SvelteKit.cpplib/framework/ESP32SvelteKit.hlib/framework/EthernetSettingsService.cpplib/framework/EthernetSettingsService.hlib/framework/EthernetStatus.cpplib/framework/EthernetStatus.hlib/framework/EventEndpoint.hlib/framework/EventSocket.cpplib/framework/EventSocket.hlib/framework/FactoryResetService.cpplib/framework/FactoryResetService.hlib/framework/FeaturesService.cpplib/framework/FeaturesService.hlib/framework/HttpEndpoint.hlib/framework/MqttSettingsService.cpplib/framework/MqttSettingsService.hlib/framework/MqttStatus.cpplib/framework/MqttStatus.hlib/framework/NTPSettingsService.cpplib/framework/NTPSettingsService.hlib/framework/NTPStatus.cpplib/framework/NTPStatus.hlib/framework/RestartService.cpplib/framework/RestartService.hlib/framework/SecurityManager.hlib/framework/SecuritySettingsService.cpplib/framework/SecuritySettingsService.hlib/framework/SleepService.cpplib/framework/SleepService.hlib/framework/SystemStatus.cpplib/framework/SystemStatus.hlib/framework/UploadFirmwareService.cpplib/framework/UploadFirmwareService.hlib/framework/WebSocketServer.hlib/framework/WiFiScanner.cpplib/framework/WiFiScanner.hlib/framework/WiFiSettingsService.cpplib/framework/WiFiSettingsService.hlib/framework/WiFiStatus.cpplib/framework/WiFiStatus.hplatformio.inisrc/LightMqttSettingsService.cppsrc/LightMqttSettingsService.hsrc/LightStateService.cppsrc/LightStateService.hsrc/main.cpp
For what it's worth, I made a lot of efforts to free up more heap in the past, really making sizeof() of things smaller and mostly I didn't see it in the free heap values. So not sure how it works, maybe these numbers is not the whole truth and things might still be better... PS: see the coderabbit reviews, you might take a look at them to improve things |
|
I have corrected, in my opinion, the most correct remarks of the rabbit. Some of them affect the style of the code - I think they are not critical. I also added my script analyze_dump.py, which makes it easy to analyze dumps using esp-idf. Added documentation on dumps, as well as how the script works. I've allocated a separate mkdocs section for it, and I don't know if this is correct. There is a large volume with examples, so it takes up a lot of space. |
|
@ewowi I will answer your comment here, to be honest, I did not analyze the work after migration. At first glance, the page started working a little faster. But I didn't take any measurements about it. The problem with memory utilization was previously associated with the transition to pioarduino. I've even done issues on this issue before. I still use espressif32 @ 6.12.0 in my project. This is due to the fact that I have implemented a telegram bot, but it is very expensive. During the ssl connection, the bot consumes from 35 to 50 kb and the controller balances on the edge using 75% RAM. |
|
I was very surprised today. On the espressif32 @ 6.12.0 framework, the free memory size is much higher than on pioarduino, even though I have several other processes running that require resources. |
|
@ewowi I think I've fixed all the comments. Will you accept MR? |
Hi @thedemoncat , that sounds great! I am willing to accept but more important upstream needs to accept. @theelims can review it and check if I can submit it as pull request to the main repo? And maybe we should ask in our discord if people want to test |
|
I will close this account and make a request to the main repository. |
2 participants
I have migrated the web server. The code has been compiled successfully and is working (I've been checking for ESP32-S3 N16R8.). Below is a revised plan of what has been done
Migration Plan: PsychicHttp → ESPAsyncWebServer
PR Reference: #1
Target Library: ESP32Async/ESPAsyncWebServer v3.10.0
Status: In Progress
Motivation
Scope
Impact Assessment
PsychictypesAPI Mapping Quick Reference
PsychicHttpServerAsyncWebServer(80)PsychicRequest*AsyncWebServerRequest*PsychicJsonResponseAsyncJsonResponse*PsychicWebSocketHandlerAsyncWebSocket("/path")PsychicWebSocketClient*AsyncWebSocketClient*PsychicUploadHandlerrequest->reply(code)request->send(code)response.send()request->send(response)client->sendMessage(msg)client->text(msg)client->socket()client->id()_socket.sendAll(msg)_socket.textAll(msg)esp_err_tvoidPhase 1: Build Configuration ✅
Files:
platformio.iniTasks
lib/PsychicHttp/folder (done in original PR commit)ESP32Async/ESPAsyncWebServer#v3.10.0→ESP32Async/ESPAsyncWebServer @ ^3.10.0Notes
PsychicMqttClientstays — it will pull its own PsychicHttp dependency internallylib_compat_mode = strictis already set, AsyncTCP will auto-resolvePhase 2: Security Layer ✅
Files:
SecurityManager.h,SecuritySettingsService.h,SecuritySettingsService.cppWhy first?
Every endpoint uses
wrapRequest(),wrapCallback(),filterRequest()from SecurityManager. Nothing compiles without this.Tasks
ArHttpRequestCallback=std::function<void(AsyncWebServerRequest*)>ArJsonRequestCallback=std::function<void(AsyncWebServerRequest*, JsonVariant&)>ArRequestFilterFunc=std::function<bool(AsyncWebServerRequest*)>SecurityManager.h— change all virtual method signaturesSecuritySettingsService.h— change member types and method signaturesSecuritySettingsService.cpp:authenticateRequest()—request->header()→request->getHeader()->value()filterRequest()— removed PsychicHttp bogus request hack andloadParams()wrapRequest()— returns void, usesrequest->send(401)wrapCallback()— same patterngenerateToken()— usesAsyncJsonResponse*, addedhasParamguardKey changes
Phase 3: Core Templates (partial ✅)
Files:
HttpEndpoint.h,EventEndpoint.h,WebSocketServer.hWhy second?
These template classes define the patterns used by ALL service classes.
Tasks
HttpEndpoint.h:PsychicHttpServer*withAsyncWebServer*AsyncJsonResponse*withresponse->setLength()before sendAsyncCallbackJsonWebHandler+addHandler()instead of_server->on()for JSONvoidEventEndpoint.h:WebSocketServer.h:PsychicWebSocketHandlerwithAsyncWebSocketonOpen/onClose/onFramewith unifiedonEventcallbackclient->socket()withclient->id()client->sendMessage()withclient->text()_webSocket.sendAll()with_webSocket.textAll()setFilter()— authentication needs different approach with AsyncWebSocketPhase 4: Core Server & Main
Files:
ESP32SvelteKit.h,ESP32SvelteKit.cpp,src/main.cppTasks
src/main.cpp:#include <PsychicHttpServer.h>with#include <ESPAsyncWebServer.h>PsychicHttpServer server;withAsyncWebServer server(80);ESP32SvelteKit.h:#include <PsychicHttp.h>with#include <ESPAsyncWebServer.h>PsychicHttpServer*member and constructor param_numberEndpoints(not needed — AsyncWebServer handlers are dynamic)ESP32SvelteKit.cpp:_server->config.max_uri_handlers = _numberEndpoints;_server->listen(80)with_server->begin()onNotFoundhandler_server->getClientList()in sleep callback (different API)Phase 5: EventSocket (WebSocket)
Files:
EventSocket.h,EventSocket.cppThis is the most complex part of the migration.
Tasks
EventSocket.h:PsychicWebSocketHandler _socketwithAsyncWebSocket _socketonWSOpen(PsychicWebSocketClient*)→onWSOpen(AsyncWebSocketClient*)onWSClose(PsychicWebSocketClient*)→onWSClose(AsyncWebSocketClient*)onFrame(PsychicWebSocketRequest*, httpd_ws_frame*)→onFrame(AsyncWebSocketClient*, AwsFrameInfo*, uint8_t*, size_t)EventSocket.cpp:_socket.onOpen/onClose/onFrame+_server->on()with_socket.onEvent()+_server->addHandler()client->socket()→client->id()HTTPD_WS_TYPE_TEXT/BINARY→WS_TEXT/WS_BINARY(viainfo->opcode)client->sendMessage(type, data, len)→client->text(data, len)/client->binary(data, len)_socket.getClient(id)→_socket.client(id)_socket.getClientList().size()→_socket.count()uint8_t*+size_t, not viaframe->payloadPhase 6: Service Layer (Batch)
Files: All service .h/.cpp files in
lib/framework/These are mostly mechanical replacements since they follow patterns from HttpEndpoint.
Files to update:
WiFiStatus.h/.cppWiFiScanner.h/.cppWiFiSettingsService.h/.cppNTPStatus.h/.cppNTPSettingsService.h/.cppMqttStatus.h/.cppMqttSettingsService.h/.cppSystemStatus.h/.cppSleepService.h/.cppAPStatus.h/.cppAPSettingsService.h/.cppEthernetStatus.h/.cppEthernetSettingsService.h/.cppRestartService.h/.cppFactoryResetService.h/.cppAuthenticationService.h/.cppFeaturesService.h/.cppCoreDump.h/.cppDownloadFirmwareService.h/.cppMqttEndpoint.hCommon replacements per file:
Phase 7: Upload Firmware Service
Files:
UploadFirmwareService.h,UploadFirmwareService.cppTasks
PsychicUploadHandler— use built-in upload inserver.on()index:uint64_t→size_tesp_err_t→void_server->maxUploadSize— validate in handleruploadCompleteandhandleErrorreturn types tovoidrequest->reply(code)withrequest->send(code)PsychicJsonResponsewithAsyncJsonResponserequest->_tempObject— works the same wayPhase 8: Application Layer
Files:
src/LightStateService.h,src/LightStateService.cpp,src/LightMqttSettingsService.h,src/LightMqttSettingsService.cppTasks
#include <PsychicHttp.h>with#include <ESPAsyncWebServer.h>PsychicHttpServer*withAsyncWebServer*Phase 9: Compile & Fix
Tasks
esp32devtargetBuild Status
ranlib.execannot find output.afile; SCons.sconsign313.tmpwrite failsRisk Register
Progress Tracking
Summary by CodeRabbit
New Features
Refactor
Chores