From d4b2ab7f99b2dad6de6621e9ebed7c60a61d5f2a Mon Sep 17 00:00:00 2001 From: matt_ed Date: Tue, 4 Aug 2026 09:08:51 -0700 Subject: [PATCH] Work through the remaining review items in TODO.md Keep the HTTP session alive between weather polls (TODO #1). Re-creating the client each time forced a fresh TLS handshake inside the window where the UI is frozen. Measured on hardware: the fetch went from ~1.3s to ~80ms, so the pause is no longer long enough to matter once thermostat controls exist. The timeout drops from 30s to 10s once a session is warm, since only a cold handshake needs the longer budget. Any transport error or non-200 tears the session down so a half-open connection cannot poison every later poll. The response buffer is now reset per request rather than on HTTP_EVENT_ON_CONNECTED -- a kept-alive connection does not re-fire that event, so the buffer would have accumulated across polls. Indicate stale readings (TODO #2). After two consecutive failures the temperature dims and the status line shows the age. Age comes from tick counts, not weather_data_t.last_update: there is no SNTP client here, so time(NULL) starts at the epoch and any age derived from it is meaningless. Gate the touch readout behind UI_DEBUG_TOUCH, default off (TODO #3). Kept the note about LVGL not bubbling events to parents -- it documents a real trap. Delete main/utils (TODO #4); it had no callers outside itself. Also refreshed project_structure.txt, which was additionally missing i2c_bus and backlight. Lay out two 4MB OTA slots (TODO #5) while no devices are fielded, and document in the README that an OTA download cannot run while the panel is scanning -- the flash write faults the bounce-buffer ISR. Log the truncation warning once per response instead of once per chunk, and record the weather task's stack high-water mark (TODO #6): ~2400 bytes spare of 6144. Co-Authored-By: Claude --- README.md | 16 ++++- main/CMakeLists.txt | 2 - main/api/ambient_weather.c | 116 +++++++++++++++++++++++++++---------- main/main.c | 48 ++++++++++++++- main/ui/ui_manager.c | 46 ++++++++++++--- main/ui/ui_manager.h | 8 +++ main/utils/utils.c | 37 ------------ main/utils/utils.h | 11 ---- partitions.csv | 22 ++++++- project_structure.txt | 17 +++--- 10 files changed, 222 insertions(+), 101 deletions(-) delete mode 100644 main/utils/utils.c delete mode 100644 main/utils/utils.h diff --git a/README.md b/README.md index d459f0d..c89cbbf 100644 --- a/README.md +++ b/README.md @@ -25,7 +25,6 @@ implemented yet. | `main/ui/ui_manager.c` | Screen layout and update entry points | | `main/wifi` | WiFi station bring-up | | `main/api` | Ambient Weather REST client | -| `main/utils` | Small helpers | ## Setup @@ -124,6 +123,21 @@ High Performance Mode and a DC-aware bootloader. is running faults it with *"Cache disabled but cached memory region accessed"*. +## Partitions + +Two 4 MB OTA slots (`ota_0` / `ota_1`) with `otadata`, laid out in +`partitions.csv`. The app is ~1.4 MB, so the slots are generous — but the +layout is fixed once devices are fielded, and re-partitioning later means +reflashing each one over USB. ~7.9 MB of the 16 MB is left unallocated; +adding a partition to free space is safe, moving an existing one is not. + +> **OTA is not yet implemented, and it is not a drop-in.** Writing an OTA image +> is a flash write, and the bounce-buffer refill ISR reads PSRAM, which shares +> the cache with flash on the S3 (see Memory notes). Downloading an update +> while the panel is scanning **will** fault that ISR. Any implementation must +> stop the panel for the download and write, or reboot into a dedicated +> updater mode first. + ## Memory notes Two 800×480×2 framebuffers (750 KB each) live in PSRAM. The 8 MB **octal** diff --git a/main/CMakeLists.txt b/main/CMakeLists.txt index d96861d..b94edea 100644 --- a/main/CMakeLists.txt +++ b/main/CMakeLists.txt @@ -9,7 +9,6 @@ idf_component_register( "ui/lvgl_port.c" "ui/ui_manager.c" "api/ambient_weather.c" - "utils/utils.c" INCLUDE_DIRS "." "wifi" @@ -19,7 +18,6 @@ idf_component_register( "drivers/touch" "ui" "api" - "utils" REQUIRES lvgl esp_wifi diff --git a/main/api/ambient_weather.c b/main/api/ambient_weather.c index 94bb9f5..3c4d0e4 100644 --- a/main/api/ambient_weather.c +++ b/main/api/ambient_weather.c @@ -24,19 +24,35 @@ static const char *TAG = "ambient_weather"; #define WEATHER_URL_MAX 320 #define WEATHER_RESP_MAX 4096 -/* A first TLS handshake on this hardware can take well over 10s. */ -#define WEATHER_TIMEOUT_MS 30000 + +/* + * Two timeouts, because the first request costs far more than the rest. + * + * A cold TLS handshake on this board takes well over 10s. Once the session is + * established and kept alive, a poll is just a short request/response. Holding + * the cold timeout forever would mean the caller has to assume a 30s stall on + * every poll -- and the caller pauses the UI for the duration of this call, so + * that assumption is expensive. + */ +#define WEATHER_TIMEOUT_COLD_MS 30000 +#define WEATHER_TIMEOUT_WARM_MS 10000 static char resp_buf[WEATHER_RESP_MAX]; static size_t resp_len = 0; +static bool resp_truncated = false; + +/* + * The client is kept alive between polls. Re-creating it each time forced a + * fresh TLS handshake -- seconds of work, all of it inside the window where + * the UI is frozen. + */ +static esp_http_client_handle_t client = NULL; +static char url[WEATHER_URL_MAX]; +static bool session_warm = false; static esp_err_t http_event_handler(esp_http_client_event_t *evt) { switch (evt->event_id) { - case HTTP_EVENT_ON_CONNECTED: - resp_len = 0; - break; - case HTTP_EVENT_ON_DATA: /* * Accumulate into a fixed buffer. The response is chunked, so @@ -46,7 +62,9 @@ static esp_err_t http_event_handler(esp_http_client_event_t *evt) if (resp_len + evt->data_len < sizeof(resp_buf)) { memcpy(resp_buf + resp_len, evt->data, evt->data_len); resp_len += evt->data_len; - } else { + } else if (!resp_truncated) { + /* Once per response, not once per remaining chunk. */ + resp_truncated = true; ESP_LOGW(TAG, "Response exceeds %d bytes, truncating", WEATHER_RESP_MAX); } break; @@ -58,6 +76,37 @@ static esp_err_t http_event_handler(esp_http_client_event_t *evt) return ESP_OK; } +/* Tear the session down so the next poll rebuilds it from scratch. Used after + * any transport error -- a half-open connection would fail every subsequent + * poll otherwise. */ +static void client_drop(void) +{ + if (client != NULL) { + esp_http_client_cleanup(client); + client = NULL; + } + session_warm = false; +} + +static esp_err_t client_ensure(void) +{ + if (client != NULL) { + return ESP_OK; + } + + const esp_http_client_config_t config = { + .url = url, + .method = HTTP_METHOD_GET, + .timeout_ms = WEATHER_TIMEOUT_COLD_MS, + .event_handler = http_event_handler, + .crt_bundle_attach = esp_crt_bundle_attach, + .keep_alive_enable = true, + }; + + client = esp_http_client_init(&config); + return (client != NULL) ? ESP_OK : ESP_ERR_NO_MEM; +} + esp_err_t weather_init(void) { if (AMBIENT_WEATHER_API_KEY[0] == '\0' || @@ -68,6 +117,17 @@ esp_err_t weather_init(void) return ESP_ERR_INVALID_STATE; } + /* Credentials are compile-time constants, so the URL never changes. */ + const int written = snprintf(url, sizeof(url), + "https://rt.ambientweather.net/v1/devices/%s" + "?apiKey=%s&applicationKey=%s&limit=1", + AMBIENT_WEATHER_DEVICE_ID, AMBIENT_WEATHER_API_KEY, AMBIENT_WEATHER_APP_KEY); + + if (written < 0 || written >= (int)sizeof(url)) { + ESP_LOGE(TAG, "URL truncated -- check credential lengths"); + return ESP_ERR_INVALID_SIZE; + } + return ESP_OK; } @@ -102,45 +162,41 @@ esp_err_t weather_update(weather_data_t *data) * partial response cannot leave stale values looking fresh. */ memset(data, 0, sizeof(*data)); - char url[WEATHER_URL_MAX]; - const int written = snprintf(url, sizeof(url), - "https://rt.ambientweather.net/v1/devices/%s" - "?apiKey=%s&applicationKey=%s&limit=1", - AMBIENT_WEATHER_DEVICE_ID, AMBIENT_WEATHER_API_KEY, AMBIENT_WEATHER_APP_KEY); - - if (written < 0 || written >= (int)sizeof(url)) { - ESP_LOGE(TAG, "URL truncated -- check credential lengths"); - return ESP_ERR_INVALID_SIZE; + esp_err_t err = client_ensure(); + if (err != ESP_OK) { + return err; } - const esp_http_client_config_t config = { - .url = url, - .method = HTTP_METHOD_GET, - .timeout_ms = WEATHER_TIMEOUT_MS, - .event_handler = http_event_handler, - .crt_bundle_attach = esp_crt_bundle_attach, - }; + /* + * Reset per request, not on HTTP_EVENT_ON_CONNECTED: a kept-alive session + * reuses the connection, so that event does not fire again and the buffer + * would accumulate across polls. + */ + resp_len = 0; + resp_truncated = false; - esp_http_client_handle_t client = esp_http_client_init(&config); - if (client == NULL) { - return ESP_ERR_NO_MEM; - } + esp_http_client_set_timeout_ms(client, session_warm ? WEATHER_TIMEOUT_WARM_MS + : WEATHER_TIMEOUT_COLD_MS); - esp_err_t err = esp_http_client_perform(client); + err = esp_http_client_perform(client); if (err != ESP_OK) { ESP_LOGE(TAG, "HTTP request failed: %s", esp_err_to_name(err)); - esp_http_client_cleanup(client); + client_drop(); return err; } const int status = esp_http_client_get_status_code(client); - esp_http_client_cleanup(client); if (status != 200) { ESP_LOGE(TAG, "HTTP %d from Ambient Weather", status); + /* Could be a stale session the server has since closed. */ + client_drop(); return ESP_FAIL; } + /* Connection is good; subsequent polls skip the handshake. */ + session_warm = true; + if (resp_len == 0) { ESP_LOGE(TAG, "Empty response"); return ESP_FAIL; diff --git a/main/main.c b/main/main.c index ac58269..d62cdf1 100644 --- a/main/main.c +++ b/main/main.c @@ -18,15 +18,36 @@ static const char *TAG = "main"; +/* + * 6144 bytes. Measured headroom after a fetch including the TLS handshake -- + * the deepest this task gets -- is ~2400 bytes, so roughly 3.7KB is in use. + * mbedTLS keeps its heap in PSRAM (CONFIG_MBEDTLS_EXTERNAL_MEM_ALLOC), which + * is why the handshake fits at all. Re-check the high-water-mark log if more + * work lands in this task. + */ #define WEATHER_TASK_STACK 6144 #define WEATHER_TASK_PRIO 3 #define WEATHER_FIRST_DELAY_MS 3000 +/* Consecutive failed polls before the displayed reading is treated as stale. */ +#define WEATHER_STALE_AFTER_POLLS 2 + static void weather_update_task(void *arg) { weather_data_t weather; char line[96]; + /* + * Age is tracked in ticks, not from weather_data_t.last_update. That field + * comes from time(NULL), and with no SNTP client the system clock starts + * at the epoch -- an age derived from it would be nonsense. Tick + * subtraction is also wrap-safe. + */ + TickType_t last_success = 0; + int consecutive_failures = 0; + bool have_reading = false; + bool stack_logged = false; + vTaskDelay(pdMS_TO_TICKS(WEATHER_FIRST_DELAY_MS)); while (1) { @@ -48,6 +69,10 @@ static void weather_update_task(void *arg) lvgl_port_resume(); if (err == ESP_OK) { + last_success = xTaskGetTickCount(); + consecutive_failures = 0; + have_reading = true; + weather_get_string(&weather, line, sizeof(line)); ESP_LOGI(TAG, "Weather: %s", line); @@ -59,12 +84,31 @@ static void weather_update_task(void *arg) * below 0°F, turning -10.6 into -10. */ ui_update_weather((int16_t)lroundf(weather.temp_f), weather.has_humidity ? detail : "Outdoor"); + ui_set_weather_stale(false); wifi_get_status(line, sizeof(line)); ui_set_status(line); + + /* One-shot, after a fetch that included a TLS handshake -- that is + * the deepest this task's stack ever gets. */ + if (!stack_logged) { + stack_logged = true; + ESP_LOGI(TAG, "weather task stack headroom: %u bytes", + (unsigned)(uxTaskGetStackHighWaterMark(NULL) * sizeof(StackType_t))); + } } else { - ESP_LOGW(TAG, "Weather update failed"); - ui_set_status("Weather update failed"); + consecutive_failures++; + ESP_LOGW(TAG, "Weather update failed (%d consecutive)", consecutive_failures); + + if (have_reading && consecutive_failures >= WEATHER_STALE_AFTER_POLLS) { + const uint32_t age_min = + ((xTaskGetTickCount() - last_success) * portTICK_PERIOD_MS) / 60000U; + ui_set_weather_stale(true); + snprintf(line, sizeof(line), "No update for %lum", (unsigned long)age_min); + ui_set_status(line); + } else { + ui_set_status("Weather update failed"); + } } vTaskDelay(pdMS_TO_TICKS(WEATHER_UPDATE_INTERVAL_MS)); diff --git a/main/ui/ui_manager.c b/main/ui/ui_manager.c index 77aca42..586f114 100644 --- a/main/ui/ui_manager.c +++ b/main/ui/ui_manager.c @@ -11,10 +11,19 @@ static const char *TAG = "ui_manager"; #define UI_LOCK_TIMEOUT_MS 1000 +/* + * Bring-up diagnostic: a live readout of the last touch coordinate. Off by + * default -- define UI_DEBUG_TOUCH to put it back on screen when working on + * the input path. + */ +/* #define UI_DEBUG_TOUCH */ + static lv_obj_t *temp_label = NULL; static lv_obj_t *cond_label = NULL; static lv_obj_t *status_label = NULL; +#ifdef UI_DEBUG_TOUCH static lv_obj_t *touch_label = NULL; +#endif /* * lv_label_set_text() invalidates the label unconditionally, even when the @@ -37,15 +46,15 @@ static void label_set_if_changed(lv_obj_t *label, const char *text) } /* - * Live readout of the last contact. - * - * Polled from an LVGL timer rather than driven by a widget event. An event - * handler on the screen object only fires for taps that actually reach the - * screen: LVGL does not bubble events to parents by default, so anything - * landing on the header or the temperature card is consumed by that child and - * never seen here. That made whole regions of the panel look like dead touch - * zones when the hardware was reporting them perfectly well. + * NOTE, even with the readout compiled out: LVGL does not bubble events to + * parents by default. An event handler on the screen object never fires for + * taps landing on a child widget -- the header or the temperature card + * consume them -- which presents as whole regions of the panel being dead to + * touch while the hardware reports them perfectly well. Anything that needs + * to observe touches screen-wide must poll the input device, as below, rather + * than hang an event handler on the screen. */ +#ifdef UI_DEBUG_TOUCH static void touch_readout_timer_cb(lv_timer_t *timer) { lv_indev_t *indev = lv_indev_get_next(NULL); @@ -64,6 +73,7 @@ static void touch_readout_timer_cb(lv_timer_t *timer) } label_set_if_changed(touch_label, buf); } +#endif void ui_update_weather(int16_t temp_f, const char *condition) { @@ -80,6 +90,24 @@ void ui_update_weather(int16_t temp_f, const char *condition) lvgl_port_unlock(); } +void ui_set_weather_stale(bool stale) +{ + if (!lvgl_port_lock(UI_LOCK_TIMEOUT_MS)) { + ESP_LOGW(TAG, "Dropped stale-state update: LVGL lock timed out"); + return; + } + + if (temp_label != NULL) { + /* Unlike lv_label_set_text(), setting an identical style value does + * not invalidate, so this is safe to call on every poll. */ + lv_obj_set_style_text_color(temp_label, + stale ? lv_color_hex(0x5A6672) : lv_color_white(), + LV_PART_MAIN); + } + + lvgl_port_unlock(); +} + void ui_set_status(const char *status) { if (status == NULL) { @@ -150,11 +178,13 @@ void ui_init(void) lv_label_set_text(status_label, "Starting..."); lv_obj_set_style_text_color(status_label, lv_color_hex(0x7F8C8D), LV_PART_MAIN); +#ifdef UI_DEBUG_TOUCH touch_label = lv_label_create(scr); lv_label_set_text(touch_label, "touch --, --"); lv_obj_set_style_text_color(touch_label, lv_color_hex(0x3498DB), LV_PART_MAIN); lv_timer_create(touch_readout_timer_cb, 100, NULL); +#endif lvgl_port_unlock(); diff --git a/main/ui/ui_manager.h b/main/ui/ui_manager.h index cd2f04e..38fbb8b 100644 --- a/main/ui/ui_manager.h +++ b/main/ui/ui_manager.h @@ -1,6 +1,7 @@ #ifndef UI_MANAGER_H #define UI_MANAGER_H +#include #include #include "lvgl.h" @@ -10,4 +11,11 @@ void ui_init(void); void ui_update_weather(int16_t temp_f, const char *condition); void ui_set_status(const char *status); +/* + * Mark the displayed reading as no longer trustworthy. Dims the temperature + * so a value that stopped updating is visibly distinct from a live one, + * rather than sitting there at full styling looking current. + */ +void ui_set_weather_stale(bool stale); + #endif diff --git a/main/utils/utils.c b/main/utils/utils.c deleted file mode 100644 index f79a331..0000000 --- a/main/utils/utils.c +++ /dev/null @@ -1,37 +0,0 @@ -#include -#include - -#include "freertos/FreeRTOS.h" -#include "freertos/task.h" - -#include "utils.h" - -void utils_delay_ms(uint32_t ms) -{ - vTaskDelay(pdMS_TO_TICKS(ms)); -} - -uint32_t utils_get_tick_ms(void) -{ - return (uint32_t)(xTaskGetTickCount() * portTICK_PERIOD_MS); -} - -void utils_trim(char *str) -{ - if (!str) return; - - char *start = str; - while (*start && isspace((unsigned char)*start)) start++; - - if (*start == '\0') { - *str = '\0'; - return; - } - - char *end = start + strlen(start) - 1; - while (end > start && isspace((unsigned char)*end)) end--; - - size_t len = end - start + 1; - memmove(str, start, len); - str[len] = '\0'; -} diff --git a/main/utils/utils.h b/main/utils/utils.h deleted file mode 100644 index 799a468..0000000 --- a/main/utils/utils.h +++ /dev/null @@ -1,11 +0,0 @@ -#ifndef UTILS_H -#define UTILS_H - -#include -#include - -void utils_delay_ms(uint32_t ms); -uint32_t utils_get_tick_ms(void); -void utils_trim(char *str); - -#endif diff --git a/partitions.csv b/partitions.csv index 79e6fd6..d8f45ce 100644 --- a/partitions.csv +++ b/partitions.csv @@ -1,8 +1,24 @@ # Partition table for ESP32-S3-WROOM-1-N16R8 (16MB flash, 8MB octal PSRAM) # as fitted to the CrowPanel 7" Advance. # +# Two 4MB OTA slots. The app is ~1.1MB, so this is generous -- but the layout +# is fixed once devices are in the field, and re-partitioning later means +# reflashing each one over USB. Laying the slots out now costs nothing. +# +# NOTE: writing an OTA image is a flash write, and the panel's bounce-buffer +# refill ISR reads PSRAM, which shares the cache with flash on the S3. An OTA +# download while the panel is scanning WILL fault that ISR ("Cache disabled but +# cached memory region accessed"). Any OTA implementation must stop the panel +# for the download+write, or reboot into a dedicated updater mode first. See +# README "Memory notes". +# +# The former 12MB SPIFFS partition was dropped -- nothing mounted it. ~7.9MB +# is left unallocated for a filesystem or a third slot if either is ever +# needed; adding a partition to free space is safe, moving one is not. +# # Name, Type, SubType, Offset, Size, Flags nvs, data, nvs, 0x9000, 0x6000, -phy_init, data, phy, 0xf000, 0x1000, -factory, app, factory, 0x10000, 0x400000, -storage, data, spiffs, 0x410000, 0xBF0000, +otadata, data, ota, 0xf000, 0x2000, +phy_init, data, phy, 0x11000, 0x1000, +ota_0, app, ota_0, 0x20000, 0x400000, +ota_1, app, ota_1, 0x420000, 0x400000, diff --git a/project_structure.txt b/project_structure.txt index fd54aa9..323088c 100644 --- a/project_structure.txt +++ b/project_structure.txt @@ -2,6 +2,7 @@ CMakeLists.txt (project root -- required by ESP-IDF) sdkconfig.defaults partitions.csv README.md +TODO.md main/ ├── CMakeLists.txt (the only component registration in the project) ├── idf_component.yml @@ -12,6 +13,11 @@ main/ │ ├── wifi_manager.h │ └── wifi_manager.c ├── drivers/ +│ ├── i2c_bus.h (shared bus: STC8H1K28 + GT911) +│ ├── i2c_bus.c +│ ├── backlight/ +│ │ ├── backlight.h (STC8H1K28 -- also gates GT911 reset) +│ │ └── backlight.c │ ├── display/ │ │ ├── display_rgb.h │ │ └── display_rgb.c @@ -19,16 +25,13 @@ main/ │ ├── touch_gt911.h │ └── touch_gt911.c ├── ui/ -│ ├── lvgl_port.h (tick, service task, locking) +│ ├── lvgl_port.h (tick, service task, locking, pause/resume) │ ├── lvgl_port.c │ ├── ui_manager.h │ └── ui_manager.c -├── api/ -│ ├── ambient_weather.h -│ └── ambient_weather.c -└── utils/ - ├── utils.h - └── utils.c +└── api/ + ├── ambient_weather.h + └── ambient_weather.c Note: main/ subdirectories are plain source folders, not ESP-IDF components. IDF only auto-discovers components under a top-level components/ directory,