jasongin / noble-uwp

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

Disconnecting doesn't work properly #20

Open hoffmannjan opened 7 years ago

hoffmannjan commented 7 years ago

I've tested noble-uwp disconnecting

  1. the device isn't properly disconnected on peripherial.disconnect()
  2. when app that device is paired with is terminated, the device is disconnected
  3. added to static void Close() in windows.devices.bluetooth/_nodert_generated.cpp & windows.device.bluetooth.generic.attribute.profile/_nodert_generated.cpp
wrapper->_instance->Dispose();
delete wrapper->_instance;
wrapper->_instance = nullptr;
return;
  1. get access to charachteristics._ref from noble that can use WriteClientCharacteristicConfigurationDescriptorAsync based on this: https://stackoverflow.com/questions/39599252/windows-ble-uwp-disconnect/43609852#43609852

Unfortunately nothings helped :(.

jasongin commented 7 years ago

I think what is needed is to add a call to deviceRecord.device.close() in the noble-uwp disconnect() function. I'm not able to test this at the moment, but I will try it later if you don't get to it first.

hoffmannjan commented 7 years ago

Added line after your suggestion - unfortunately nothing has changed :(.

jasongin commented 7 years ago

I found some discussion of this issue here: https://social.msdn.microsoft.com/Forums/windowsdesktop/en-US/9eae39ff-f6ca-4aa9-adaf-97450f2b4a6c/disconnect-bluetooth-low-energy?forum=wdk

There is a suggestion that also calling close() on all the GattDeviceService objects obtained from the device should allow it to be disconnected. But then others say that still doesn't work.

I'll try to work on this, but it might be a few days before I get the time.

hoffmannjan commented 7 years ago

Tried this as well - nothing changed... 👎

jasongin commented 7 years ago

I tried a few things, but could not get the device to disconnect. As far as I can tell this is a bug / limitation in the Windows bluetooth API.

jasongin commented 7 years ago

FWIW, here is a noble-uwp change that calls close() on the device and all GATT service objects. However as noted above, Windows still does not actually disconnect from the device.

jasongin commented 7 years ago

I've been in contact with the Windows Bluetooth team about this, and submitted details about this issue via the Windows feedback tool, at https://aka.ms/Ss9u8h. They've said they will look into it.

jasongin commented 7 years ago

I see 4 hearts above, so I assume that means others are affected by this as well. If anyone can provide specific scenarios where the disconnection issue is blocking you, that may help the Windows Bluetooth dev team better understand the context and prioritize appropriately.

mhle commented 7 years ago

Hi @jasongin, I've been testing out the UWP bluetooth API and saw some undocumented behaviour that I thought I'd share in case it can help. I noticed that calling disconnect would fail if there are event handlers still attached to the device. In my case it was the BluetoothLeDevice.ConnectionStatusChanged event. In addition an explicit call to GC.Collect() was necessary. Eventually this is what worked for me:

device.ConnectionStatusChanged -= onConnectionStatusChanged;
device.Dispose(); // according to docs, this one line should be enough to disconnect

//Following two lines are undocumented but necessary
device = null;
GC.Collect(); 

Where device is of type BluetoothLEDevice.

According to the docs calling device.Dispose() should be enough to trigger a disconnect but this didn't work for me. Also worth noting is that I didn't have to call Dispose() on any of the services found.

Michael

hoffmannjan commented 7 years ago

I've tried adding GC.Collect to close() in uwp/_nodert_generated.cpp but always get problems with compilation flags while recompiling.

jasongin commented 7 years ago

In a C# UWP app, I also see that a call to GC.Collect() after disposing device objects is necessary to allow the device to disconnect. But noble-uwp doesn't use C#; the code that calls the Windows bluetooth APIs is C++/CX code generated by NodeRT. There is no garbage-collection involved there, only ref-counting. (And JavaScript garbage-collection is irrelevant here because the JS code disposes the C++ objects explicitly.)

noble-uwp does not currently use the BluetoothLEDevice.ConnectionStatusChanged event at all.

I'm still working with the Windows Bluetooth team on this, collecting and sending them additional diagnostic traces. And if necessary I may try to reproduce this issue in just C++/CX, in effort to eliminate any Node.js/NodeRT issues.

jasongin commented 7 years ago

I pushed a commit that ensures all BluetoothLEDevice and GattDeviceService objects are closed and references are released, and published the update as version 0.4.4.

The Windows Bluetooth team has confirmed there is a bug in the BluetoothLEDevice.Close() method. The effect is that the device will not become disconnected until the BluetoothLEDevice object is destructed. That explains why in C# the GC.Collect() call is necessary to cause the device to disconnect.

noble-uwp/NodeRT uses C++, and uses Nan::ObjectWrap such that the lifetime of the BluetoothLEDevice object is tied to the lifetime of the JavaScript projection. So similar to C#, the device will not disconnect until the JS garbage collector destroys the device object. (I was mistaken above in thinking JS GC didn't matter.) In my testing, JS garbage collection seems to happen reliably after all JS references are released. If some applications are having trouble with disconnection they may need to add a call to global.gc() after disconnecting the device. But that is only possible by using the --expose-gc command-line option to node.exe. Once the Windows bug is fixed, a GC should no longer be necessary.

In summary, noble-uwp@0.4.4 mostly fixes this problem, and if you still have disconnection issues you may need to add a call to global.gc() along with the --expose-gc option to node.exe.

hoffmannjan commented 7 years ago

I'm testing your update, and disconnecting seems to work correctly when added global.gc() as you mentioned, which is great! There is only one problem, I suposse that after disconnecting device should be again discoverable and should be listed .on('discover'), am I right? Because now this doesn't happen.

jasongin commented 7 years ago

@hoffmannjan I don't see that problem. I can repeatedly disconnect and rediscover the device (without restarting the node process).

hoffmannjan commented 7 years ago

I can connect, disconnect, and connect again, which gives me the same effect.

Anyway, big thanks for helping with this!

hoffmannjan commented 7 years ago

As I said after upgrade I can disconnect the devices - but sometimes It takes up to 5 minutes after calling disconnect() and global.gc().

jasongin commented 7 years ago

sometimes It takes up to 5 minutes after calling disconnect() and global.gc()

Does it depend on whether you've done some particular operation with the device? Or is it just unpredictable?

hoffmannjan commented 7 years ago

@jasongin totally unpredictable, but more frequently devices disconnect immediately when It is their first connection after starting Windows. Next tries take more time.

taktran commented 7 years ago

I'm seeing this problem on 0.5.1 too. Turning off a device wouldn't get any disconnect callback, even after 5+ minutes

hoffmannjan commented 7 years ago

Same story as @taktran here, can you reproduce this?

jasongin commented 7 years ago

Turning off a device wouldn't get any disconnect callback

I think that's a separate issue. Probably the code needs to listen to the BluetoothLEDevice.ConnectionStatusChanged event so that it can detect disconnections that were not caused by an explicit call to the noble disconnect() method.

jasongin commented 7 years ago

Reporting of disconnection events is fixed in noble-uwp 0.5.4. That should handle situations when the device is turned off or moves out of range.

jasongin commented 7 years ago

I'll try to investigate the issue where the device does not disconnect after calling disconnect() and global.gc(). I have also observed that disconnection works reliably at first after rebooting Windows, but then after some time working with Bluetooth devices it sometimes stops working until a reboot.

I think the rediscovery problem after disconnecting is probably a side effect of the failure to disconnect. If the device does not actually get disconnected, then it does not begin broadcasting advertisements again, so it's not discoverable then. (And noble-uwp discards the device information during the call to disconnect().)

taktran commented 7 years ago

I've upgraded to 0.5.4, and the calls to disconnect() work (albeit with a 30s+ lag), however, I haven't been able to get the disconnect event, even leaving the device off for a couple of minutes.

I've tried rebooting as well, but that doesn't seem to affect it.

Although, it does rediscover, after turning on the device again.

I'm using the following to detect disconnect:

peripheral.once('disconnect', (p, error) => {
  // ...
});
jasongin commented 7 years ago

the calls to disconnect() work (albeit with a 30s+ lag)

Are you calling global.gc() after disconnect()? As explained above, that's currently needed to work around a Windows bug. (Rebooting doesn't fix that problem.)

I haven't been able to get the disconnect event

Hmm... this worked for me. Are you doing anything with the device besides connecting before turning it off? The reason I ask is because Windows doesn't actually connect until a connection is needed to do something, like discover services. If the device was not actually connected, then a disconnection would not be detected.

Currently the noble-uwp 'connect' event is sort of a lie. While Windows doesn't offer an explicit connect API, maybe noble-uwp could go ahead and discover services when connect() is called, and then return the cached results later.

hoffmannjan commented 7 years ago

Hmm... this worked for me. Are you doing anything with the device besides connecting before turning it off?

I've tried same code as @mhle on 0.5.4 to detect turning off the device and still nothing happened. After connecting I've discover services, subscribe on charachteristic, write and all of this in different configurations, still disconnect event never fired.

jasongin commented 7 years ago

OK I'll take another look at this when I get a chance.

subscribe on characteristic

Did you unsubscribe? It's possible that a subscription may prevent Windows from disconnecting. Though if the device gets turned off, I think it shouldn't matter.

hoffmannjan commented 7 years ago

For now we created a hack, but It will be really nice when this feature will work natively, thanks for your attention. Also now in 0.5.4 the disconnect delay is pretty small (max 5sec). Super cool!

omarwin commented 6 years ago

2018 and the problem still here. This still without work, Microsoft does not have idea how to solve this issue? I have checked and they have this problem since many years ago.. It is incredible they have not fix this problem.

I have tried doing this: removing the characteristics in the Characteristic_ValueChanged:

characteristic.ValueChanged -= Characteristic_ValueChanged;

Then removing the ConnectionStatusChanged in the ConnectionStatusChangeHandlerAsync:

bluetoothLEDevice.ConnectionStatusChanged -= ConnectionStatusChangeHandlerAsync; bluetoothLEDevice?.Dispose(); bluetoothLEDevice = null; GC.Collect(); GC.WaitForPendingFinalizers();

and nothing from this works could Microsoft fix this someday?..... Still lighting my device like is connect, Just stop listening the values changes.

hotlips commented 6 years ago

I had similar issues with another BLE Device. What helped for me was to dispose all Gatt-services before disposing the bluetoothLEDevice The explicit GC is still necessary, but no need to WaitForPendingFinalizers in my case. The GC.Collect is a synchronous method (AFAIK) Best Regards, Kristian Lippert

omarwin commented 6 years ago

Thank you so much @hotlips it worked perfectly. so conclusion Try doing this will work:

removing all the characteristics Characteristic_ValueChanged for each service :

characteristic.ValueChanged -= Characteristic_ValueChanged;

after you have to use the .Dispose() for each service

service.Dispose();

Then remove the ConnectionStatusChanged:

bluetoothLEDevice.ConnectionStatusChanged -= ConnectionStatusChangeHandlerAsync; bluetoothLEDevice?.Dispose(); bluetoothLEDevice = null; GC.Collect();

Thanks again @hotlips :) Hope this help more people :)

cseatec commented 4 years ago

I know this thread is old, but I was just having the same issue with Visual Studio 2019 and a C# program for a BLE device. I did not have to remove the ValueChanged for each characteristic. All I had to do to complete dispose of the connection was as follows. After doing this, I was immediately able to reconnect.

service.Dispose(); bluetoothLEDevice.Dispose(); GC.Collect();