QLowEnergyController - concurrent characteristic writes

Qt's API assumes that several write operations must be
serialized (this is not guaranteed with Core Bluetooth AFAIK).
I can enqueu only write with response operations, otherwise
I never know when to do the next write.

Change-Id: Iaa83514748358437e2c39335ab142d084ff197e3
Reviewed-by: Alex Blasche <alexander.blasche@theqtcompany.com>

5 files changed, 75 insertions, 78 deletions

diff --git a/src/bluetooth/osx/osxbtcentralmanager.mm b/src/bluetooth/osx/osxbtcentralmanager.mm
index 35a284c2..2b4a2dd4 100644
--- a/src/bluetooth/osx/osxbtcentralmanager.mm
+++ b/src/bluetooth/osx/osxbtcentralmanager.mm
@@ -577,7 +577,27 @@ using namespace QT_NAMESPACE;
         writePending = true;
         return [peripheral writeValue:data.data() forDescriptor:d];
     } else {
-        // TODO: characteristics write requests.
+        CBCharacteristic *const ch = charMap[request.handle];
+        Q_ASSERT_X(ch, "-performNextWriteRequest", "invalid characteristic (nil)");
+
+        ObjCStrongReference<NSData> data(data_from_bytearray(request.value));
+        if (!data) {
+            // Even if qtData.size() == 0, we still need NSData object.
+            qCWarning(QT_BT_OSX) << "-performNextWriteRequest, "
+                                    "failed to allocate NSData object";
+            [self performNextWriteRequest];
+        }
+
+        // TODO: check what happens if I'm using NSData with length 0.
+        if (request.withResponse) {
+            writePending = true;
+            [peripheral writeValue:data.data() forCharacteristic:ch
+                        type:CBCharacteristicWriteWithResponse];
+        } else {
+            [peripheral writeValue:data.data() forCharacteristic:ch
+                        type:CBCharacteristicWriteWithoutResponse];
+            [self performNextWriteRequest];
+        }
     }
 }
 
@@ -597,6 +617,22 @@ using namespace QT_NAMESPACE;
         return false;
     }
 
+    LEWriteRequest request;
+    request.isDescriptor = false;
+    request.withResponse = withResponse;
+    request.handle = charHandle;
+    request.value = value;
+
+    writeQueue.enqueue(request);
+    [self performNextWriteRequest];
+    // TODO: this is quite ugly: true value can be returned after
+    // write actually. If I have any problems with the order later,
+    // I'll use performSelector afterDelay with some delay.
+    return true;
+/*
+    // Write without responce is not serialized - no way I can
+    // know about the write operation success/failure - and
+    // I will never perform the next write.
     CBCharacteristic *const ch = charMap[charHandle];
     Q_ASSERT_X(ch, "-write:charHandle:withResponse:", "invalid characteristic (nil) for a give handle");
     Q_ASSERT_X(peripheral, "-write:charHandle:withResponse:", "invalid peripheral (nil)");
@@ -611,9 +647,9 @@ using namespace QT_NAMESPACE;
 
     // TODO: check what happens if I'm using NSData with length 0.
     [peripheral writeValue:data.data() forCharacteristic:ch
-                type: withResponse? CBCharacteristicWriteWithResponse : CBCharacteristicWriteWithoutResponse];
+                type:CBCharacteristicWriteWithoutResponse];
 
-    return true;
+    return true;*/
 }
 
 - (bool)write:(const QByteArray &)value descHandle:(QLowEnergyHandle)descHandle
