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 <noreply@anthropic.com>

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**

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

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];
+    esp_err_t err = client_ensure();
-    const int written = snprintf(url, sizeof(url),
+    if (err != ESP_OK) {
-        "https://rt.ambientweather.net/v1/devices/%s"
+        return err;
-        "?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;
     }
 
-    const esp_http_client_config_t config = {
+    /*
-        .url = url,
+     * Reset per request, not on HTTP_EVENT_ON_CONNECTED: a kept-alive session
-        .method = HTTP_METHOD_GET,
+     * reuses the connection, so that event does not fire again and the buffer
-        .timeout_ms = WEATHER_TIMEOUT_MS,
+     * would accumulate across polls.
-        .event_handler = http_event_handler,
+     */
-        .crt_bundle_attach = esp_crt_bundle_attach,
+    resp_len = 0;
-    };
+    resp_truncated = false;
 
-    esp_http_client_handle_t client = esp_http_client_init(&config);
+    esp_http_client_set_timeout_ms(client, session_warm ? WEATHER_TIMEOUT_WARM_MS
-    if (client == NULL) {
+                                                        : WEATHER_TIMEOUT_COLD_MS);
-        return ESP_ERR_NO_MEM;
-    }
 
-    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;

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,13 +84,32 @@ 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 {
+            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 {
-            ESP_LOGW(TAG, "Weather update failed");
                 ui_set_status("Weather update failed");
             }
+        }
 
         vTaskDelay(pdMS_TO_TICKS(WEATHER_UPDATE_INTERVAL_MS));
     }

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.
+ * 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
- * Polled from an LVGL timer rather than driven by a widget event. An event
+ * taps landing on a child widget -- the header or the temperature card
- * handler on the screen object only fires for taps that actually reach the
+ * consume them -- which presents as whole regions of the panel being dead to
- * screen: LVGL does not bubble events to parents by default, so anything
+ * touch while the hardware reports them perfectly well. Anything that needs
- * landing on the header or the temperature card is consumed by that child and
+ * to observe touches screen-wide must poll the input device, as below, rather
- * never seen here. That made whole regions of the panel look like dead touch
+ * than hang an event handler on the screen.
- * zones when the hardware was reporting them perfectly well.
  */
+#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();
 

main/ui/ui_manager.h

@@ -1,6 +1,7 @@
 #ifndef UI_MANAGER_H
 #define UI_MANAGER_H
 
+#include <stdbool.h>
 #include <stdint.h>
 #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

main/utils/utils.c

@@ -1,37 +0,0 @@
-#include <ctype.h>
-#include <string.h>
-
-#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';
-}

main/utils/utils.h

@@ -1,11 +0,0 @@
-#ifndef UTILS_H
-#define UTILS_H
-
-#include <stddef.h>
-#include <stdint.h>
-
-void utils_delay_ms(uint32_t ms);
-uint32_t utils_get_tick_ms(void);
-void utils_trim(char *str);
-
-#endif

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,
+otadata,    data, ota,      0xf000,   0x2000,
-factory,    app,  factory,  0x10000,  0x400000,
+phy_init,   data, phy,      0x11000,  0x1000,
-storage,    data, spiffs,   0x410000, 0xBF0000,
+ota_0,      app,  ota_0,    0x20000,  0x400000,
+ota_1,      app,  ota_1,    0x420000, 0x400000,

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/
+└── api/
-│   ├── ambient_weather.h
+    ├── ambient_weather.h
-│   └── ambient_weather.c
+    └── ambient_weather.c
-└── utils/
-    ├── utils.h
-    └── utils.c
 
 Note: main/ subdirectories are plain source folders, not ESP-IDF components.
 IDF only auto-discovers components under a top-level components/ directory,