From 0cc0059aa93a923b9458fec7a53c444643130488 Mon Sep 17 00:00:00 2001 From: Paul Barrett Date: Thu, 27 Aug 2026 18:35:58 +0100 Subject: [PATCH 1/2] fix: drive track power from the active transport, and size the WS handshake properly Two things, both surfaced by running the orchestrator transport on the bench. PowerStatusBar read JmriJsonClient directly, so under the orchestrator the power button did nothing and the label read "Disconnected" while the layout was in fact connected. Same shape as the knob gating fixed alongside this: a UI element wired to a concrete client rather than to whichever transport is in use. Track power now goes through the ThrottleBackend port. Strictly it is a layout command rather than a throttle one, but it rides the same connection and the UI needs one place to ask, so it lives there rather than in a second port with two more adapters. supportsTrackPower() lets a transport decline, and the button is hidden rather than left dead when it does. - WiThrottleBackend delegates power to JmriJsonClient, not to WiThrottle's PPA command. Under JMRI that is where power has always gone, and switching it would have quietly changed behaviour for every existing JMRI user. - OrchestratorClient gains setTrackPower(), a REST POST to /api/layouts/{id}/dcc-link/power -- the ClientMessage union has no track-power member. The reply body is deliberately ignored: the DCC_LINK event pushed the moment it lands is what tells us the truth, per the route's own contract. - DCC_LINK is parsed, and read off the snapshot too, so the button is right from the first frame. mainPowerOn is boolean-or-null; null is UNKNOWN, not off, because reporting it as off claims the rails are dead when nobody knows. - The power write happens on a short-lived task. It is a blocking HTTP round trip called straight from an LVGL button handler (F-05). getLayoutId() is now fetched once and cached; refreshRoster() no longer carries its own copy of that lookup. The second thing corrects a wrong fix from the previous commit. I raised esp_websocket_client_config_t::buffer_size and reported that as the cure for "transport_ws: Header size exceeded buffer size". It was not -- that field is only the client's frame buffer. The handshake is built and read in a separate buffer sized by CONFIG_WS_BUFFER_SIZE, a Kconfig value. The one clean connect I saw afterwards was luck, and the failure came straight back. Reading transport_ws.c: the read loop exits once it finds the header terminator, but then still fails if the buffer filled -- and the orchestrator pushes a whole-layout STATE_SNAPSHOT the instant the socket opens, so the 101 and a chunk of that snapshot regularly arrive in one TCP read. That is why it was intermittent: packet timing, not header length. CONFIG_WS_BUFFER_SIZE was also only ever in the generated sdkconfig, never in sdkconfig.defaults, so a clean checkout and CI both built at the 1024 default and could never have connected at all. It is now tracked, at 16384. Verified on hardware: 14 boots, every one that reset cleanly connected on the first attempt with no handshake error, loaded the roster, read track power off the snapshot and applied 2 loco states. Tests: three new, covering that power state comes from the backend, that a change is forwarded to the UI, and that a transport declining power has none faked on its behalf. 58 Tests 0 Failures 0 Ignored on an ESP32-S3 over COM3. Co-Authored-By: Claude Opus 5 --- main/communication/OrchestratorBackend.cpp | 54 ++++ main/communication/OrchestratorBackend.h | 6 + main/communication/OrchestratorClient.cpp | 271 +++++++++++++++++---- main/communication/OrchestratorClient.h | 33 +++ main/communication/ThrottleBackend.h | 26 ++ main/communication/WiThrottleBackend.cpp | 59 ++++- main/communication/WiThrottleBackend.h | 19 +- main/controller/AppController.cpp | 5 +- main/controller/ThrottleController.cpp | 56 +++++ main/controller/ThrottleController.h | 28 +++ main/tests/ThrottleControllerTests.cpp | 84 +++++++ main/ui/MainScreen.cpp | 8 +- main/ui/components/PowerStatusBar.cpp | 135 +++++----- main/ui/components/PowerStatusBar.h | 23 +- sdkconfig.defaults | 20 ++ 15 files changed, 710 insertions(+), 117 deletions(-) diff --git a/main/communication/OrchestratorBackend.cpp b/main/communication/OrchestratorBackend.cpp index 05cb96a..30c1f70 100644 --- a/main/communication/OrchestratorBackend.cpp +++ b/main/communication/OrchestratorBackend.cpp @@ -53,6 +53,7 @@ OrchestratorBackend::~OrchestratorBackend() if (m_client) { m_client->setLocoStateCallback(nullptr); m_client->setConnectionStateCallback(nullptr); + m_client->setTrackPowerCallback(nullptr); } if (m_mutex) { vSemaphoreDelete(m_mutex); @@ -379,6 +380,59 @@ void OrchestratorBackend::setConnectionStateCallback(ConnectionStateCallback cal }); } +esp_err_t OrchestratorBackend::setTrackPower(bool on) +{ + if (!m_client) { + return ESP_ERR_NOT_SUPPORTED; + } + // Blocking HTTP. The caller is responsible for not being the LVGL task. + return m_client->setTrackPower(on); +} + +ThrottleBackend::TrackPower OrchestratorBackend::getTrackPower() const +{ + if (!m_client) { + return TrackPower::UNKNOWN; + } + switch (m_client->getTrackPower()) { + case OrchestratorClient::TrackPower::ON: return TrackPower::ON; + case OrchestratorClient::TrackPower::OFF: return TrackPower::OFF; + default: return TrackPower::UNKNOWN; + } +} + +void OrchestratorBackend::setTrackPowerCallback(TrackPowerCallback callback) +{ + m_trackPowerCallback = std::move(callback); + + if (!m_client) { + return; + } + + if (!m_trackPowerCallback) { + m_client->setTrackPowerCallback(nullptr); + return; + } + + m_client->setTrackPowerCallback( + [this](OrchestratorClient::TrackPower state) { + if (!m_trackPowerCallback) { + return; + } + switch (state) { + case OrchestratorClient::TrackPower::ON: + m_trackPowerCallback(TrackPower::ON); + break; + case OrchestratorClient::TrackPower::OFF: + m_trackPowerCallback(TrackPower::OFF); + break; + default: + m_trackPowerCallback(TrackPower::UNKNOWN); + break; + } + }); +} + void OrchestratorBackend::setFunctionLabelsCallback(FunctionLabelsCallback /*callback*/) { // Never invoked: providesFunctionLabels() is false, so ThrottleController diff --git a/main/communication/OrchestratorBackend.h b/main/communication/OrchestratorBackend.h index 69029ed..83746df 100644 --- a/main/communication/OrchestratorBackend.h +++ b/main/communication/OrchestratorBackend.h @@ -59,6 +59,11 @@ class OrchestratorBackend : public ThrottleBackend { void setFunctionLabelsCallback(FunctionLabelsCallback callback) override; void setConnectionStateCallback(ConnectionStateCallback callback) override; + bool supportsTrackPower() const override { return m_client != nullptr; } + esp_err_t setTrackPower(bool on) override; + TrackPower getTrackPower() const override; + void setTrackPowerCallback(TrackPowerCallback callback) override; + private: /** * @brief What this device currently has on each throttle. @@ -91,6 +96,7 @@ class OrchestratorBackend : public ThrottleBackend { ThrottleStateCallback m_throttleStateCallback; ConnectionStateCallback m_connectionStateCallback; + TrackPowerCallback m_trackPowerCallback; mutable SemaphoreHandle_t m_mutex; }; diff --git a/main/communication/OrchestratorClient.cpp b/main/communication/OrchestratorClient.cpp index ed3256e..e6cd0f5 100644 --- a/main/communication/OrchestratorClient.cpp +++ b/main/communication/OrchestratorClient.cpp @@ -123,6 +123,7 @@ OrchestratorClient::OrchestratorClient() , m_state(ConnectionState::DISCONNECTED) , m_port(DEFAULT_PORT) , m_systemStatus(SystemStatus::UNKNOWN) + , m_trackPower(TrackPower::UNKNOWN) , m_lastMessageUs(0) , m_stateMutex(nullptr) { @@ -392,13 +393,14 @@ esp_err_t OrchestratorClient::openSocket(const std::string& host, uint16_t port) cfg.ping_interval_sec = 20; cfg.disable_auto_reconnect = false; cfg.task_stack = 6144; - // Also sizes the handshake buffer, which is what forced this up: the - // orchestrator re-issues the session cookie on every response (sliding - // expiry), so the upgrade's 101 carries a Set-Cookie on top of the usual - // headers and 4 KB overflowed with "Header size exceeded buffer size". - // It cost the first connection attempt outright and only recovered on the - // 10 s auto-reconnect. - cfg.buffer_size = 8192; + // Frame buffer only. It does NOT size the HTTP Upgrade handshake -- that + // is CONFIG_WS_BUFFER_SIZE in sdkconfig.defaults, and it is what + // "transport_ws: Header size exceeded buffer size" is complaining about. + // Raising this instead does nothing for the handshake. + // + // A whole-layout STATE_SNAPSHOT is larger than this and arrives split + // across several events; m_rxBuffer reassembles it. + cfg.buffer_size = 4096; m_client = esp_websocket_client_init(&cfg); if (!m_client) { @@ -544,6 +546,8 @@ void OrchestratorClient::handleMessage(const std::string& json) handleLocoState(payload); } else if (type == "SYSTEM_STATUS") { handleSystemStatus(payload); + } else if (type == "DCC_LINK") { + handleDccLink(payload); } else if (type == "ERROR") { std::string message; if (payload && jsonString(payload, "message", message)) { @@ -591,6 +595,13 @@ void OrchestratorClient::handleStateSnapshot(const void* payloadPtr) } } + // Track power rides in on the snapshot too, so the button is right from + // the first frame rather than waiting for the next DCC_LINK. + const cJSON* dccLink = cJSON_GetObjectItemCaseSensitive(payload, "dccLink"); + if (cJSON_IsObject(dccLink)) { + handleDccLink(dccLink); + } + // `locos` is an object keyed by address. This is the layout's belief about // what is already moving -- it updates what we display, and is never // replayed outward as a command. @@ -672,6 +683,199 @@ void OrchestratorClient::handleLocoState(const void* payloadPtr) } } +void OrchestratorClient::handleDccLink(const void* payloadPtr) +{ + const cJSON* payload = static_cast(payloadPtr); + if (!cJSON_IsObject(payload)) { + ESP_LOGW(TAG, "Refusing DCC_LINK with no payload object"); + return; + } + + // `mainPowerOn` is boolean-or-null by contract: null means the command + // station has not said. That is UNKNOWN, not off -- reporting it as off + // would tell the operator the rails are dead when nobody knows. + const cJSON* mainPower = cJSON_GetObjectItemCaseSensitive(payload, "mainPowerOn"); + + TrackPower power = TrackPower::UNKNOWN; + if (cJSON_IsBool(mainPower)) { + power = cJSON_IsTrue(mainPower) ? TrackPower::ON : TrackPower::OFF; + } else if (!cJSON_IsNull(mainPower) && mainPower != nullptr) { + ESP_LOGW(TAG, "Refusing DCC_LINK: mainPowerOn is neither boolean nor null"); + return; + } + + TrackPowerCallback callback; + bool changed = false; + if (lockState(pdMS_TO_TICKS(50))) { + changed = (m_trackPower != power); + m_trackPower = power; + callback = m_trackPowerCallback; + unlockState(); + } else { + changed = (m_trackPower != power); + m_trackPower = power; + callback = m_trackPowerCallback; + } + + if (changed) { + ESP_LOGI(TAG, "Track power: %s", + power == TrackPower::ON ? "on" + : power == TrackPower::OFF ? "off" : "unknown"); + if (callback) { + callback(power); + } + } +} + +OrchestratorClient::TrackPower OrchestratorClient::getTrackPower() const +{ + TrackPower power = TrackPower::UNKNOWN; + if (lockState(pdMS_TO_TICKS(50))) { + power = m_trackPower; + unlockState(); + } + return power; +} + +esp_err_t OrchestratorClient::setTrackPower(bool on) +{ + std::string token; + std::string host; + uint16_t port = DEFAULT_PORT; + if (lockState(pdMS_TO_TICKS(200))) { + token = m_sessionToken; + host = m_host; + port = m_port; + unlockState(); + } + + if (token.empty() || host.empty()) { + ESP_LOGW(TAG, "Cannot set track power before authenticating"); + return ESP_ERR_INVALID_STATE; + } + + const std::string layoutId = getLayoutId(); + if (layoutId.empty()) { + ESP_LOGE(TAG, "Cannot set track power: no layout id"); + return ESP_ERR_INVALID_STATE; + } + + const std::string url = "http://" + host + ":" + std::to_string(port) + + "/api/layouts/" + layoutId + "/dcc-link/power"; + const std::string cookie = std::string(SESSION_COOKIE_NAME) + "=" + token; + const std::string body = on ? "{\"on\":true}" : "{\"on\":false}"; + + HttpCapture capture; + esp_http_client_config_t cfg = {}; + cfg.url = url.c_str(); + cfg.method = HTTP_METHOD_POST; + cfg.timeout_ms = HTTP_TIMEOUT_MS; + cfg.event_handler = httpEventHandler; + cfg.user_data = &capture; + + esp_http_client_handle_t client = esp_http_client_init(&cfg); + if (!client) { + return ESP_FAIL; + } + esp_http_client_set_header(client, "Content-Type", "application/json"); + esp_http_client_set_header(client, "Cookie", cookie.c_str()); + esp_http_client_set_post_field(client, body.c_str(), static_cast(body.size())); + + esp_err_t err = esp_http_client_perform(client); + const int status = esp_http_client_get_status_code(client); + esp_http_client_cleanup(client); + + if (err != ESP_OK) { + ESP_LOGE(TAG, "Track power request failed: %s", esp_err_to_name(err)); + return err; + } + if (status == 403) { + ESP_LOGE(TAG, "Track power refused: this credential may not drive"); + return ESP_ERR_INVALID_STATE; + } + if (status != 200) { + ESP_LOGE(TAG, "Track power returned HTTP %d", status); + return ESP_FAIL; + } + + // The reply body is deliberately ignored. The DCC_LINK event pushed the + // moment this lands is what tells us the truth, and it arrives on the + // socket like any other state change. + ESP_LOGI(TAG, "Track power %s requested", on ? "on" : "off"); + return ESP_OK; +} + +std::string OrchestratorClient::getLayoutId() +{ + if (lockState(pdMS_TO_TICKS(200))) { + if (!m_layoutId.empty()) { + const std::string cached = m_layoutId; + unlockState(); + return cached; + } + unlockState(); + } + + std::string token; + std::string host; + uint16_t port = DEFAULT_PORT; + if (lockState(pdMS_TO_TICKS(200))) { + token = m_sessionToken; + host = m_host; + port = m_port; + unlockState(); + } + if (token.empty() || host.empty()) { + return std::string(); + } + + const std::string url = "http://" + host + ":" + std::to_string(port) + "/api/layouts"; + const std::string cookie = std::string(SESSION_COOKIE_NAME) + "=" + token; + + HttpCapture capture; + esp_http_client_config_t cfg = {}; + cfg.url = url.c_str(); + cfg.method = HTTP_METHOD_GET; + cfg.timeout_ms = HTTP_TIMEOUT_MS; + cfg.event_handler = httpEventHandler; + cfg.user_data = &capture; + + esp_http_client_handle_t client = esp_http_client_init(&cfg); + if (!client) { + return std::string(); + } + esp_http_client_set_header(client, "Cookie", cookie.c_str()); + esp_err_t err = esp_http_client_perform(client); + const int status = esp_http_client_get_status_code(client); + esp_http_client_cleanup(client); + + if (err != ESP_OK || status != 200) { + ESP_LOGE(TAG, "Failed to list layouts (HTTP %d)", status); + return std::string(); + } + + cJSON* layouts = cJSON_Parse(capture.body.c_str()); + if (!cJSON_IsArray(layouts) || cJSON_GetArraySize(layouts) == 0) { + ESP_LOGE(TAG, "Refusing layout list: not a non-empty array"); + if (layouts) cJSON_Delete(layouts); + return std::string(); + } + + std::string layoutId; + const bool haveId = jsonString(cJSON_GetArrayItem(layouts, 0), "id", layoutId); + cJSON_Delete(layouts); + if (!haveId || layoutId.empty()) { + ESP_LOGE(TAG, "Refusing layout list: first entry has no id"); + return std::string(); + } + + if (lockState(pdMS_TO_TICKS(200))) { + m_layoutId = layoutId; + unlockState(); + } + return layoutId; +} + void OrchestratorClient::handleSystemStatus(const void* payloadPtr) { const cJSON* payload = static_cast(payloadPtr); @@ -834,45 +1038,16 @@ esp_err_t OrchestratorClient::refreshRoster() const std::string cookie = std::string(SESSION_COOKIE_NAME) + "=" + token; // The control plane carries loco state keyed by address but no names, so - // the roster is a REST read. It needs a layout id, and the layout list is - // where that comes from. - HttpCapture layoutsCapture; - esp_http_client_config_t layoutsCfg = {}; - const std::string layoutsUrl = base + "/api/layouts"; - layoutsCfg.url = layoutsUrl.c_str(); - layoutsCfg.method = HTTP_METHOD_GET; - layoutsCfg.timeout_ms = HTTP_TIMEOUT_MS; - layoutsCfg.event_handler = httpEventHandler; - layoutsCfg.user_data = &layoutsCapture; - - esp_http_client_handle_t client = esp_http_client_init(&layoutsCfg); - if (!client) { - return ESP_FAIL; - } - esp_http_client_set_header(client, "Cookie", cookie.c_str()); - esp_err_t err = esp_http_client_perform(client); - int status = esp_http_client_get_status_code(client); - esp_http_client_cleanup(client); - - if (err != ESP_OK || status != 200) { - ESP_LOGE(TAG, "Failed to list layouts (HTTP %d)", status); - return (err != ESP_OK) ? err : ESP_FAIL; - } - - cJSON* layouts = cJSON_Parse(layoutsCapture.body.c_str()); - if (!cJSON_IsArray(layouts) || cJSON_GetArraySize(layouts) == 0) { - ESP_LOGE(TAG, "Refusing layout list: not a non-empty array"); - if (layouts) cJSON_Delete(layouts); + // the roster is a REST read. It needs a layout id, which getLayoutId() + // fetches once and caches -- the power POST needs the same id. + const std::string layoutId = getLayoutId(); + if (layoutId.empty()) { return ESP_ERR_INVALID_RESPONSE; } - std::string layoutId; - const bool haveId = jsonString(cJSON_GetArrayItem(layouts, 0), "id", layoutId); - cJSON_Delete(layouts); - if (!haveId || layoutId.empty()) { - ESP_LOGE(TAG, "Refusing layout list: first entry has no id"); - return ESP_ERR_INVALID_RESPONSE; - } + esp_http_client_handle_t client = nullptr; + esp_err_t err = ESP_OK; + int status = 0; HttpCapture locosCapture; const std::string locosUrl = base + "/api/layouts/" + layoutId + "/locos"; @@ -1004,6 +1179,16 @@ void OrchestratorClient::setRosterCallback(RosterCallback callback) } } +void OrchestratorClient::setTrackPowerCallback(TrackPowerCallback callback) +{ + if (lockState(pdMS_TO_TICKS(100))) { + m_trackPowerCallback = std::move(callback); + unlockState(); + } else { + m_trackPowerCallback = std::move(callback); + } +} + const char* OrchestratorClient::stateName(ConnectionState state) { switch (state) { diff --git a/main/communication/OrchestratorClient.h b/main/communication/OrchestratorClient.h index 3affb5b..9cf0958 100644 --- a/main/communication/OrchestratorClient.h +++ b/main/communication/OrchestratorClient.h @@ -56,6 +56,13 @@ class OrchestratorClient { STOP }; + /** Main-track power, from `DccLinkView.mainPowerOn` (null = unknown). */ + enum class TrackPower { + OFF, + ON, + UNKNOWN + }; + /** Mirrors SystemStatus: 'online' | 'safe-stop' | 'offline'. */ enum class SystemStatus { ONLINE, @@ -88,6 +95,7 @@ class OrchestratorClient { using LocoStateCallback = std::function; using SystemStatusCallback = std::function; using RosterCallback = std::function& roster)>; + using TrackPowerCallback = std::function; OrchestratorClient(); ~OrchestratorClient(); @@ -138,6 +146,23 @@ class OrchestratorClient { */ esp_err_t sendEmergencyStop(); + // --- Track power ------------------------------------------------------ + + /** + * @brief Turn main-track power on or off. + * + * `POST /api/layouts/{id}/dcc-link/power` — a REST call, because the + * control plane's `ClientMessage` union has no track-power member. The + * reply body is a courtesy; what tells us the truth is the `DCC_LINK` + * event pushed the moment it lands, so this does not parse the response. + * + * Blocking HTTP: never call from an LVGL event handler (F-05). + */ + esp_err_t setTrackPower(bool on); + + /** UNKNOWN until a DCC_LINK or snapshot has said otherwise. */ + TrackPower getTrackPower() const; + // --- Roster (REST, not WebSocket) ------------------------------------- /** @@ -157,6 +182,7 @@ class OrchestratorClient { void setLocoStateCallback(LocoStateCallback callback); void setSystemStatusCallback(SystemStatusCallback callback); void setRosterCallback(RosterCallback callback); + void setTrackPowerCallback(TrackPowerCallback callback); /** Seconds since the last message of any kind. Large means a stale link. */ uint32_t secondsSinceLastMessage() const; @@ -204,6 +230,10 @@ class OrchestratorClient { void handleStateSnapshot(const void* payload); void handleLocoState(const void* payload); void handleSystemStatus(const void* payload); + void handleDccLink(const void* payload); + + /** Cached so the roster fetch and the power POST can both reach it. */ + std::string getLayoutId(); esp_err_t sendJson(const std::string& json); @@ -226,6 +256,8 @@ class OrchestratorClient { std::string m_rxBuffer; SystemStatus m_systemStatus; + TrackPower m_trackPower; + std::string m_layoutId; std::vector m_roster; int64_t m_lastMessageUs; @@ -233,6 +265,7 @@ class OrchestratorClient { LocoStateCallback m_locoStateCallback; SystemStatusCallback m_systemStatusCallback; RosterCallback m_rosterCallback; + TrackPowerCallback m_trackPowerCallback; mutable SemaphoreHandle_t m_stateMutex; }; diff --git a/main/communication/ThrottleBackend.h b/main/communication/ThrottleBackend.h index 8537979..6207640 100644 --- a/main/communication/ThrottleBackend.h +++ b/main/communication/ThrottleBackend.h @@ -69,7 +69,20 @@ class ThrottleBackend { bool functionState = false; }; + /** + * @brief Track power, as the layout reports it. + * + * UNKNOWN is not "off": it means nothing has told us yet. The UI must show + * it as its own state rather than implying the rails are dead. + */ + enum class TrackPower { + OFF, + ON, + UNKNOWN + }; + using ConnectionStateCallback = std::function; + using TrackPowerCallback = std::function; using ThrottleStateCallback = std::function; using FunctionLabelsCallback = std::function& labels)>; @@ -181,4 +194,17 @@ class ThrottleBackend { * client the UI happens to know about. */ virtual void setConnectionStateCallback(ConnectionStateCallback callback) = 0; + + // --- Track power ------------------------------------------------------- + // + // Strictly this is a layout command rather than a throttle one, but it + // rides the same connection and the UI needs one place to ask. Keeping it + // here rather than inventing a second port with two more adapters. + + /** False means the UI must not offer a power button at all. */ + virtual bool supportsTrackPower() const = 0; + + virtual esp_err_t setTrackPower(bool on) = 0; + virtual TrackPower getTrackPower() const = 0; + virtual void setTrackPowerCallback(TrackPowerCallback callback) = 0; }; diff --git a/main/communication/WiThrottleBackend.cpp b/main/communication/WiThrottleBackend.cpp index 902773f..3f0c5be 100644 --- a/main/communication/WiThrottleBackend.cpp +++ b/main/communication/WiThrottleBackend.cpp @@ -25,12 +25,69 @@ ThrottleBackend::ConnectionState toPortState(WiThrottleClient::ConnectionState s } // namespace -WiThrottleBackend::WiThrottleBackend(WiThrottleClient* client) +WiThrottleBackend::WiThrottleBackend(WiThrottleClient* client, JmriJsonClient* jsonClient) : m_client(client) + , m_jsonClient(jsonClient) { if (!m_client) { ESP_LOGE(TAG, "Constructed with a null WiThrottleClient; every command will be refused"); } + if (!m_jsonClient) { + ESP_LOGW(TAG, "No JSON client; track power will be unavailable"); + } +} + +esp_err_t WiThrottleBackend::setTrackPower(bool on) +{ + if (!m_jsonClient) { + return ESP_ERR_NOT_SUPPORTED; + } + return m_jsonClient->setPower(on); +} + +ThrottleBackend::TrackPower WiThrottleBackend::getTrackPower() const +{ + if (!m_jsonClient) { + return TrackPower::UNKNOWN; + } + + switch (m_jsonClient->getPower()) { + case JmriJsonClient::PowerState::ON: return TrackPower::ON; + case JmriJsonClient::PowerState::OFF: return TrackPower::OFF; + default: return TrackPower::UNKNOWN; + } +} + +void WiThrottleBackend::setTrackPowerCallback(TrackPowerCallback callback) +{ + m_trackPowerCallback = std::move(callback); + + if (!m_jsonClient) { + return; + } + + if (!m_trackPowerCallback) { + m_jsonClient->setPowerStateCallback(nullptr); + return; + } + + m_jsonClient->setPowerStateCallback( + [this](const std::string& /*powerName*/, JmriJsonClient::PowerState state) { + if (!m_trackPowerCallback) { + return; + } + switch (state) { + case JmriJsonClient::PowerState::ON: + m_trackPowerCallback(TrackPower::ON); + break; + case JmriJsonClient::PowerState::OFF: + m_trackPowerCallback(TrackPower::OFF); + break; + default: + m_trackPowerCallback(TrackPower::UNKNOWN); + break; + } + }); } bool WiThrottleBackend::isConnected() const diff --git a/main/communication/WiThrottleBackend.h b/main/communication/WiThrottleBackend.h index 0e2dbea..3dacc32 100644 --- a/main/communication/WiThrottleBackend.h +++ b/main/communication/WiThrottleBackend.h @@ -1,5 +1,6 @@ #pragma once +#include "JmriJsonClient.h" #include "ThrottleBackend.h" #include "WiThrottleClient.h" @@ -15,7 +16,16 @@ */ class WiThrottleBackend : public ThrottleBackend { public: - explicit WiThrottleBackend(WiThrottleClient* client); + /** + * @param client WiThrottle client. Not owned; must outlive this adapter. + * @param jsonClient JMRI JSON client, used **only** for track power. + * Under JMRI, power goes over the JSON API and not the WiThrottle + * `PPA` command -- that is what the power button has always done, + * and routing it through WiThrottle here would quietly change + * behaviour for every existing JMRI user. May be null, in which + * case supportsTrackPower() is false. + */ + WiThrottleBackend(WiThrottleClient* client, JmriJsonClient* jsonClient); ~WiThrottleBackend() override = default; WiThrottleBackend(const WiThrottleBackend&) = delete; @@ -45,6 +55,11 @@ class WiThrottleBackend : public ThrottleBackend { void setFunctionLabelsCallback(FunctionLabelsCallback callback) override; void setConnectionStateCallback(ConnectionStateCallback callback) override; + bool supportsTrackPower() const override { return m_jsonClient != nullptr; } + esp_err_t setTrackPower(bool on) override; + TrackPower getTrackPower() const override; + void setTrackPowerCallback(TrackPowerCallback callback) override; + private: /** Throttle index to WiThrottle's character id, or 0 when out of range. * Callers check validity with isValidThrottle first. */ @@ -54,10 +69,12 @@ class WiThrottleBackend : public ThrottleBackend { } WiThrottleClient* m_client; + JmriJsonClient* m_jsonClient; // Held so the lambdas registered on the client stay valid, and so a second // setThrottleStateCallback replaces the first rather than stacking. ThrottleStateCallback m_throttleStateCallback; FunctionLabelsCallback m_functionLabelsCallback; ConnectionStateCallback m_connectionStateCallback; + TrackPowerCallback m_trackPowerCallback; }; diff --git a/main/controller/AppController.cpp b/main/controller/AppController.cpp index 67b83dd..f0834bf 100644 --- a/main/controller/AppController.cpp +++ b/main/controller/AppController.cpp @@ -95,7 +95,10 @@ void AppController::initialise() m_throttleBackend = std::make_unique(m_orchestratorClient.get()); startOrchestratorConnectTask(); } else { - m_throttleBackend = std::make_unique(m_wiThrottleClient.get()); + // The JSON client comes in for track power only: under JMRI that + // has always gone over the JSON API rather than WiThrottle's PPA. + m_throttleBackend = std::make_unique(m_wiThrottleClient.get(), + m_jmriClient.get()); } ESP_LOGI(TAG, "Throttle transport: %s", diff --git a/main/controller/ThrottleController.cpp b/main/controller/ThrottleController.cpp index 34a1f97..98cf129 100644 --- a/main/controller/ThrottleController.cpp +++ b/main/controller/ThrottleController.cpp @@ -18,6 +18,8 @@ ThrottleController::ThrottleController(ThrottleBackend* backend) , m_stateMutex(nullptr) , m_uiUpdateCallback(nullptr) , m_uiUpdateUserData(nullptr) + , m_trackPowerCallback(nullptr) + , m_trackPowerUserData(nullptr) , m_pollingTask(nullptr) , m_pollingRunning(false) , m_cachedSpeedSteps(4) @@ -46,6 +48,13 @@ ThrottleController::ThrottleController(ThrottleBackend* backend) this->updateUI(); } ); + m_backend->setTrackPowerCallback( + [this](ThrottleBackend::TrackPower state) { + if (m_trackPowerCallback) { + m_trackPowerCallback(m_trackPowerUserData, state); + } + } + ); if (m_backend->providesFunctionLabels()) { m_backend->setFunctionLabelsCallback( [this](int throttleId, const std::vector& labels) { @@ -373,6 +382,53 @@ Knob* ThrottleController::getKnob(int knobId) } #endif +bool ThrottleController::supportsTrackPower() const +{ + return m_backend && m_backend->supportsTrackPower(); +} + +ThrottleBackend::TrackPower ThrottleController::getTrackPower() const +{ + if (!m_backend) { + return ThrottleBackend::TrackPower::UNKNOWN; + } + return m_backend->getTrackPower(); +} + +void ThrottleController::setTrackPowerCallback( + void (*callback)(void*, ThrottleBackend::TrackPower), void* userData) +{ + m_trackPowerCallback = callback; + m_trackPowerUserData = userData; +} + +void ThrottleController::trackPowerTaskFunc(void* arg) +{ + auto* request = static_cast(arg); + if (request && request->controller && request->controller->m_backend) { + request->controller->m_backend->setTrackPower(request->on); + } + delete request; + vTaskDelete(nullptr); +} + +void ThrottleController::requestTrackPower(bool on) +{ + if (!m_backend || !m_backend->supportsTrackPower()) { + ESP_LOGW(TAG, "Track power not supported by the active transport"); + return; + } + + // On its own task: the orchestrator's power command is a blocking HTTP + // round trip, and this is called straight from an LVGL button handler. + // 4 KB covers the HTTP client. + auto* request = new TrackPowerRequest{this, on}; + if (xTaskCreate(trackPowerTaskFunc, "track_power", 4096, request, 5, nullptr) != pdPASS) { + ESP_LOGE(TAG, "Failed to create track power task"); + delete request; + } +} + bool ThrottleController::isConnected() const { return m_backend && m_backend->isConnected(); diff --git a/main/controller/ThrottleController.h b/main/controller/ThrottleController.h index 3b4ba51..f4fb37c 100644 --- a/main/controller/ThrottleController.h +++ b/main/controller/ThrottleController.h @@ -148,6 +148,25 @@ class ThrottleController */ esp_err_t setFunction(int throttleId, int functionNumber, bool state); + /** Whether the active transport offers a track-power control at all. */ + bool supportsTrackPower() const; + + ThrottleBackend::TrackPower getTrackPower() const; + + /** + * @brief Request a track-power change. + * + * Returns immediately: the orchestrator's power command is a blocking HTTP + * round trip, so the write happens on a short-lived task. The button must + * never wait for it (F-05). The real state arrives through the power + * callback, so the UI is driven by what the layout says, not by what we + * asked for. + */ + void requestTrackPower(bool on); + + /** Called on any track-power change. Registered by the UI. */ + void setTrackPowerCallback(void (*callback)(void*, ThrottleBackend::TrackPower), void* userData); + /** * @brief Get current roster size */ @@ -193,6 +212,12 @@ class ThrottleController void onFunctionLabelsReceived(int throttleId, const std::vector& labels); + struct TrackPowerRequest { + ThrottleController* controller; + bool on; + }; + static void trackPowerTaskFunc(void* arg); + // Polling for state synchronization void pollThrottleStates(); static void pollingTaskFunc(void* arg); @@ -207,6 +232,9 @@ class ThrottleController void (*m_uiUpdateCallback)(void*); void* m_uiUpdateUserData; + + void (*m_trackPowerCallback)(void*, ThrottleBackend::TrackPower); + void* m_trackPowerUserData; TaskHandle_t m_pollingTask; bool m_pollingRunning; diff --git a/main/tests/ThrottleControllerTests.cpp b/main/tests/ThrottleControllerTests.cpp index 489e65e..33375a8 100644 --- a/main/tests/ThrottleControllerTests.cpp +++ b/main/tests/ThrottleControllerTests.cpp @@ -102,6 +102,24 @@ namespace { connectionStateCallback = std::move(callback); } + bool supportsTrackPower() const override { return trackPowerSupported; } + esp_err_t setTrackPower(bool on) override { + trackPowerWrites.push_back(on); + return ESP_OK; + } + TrackPower getTrackPower() const override { return trackPower; } + void setTrackPowerCallback(TrackPowerCallback callback) override { + trackPowerCallback = std::move(callback); + } + + /** Reports a power change, as the layout would. */ + void emitTrackPower(TrackPower state) { + trackPower = state; + if (trackPowerCallback) { + trackPowerCallback(state); + } + } + /** Drives a link up/down event, as a transport would. */ void emitConnectionState(ConnectionState state) { connected = (state == ConnectionState::CONNECTED); @@ -121,6 +139,11 @@ namespace { ThrottleStateCallback throttleStateCallback; FunctionLabelsCallback functionLabelsCallback; ConnectionStateCallback connectionStateCallback; + + bool trackPowerSupported = true; + TrackPower trackPower = TrackPower::UNKNOWN; + std::vector trackPowerWrites; + TrackPowerCallback trackPowerCallback; }; void uiUpdateCallback(void* userData) @@ -461,6 +484,64 @@ static void test_controller_pairs_speed_and_direction_on_flip(void) TEST_ASSERT_FALSE(backend.pairedCommands[0].boolArg); } +namespace { + struct PowerCapture { + int calls = 0; + ThrottleBackend::TrackPower last = ThrottleBackend::TrackPower::UNKNOWN; + }; + + void powerCallback(void* userData, ThrottleBackend::TrackPower state) + { + auto* capture = static_cast(userData); + if (capture) { + capture->calls++; + capture->last = state; + } + } +} + +static void test_controller_reports_track_power_from_backend(void) +{ + FakeThrottleBackend backend; + ThrottleController controller(&backend); + + // The power bar read JmriJsonClient directly, so under the orchestrator + // transport the button did nothing and the label lied. + TEST_ASSERT_TRUE(controller.supportsTrackPower()); + TEST_ASSERT_TRUE(controller.getTrackPower() == ThrottleBackend::TrackPower::UNKNOWN); + + backend.trackPower = ThrottleBackend::TrackPower::ON; + TEST_ASSERT_TRUE(controller.getTrackPower() == ThrottleBackend::TrackPower::ON); +} + +static void test_controller_forwards_track_power_changes(void) +{ + FakeThrottleBackend backend; + ThrottleController controller(&backend); + + PowerCapture capture; + controller.setTrackPowerCallback(powerCallback, &capture); + + backend.emitTrackPower(ThrottleBackend::TrackPower::ON); + + TEST_ASSERT_EQUAL_INT(1, capture.calls); + TEST_ASSERT_TRUE(capture.last == ThrottleBackend::TrackPower::ON); +} + +static void test_controller_hides_track_power_when_unsupported(void) +{ + FakeThrottleBackend backend; + backend.trackPowerSupported = false; + ThrottleController controller(&backend); + + TEST_ASSERT_FALSE(controller.supportsTrackPower()); + + // Refused rather than sent: a transport with no power control must not + // have one faked on its behalf. + controller.requestTrackPower(true); + TEST_ASSERT_EQUAL_INT(0, (int)backend.trackPowerWrites.size()); +} + extern "C" void register_controller_tests(void) { RUN_TEST(test_controller_assign_knob_to_unallocated); @@ -479,4 +560,7 @@ extern "C" void register_controller_tests(void) RUN_TEST(test_controller_repaints_on_connection_change); RUN_TEST(test_controller_function_reaches_backend); RUN_TEST(test_controller_pairs_speed_and_direction_on_flip); + RUN_TEST(test_controller_reports_track_power_from_backend); + RUN_TEST(test_controller_forwards_track_power_changes); + RUN_TEST(test_controller_hides_track_power_when_unsupported); } diff --git a/main/ui/MainScreen.cpp b/main/ui/MainScreen.cpp index 93ca9fe..289952f 100644 --- a/main/ui/MainScreen.cpp +++ b/main/ui/MainScreen.cpp @@ -128,7 +128,7 @@ void MainScreen::createRightPanel() // Add track power controls at the top m_powerStatusBar = std::make_unique(); - m_powerStatusBar->create(m_rightPanel, m_jmriClient); + m_powerStatusBar->create(m_rightPanel, m_throttleController); // Roster selection carousel m_rosterCarousel = std::make_unique(); @@ -273,6 +273,12 @@ void MainScreen::updateThrottle(int throttleId) // every knob dead. bool transportConnected = m_throttleController && m_throttleController->isConnected(); + // Repainted here too, so the status label follows the same link state the + // knobs do rather than a separate client's. + if (m_powerStatusBar) { + m_powerStatusBar->refresh(); + } + // Hide function panel when entering roster selection if (snapshot.state == Throttle::State::SELECTING && m_functionPanel && m_functionPanel->isVisible()) { m_functionPanel->hide(); diff --git a/main/ui/components/PowerStatusBar.cpp b/main/ui/components/PowerStatusBar.cpp index 460cbda..d1a5964 100644 --- a/main/ui/components/PowerStatusBar.cpp +++ b/main/ui/components/PowerStatusBar.cpp @@ -1,4 +1,8 @@ #include "PowerStatusBar.h" + +#include + +#include "controller/ThrottleController.h" #include "esp_log.h" #include "lvgl_port.h" @@ -13,21 +17,20 @@ PowerStatusBar::PowerStatusBar() : m_container(nullptr) , m_trackPowerButton(nullptr) , m_connectionStatusLabel(nullptr) - , m_jmriClient(nullptr) + , m_throttleController(nullptr) { } PowerStatusBar::~PowerStatusBar() { - if (m_jmriClient) { - m_jmriClient->setPowerStateCallback(nullptr); - m_jmriClient->setConnectionStateCallback(nullptr); + if (m_throttleController) { + m_throttleController->setTrackPowerCallback(nullptr, nullptr); } } -lv_obj_t* PowerStatusBar::create(lv_obj_t* parent, JmriJsonClient* jmriClient) +lv_obj_t* PowerStatusBar::create(lv_obj_t* parent, ThrottleController* throttleController) { - m_jmriClient = jmriClient; + m_throttleController = throttleController; m_container = lv_obj_create(parent); lv_obj_set_size(m_container, LV_PCT(90), 50); @@ -49,46 +52,72 @@ lv_obj_t* PowerStatusBar::create(lv_obj_t* parent, JmriJsonClient* jmriClient) lv_obj_set_style_text_color(m_connectionStatusLabel, lv_color_hex(0xFF0000), 0); lv_obj_center(m_connectionStatusLabel); - if (m_jmriClient) { - updateTrackPowerButton(m_jmriClient->getPower()); - updateConnectionStatus(m_jmriClient->getState()); - - m_jmriClient->setPowerStateCallback([this](const std::string& powerName, JmriJsonClient::PowerState state) { - (void)powerName; - if (lvgl_port_lock(200)) { - updateTrackPowerButton(state); - lvgl_port_unlock(); - } - }); - - m_jmriClient->setConnectionStateCallback([this](JmriJsonClient::ConnectionState state) { - if (lvgl_port_lock(200)) { - updateConnectionStatus(state); - lvgl_port_unlock(); - } - }); + if (m_throttleController) { + // A transport with no power control gets no button at all, rather than + // a dead one the operator can press and wonder about. + if (!m_throttleController->supportsTrackPower()) { + lv_obj_add_flag(m_trackPowerButton, LV_OBJ_FLAG_HIDDEN); + } + + m_throttleController->setTrackPowerCallback(onTrackPowerChanged, this); + refresh(); } ESP_LOGI(TAG, "Power/status bar created"); return m_container; } +void PowerStatusBar::refresh() +{ + if (!m_throttleController) { + return; + } + updateTrackPowerButton(m_throttleController->getTrackPower()); + updateConnectionStatus(m_throttleController->isConnected()); +} + +void PowerStatusBar::onTrackPowerChanged(void* userData, ThrottleBackend::TrackPower state) +{ + auto* bar = static_cast(userData); + if (!bar) { + return; + } + // Arrives on a transport task, so the LVGL lock is required. 200 ms and + // skip on contention -- a missed repaint is corrected by the next event. + if (lvgl_port_lock(200)) { + bar->updateTrackPowerButton(state); + lvgl_port_unlock(); + } +} + void PowerStatusBar::onTrackPowerClicked(lv_event_t* e) { auto* bar = static_cast(lv_event_get_user_data(e)); - if (!bar || !bar->m_jmriClient || !bar->m_jmriClient->isConnected()) { - ESP_LOGW(TAG, "Not connected to JMRI server"); + if (!bar || !bar->m_throttleController) { return; } - auto currentState = bar->m_jmriClient->getPower(); - bool newState = (currentState != JmriJsonClient::PowerState::ON); + if (!bar->m_throttleController->isConnected()) { + ESP_LOGW(TAG, "Not connected; ignoring track power press"); + return; + } + + // UNKNOWN turns power on: the useful thing to do when nobody has said what + // the rails are doing is to energise them, and the operator can press + // again to turn it off once the state is known. + const ThrottleBackend::TrackPower current = bar->m_throttleController->getTrackPower(); + const bool newState = (current != ThrottleBackend::TrackPower::ON); ESP_LOGI(TAG, "Toggling track power: %s", newState ? "ON" : "OFF"); - bar->m_jmriClient->setPower(newState); + + // Returns immediately; the write happens on its own task because the + // orchestrator's power command is a blocking HTTP round trip and this is + // an LVGL event handler (F-05). The button repaints when the layout says + // it changed, not when we asked. + bar->m_throttleController->requestTrackPower(newState); } -void PowerStatusBar::updateTrackPowerButton(JmriJsonClient::PowerState state) +void PowerStatusBar::updateTrackPowerButton(ThrottleBackend::TrackPower state) { if (!m_trackPowerButton) return; @@ -99,17 +128,19 @@ void PowerStatusBar::updateTrackPowerButton(JmriJsonClient::PowerState state) const char* stateText; switch (state) { - case JmriJsonClient::PowerState::ON: + case ThrottleBackend::TrackPower::ON: color = 0x00AA00; - stateText = "Power"; + stateText = "Power On"; break; - case JmriJsonClient::PowerState::OFF: + case ThrottleBackend::TrackPower::OFF: color = 0xAA0000; - stateText = "Power"; + stateText = "Power Off"; break; default: + // Not "off": nothing has told us yet, and showing it as off would + // claim the rails are dead when nobody knows. color = 0x888888; - stateText = "Power"; + stateText = "Power ?"; break; } @@ -117,39 +148,15 @@ void PowerStatusBar::updateTrackPowerButton(JmriJsonClient::PowerState state) lv_label_set_text(label, stateText); } -void PowerStatusBar::updateConnectionStatus(JmriJsonClient::ConnectionState state) +void PowerStatusBar::updateConnectionStatus(bool connected) { if (!m_connectionStatusLabel) return; - const char* icon; - const char* text; - uint32_t color; - - switch (state) { - case JmriJsonClient::ConnectionState::CONNECTED: - icon = LV_SYMBOL_OK; - text = " Connected"; - color = 0x00AA00; - break; - case JmriJsonClient::ConnectionState::CONNECTING: - icon = LV_SYMBOL_REFRESH; - text = " Connecting"; - color = 0xFFAA00; - break; - case JmriJsonClient::ConnectionState::FAILED: - icon = LV_SYMBOL_WARNING; - text = " Failed"; - color = 0xFF0000; - break; - case JmriJsonClient::ConnectionState::DISCONNECTED: - default: - icon = LV_SYMBOL_CLOSE; - text = " Disconnected"; - color = 0x888888; - break; - } + const char* icon = connected ? LV_SYMBOL_OK : LV_SYMBOL_CLOSE; + const char* text = connected ? " Connected" : " Disconnected"; + const uint32_t color = connected ? 0x00AA00 : 0x888888; - std::string statusText = std::string(icon) + text; + const std::string statusText = std::string(icon) + text; lv_label_set_text(m_connectionStatusLabel, statusText.c_str()); lv_obj_set_style_text_color(m_connectionStatusLabel, lv_color_hex(color), 0); } diff --git a/main/ui/components/PowerStatusBar.h b/main/ui/components/PowerStatusBar.h index 903e1f3..1e9b389 100644 --- a/main/ui/components/PowerStatusBar.h +++ b/main/ui/components/PowerStatusBar.h @@ -1,10 +1,17 @@ #pragma once #include "lvgl.h" -#include "communication/JmriJsonClient.h" +#include "communication/ThrottleBackend.h" + +class ThrottleController; /** * @brief Track power button + connection status bar + * + * Driven by the active transport through `ThrottleController`, never by a + * concrete client. It used to read `JmriJsonClient` directly, which meant that + * under the orchestrator transport the button did nothing and the label read + * "Disconnected" while the layout was in fact connected. */ class PowerStatusBar { public: @@ -18,20 +25,24 @@ class PowerStatusBar { /** * @brief Create the status bar * @param parent Parent LVGL object - * @param jmriClient JMRI JSON client (not owned) + * @param throttleController Controller owning the active transport (not owned) * @return Container object */ - lv_obj_t* create(lv_obj_t* parent, JmriJsonClient* jmriClient); + lv_obj_t* create(lv_obj_t* parent, ThrottleController* throttleController); + + /** Repaint from current transport state. Must hold the LVGL lock. */ + void refresh(); private: static void onTrackPowerClicked(lv_event_t* e); + static void onTrackPowerChanged(void* userData, ThrottleBackend::TrackPower state); - void updateTrackPowerButton(JmriJsonClient::PowerState state); - void updateConnectionStatus(JmriJsonClient::ConnectionState state); + void updateTrackPowerButton(ThrottleBackend::TrackPower state); + void updateConnectionStatus(bool connected); lv_obj_t* m_container; lv_obj_t* m_trackPowerButton; lv_obj_t* m_connectionStatusLabel; - JmriJsonClient* m_jmriClient; + ThrottleController* m_throttleController; }; diff --git a/sdkconfig.defaults b/sdkconfig.defaults index 155f455..b5f002d 100644 --- a/sdkconfig.defaults +++ b/sdkconfig.defaults @@ -41,3 +41,23 @@ CONFIG_LV_USE_IMGFONT=y # CONFIG_LV_USE_DEMO_BENCHMARK is not set # CONFIG_LV_USE_DEMO_STRESS is not set # CONFIG_LV_USE_DEMO_MUSIC is not set + +# WebSocket handshake buffer. This sizes the buffer transport_ws builds the +# HTTP Upgrade exchange in -- NOT esp_websocket_client_config_t::buffer_size, +# which is only the client's frame buffer and has no bearing on the handshake. +# +# The orchestrator re-issues its session cookie on every response (sliding +# expiry), so the /ws upgrade's 101 carries a Set-Cookie on top of the usual +# headers. At the 1024 default that overflows and every connection attempt dies +# with "transport_ws: Header size exceeded buffer size" before the socket opens. +# +# Sized for the 101 response PLUS however much of the first frame arrives in the +# same TCP read. transport_ws exits its read loop once it finds the header +# terminator, but then still fails if the buffer filled -- and the orchestrator +# pushes a whole-layout STATE_SNAPSHOT the instant the socket opens, so the two +# regularly land together. That is why the failure was intermittent: it depends +# on packet timing, not on the headers alone. +# +# The value was previously only in the generated sdkconfig, so a clean checkout +# and CI both built with 1024 and could never have connected to the orchestrator. +CONFIG_WS_BUFFER_SIZE=16384 From 67921bf6e711c3554a9fa043b6ae531c188770d6 Mon Sep 17 00:00:00 2001 From: Paul Barrett Date: Thu, 27 Aug 2026 18:38:15 +0100 Subject: [PATCH 2/2] docs: record track power on the port, and why the WS handshake buffer is sized as it is The port reference gains the track-power capability and the UNKNOWN-is-not-off rule; the UI reference gains PowerStatusBar; the threading table gains the track_power task. CLAUDE.md gains two entries: that the UI asks ThrottleController rather than a concrete client, and that CONFIG_WS_BUFFER_SIZE is not oversizing -- it holds the 101 plus whatever of the opening snapshot lands with it, and lowering it brings the intermittent handshake failure back. Co-Authored-By: Claude Opus 5 --- CLAUDE.md | 8 ++++ docs/architecture/THREADING_MODEL.md | 8 ++-- docs/components/COMMUNICATION_LAYER.md | 51 ++++++++++++++++++++++++-- docs/components/UI_LAYER.md | 19 ++++++++++ 4 files changed, 79 insertions(+), 7 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index bf51978..2e7be5c 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -155,6 +155,10 @@ Decided 2026-08-24, **built 2026-08-27**. Recorded so it is not re-litigated: `TransportSettings` before anything connects. Under the orchestrator, JMRI auto-connect is never started; under WiThrottle, no orchestrator task exists. The device must never sit retrying a server the operator did not choose. +- **The UI asks `ThrottleController`, never a concrete client.** Connection state, knob + gating, functions and track power all come through the port. Reaching past it into + `WiThrottleClient` or `JmriJsonClient` is what made the orchestrator transport look dead + on its first bench run — both are disconnected when the orchestrator is selected. - **`setSpeedAndDirection` is the call to use when both change.** `THROTTLE_COMMAND` carries the pair, and sending speed against the *old* direction first commands the loco faster the way it was already going before reversing it. The port's default implementation orders it @@ -184,6 +188,10 @@ Things that look like bugs or oversights and are not. One line each. turned would move a model that no longer tracks anything real. - **`sdkconfig.tests` is tracked despite appearing in `.gitignore`** — it predates the ignore rule. `sdkconfig.test.defaults` is the file that actually matters. +- **`CONFIG_WS_BUFFER_SIZE=16384` is not oversizing.** It sizes the WebSocket *handshake* + buffer, not the frame buffer, and must hold the `101` plus whatever of the orchestrator's + opening `STATE_SNAPSHOT` arrives in the same TCP read. Lowering it brings back an + intermittent `Header size exceeded buffer size` that kills the connection outright. ## Open limits diff --git a/docs/architecture/THREADING_MODEL.md b/docs/architecture/THREADING_MODEL.md index 7cbfa9f..25c0013 100644 --- a/docs/architecture/THREADING_MODEL.md +++ b/docs/architecture/THREADING_MODEL.md @@ -20,6 +20,7 @@ The ESP32-S3 is dual-core. LVGL rendering runs on a dedicated task; network I/O | `orch_connect` | 6 KB | 5 | Wait for WiFi → orchestrator login → fetch roster | `AppController::startOrchestratorConnectTask()` | | `orch_ui_conn` | 6 KB | 5 | Same, triggered by the config screen's Connect button | `OrchestratorConfigScreen::onConnectClicked()` | | `websocket_task` | 6 KB | — | Orchestrator control-plane receive loop (owned by `esp_websocket_client`) | `OrchestratorClient::connect()` | +| `track_power` | 4 KB | 5 | One-shot track-power write | `ThrottleController::requestTrackPower()` | `throttle_poll` is created **only when the active `ThrottleBackend` reports `requiresPolling()`**. WiThrottle does, because it answers queries rather than volunteering @@ -31,9 +32,10 @@ task is created at all and its 4 KB stack is never allocated. no orchestrator task does; under the orchestrator, `orch_connect` starts and JMRI auto-connect is never begun. Nothing sits retrying a server the operator has not chosen. -`orch_connect` and `orch_ui_conn` both exist because the orchestrator login is a **blocking -HTTP round trip** and the roster is two more. None of that may happen on the LVGL task -(F-05). Both are one-shot: they delete themselves when done. +`orch_connect`, `orch_ui_conn` and `track_power` all exist for the same reason: the +orchestrator's login, roster read and power command are **blocking HTTP round trips**, and +none of that may happen on the LVGL task (F-05). `track_power` in particular is spawned +straight from a button handler. All three are one-shot and delete themselves when done. --- diff --git a/docs/components/COMMUNICATION_LAYER.md b/docs/components/COMMUNICATION_LAYER.md index ee6f0c9..f85a516 100644 --- a/docs/components/COMMUNICATION_LAYER.md +++ b/docs/components/COMMUNICATION_LAYER.md @@ -26,6 +26,22 @@ and that difference surfaces here rather than as a fake session. | `providesRoster()` | No selectable roster — the controller must not offer loco selection | | `providesFunctionLabels()` | UI falls back to `F0`…`F28` | | `requiresPolling()` | State arrives unprompted; no `throttle_poll` task is created | +| `supportsTrackPower()` | The power button is **hidden**, not left dead | + +### Track power lives on this port too + +Strictly a layout command rather than a throttle one, but it rides the same +connection and the UI needs one place to ask — so it is here rather than in a second +port with two more adapters. + +`TrackPower::UNKNOWN` is **not** "off". It means nothing has told us yet, and showing it as +off would claim the rails are dead when nobody knows. The UI renders it as its own state. + +Everything the UI needs — connection state, knob gating, functions, track power — comes +through `ThrottleController` and this port. Reaching past it into a concrete client is the +bug that made the orchestrator transport look dead: the knobs were gated on +`WiThrottleClient::isConnected()`, and the power bar read `JmriJsonClient` directly, neither +of which is connected when the orchestrator is the selected transport. ### Threading @@ -117,11 +133,38 @@ Refused, each with a log line and no callback: Within a `STATE_SNAPSHOT`, one bad loco entry is skipped without costing the rest. -### Roster +### Roster and track power are REST, not WebSocket + +The `ClientMessage` union has no track-power member and the snapshot carries loco state keyed +by address but no names, so both are HTTP: + +| Need | Call | +|------|------| +| Roster | `GET /api/layouts/{id}/locos` | +| Track power | `POST /api/layouts/{id}/dcc-link/power` with `{"on": bool}` | + +The layout id comes from `GET /api/layouts`, fetched once and cached. The roster is built +aside and swapped in, so a partly-built roster is never visible to the carousel. + +The power POST's **reply body is deliberately ignored**. The `DCC_LINK` event pushed the +moment it lands is what tells us the truth — that is the route's own contract, not our +preference. `DCC_LINK` is also read off the snapshot, so the button is right from the first +frame rather than waiting for the next change. + +### CONFIG_WS_BUFFER_SIZE, and why it is not the client's buffer_size + +`esp_websocket_client_config_t::buffer_size` is the **frame** buffer. The HTTP Upgrade +handshake is built and read in a *separate* buffer sized by `CONFIG_WS_BUFFER_SIZE` +(`sdkconfig.defaults`). Raising the former does nothing for the latter. + +`transport_ws` reads until it finds the header terminator, then **still fails** if the buffer +filled. The orchestrator pushes a whole-layout `STATE_SNAPSHOT` the instant the socket opens, +so the `101` and a chunk of that snapshot regularly arrive in one TCP read — which is why +`transport_ws: Header size exceeded buffer size` was intermittent rather than constant. It +depends on packet timing, not on header length. -A REST read (`GET /api/layouts/{id}/locos`), not a control-plane message: the snapshot -carries loco state keyed by address but no names. The layout id comes from `GET /api/layouts`. -Built aside and swapped in, so a partly-built roster is never visible to the carousel. +Set to **16384**. It must exceed the response *plus* whatever of the first frame arrives with +it, so a layout that grows enough to inflate the snapshot could eventually need more. --- diff --git a/docs/components/UI_LAYER.md b/docs/components/UI_LAYER.md index 5ce4b93..99d4e77 100644 --- a/docs/components/UI_LAYER.md +++ b/docs/components/UI_LAYER.md @@ -128,6 +128,25 @@ task would freeze every throttle at once (F-05). ## UI Components +### PowerStatusBar + +**File:** `main/ui/components/PowerStatusBar.cpp/h` + +**Purpose:** Track power button and link status, driven by the **active transport** through +`ThrottleController` — never by a concrete client. + +It previously read `JmriJsonClient` directly, so under the orchestrator transport the button +did nothing and the label read "Disconnected" while the layout was in fact connected. + +- A transport answering `supportsTrackPower() == false` gets the button **hidden**, not left + dead for the operator to press and wonder about. +- `TrackPower::UNKNOWN` renders as its own state ("Power ?"), not as off. +- The press returns immediately: the orchestrator's power command is a blocking HTTP round + trip, so the write happens on a short-lived task (F-05). The button repaints when the + layout says power changed, not when we asked. + +--- + ### ThrottleMeter **File:** `main/ui/components/ThrottleMeter.cpp/h`