Fix TWDT starvation, vsync race, and UI responsiveness from review

Three bugs and two UX defects found on review, plus hardening:

- The control loop fed the task watchdog then slept exactly the watchdog
  period (both 10s), tripping the TWDT fractionally late on every single
  iteration. Sleep is now sliced 1s with a reset per slice. Also set
  CONFIG_ESP_TASK_WDT_PANIC=y - the sdkconfig comment claimed a hung task
  panics the chip, but nothing actually made that true.

- The LVGL flush called draw_bitmap (which only queues the framebuffer
  switch) and released LVGL immediately, letting it render the next frame
  into the buffer still being scanned out - the exact PRD 8.2 race the
  double-buffer design exists to prevent. display_present_frame() now
  blocks on a vsync handshake until the swap has latched.

- WiFi reconnect slept up to 30s inside the system event handler, stalling
  all queued WiFi/IP events. Moved to an esp_timer.

- The setpoint label, arc, and mode/room lines rendered the control loop's
  copy of state, which refreshes every 10s - every knob detent looked dead
  for up to 10 seconds. The UI now reads settings live; when the fallback
  ladder clamps the setpoint it shows both values. Room-select focus now
  shows which room is being selected.

- z2m ISO_8601_local timestamps carry a UTC offset the parser ignored,
  which would shift apparent reading age by the local offset (5-6h in the
  US) and permanently trip the fallback ladder. Offset is now honoured.

- Suppressed the cosmetic "control loop stopped" fault in the window
  between relay_init and the control task's first call-in at boot.

- Explicit pullups on the encoder A/B lines.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

main/drivers/display/display.c

@@ -60,6 +60,16 @@ static const char *TAG = "display";
 static esp_lcd_panel_handle_t s_panel;
 static void *s_fb0, *s_fb1;
 
+/*
+ * Vsync handshake for tear-free buffer swaps (the pattern from Espressif's
+ * own RGB-panel LVGL example). present_frame gives s_sem_present_pending after
+ * queueing the swap; the vsync ISR converts that into s_sem_swap_done only
+ * when a present is actually waiting, so a stale vsync can never satisfy a
+ * later wait.
+ */
+static SemaphoreHandle_t s_sem_present_pending;
+static SemaphoreHandle_t s_sem_swap_done;
+
 /* ------------------------------------------------------------ backlight -- */
 
 static esp_err_t backlight_init(void)
@@ -122,6 +132,52 @@ static esp_err_t panel_power_and_reset(void)
     return ESP_OK;
 }
 
+/*
+ * Runs from the RGB panel's frame ISR. CONFIG_LCD_RGB_ISR_IRAM_SAFE=y requires
+ * this to live in IRAM.
+ */
+IRAM_ATTR static bool on_vsync_cb(esp_lcd_panel_handle_t panel,
+                                  const esp_lcd_rgb_panel_event_data_t *edata,
+                                  void *user_ctx)
+{
+    (void)panel;
+    (void)edata;
+    (void)user_ctx;
+
+    BaseType_t hpw = pdFALSE;
+    if (xSemaphoreTakeFromISR(s_sem_present_pending, &hpw) == pdTRUE) {
+        xSemaphoreGiveFromISR(s_sem_swap_done, &hpw);
+    }
+    return hpw == pdTRUE;
+}
+
+esp_err_t display_present_frame(void *fb)
+{
+    if (!s_panel || !fb) {
+        return ESP_ERR_INVALID_STATE;
+    }
+
+    esp_err_t err = esp_lcd_panel_draw_bitmap(s_panel, 0, 0,
+                                              DISPLAY_H_RES, DISPLAY_V_RES, fb);
+    if (err != ESP_OK) {
+        return err;
+    }
+
+    /* Wait for the vsync at which the queued switch latches. The timeout is a
+     * guard so a stalled panel degrades to tearing instead of wedging the
+     * LVGL task; one frame at these timings is ~15ms. */
+    xSemaphoreGive(s_sem_present_pending);
+    if (xSemaphoreTake(s_sem_swap_done, pdMS_TO_TICKS(100)) != pdTRUE) {
+        static bool warned;
+        if (!warned) {
+            warned = true;
+            ESP_LOGW(TAG, "vsync wait timed out — panel not producing frames?");
+        }
+    }
+
+    return ESP_OK;
+}
+
 /* --------------------------------------------------------------- init ---- */
 
 static esp_err_t send_init_sequence(void)
