jasongin / noble-uwp

Noble (Node.js Bluetooth LE) with Windows 10 UWP bindings
MIT License
83 stars 45 forks source link

connect() returns a null device pointer, causing multiple disconnections. #74

Closed Aendir closed 5 years ago

Aendir commented 5 years ago

In this section of the code for connect() function in bindings.js, I encountered a scenario multiple times where in the following promise returns a null/undefined device pointer.

rt.promisify(BluetoothLEDevice.fromBluetoothAddressAsync)(deviceRecord.address).then(device => {
            debug('got bluetooth device: %s (%s)', device.name, device.deviceInformation.kind);
            deviceRecord.device = rt.trackDisposable(deviceUuid, device);

            deviceRecord.device.on(
                'connectionStatusChanged', this._onConnectionStatusChanged);

            this.emit('connect', deviceUuid, null);
            rt.keepAlive(true);
    }).catch(ex => {
        debug('failed to get device %s: %s', deviceRecord.formattedAddress, ex.stack);
        this.emit('connect', deviceUuid, ex);
    });

On inspecting the code in windows.devices.bluetooth\_nodert_generated.cpp for function FromBluetoothAddressAsync(),

 {
      TryCatch tryCatch;
      arg1 = WrapBluetoothDevice(result);
      if (tryCatch.HasCaught())
      {
        error = Nan::To<Object>(tryCatch.Exception()).ToLocalChecked();
      }
      else 
      {
        error = Undefined();
      }
      if (arg1.IsEmpty()) arg1 = Undefined();
 }
 Local<Value> args[] = {error, arg1};

 invokeCallback(_countof(args), args);

Here arg1 can be returned as undefined.

This scenario was seen 3-4 times, resulting in a JavaScript error and disconnection of all the devices that were connected. Adding a check

rt.promisify(BluetoothLEDevice.fromBluetoothAddressAsync)(deviceRecord.address).then(device => {
        if (device) {
            debug('got bluetooth device: %s (%s)', device.name, device.deviceInformation.kind);
            deviceRecord.device = rt.trackDisposable(deviceUuid, device);

            deviceRecord.device.on(
                'connectionStatusChanged', this._onConnectionStatusChanged);

            this.emit('connect', deviceUuid, null);
            rt.keepAlive(true);
        } else {
            this.emit('connect', deviceUuid, new Error('Failed to acquire bluetooth device pointer from address'));
        }

seems to have fixed this problem.

Is there a reason noble-uwp is allowing device to be undefined/null? Should I raise a PR for this fix?

jasongin commented 5 years ago

Good bug. Can you submit the fix as a PR please.