NimbleBluetooth: process fromPhoneQueue before toPhoneQueue (fixes bug with 0-length reads during config phase)

src/nimble/NimbleBluetooth.cpp

@@ -144,21 +144,28 @@ class BluetoothPhoneAPI : public PhoneAPI, public concurrency::OSThread
   protected:
     virtual int32_t runOnce() override
     {
-        bool shouldBreakAndRetryLater = false;
-
         while (runOnceHasWorkToDo()) {
-            // Important that we service onRead first, because the onRead callback blocks NimBLE until we clear
+            /*
-            // onReadCallbackIsWaitingForData.
+              PROCESS fromPhoneQueue BEFORE toPhoneQueue:
-            shouldBreakAndRetryLater = runOnceHandleToPhoneQueue(); // push data from getFromRadio to onRead
-            runOnceHandleFromPhoneQueue();                          // pull data from onWrite to handleToRadio
 
-            if (shouldBreakAndRetryLater) {
+              In normal STATE_SEND_PACKETS operation, it's unlikely that we'll have both writes and reads to process at the same
-                // onRead still wants data, but it's not available yet. Return so we can try again when a packet may be ready.
+              time, because either onWrite or onRead will trigger this runOnce. And in STATE_SEND_PACKETS, it's generally ok to
-#ifdef DEBUG_NIMBLE_ON_READ_TIMING
+              service either the reads or writes first.
-                LOG_INFO("BLE runOnce breaking to retry later (leaving onRead waiting)");
+
-#endif
+              However, during the initial setup wantConfig packet, the clients send a write and immediately send a read, and they
-                return 100; // try again in 100ms
+              expect the read will respond to the write. (This also happens when a client goes from STATE_SEND_PACKETS back to
-            }
+              another wantConfig, like the iOS client does when requesting the nodedb after requesting the main config only.)
+
+              So it's safest to always service writes (fromPhoneQueue) before reads (toPhoneQueue), so that any "synchronous"
+              write-then-read sequences from the client work as expected, even if this means we block onRead for a while: this is
+              what the client wants!
+            */
+
+            // PHONE -> RADIO:
+            runOnceHandleFromPhoneQueue(); // pull data from onWrite to handleToRadio
+
+            // RADIO -> PHONE:
+            runOnceHandleToPhoneQueue(); // push data from getFromRadio to onRead
         }
 
         // the run is triggered via NimbleBluetoothToRadioCallback and NimbleBluetoothFromRadioCallback
@@ -220,12 +227,8 @@ class BluetoothPhoneAPI : public PhoneAPI, public concurrency::OSThread
         }
     }
 
-    bool runOnceHandleToPhoneQueue()
+    void runOnceHandleToPhoneQueue()
     {
-        // Returns false normally.
-        // Returns true if we should break out of runOnce and retry later, such as setup states where getFromRadio returns 0
-        // bytes.
-
         // Stack buffer for getFromRadio packet
         uint8_t fromRadioBytes[meshtastic_FromRadio_size] = {0};
         size_t numBytes = 0;
@@ -234,28 +237,15 @@ class BluetoothPhoneAPI : public PhoneAPI, public concurrency::OSThread
             numBytes = getFromRadio(fromRadioBytes);
 
             if (numBytes == 0) {
-                // Client expected a read, but we have nothing to send.
+                /*
-                // Returning a 0-byte packet breaks clients during the config phase, so we have to block onRead until there's a
+                  Client expected a read, but we have nothing to send.
-                // packet ready.
-                if (isSendingPackets()) {
-                    // In STATE_SEND_PACKETS, it is 100% OK to return a 0-byte response, as we expect clients to do read beyond
-                    // notifies regularly, to make sure they have nothing else to read.
-#ifdef DEBUG_NIMBLE_ON_READ_TIMING
-                    LOG_DEBUG("BLE getFromRadio returned numBytes=0, but in STATE_SEND_PACKETS, so clearing "
-                              "onReadCallbackIsWaitingForData flag");
-#endif
-                } else {
-                    // In other states, this breaks clients.
-                    // Return early, leaving onReadCallbackIsWaitingForData==true so onRead knows to try again.
-                    // This gives runOnce a chance to handleToRadio and produce a response.
-#ifdef DEBUG_NIMBLE_ON_READ_TIMING
-                    LOG_DEBUG("BLE getFromRadio returned numBytes=0. Blocking onRead until we have data");
-#endif
 
-                    // Return true to tell runOnce to shouldBreakAndRetryLater, so we don't busy-loop in runOnce even though
+                  In STATE_SEND_PACKETS, it is 100% OK to return a 0-byte response, as we expect clients to do read beyond
-                    // onRead is still waiting!
+                  notifies regularly, to make sure they have nothing else to read.
-                    return true;
+
-                }
+                  In other states, this is fine **so long as we've already processed pending onWrites first**, because the client
+                  may requesting wantConfig and immediately doing a read.
+                */
             } else {
                 // Push to toPhoneQueue, protected by toPhoneMutex. Hold the mutex as briefly as possible.
                 if (toPhoneQueueSize < NIMBLE_BLUETOOTH_TO_PHONE_QUEUE_SIZE) {
@@ -282,8 +272,6 @@ class BluetoothPhoneAPI : public PhoneAPI, public concurrency::OSThread
             // Clear the onReadCallbackIsWaitingForData flag so onRead knows it can proceed.
             onReadCallbackIsWaitingForData = false; // only clear this flag AFTER the push
         }
-
-        return false;
     }
 
     bool runOnceHasWorkFromPhone() { return fromPhoneQueueSize > 0; }
@@ -466,10 +454,6 @@ class NimbleBluetoothFromRadioCallback : public NimBLECharacteristicCallbacks
     virtual void onRead(NimBLECharacteristic *pCharacteristic)
 #endif
     {
-        // In some cases, it seems a new connection starts with a read.
-        // The API has no bytes to send, leading to a timeout. This short-circuits this problem.
-        if (!bluetoothPhoneAPI->isConnected())
-            return;
         // CAUTION: This callback runs in the NimBLE task!!! Don't do anything except communicate with the main task's runOnce.
 
         int currentReadCount = bluetoothPhoneAPI->readCount.fetch_add(1);