From 6ce784bbd6471b1e2c38e35eea113d572b37ee4f Mon Sep 17 00:00:00 2001 From: mikecarper Date: Sun, 18 Oct 2026 09:11:51 -0700 Subject: [PATCH] Fit all STM32 Companions with checked fractional preferences Stage STM32 preference prefixes and checked footers in one atomic image, reject declared tails that encounter premature EOF, and share the typed CLI rollback helper. Use the proven Wio Mini code-layout flags on Wio E5 and Tiny Relay without changing features or memory boundaries. All four STM32 targets and RAK4631 Full pass at the full 1.18.0.1 release version length. --- examples/companion_radio/DataStore.cpp | 83 +++++++++++++++----------- examples/companion_radio/MyMesh.cpp | 9 +-- test/test_companion_prefs_image.py | 46 +++++++++++++- test/test_companion_prefs_values.py | 8 +-- test/test_companion_timezone_prefs.py | 21 +++++++ test/test_local_cli_access.py | 24 +++++++- variants/tiny_relay/platformio.ini | 4 ++ variants/wio-e5-dev/platformio.ini | 4 ++ 8 files changed, 149 insertions(+), 50 deletions(-) diff --git a/examples/companion_radio/DataStore.cpp b/examples/companion_radio/DataStore.cpp index b63f0e255..8092b877d 100644 --- a/examples/companion_radio/DataStore.cpp +++ b/examples/companion_radio/DataStore.cpp @@ -1420,22 +1420,23 @@ bool DataStore::loadPrefsInt(const char *filename, } #endif - // The compact ARM table and the explicit readers both end at the - // unchanged historical prefix. Consume the common footer only afterward. - if (file.available() != 0) { - uint8_t traffic_footer[mesh::companion::traffic::FooterSize] = {}; - success = success - && file.read(traffic_footer, sizeof(traffic_footer)) == sizeof(traffic_footer) - && mesh::companion::traffic::decodeFooter(traffic_footer, - sizeof(traffic_footer), loaded_prefs.tx_media, loaded_prefs.tx_ssh); - } - - if (prefs_size == TIMEZONE_PREFS_SIZE) { - uint8_t timezone_footer[mesh::companion::timezone::FooterSize] = {}; - success = success - && file.read(timezone_footer, sizeof(timezone_footer)) == sizeof(timezone_footer) - && mesh::companion::timezone::decodeFooter(timezone_footer, - sizeof(timezone_footer), loaded_prefs.tz_offset); + // Both readers end at the frozen prefix. Read the existing TXR and + // optional TZF records together; their wire positions and CRCs stay + // independent. A short read never reaches either decoder. Use the declared + // known image size so a premature available()==0 cannot skip validation. + if (prefs_size >= TIMEZONE_PREFS_SIZE - mesh::companion::timezone::FooterSize) { + uint8_t footers[mesh::companion::traffic::FooterSize + + mesh::companion::timezone::FooterSize]; + const bool fractional = prefs_size == TIMEZONE_PREFS_SIZE; + const size_t length = fractional ? sizeof(footers) + : mesh::companion::traffic::FooterSize; + success = success && file.read(footers, length) == length + && mesh::companion::traffic::decodeFooter(footers, + mesh::companion::traffic::FooterSize, + loaded_prefs.tx_media, loaded_prefs.tx_ssh) + && (!fractional || mesh::companion::timezone::decodeFooter( + footers + mesh::companion::traffic::FooterSize, + mesh::companion::timezone::FooterSize, loaded_prefs.tz_offset)); } // Any bytes left over form only part of a historically appended field. @@ -1474,15 +1475,20 @@ bool DataStore::savePrefs(const CompanionNodePrefs& _prefs, double node_lat, dou File file = openWrite(_fs, "/new_prefs"); #endif if (file) { + static constexpr size_t MAX_PREFS_FOOTER_SIZE = + mesh::companion::traffic::FooterSize + mesh::companion::timezone::FooterSize; +#if defined(STM32_PLATFORM) + static constexpr size_t PREFS_PREFIX_SIZE = 236; + uint8_t image[PREFS_PREFIX_SIZE + MAX_PREFS_FOOTER_SIZE]; +#endif #if defined(STM32_PLATFORM) && defined(__GNUC__) using namespace mesh::companion_prefs; - uint8_t image[236]; const uint8_t pad[4] = {}; size_t used = 0; for (const PrefField& field : PREF_FIELDS) { - if (field.length > sizeof(image) - used) { + if (field.length > PREFS_PREFIX_SIZE - used) { // Latch failure and stop before any out-of-bounds pointer arithmetic. - used = sizeof(image) + 1; + used = PREFS_PREFIX_SIZE + 1; break; } memcpy(image + used, prefValue(field, _prefs, node_lat, node_lon, pad), @@ -1492,8 +1498,7 @@ bool DataStore::savePrefs(const CompanionNodePrefs& _prefs, double node_lat, dou // STM32 stages the frozen prefix: patch its published integer hour once, // rather than selecting an alternate source for every table entry. image[225] = static_cast(legacy_tz_offset); - bool success = used == sizeof(image) - && file.write(image, sizeof(image)) == sizeof(image); + bool success = used == PREFS_PREFIX_SIZE; #elif defined(NRF52_PLATFORM) && defined(__GNUC__) // nRF52 retains streaming writes and its existing small stack footprint. // The table is the same validated wire order used by the scalar emitter. @@ -1510,15 +1515,14 @@ bool DataStore::savePrefs(const CompanionNodePrefs& _prefs, double node_lat, dou #if defined(STM32_PLATFORM) // Keep the explicit wire field list, but make one bounded atomic write. // Other platforms retain direct writes and their existing stack footprint. - uint8_t image[236]; size_t image_length = 0; const auto writeField = [&](const void* data, size_t size) -> bool { - if (image_length > sizeof(image) - || size > sizeof(image) - image_length) { + if (image_length > PREFS_PREFIX_SIZE + || size > PREFS_PREFIX_SIZE - image_length) { // Latch failure without reading data or advancing past the buffer. // The return value only preserves the common emitter interface; // the exact-length check below is the authoritative write gate. - image_length = sizeof(image) + 1; + image_length = PREFS_PREFIX_SIZE + 1; return true; } memcpy(image + image_length, data, size); @@ -1648,8 +1652,7 @@ bool DataStore::savePrefs(const CompanionNodePrefs& _prefs, double node_lat, dou sizeof(_prefs.gps_sync_interval_hours)); success = success && writeField(tail, sizeof(tail)); #if defined(STM32_PLATFORM) - success = success && image_length == sizeof(image) - && file.write(image, sizeof(image)) == sizeof(image); + success = success && image_length == PREFS_PREFIX_SIZE; #endif #endif #if defined(RAK_4631) @@ -1658,19 +1661,27 @@ bool DataStore::savePrefs(const CompanionNodePrefs& _prefs, double node_lat, dou success = success && file.write(peripheral_footer, sizeof(peripheral_footer)) == sizeof(peripheral_footer); #endif - uint8_t traffic_footer[mesh::companion::traffic::FooterSize]; - mesh::companion::traffic::encodeFooter(traffic_footer, - _prefs.tx_media, _prefs.tx_ssh); - success = success - && file.write(traffic_footer, sizeof(traffic_footer)) == sizeof(traffic_footer); +#if defined(STM32_PLATFORM) + // Keep the frozen prefix bound separate from this larger image. Append + // the checked records in place, then stage the exact 244/254-byte image. + uint8_t* const footers = image + PREFS_PREFIX_SIZE; +#else + uint8_t footers[MAX_PREFS_FOOTER_SIZE]; +#endif + mesh::companion::traffic::encodeFooter(footers, _prefs.tx_media, _prefs.tx_ssh); + size_t footer_length = mesh::companion::traffic::FooterSize; // Range was checked before opening the transaction. Reuse its integer // fallback instead of validating and converting the same float again. if (_prefs.tz_offset != legacy_tz_offset) { - uint8_t timezone_footer[mesh::companion::timezone::FooterSize]; - mesh::companion::timezone::encodeFooter(timezone_footer, _prefs.tz_offset); - success = success - && file.write(timezone_footer, sizeof(timezone_footer)) == sizeof(timezone_footer); + mesh::companion::timezone::encodeFooter(footers + footer_length, _prefs.tz_offset); + footer_length = MAX_PREFS_FOOTER_SIZE; } +#if defined(STM32_PLATFORM) + const size_t image_length_with_footers = PREFS_PREFIX_SIZE + footer_length; + success = success && file.write(image, image_length_with_footers) == image_length_with_footers; +#else + success = success && file.write(footers, footer_length) == footer_length; +#endif #if defined(NRF52_PLATFORM) || defined(STM32_PLATFORM) || defined(ESP32_PLATFORM) || defined(RP2040_PLATFORM) success = file.commit(success); if (!success) MESH_DEBUG_PRINTLN("DataStore: atomic preferences write failed"); diff --git a/examples/companion_radio/MyMesh.cpp b/examples/companion_radio/MyMesh.cpp index a88f42718..bea304921 100644 --- a/examples/companion_radio/MyMesh.cpp +++ b/examples/companion_radio/MyMesh.cpp @@ -11003,13 +11003,8 @@ bool MyMesh::handleCommand(const char* command, uint32_t sender_timestamp, || !mesh::companion::timezone::valid(tz)) { strcpy(reply, "Error, must be from -12 to +14"); } else { - const float previous = _prefs.tz_offset; - _prefs.tz_offset = tz; - if (savePrefs()) strcpy(reply, "OK"); - else { - _prefs.tz_offset = previous; - strcpy(reply, "Error: setting could not be saved"); - } + strcpy(reply, savePreference(_prefs.tz_offset, tz) + ? "OK" : "Error: setting could not be saved"); } return true; } diff --git a/test/test_companion_prefs_image.py b/test/test_companion_prefs_image.py index a0f5bd32e..e180e4e29 100644 --- a/test/test_companion_prefs_image.py +++ b/test/test_companion_prefs_image.py @@ -130,6 +130,10 @@ int main() { assert(store.savePrefs(prefs, 47.125, -122.25)); assert(store.fs.files.at("/new_prefs") == golden(prefs, 47.125, -122.25)); assert(!store.fs.exists("/new_prefs.tmp")); +#if defined(STM32_PLATFORM) + assert(WRITE_CALLS == std::vector{244}); + assert(store.fs.files.at("/new_prefs").size() == 244); +#endif #if defined(NRF52_PLATFORM) const std::vector streaming = {@WRITE_SIZES@ #if defined(RAK_4631) @@ -364,6 +368,43 @@ int main() { assert(lat == 42.25 && lon == -121.125); #if defined(STM32_PLATFORM) + // The extended STM32 image is still one atomic write. Its old prefix and + // TXR bytes come from the independent wire definition; TZF is a literal + // binary32/CRC vector, not the production footer encoder. + { + CompanionNodePrefs fractional; + assert(fractional.copyPersistedValuesFrom(changed)); + fractional.tz_offset = 5.75f; + auto expected = golden(fractional, 42.25, -121.125); + expected.insert(expected.end(), {'T', 'Z', 'F', 1, 0, 0, 0xb8, 0x40, 0x7e, 0x2e}); + assert(expected.size() == 254 && expected[225] == 5); + DataStore writer; + writer.fs.files["/new_prefs"] = before; + WRITE_CALLS.clear(); + assert(writer.savePrefs(fractional, 42.25, -121.125)); + assert(WRITE_CALLS == std::vector{254}); + assert(writer.fs.files.at("/new_prefs") == expected); + for (size_t quota = 0; quota < expected.size(); ++quota) { + for (bool readback : {false, true}) { + DataStore failed; + failed.fs.files["/new_prefs"] = before; + if (readback) failed.fs.fail_read_after = static_cast(quota); + else failed.fs.fail_write_after = static_cast(quota); + assert(!failed.savePrefs(fractional, 42.25, -121.125)); + unchanged(failed, before); + failed.fs.fail_read_after = failed.fs.fail_write_after = -1; + WRITE_CALLS.clear(); + assert(failed.savePrefs(fractional, 42.25, -121.125)); + assert(WRITE_CALLS == std::vector{254}); + assert(failed.fs.files.at("/new_prefs") == expected); + ++comparisons; + } + } + CompanionNodePrefs reboot; + assert(writer.loadPrefsInt("/new_prefs", reboot, lat, lon)); + assert(reboot.tz_offset == 5.75f && reboot.freq == fractional.freq); + assert(reboot.tx_media == fractional.tx_media && reboot.tx_ssh == fractional.tx_ssh); + } // A future field or wrong staging capacity must fail closed, not overrun // the stack or silently publish a truncated/extra-byte format. for (bool undersized : {false, true}) { @@ -429,8 +470,9 @@ class CompanionPrefsImageTests(unittest.TestCase): ) + '\n' + save + '\n' + legacy_writer for suffix, capacity in (('TooSmall', 235), ('TooLarge', 237)): variant = save.replace('DataStore::savePrefs(', 'DataStore::savePrefs' + suffix + '(', 1) - self.assertIn('uint8_t image[236];', variant) - variant = variant.replace('uint8_t image[236];', f'uint8_t image[{capacity}];', 1) + prefix = 'static constexpr size_t PREFS_PREFIX_SIZE = 236;' + self.assertIn(prefix, variant) + variant = variant.replace(prefix, f'static constexpr size_t PREFS_PREFIX_SIZE = {capacity};', 1) methods += '\n#if defined(STM32_PLATFORM)\n' + variant + '\n#endif\n' code = HARNESS.split('int main(')[0].replace('@METHODS@', methods) code = code.replace(' bool savePrefs(const CompanionNodePrefs&, double, double);', diff --git a/test/test_companion_prefs_values.py b/test/test_companion_prefs_values.py index d7da1e110..3d45464ba 100644 --- a/test/test_companion_prefs_values.py +++ b/test/test_companion_prefs_values.py @@ -132,9 +132,9 @@ int main() { "/new_prefs", actual, lat, lon); assert(got == expected); assert(READ_CALLS == expected_reads); - // The common footer reader adds one EOF probe after any recognized - // historical image; no original field read or ordering changes. - assert(AVAILABLE_CALLS == expected_available+(expected_reads.empty()?0:1)); + // Footer presence follows the already validated declared size; every + // historical field read and EOF probe still matches the old reader. + assert(AVAILABLE_CALLS == expected_available); if (length == 84 && quota == 84) { // The original mandatory prefix has 21 reads, ending at ble_pin. // This order is an independent wire contract, not the new table. @@ -154,7 +154,7 @@ int main() { #endif // Every absent optional field observes EOF, then the completed // image receives the existing final trailing-byte check. - assert(AVAILABLE_CALLS == optional_fields + 2); + assert(AVAILABLE_CALLS == optional_fields + 1); } if(got)legacy.tx_media=legacy.tx_ssh=mesh::RADIO_TX_AUTO; equalValues(actual, legacy); diff --git a/test/test_companion_timezone_prefs.py b/test/test_companion_timezone_prefs.py index f87987852..a55ede411 100644 --- a/test/test_companion_timezone_prefs.py +++ b/test/test_companion_timezone_prefs.py @@ -189,6 +189,7 @@ public: DataStore store; unsigned saves = 0; bool savePrefs() { ++saves; return store.savePrefs(_prefs, 47.125, -122.25); } + @SAVE_PREFERENCE@ bool command(const char* command, char* reply) { @GETTER@ @SETTER@ @@ -303,6 +304,13 @@ int main() { const std::vector fraction = @FIVE_SEVENTY_FIVE@; complete.insert(complete.end(), fraction.begin(), fraction.end()); const size_t extension = sizeof(LEGACY_IMAGE); + // A declared fractional image must not silently fall back to its legacy + // byte when a stream prematurely reports EOF before the checked footers. + for (size_t cutoff : {size_t(84), extension - size_t(8), extension}) { + FORCED_EOF_CURSOR = cutoff; + rejectImage(complete, static_cast(cutoff)); + FORCED_EOF_CURSOR = size_t(-1); + } // Every damaged extension byte fails atomically. Removing the complete // optional footer is a legitimate old-format integer image, tested above. for (size_t i = 0; i < 10; ++i) { @@ -459,7 +467,10 @@ int main() { code = HARNESS.split("int main(")[0].replace("@METHODS@", methods) getter = method(mesh, ' if (strcmp(command, "get tz.offset") == 0)') setter = method(mesh, ' if (strncmp(command, "set tz.offset ", 14) == 0)') + header = (ROOT / "examples/companion_radio/MyMesh.h").read_text() + save_preference = method(header, 'template bool savePreference(') tests = TESTS.replace("@GETTER@", getter).replace("@SETTER@", setter) + tests = tests.replace("@SAVE_PREFERENCE@", save_preference) values = (5.5, 5.75, -3.5, -3.75, 12.75, 13.75, -11.75, .25, -.25) tests = tests.replace("@FRACTIONS@", ",\n ".join( "{" + str(value) + "f," + cpp_bytes(fractional_footer(value)) + "}" for value in values)) @@ -485,6 +496,16 @@ inline char* utoa(unsigned int value,char* output,int base){ if(base!=10)abort();sprintf(output,"%u",value);return output; } ''') + # Only the transport boundary is fault-injected: size() retains + # the complete declared image while available() can report EOF. + mock = (ROOT / "test/fixtures/radio_profiles/mocks/helpers/IdentityStore.h").read_text() + mock = mock.replace("class MemoryFS;", "static size_t FORCED_EOF_CURSOR = size_t(-1);\nclass MemoryFS;", 1) + available = "size_t available() const { return size() - cursor_; }" + self.assertEqual(mock.count(available), 1) + mock = mock.replace(available, + "size_t available() const { return cursor_ >= FORCED_EOF_CURSOR ? 0 : size() - cursor_; }") + (work / "helpers").mkdir() + (work / "helpers/IdentityStore.h").write_text(mock) transaction = (ROOT / "src/helpers/ContactFileTransaction.h").read_text() (work / "ContactFileTransaction.h").write_text(transaction.replace( '#include "IdentityStore.h"', '#include ')) diff --git a/test/test_local_cli_access.py b/test/test_local_cli_access.py index b0c52af72..870bc7247 100644 --- a/test/test_local_cli_access.py +++ b/test/test_local_cli_access.py @@ -16,6 +16,7 @@ PREAMBLE = r''' #include #include #include +#include #define TBEAM_SX1262 1 #include #include @@ -164,6 +165,7 @@ struct MyMesh : CompanionFleetFixture { void cancelAllFloodRetries(){} void resetContacts(){++resets;} bool savePrefs(){++saves;return save_ok;} + @SAVE_PREFERENCE@ int getTotalAirTime(){return 0;} int getReceiveAirTime(){return 0;} int getNumSentFlood(){return 0;} @@ -288,6 +290,23 @@ int main() { assert(node.handleCommand(command,0,reply)); assert(!strcmp(reply,"OK") && board.power_offs==previous+1); } + // Exercise the real timezone dispatch and typed transaction helper, not a + // fixture-only setter: a failed save must preserve the exact fraction. + node.save_ok=true; + assert(node.handleCommand("set tz.offset 5.75",0,reply) && !strcmp(reply,"OK")); + assert(node._prefs.tz_offset==5.75f); + assert(node.handleCommand("get tz.offset",0,reply) && !strcmp(reply,"> 5.75")); + node.save_ok=false; + const int timezone_saves=node.saves; + assert(node.handleCommand("set tz.offset -3.5",0,reply)); + assert(strstr(reply,"Error") && node.saves==timezone_saves+1); + assert(node._prefs.tz_offset==5.75f); + for(const char* bad:{"nan","inf","5.75junk","15","-12.25"}) { + const std::string command=std::string("set tz.offset ")+bad; + assert(node.handleCommand(command.c_str(),0,reply)); + assert(strstr(reply,"Error") && node.saves==timezone_saves+1); + assert(node._prefs.tz_offset==5.75f); + } } ''' @@ -405,6 +424,9 @@ int main() { "void MyMesh::onContactCacheFlushed()", "bool MyMesh::handleDirectCommand(", "bool MyMesh::handleCommand(", "void MyMesh::onCLICommandRecv(")) + header = (ROOT / "examples/companion_radio/MyMesh.h").read_text() + preamble = PREAMBLE.replace("@SAVE_PREFERENCE@", extract_braced( + header, "template bool savePreference(")) for export, webconfig in ((0, False), (1, False), (0, True), (1, True)): with self.subTest(export=export, webconfig=webconfig), tempfile.TemporaryDirectory() as temp: binary = Path(temp) / "test" @@ -415,7 +437,7 @@ int main() { "-DMESH_ENABLE_LOST_REPLY=1", *(["-DWITH_WEBCONFIG=1"] if webconfig else []), *SANITIZER_FLAGS, "-o", str(binary), - ], input=PREAMBLE+implementation+SCENARIOS, text=True, capture_output=True) + ], input=preamble+implementation+SCENARIOS, text=True, capture_output=True) self.assertEqual(result.returncode, 0, result.stderr) result = subprocess.run([str(binary)], capture_output=True, text=True) self.assertEqual(result.returncode, 0, result.stdout+result.stderr) diff --git a/variants/tiny_relay/platformio.ini b/variants/tiny_relay/platformio.ini index d7526bfbd..3e87ac693 100644 --- a/variants/tiny_relay/platformio.ini +++ b/variants/tiny_relay/platformio.ini @@ -82,6 +82,10 @@ build_flags = ${Tiny_Relay.build_flags} -flto -fno-inline-small-functions -fno-tree-dominator-opts ; preserve Companion features and the 32 KiB filesystem + ; Match the compact Wio Mini code layout with the complete CLI retained. + -fno-reorder-blocks + -fno-reorder-functions + -fno-caller-saves -fipa-pta -fmerge-all-constants -fno-ipa-cp-clone diff --git a/variants/wio-e5-dev/platformio.ini b/variants/wio-e5-dev/platformio.ini index a5a486c0a..378d25258 100644 --- a/variants/wio-e5-dev/platformio.ini +++ b/variants/wio-e5-dev/platformio.ini @@ -101,6 +101,10 @@ build_flags = ${lora_e5.build_flags} -flto -fno-inline-small-functions -fno-tree-dominator-opts ; preserve Companion features and the 32 KiB filesystem + ; Match the compact Wio Mini code layout with the complete CLI retained. + -fno-reorder-blocks + -fno-reorder-functions + -fno-caller-saves -fipa-pta -fmerge-all-constants -fno-ipa-cp-clone