Bluetooth: macOS: Implementing read/write for descriptors

device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm

Index: device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm
diff --git a/device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm b/device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm
index 76cda0c349c19726f6917e40af48db3fd210a4a2..31b98b44e9ada6a39aec3c36642964c2286fce46 100644
--- a/device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm
+++ b/device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm
@@ -4,12 +4,37 @@
 
 #include "device/bluetooth/bluetooth_remote_gatt_descriptor_mac.h"
 
+#include "base/bind.h"
+#include "base/mac/foundation_util.h"
 #include "base/strings/sys_string_conversions.h"
+#include "base/threading/thread_task_runner_handle.h"
 #include "device/bluetooth/bluetooth_adapter_mac.h"
 #include "device/bluetooth/bluetooth_remote_gatt_characteristic_mac.h"
 
+using base::mac::ObjCCast;
+
 namespace device {
 
+std::vector<uint8_t> VectorValueFromNSValue(id nsvalue) {

[ortuno, 2017/04/03 01:27:50]

This name is pretty confusing since nsvalue is not an actual NSValue.

[jlebel, 2017/04/04 22:13:12]

Done.

+  // According to
+  // https://developer.apple.com/reference/corebluetooth/cbdescriptor some of

[ortuno, 2017/04/03 01:27:50]

We should handle all documented cases. It's certainly possible that Apple
notices the bug, fixes it and breaks our code.

[jlebel, 2017/04/04 22:13:12]

Done.

+  // the descriptor values should be NSNumber (like
+  // CBUUIDServerCharacteristicConfigurationString). But while testing, only
+  // NSString and NSData values were received.
+  std::vector<uint8_t> value;
+  if ([nsvalue isKindOfClass:[NSString class]]) {
+    NSString* string = ObjCCast<NSString>(nsvalue);
+    const uint8_t* buffer = (const uint8_t*)(string.UTF8String);
+    value.assign(buffer, buffer + string.length);
+  } else {
+    DCHECK([nsvalue isKindOfClass:[NSData class]]);
+    NSData* data = ObjCCast<NSData>(nsvalue);
+    const uint8_t* buffer = (const uint8_t*)(data.bytes);
+    value.assign(buffer, buffer + data.length);
+  }

[ortuno, 2017/04/03 01:27:50]

Could we log the class if it's not one of the three expected classes?

[jlebel, 2017/04/04 22:13:12]

Done.

+  return value;
+}
+
 BluetoothRemoteGattDescriptorMac::BluetoothRemoteGattDescriptorMac(
     BluetoothRemoteGattCharacteristicMac* characteristic,
     CBDescriptor* descriptor)
@@ -55,19 +80,91 @@ BluetoothRemoteGattDescriptorMac::GetCharacteristic() const {
 void BluetoothRemoteGattDescriptorMac::ReadRemoteDescriptor(
     const ValueCallback& callback,
     const ErrorCallback& error_callback) {
-  NOTIMPLEMENTED();
+  if (value_read_or_write_in_progress_) {
+    VLOG(1) << *this << ": Read failed, already in progress.";
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE,
+        base::Bind(error_callback,
+                   BluetoothRemoteGattService::GATT_ERROR_IN_PROGRESS));
+    return;
+  }
+  VLOG(1) << *this << ": Read value.";
+  value_read_or_write_in_progress_ = true;
+  read_value_callbacks_ = std::make_pair(callback, error_callback);
+  [GetCBPeripheral() readValueForDescriptor:cb_descriptor_];
 }
 
 // Sends a write request to a remote characteristic descriptor, to modify the

[ortuno, 2017/04/03 01:27:50]

Why do we need this comment?

[jlebel, 2017/04/04 22:13:12]

Done.

-// value of the descriptor with the new value |new_value|. |callback| is
-// called to signal success and |error_callback| for failures. This method
-// only applies to remote descriptors and will fail for those that are locally
-// hosted.
+// value of the descriptor with the new value |value|. |callback| is called to
+// signal success and |error_callback| for failures. This method only applies to
+// remote descriptors and will fail for those that are locally hosted.
 void BluetoothRemoteGattDescriptorMac::WriteRemoteDescriptor(
-    const std::vector<uint8_t>& new_value,
+    const std::vector<uint8_t>& value,
     const base::Closure& callback,
     const ErrorCallback& error_callback) {
-  NOTIMPLEMENTED();
+  if (value_read_or_write_in_progress_) {
+    VLOG(1) << *this << ": Write failed, already in progress.";
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE,
+        base::Bind(error_callback,
+                   BluetoothRemoteGattService::GATT_ERROR_IN_PROGRESS));
+    return;
+  }
+  VLOG(1) << *this << ": Write value.";
+  value_read_or_write_in_progress_ = true;
+  write_value_callbacks_ = std::make_pair(callback, error_callback);
+  base::scoped_nsobject<NSData> nsdata_value(
+      [[NSData alloc] initWithBytes:value.data() length:value.size()]);
+  [GetCBPeripheral() writeValue:nsdata_value forDescriptor:GetCBDescriptor()];
+}
+
+void BluetoothRemoteGattDescriptorMac::DidUpdateValueForDescriptor(
+    NSError* error) {
+  if (!value_read_or_write_in_progress_) {
+    VLOG(1) << *this << ": Value updated, no read in progress.";
+    return;
+  }
+  std::pair<ValueCallback, ErrorCallback> callbacks;
+  callbacks.swap(read_value_callbacks_);
+  value_read_or_write_in_progress_ = false;
+  if (error) {
+    BluetoothGattService::GattErrorCode error_code =
+        BluetoothDeviceMac::GetGattErrorCodeFromNSError(error);
+    VLOG(1) << *this << ": Read value failed with error: "
+            << BluetoothAdapterMac::String(error)

[ortuno, 2017/04/03 01:27:50]

Didn't we have a patch to make it easier to print errors?

[jlebel, 2017/04/04 22:13:12]

I did that:
crrev.com/2745323002
crrev.com/2784373003

So that's the best I came up with.

+            << ", converted to error code: " << error_code;
+    callbacks.second.Run(error_code);
+    return;
+  }
+  VLOG(1) << *this << ": Value updated.";

[ortuno, 2017/04/03 01:27:50]

nit: Value read.

[jlebel, 2017/04/04 22:13:12]

Done.

+  value_ = VectorValueFromNSValue(cb_descriptor_.get().value);
+  callbacks.first.Run(value_);
+}
+
+void BluetoothRemoteGattDescriptorMac::DidWriteValueForDescriptor(
+    NSError* error) {
+  if (!value_read_or_write_in_progress_) {
+    VLOG(1) << *this << ": Value written, no write in progress.";
+    return;
+  }
+  std::pair<base::Closure, ErrorCallback> callbacks;
+  callbacks.swap(write_value_callbacks_);
+  value_read_or_write_in_progress_ = false;
+  if (error) {
+    BluetoothGattService::GattErrorCode error_code =
+        BluetoothDeviceMac::GetGattErrorCodeFromNSError(error);
+    VLOG(1) << *this << ": Write value failed with error: "
+            << BluetoothAdapterMac::String(error)
+            << ", converted to error code: " << error_code;
+    callbacks.second.Run(error_code);
+    return;
+  }
+  VLOG(1) << *this << ": Value updated.";

[ortuno, 2017/04/03 01:27:50]

nit: Value written.

[jlebel, 2017/04/04 22:13:12]

Done.

+  callbacks.first.Run();
+}
+
+CBPeripheral* BluetoothRemoteGattDescriptorMac::GetCBPeripheral() const {
+  return gatt_characteristic_->GetCBPeripheral();
 }
 
 CBDescriptor* BluetoothRemoteGattDescriptorMac::GetCBDescriptor() const {