diff --git a/.github/workflows/run-unit-tests.yml b/.github/workflows/run-unit-tests.yml index bcf94115..945daac7 100644 --- a/.github/workflows/run-unit-tests.yml +++ b/.github/workflows/run-unit-tests.yml @@ -91,6 +91,7 @@ jobs: python3 -B test/test_ota_identity_policy.py -v python3 -B test/test_companion_radio_gain_restore.py -v python3 -B test/test_common_radio_persistence.py -v + python3 -B test/test_common_cli_save_results.py -v python3 -B test/test_common_prefs_commit.py -v python3 -B test/test_mqtt_prefs_commit.py -v python3 -B test/test_identity_and_settings_recovery.py -v diff --git a/build.sh b/build.sh index 3d399aa9..9db250b9 100755 --- a/build.sh +++ b/build.sh @@ -2821,6 +2821,12 @@ is_lora_ota_build() { local env_name=$1 local env_name_lc=${env_name,,} + # KISS uses the radio as a host-controlled modem. Shared board flags do not + # add an application OTA manager or CLI, even under the auto/FULL profiles. + if is_kiss_modem_target "$env_name"; then + return 1 + fi + # ESP32 USB and WiFi companions keep OTA so they can seed a host folder over # serial or TCP and can participate in LoRa OTA without using the FULL profile. if is_esp32_usb_wifi_companion_ota_build "$env_name"; then diff --git a/examples/simple_repeater/MyMesh.cpp b/examples/simple_repeater/MyMesh.cpp index 494a57bc..9e08a5ff 100644 --- a/examples/simple_repeater/MyMesh.cpp +++ b/examples/simple_repeater/MyMesh.cpp @@ -10574,7 +10574,7 @@ void MyMesh::formatPacketStatsReply(char *reply) { getNumRecvFlood(), getNumRecvDirect()); } -void MyMesh::saveIdentity(const mesh::LocalIdentity &new_id) { +bool MyMesh::saveIdentity(const mesh::LocalIdentity &new_id) { #if defined(NRF52_PLATFORM) || defined(STM32_PLATFORM) IdentityStore store(*_fs, ""); #elif defined(ESP32) @@ -10584,7 +10584,7 @@ void MyMesh::saveIdentity(const mesh::LocalIdentity &new_id) { #else #error "need to define saveIdentity()" #endif - store.save("_main", new_id); + return store.saveWithRetry("_main", new_id); } void MyMesh::clearStats() { diff --git a/examples/simple_repeater/MyMesh.h b/examples/simple_repeater/MyMesh.h index 5a430416..a3a8d79e 100644 --- a/examples/simple_repeater/MyMesh.h +++ b/examples/simple_repeater/MyMesh.h @@ -1067,7 +1067,7 @@ public: void onDefaultRegionChanged(const RegionEntry* r) override; mesh::LocalIdentity& getSelfId() override { return self_id; } - void saveIdentity(const mesh::LocalIdentity& new_id) override; + bool saveIdentity(const mesh::LocalIdentity& new_id) override; void clearStats() override; #if defined(WITH_WEBCONFIG) || defined(ETHERNET_ENABLED) diff --git a/examples/simple_room_server/MyMesh.cpp b/examples/simple_room_server/MyMesh.cpp index bee3a640..9fa43f9c 100644 --- a/examples/simple_room_server/MyMesh.cpp +++ b/examples/simple_room_server/MyMesh.cpp @@ -1751,7 +1751,7 @@ bool MyMesh::setRxBoostedGain(bool enable) { return radio_driver.setRxBoostedGainMode(enable); } -void MyMesh::saveIdentity(const mesh::LocalIdentity &new_id) { +bool MyMesh::saveIdentity(const mesh::LocalIdentity &new_id) { #if defined(NRF52_PLATFORM) || defined(STM32_PLATFORM) IdentityStore store(*_fs, ""); #elif defined(ESP32) @@ -1761,7 +1761,7 @@ void MyMesh::saveIdentity(const mesh::LocalIdentity &new_id) { #else #error "need to define saveIdentity()" #endif - store.save("_main", new_id); + return store.saveWithRetry("_main", new_id); } void MyMesh::startRegionsLoad() { diff --git a/examples/simple_room_server/MyMesh.h b/examples/simple_room_server/MyMesh.h index 479c0ab6..a36dcf30 100644 --- a/examples/simple_room_server/MyMesh.h +++ b/examples/simple_room_server/MyMesh.h @@ -504,7 +504,7 @@ public: static bool saveFilter(ClientInfo* client); - void saveIdentity(const mesh::LocalIdentity& new_id) override; + bool saveIdentity(const mesh::LocalIdentity& new_id) override; void clearStats() override; #if defined(WITH_WEBCONFIG) || defined(ETHERNET_ENABLED) void handleLocalCommand(char* command, char* reply, Stream& output) { diff --git a/examples/simple_sensor/SensorMesh.cpp b/examples/simple_sensor/SensorMesh.cpp index b3a0c1b4..addfe490 100644 --- a/examples/simple_sensor/SensorMesh.cpp +++ b/examples/simple_sensor/SensorMesh.cpp @@ -1092,7 +1092,7 @@ bool SensorMesh::formatFileSystem() { #endif } -void SensorMesh::saveIdentity(const mesh::LocalIdentity& new_id) { +bool SensorMesh::saveIdentity(const mesh::LocalIdentity& new_id) { #if defined(NRF52_PLATFORM) || defined(STM32_PLATFORM) IdentityStore store(*_fs, ""); #elif defined(ESP32) @@ -1102,7 +1102,7 @@ void SensorMesh::saveIdentity(const mesh::LocalIdentity& new_id) { #else #error "need to define saveIdentity()" #endif - store.save("_main", new_id); + return store.saveWithRetry("_main", new_id); } void SensorMesh::applyTempRadioParams(float freq, float bw, uint8_t sf, uint8_t cr, int timeout_mins, uint16_t preamble) { diff --git a/examples/simple_sensor/SensorMesh.h b/examples/simple_sensor/SensorMesh.h index d11a455c..f60a3a7a 100644 --- a/examples/simple_sensor/SensorMesh.h +++ b/examples/simple_sensor/SensorMesh.h @@ -92,7 +92,7 @@ public: void formatRadioStatsReply(char *reply) override; void formatPacketStatsReply(char *reply) override; mesh::LocalIdentity& getSelfId() override { return self_id; } - void saveIdentity(const mesh::LocalIdentity& new_id) override; + bool saveIdentity(const mesh::LocalIdentity& new_id) override; void clearStats() override { } mesh::Radio* getProfileRadio() override { return _radio; } void applyTempRadioParams(float freq, float bw, uint8_t sf, uint8_t cr, int timeout_mins, uint16_t preamble = 0) override; diff --git a/src/helpers/CommonCLI.cpp b/src/helpers/CommonCLI.cpp index 4e99d6bd..962fc8cf 100644 --- a/src/helpers/CommonCLI.cpp +++ b/src/helpers/CommonCLI.cpp @@ -1,5 +1,6 @@ #include #include "CommonCLI.h" +#include "PrefsSaveReplyGuard.h" #include #include "CLICommandUtils.h" #include "FloodAdvertCLI.h" @@ -2448,16 +2449,52 @@ bool CommonCLI::saveMQTTPrefs(FILESYSTEM* fs) { #define MIN_LOCAL_ADVERT_INTERVAL 60 void CommonCLI::savePrefs(PrefsSaveRouting::Scope scope) { - uint8_t old_advert_interval = _prefs->advert_interval; - if (_prefs->advert_interval * 2 < MIN_LOCAL_ADVERT_INTERVAL) { + if (!trySavePrefs(scope)) { + ++_prefs_save_failures; + if (PrefsSaveRouting::planFor(scope).common + && (!_common_save_result_known || !_common_save_succeeded)) { + _prefs->markUnsaved(); + } + } +} + +bool CommonCLI::trySavePrefs(PrefsSaveRouting::Scope scope) { + const PrefsSaveRouting::Plan plan = PrefsSaveRouting::planFor(scope); + const uint8_t old_advert_interval = _prefs->advert_interval; + if (plan.common && _prefs->advert_interval * 2 < MIN_LOCAL_ADVERT_INTERVAL) { _prefs->advert_interval = 0; // turn it off, now that device has been manually configured } - // If advert_interval was changed, update the timer to reflect the change - if (old_advert_interval != _prefs->advert_interval) { - _callbacks->updateAdvertTimer(); + // A previous successful transaction must not mask a callback that did not + // save this time. Both images have independent commit results. + if (plan.common) { + _common_save_result_known = false; + _common_save_succeeded = false; } +#ifdef WITH_MQTT_BRIDGE + if (plan.observer) { + _observer_save_result_known = false; + _observer_save_succeeded = false; + } +#endif _callbacks->savePrefs(scope); - _prefs->clearDirty(); + const bool common_saved = !plan.common + || (_common_save_result_known && _common_save_succeeded); + if (plan.common) { + if (common_saved) { + _prefs->clearDirty(); + if (old_advert_interval != _prefs->advert_interval) { + _callbacks->updateAdvertTimer(); + } + } else { + _prefs->advert_interval = old_advert_interval; + } + } +#ifdef WITH_MQTT_BRIDGE + return common_saved && (!plan.observer + || (_observer_save_result_known && _observer_save_succeeded)); +#else + return common_saved; +#endif } bool CommonCLI::saveObserverPrefs() { @@ -2465,7 +2502,9 @@ bool CommonCLI::saveObserverPrefs() { _observer_save_result_known = false; _observer_save_succeeded = false; _callbacks->savePrefs(PrefsSaveRouting::Scope::Observer); - return _observer_save_result_known && _observer_save_succeeded; + const bool saved = _observer_save_result_known && _observer_save_succeeded; + if (!saved) ++_prefs_save_failures; + return saved; #else return false; #endif @@ -2518,6 +2557,7 @@ uint8_t CommonCLI::buildAdvertData(uint8_t node_type, uint8_t* app_data) { } void CommonCLI::handleCommand(uint32_t sender_timestamp, char* command, char* reply) { + PrefsSaveReplyGuard save_reply(_prefs_save_failures, reply); mesh::cli::normalizeCommandVerb(command); if (mesh::wireless::control().handle(command, reply, 160, millis(), _callbacks->wirelessCommandSource(sender_timestamp))) return; @@ -2712,8 +2752,14 @@ void CommonCLI::handleCommand(uint32_t sender_timestamp, char* command, char* re } } else if (memcmp(command, "password ", 9) == 0) { // change admin password + char previous[sizeof(_prefs->password)]; + memcpy(previous, _prefs->password, sizeof(previous)); StrHelper::strncpy(_prefs->password, &command[9], sizeof(_prefs->password)); - savePrefs(); + if (!trySavePrefs()) { + memcpy(_prefs->password, previous, sizeof(previous)); + strcpy(reply, "Error: password not saved; unchanged"); + return; + } sprintf(reply, "password now: %s", _prefs->password); // echo back just to let admin know for sure!! } else if (memcmp(command, "clear stats", 11) == 0) { _callbacks->clearStats(); @@ -3462,28 +3508,46 @@ void CommonCLI::handleSetCmd(uint32_t sender_timestamp, char* command, char* rep strcpy(reply, "ERROR: telemetry.access must be all or acl"); } } else if (memcmp(config, "flood.advert.interval ", 22) == 0) { - int hours = _atoi(&config[22]); - if ((hours > 0 && hours < 3) || (hours > 168)) { - strcpy(reply, "Error: interval range is 3-168 hours"); + uint32_t hours = 0; + if (!parseUint32Strict(&config[22], hours) + || (hours != 0 && (hours < 3 || hours > 168))) { + strcpy(reply, "Error: interval must be 0 (off) or 3-168 hours"); } else { + const uint8_t previous = _prefs->flood_advert_interval; _prefs->flood_advert_interval = (uint8_t)(hours); + if (!trySavePrefs()) { + _prefs->flood_advert_interval = previous; + strcpy(reply, "Error: interval not saved; unchanged"); + return; + } _callbacks->updateFloodAdvertTimer(); - savePrefs(); strcpy(reply, "OK"); } } else if (memcmp(config, "advert.interval ", 16) == 0) { - int mins = _atoi(&config[16]); - if ((mins > 0 && mins < MIN_LOCAL_ADVERT_INTERVAL) || (mins > 240)) { - sprintf(reply, "Error: interval range is %d-240 minutes", MIN_LOCAL_ADVERT_INTERVAL); + uint32_t mins = 0; + if (!parseUint32Strict(&config[16], mins) + || (mins != 0 && (mins < MIN_LOCAL_ADVERT_INTERVAL || mins > 240))) { + sprintf(reply, "Error: interval must be 0 (off) or %d-240 minutes", MIN_LOCAL_ADVERT_INTERVAL); } else { + const uint8_t previous = _prefs->advert_interval; _prefs->advert_interval = (uint8_t)(mins / 2); + if (!trySavePrefs()) { + _prefs->advert_interval = previous; + strcpy(reply, "Error: interval not saved; unchanged"); + return; + } _callbacks->updateAdvertTimer(); - savePrefs(); strcpy(reply, "OK"); } } else if (memcmp(config, "guest.password ", 15) == 0) { + char previous[sizeof(_prefs->guest_password)]; + memcpy(previous, _prefs->guest_password, sizeof(previous)); StrHelper::strncpy(_prefs->guest_password, &config[15], sizeof(_prefs->guest_password)); - savePrefs(); + if (!trySavePrefs()) { + memcpy(_prefs->guest_password, previous, sizeof(previous)); + strcpy(reply, "Error: guest password not saved; unchanged"); + return; + } strcpy(reply, "OK"); } else if (memcmp(config, "prv.key ", 8) == 0) { uint8_t prv_key[PRV_KEY_SIZE]; @@ -3492,7 +3556,10 @@ void CommonCLI::handleSetCmd(uint32_t sender_timestamp, char* command, char* rep if (success && mesh::LocalIdentity::validatePrivateKey(prv_key)) { mesh::LocalIdentity new_id; new_id.readFrom(prv_key, PRV_KEY_SIZE); - _callbacks->saveIdentity(new_id); + if (!_callbacks->saveIdentity(new_id)) { + strcpy(reply, "Error: identity not saved; current key unchanged"); + return; + } strcpy(reply, "OK, reboot to apply! New pubkey: "); mesh::Utils::toHex(&reply[33], new_id.pub_key, PUB_KEY_SIZE); } else { diff --git a/src/helpers/CommonCLI.h b/src/helpers/CommonCLI.h index 87bfc5c7..64cbbe1e 100644 --- a/src/helpers/CommonCLI.h +++ b/src/helpers/CommonCLI.h @@ -424,6 +424,7 @@ public: bool isDirty() const override { return ConfigSerializer::isDirty() || radio.isDirty() || custom.isDirty(); } void clearDirty() override { ConfigSerializer::clearDirty(); radio.clearDirty(); custom.clearDirty(); } + void markUnsaved() { markDirty(); } }; #ifdef WITH_MQTT_BRIDGE @@ -515,7 +516,7 @@ public: virtual bool supportsAdvancedRetryConfig() const { return false; } virtual void onRetryConfigChanged() { } virtual mesh::LocalIdentity& getSelfId() = 0; - virtual void saveIdentity(const mesh::LocalIdentity& new_id) = 0; + virtual bool saveIdentity(const mesh::LocalIdentity& new_id) = 0; virtual void clearStats() = 0; virtual void applyTempRadioParams(float freq, float bw, uint8_t sf, uint8_t cr, int timeout_mins, uint16_t preamble = 0) = 0; virtual uint32_t getTempRadioDurationSeconds() const { return 0; } @@ -761,11 +762,14 @@ class CommonCLI { bool _com_prefs_needs_upgrade = false; // old-format legacy prefs detected; rewrite once after load bool _common_save_result_known = false; bool _common_save_succeeded = false; + uint32_t _prefs_save_failures = 0; mesh::RadioProfileCLI _radio_profiles; mesh::RTCClock* getRTCClock() { return _rtc; } void savePrefs( PrefsSaveRouting::Scope scope = PrefsSaveRouting::Scope::Common); + bool trySavePrefs( + PrefsSaveRouting::Scope scope = PrefsSaveRouting::Scope::Common); bool saveObserverPrefs(); void loadPrefsInt(FILESYSTEM* _fs, const char* filename); #ifdef WITH_MQTT_BRIDGE diff --git a/src/helpers/CommonCLI_Observer.cpp b/src/helpers/CommonCLI_Observer.cpp index c1bf2009..8a7b39e4 100644 --- a/src/helpers/CommonCLI_Observer.cpp +++ b/src/helpers/CommonCLI_Observer.cpp @@ -637,7 +637,7 @@ bool CommonCLI::handleObserverSetCmd(uint32_t sender_timestamp, const char* conf strcpy(reply, "Error: filter must be all, none, or a CSV of types 0-15 / names (advert,txt_msg,...)"); } else { _mqtt_prefs.mqtt_slot_packet_filter[slot] = filter_mask; - savePrefs(); + saveObserverPrefs(); char filter_text[MQTTPacketFilter::kFilterTextSize]; MQTTPacketFilter::format(filter_mask, filter_text, sizeof(filter_text)); snprintf(reply, 160, "OK - slot %d packet types: %s", slot + 1, filter_text); diff --git a/src/helpers/PrefsSaveReplyGuard.h b/src/helpers/PrefsSaveReplyGuard.h new file mode 100644 index 00000000..0619421c --- /dev/null +++ b/src/helpers/PrefsSaveReplyGuard.h @@ -0,0 +1,21 @@ +#pragma once + +#include +#include + +// Legacy setters can apply live settings before saving. Never let their final +// OK overwrite a persistence error, including through nested CLI dispatch. +class PrefsSaveReplyGuard { + const uint32_t& _failures; + const uint32_t _start; + char* _reply; + +public: + PrefsSaveReplyGuard(const uint32_t& failures, char* reply) + : _failures(failures), _start(failures), _reply(reply) {} + ~PrefsSaveReplyGuard() { + if (_failures != _start) { + strcpy(_reply, "Error: settings not saved; unsaved changes may remain active until reboot"); + } + } +}; diff --git a/test/fixtures/common_cli_save_results/main.cpp b/test/fixtures/common_cli_save_results/main.cpp new file mode 100644 index 00000000..8ac0867d --- /dev/null +++ b/test/fixtures/common_cli_save_results/main.cpp @@ -0,0 +1,222 @@ +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#define MIN_LOCAL_ADVERT_INTERVAL 60 +#define PUB_KEY_SIZE 32 +#define PRV_KEY_SIZE 64 +namespace mesh { +struct LocalIdentity { + uint8_t pub_key[PUB_KEY_SIZE]{}; + static bool validatePrivateKey(const uint8_t*) { return true; } + void readFrom(const uint8_t* key, size_t) { memcpy(pub_key, key, sizeof(pub_key)); } +}; +struct Utils { + // Valid synthetic keys only; cryptography and real file transactions have + // their own production identity recovery tests. + static bool fromHex(uint8_t* out, size_t n, const char*) { memset(out, 0x11, n); return true; } + static void toHex(char* out, const uint8_t*, size_t n) { memset(out, '1', 2*n); out[2*n] = 0; } +}; +} +struct Prefs { + uint8_t advert_interval = 30, flood_advert_interval = 12, multi_acks = 0; + char password[16] = "old-admin", guest_password[16] = "old-guest"; + bool dirty = true; + void clearDirty() { dirty = false; } + void markUnsaved() { dirty = true; } +}; +struct StrHelper { + static void strncpy(char* out, const char* in, size_t n) { + std::strncpy(out, in, n-1); out[n-1] = 0; + } +}; +struct CommonCLI; +struct Callbacks { + CommonCLI* cli; + Prefs persisted; + bool common_ok = true, observer_ok = true, known = true, identity_ok = false; + unsigned saves = 0, identity_saves = 0, local_updates = 0, flood_updates = 0; + void savePrefs(PrefsSaveRouting::Scope); + void updateAdvertTimer(); + void updateFloodAdvertTimer(); + bool saveIdentity(const mesh::LocalIdentity&) { ++identity_saves; return identity_ok; } +}; +struct CommonCLI { + Prefs prefs; + Prefs* _prefs = &prefs; + Callbacks callbacks{this, prefs}; + Callbacks* _callbacks = &callbacks; + bool _common_save_result_known = false, _common_save_succeeded = false; + bool _observer_save_result_known = false, _observer_save_succeeded = false; + uint32_t _prefs_save_failures = 0; + void savePrefs(PrefsSaveRouting::Scope scope = PrefsSaveRouting::Scope::Common); + bool trySavePrefs(PrefsSaveRouting::Scope scope = PrefsSaveRouting::Scope::Common); + bool saveObserverPrefs(); + void set(const char* config, char* reply); + void password(const char* command, char* reply); +}; +void Callbacks::savePrefs(PrefsSaveRouting::Scope scope) { + ++saves; + if (!known) return; + const auto plan = PrefsSaveRouting::planFor(scope); + if (plan.common) { + cli->_common_save_result_known = true; + cli->_common_save_succeeded = common_ok; + if (common_ok) persisted = cli->prefs; + } +#ifdef WITH_MQTT_BRIDGE + if (plan.observer) { + cli->_observer_save_result_known = true; + cli->_observer_save_succeeded = observer_ok; + } +#endif +} +void Callbacks::updateAdvertTimer() { + assert(persisted.advert_interval == cli->prefs.advert_interval); + ++local_updates; +} +void Callbacks::updateFloodAdvertTimer() { + assert(persisted.flood_advert_interval == cli->prefs.flood_advert_interval); + ++flood_updates; +} +@PARSERS@ +@SAVE_METHODS@ +void CommonCLI::set(const char* input, char* reply) { + // Match the firmware's fixed-capacity CLI input buffer. + char command[160]{}; + snprintf(command, sizeof(command), "%s", input); + const char* config = command; + @REPLY_GUARD@ + @SET_BRANCHES@ +} +void CommonCLI::password(const char* command, char* reply) { + @REPLY_GUARD@ + @PASSWORD@ +} +bool error(const char* reply) { return strncmp(reply, "Error:", 6) == 0; } +int main() { + char reply[160]{}; + for (bool guest : {false, true}) { + CommonCLI cli; + cli.prefs.advert_interval = 2; + cli.callbacks.common_ok = false; + if (guest) cli.set("guest.password new-guest", reply); + else cli.password("password new-admin", reply); + assert(error(reply) && strstr(reply, "unchanged")); + assert(strcmp(cli.prefs.password, "old-admin") == 0); + assert(strcmp(cli.prefs.guest_password, "old-guest") == 0); + assert(cli.prefs.advert_interval == 2 && cli.callbacks.local_updates == 0); + assert(cli.prefs.dirty && cli.callbacks.saves == 1); + cli.callbacks.common_ok = true; + if (guest) cli.set("guest.password new-guest", reply); + else cli.password("password new-admin", reply); + assert(!error(reply)); + assert(strcmp(guest ? cli.prefs.guest_password : cli.prefs.password, + guest ? "new-guest" : "new-admin") == 0); + assert(cli.prefs.advert_interval == 0 && cli.callbacks.local_updates == 1); + assert(!cli.prefs.dirty && cli.callbacks.saves == 2); + assert(strcmp(cli.prefs.password, cli.callbacks.persisted.password) == 0); + assert(strcmp(cli.prefs.guest_password, cli.callbacks.persisted.guest_password) == 0); + } + for (bool flood : {false, true}) { + const char* key = flood ? "flood.advert.interval " : "advert.interval "; + CommonCLI cli; + for (const char* invalid : {"", " ", "nonsense", "-1", "+3", "1x", "3 4", + "4294967296", "999999999999999999999999999999", "1", "2"}) { + cli.set((std::string(key) + invalid).c_str(), reply); + assert(error(reply)); + assert(cli.prefs.advert_interval == 30 && cli.prefs.flood_advert_interval == 12); + assert(cli.callbacks.saves == 0 && cli.callbacks.local_updates == 0 + && cli.callbacks.flood_updates == 0); + } + cli.set((std::string(key) + (flood ? "169" : "241")).c_str(), reply); + assert(error(reply) && cli.callbacks.saves == 0); + cli.callbacks.common_ok = false; + cli.set((std::string(key) + (flood ? "3" : "120")).c_str(), reply); + assert(error(reply) && strstr(reply, "unchanged")); + assert(cli.prefs.advert_interval == 30 && cli.prefs.flood_advert_interval == 12); + assert(cli.callbacks.local_updates == 0 && cli.callbacks.flood_updates == 0); + cli.callbacks.common_ok = true; + for (const char* valid : {"0", flood ? "3" : "60", flood ? "168" : "240"}) { + cli.set((std::string(key) + valid).c_str(), reply); + assert(strcmp(reply, "OK") == 0); + assert(cli.prefs.advert_interval == cli.callbacks.persisted.advert_interval); + assert(cli.prefs.flood_advert_interval == cli.callbacks.persisted.flood_advert_interval); + } + assert(flood ? cli.callbacks.flood_updates == 3 : cli.callbacks.local_updates == 3); + } + { + CommonCLI cli; + const std::string key = "prv.key " + std::string(128, '1'); + cli.set(key.c_str(), reply); + assert(error(reply) && cli.callbacks.identity_saves == 1); + cli.callbacks.identity_ok = true; + cli.set(key.c_str(), reply); + assert(strncmp(reply, "OK, reboot", 10) == 0 && cli.callbacks.identity_saves == 2); + } + { + CommonCLI cli; + cli.prefs.dirty = false; + cli.callbacks.common_ok = false; + cli.set("multi.acks 2", reply); + assert(error(reply) && strstr(reply, "unsaved changes")); + assert(cli.prefs.multi_acks == 2 && cli.callbacks.persisted.multi_acks == 0); + assert(cli.prefs.dirty && cli._prefs_save_failures == 1); + cli.callbacks.common_ok = true; + cli.set("multi.acks 1", reply); + assert(strcmp(reply, "OK") == 0 && !cli.prefs.dirty); + // A callback skipping a save must not inherit the previous success latch. + cli.callbacks.known = false; + cli.set("multi.acks 2", reply); + assert(error(reply) && cli.prefs.dirty && cli._prefs_save_failures == 2); + } + { + CommonCLI cli; + cli.prefs.advert_interval = 2; + assert(cli.trySavePrefs(PrefsSaveRouting::Scope::Observer)); + assert(cli.prefs.dirty && cli.prefs.advert_interval == 2 && cli.callbacks.local_updates == 0); +#ifdef WITH_MQTT_BRIDGE + cli.callbacks.observer_ok = false; + { + PrefsSaveReplyGuard guard(cli._prefs_save_failures, reply); + assert(!cli.saveObserverPrefs()); + strcpy(reply, "OK"); + } + assert(error(reply)); + cli.callbacks.observer_ok = true; + { + PrefsSaveReplyGuard guard(cli._prefs_save_failures, reply); + assert(cli.saveObserverPrefs()); + strcpy(reply, "OK"); + } + assert(strcmp(reply, "OK") == 0); + cli.callbacks.observer_ok = false; + assert(!cli.trySavePrefs(PrefsSaveRouting::Scope::Both)); + assert(!cli.prefs.dirty && cli.prefs.advert_interval == 0); // common DID commit + cli.callbacks.observer_ok = true; + cli.callbacks.common_ok = false; + cli.prefs.advert_interval = 2; cli.prefs.dirty = true; + assert(!cli.trySavePrefs(PrefsSaveRouting::Scope::Both)); + assert(cli.prefs.dirty && cli.prefs.advert_interval == 2); // common did NOT commit +#endif + } + { + uint32_t failures = UINT32_MAX; + { + PrefsSaveReplyGuard outer(failures, reply); + { PrefsSaveReplyGuard inner(failures, reply); ++failures; strcpy(reply, "OK"); } + assert(error(reply)); + strcpy(reply, "OK"); + } + assert(error(reply)); + { PrefsSaveReplyGuard next(failures, reply); strcpy(reply, "OK"); } + assert(strcmp(reply, "OK") == 0); + } + puts("Common CLI persistence and interval regressions passed"); +} diff --git a/test/test_build_profiles.sh b/test/test_build_profiles.sh index 95d04bd4..0f00138d 100644 --- a/test/test_build_profiles.sh +++ b/test/test_build_profiles.sh @@ -319,6 +319,29 @@ require(rak_usb, "build_flags", "FORCE_GPS_ALIVE") # replace each with that exact board's Full target. init_project_context >/dev/null +# KISS inherits board OTA flags, not the application OTA implementation. +for kiss_env in "${SUPPORTED_PIO_ENVS[@]}"; do + is_kiss_modem_target "$kiss_env" || continue + for kiss_profile in auto standard full; do + ( + BUILD_PROFILE_FOR_TARGET=$kiss_profile + ESP32_FULL_BUILD=1 + if is_lora_ota_build "$kiss_env"; then + fail "$kiss_env $kiss_profile incorrectly promises LoRa OTA" + fi + BUILD_CAPABILITIES=() + BUILD_EXPECTATIONS=() + BUILD_REDUCTIONS=() + declare_build_capability_contract "$kiss_env" "${PIO_ENV_PLATFORM_BY_NAME[$kiss_env]}" + for expectation in "${BUILD_EXPECTATIONS[@]}"; do + case "$expectation" in + ota.update.lora=*|ota.cli=*) fail "$kiss_env $kiss_profile has an impossible OTA contract" ;; + esac + done + ) + done +done + # The portable inflater is shared by every Arduino platform, but it does not # supply the OTA manager or a supported bootloader/apply path. In particular, # auto overlays for PicoW and RAK_3x72 must remain buildable without OTA. diff --git a/test/test_common_cli_save_results.py b/test/test_common_cli_save_results.py new file mode 100644 index 00000000..c85c2129 --- /dev/null +++ b/test/test_common_cli_save_results.py @@ -0,0 +1,102 @@ +"""Exercise production CLI save acknowledgements, rollback, and interval parsing.""" +from pathlib import Path +import re +import subprocess +import tempfile +import unittest + +from test_replay_reset_integration import extract_braced + +ROOT = Path(__file__).resolve().parents[1] + + +def compile_run(source, macros=()): + with tempfile.TemporaryDirectory(prefix='common-cli-save-') as directory: + work = Path(directory) + cpp, exe = work / 'test.cpp', work / 'test' + cpp.write_text(source, encoding='utf-8') + subprocess.run(['g++', '-std=c++17', '-Wall', '-Wextra', '-Werror', + '-fsanitize=address,undefined', '-fno-sanitize-recover=all', + '-fno-pie', '-no-pie', *['-D' + m + '=1' for m in macros], + '-I' + str(ROOT / 'src'), str(cpp), '-o', str(exe)], check=True) + subprocess.run([str(exe)], check=True) + + +class CommonCLISaveResultsTest(unittest.TestCase): + def test_production_handlers(self): + source = (ROOT / 'src/helpers/CommonCLI.cpp').read_text(encoding='utf-8') + code = (ROOT / 'test/fixtures/common_cli_save_results/main.cpp').read_text() + methods = '\n'.join(extract_braced(source, signature) for signature in ( + 'void CommonCLI::savePrefs(PrefsSaveRouting::Scope scope)', + 'bool CommonCLI::trySavePrefs(', 'bool CommonCLI::saveObserverPrefs(')) + parsers = '\n'.join(extract_braced(source, signature) for signature in ( + 'static bool looksUnsignedInteger(', 'static const char* skipSpacesConst(', + 'static bool parseUint32Strict(')) + branches = '\nelse '.join(extract_braced(source, signature) for signature in ( + 'if (memcmp(config, "flood.advert.interval ", 22) == 0)', + 'if (memcmp(config, "advert.interval ", 16) == 0)', + 'if (memcmp(config, "guest.password ", 15) == 0)', + 'if (memcmp(config, "prv.key ", 8) == 0)', + 'if (memcmp(config, "multi.acks ", 11) == 0)')) + handler = extract_braced(source, 'void CommonCLI::handleCommand(') + guard = re.search(r'PrefsSaveReplyGuard save_reply\([^;]+;', handler).group() + for key, value in { + 'PARSERS': parsers, 'SAVE_METHODS': methods, 'SET_BRANCHES': branches, + 'REPLY_GUARD': guard, + 'PASSWORD': extract_braced(source, 'if (memcmp(command, "password ", 9) == 0)'), + }.items(): + code = code.replace('@' + key + '@', value) + for macros in ((), ('WITH_MQTT_BRIDGE',)): + with self.subTest(macros=macros): + compile_run(code, macros) + + def test_observer_setters_use_the_observer_store(self): + source = (ROOT / 'src/helpers/CommonCLI_Observer.cpp').read_text() + setter = extract_braced(source, 'bool CommonCLI::handleObserverSetCmd(') + self.assertNotIn('savePrefs(', setter) + self.assertIn('saveObserverPrefs();', setter) + + def test_all_identity_callbacks_return_the_store_result(self): + methods = [] + for role, name, cls in (('simple_repeater', 'MyMesh', 'Repeater'), + ('simple_room_server', 'MyMesh', 'Room'), + ('simple_sensor', 'SensorMesh', 'Sensor')): + source = (ROOT / 'examples' / role / (name + '.cpp')).read_text() + header = (ROOT / 'examples' / role / (name + '.h')).read_text() + self.assertIn('bool saveIdentity(', header) + method = extract_braced(source, f'bool {name}::saveIdentity(') + methods.append(method.replace(name + '::', cls + '::')) + code = r''' +#include +#include +#include +namespace mesh { struct LocalIdentity {}; } +struct FS { bool ok = false; unsigned calls = 0; }; +struct IdentityStore { + FS& fs; + IdentityStore(FS& f, const char*) : fs(f) {} + bool saveWithRetry(const char* name, const mesh::LocalIdentity&) { + assert(strcmp(name, "_main") == 0); ++fs.calls; return fs.ok; + } +}; +struct Repeater { FS fs; FS* _fs = &fs; bool saveIdentity(const mesh::LocalIdentity&); }; +struct Room { FS fs; FS* _fs = &fs; bool saveIdentity(const mesh::LocalIdentity&); }; +struct Sensor { FS fs; FS* _fs = &fs; bool saveIdentity(const mesh::LocalIdentity&); }; +@METHODS@ +template void check() { + Role role; mesh::LocalIdentity id; + for (bool result : {false, true, false}) { + role.fs.ok = result; + assert(role.saveIdentity(id) == result); + } + assert(role.fs.calls == 3); +} +int main() { check(); check(); check(); } +'''.replace('@METHODS@', '\n'.join(methods)) + for platform in ('NRF52_PLATFORM', 'STM32_PLATFORM', 'ESP32', 'RP2040_PLATFORM'): + with self.subTest(platform=platform): + compile_run(code, (platform,)) + + +if __name__ == '__main__': + unittest.main()