diff --git a/README.md b/README.md index 7188a98..22e3303 100644 --- a/README.md +++ b/README.md @@ -231,9 +231,17 @@ the buffer being displayed — copying into the live framebuffer races the LCD's own reads, and the resulting corrupt pixels persist indefinitely because LVGL does not know they are wrong and never reinvalidates them. -A 16-scanline bounce buffer stages each chunk in internal RAM. 20 scanlines -would be better but needs 64KB of internal RAM and fails to allocate once -WiFi is up. +A **20**-scanline bounce buffer stages each chunk in internal RAM (64KB across +two buffers). This was 16 for a long time because 20 "fails to allocate once +WiFi is up" — which was true when `display_init()` ran after +`wifi_init_sta()`. The two-phase split changed that: `display_alloc()` now +takes these buffers from a virgin heap with ~200KB free, long before WiFi has +fragmented it. Bigger chunks give the refill ISR more time to fetch the next +one before the LCD needs it, which attacks the artefact mechanism directly. + +That leaves ~24KB of internal heap once everything is up. Watch the +"Free internal heap" log line — if it approaches single digits, come back +down to 18 or 16 scanlines. ### Known limitation: occasional left-edge artefacts @@ -283,6 +291,20 @@ about PSRAM bandwidth or panel timing. ## Gotchas +- **Setting a style property always invalidates, even to an identical value.** + `lv_obj_set_local_style_prop()` calls `lv_obj_refresh_style()` + unconditionally — there is no equality short-circuit, unlike the one you + might assume by analogy with `label_set_if_changed()`. A periodic update + that restyles widgets "harmlessly" is a periodic full repaint of them. + This cost a round of debugging: `ui_update_thermostat()` ran at 1Hz writing + seven styles, which turned the left-edge artefacts from occasional into + permanent. Every update path here is now change-guarded. +- **Do not repaint to fix artefacts.** It is tempting, because LVGL never + reinvalidates pixels it does not know are wrong. But the corruption scales + with how much is written, so a full-screen invalidate is 768KB of exactly + the PSRAM contention that causes it. Two attempts at this (post-poll and a + post-interaction "repair" timer) both made it visibly worse and were + removed. Redraw *less*, not more. - **LVGL does not bubble events to parents.** An event handler on the screen object never sees taps that land on a child widget, which looks exactly like dead touch zones. Light tiles therefore carry their own handlers, and the diff --git a/TODO.md b/TODO.md index 01a791c..55e7dd2 100644 --- a/TODO.md +++ b/TODO.md @@ -27,13 +27,12 @@ no errors), but eyes and fingers haven't confirmed: 2. Trend arrows appear ~5 minutes after boot (indoor big card, outdoor and pressure cells) once the local ring has history. They are local-history based and reset on reboot — that is by design, not a bug. -3. Artefact lifetime: interactions schedule a one-shot full repaint 600ms - after the last touch, and every poll repaints too. Artefacts should now - visibly self-clean in under a second after touching, and within a poll - otherwise. If they *persist* beyond that, the repaint isn't reaching - them; if they merely *recur* during interaction, that is the PSRAM - bandwidth ceiling (README) and the honest remaining levers are fewer - redraws or 120MHz PSRAM on a newer IDF. +3. Artefact lifetime. Two "repaint to clean up" mitigations were tried and + both made it worse -- see README Gotchas. They are gone, the 1Hz restyle + that made the artefacts permanent is fixed, and the bounce buffer is up to + 20 scanlines. If lines still come and go, the remaining honest levers are + fewer redraws still, less WiFi interrupt load during rendering, or 120MHz + PSRAM on a newer IDF. ## Done diff --git a/main/drivers/display/display_rgb.c b/main/drivers/display/display_rgb.c index da75590..32be757 100644 --- a/main/drivers/display/display_rgb.c +++ b/main/drivers/display/display_rgb.c @@ -80,9 +80,22 @@ static const char *TAG = "display_rgb"; * displays whatever was left in the buffer, showing up as short stale lines * at the left edge where each chunk begins. */ -/* 16 is the practical ceiling: two buffers at 20 lines need 64KB of internal - * RAM and the allocation fails once WiFi is up. */ -#define BOUNCE_BUFFER_LINES 16 +/* + * 20 lines: two buffers at 800px x 2 bytes = 64KB of internal RAM. + * + * This used to be 16 because 20 "fails to allocate once WiFi is up" -- true + * when display_init() ran after wifi_init_sta(). It no longer does: the + * two-phase split means display_alloc() takes these buffers from a virgin + * heap before WiFi has touched it (~200KB free at that point), so the extra + * 12.8KB is affordable now. + * + * Bigger directly attacks the artefact mechanism: each chunk buys the refill + * ISR that much more time to fetch the next one out of PSRAM before the LCD + * needs it. Going further eats the internal RAM WiFi wants at runtime -- + * watch the "Free internal heap" line, and if it drops near single digits, + * come back down. + */ +#define BOUNCE_BUFFER_LINES 20 static esp_lcd_panel_handle_t panel = NULL; static lv_display_t *disp = NULL; diff --git a/main/main.c b/main/main.c index da6b11b..e4b15ab 100644 --- a/main/main.c +++ b/main/main.c @@ -289,20 +289,19 @@ static void ha_task(void *arg) lvgl_port_resume(); /* - * Repaint everything after every fetch window. + * No full-screen invalidate here, deliberately. * - * The known left-edge artefacts (see README) happen when WiFi - * interrupt load starves the bounce-refill ISR, and once bad pixels - * are on screen LVGL has no idea they are wrong -- it never - * reinvalidates them, so they sit there indefinitely. A full - * invalidate after each poll bounds their lifetime at one poll - * interval. Mitigation, not cure: it cannot prevent the glitch, - * only clean up after it. + * There used to be one, on the theory that repainting would clean up + * glitched pixels LVGL does not know are wrong. It made things + * materially worse, and the README already said why: the corruption + * scales with how much is written, because LVGL's writes into PSRAM + * compete with the panel's own DMA reads out of it. A full repaint is + * 768KB of exactly that contention -- the largest possible dose of + * the cause, applied as a cure. + * + * Redraw as little as possible is the actual lever. See ui_update*(), + * which now write nothing when nothing changed. */ - if (lvgl_port_lock(100)) { - lv_obj_invalidate(lv_screen_active()); - lvgl_port_unlock(); - } poll_count++; diff --git a/main/ui/ui_manager.c b/main/ui/ui_manager.c index 3b87885..fe02b3b 100644 --- a/main/ui/ui_manager.c +++ b/main/ui/ui_manager.c @@ -195,33 +195,20 @@ static bool slider_touched[HA_LIGHT_COUNT]; static int32_t slider_sent_pct[HA_LIGHT_COUNT]; /* - * One-shot "repair" repaint, rescheduled by every interaction to fire 600ms - * after the last one. + * There is deliberately no "repair" repaint here any more. * - * Redrawing is itself what glitches the panel: the flush's PSRAM writes - * contend with the LCD's scan-out, and a burst of interactive redraws is the - * most likely moment for the bounce ISR to lose (README, PSRAM bandwidth). - * The glitched lines then persist because LVGL believes those pixels are - * fine. One full repaint after the burst settles cleans them up within - * ~0.6s instead of leaving them until the next poll's repaint, up to 60s - * away. It runs once per burst, not periodically -- a constant repaint loop - * would be a constant supply of fresh glitch opportunities. + * An earlier version scheduled a full-screen invalidate 600ms after each + * interaction, on the theory that repainting would clear glitched pixels + * LVGL does not know are wrong. It made the artefacts worse and constant + * rather than occasional, for the reason the README already gave: the + * corruption scales with how much is written, because LVGL's PSRAM writes + * compete with the panel's DMA reads out of the same PSRAM. Repainting to + * fix corruption caused by repainting does not converge. + * + * The lever that actually works is writing less. Everything below is + * change-guarded: if a value has not moved, no style is written and nothing + * is invalidated. */ -static lv_timer_t *repair_timer; - -static void repair_timer_cb(lv_timer_t *t) -{ - lv_obj_invalidate(lv_screen_active()); - lv_timer_pause(t); -} - -static void schedule_repair(void) -{ - if (repair_timer != NULL) { - lv_timer_reset(repair_timer); - lv_timer_resume(repair_timer); - } -} /* --------------------------------------------------------- condition icons */ @@ -348,18 +335,24 @@ static void trend_render(lv_obj_t *icon, ha_trend_t t) return; } - switch (t) { - case HA_TREND_RISING: - label_set_if_changed(icon, ICON_TREND_UP); + /* + * The colour write is skipped when the glyph did not change. Style writes + * invalidate unconditionally, so an unguarded one here would repaint + * three arrows on every poll to redraw the same pixels. + */ + const char *glyph = (t == HA_TREND_RISING) ? ICON_TREND_UP + : (t == HA_TREND_FALLING) ? ICON_TREND_DOWN + : ""; + const char *current = lv_label_get_text(icon); + if (current != NULL && strcmp(current, glyph) == 0) { + return; + } + + lv_label_set_text(icon, glyph); + if (t == HA_TREND_RISING) { lv_obj_set_style_text_color(icon, lv_color_hex(COL_ACCENT_WARM), LV_PART_MAIN); - break; - case HA_TREND_FALLING: - label_set_if_changed(icon, ICON_TREND_DOWN); + } else if (t == HA_TREND_FALLING) { lv_obj_set_style_text_color(icon, lv_color_hex(COL_ACCENT), LV_PART_MAIN); - break; - default: - label_set_if_changed(icon, ""); - break; } } @@ -409,7 +402,6 @@ static void therm_button_clicked(lv_event_t *e) if (therm_cb != NULL) { therm_cb(cmd); } - schedule_repair(); } /* ------------------------------------------------------------ light tiles */ @@ -469,7 +461,6 @@ static void light_tile_clicked(lv_event_t *e) : -1, }; light_apply_optimistic(idx, expect); - schedule_repair(); /* -1: a tap is on/off only. Brightness is the slider's job, and sending * the current brightness here would fight a user who dimmed the light @@ -525,7 +516,6 @@ static void light_slider_changed(lv_event_t *e) lv_color_hex(pct > 0 ? COL_ON : COL_TEXT_DIM), LV_PART_MAIN); - schedule_repair(); if (light_cb == NULL || !lv_obj_has_state(slider, LV_STATE_PRESSED)) { return; @@ -552,7 +542,6 @@ static void light_slider_released(lv_event_t *e) const bool touched = slider_touched[idx]; slider_touched[idx] = false; - schedule_repair(); if (touched && pct == slider_sent_pct[idx]) { return; /* tap without drag: the press command already said this */ @@ -1070,9 +1059,12 @@ static void update_forecast(const ha_data_t *d) if (icon_font_26 != NULL) { const cond_icon_t *ci = cond_icon_lookup(f->condition); - label_set_if_changed(fc_icon[i], ci->icon); - lv_obj_set_style_text_color(fc_icon[i], lv_color_hex(ci->colour), - LV_PART_MAIN); + const char *current = lv_label_get_text(fc_icon[i]); + if (current == NULL || strcmp(current, ci->icon) != 0) { + lv_label_set_text(fc_icon[i], ci->icon); + lv_obj_set_style_text_color(fc_icon[i], lv_color_hex(ci->colour), + LV_PART_MAIN); + } } if (f->high_f.valid && f->low_f.valid) { @@ -1105,8 +1097,12 @@ void ui_update(const ha_data_t *data) if (icon_font_26 != NULL) { const cond_icon_t *ci = cond_icon_lookup( data->condition[0] ? data->condition : NULL); - label_set_if_changed(cond_icon, ci->icon); - lv_obj_set_style_text_color(cond_icon, lv_color_hex(ci->colour), LV_PART_MAIN); + /* Colour only when the glyph moved -- see trend_render(). */ + const char *current = lv_label_get_text(cond_icon); + if (current == NULL || strcmp(current, ci->icon) != 0) { + lv_label_set_text(cond_icon, ci->icon); + lv_obj_set_style_text_color(cond_icon, lv_color_hex(ci->colour), LV_PART_MAIN); + } } /* The label's width changed with its words; re-hang it off the icon. */ lv_obj_align_to(cond_label, cond_icon, LV_ALIGN_OUT_LEFT_MID, -8, 0); @@ -1138,19 +1134,34 @@ void ui_update(const ha_data_t *data) void ui_set_stale(bool stale) { + /* + * Guarded, because setting an identical style value is NOT free -- + * lv_obj_set_local_style_prop() calls lv_obj_refresh_style() + * unconditionally and invalidates the object. An earlier comment here + * claimed the opposite and was wrong; called every poll, it repainted the + * temperature for no reason. + */ + static bool last_stale; + static bool last_stale_valid; + + if (last_stale_valid && last_stale == stale) { + return; + } + 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, lv_color_hex(stale ? COL_STALE : COL_TEXT), LV_PART_MAIN); } + last_stale = stale; + last_stale_valid = true; + lvgl_port_unlock(); } @@ -1180,12 +1191,37 @@ void ui_set_therm_cb(ui_therm_cmd_cb_t cb) therm_cb = cb; } +/* + * Previous state pushed to the thermostat card. This is not an optimisation, + * it is the fix for a real bug: this function is called once a second by the + * control task, and lv_obj_set_local_style_prop() does NOT skip identical + * values -- it calls lv_obj_refresh_style() unconditionally, which + * invalidates. Writing seven styles per second was seven redraws per second + * forever, which is what turned the left-edge artefacts from occasional into + * permanent. + */ +static therm_status_t last_therm; +static bool last_therm_valid; + void ui_update_thermostat(const therm_status_t *st) { if (st == NULL) { return; } + const bool first = !last_therm_valid; + + /* The common case by far: nothing has moved, so touch nothing and do not + * even take the lock. */ + if (!first && + st->state == last_therm.state && + st->mode == last_therm.mode && + st->fan == last_therm.fan && + st->lockout_remaining_s == last_therm.lockout_remaining_s && + (int)lroundf(st->setpoint_f) == (int)lroundf(last_therm.setpoint_f)) { + return; + } + if (!lvgl_port_lock(UI_LOCK_TIMEOUT_MS)) { ESP_LOGW(TAG, "Dropped thermostat update: LVGL lock timed out"); return; @@ -1193,61 +1229,81 @@ void ui_update_thermostat(const therm_status_t *st) char buf[32]; - snprintf(buf, sizeof(buf), "%d°", (int)lroundf(st->setpoint_f)); - label_set_if_changed(setpoint_label, buf); - - thermostat_state_text(st, buf, sizeof(buf)); - label_set_if_changed(therm_state_label, buf); - - /* - * The dot colour is the at-a-glance channel: amber means the burner is - * lit, red means something is wrong and no heat is coming, dim means - * everything is fine and there is nothing to do. - */ - uint32_t dot = COL_TEXT_FAINT; - uint32_t text = COL_TEXT_DIM; - switch (st->state) { - case THERM_STATE_HEATING: - dot = COL_ON; - text = COL_ON; - break; - case THERM_STATE_LOCKOUT: - dot = COL_ACCENT_WARM; - text = COL_ACCENT_WARM; - break; - case THERM_STATE_NO_SENSOR: - dot = 0xE74C3C; - text = 0xE74C3C; - break; - default: - break; - } - lv_obj_set_style_bg_color(therm_state_dot, lv_color_hex(dot), LV_PART_MAIN); - lv_obj_set_style_text_color(therm_state_label, lv_color_hex(text), LV_PART_MAIN); - const bool heat_mode = (st->mode == THERM_MODE_HEAT); - label_set_if_changed(mode_btn_label, heat_mode ? "Heat" : "Off"); - lv_obj_set_style_bg_color(mode_btn, - lv_color_hex(heat_mode ? COL_ACCENT_WARM : COL_PANEL_HI), - LV_PART_MAIN); - lv_obj_set_style_text_color(mode_btn_label, - lv_color_hex(heat_mode ? 0x2A1A05 : COL_TEXT_DIM), - LV_PART_MAIN); + const bool mode_changed = first || st->mode != last_therm.mode; - const bool fan_on = (st->fan == THERM_FAN_ON); - label_set_if_changed(fan_btn_label, fan_on ? "Fan on" : "Fan auto"); - lv_obj_set_style_bg_color(fan_btn, - lv_color_hex(fan_on ? COL_ACCENT : COL_PANEL_HI), - LV_PART_MAIN); - lv_obj_set_style_text_color(fan_btn_label, - lv_color_hex(fan_on ? 0x05131F : COL_TEXT), - LV_PART_MAIN); + if (first || mode_changed || + (int)lroundf(st->setpoint_f) != (int)lroundf(last_therm.setpoint_f)) { + snprintf(buf, sizeof(buf), "%d°", (int)lroundf(st->setpoint_f)); + label_set_if_changed(setpoint_label, buf); - /* Setpoint greys out when the mode is off -- it still adjusts, it just - * is not controlling anything. */ - lv_obj_set_style_text_color(setpoint_label, - lv_color_hex(heat_mode ? COL_ACCENT_WARM : COL_TEXT_FAINT), - LV_PART_MAIN); + /* Greys out when the mode is off -- it still adjusts, it just is not + * controlling anything. */ + lv_obj_set_style_text_color(setpoint_label, + lv_color_hex(heat_mode ? COL_ACCENT_WARM + : COL_TEXT_FAINT), + LV_PART_MAIN); + } + + /* Text changes every second while a lockout counts down, but it is one + * small label rather than the whole card. */ + if (first || st->state != last_therm.state || + st->lockout_remaining_s != last_therm.lockout_remaining_s) { + thermostat_state_text(st, buf, sizeof(buf)); + label_set_if_changed(therm_state_label, buf); + } + + if (first || st->state != last_therm.state) { + /* + * The dot colour is the at-a-glance channel: amber means the burner + * is lit, warm orange means it wants to run but a protection timer + * says not yet, red means no heat is coming at all. + */ + uint32_t dot = COL_TEXT_FAINT; + uint32_t text = COL_TEXT_DIM; + switch (st->state) { + case THERM_STATE_HEATING: + dot = COL_ON; + text = COL_ON; + break; + case THERM_STATE_LOCKOUT: + dot = COL_ACCENT_WARM; + text = COL_ACCENT_WARM; + break; + case THERM_STATE_NO_SENSOR: + dot = 0xE74C3C; + text = 0xE74C3C; + break; + default: + break; + } + lv_obj_set_style_bg_color(therm_state_dot, lv_color_hex(dot), LV_PART_MAIN); + lv_obj_set_style_text_color(therm_state_label, lv_color_hex(text), LV_PART_MAIN); + } + + if (mode_changed) { + label_set_if_changed(mode_btn_label, heat_mode ? "Heat" : "Off"); + lv_obj_set_style_bg_color(mode_btn, + lv_color_hex(heat_mode ? COL_ACCENT_WARM : COL_PANEL_HI), + LV_PART_MAIN); + lv_obj_set_style_text_color(mode_btn_label, + lv_color_hex(heat_mode ? 0x2A1A05 : COL_TEXT_DIM), + LV_PART_MAIN); + } + + if (first || st->fan != last_therm.fan) { + const bool fan_on = (st->fan == THERM_FAN_ON); + label_set_if_changed(fan_btn_label, fan_on ? "Fan on" : "Fan auto"); + lv_obj_set_style_bg_color(fan_btn, + lv_color_hex(fan_on ? COL_ACCENT : COL_PANEL_HI), + LV_PART_MAIN); + lv_obj_set_style_text_color(fan_btn_label, + lv_color_hex(fan_on ? 0x05131F : COL_TEXT), + LV_PART_MAIN); + } + + last_therm = *st; + last_therm_valid = true; lvgl_port_unlock(); } @@ -1280,12 +1336,6 @@ void ui_init(void) lv_obj_set_flex_flow(scr, LV_FLEX_FLOW_COLUMN); lv_obj_remove_flag(scr, LV_OBJ_FLAG_SCROLLABLE); - /* Created paused; interactions reset-and-resume it. See schedule_repair. */ - repair_timer = lv_timer_create(repair_timer_cb, 600, NULL); - if (repair_timer != NULL) { - lv_timer_pause(repair_timer); - } - build_header(scr); build_body(scr); build_lights(scr);