Add TODO.md handoff notes for the remaining review items
Records the design-level suggestions from the 2026-08-03 review that were
not part of dd2d428 (UI freeze during fetches, stale-data indication,
debug readout removal, dead utils.c, OTA partitions, small cleanups),
plus the facts already verified and questions already answered so a
future agent does not re-derive or re-ask them.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
1 parent
dd2d4283e2
commit
5bbb50c4eb
1 file changed
+134
@@ -0,0 +1,134 @@
|
|||||||
|
# TODO — remaining items from the 2026-08-03 code review
|
||||||
|
|
||||||
|
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.
|
||||||
|
|
||||||
|
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()`.
|
||||||
|
|
||||||
|
## Already verified — do not re-investigate
|
||||||
|
|
||||||
|
- 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).
|
||||||
|
- 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.
|
||||||
Reference in new issue
Block a user