Bluetooth: macOS: Implementing read/write for descriptors

device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm

 // Copyright 2017 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #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)
     : gatt_characteristic_(characteristic),
       cb_descriptor_(descriptor, base::scoped_policy::RETAIN) {
   uuid_ =
       BluetoothAdapterMac::BluetoothUUIDWithCBUUID([cb_descriptor_.get() UUID]);
   identifier_ = base::SysNSStringToUTF8(
       [NSString stringWithFormat:@"%s-%p", uuid_.canonical_value().c_str(),
                                  (void*)cb_descriptor_]);
@@ skipping 10 matching lines @@
 BluetoothGattCharacteristic::Permissions
 BluetoothRemoteGattDescriptorMac::GetPermissions() const {
   NOTIMPLEMENTED();
   return BluetoothGattCharacteristic::PERMISSION_NONE;
 }
 
 const std::vector<uint8_t>& BluetoothRemoteGattDescriptorMac::GetValue() const {
   return value_;
 }
 
 BluetoothRemoteGattDescriptorMac::~BluetoothRemoteGattDescriptorMac() {}

[ortuno, 2017/04/03 04:51:46]

We need to call pending error callbacks when the object gets destroyed. The
necessary code and tests should be very similar to what we have for
characteristics.

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

Done.

 
 // Returns a pointer to the GATT characteristic that this characteristic
 // descriptor belongs to.
 BluetoothRemoteGattCharacteristic*
 BluetoothRemoteGattDescriptorMac::GetCharacteristic() const {
   return static_cast<BluetoothRemoteGattCharacteristic*>(gatt_characteristic_);
 }
 
 // Sends a read request to a remote characteristic descriptor to read its
 // value. |callback| is called to return the read value on success and
 // |error_callback| is called for failures.
 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 {
   return cb_descriptor_.get();
 }
 
 DEVICE_BLUETOOTH_EXPORT std::ostream& operator<<(
     std::ostream& out,
     const BluetoothRemoteGattDescriptorMac& descriptor) {
   const BluetoothRemoteGattCharacteristicMac* characteristic_mac =
       static_cast<const BluetoothRemoteGattCharacteristicMac*>(
           descriptor.GetCharacteristic());
   return out << "<BluetoothRemoteGattServiceMac "
              << descriptor.GetUUID().canonical_value() << "/" << &descriptor
              << ", characteristic: "
              << characteristic_mac->GetUUID().canonical_value() << "/"
              << characteristic_mac << ">";
 }
 
 }  // namespace device.