@@ -1223,6 +1259,9 @@ using namespace QT_NAMESPACE;
         didWriteValueForCharacteristic:(CBCharacteristic *)characteristic
         error:(NSError *)error
 {
+    Q_UNUSED(aPeripheral)
+    Q_UNUSED(characteristic)
+
     // From docs:
     //
     // "This method is invoked only when your app calls the writeValue:forCharacteristic:type:
@@ -1232,8 +1271,7 @@ using namespace QT_NAMESPACE;
 
     using namespace OSXBluetooth;
 
-    Q_UNUSED(aPeripheral)
-    Q_UNUSED(characteristic)
+    writePending = false;
 
     Q_ASSERT_X(delegate, "-peripheral:didWriteValueForCharacteristic:error",
                "invalid delegate (null)");
@@ -1247,9 +1285,14 @@ using namespace QT_NAMESPACE;
         delegate->error(qt_uuid(characteristic.service.UUID), 0,
                         QLowEnergyService::CharacteristicWriteError);
     } else {
-        ObjCStrongReference<CBCharacteristic> ch(characteristic, true);
-        delegate->characteristicWriteNotification(ch);
+        // Keys are unique.
+        const QLowEnergyHandle cHandle = charMap.key(characteristic);
+        Q_ASSERT_X(cHandle, "-peripheral:didWriteValueForCharacteristic:error",
+                   "invalid handle, not found in the characteristics map");
+        delegate->characteristicWriteNotification(cHandle, qt_bytearray(characteristic.value));
     }
+
+    [self performNextWriteRequest];
 }
 
 - (void)peripheral:(CBPeripheral *)aPeripheral
@@ -1272,10 +1315,10 @@ using namespace QT_NAMESPACE;
         delegate->error(qt_uuid(descriptor.characteristic.service.UUID), 0,
                         QLowEnergyService::DescriptorWriteError);
     } else {
-        // We know that keys are unique, so can find a key for a given descriptor.
+        // We know that keys are unique, so we can find a key for a given descriptor.
         const QLowEnergyHandle dHandle = descMap.key(descriptor);
         Q_ASSERT_X(dHandle, "-peripheral:didWriteValueForDescriptor:error:",
-                   "invalid descriptor, not found in a descMap");
+                   "invalid descriptor, not found in the descriptors map");
         delegate->descriptorWriteNotification(dHandle, qt_bytearray(descriptor.value));
     }
 
diff --git a/src/bluetooth/osx/osxbtcentralmanager_p.h b/src/bluetooth/osx/osxbtcentralmanager_p.h
index 2bdea3c0..86c621c2 100644
--- a/src/bluetooth/osx/osxbtcentralmanager_p.h
+++ b/src/bluetooth/osx/osxbtcentralmanager_p.h
@@ -80,7 +80,8 @@ public:
     virtual void connectSuccess() = 0;
     virtual void serviceDiscoveryFinished(LEServices services) = 0;
     virtual void serviceDetailsDiscoveryFinished(LEService service) = 0;
-    virtual void characteristicWriteNotification(LECharacteristic ch) = 0;
+    virtual void characteristicWriteNotification(QLowEnergyHandle charHandle,
+                                                 const QByteArray &value) = 0;
     virtual void descriptorWriteNotification(QLowEnergyHandle descHandle,
                                              const QByteArray &value) = 0;
     virtual void disconnected() = 0;
@@ -120,10 +121,13 @@ typedef QHash<QLowEnergyHandle, CBDescriptor *> DescHash;
 // Descriptor write request - we have to serialize 'concurrent' write requests.
 struct LEWriteRequest
 {
-    LEWriteRequest() : isDescriptor(false), handle(0)
+    LEWriteRequest() : isDescriptor(false),
+                       withResponse(false),
+                       handle(0)
     {}
 
     bool isDescriptor;
+    bool withResponse;
     QLowEnergyHandle handle;
     QByteArray value;
 };
diff --git a/src/bluetooth/qlowenergycontroller_osx.mm b/src/bluetooth/qlowenergycontroller_osx.mm
index 7a938ae9..7191c160 100644
--- a/src/bluetooth/qlowenergycontroller_osx.mm
+++ b/src/bluetooth/qlowenergycontroller_osx.mm
@@ -123,34 +123,6 @@ UUIDList qt_servicesUuids(NSArray *services)
     return uuids;
 }
 
