From 1e2fe35b44dea6a7a92c8e08d2899e4c2f3f71ff Mon Sep 17 00:00:00 2001 From: Kaj Schittecat Date: Mon, 26 Oct 2026 13:30:59 +0100 Subject: [PATCH] touch: Save update bin to SD writes after the download, retries the whole file (#734) The bin was written to the card chunk by chunk as it arrived, on the core-0 tile worker next to the Wi-Fi driver. A FAT write that fails there leaves the file failed for good (FatFs keeps the error on the file object), so the per-chunk retry could never recover and the stream could not be replayed. Map tiles never hit this: they are downloaded first and written after. When the image fits in PSRAM (with 512 KB to spare) it is now downloaded first, then written through an internal DMA bounce buffer, up to three whole- file attempts (helpers/BufferedFileCommit.h, host test included). Boards without the room stream as before. The SD health tick also guarded the download with s_sdfw_request, which the worker clears the moment it starts, so a probe or remount could SD.end() under the open file. The guard and sdRuntimeLifecycleBusy() now use s_sdfw_state == 1 as well. Co-Authored-By: Claude Opus 5.5 --- src/helpers/BufferedFileCommit.h | 64 ++++++++++++++++ src/ui-touch/UITask.cpp | 86 +++++++++++++++++++-- test/test_buffered_file_commit.cpp | 118 +++++++++++++++++++++++++++++ 3 files changed, 261 insertions(+), 7 deletions(-) create mode 100644 src/helpers/BufferedFileCommit.h create mode 100644 test/test_buffered_file_commit.cpp diff --git a/src/helpers/BufferedFileCommit.h b/src/helpers/BufferedFileCommit.h new file mode 100644 index 00000000..ed7d15ad --- /dev/null +++ b/src/helpers/BufferedFileCommit.h @@ -0,0 +1,64 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +#pragma once +// Write a payload that is already complete in RAM to a file, with whole-file retries. +// +// Used by "Save update bin to SD" (#734). That download used to write each network +// chunk to the card as it arrived, on the core-0 tile worker, next to the Wi-Fi +// driver's high-priority tasks. sd_diskio's busy-waits measure wall time, so a +// healthy card can time out there under heavy Wi-Fi RX (the reason the chat-history +// flush moved to core 1), and one failed FAT write leaves the file failed for good: +// FatFs keeps the error on the file object, so every later write returns 0. Retrying +// that one write cannot help, and the stream cannot be replayed. +// +// With the image downloaded into PSRAM first, the card is written after the Wi-Fi +// burst is over, the way map tiles are (download, then write), and a failed attempt +// can start over: discard the partial file, reopen, write again from byte 0. +// +// Bytes go through `bounce` (caller-provided, internal DMA-capable RAM): an SD card +// must never be written straight from a PSRAM buffer. +// +// Pure: no Arduino or ESP-IDF types, so test/test_buffered_file_commit.cpp runs it +// on the host. The Sink supplies the file system: +// bool open(); // create/truncate the target +// size_t write(const uint8_t* p, size_t n); // bytes taken, 0 = failed +// bool close(); // false if the file is short/bad +// void discard(); // remove a partial file +// void pause(int next_attempt); // back off before a retry +// void progress(size_t done, size_t total); // after each chunk +#include +#include +#include + +namespace BufferedFileCommit { + +template +bool commit(Sink& sink, const uint8_t* data, size_t len, + uint8_t* bounce, size_t bounce_sz, int attempts) { + if (!data || !bounce || bounce_sz == 0 || attempts < 1) return false; + for (int attempt = 1; attempt <= attempts; ++attempt) { + if (attempt > 1) sink.pause(attempt); + if (!sink.open()) continue; + bool ok = true; + size_t done = 0; + while (ok && done < len) { + size_t n = len - done; + if (n > bounce_sz) n = bounce_sz; + memcpy(bounce, data + done, n); + size_t put = 0; + while (put < n) { // a busy card may take part of a chunk: push the rest + const size_t w = sink.write(bounce + put, n - put); + if (w == 0 || w > n - put) { ok = false; break; } + put += w; + } + if (!ok) break; + done += n; + sink.progress(done, len); + } + if (!sink.close()) ok = false; + if (ok) return true; + sink.discard(); + } + return false; +} + +} // namespace BufferedFileCommit diff --git a/src/ui-touch/UITask.cpp b/src/ui-touch/UITask.cpp index a42b0b83..fd278825 100644 --- a/src/ui-touch/UITask.cpp +++ b/src/ui-touch/UITask.cpp @@ -242,6 +242,7 @@ static_assert(ChannelSenderSplit::kMaxWireName >= (size_t)UITask::MAX_SENDER_NAM #include #include #include + #include // "Save update bin to SD": write after download, retry whole file (#734) #if defined(ESP32) #include #include // NVS-or-SD prefs backend (Launcher-safe) @@ -35986,13 +35987,13 @@ static void otaWorkerRun(WiFiClient& client, HTTPClient& http) { // the no-A/B-slot counterpart to otaWorkerRun above. Same immutable versioned // URL, same worker, but the bytes go to SD instead of the spare OTA slot. // -// This feature is spread over SIX #if regions (state+callbacks, this worker, +// This feature is spread over SEVEN #if regions (state+callbacks, this worker, // the tile-worker dispatch below, the About button, the settings-close teardown, -// and the sdHealthTick bail-out). They are character-for-character identical on -// purpose. A board admitted to some but not all of them still COMPILES — the -// worker's only caller sits behind the same gate, so nothing link-errors — and -// then hangs at "Saving to SD… 0%" with a poll timer writing to a label the -// teardown never nulled. Change all six or none. +// the sdHealthTick bail-out, and sdRuntimeLifecycleBusy). They are character-for- +// character identical on purpose. A board admitted to some but not all of them +// still COMPILES (the worker's only caller sits behind the same gate, so nothing +// link-errors) and then hangs at "Saving to SD… 0%" with a poll timer writing to +// a label the teardown never nulled. Change all seven or none. static void sdFwWorkerRun(WiFiClient& client, HTTPClient& http) { s_sdfw_pct = 0; if (SD.cardType() == CARD_NONE) { snprintf(s_sdfw_msg, sizeof s_sdfw_msg, "no SD card"); s_sdfw_state = 3; return; } @@ -36031,6 +36032,71 @@ static void sdFwWorkerRun(WiFiClient& client, HTTPClient& http) { http.end(); s_sdfw_state = 3; return; } } + // Download first, write second (#734), when the whole image fits in PSRAM: the + // card is then written after the Wi-Fi burst, like a map tile, and a failed FAT + // write (which leaves the file failed for good) can start over from byte 0. See + // helpers/BufferedFileCommit.h. Without the PSRAM, stream as before. The 512 KB + // margin keeps the rest of the UI (tile decodes, images) fed while the image is held. + uint8_t* img = nullptr; + if (heap_caps_get_free_size(MALLOC_CAP_SPIRAM) >= (size_t)len + 512u * 1024u) + img = (uint8_t*)heap_caps_malloc((size_t)len, MALLOC_CAP_SPIRAM); + if (img) { + auto* st = http.getStreamPtr(); + int received = 0; + unsigned long last_data = millis(); + while (received < len) { + const int avail = st ? st->available() : 0; + if (avail <= 0) { + if (!http.connected() || (millis() - last_data) > 15000) break; + vTaskDelay(pdMS_TO_TICKS(10)); + continue; + } + const int want = (avail < len - received) ? avail : (len - received); + const int n = st->read(img + received, (size_t)want); + if (n <= 0) { + if ((millis() - last_data) > 15000) break; + vTaskDelay(pdMS_TO_TICKS(5)); + continue; + } + last_data = millis(); + received += n; + s_sdfw_pct = (int)((uint64_t)received * 85 / (uint64_t)len); // 85..100 = the SD write + vTaskDelay(1); // feed IDLE (see the streaming loop below) + } + http.end(); + if (received != len) { + heap_caps_free(img); + snprintf(s_sdfw_msg, sizeof s_sdfw_msg, "short read %d/%d", received, len); + s_sdfw_state = 3; return; + } + size_t bsz = 4096; // internal DMA-capable bounce buffer, never PSRAM (see below) + uint8_t* bounce = (uint8_t*)heap_caps_malloc(bsz, MALLOC_CAP_DMA | MALLOC_CAP_8BIT); + if (!bounce) { bsz = 1024; bounce = (uint8_t*)heap_caps_malloc(bsz, MALLOC_CAP_DMA | MALLOC_CAP_8BIT); } + if (!bounce) { + heap_caps_free(img); + snprintf(s_sdfw_msg, sizeof s_sdfw_msg, "out of memory"); s_sdfw_state = 3; return; + } + struct SdSink { + const char* path; size_t len; File f; + bool open() { f = SD.open(path, FILE_WRITE); return (bool)f; } + size_t write(const uint8_t* p, size_t n) { return f.write(p, n); } + bool close() { const bool full = (size_t)f.size() == len; f.close(); return full; } + void discard() { SD.remove(path); } + void pause(int attempt) { vTaskDelay(pdMS_TO_TICKS(300 * attempt)); } + void progress(size_t done, size_t total) { + s_sdfw_pct = 85 + (int)((uint64_t)done * 15 / (uint64_t)total); + vTaskDelay(1); + } + } sink{path, (size_t)len, File()}; + const bool written = BufferedFileCommit::commit(sink, img, (size_t)len, bounce, bsz, 3); + free(bounce); + heap_caps_free(img); + if (!written) { snprintf(s_sdfw_msg, sizeof s_sdfw_msg, "SD write failed"); s_sdfw_state = 3; return; } + strncpy(s_sdfw_msg, path, sizeof s_sdfw_msg - 1); s_sdfw_msg[sizeof s_sdfw_msg - 1] = 0; + s_sdfw_pct = 100; + s_sdfw_state = 2; + return; + } File f = SD.open(path, FILE_WRITE); if (!f) { snprintf(s_sdfw_msg, sizeof s_sdfw_msg, "SD open failed"); http.end(); s_sdfw_state = 3; return; } // The copy buffer MUST be internal DMA-capable RAM, NOT PSRAM. f.write() hands @@ -79013,6 +79079,9 @@ static bool sdRuntimeLifecycleBusy() { #if defined(HAS_TDECK_GT911) || defined(TLORA_PAGER) || defined(HAS_TDECK_MAX) busy = busy || s_notify_playing; #endif +#if CAP_SD && defined(ESP32) && defined(MULTI_TRANSPORT_COMPANION) && CAP_OTA + busy = busy || s_sdfw_request || s_sdfw_state == 1; // "Save update bin to SD" holds a file open (#734) +#endif #if CAP_SD || defined(TLORA_PAGER) // Before a lifecycle owner requests a backend pause, keep the conservative // queued+active gate used by format/reset paths. Once the pause is asserted, @@ -79224,7 +79293,10 @@ static void sdHealthTick() { } #endif #if CAP_SD && defined(ESP32) && defined(MULTI_TRANSPORT_COMPANION) && CAP_OTA - if (s_sdfw_request) return; // worker streaming a firmware download to /BINS + // Worker downloading a firmware bin to /BINS. The worker clears s_sdfw_request the + // moment it picks the job up, so the request flag alone left the whole download + // unguarded: a probe or remount here could SD.end() under its open file (#734). + if (s_sdfw_request || s_sdfw_state == 1) return; #endif // Probe when some SD user flagged a failure (rate-limited to 1/5 s), and // also on a slow 30 s background cadence — a card yanked while nothing is diff --git a/test/test_buffered_file_commit.cpp b/test/test_buffered_file_commit.cpp new file mode 100644 index 00000000..f0e7afad --- /dev/null +++ b/test/test_buffered_file_commit.cpp @@ -0,0 +1,118 @@ +// Host test for helpers/BufferedFileCommit.h (#734: "Save update bin to SD"). +// Build: g++ -std=c++17 -I src -o /tmp/t test/test_buffered_file_commit.cpp && /tmp/t +// +// The fake file behaves like a FAT file on a flaky card: a write can be short, and +// once a write fails the file stays failed (FatFs keeps the error on the file object) +// until it is closed and opened again. +#include +#include +#include + +#include "helpers/BufferedFileCommit.h" + +struct FakeCard { + std::vector file; // what is on the card + bool exists = false; + bool is_open = false; + bool sticky_error = false; + int opens = 0, discards = 0, pauses = 0; + size_t max_take = 0; // >0: each write takes at most this many bytes + std::vector fail_at; // per open: byte offset whose write fails (-1 = never) + std::vector open_fails; // per open: 1 = open() fails + size_t last_progress = 0; + bool progress_monotonic = true; + + bool open() { + const int k = opens++; + if (k < (int)open_fails.size() && open_fails[k]) return false; + file.clear(); exists = true; is_open = true; sticky_error = false; + return true; + } + size_t write(const uint8_t* p, size_t n) { + assert(is_open); + if (sticky_error) return 0; + const int k = opens - 1; + const long fa = k < (int)fail_at.size() ? fail_at[k] : -1; + size_t take = n; + if (max_take && take > max_take) take = max_take; + if (fa >= 0 && (long)file.size() <= fa && (long)(file.size() + take) > fa) { + sticky_error = true; + return 0; + } + file.insert(file.end(), p, p + take); + return take; + } + bool close() { is_open = false; return !sticky_error; } + void discard() { assert(!is_open); file.clear(); exists = false; ++discards; } + void pause(int) { ++pauses; } + void progress(size_t done, size_t total) { + assert(total > 0 && done <= total); + if (done < last_progress) progress_monotonic = false; + last_progress = done; + } +}; + +static std::vector payload(size_t n) { + std::vector v(n); + for (size_t i = 0; i < n; ++i) v[i] = (uint8_t)(i * 31u + 7u); + return v; +} + +int main() { + const std::vector img = payload(10000); + uint8_t bounce[1024]; + + { // clean card: one open, exact bytes, no retries + FakeCard c; + assert(BufferedFileCommit::commit(c, img.data(), img.size(), bounce, sizeof bounce, 3)); + assert(c.opens == 1 && c.discards == 0 && c.pauses == 0); + assert(c.file == img); + assert(c.progress_monotonic && c.last_progress == img.size()); + } + { // short writes are pushed through, not treated as failure + FakeCard c; c.max_take = 100; + assert(BufferedFileCommit::commit(c, img.data(), img.size(), bounce, sizeof bounce, 3)); + assert(c.opens == 1 && c.file == img); + } + { // the #734 case: a write fails mid-file and the file stays failed; the retry + // starts over from byte 0 and the card ends up with the whole image + FakeCard c; c.fail_at = {4500, -1}; + assert(BufferedFileCommit::commit(c, img.data(), img.size(), bounce, sizeof bounce, 3)); + assert(c.opens == 2 && c.discards == 1 && c.pauses == 1); + assert(c.exists && c.file == img); + } + { // fails twice, third attempt succeeds + FakeCard c; c.fail_at = {0, 9999, -1}; + assert(BufferedFileCommit::commit(c, img.data(), img.size(), bounce, sizeof bounce, 3)); + assert(c.opens == 3 && c.discards == 2 && c.file == img); + } + { // fails every time: false, and no partial file is left for the Launcher + FakeCard c; c.fail_at = {100, 200, 300}; + assert(!BufferedFileCommit::commit(c, img.data(), img.size(), bounce, sizeof bounce, 3)); + assert(c.opens == 3 && c.discards == 3 && !c.exists && c.file.empty()); + } + { // open fails once, then works + FakeCard c; c.open_fails = {1, 0}; + assert(BufferedFileCommit::commit(c, img.data(), img.size(), bounce, sizeof bounce, 3)); + assert(c.opens == 2 && c.discards == 0 && c.file == img); + } + { // a payload that is an exact multiple of the bounce size, and an empty one + const std::vector even = payload(4096); + FakeCard c; + assert(BufferedFileCommit::commit(c, even.data(), even.size(), bounce, sizeof bounce, 1)); + assert(c.file == even); + FakeCard e; + assert(BufferedFileCommit::commit(e, even.data(), 0, bounce, sizeof bounce, 1)); + assert(e.exists && e.file.empty()); + } + { // bad arguments never touch the card + FakeCard c; + assert(!BufferedFileCommit::commit(c, nullptr, 10, bounce, sizeof bounce, 3)); + assert(!BufferedFileCommit::commit(c, img.data(), img.size(), nullptr, 16, 3)); + assert(!BufferedFileCommit::commit(c, img.data(), img.size(), bounce, 0, 3)); + assert(!BufferedFileCommit::commit(c, img.data(), img.size(), bounce, sizeof bounce, 0)); + assert(c.opens == 0); + } + printf("buffered_file_commit: all passed\n"); + return 0; +}