From face2432e42b1d10e83646c173ff052e4e98cbeb Mon Sep 17 00:00:00 2001 From: matt_ed Date: Tue, 4 Aug 2026 09:09:30 -0700 Subject: [PATCH] Update TODO.md for the completed review items Records what d4b2ab7 did and what genuinely remains: confirming the left-edge artefacts by eye (the timing win was measured, the visual result was not), OTA download logic, optional backlight scheduling, and the thermostat itself. Co-Authored-By: Claude --- TODO.md | 203 +++++++++++++++++++++----------------------------------- 1 file changed, 74 insertions(+), 129 deletions(-) diff --git a/TODO.md b/TODO.md index 300e719..3d9a1ae 100644 --- a/TODO.md +++ b/TODO.md @@ -1,134 +1,79 @@ -# TODO — remaining items from the 2026-08-03 code review +# TODO -Context for whoever (human or AI agent) picks this up. A full review of this -firmware was done on 2026-08-03. The outright bugs it found were fixed in -commit `dd2d428` ("Fix WiFi reconnect bugs, negative-temp rounding, and stale -docs"). This file holds the items that were deliberately **not** done in that -commit — design-level changes and enhancements — plus the facts the review -already established so you don't re-derive or re-ask them. +## Done -Read `README.md` first. It is accurate as of `dd2d428` and documents the -hardware constraints that make this board unusual (PSRAM bandwidth, the -STC8H1K28 board controller, init-order requirements). Do not violate the -init order in `main.c`: WiFi/NVS before `display_init()`, `backlight_init()` -before `touch_init()`. +The six items from the 2026-08-03 review were completed in `d4b2ab7` +("Work through the remaining review items in TODO.md"). See that commit +message for the reasoning behind each. Summary: -## Already verified — do not re-investigate +1. **UI pause window shrunk** — HTTP session kept alive between polls, timeout + drops 30s→10s once warm. Measured on hardware: fetch went from ~1.3s to + ~80ms, so `lvgl_port_pause()` no longer blocks rendering and touch for a + meaningful period. +2. **Stale readings indicated** — temperature dims and the status line shows + age after two consecutive failures. +3. **Touch readout gated** behind `UI_DEBUG_TOUCH`, default off. +4. **`main/utils` deleted** — no callers. +5. **OTA partitions laid out** — two 4MB slots, `otadata`, ~7.9MB unallocated. +6. **Cleanups** — truncation logged once per response; weather task stack + high-water mark measured (~2400 bytes spare of 6144). -- The build compiles clean on ESP-IDF v5.3 (`. ~/esp/esp-idf/export.sh && - idf.py build`). Binary is ~1.1 MB, 74% of the 4 MB factory partition free. -- `main/app_config.h` exists locally (gitignored, copied from the example - with empty credentials, done only to make the build compile). Real WiFi + - Ambient Weather credentials still need to be filled in before flashing. -- `resp_buf` bounds/NUL-termination in `ambient_weather.c` is correct — the - guard leaves room for the terminator. Not a bug. -- GT911 read-then-clear-status ordering in `touch_gt911.c` is correct. -- Backlight duty inversion in `backlight.c` maps correctly at both endpoints - (255→0x00 full, 0→0xF5 off). +Verified on hardware after the change: boots from the new OTA layout, three +consecutive successful polls at 60s, no reboots. + +## Still open + +### 1. Confirm the left-edge artefacts are actually gone + +The pause window is now ~80ms instead of ~1.3s, which should make the +artefacts much rarer — but **this has not been confirmed by eye**. The +timing improvement was measured from the log; the artefacts can only be +verified by watching the panel around the moment the status line updates. + +If they persist, the remaining lever is pausing only around LVGL's flush (the +actual PSRAM writes) rather than the whole network transaction. That is more +invasive; do not start it until the current change has been observed to be +insufficient. + +### 2. OTA implementation + +Partition layout exists; the download/apply logic does not. **This is not a +drop-in.** An OTA write is a flash write, and the bounce-buffer refill ISR +reads PSRAM, which shares the cache with flash on the S3 — downloading while +the panel scans will fault it with "Cache disabled but cached memory region +accessed". The implementation must stop the panel for the download and write, +or reboot into a dedicated updater mode. See README "Partitions". + +### 3. Backlight control (optional feature) + +`backlight_set()` is only ever called once, with 255. A night-dim schedule or +touch-to-wake is a natural small feature. Note the STC8H1K28 shares the I2C +bus with the GT911, so brightness writes must stay in task context (never an +ISR) and will contend with touch polls on the same bus. + +### 4. The actual thermostat + +Setpoint, HVAC relay control, scheduling and persistence are all still +unimplemented — this remains a weather display. Note a separate project +(`~/workspace/rotaryThermo`) has a PRD for building this on the CrowPanel 2.1" +rotary display instead, driven by Zigbee room sensors. Decide which board +hosts the thermostat before building it twice. + +## Facts already established — do not re-investigate + +- Build is clean on ESP-IDF v5.3 (`. ~/esp/esp-idf/export.sh && idf.py build`). + App is ~1.4MB in a 4MB OTA slot. +- `main/app_config.h` is gitignored and holds real credentials locally. +- `resp_buf` bounds/NUL-termination in `ambient_weather.c` is correct. +- GT911 read-then-clear-status ordering is correct. +- Backlight duty inversion maps correctly at both endpoints (255→0x00 full, + 0→0xF5 off). - The `°` glyph exists in LVGL's built-in Montserrat fonts; the `·` separator - that does NOT exist in them only appears in log strings, never on screen. -- The LVGL recursive-mutex locking discipline is consistent across all UI - entry points. -- WiFi reconnect, `wifi_is_connected()` staleness, boot-timeout handling, and - negative-temperature rounding were all fixed in `dd2d428`. Don't redo them. - -## Tasks, in priority order - -### 1. Stop freezing the UI during weather fetches - -**Problem:** `weather_update_task` in `main.c` wraps the whole HTTP fetch in -`lvgl_port_pause()`/`resume()`. Pause stops `lv_timer_handler()` entirely, so -rendering AND touch input are dead for the whole fetch — worst case -`WEATHER_TIMEOUT_MS` (30 s), every 60 s. Harmless while the screen is -passive; a showstopper once thermostat controls (setpoint buttons) exist, -which is the stated next step in the README. - -**Why the pause exists:** the TLS session, the LVGL redraw, and the flush all -contend with the LCD's own DMA out of PSRAM; overlapping them produces the -left-edge artefacts documented in the README. Do not simply delete the pause -— the artefacts will come back. Options, roughly in order of preference: - -- Keep the HTTP client alive between polls (`keep_alive_enable` in - `esp_http_client_config_t`, don't `cleanup` between calls). The expensive - part is the TLS handshake; a reused session makes each poll a short - request/response, so the paused window shrinks from seconds to ~100s of ms. -- After the first successful fetch, drop the timeout from 30 s to ~10 s (the - 30 s exists only because a *first* handshake on this board is slow). -- Alternatively pause only around LVGL's flush (the actual PSRAM writes) - rather than the whole network transaction — more invasive, only if the - above is insufficient. - -**Verify on hardware**: watch for the left-edge artefacts after weather -fetches (they appear near the moment the status line updates). This cannot be -verified by compiling alone. - -### 2. Indicate stale weather data - -`weather_data_t.last_update` is set in `ambient_weather.c` but never read. -After fetch failures the old temperature stays on screen at full styling -indefinitely. Track the last successful update (the weather task is the -natural owner); after ~2 missed polls, dim the temperature label -(e.g. change its text color) or append the age to the status line. Keep it in -the weather task / `ui_manager` — do not add timekeeping to the API client. - -### 3. Remove the debug touch readout from the production UI - -`touch_label` + `touch_readout_timer_cb` + the 100 ms `lv_timer` in -`ui_manager.c` are bring-up diagnostics baked into the shipped screen. Remove -them or gate behind a compile-time flag (e.g. `#ifdef UI_DEBUG_TOUCH`, -default off). Keep the explanatory comment about LVGL not bubbling events to -parents somewhere — it documents a real trap that cost debugging time. - -### 4. Delete dead code: `main/utils/utils.c` - -`utils_delay_ms`, `utils_get_tick_ms`, `utils_trim` have zero callers; -nothing outside utils.c includes `utils.h`. Delete `main/utils/` and remove -its entries from `main/CMakeLists.txt` (SRCS and INCLUDE_DIRS), and drop the -row from README's Components table and from `project_structure.txt`. -`touch_get_indev()`, `display_get_panel()`, `lvgl_port_is_paused()` are also -currently uncalled but were deliberately KEPT as driver API surface — leave -them. - -### 5. Add OTA partitions while the fleet is size zero - -16 MB flash, but `partitions.csv` has a single 4 MB factory app and a ~12 MB -SPIFFS partition nothing mounts. Re-partitioning fielded devices later means -reflashing over USB, so lay out `otadata` + `ota_0` + `ota_1` now (two 4 MB -slots fit easily; shrink or drop the unused SPIFFS partition). Note -`CONFIG_ESP_WIFI_NVS_ENABLED=n` and the flash-write-while-panel-running -constraint (README "Memory notes"): an OTA *write* happens while the panel is -scanning, which will fault the bounce-buffer ISR. Any future OTA -implementation must pause/stop the panel during the download+write, or -restart into a dedicated updater mode. Adding the partition layout now is -safe; implementing OTA download is a separate, harder task. - -### 6. Small cleanups (batch into one commit) - -- `ambient_weather.c` truncation path: the "Response exceeds N bytes" warning - logs once per remaining chunk and then JSON parse fails anyway. Log once - (set a flag) and consider aborting the transfer. -- Weather task stack is 6144 bytes with the TLS handshake running in-task. - Works (mbedTLS heap goes to PSRAM via `CONFIG_MBEDTLS_EXTERNAL_MEM_ALLOC`) - but tight — log `uxTaskGetStackHighWaterMark(NULL)` once after a successful - fetch and right-size before more features land in that task. -- `backlight_set()` is only ever called once with 255. A night-dim schedule - or touch-to-wake is a natural small feature; note the STC8H1K28 shares the - I2C bus with the GT911, so calls must stay in task context (never ISRs) and - brightness writes contend with touch polls on the same bus. - -## Questions already asked and answered (don't re-ask the user) - -- **"Should the bugs be fixed or just reported?"** — Answered: fix and - commit. That happened in `dd2d428`. The items in this file were explicitly - left for later; the user has seen this list and asked for it to be recorded - here. Proceeding with these tasks does not need re-confirmation, but - hardware-verification steps (task 1) need a human with the board. -- **"Is the bounce buffer supposed to be enabled?"** — Yes. The README - previously contradicted the code; fixed in `dd2d428`. It is enabled and - load-bearing. -- **"Is the board N4R8 or N16R8?"** — N16R8 (16 MB flash / 8 MB octal - PSRAM). The old sdkconfig header comment saying N4R8 was stale; fixed. -- **"Why is WiFi brought up before the display?"** — Deliberate and - required; see README "Gotchas". Never reorder. -- **"Why is rendering paused during fetches?"** — Deliberate mitigation for - PSRAM bus contention (see task 1). Improve it, don't just remove it. + that does not appears only in log strings, never on screen. +- LVGL recursive-mutex locking is consistent across all UI entry points. +- Init order in `main.c` is deliberate and load-bearing: WiFi/NVS before + `display_init()`, `backlight_init()` before `touch_init()`. Never reorder. +- The bounce buffer is enabled and load-bearing. +- Board is N16R8 (16MB flash / 8MB octal PSRAM). +- `touch_get_indev()`, `display_get_panel()` and `lvgl_port_is_paused()` are + uncalled but deliberately kept as driver API surface.