From cfa298f053fc8a08fd714d51995fcd8eb9474c59 Mon Sep 17 00:00:00 2001 From: cjkas Date: Tue, 27 Oct 2026 22:11:54 +0100 Subject: [PATCH] fix: review fixes for step 4a - e2e: the heap check uses a socket slot now, so it waits for the device to free it, and test_every_client_reconnecting_at_once closes its clients before the check runs. - Comments and docs that still named removed things (the /controller read, the event buffer, memStatus, SocketEmitter, ModelSnapshot). - ESPNetwork::emitSockets is emitStatus: one socket, and it sends the status. - Unused includes, whitespace-only lines left by the toJSON removal. Co-Authored-By: Claude Opus 5.5 --- GLOSSARY.md | 2 +- docs/PROTOCOL.md | 2 +- ...task-is-the-only-writer-of-the-shade-model.md | 2 +- src/ConfigSettings.h | 16 ++++++++-------- src/ESPNetwork.cpp | 7 +++---- src/ESPNetwork.h | 2 +- src/SomfyController.cpp | 2 +- src/entryPoints/HttpRoutes.cpp | 4 ++-- src/entryPoints/Web.cpp | 1 - src/outputs/ProtoSocket.cpp | 2 +- test/e2e/proto_client.py | 3 +++ test/e2e/test_survival.py | 4 +++- test/test_utils/test_utils.cpp | 2 +- 13 files changed, 26 insertions(+), 23 deletions(-) diff --git a/GLOSSARY.md b/GLOSSARY.md index e84de71..5753199 100644 --- a/GLOSSARY.md +++ b/GLOSSARY.md @@ -84,7 +84,7 @@ The seam through which a shade's behaviour reports what happened: changes for th **Entry point** Where a request from outside reaches the firmware: an HTTP route, an MQTT command topic or a frame received from a remote. -_Avoid_: controller (it already names the owner of the shade list and the `/controller` read) +_Avoid_: controller (it already names the owner of the shade list) **HttpRoutes** The HTTP routes on port 80: the web UI's files, login, the file transfers and the three commands for scripts. Everything else is an op on the socket. diff --git a/docs/PROTOCOL.md b/docs/PROTOCOL.md index 7457a9b..2bbafeb 100644 --- a/docs/PROTOCOL.md +++ b/docs/PROTOCOL.md @@ -180,4 +180,4 @@ A malformed message gets a bad request, with its `id` when it had one. A type a ## Inside the device -async_tcp reads client frames and answers with fixed frames that hold nothing of the shade model: `hello`, errors and `ok`. Everything formatted from the shade model goes out from `loop()`, which is the only task that reads and writes it ([ADR 0002](adr/0002-loop-task-is-the-only-writer-of-the-shade-model.md)). Sync frames and events share the socket event buffer, so protocol 2 needs no more static memory. +async_tcp reads client frames and answers with fixed frames that hold nothing of the shade model: `hello`, errors and `ok`. Everything formatted from the shade model goes out from `loop()`, which is the only task that reads and writes it ([ADR 0002](adr/0002-loop-task-is-the-only-writer-of-the-shade-model.md)). Sync frames and events share one frame buffer, which belongs to `loop()`. diff --git a/docs/adr/0002-loop-task-is-the-only-writer-of-the-shade-model.md b/docs/adr/0002-loop-task-is-the-only-writer-of-the-shade-model.md index ed16116..872d366 100644 --- a/docs/adr/0002-loop-task-is-the-only-writer-of-the-shade-model.md +++ b/docs/adr/0002-loop-task-is-the-only-writer-of-the-shade-model.md @@ -13,7 +13,7 @@ The shade model (rooms, shades, groups and repeaters) is changed only from `loop ## Consequences -> **Amended by [ADR 0005](0005-one-websocket-protocol.md) (socket protocol 2).** The rule above stands. These consequences are replaced: model edits no longer answer "accepted" and report through `requestResult`, because every socket op gets one reply from `loop()`. Reads no longer go through a ModelSnapshot, because `loop()` formats a sync from the live model. The caps on reads and the UI's 5 s `requestResult` wait go with them. +> **Amended by [ADR 0005](0005-one-websocket-protocol.md) (socket protocol 2).** The rule above stands. These consequences are replaced: model edits no longer answer "accepted" and report through `requestResult`, because every socket op gets one reply from `loop()`. Reads no longer go through a ModelSnapshot, because `loop()` formats a sync from the live model. The caps on reads and the UI's 5 s `requestResult` wait go with them, and 4.0.0 removes ModelSnapshot. - async_tcp must still never wait for `loop()`, so a model edit's HTTP reply can't block until the change is made. The reply only says the request was accepted; the result reaches clients as a WebSocket event once `loop()` has made the change. We chose that over a chunked HTTP response that `loop()` completes, because pushing results to clients is what the WebSocket is for and it needs no pool of pending responses. - A read has to reply with data, so it does wait, without blocking: its chunked response sends nothing until `loop()` has filled the read's ModelSnapshot. diff --git a/src/ConfigSettings.h b/src/ConfigSettings.h index b2e2f38..759a389 100644 --- a/src/ConfigSettings.h +++ b/src/ConfigSettings.h @@ -45,7 +45,7 @@ class BaseSettings { public: bool loadFile(const char* filename); bool fromJSON(JsonObject &obj); - + bool parseIPAddress(JsonObject &obj, const char *prop, IPAddress *); bool parseValueString(JsonObject &obj, const char *prop, char *dest, size_t size); int parseValueInt(JsonObject &obj, const char *prop, int defVal); @@ -58,7 +58,7 @@ class NTPSettings: BaseSettings { char ntpServer[65] = "pool.ntp.org"; char posixZone[64] = ""; bool fromJSON(JsonObject &obj); - + bool apply(); bool begin(); bool save(); @@ -75,7 +75,7 @@ class WifiSettings: BaseSettings { //bool ssdpBroadcast = true; bool begin(); bool fromJSON(JsonObject &obj); - + String mapEncryptionType(int type); bool ssidExists(const char *ssid); void printNetworks(); @@ -99,7 +99,7 @@ class EthernetSettings: BaseSettings { bool begin(); bool fromJSON(JsonObject &obj); - + bool load(); bool save(); void print(); @@ -117,7 +117,7 @@ class IPSettings: BaseSettings { IPAddress dns2 = IPAddress(0,0,0,0); bool begin(); bool fromJSON(JsonObject &obj); - + bool load(); bool save(); void print(); @@ -141,7 +141,7 @@ class SecuritySettings: BaseSettings { bool save(); bool load(); void print(); - + bool fromJSON(JsonObject &obj); }; class MQTTSettings: BaseSettings { @@ -158,7 +158,7 @@ class MQTTSettings: BaseSettings { bool begin(); bool save(); bool load(); - + bool fromJSON(JsonObject &obj); }; class ConfigSettings: BaseSettings { @@ -185,7 +185,7 @@ class ConfigSettings: BaseSettings { SecuritySettings Security; bool requiresAuth(); bool fromJSON(JsonObject &obj); - + bool begin(); bool save(); bool load(); diff --git a/src/ESPNetwork.cpp b/src/ESPNetwork.cpp index a3a7edb..28fce1e 100644 --- a/src/ESPNetwork.cpp +++ b/src/ESPNetwork.cpp @@ -5,7 +5,7 @@ #include "esp_log.h" #include "ConfigSettings.h" #include "ESPNetwork.h" -#include "entryPoints/Web.h" +#include "Somfy.h" #include "outputs/ProtoSocket.h" #include "Utils.h" #include "SSDP.h" @@ -14,7 +14,6 @@ static const char *TAG = "Network"; extern ConfigSettings settings; -extern Web webServer; extern MqttConnection mqtt; extern rebootDelay_t rebootDelay; extern ESPNetwork net; @@ -172,7 +171,7 @@ void ESPNetwork::loop() { } if(millis() - this->lastEmit >= STATUS_EMIT_MS) { this->lastEmit = millis(); - if(this->connected()) this->emitSockets(); + if(this->connected()) this->emitStatus(); resetTaskWdt(); // Make sure we do not reboot here. } @@ -198,7 +197,7 @@ bool ESPNetwork::changeAP(const uint8_t *bssid, const int32_t channel) { } // The status broadcast, every STATUS_EMIT_MS from loop(). A wired link goes // out every time, the WiFi signal only when it really changed. -void ESPNetwork::emitSockets() { +void ESPNetwork::emitStatus() { this->emitHeap(); #if HAS_ETHERNET if(this->connType == conn_types_t::ethernet) { diff --git a/src/ESPNetwork.h b/src/ESPNetwork.h index 77e4935..b9ba77e 100644 --- a/src/ESPNetwork.h +++ b/src/ESPNetwork.h @@ -70,7 +70,7 @@ class ESPNetwork { bool setup(); void loop(); void end(); - void emitSockets(); + void emitStatus(); void emitHeap(); uint32_t getChipId(); static void networkEvent(WiFiEvent_t event); diff --git a/src/SomfyController.cpp b/src/SomfyController.cpp index cd1ac1c..bd0024f 100644 --- a/src/SomfyController.cpp +++ b/src/SomfyController.cpp @@ -155,7 +155,7 @@ void setup() { void loop() { // put your main code here, to run repeatedly: //uint32_t heap = ESP.getFreeHeap(); - // Times the whole pass for memStatus, including any time the loop task was + // Times the whole pass for the memory event, including any time the loop task was // pre-empted, since that delays the loop too. ElapsedTimer pass; if(rebootDelay.reboot && millis() > rebootDelay.rebootTime) { diff --git a/src/entryPoints/HttpRoutes.cpp b/src/entryPoints/HttpRoutes.cpp index a64d092..6a8a08f 100644 --- a/src/entryPoints/HttpRoutes.cpp +++ b/src/entryPoints/HttpRoutes.cpp @@ -53,7 +53,7 @@ void registerHttpRoutes(AsyncWebServer &server) { root["serverId"] = settings.serverId; root["version"] = settings.fwVersion.name; root["model"] = "ESPSomfyRTS"; - root["hostname"] = settings.hostname; + root["hostname"] = settings.hostname; response->setLength(); request->send(response); }); @@ -92,7 +92,7 @@ void registerHttpRoutes(AsyncWebServer &server) { JsonDocument doc; DeserializationError err = deserializeJson(doc, dataStr); if(err) { - request->send(500, "application/json", "{\"status\":\"ERROR\",\"desc\":\"JSON parse error\"}"); + request->send(500, _encoding_json, "{\"status\":\"ERROR\",\"desc\":\"JSON parse error\"}"); return; } else { diff --git a/src/entryPoints/Web.cpp b/src/entryPoints/Web.cpp index 7759dca..19797b9 100644 --- a/src/entryPoints/Web.cpp +++ b/src/entryPoints/Web.cpp @@ -10,7 +10,6 @@ #include "Somfy.h" #include "WResp.h" #include "LoopQueue.h" -#include "ItemSnapshot.h" #include "SomfySnapshot.h" #include "Web.h" #include "WebRoutes.h" diff --git a/src/outputs/ProtoSocket.cpp b/src/outputs/ProtoSocket.cpp index b8db70a..d739451 100644 --- a/src/outputs/ProtoSocket.cpp +++ b/src/outputs/ProtoSocket.cpp @@ -804,7 +804,7 @@ void ProtoSocket::wifiNetworks(int16_t count) { } } bool ProtoSocket::subscribed(proto_feed_t feed) const { - // The feed is formatted into the event buffer, which is loop()'s. + // The feed is formatted into the frame buffer, which is loop()'s. if(!onLoopTask()) return false; const uint8_t bit = 1 << static_cast(feed); for(const proto_slot_t &s : slots) diff --git a/test/e2e/proto_client.py b/test/e2e/proto_client.py index ca87d3d..31b6f7f 100644 --- a/test/e2e/proto_client.py +++ b/test/e2e/proto_client.py @@ -234,6 +234,9 @@ def heap(host, token=None): _, frames, _ = run_sync(c, schema) finally: c.close() + # The device frees the slot when it sees the disconnect; the caller + # may need every slot. + time.sleep(0.5) key = schema.keys["status"] status = schema.expand(next(f["d"][key] for f in frames if key in f["d"])) return {"free": status["freeHeap"], "min": status["minHeap"], "max": status["maxHeap"], "uptime": status["uptime"]} diff --git a/test/e2e/test_survival.py b/test/e2e/test_survival.py index f8ececd..63f3a07 100644 --- a/test/e2e/test_survival.py +++ b/test/e2e/test_survival.py @@ -47,7 +47,9 @@ def all_slots_free(host, token): pytest.skip("%u socket slot(s) in use, likely by the web UI: close its tabs" % taken) -def test_every_client_reconnecting_at_once(connect, schema, watch_heap, all_slots_free): +# watch_heap comes before connect so pytest tears connect down first: the +# heap check after the test needs a free slot. +def test_every_client_reconnecting_at_once(watch_heap, all_slots_free, connect, schema): clients = [connect() for _ in range(MAX_CLIENTS)] errors = [] diff --git a/test/test_utils/test_utils.cpp b/test/test_utils/test_utils.cpp index c941d69..7b8bfba 100644 --- a/test/test_utils/test_utils.cpp +++ b/test/test_utils/test_utils.cpp @@ -232,7 +232,7 @@ static void test_emitAllowed_refuses_another_task() { TEST_ASSERT_FALSE(emitAllowed(&wifiEventTask, &loopTask)); } static void test_emitAllowed_before_the_owner_is_known() { - // setup() before SocketEmitter::begin(), which is loop()'s task anyway. + // setup() before ProtoSocket::begin(), which is loop()'s task anyway. TEST_ASSERT_TRUE(emitAllowed(&loopTask, nullptr)); }