-// TODO: get rid of this.
-QLowEnergyHandle qt_findCharacteristicHandle(QLowEnergyHandle serviceHandle,
-                                             CBService *service, CBCharacteristic *ch)
-{
-    // This mapping from CB -> Qt Qt -> CB is quite verbose and annoying,
-    // but duplicating data structures (CB char-tree, Qt char-tree, etc.)
-    // is even more annoying.
-
-    Q_ASSERT_X(serviceHandle, "qt_findCharacteristicHandle", "invalid service handle (0)");
-    Q_ASSERT_X(service, "qt_findCharacteristicHandle", "invalid service (nil)");
-    Q_ASSERT_X(ch, "qt_findCharacteristicHandle", "invalid characteristic (nil)");
-
-    NSArray *const chars = service.characteristics;
-    if (!chars || !chars.count)
-        return 0; // Invalid handle, to be .. handled by the caller.
-
-    QLowEnergyHandle handle = serviceHandle + 1;
-    for (CBCharacteristic *candidate in chars) {
-        if (candidate == ch)
-            return handle;
-        NSArray *const ds = candidate.descriptors;
-        if (ds && ds.count)
-            handle += ds.count + 1; // + 1 is for char itself.
-    }
-
-    return 0;
-}
-
 }
 
 QLowEnergyControllerPrivateOSX::QLowEnergyControllerPrivateOSX(QLowEnergyController *q)