@@ -243,6 +299,19 @@ static esp_err_t rgb_panel_init(void)
         esp_lcd_rgb_panel_get_frame_buffer(s_panel, 2, &s_fb0, &s_fb1),
         TAG, "get framebuffers");
 
+    s_sem_present_pending = xSemaphoreCreateBinary();
+    s_sem_swap_done = xSemaphoreCreateBinary();
+    if (!s_sem_present_pending || !s_sem_swap_done) {
+        return ESP_ERR_NO_MEM;
+    }
+
+    esp_lcd_rgb_panel_event_callbacks_t cbs = {
+        .on_vsync = on_vsync_cb,
+    };
+    ESP_RETURN_ON_ERROR(
+        esp_lcd_rgb_panel_register_event_callbacks(s_panel, &cbs, NULL),
+        TAG, "vsync callback");
+
     ESP_LOGI(TAG, "RGB panel up: %dx%d @ %d MHz, fb0=%p fb1=%p",
              DISPLAY_H_RES, DISPLAY_V_RES, LCD_PCLK_HZ / 1000000, s_fb0, s_fb1);
     return ESP_OK;
@@ -321,6 +390,6 @@ void display_test_pattern(void)
         }
     }
 
-    esp_lcd_panel_draw_bitmap(s_panel, 0, 0, DISPLAY_H_RES, DISPLAY_V_RES, fb);
+    display_present_frame(fb);
     ESP_LOGI(TAG, "colour bars drawn — this is the PRD phase-2 check");
 }

main/drivers/display/display.h

@@ -40,6 +40,18 @@ esp_err_t display_init(void);
 /* The esp_lcd panel handle, for the LVGL port. */
 esp_lcd_panel_handle_t display_panel(void);
 
+/*
+ * Hand a completed frame (one of the two framebuffers) to the panel and block
+ * until the swap has latched at vsync.
+ *
+ * esp_lcd_panel_draw_bitmap() only *queues* the framebuffer switch — the RGB
+ * driver latches it at the next frame boundary. Returning before that point
+ * lets the caller start rendering into the other buffer while it is still
+ * being scanned out, which is exactly the persistent-corruption race PRD 8.2
+ * warns about. All frame presentation goes through here so that cannot happen.
+ */
+esp_err_t display_present_frame(void *fb);
+
 /* Both framebuffers, for LVGL to render into directly. */
 esp_err_t display_get_framebuffers(void **fb0, void **fb1);
 

main/drivers/encoder/encoder.c

@@ -1,5 +1,6 @@
 #include "encoder.h"
 
+#include "driver/gpio.h"
 #include "driver/pulse_cnt.h"
 #include "esp_check.h"
 #include "esp_log.h"
@@ -125,6 +126,16 @@ esp_err_t encoder_init(void)
                                       PCNT_CHANNEL_LEVEL_ACTION_INVERSE),
         TAG, "chan b level");
 
+    /*
+     * The vendor's Arduino example uses bare INPUT, implying external pullups
+     * on the board, but their ESPHome config enables internal ones as well.
+     * Enabling them here costs nothing and guards against a floating line
+     * reading as phantom rotation. (Done after channel creation so the PCNT
+     * driver's own GPIO setup cannot undo it.)
+     */
+    gpio_pullup_en(ENCODER_PIN_A);
+    gpio_pullup_en(ENCODER_PIN_B);
+
     ESP_RETURN_ON_ERROR(pcnt_unit_enable(s_unit), TAG, "enable");
     ESP_RETURN_ON_ERROR(pcnt_unit_clear_count(s_unit), TAG, "clear");
     ESP_RETURN_ON_ERROR(pcnt_unit_start(s_unit), TAG, "start");

main/hvac/control.c

@@ -310,7 +310,16 @@ static void control_task(void *arg)
             last_call = st.calling_for_heat;
         }
 
