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