fix: settings writes silently lost, and unknown keys panic the device

src/JsonSettings.cpp

@@ -4,27 +4,35 @@
 #include <sstream>
 
 String JsonSettings::getString(const char *key) {
+    Guard guard(mutex);
+
     preferences.begin(name, true);
     String value = preferences.getString(key, this->find(key).strDefault);
     preferences.end();
     return value;
 }
 
 int JsonSettings::getInt(const char *key) {
+    Guard guard(mutex);
+
     preferences.begin(name, true);
     int value = preferences.getInt(key, this->find(key).intDefault);
     preferences.end();
     return value;
 }
 
 float JsonSettings::getFloat(const char *key) {
+    Guard guard(mutex);
+
     preferences.begin(name, true);
     float value = preferences.getFloat(key, this->find(key).floatDefault);
     preferences.end();
     return value;
 }
 
 std::vector<int> JsonSettings::getIntVector(const char *key) {
+    Guard guard(mutex);
+
     preferences.begin(name, true);
     String value = preferences.getString(key, this->find(key).strDefault);
     preferences.end();
@@ -45,18 +53,24 @@ std::vector<int> JsonSettings::getIntVector(const char *key) {
 }
 
 void JsonSettings::putString(const char *key, String value) {
+    Guard guard(mutex);
+
     preferences.begin(name, false);
     preferences.putString(key, value);
     preferences.end();
 }
 
 void JsonSettings::putInt(const char *key, int value) {
+    Guard guard(mutex);
+
     preferences.begin(name, false);
     preferences.putInt(key, value);
     preferences.end();
 }
 
 void JsonSettings::putFloat(const char *key, float value) {
+    Guard guard(mutex);
+
     preferences.begin(name, false);
     preferences.putFloat(key, value);
     preferences.end();
@@ -74,6 +88,8 @@ void JsonSettings::putIntVector(const char *key, std::vector<int> value) {
 }
 
 JsonDocument JsonSettings::toJson() {
+    Guard guard(mutex);
+
     JsonDocument settings;
 
     preferences.begin(name, true);
@@ -99,15 +115,30 @@ JsonDocument JsonSettings::toJson() {
 }
 
 bool JsonSettings::fromJson(JsonDocument settings) {
+    Guard guard(mutex);
+
     preferences.begin(name, false);
 
     for (JsonPair kv : settings.as<JsonObject>()) {
         const char *key = kv.key().c_str();
+
+        // Skip keys we do not know about instead of letting find() throw. This
+        // body runs on the web server task, where an uncaught exception panics
+        // the whole device - and a browser still holding a page from a
+        // different firmware version will happily post keys we have never
+        // heard of.
+        if (this->map.find(key) == this->map.end()) {
+            Serial.print("Ignoring unknown setting: ");
+            Serial.println(key);
+            continue;
+        }
+
         JsonSetting setting = this->find(key);
 
         if (! setting.validate(kv.value().as<String>())) {
             lastValidationError = setting.getLastValidationError();
             lastValidationKey = String(key);
+            preferences.end(); // do not leak the nvs handle on the error path
             return false;
         }
 
@@ -125,6 +156,8 @@ bool JsonSettings::fromJson(JsonDocument settings) {
 }
 
 bool JsonSettings::reset() {
+    Guard guard(mutex);
+
     preferences.begin("config", false);
     preferences.clear();
     preferences.end();

src/JsonSettings.h

@@ -5,11 +5,14 @@
 #include <Arduino.h>
 #include <ArduinoJson.h>
 #include <Preferences.h>
+#include <freertos/FreeRTOS.h>
+#include <freertos/semphr.h>
 #include <map>
 
 class JsonSettings {
   public:
-    JsonSettings(const char *name, std::map<String, JsonSetting> map) : name(name), map(map) {}
+    JsonSettings(const char *name, std::map<String, JsonSetting> map)
+        : name(name), map(map), mutex(xSemaphoreCreateRecursiveMutex()) {}
 
     String getString(const char *key);
     int getInt(const char *key);
@@ -37,5 +40,31 @@ class JsonSettings {
 
     JsonSetting find(const char *key);
 
+    // Every accessor below opens and closes NVS on the single `preferences`
+    // instance. The Arduino loop task reads settings (getMode() runs on every
+    // pass through loop()) while the AsyncTCP task writes them from the HTTP
+    // handlers, so without this lock one task's end() closes the handle the
+    // other is mid-way through using: reads silently fall back to the compiled
+    // default and writes are lost while the request still answers "success".
+    //
+    // Recursive because reset() calls fromJson(toJson()), and both take it.
+    SemaphoreHandle_t mutex;
+
+    class Guard {
+      public:
+        explicit Guard(SemaphoreHandle_t mutex) : mutex(mutex) {
+            xSemaphoreTakeRecursive(mutex, portMAX_DELAY);
+        }
+        ~Guard() {
+            xSemaphoreGiveRecursive(mutex);
+        }
+
+        Guard(const Guard &) = delete;
+        Guard &operator=(const Guard &) = delete;
+
+      private:
+        SemaphoreHandle_t mutex;
+    };
+
     Preferences preferences;
 };