noble-uwp icon indicating copy to clipboard operation
noble-uwp copied to clipboard

Disconnecting doesn't work properly

Open hoffmannjan opened this issue 7 years ago • 32 comments

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 :(.

hoffmannjan avatar Jun 24 '17 17:06 hoffmannjan

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.

jasongin avatar Jun 27 '17 23:06 jasongin

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

hoffmannjan avatar Jun 28 '17 09:06 hoffmannjan

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.

jasongin avatar Jun 28 '17 16:06 jasongin

Tried this as well - nothing changed... 👎

hoffmannjan avatar Jun 29 '17 09:06 hoffmannjan

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 avatar Jul 05 '17 07:07 jasongin

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 avatar Jul 05 '17 19:07 jasongin

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 avatar Jul 07 '17 05:07 jasongin

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.

jasongin avatar Jul 07 '17 20:07 jasongin

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

mhle avatar Jul 10 '17 12:07 mhle

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

hoffmannjan avatar Jul 10 '17 13:07 hoffmannjan

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 avatar Jul 10 '17 22:07 jasongin

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, [email protected] 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.

jasongin avatar Jul 11 '17 17:07 jasongin

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.

hoffmannjan avatar Jul 11 '17 21:07 hoffmannjan

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

jasongin avatar Jul 11 '17 22:07 jasongin

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

Anyway, big thanks for helping with this!

hoffmannjan avatar Jul 14 '17 10:07 hoffmannjan

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

hoffmannjan avatar Jul 16 '17 18:07 hoffmannjan

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?

jasongin avatar Jul 17 '17 16:07 jasongin

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

hoffmannjan avatar Jul 18 '17 12:07 hoffmannjan

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

taktran avatar Jul 31 '17 18:07 taktran

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

hoffmannjan avatar Jul 31 '17 20:07 hoffmannjan

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 avatar Jul 31 '17 21:07 jasongin

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 avatar Aug 03 '17 00:08 jasongin

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().)

jasongin avatar Aug 03 '17 00:08 jasongin

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) => {
  // ...
});

taktran avatar Aug 03 '17 11:08 taktran

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.

jasongin avatar Aug 03 '17 17:08 jasongin

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.

hoffmannjan avatar Aug 03 '17 22:08 hoffmannjan

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.

jasongin avatar Aug 07 '17 19:08 jasongin

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!

hoffmannjan avatar Aug 07 '17 20:08 hoffmannjan

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.

omarwin avatar Feb 21 '18 13:02 omarwin

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

hotlips avatar Mar 08 '18 07:03 hotlips

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 :)

omarwin avatar Mar 09 '18 11:03 omarwin

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();

cseatec avatar Mar 03 '20 01:03 cseatec