From c917bf59fdd4040796ba54b235ddce169ea1fec4 Mon Sep 17 00:00:00 2001 From: MarekZegare4 Date: Wed, 24 Jun 2026 21:39:46 +0200 Subject: [PATCH] fix(companion): atomic save for contacts/channels/prefs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit saveContacts/saveChannels/savePrefs truncated the live file up front and then wrote in place, so a crash, reset or full flash mid-save wiped the whole file — observed as all contacts disappearing after a UI hang. Write to a temp file and only swap it in (via LittleFS atomic rename) once every record has written cleanly; on any failure keep the previous good file and drop the temp. savePrefs gates the swap on the tail-sentinel write succeeding (writes return 0 once flash fills, so a good sentinel means the whole record fit). Contacts/channels live on the multi-MB QSPIFlash, so the transient second copy fits easily. Co-Authored-By: Claude Opus 4.8 --- examples/companion_radio/DataStore.cpp | 145 ++++++++++++++++--------- 1 file changed, 96 insertions(+), 49 deletions(-) diff --git a/examples/companion_radio/DataStore.cpp b/examples/companion_radio/DataStore.cpp index 9ab113e6..4126b5f9 100644 --- a/examples/companion_radio/DataStore.cpp +++ b/examples/companion_radio/DataStore.cpp @@ -44,6 +44,21 @@ static File openWrite(FILESYSTEM* fs, const char* filename) { #endif } +// Atomically swap a fully-written temp file over its final path. LittleFS +// (nRF52/STM32) rename replaces an existing destination atomically, so a crash +// leaves either the old file or the new one intact — never a truncated mix. +// Other Arduino filesystems can't rename onto an existing file, so the +// destination is dropped first (a metadata-only window, vs. the record-by-record +// write window of a direct overwrite). Returns false if the swap fails. +static bool commitTempFile(FILESYSTEM* fs, const char* tmp, const char* final_path) { +#if defined(NRF52_PLATFORM) || defined(STM32_PLATFORM) + return fs->rename(tmp, final_path); +#else + fs->remove(final_path); + return fs->rename(tmp, final_path); +#endif +} + #if defined(NRF52_PLATFORM) || defined(STM32_PLATFORM) static uint32_t _ContactsChannelsTotalBlocks = 0; #endif @@ -474,7 +489,9 @@ void DataStore::loadPrefsInt(const char *filename, NodePrefs& _prefs, double& no } void DataStore::savePrefs(const NodePrefs& _prefs, double node_lat, double node_lon) { - File file = ::openWrite(_fs, "/new_prefs"); + // Atomic temp-then-rename (see commitTempFile) so an interrupted save can't + // wipe settings; loadPrefs() still validates the tail sentinel on read. + File file = ::openWrite(_fs, "/new_prefs.tmp"); if (file) { uint8_t pad[8]; memset(pad, 0, sizeof(pad)); @@ -598,11 +615,18 @@ void DataStore::savePrefs(const NodePrefs& _prefs, double node_lat, double node_ file.write((uint8_t *)&_prefs.locator_target_kind, sizeof(_prefs.locator_target_kind)); file.write((uint8_t *)_prefs.locator_key, sizeof(_prefs.locator_key)); - // Tail sentinel — must be last. See NodePrefs::SCHEMA_SENTINEL. + // Tail sentinel — must be last. See NodePrefs::SCHEMA_SENTINEL. Its write is + // the one we check: once the flash fills, writes return 0, so a good + // sentinel write means the whole record fit. Only then swap it in. uint32_t sentinel = NodePrefs::SCHEMA_SENTINEL; - file.write((uint8_t *)&sentinel, sizeof(sentinel)); + bool ok = (file.write((uint8_t *)&sentinel, sizeof(sentinel)) == sizeof(sentinel)); file.close(); + if (ok) { + commitTempFile(_fs, "/new_prefs.tmp", "/new_prefs"); + } else { + _fs->remove("/new_prefs.tmp"); // keep the previous good /new_prefs + } } } @@ -658,35 +682,48 @@ File file = openRead(_getContactsChannelsFS(), "/contacts3"); } void DataStore::saveContacts(DataStoreHost* host, bool (*filter)(const ContactInfo& c)) { - File file = ::openWrite(_getContactsChannelsFS(), "/contacts3"); - if (file) { - uint32_t idx = 0; - ContactInfo c; - uint8_t unused = 0; + FILESYSTEM* fs = _getContactsChannelsFS(); + // Write to a temp file, then atomically rename it over /contacts3 only once + // every record has written cleanly. The old code truncated /contacts3 up + // front and wrote in place, so a crash, reset or full flash mid-save wiped + // the entire contact list. Now an interrupted save leaves the previous good + // file untouched. + File file = ::openWrite(fs, "/contacts3.tmp"); + if (!file) return; - while (host->getContactForSave(idx, c)) { - if (filter && !filter(c)) { - idx++; // advance to next contact - continue; - } - bool success = (file.write(c.id.pub_key, 32) == 32); - success = success && (file.write((uint8_t *)&c.name, 32) == 32); - success = success && (file.write(&c.type, 1) == 1); - success = success && (file.write(&c.flags, 1) == 1); - success = success && (file.write(&unused, 1) == 1); - success = success && (file.write((uint8_t *)&c.sync_since, 4) == 4); - success = success && (file.write((uint8_t *)&c.out_path_len, 1) == 1); - success = success && (file.write((uint8_t *)&c.last_advert_timestamp, 4) == 4); - success = success && (file.write(c.out_path, 64) == 64); - success = success && (file.write((uint8_t *)&c.lastmod, 4) == 4); - success = success && (file.write((uint8_t *)&c.gps_lat, 4) == 4); - success = success && (file.write((uint8_t *)&c.gps_lon, 4) == 4); - - if (!success) break; // write failed + bool ok = true; + uint32_t idx = 0; + ContactInfo c; + uint8_t unused = 0; + while (host->getContactForSave(idx, c)) { + if (filter && !filter(c)) { idx++; // advance to next contact + continue; } - file.close(); + bool success = (file.write(c.id.pub_key, 32) == 32); + success = success && (file.write((uint8_t *)&c.name, 32) == 32); + success = success && (file.write(&c.type, 1) == 1); + success = success && (file.write(&c.flags, 1) == 1); + success = success && (file.write(&unused, 1) == 1); + success = success && (file.write((uint8_t *)&c.sync_since, 4) == 4); + success = success && (file.write((uint8_t *)&c.out_path_len, 1) == 1); + success = success && (file.write((uint8_t *)&c.last_advert_timestamp, 4) == 4); + success = success && (file.write(c.out_path, 64) == 64); + success = success && (file.write((uint8_t *)&c.lastmod, 4) == 4); + success = success && (file.write((uint8_t *)&c.gps_lat, 4) == 4); + success = success && (file.write((uint8_t *)&c.gps_lon, 4) == 4); + + if (!success) { ok = false; break; } // write failed (e.g. flash full) + + idx++; // advance to next contact + } + file.close(); + + if (ok) { + commitTempFile(fs, "/contacts3.tmp", "/contacts3"); + } else { + fs->remove("/contacts3.tmp"); // keep the previous good /contacts3 } } @@ -738,29 +775,39 @@ void DataStore::loadChannels(DataStoreHost* host) { } void DataStore::saveChannels(DataStoreHost* host) { - File file = ::openWrite(_getContactsChannelsFS(), "/channels2"); - if (file) { - uint8_t channel_idx = 0; - ChannelDetails ch; - uint8_t unused[4]; - memset(unused, 0, 4); + FILESYSTEM* fs = _getContactsChannelsFS(); + // Same atomic temp-then-rename pattern as saveContacts() — never truncate the + // live /channels2 before the new copy is fully written. + File file = ::openWrite(fs, "/channels2.tmp"); + if (!file) return; - while (host->getChannelForSave(channel_idx, ch)) { - channel_idx++; - // getChannelForSave() returns every slot up to MAX_GROUP_CHANNELS, so skip - // the unused ones (all-zero secret) rather than writing all 40 — otherwise - // the file is always ~2.7 KB and wears the flash needlessly. loadChannels() - // already compacts empty entries on read, so the loaded result is identical. - bool empty = true; - for (int b = 0; b < 32; b++) if (ch.channel.secret[b]) { empty = false; break; } - if (empty) continue; + bool ok = true; + uint8_t channel_idx = 0; + ChannelDetails ch; + uint8_t unused[4]; + memset(unused, 0, 4); - bool success = (file.write(unused, 4) == 4); - success = success && (file.write((uint8_t *)ch.name, 32) == 32); - success = success && (file.write((uint8_t *)ch.channel.secret, 32) == 32); - if (!success) break; // write failed - } - file.close(); + while (host->getChannelForSave(channel_idx, ch)) { + channel_idx++; + // getChannelForSave() returns every slot up to MAX_GROUP_CHANNELS, so skip + // the unused ones (all-zero secret) rather than writing all 40 — otherwise + // the file is always ~2.7 KB and wears the flash needlessly. loadChannels() + // already compacts empty entries on read, so the loaded result is identical. + bool empty = true; + for (int b = 0; b < 32; b++) if (ch.channel.secret[b]) { empty = false; break; } + if (empty) continue; + + bool success = (file.write(unused, 4) == 4); + success = success && (file.write((uint8_t *)ch.name, 32) == 32); + success = success && (file.write((uint8_t *)ch.channel.secret, 32) == 32); + if (!success) { ok = false; break; } // write failed + } + file.close(); + + if (ok) { + commitTempFile(fs, "/channels2.tmp", "/channels2"); + } else { + fs->remove("/channels2.tmp"); // keep the previous good /channels2 } }