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; +}