@@ -158,8 +130,7 @@ QLowEnergyControllerPrivateOSX::QLowEnergyControllerPrivateOSX(QLowEnergyControl
       isConnecting(false),
       lastError(QLowEnergyController::NoError),
       controllerState(QLowEnergyController::UnconnectedState),
-      addressType(QLowEnergyController::PublicAddress),
-      lastValidHandle(0) // 0 == invalid.
+      addressType(QLowEnergyController::PublicAddress)
 {
     // This is the "wrong" constructor - no valid device UUID to connect later.
     Q_ASSERT_X(q, "QLowEnergyControllerPrivate", "invalid q_ptr (null)");
@@ -180,8 +151,7 @@ QLowEnergyControllerPrivateOSX::QLowEnergyControllerPrivateOSX(QLowEnergyControl
       isConnecting(false),
       lastError(QLowEnergyController::NoError),
       controllerState(QLowEnergyController::UnconnectedState),
-      addressType(QLowEnergyController::PublicAddress),
-      lastValidHandle(0) // 0 == invalid.
+      addressType(QLowEnergyController::PublicAddress)
 {
     Q_ASSERT_X(q, "QLowEnergyControllerPrivateOSX", "invalid q_ptr (null)");
     centralManager.reset([[ObjCCentralManager alloc] initWithDelegate:this]);
@@ -338,30 +308,16 @@ void QLowEnergyControllerPrivateOSX::serviceDetailsDiscoveryFinished(LEService s
     qtService->stateChanged(QLowEnergyService::ServiceDiscovered);
 }
 
-void QLowEnergyControllerPrivateOSX::characteristicWriteNotification(LECharacteristic ch)
+void QLowEnergyControllerPrivateOSX::characteristicWriteNotification(QLowEnergyHandle charHandle,
+                                                                     const QByteArray &value)
 {
-    Q_ASSERT_X(ch, "characteristicWriteNotification", "invalid characteristic (nil)");
+    Q_ASSERT_X(charHandle, "characteristicWriteNotification",
+               "invalid characteristic handle(0)");
 
-    QT_BT_MAC_AUTORELEASEPOOL;
-
-    CBService *const cbService = [ch service];
-    const QBluetoothUuid serviceUuid(OSXBluetooth::qt_uuid(cbService.UUID));
-    if (!discoveredServices.contains(serviceUuid)) {
-        qCDebug(QT_BT_OSX) << "QLowEnergyControllerPrivateOSX::characteristicWriteNotification(), "
-                              "unknown service uuid: " << serviceUuid;
-        return;
-    }
-
-    ServicePrivate service(discoveredServices.value(serviceUuid));
-    Q_ASSERT_X(service->startHandle, "characteristicWriteNotification",
-               "invalid service handle (0)");
-
-    const QLowEnergyHandle charHandle =
-        qt_findCharacteristicHandle(service->startHandle, cbService, ch);
-
-    if (!charHandle) {
-        qCDebug(QT_BT_OSX) << "QLowEnergyControllerPrivateOSX::characteristicWriteNotification(), "
-                              "unknown characteristic";
+    ServicePrivate service(serviceForHandle(charHandle));
+    if (service.isNull()) {
+        qCWarning(QT_BT_OSX) << "QLowEnergyControllerPrivateOSX::characteristicWriteNotification(), "
+                                "can not find service for characteristic handle " << charHandle;
         return;
     }
 
@@ -372,9 +328,8 @@ void QLowEnergyControllerPrivateOSX::characteristicWriteNotification(LECharacter
         return;
     }
 
-    const QByteArray data(OSXBluetooth::qt_bytearray([ch value]));
-    updateValueOfCharacteristic(charHandle, data, false);
-    emit service->characteristicWritten(characteristic, data);
+    updateValueOfCharacteristic(charHandle, value, false);
+    emit service->characteristicWritten(characteristic, value);
 }
 
 void QLowEnergyControllerPrivateOSX::descriptorWriteNotification(QLowEnergyHandle dHandle, const QByteArray &value)
@@ -722,7 +677,6 @@ void QLowEnergyControllerPrivateOSX::invalidateServices()
         service->setState(QLowEnergyService::InvalidService);
     }
 
-    lastValidHandle = 0;
     discoveredServices.clear();
 }
 
diff --git a/src/bluetooth/qlowenergycontroller_osx_p.h b/src/bluetooth/qlowenergycontroller_osx_p.h
index df25e88a..c723b1a5 100644
--- a/src/bluetooth/qlowenergycontroller_osx_p.h
+++ b/src/bluetooth/qlowenergycontroller_osx_p.h
@@ -72,7 +72,8 @@ private:
 
     void serviceDiscoveryFinished(LEServices services) Q_DECL_OVERRIDE;
     void serviceDetailsDiscoveryFinished(LEService service) Q_DECL_OVERRIDE;
-    void characteristicWriteNotification(LECharacteristic ch) Q_DECL_OVERRIDE;
+    void characteristicWriteNotification(QLowEnergyHandle charHandle,
+                                         const QByteArray &newValue) Q_DECL_OVERRIDE;
     void descriptorWriteNotification(QLowEnergyHandle descHandle,
                                      const QByteArray &newValue) Q_DECL_OVERRIDE;
     void disconnected() Q_DECL_OVERRIDE;
@@ -141,13 +142,6 @@ private:
     typedef ServiceMap::const_iterator ConstServiceIterator;
     typedef ServiceMap::iterator ServiceIterator;
     ServiceMap discoveredServices;
-
-    // While Core Bluetooth has _startHandle/_endHandle for
-    // CBServices, this information is not a part of a public
-    // API and can not be used. Instead we have to 'emulate'
-    // these handles using something that looks like/works like
-    // handles:
-    QLowEnergyHandle lastValidHandle;
 };
 
 QT_END_NAMESPACE
diff --git a/src/bluetooth/qlowenergyservice_osx.mm b/src/bluetooth/qlowenergyservice_osx.mm
index 996de64b..05763811 100644
--- a/src/bluetooth/qlowenergyservice_osx.mm
+++ b/src/bluetooth/qlowenergyservice_osx.mm
@@ -184,8 +184,10 @@ void QLowEnergyService::writeCharacteristic(const QLowEnergyCharacteristic &ch,
     if (!contains(ch))
         return;
 
-    if (state() != ServiceDiscovered)
+    if (state() != ServiceDiscovered) {
         d_ptr->setError(QLowEnergyService::OperationError);
+        return;
+    }
 
     QLowEnergyControllerPrivateOSX *const controller = qt_mac_le_controller(d_ptr);
     if (!controller)