search

noble / noble-device

A Node.js lib to abstract BLE (Bluetooth Low Energy) peripherals, uses noble
MIT License
97 stars 66 forks source link

Noble can permanently stop scanning after a bluetooth adapter state change #9

Closed raykamp closed 9 years ago

raykamp commented 9 years ago

Hey @sandeepmistry, I've been racking my brain over this one all morning, and could really use some help. Here's the context:

I'm testing this on a Mac

  1. I call NobleDeviceSubclass.discoverByUuid(...)
  2. While Noble is scanning, before it's found a device, I disable my bluetooth adapter.
  3. I then enable the bluetooth adapter.
  4. My initial discover/scan requests is lost. CoreBluetooth does not restart the scan after the CBCentralManager state has changed. However, the NobleDeviceSubclass emitter still has a listener for the the 'discovery' event.
  5. There's a timeout in my code and I call NobleDeviceSubclass.discoverByUuid(...) again.
  6. Now there are 2 listeners assigned to 'discover'. The condition on line 37 in "util.js" is then false, and noble will not start scanning for this discovery request, or future ones.

Is there a way that we can clean up stale listeners on the 'discovery' event?

Perhaps we can add error parameters to the discovery callbacks. In the event that the Bluetooth adapter changes state, scanning is stopped, callbacks fired with an error, and 'discover' listeners are removed.

Very curious to hear your thoughts.

-Ray

sandeepmistry commented 9 years ago

Is there a way that we can clean up stale listeners on the 'discovery' event?

I think we can just remove the listener here: https://github.com/sandeepmistry/noble-device/blob/master/lib/util.js#L44

One is added in the state changes to poweredOn above. The .once would also have to be changed to .on to track the state changes after powered on.

Perhaps we can add error parameters to the discovery callbacks. In the event that the Bluetooth adapter changes state, scanning is stopped, callbacks fired with an error, and 'discover' listeners are removed.

Yes, error aren't handled at all currently. I'm on the fence on this one, the NobleDevice.discover* API's callback parameters have the potential to be called more than once. I was thinking of them more as event callbacks, which usually don't have error parameters.

An alternative would to have an event handler for discover errors, something like:

YourThing.onDiscoverError(function(error) {
  // ...
});

or

YourThing.on('discoverError', function(error) {
  // ...
});

I like the syntax of the second option, but it might be a little more complicated to implement ... thoughts?

Another alternative is to automatically start scanning again, if a discover is pending, after the state returns to powered on. Would that be cleaner?

raykamp commented 9 years ago

@sandeepmistry, nice suggestion about automatically restarting the discovery. I'm up for making things simple for the user and having a pending discovery resume when the state returns to powered on.

It seems like there should be a way to have a user request that a discovery attempt should end. If I call "discoverAll", then I can end this attempt by calling "stopDiscoverAll" with the same listener function. If I call "discover", "discoverWithFilter", or "discoverByUuid" then I'm unable to cancel these discovery attempts. Do you agree?

That makes sense about the error parameters. I also prefer of the syntax of the second option.

sandeepmistry commented 9 years ago

@raykamp so it sounds like a reasonable fix to this issue is to automatically restarting the discovery. It's nice and simple. Then we probably can defer the error handling stuff for now, thoughts?

Good suggestion on having "stopDiscover*" methods, please open a separate issue to track that one.

raykamp commented 9 years ago

Sure thing, @sandeepmistry. I've created a new issue here: https://github.com/sandeepmistry/noble-device/issues/10

I agree in thinking that we can defer the error handling for now.

sandeepmistry commented 9 years ago

@raykamp try out https://github.com/sandeepmistry/noble-device/commit/02f5fb9abfbc964d1edd6e97cb08bd746fa332b1 when you get a chance.

A side effect of this change is peripheral's might be discovered multiple time every time a state change of powered on is triggered - I think this is only for discoverAll ...

raykamp commented 9 years ago

@sandeepmistry, I tested the commit and it is working well.

Good point about that side effect. Personally, I'm not worried by it, but if a user needed, they could write this process of filtering duplicates in their application.

sandeepmistry commented 9 years ago

@raykamp agreed, if the state changes back to powered on, I imagine most users would want to re-connect anyways.