-        vTaskDelay(pdMS_TO_TICKS(CONTROL_INTERVAL_S * 1000));
+        /*
+         * Sleep in 1s slices, feeding the task watchdog each one. A single
+         * CONTROL_INTERVAL_S delay would finish fractionally *after* the TWDT
+         * deadline every iteration (both are 10s) and trip it spuriously —
+         * while still keeping the evaluation cadence at CONTROL_INTERVAL_S.
+         */
+        for (int i = 0; i < CONTROL_INTERVAL_S; i++) {
+            esp_task_wdt_reset();
+            vTaskDelay(pdMS_TO_TICKS(1000));
+        }
     }
 }
 

main/hvac/relay.c

@@ -31,6 +31,7 @@ static bool s_initialised;
 
 static bool s_heat_on;
 static bool s_requested;
+static bool s_ever_requested;   /* control loop has called in at least once */
 static relay_fault_t s_fault;
 
 static int64_t s_boot_us;
@@ -221,7 +222,15 @@ static void safety_task(void *arg)
         if (!control_alive) {
             if (s_heat_on) {
                 turn_off_locked(RELAY_FAULT_WATCHDOG);
-            } else if (s_fault == RELAY_FAULT_NONE) {
+            } else if (s_fault == RELAY_FAULT_NONE && s_ever_requested) {
+                /*
+                 * Only fault once the control loop has *stopped* talking.
+                 * Before its first-ever call this is just boot ordering — the
+                 * relay is held off regardless (the seeded-stale watchdog is
+                 * the bounded-startup interlock), and flashing "control loop
+                 * stopped" on every power-up would teach the user to ignore
+                 * the one fault that matters most.
+                 */
                 s_fault = RELAY_FAULT_WATCHDOG;
             }
             s_requested = false;
@@ -269,6 +278,7 @@ esp_err_t relay_init(void)
     s_last_start_us = 0;
     s_heat_on = false;
     s_requested = false;
+    s_ever_requested = false;
     s_fault = RELAY_FAULT_NONE;
 
     /*
@@ -303,6 +313,7 @@ void relay_request_heat(bool want_heat)
 
     xSemaphoreTake(s_lock, portMAX_DELAY);
     s_requested = want_heat;
+    s_ever_requested = true;
     s_last_request_us = now_us();
 
     /* Recovering from a watchdog trip: the control loop is talking again. */

main/net/wifi_manager.c

@@ -9,6 +9,7 @@
 #include "esp_log.h"
 #include "esp_netif.h"
 #include "esp_netif_sntp.h"
+#include "esp_timer.h"
 #include "esp_wifi.h"
 #include "freertos/FreeRTOS.h"
 #include "freertos/event_groups.h"
@@ -24,18 +25,31 @@ static EventGroupHandle_t s_events;
 static volatile bool s_connected;
 static int s_retry_ms = RETRY_MIN_MS;
 static bool s_sntp_started;
+static esp_timer_handle_t s_reconnect_timer;
 
+static void reconnect_timer_cb(void *arg)
+{
+    (void)arg;
+    esp_wifi_connect();
+}
+
+/*
+ * Schedule the retry on a timer rather than sleeping here. This runs on the
+ * shared system event task — a vTaskDelay would stall every queued WiFi/IP
+ * event behind it (up to 30s at full backoff) and can overflow the default
+ * event queue during an outage.
+ */
 static void schedule_reconnect(void)
 {
     ESP_LOGW(TAG, "reconnecting in %d ms", s_retry_ms);
-    vTaskDelay(pdMS_TO_TICKS(s_retry_ms));
+
+    esp_timer_stop(s_reconnect_timer);   /* no-op if not running */
+    esp_timer_start_once(s_reconnect_timer, (uint64_t)s_retry_ms * 1000);
 
     s_retry_ms *= 2;
     if (s_retry_ms > RETRY_MAX_MS) {
         s_retry_ms = RETRY_MAX_MS;
     }
-
-    esp_wifi_connect();
 }
 
 static void event_handler(void *arg, esp_event_base_t base,
@@ -49,11 +63,6 @@ static void event_handler(void *arg, esp_event_base_t base,
     } else if (base == WIFI_EVENT && id == WIFI_EVENT_STA_DISCONNECTED) {
         s_connected = false;
         xEventGroupClearBits(s_events, CONNECTED_BIT);
-        /*
-         * Runs on the event task, so the delay in schedule_reconnect() blocks
-         * only WiFi event processing — never the control loop, which lives on
-         * its own task and does not consult connection state anyway.
-         */
         schedule_reconnect();
 
     } else if (base == IP_EVENT && id == IP_EVENT_STA_GOT_IP) {
@@ -92,6 +101,12 @@ esp_err_t wifi_manager_init(void)
     setenv("TZ", APP_TIMEZONE, 1);
     tzset();
 
+    const esp_timer_create_args_t timer_args = {
+        .callback = reconnect_timer_cb,
+        .name = "wifi_retry",
+    };
+    ESP_ERROR_CHECK(esp_timer_create(&timer_args, &s_reconnect_timer));
+
     ESP_ERROR_CHECK(esp_netif_init());
     ESP_ERROR_CHECK(esp_event_loop_create_default());
     esp_netif_create_default_wifi_sta();

main/net/zigbee_client.c

@@ -103,10 +103,37 @@ static bool parse_sensor_age(const cJSON *root, uint32_t *age_s)
         seen = (time_t)(ls->valuedouble / 1000.0);
     } else if (cJSON_IsString(ls) && ls->valuestring) {
         struct tm tm = {0};
-        if (!strptime(ls->valuestring, "%Y-%m-%dT%H:%M:%S", &tm)) {
+        const char *rest = strptime(ls->valuestring, "%Y-%m-%dT%H:%M:%S", &tm);
+        if (!rest) {
             return false;
         }
-        seen = (time_t)tm_to_epoch_utc(&tm);
+        int64_t epoch = tm_to_epoch_utc(&tm);
+
+        /*
+         * Honour the offset suffix. z2m's ISO_8601 setting emits UTC ("Z"),
+         * but ISO_8601_local emits local time with an offset ("-05:00").
+         * Ignoring that suffix would shift every reading's apparent age by
+         * the local offset — on a US install that is five or six hours, which
+         * makes live sensors look stale and trips the fallback ladder
+         * permanently. local = UTC + offset, so subtract it.
+         */
+        if (*rest == '.') {                     /* fractional seconds */
+            rest++;
+            while (*rest >= '0' && *rest <= '9') {
+                rest++;
+            }
+        }
+        if (*rest == '+' || *rest == '-') {
+            int sign = (*rest == '-') ? -1 : 1;
+            int oh = 0, om = 0;
+            if (sscanf(rest + 1, "%2d:%2d", &oh, &om) < 1) {
+                return false;
+            }
+            epoch -= (int64_t)sign * (oh * 3600 + om * 60);
+        }
+        /* 'Z' or nothing: already UTC. */
+
+        seen = (time_t)epoch;
     } else {
         return false;
     }

main/ui/lvgl_port.c

@@ -36,12 +36,12 @@ static void flush_cb(lv_display_t *disp, const lv_area_t *area, uint8_t *px_map)
     (void)area;
 
     /*
-     * px_map is one of the panel's own framebuffers, so this does not copy —
+     * px_map is one of the panel's own framebuffers, so no copy happens — the
-     * the RGB driver just switches which buffer is being scanned out and waits
+     * RGB driver switches which buffer is scanned out. present_frame blocks
-     * for the switch to take effect.
+     * until that switch has latched at vsync; releasing LVGL any earlier would
+     * let it render the next frame into the buffer still on screen.
      */
-    esp_lcd_panel_handle_t panel = display_panel();
+    display_present_frame(px_map);
-    esp_lcd_panel_draw_bitmap(panel, 0, 0, DISPLAY_H_RES, DISPLAY_V_RES, px_map);
 
     lv_display_flush_ready(disp);
 }

main/ui/ui_manager.c

@@ -179,7 +179,7 @@ static void build_screen(void)
 
 static void refresh_temperatures(const control_state_t *st)
 {
-    char buf[32];
+    char buf[40];
 
     if (isnan(st->governing_temp_c)) {
         set_text_if_changed(s_lbl_temp, "--" DEG);
@@ -189,15 +189,27 @@ static void refresh_temperatures(const control_state_t *st)
     }
 
     /*
-     * Show the effective setpoint, not the requested one. When the fallback
+     * The setpoint shown must come from settings, live — not from control
-     * ladder has clamped it, the screen must say what the thermostat is
+     * state, which only refreshes every CONTROL_INTERVAL_S. Rendering the
-     * actually doing — the fault text below explains why.
+     * control loop's copy makes the knob feel dead for up to 10 seconds per
+     * detent. When the fallback ladder has clamped the working setpoint, show
+     * both: what the user asked for and what the thermostat is actually
+     * holding — the fault text below explains why they differ.
      */
+    float live_sp = control_get_setpoint();
+    bool capped = st->effective_setpoint_c < live_sp - 0.05f;
+
+    if (capped) {
+        snprintf(buf, sizeof(buf), "Set %.0f%s (cap %.0f)",
+                 c_to_display(live_sp), unit_suffix(),
+                 c_to_display(st->effective_setpoint_c));
+    } else {
         snprintf(buf, sizeof(buf), "Set %.0f%s",
-             c_to_display(st->effective_setpoint_c), unit_suffix());
+                 c_to_display(live_sp), unit_suffix());
+    }
     set_text_if_changed(s_lbl_setpoint, buf);
 
-    int pct = (int)((st->effective_setpoint_c - CONTROL_SETPOINT_MIN_C) * 100.0f /
+    int pct = (int)((live_sp - CONTROL_SETPOINT_MIN_C) * 100.0f /
                     (CONTROL_SETPOINT_MAX_C - CONTROL_SETPOINT_MIN_C));
     if (pct < 0) {
         pct = 0;
@@ -216,13 +228,21 @@ static void refresh_room_and_mode(const control_state_t *st)
                            : "No sensor";
     set_text_if_changed(s_lbl_room, room);
 
+    /*
+     * While a selection is in progress, read the *settings* value, not control
+     * state: the control loop only re-evaluates every CONTROL_INTERVAL_S, and
+     * showing its lagging copy would make the knob appear to do nothing.
+     * control_get_mode()/control_get_selected_room() go straight to settings.
+     */
     char buf[48];
     switch (s_focus) {
     case FOCUS_ROOM:
-        snprintf(buf, sizeof(buf), "> SELECT ROOM <");
+        snprintf(buf, sizeof(buf), "> ROOM: %s <",
+                 zigbee_client_room_name(control_get_selected_room()));
         break;
     case FOCUS_MODE:
-        snprintf(buf, sizeof(buf), "> MODE: %s <", control_mode_str(st->mode));
+        snprintf(buf, sizeof(buf), "> MODE: %s <",
+                 control_mode_str(control_get_mode()));
         break;
     default:
         snprintf(buf, sizeof(buf), "%s", control_mode_str(st->mode));

sdkconfig.defaults

@@ -71,8 +71,13 @@ CONFIG_GDMA_CTRL_FUNC_IN_IRAM=y
 # --- Watchdogs ---------------------------------------------------------------
 # The task watchdog is part of the safety story: a hung control task panics the
 # chip, and the reboot path de-asserts the relay within about a second.
+# ESP_TASK_WDT_PANIC is what makes that true — without it the TWDT only logs a
+# warning and a hung task keeps hanging. (The relay layer's own 30s watchdog
+# has already dropped the heat call by then; this reboots the chip so the
+# thermostat comes back instead of staying wedged.)
 CONFIG_ESP_TASK_WDT_INIT=y
 CONFIG_ESP_TASK_WDT_TIMEOUT_S=10
+CONFIG_ESP_TASK_WDT_PANIC=y
 CONFIG_ESP_TASK_WDT_CHECK_IDLE_TASK_CPU0=n
 CONFIG_ESP_TASK_WDT_CHECK_IDLE_TASK_CPU1=n
 CONFIG_ESP_SYSTEM_PANIC_PRINT_REBOOT=y