diff --git a/docs/KNOWN_ISSUES.md b/docs/KNOWN_ISSUES.md index 8bb915e..5221722 100644 --- a/docs/KNOWN_ISSUES.md +++ b/docs/KNOWN_ISSUES.md @@ -57,3 +57,26 @@ Unlike the `/controller` HTTP crash, this path does **not** abort — there is n - Not the same code path as the `/controller` crash. Solve independently. - Worth grepping for `JsonSockEvent` usages that iterate collections (see references in `Somfy.cpp`, `ESPNetwork.cpp`, `GitOTA.cpp`) to identify the most at-risk events. + +## Rolling codes written from two tasks + +**Status:** open +**Location:** `SomfyRemote::sendCommandAfter` and `setRollingCode` in [src/Somfy.cpp](../src/Somfy.cpp), the `/setRollingCode` handlers in [src/Web.cpp](../src/Web.cpp), and `ShadeConfigFile::readShadeRecord`/`readGroupRecord` in [src/ConfigFile.cpp](../src/ConfigFile.cpp). + +### Summary + +Sending a command reads the next rolling code, queues the frame, then commits the code (`peekNextRollingCode` → `submitTx` → `commitRollingCode`), all on `loop()`. `/setRollingCode` and a shades restore change the same remote's `lastRollingCode` and its `ShadeCodes` NVS key from the async web server task, with no lock against that sequence. Each NVS write is safe on its own, since every function opens its own handle (see [ADR-0001](adr/0001-local-nvs-handles-no-wrapper.md)), but the read-then-commit sequence is not. + +### Concrete failure modes + +1. `/setRollingCode` lands between the peek and the commit of a command. The command's commit overwrites the code just set, so the set has no effect. +2. A restore reads `ShadeCodes` while a command commits a newer code. The restored shade can come back with the older code, and the motor rejects its frames until the code passes the newer one. + +### Fix options (pick one later) + +- **(a) Run these handlers on `loop()`.** Queue `/setRollingCode` and restore through the web command queue like shade commands, so `loop()` is the only task that touches a remote's rolling code. Part of making `loop()` the only owner of the shade model. +- **(b) A rolling-code lock.** Hold a mutex across peek → queue → commit, and in `setRollingCode` and restore. Narrower, but it adds a lock to the command path. + +### Related + +- Same root cause as [Concurrent mutation during chunked `/controller` response](#concurrent-mutation-during-chunked-controller-response): the web server task changes the shade model that `loop()` owns. diff --git a/docs/adr/0001-local-nvs-handles-no-wrapper.md b/docs/adr/0001-local-nvs-handles-no-wrapper.md new file mode 100644 index 0000000..8fb0ce1 --- /dev/null +++ b/docs/adr/0001-local-nvs-handles-no-wrapper.md @@ -0,0 +1,12 @@ +# Each function opens NVS with its own Preferences handle, with no lock or wrapper + +A single global `Preferences pref` was used from both `loop()` and the async web server task. A `begin()` from one task could fail while the other task had a namespace open, and its `clear()` or `remove()` then hit the wrong namespace (a radio settings save could erase every rolling code). We replaced the global with a local `Preferences` in each function that uses NVS. We did not put an app-level lock or a wrapper module around it, because ESP-IDF's NVS API already serializes access internally (`nvs::Lock`), each `Preferences` holds its own handle, and its destructor calls `end()`. + +## Considered Options + +- **Global handle plus a mutex-backed `NvsSession`.** Rejected: the lock would only protect the shared handle, which is the actual bug, and it would block `loop()`'s rolling-code writes while `ConfigFile` keeps `ShadeCodes` open across LittleFS reads. Remove the shared handle and nothing is left for the lock to do. +- **An `NvsStore` seam with typed accessors and an in-memory adapter for native tests.** Rejected: it would have exactly one real adapter, so the seam would be hypothetical, and deleting it would just give back `Preferences`. The only NVS logic worth testing (the next rolling code) is already tested natively through `RollingCode.h`, which takes its loader as a parameter. + +## Consequences + +Separate handles make each NVS operation safe on its own, but not a read-then-write sequence across tasks (rolling-code peek/commit in `loop()` against `/setRollingCode` or a restore on the web server task). Those races belong to the model, not to NVS, and are tracked in `docs/KNOWN_ISSUES.md`. diff --git a/src/ConfigFile.cpp b/src/ConfigFile.cpp index b012400..ac8ffad 100644 --- a/src/ConfigFile.cpp +++ b/src/ConfigFile.cpp @@ -8,7 +8,6 @@ static const char *TAG = "ConfigFile"; -extern Preferences pref; #define SHADE_HDR_VER 24 #define SHADE_HDR_SIZE 76 @@ -739,6 +738,7 @@ bool ShadeConfigFile::readSettingsRecord() { return true; } bool ShadeConfigFile::readGroupRecord(SomfyGroup *group) { + Preferences pref; pref.begin("ShadeCodes"); uint32_t startPos = this->file.position(); group->setGroupId(this->readUInt8(255)); @@ -802,6 +802,7 @@ bool ShadeConfigFile::readRoomRecord(SomfyRoom *room) { } bool ShadeConfigFile::readShadeRecord(SomfyShade *shade) { + Preferences pref; pref.begin("ShadeCodes"); uint32_t startPos = this->file.position(); shade->setShadeId(this->readUInt8(255)); diff --git a/src/ConfigSettings.cpp b/src/ConfigSettings.cpp index 925c2a7..b87cc38 100644 --- a/src/ConfigSettings.cpp +++ b/src/ConfigSettings.cpp @@ -10,7 +10,6 @@ static const char *TAG = "Config"; -Preferences pref; void restore_options_t::fromJSON(JsonObject &obj) { if(!obj["shades"].isNull()) this->shades = obj["shades"]; @@ -190,6 +189,7 @@ bool ConfigSettings::begin() { return true; } bool ConfigSettings::load() { + Preferences pref; this->fwVersion.parse(FW_VERSION); this->getAppVersion(); pref.begin("CFG"); @@ -224,6 +224,7 @@ bool ConfigSettings::getAppVersion() { return true; } bool ConfigSettings::save() { + Preferences pref; pref.begin("CFG"); pref.putString("hostname", this->hostname); pref.putBool("ssdpBroadcast", this->ssdpBroadcast); @@ -318,6 +319,7 @@ bool MQTTSettings::fromJSON(JsonObject &obj) { return true; } bool MQTTSettings::save() { + Preferences pref; pref.begin("MQTT"); pref.clear(); pref.putString("protocol", this->protocol); @@ -333,6 +335,7 @@ bool MQTTSettings::save() { return true; } bool MQTTSettings::load() { + Preferences pref; pref.begin("MQTT"); pref.getString("protocol", this->protocol, sizeof(this->protocol)); pref.getString("hostname", this->hostname, sizeof(this->hostname)); @@ -362,6 +365,7 @@ bool NTPSettings::begin() { return true; } bool NTPSettings::save() { + Preferences pref; pref.begin("NTP"); pref.clear(); pref.putString("ntpServer", this->ntpServer); @@ -374,6 +378,7 @@ bool NTPSettings::save() { return true; } bool NTPSettings::load() { + Preferences pref; pref.begin("NTP"); pref.getString("ntpServer", this->ntpServer, sizeof(this->ntpServer)); pref.getString("posixZone", this->posixZone, sizeof(this->posixZone)); @@ -425,6 +430,7 @@ bool IPSettings::toJSON(JsonObject &obj) { return true; } bool IPSettings::save() { + Preferences pref; pref.begin("IP"); pref.clear(); pref.putBool("dhcp", this->dhcp); @@ -437,6 +443,7 @@ bool IPSettings::save() { return true; } bool IPSettings::load() { + Preferences pref; pref.begin("IP"); this->dhcp = pref.getBool("dhcp", true); char buff[16]; @@ -485,6 +492,7 @@ bool SecuritySettings::toJSON(JsonObject &obj) { return true; } bool SecuritySettings::save() { + Preferences pref; pref.begin("SEC"); pref.clear(); pref.putChar("type", static_cast(this->type)); @@ -496,6 +504,7 @@ bool SecuritySettings::save() { return true; } bool SecuritySettings::load() { + Preferences pref; pref.begin("SEC"); this->type = static_cast(pref.getChar("type", 0)); if(pref.isKey("username")) pref.getString("username", this->username, sizeof(this->username)); @@ -530,6 +539,7 @@ bool WifiSettings::toJSON(JsonObject &obj) { return true; } bool WifiSettings::save() { + Preferences pref; pref.begin("WIFI"); pref.clear(); pref.putString("ssid", this->ssid); @@ -540,6 +550,7 @@ bool WifiSettings::save() { return true; } bool WifiSettings::load() { + Preferences pref; pref.begin("WIFI"); pref.getString("ssid", this->ssid, sizeof(this->ssid)); pref.getString("passphrase", this->passphrase, sizeof(this->passphrase)); @@ -623,6 +634,7 @@ bool EthernetSettings::usesPin(uint8_t pin) { return false; } bool EthernetSettings::save() { + Preferences pref; pref.begin("ETH"); pref.clear(); pref.putChar("boardType", this->boardType); @@ -636,6 +648,7 @@ bool EthernetSettings::save() { return true; } bool EthernetSettings::load() { + Preferences pref; pref.begin("ETH"); this->boardType = pref.getChar("boardType", this->boardType); this->phyType = static_cast(pref.getChar("phyType", ETH_PHY_LAN8720)); diff --git a/src/Somfy.cpp b/src/Somfy.cpp index 89a8c2d..3d91e37 100644 --- a/src/Somfy.cpp +++ b/src/Somfy.cpp @@ -20,7 +20,6 @@ static const char *TAG = "Somfy"; -extern Preferences pref; extern SomfyShadeController somfy; extern SocketEmitter sockEmit; extern ConfigSettings settings; @@ -154,6 +153,7 @@ void SomfyShadeController::updateGroupFlags() { } #ifdef USE_NVS bool SomfyShadeController::loadLegacy() { + Preferences pref; ESP_LOGI(TAG, "Loading Legacy shades using NVS"); pref.begin("Shades", true); pref.getBytes("shadeIds", this->m_shadeIds, sizeof(this->m_shadeIds)); @@ -203,6 +203,7 @@ bool SomfyShadeController::loadLegacy() { } #endif bool SomfyShadeController::begin() { + Preferences pref; // Load up all the configuration data. //ShadeConfigFile::getAppVersion(this->appVersion); ESP_LOGI(TAG, "App Version:%u.%u.%u", settings.appVersion.major, settings.appVersion.minor, settings.appVersion.build); @@ -357,6 +358,7 @@ void SomfyGroup::clear() { memset(&this->linkedShades, 0x00, sizeof(this->linkedShades)); } bool SomfyShade::linkRemote(uint32_t address, uint16_t rollingCode) { + Preferences pref; // Check to see if the remote is already linked. If it is // just return true after setting the rolling code for(uint8_t i = 0; i < SOMFY_MAX_LINKED_REMOTES; i++) { @@ -409,6 +411,7 @@ bool SomfyGroup::linkShade(uint8_t shadeId) { } void SomfyShade::commit() { somfy.commit(); } void SomfyShade::commitShadePosition() { + Preferences pref; somfy.isDirty = true; #ifdef USE_NVS char shadeKey[15]; @@ -422,6 +425,7 @@ void SomfyShade::commitShadePosition() { #endif } void SomfyShade::commitMyPosition() { + Preferences pref; somfy.isDirty = true; #ifdef USE_NVS if(somfy.useNVS()) { @@ -435,6 +439,7 @@ void SomfyShade::commitMyPosition() { #endif } void SomfyShade::commitTiltPosition() { + Preferences pref; somfy.isDirty = true; #ifdef USE_NVS if(somfy.useNVS()) { @@ -448,6 +453,7 @@ void SomfyShade::commitTiltPosition() { #endif } bool SomfyShade::unlinkRemote(uint32_t address) { + Preferences pref; for(uint8_t i = 0; i < SOMFY_MAX_LINKED_REMOTES; i++) { if(this->linkedRemotes[i].getRemoteAddress() == address) { this->linkedRemotes[i].setRemoteAddress(0); @@ -917,6 +923,7 @@ void SomfyShade::checkMovement() { } #ifdef USE_NVS void SomfyShade::load() { + Preferences pref; char shadeKey[15]; uint32_t linkedAddresses[SOMFY_MAX_LINKED_REMOTES]; memset(linkedAddresses, 0x00, sizeof(uint32_t) * SOMFY_MAX_LINKED_REMOTES); @@ -2708,6 +2715,7 @@ bool SomfyShade::moveToTarget(float pos, float tilt) { return true; } bool SomfyShade::save() { + Preferences pref; #ifdef USE_NVS if(somfy.useNVS()) { char shadeKey[15]; @@ -3285,6 +3293,7 @@ SomfyShade *SomfyShadeController::addShade(JsonObject &obj) { return shade; } SomfyShade *SomfyShadeController::addShade() { + Preferences pref; uint8_t shadeId = this->getNextShadeId(); // So the next shade id will be the first one we run into with an id of 255 so // if it gets deleted in the middle then it will get the first slot that is empty. @@ -3527,6 +3536,7 @@ uint32_t SomfyShadeController::sendFrame(somfy_frame_t &frame, uint8_t repeat, u return this->transceiver.submitTx(job, priority, delayMs); } bool SomfyShadeController::deleteShade(uint8_t shadeId) { + Preferences pref; for(uint8_t i = 0; i < SOMFY_MAX_SHADES; i++) { if(this->shades[i].getShadeId() == shadeId) { shades[i].emitState("shadeRemoved"); @@ -3594,6 +3604,7 @@ bool SomfyShadeController::loadShadesFile(const char *filename) { return ShadeCo // it has been queued. After that lastRollingCode holds what NVS does. uint16_t SomfyRemote::peekNextRollingCode() { return nextRollingCode(this->m_rollingCodeSynced, this->lastRollingCode, [this]() { + Preferences pref; if(!pref.begin("ShadeCodes")) { ESP_LOGE(TAG, "peekNextRollingCode() pref.begin(ShadeCodes) FAILED"); } @@ -3603,6 +3614,7 @@ uint16_t SomfyRemote::peekNextRollingCode() { }); } void SomfyRemote::commitRollingCode(uint16_t code) { + Preferences pref; if(!pref.begin("ShadeCodes")) { ESP_LOGE(TAG, "commitRollingCode() pref.begin(ShadeCodes) FAILED"); } @@ -3617,6 +3629,7 @@ uint16_t SomfyRemote::p_lastRollingCode(uint16_t code) { return old; } uint16_t SomfyRemote::setRollingCode(uint16_t code) { + Preferences pref; if(this->lastRollingCode != code) { pref.begin("ShadeCodes"); pref.putUShort(this->m_remotePrefId, code); @@ -4295,6 +4308,7 @@ void transceiver_config_t::toJSON(JsonObject& obj) { } */ void transceiver_config_t::save() { + Preferences pref; pref.begin("CC1101"); pref.clear(); pref.putUChar("type", this->type); @@ -4343,13 +4357,14 @@ void transceiver_config_t::save() { ESP_LOGI(TAG, "Save Radio Settings SCK:%u MISO:%u MOSI:%u CSN:%u RX:%u TX:%u", this->SCKPin, this->MISOPin, this->MOSIPin, this->CSNPin, this->RXPin, this->TXPin); } -void transceiver_config_t::removeNVSKey(const char *key) { +void transceiver_config_t::removeNVSKey(Preferences &pref, const char *key) { if(pref.isKey(key)) { ESP_LOGD(TAG, "Removing NVS Key: CC1101.%s", key); pref.remove(key); } } void transceiver_config_t::load() { + Preferences pref; esp_chip_info_t ci; esp_chip_info(&ci); switch(ci.model) { @@ -4413,32 +4428,33 @@ void transceiver_config_t::load() { this->rxBandwidth = pref.getFloat("rxBandwidth", this->rxBandwidth); this->proto = static_cast(pref.getChar("proto", static_cast(this->proto))); this->noiseDetection = pref.getBool("noiseDetect", false); - this->removeNVSKey("internalCCMode"); - this->removeNVSKey("modulationMode"); - this->removeNVSKey("channel"); - this->removeNVSKey("channelSpacing"); - this->removeNVSKey("dataRate"); - this->removeNVSKey("syncMode"); - this->removeNVSKey("syncWordHigh"); - this->removeNVSKey("syncWordLow"); - this->removeNVSKey("addrCheckMode"); - this->removeNVSKey("checkAddr"); - this->removeNVSKey("dataWhitening"); - this->removeNVSKey("pktFormat"); - this->removeNVSKey("pktLengthMode"); - this->removeNVSKey("pktLength"); - this->removeNVSKey("useCRC"); - this->removeNVSKey("autoFlushCRC"); - this->removeNVSKey("disableDCFilter"); - this->removeNVSKey("enableManchester"); - this->removeNVSKey("enableFEC"); - this->removeNVSKey("minPreambleBytes"); - this->removeNVSKey("pqtThreshold"); - this->removeNVSKey("appendStatus"); + this->removeNVSKey(pref, "internalCCMode"); + this->removeNVSKey(pref, "modulationMode"); + this->removeNVSKey(pref, "channel"); + this->removeNVSKey(pref, "channelSpacing"); + this->removeNVSKey(pref, "dataRate"); + this->removeNVSKey(pref, "syncMode"); + this->removeNVSKey(pref, "syncWordHigh"); + this->removeNVSKey(pref, "syncWordLow"); + this->removeNVSKey(pref, "addrCheckMode"); + this->removeNVSKey(pref, "checkAddr"); + this->removeNVSKey(pref, "dataWhitening"); + this->removeNVSKey(pref, "pktFormat"); + this->removeNVSKey(pref, "pktLengthMode"); + this->removeNVSKey(pref, "pktLength"); + this->removeNVSKey(pref, "useCRC"); + this->removeNVSKey(pref, "autoFlushCRC"); + this->removeNVSKey(pref, "disableDCFilter"); + this->removeNVSKey(pref, "enableManchester"); + this->removeNVSKey(pref, "enableFEC"); + this->removeNVSKey(pref, "minPreambleBytes"); + this->removeNVSKey(pref, "pqtThreshold"); + this->removeNVSKey(pref, "appendStatus"); pref.end(); //this->printBuffer = somfy.transceiver.printBuffer; } void transceiver_config_t::apply() { + Preferences pref; // A transmission in flight stops after its current frame so the radio can // be reconfigured. Queued frames are already encoded and go out with the // new settings. diff --git a/src/Somfy.h b/src/Somfy.h index bf3e6a5..9ac9134 100644 --- a/src/Somfy.h +++ b/src/Somfy.h @@ -1,5 +1,6 @@ #ifndef SOMFY_H #define SOMFY_H +#include #include "ConfigSettings.h" #include "WResp.h" @@ -421,7 +422,8 @@ struct transceiver_config_t { void save(); void load(); void apply(); - void removeNVSKey(const char *key); + // Removes a key no longer used from the open CC1101 namespace. + void removeNVSKey(Preferences &pref, const char *key); }; class Transceiver { private: