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..ec735a628e20ff73164eb207b316ac079ee6eee7 100644
--- a/device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm
+++ b/device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm
@@ -4,19 +4,54 @@
 
 #include "device/bluetooth/bluetooth_remote_gatt_descriptor_mac.h"
 
+#include "base/bind.h"
+#import "base/mac/foundation_util.h"
 #include "base/strings/sys_string_conversions.h"
-#include "device/bluetooth/bluetooth_adapter_mac.h"
-#include "device/bluetooth/bluetooth_remote_gatt_characteristic_mac.h"
+#include "base/threading/thread_task_runner_handle.h"
+#import "device/bluetooth/bluetooth_adapter_mac.h"
+#import "device/bluetooth/bluetooth_remote_gatt_characteristic_mac.h"
+
+using base::mac::ObjCCast;
 
 namespace device {
 
+std::vector<uint8_t> VectorValueFromObjC(id objc_value) {
+  // According to
+  // https://developer.apple.com/reference/corebluetooth/cbdescriptor some
+  // descriptor values can be NSData, NSString or NSNumber.
+  std::vector<uint8_t> value;
+  NSData* data = ObjCCast<NSData>(objc_value);
+  NSString* as_string = ObjCCast<NSString>(objc_value);
+  NSNumber* as_number = ObjCCast<NSNumber>(objc_value);
+
+  if (as_number) {
+    CFNumberRef number = base::mac::CFCast<CFNumberRef>(objc_value);
+    CFIndex byteSize = CFNumberGetByteSize(number);

[tapted, 2017/04/07 04:40:43]

https://developer.apple.com/reference/corefoundation/1542080-cfnumbergetbytes...
says

"Because a CFNumber object might store a value using a type different from that
of the original value with which it was created, this function may return a size
different from the size of the original value's type."

It also says

"In order to improve performance, some commonly-used numbers (such as 0 and 1)
are uniqued. You should not expect that allocating multiple CFNumber instances
will necessarily result in distinct objects."


So whenever a device reports `0` or `1` it's going to point to some shared
NSNumber whose size may not match the bytesize of the number used to create it.


https://developer.apple.com/reference/foundation/nsnumber?language=objc also
says

"(Note that number objects do not necessarily preserve the type they are created
with.)"

(I'd hazard a guess that these descriptors are always `int` anyway..)


So I really think `CFNumberGetByteSize` is giving a false sense of usefulness. 


If you still think otherwise - I don't think we should use malloc -- it doesn't
belong in modern C++. For this case I think it would make sense to skip NSData,
do the value.resize() and assign into it rather than malloc. But that doesn't
override my next concern.


(also byteSize -> byte_size)

[jlebel, 2017/04/09 22:41:32]

This documentation means that if we do +[NSNumber numberWithUnsignedShort:], the
internal storage might be 4 bytes, and then CFNumberGetByteSize() will not match
the original type.
And CFNumberGetType() returns the internal type, not the original type.

+    CFNumberType type = CFNumberGetType(number);

[tapted, 2017/04/07 04:40:43]

Maybe I'm overthinking it, but I can't tell from reading the API whether
CFNumberGetByteSize guarantees that it returns
sizeof(primitive-type-returned-by-CFNumberGetType()). That is, it doesn't seem
guarantee that the buffer will be big enough for CFNumberGetValue to write to.
The way CFNumberGetValue is meant to be used is something like

int value;
if (!CFNumberGetValue(number, k​CFNumber​Int​Type, &value))
  NOTREACHED();


e.g. you could

template <class T>
std::vector<uint8_t> CopyBitsAs(CFNumberRef number,
                                CFNumberType cf_type) {
  T value;
  if (!CFNumberGetValue(number, cf_type, &value))
    NOTREACHED();
  uint8_t* as_bytes = reinterpret_cast<uint8_t*>(&value);
  return std::vector<uint8_t>(as_bytes, as_bytes + sizeof(value));
}


switch (CFNumberGetType(number)) {
  case k​CFNumber​Int​Type:
    return CopyBitsAs<int>(number, k​CFNumber​Int​Type);
  etc.


Note this approach doesn't use CFNumberGetByteSize at all.

But also I don't think this is useful.

[jlebel, 2017/04/09 22:41:32]

Ok, I'm going for the simple version, but at least, I will limit to 2 bytes.
According to the documentation all descriptors with NSNumber should return 2
bytes only. With this, the behavior should the same on macOS and on other
platforms.

+    uint8_t* buffer = (uint8_t*)malloc(byteSize);

[tapted, 2017/04/07 04:40:43]

static_cast<> (style guide prohibits C-style casts.)

[jlebel, 2017/04/09 22:41:32]

Done.

+    CFNumberGetValue(number, type, buffer);
+    data = [NSData dataWithBytes:buffer length:byteSize];
+    free(buffer);
+  }
+  if (as_string)
+    data = [as_string dataUsingEncoding:NSUTF8StringEncoding];
+
+  DCHECK(data) << "Unexpected value: "
+               << base::SysNSStringToUTF8([objc_value description]);
+  if (data) {
+    value.resize([data length]);
+    [data getBytes:value.data() length:value.size()];
+  }
+  return value;
+}
+
 BluetoothRemoteGattDescriptorMac::BluetoothRemoteGattDescriptorMac(
     BluetoothRemoteGattCharacteristicMac* characteristic,
     CBDescriptor* descriptor)
     : gatt_characteristic_(characteristic),
-      cb_descriptor_(descriptor, base::scoped_policy::RETAIN) {
-  uuid_ =
-      BluetoothAdapterMac::BluetoothUUIDWithCBUUID([cb_descriptor_.get() UUID]);
+      cb_descriptor_(descriptor, base::scoped_policy::RETAIN),
+      value_read_or_write_in_progress_(false) {
+  uuid_ = BluetoothAdapterMac::BluetoothUUIDWithCBUUID([cb_descriptor_ UUID]);
   identifier_ = base::SysNSStringToUTF8(
       [NSString stringWithFormat:@"%s-%p", uuid_.canonical_value().c_str(),
                                  (void*)cb_descriptor_]);
@@ -40,10 +75,19 @@ const std::vector<uint8_t>& BluetoothRemoteGattDescriptorMac::GetValue() const {
   return value_;
 }
 
-BluetoothRemoteGattDescriptorMac::~BluetoothRemoteGattDescriptorMac() {}
+BluetoothRemoteGattDescriptorMac::~BluetoothRemoteGattDescriptorMac() {
+  if (!read_value_callbacks_.first.is_null()) {
+    std::pair<ValueCallback, ErrorCallback> callbacks;
+    callbacks.swap(read_value_callbacks_);
+    callbacks.second.Run(BluetoothGattService::GATT_ERROR_FAILED);
+  }
+  if (!write_value_callbacks_.first.is_null()) {
+    std::pair<base::Closure, ErrorCallback> callbacks;
+    callbacks.swap(write_value_callbacks_);
+    callbacks.second.Run(BluetoothGattService::GATT_ERROR_FAILED);
+  }
+}
 
-// Returns a pointer to the GATT characteristic that this characteristic
-// descriptor belongs to.
 BluetoothRemoteGattCharacteristic*
 BluetoothRemoteGattDescriptorMac::GetCharacteristic() const {
   return static_cast<BluetoothRemoteGattCharacteristic*>(gatt_characteristic_);
@@ -55,23 +99,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
-// 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.
 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)
+            << ", converted to error code: " << error_code;
+    callbacks.second.Run(error_code);
+    return;
+  }
+  VLOG(1) << *this << ": Value read.";
+  value_ = VectorValueFromObjC([cb_descriptor_ 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 written.";
+  callbacks.first.Run();
+}
+
+CBPeripheral* BluetoothRemoteGattDescriptorMac::GetCBPeripheral() const {
+  return gatt_characteristic_->GetCBPeripheral();
 }
 
 CBDescriptor* BluetoothRemoteGattDescriptorMac::GetCBDescriptor() const {
-  return cb_descriptor_.get();
+  return cb_descriptor_;
 }
 
 DEVICE_BLUETOOTH_EXPORT std::ostream& operator<<(