fix: settings ops review findings

- The frequency scan ops and reboot run on loop() like every op (ADR 0005).
  endFrequencyScan goes to the front of the loop queue, so it never waits
  behind an unpair that waits for the scan to end.
- A security save that leaves the device Secured, or changes its mode,
  closes every client, the saver too once its reply has gone. None to None
  closes nobody.
- proto must be a radio_proto value; frequency, rxBandwidth and deviation
  are checked against what the CC1101 can be set to.
- wifiNetworks sends fewer networks instead of none when they don't fit.
- A client WiFi scan doesn't start while the device joins a network.
- rebootTime is set before reboot; cancelSettingsSave is freeSettingsSave.
- PROTOCOL.md: pins that may be -1, and the fields the routes sent that
  moved elsewhere. e2e: the WiFi scan test goes by the connection in use.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

src/DeviceOps.cpp

@@ -31,7 +31,7 @@ settings_save_t *newSettingsSave(bool &busy) {
   if(!save) saveBusy.store(false);
   return save;
 }
-void cancelSettingsSave(settings_save_t *save) {
+void freeSettingsSave(settings_save_t *save) {
   delete save;
   saveBusy.store(false);
 }
@@ -160,6 +160,7 @@ static void runSave(const proto_reply_to_t &to, settings_save_t *save) {
   const uint16_t parts = settingsParts(section, req);
   bool reboot = false;
   char token[65] = "";
+  const security_types securityBefore = settings.Security.type;
   switch(section) {
     case settings_section_t::general:
       if(settingsSets(parts, settings_part_t::general)) {
@@ -195,19 +196,23 @@ static void runSave(const proto_reply_to_t &to, settings_save_t *save) {
       break;
     }
   }
-  cancelSettingsSave(save);
+  freeSettingsSave(save);
   ProtoWriter w = protoSocket.beginReply(to);
   w.add(proto_field_t::apiKey, token);
   w.add(proto_field_t::rebooting, reboot);
   protoSocket.endReply(to);
   ESP_LOGI(TAG, "Socket [%lu] saved the %s settings%s", (unsigned long)to.client, settingsSectionName(section), reboot ? ", rebooting" : "");
-  // The other clients' tokens may have changed, and some had none: they
-  // connect again with what they have, and log in when it is refused.
-  if(section == settings_section_t::security) protoSocket.closeAllBut(to.client);
+  // Under Secured every token may have changed, and a client that connected
+  // under None had none: they all connect again, the saver with the token in
+  // its reply, and the rest log in when theirs is refused.  None to None
+  // changes nobody's access.
+  const bool closeAll = section == settings_section_t::security &&
+    (securityBefore != settings.Security.type || settings.requiresAuth());
+  if(closeAll) protoSocket.closeAll(to.client);
   else protoSocket.settingsChanged(to.client, settingsSectionName(section));
   if(reboot) {
-    rebootDelay.reboot = true;
     rebootDelay.rebootTime = millis() + 1000;
+    rebootDelay.reboot = true;
   }
 }
 
@@ -229,8 +234,30 @@ void runDeviceOp(const loop_item_t &item) {
       protoSocket.beginReply(item.replyTo);
       protoSocket.endReply(item.replyTo);
       break;
+    case proto_type_t::beginFrequencyScan:
+      if(!somfy.transceiver.config.enabled) {
+        protoSocket.replyRefused(item.replyTo, proto_error_t::radioUnavailable, "The radio is disabled");
+        break;
+      }
+      somfy.transceiver.beginFrequencyScan();
+      protoSocket.beginReply(item.replyTo);
+      protoSocket.endReply(item.replyTo);
+      break;
+    case proto_type_t::endFrequencyScan:
+      somfy.transceiver.endFrequencyScan();
+      protoSocket.beginReply(item.replyTo);
+      protoSocket.endReply(item.replyTo);
+      break;
+    case proto_type_t::reboot:
+      ESP_LOGI(TAG, "Socket [%lu] asked for a reboot", (unsigned long)item.replyTo.client);
+      protoSocket.beginReply(item.replyTo);
+      protoSocket.endReply(item.replyTo);
+      // Time for the reply to go out.
+      rebootDelay.rebootTime = millis() + 500;
+      rebootDelay.reboot = true;
+      break;
     default:
-      if(item.save) cancelSettingsSave(item.save);
+      if(item.save) freeSettingsSave(item.save);
       protoSocket.replyRefused(item.replyTo, proto_error_t::internal, "Not a device op");
       break;
   }

src/DeviceOps.h

@@ -21,8 +21,8 @@ struct settings_save_t {
 // whichever client sent it: nullptr and busy while another is waiting, and
 // nullptr when there is no memory for it.
 settings_save_t *newSettingsSave(bool &busy);
-// Frees a save that never reached loop().
-void cancelSettingsSave(settings_save_t *save);
+// Frees a save, run or refused, so another may follow.
+void freeSettingsSave(settings_save_t *save);
 // loop(): runs a proto_device item and replies.
 void runDeviceOp(const loop_item_t &item);
 #endif

src/ESPNetwork.cpp

@@ -42,7 +42,8 @@ static void clientScanLoop(ESPNetwork &net) {
     WiFi.scanDelete();
     return;
   }
-  if(!_clientScanWanted || _apScanning) return;
+  // Not in the middle of joining a network either, as the roaming scan isn't.
+  if(!_clientScanWanted || _apScanning || net.connecting()) return;
   _clientScanWanted = false;
   // As /scanaps does: the soft AP's connection attempts would get in its way.
   if(net.softAPOpened) WiFi.disconnect(false);

src/LoopQueue.cpp

@@ -32,11 +32,12 @@ static restore_options_t restoreOptions(uint8_t parts) {
 void LoopQueue::begin() {
   if(!this->queue) this->queue = xQueueCreate(LOOP_QUEUE_SIZE, sizeof(loop_item_t));
 }
-bool LoopQueue::submit(const loop_item_t &item) {
+bool LoopQueue::submit(const loop_item_t &item, bool first) {
   if(!this->queue) return false;
   // async_tcp also services the sockets, so it must never wait on loop() or
   // the radio.
-  if(xQueueSend(static_cast<QueueHandle_t>(this->queue), &item, 0) != pdTRUE) {
+  QueueHandle_t q = static_cast<QueueHandle_t>(this->queue);
+  if((first ? xQueueSendToFront(q, &item, 0) : xQueueSend(q, &item, 0)) != pdTRUE) {
     ESP_LOGE(TAG, "Loop queue full, dropping item");
     return false;
   }

src/LoopQueue.h

@@ -67,8 +67,10 @@ class LoopQueue {
   public:
     void begin();
     // Hands the item to loop() without waiting, so it is safe from any task,
-    // including async_tcp.  False when the queue is full.
-    bool submit(const loop_item_t &item);
+    // including async_tcp.  False when the queue is full.  first puts it
+    // ahead of everything waiting: an item that ends a frequency scan, which
+    // an unpair at the head waits for (holdLoopItem).
+    bool submit(const loop_item_t &item, bool first = false);
     // Runs the waiting items in order.  Only from loop().
     void drain();
   private:

src/ProtoSchema.h

@@ -61,9 +61,9 @@
   TYPE(getSettings, 49, 'c', 'd') \
   TYPE(saveSettings, 50, 'c', 'd') \
   TYPE(scanWifi, 51, 'c', 'd') \
-  TYPE(beginFrequencyScan, 52, 'c', 'a') \
-  TYPE(endFrequencyScan, 53, 'c', 'a') \
-  TYPE(reboot, 54, 'c', 'a') \
+  TYPE(beginFrequencyScan, 52, 'c', 'd') \
+  TYPE(endFrequencyScan, 53, 'c', 'd') \
+  TYPE(reboot, 54, 'c', 'd') \
   TYPE(shadeState, 128, 'd', '-') \
   TYPE(shade, 129, 'd', '-') \
   TYPE(shadeRemoved, 130, 'd', '-') \
@@ -298,11 +298,7 @@ enum class proto_op_t : char {
   remoteFrame = 'f',   // sendRemoteCommand: a frame that goes to the radio at once
   edit = 'e',          // an edit loop() applies
   read = 'r',          // a read loop() answers from the shade model
-  device = 'd',        // a settings op or the WiFi scan, which loop() runs
-  // The frequency scan and reboot, run at once as their routes are: they
-  // touch nothing loop() owns, and a scan's end mustn't wait in the loop
-  // queue behind an unpair that waits for the scan to end.
-  atOnce = 'a'
+  device = 'd'         // a settings, scan or reboot op loop() runs
 };
 const proto_field_info_t &protoField(proto_field_t field);
 const proto_field_info_t *protoFieldByKey(const char *key);

src/ProtoSettings.cpp

@@ -21,9 +21,10 @@ static settings_section_t sectionOf(settings_part_t part) {
   }
 }
 
-// What a save may set.  lo..hi is a whole number's range, and hi a string's
-// longest length: what the setting holds, so the route's fromJSON neither cuts
-// it short nor wraps it.  The has* fields and radioInit are only ever sent.
+// What a save may set.  lo..hi is a whole number's range, a number's in
+// hundredths, and hi a string's longest length: what the setting holds, so the
+// route's fromJSON neither cuts it short nor wraps it.  The has* fields and
+// radioInit are only ever sent.
 struct settings_field_t {
   settings_part_t part;
   proto_field_t field;
@@ -72,6 +73,7 @@ static const settings_field_t FIELDS[] = {
   {settings_part_t::security, proto_field_t::permissions, nullptr, 0, 1},
   // 56 or 80, checked on its own.
   {settings_part_t::radio, proto_field_t::bitLength, "type", 56, 80},
+  // RTS, RTW, RTV, GP Relay or GP Remote (radio_proto), checked on its own.
   {settings_part_t::radio, proto_field_t::proto, nullptr, 0, 9},
   {settings_part_t::radio, proto_field_t::enabled, nullptr, 0, 0},
   {settings_part_t::radio, proto_field_t::TXPin, nullptr, 0, 48},
@@ -80,9 +82,10 @@ static const settings_field_t FIELDS[] = {
   {settings_part_t::radio, proto_field_t::MOSIPin, nullptr, 0, 48},
   {settings_part_t::radio, proto_field_t::MISOPin, nullptr, 0, 48},
   {settings_part_t::radio, proto_field_t::CSNPin, nullptr, 0, 48},
-  {settings_part_t::radio, proto_field_t::frequency, nullptr, 0, 0},
-  {settings_part_t::radio, proto_field_t::rxBandwidth, nullptr, 0, 0},
-  {settings_part_t::radio, proto_field_t::deviation, nullptr, 0, 0},
+  // What the CC1101 can be set to, in MHz and kHz.
+  {settings_part_t::radio, proto_field_t::frequency, nullptr, 30000, 92800},
+  {settings_part_t::radio, proto_field_t::rxBandwidth, nullptr, 5803, 81250},
+  {settings_part_t::radio, proto_field_t::deviation, nullptr, 158, 38085},
   {settings_part_t::radio, proto_field_t::txPower, nullptr, -30, 12},
   {settings_part_t::radio, proto_field_t::noiseDetection, nullptr, 0, 0}
 };
@@ -129,10 +132,23 @@ static const char *checkValue(const settings_field_t &s, const proto_field_info_
       snprintf(why, sizeof(why), "%s must be 56 or 80", f.name);
       return why;
     }
+    if(s.field == proto_field_t::proto) {
+      if(inRange && (value.as<int>() <= 2 || value.as<int>() >= 8)) return nullptr;
+      snprintf(why, sizeof(why), "%s must be 0, 1, 2, 8 or 9", f.name);
+      return why;
+    }
     if(inRange) return nullptr;
     snprintf(why, sizeof(why), "%s must be %ld-%ld", f.name, (long)s.lo, (long)s.hi);
     return why;
   }
+  if(f.kind == 'n') {
+    // Half a hundredth either way, as 58.03 is a little under it in a float.
+    const double hundredths = value.as<double>() * 100;
+    if(hundredths > s.lo - 0.5 && hundredths < s.hi + 0.5) return nullptr;
+    snprintf(why, sizeof(why), "%s must be %ld.%02ld-%ld.%02ld", f.name, (long)(s.lo / 100), (long)(s.lo % 100),
+      (long)(s.hi / 100), (long)(s.hi % 100));
+    return why;
+  }
   if(f.kind != 's') return nullptr;
   const char *text = value.as<const char *>();
   if(strlen(text) > static_cast<size_t>(s.hi)) {

src/outputs/ProtoSocket.cpp

@@ -36,7 +36,6 @@ extern Web webServer;
 extern LoopStats loopStats;
 extern SocketEmitter sockEmit;
 extern LoopQueue loopQueue;
-extern rebootDelay_t rebootDelay;
 
 ProtoSocket protoSocket;
 static AsyncWebSocket ws("/ws");
@@ -244,7 +243,7 @@ static void queueDeviceOp(AsyncWebSocketClient *client, const proto_message_t &m
     }
     const char *bad = expandSettingsPayload(d, HAS_ETHERNET, save->section, save->request);
     if(bad) {
-      cancelSettingsSave(save);
+      freeSettingsSave(save);
       replyError(client, msg.id, proto_error_t::badRequest, bad);
       return;
     }
@@ -252,35 +251,13 @@ static void queueDeviceOp(AsyncWebSocketClient *client, const proto_message_t &m
     save->ip = client->remoteIP();
     item.save = save;
   }
-  if(loopQueue.submit(item)) return;
-  if(item.save) cancelSettingsSave(item.save);
+  // An unpair waits at the head of the queue while a frequency scan runs, so
+  // the scan's end goes ahead of it.
+  const bool first = msg.type == static_cast<uint8_t>(proto_type_t::endFrequencyScan);
+  if(loopQueue.submit(item, first)) return;
+  if(item.save) freeSettingsSave(item.save);
   replyError(client, msg.id, proto_error_t::busy, "Command queue full");
 }
-// The frequency scan and reboot, as their routes run them.
-static void runAtOnce(AsyncWebSocketClient *client, const proto_message_t &msg) {
-  switch(static_cast<proto_type_t>(msg.type)) {
-    case proto_type_t::beginFrequencyScan:
-      if(!somfy.transceiver.config.enabled) {
-        replyError(client, msg.id, proto_error_t::radioUnavailable, "The radio is disabled");
-        return;
-      }
-      somfy.transceiver.beginFrequencyScan();
-      break;
-    case proto_type_t::endFrequencyScan:
-      somfy.transceiver.endFrequencyScan();
-      break;
-    case proto_type_t::reboot:
-      ESP_LOGI(TAG, "Socket [%lu] asked for a reboot", (unsigned long)client->id());
-      // Time for the reply to go out.
-      rebootDelay.reboot = true;
-      rebootDelay.rebootTime = millis() + 500;
-      break;
-    default:
-      replyError(client, msg.id, proto_error_t::internal, "Not an op run at once");
-      return;
-  }
-  replyOk(client, msg.id);
-}
 static bool authorized(AsyncWebServerRequest *request) {
   if(settings.Security.type == security_types::None) return true;
   const AsyncWebParameter *key = request->getParam("apikey");
@@ -369,9 +346,6 @@ static void onMessage(AsyncWebSocketClient *client, const char *text, size_t len
     case proto_op_t::device:
       queueDeviceOp(client, msg, doc["d"]);
       return;
-    case proto_op_t::atOnce:
-      runAtOnce(client, msg);
-      return;
     case proto_op_t::none:
       replyError(client, msg.id, proto_error_t::unknownOp, "Unknown message type");
       return;
@@ -451,6 +425,7 @@ bool ProtoSocket::sending() const { return sockEmit.onLoopTask() && this->hasCli
 /*********************************************************************
  * loop(): syncs
  ********************************************************************/
+static void closeOnceSent();
 static void sendSyncFrame(proto_slot_t &s) {
   LiveSource src;
   char *buf = socketFrameBuffer();
@@ -491,6 +466,7 @@ void ProtoSocket::loop() {
     writeErrorReply(json, 0, proto_error_t::badRequest, "The message took too long to arrive");
     ws.text(stalled, buf, json.length());
   }
+  closeOnceSent();
   ws.cleanupClients(PROTO_MAX_CLIENTS);
 }
 
@@ -754,12 +730,27 @@ void ProtoSocket::settingsChanged(uint32_t savedBy, const char *section) {
     if(id && id != savedBy) ws.text(id, socketFrameBuffer(), eventJson.length());
   }
 }
-void ProtoSocket::closeAllBut(uint32_t keep) {
+// The client that saved the security settings, closed once its reply has
+// gone: a close frame goes ahead of the messages waiting to be sent.
+// loop() only.
+static uint32_t closeWhenSent = 0;
+static uint32_t closeWhenSentSince = 0;
+#define CLOSE_WHEN_SENT_MAX_MS 2000
+void ProtoSocket::closeAll(uint32_t savedBy) {
   if(!sockEmit.onLoopTask()) return;
   for(proto_slot_t &s : slots) {
     const uint32_t id = s.clientId.load();
-    if(id && id != keep) ws.close(id);
+    if(id && id != savedBy) ws.close(id);
   }
+  closeWhenSent = savedBy;
+  closeWhenSentSince = millis();
+}
+static void closeOnceSent() {
+  if(!closeWhenSent) return;
+  AsyncWebSocketClient *client = ws.client(closeWhenSent);
+  if(client && client->queueLen() > 0 && millis() - closeWhenSentSince < CLOSE_WHEN_SENT_MAX_MS) return;
+  if(client) client->close();
+  closeWhenSent = 0;
 }
 // The clients waiting for a WiFi scan.  loop() only.
 static uint32_t scanAskers[PROTO_MAX_CLIENTS];
@@ -781,22 +772,28 @@ void ProtoSocket::wifiNetworks(int16_t count) {
   bool asked = false;
   for(uint32_t id : scanAskers) asked |= id != 0;
   if(!asked) return;
-  ProtoWriter w(beginEvent(proto_type_t::wifiNetworks));
-  w.beginList(proto_field_t::networks);
-  uint8_t listed = 0;
-  for(int16_t i = 0; i < count && listed < PROTO_MAX_NETWORKS; i++) {
-    // As /scanaps lists them: named networks that can be heard.
-    if(WiFi.SSID(i).length() == 0 || WiFi.RSSI(i) < -95) continue;
-    w.beginItem();
-    w.add(proto_field_t::ssid, WiFi.SSID(i).c_str());
-    w.add(proto_field_t::channel, (int32_t)WiFi.channel(i));
-    w.add(proto_field_t::strength, (int32_t)WiFi.RSSI(i));
-    w.add(proto_field_t::macAddress, WiFi.BSSIDstr(i).c_str());
-    w.endItem();
-    listed++;
+  // Each asker was told the networks would follow, so when they don't fit
+  // fewer go: names that need escaping can make one five times longer.
+  bool fits = false;
+  for(uint8_t most = PROTO_MAX_NETWORKS; !fits && most > 0; most /= 2) {
+    ProtoWriter w(beginEvent(proto_type_t::wifiNetworks));
+    w.beginList(proto_field_t::networks);
+    uint8_t listed = 0;
+    for(int16_t i = 0; i < count && listed < most; i++) {
+      // As /scanaps lists them: named networks that can be heard.
+      if(WiFi.SSID(i).length() == 0 || WiFi.RSSI(i) < -95) continue;
+      w.beginItem();
+      w.add(proto_field_t::ssid, WiFi.SSID(i).c_str());
+      w.add(proto_field_t::channel, (int32_t)WiFi.channel(i));
+      w.add(proto_field_t::strength, (int32_t)WiFi.RSSI(i));
+      w.add(proto_field_t::macAddress, WiFi.BSSIDstr(i).c_str());
+      w.endItem();
+      listed++;
+    }
+    w.endList();
+    endFrame(eventJson);
+    fits = !eventJson.overflowed();
   }
-  w.endList();
-  const bool fits = finishEvent();
   for(uint32_t &id : scanAskers) {
     if(fits && id) ws.text(id, socketFrameBuffer(), eventJson.length());
     id = 0;

src/outputs/ProtoSocket.h

@@ -38,9 +38,10 @@ class ProtoSocket {
     void replyRefused(const proto_reply_to_t &to, proto_error_t code, const char *desc);
     // settingsChanged, to every client but the one that saved them.
     void settingsChanged(uint32_t savedBy, const char *section);
-    // Closes every client but keep, as a security save does: their tokens may
-    // no longer be good.
-    void closeAllBut(uint32_t keep);
+    // Closes every client, as a security save does: their tokens may no
+    // longer be good, or they had none.  savedBy is closed once its reply has
+    // gone, so it reads its new token first.
+    void closeAll(uint32_t savedBy);
     // wifiNetworks goes to the clients that asked for a scan since the last
     // one, once it is done: count is WiFi.scanComplete(), and the networks
     // are WiFi's scan results.