Wait for le scan to stop before connecting to device#2
Wait for le scan to stop before connecting to device#2steinerj wants to merge 1 commit intosandeepmistry:masterfrom
Conversation
There seems to be a bug in certain Intel BT chipsets which require a LE device scan to be stopped before being able to connect to a device.
|
Didn't pay attention, apologies... This PR depends on noble/noble-device#25 |
|
|
||
| this._microbit = microbit; | ||
| //Certain Intel BT Chipsets seem to require a scan stop before connections are allowed, otherwise they'll throw a: Connection Rejected due to Limited Resources | ||
| BBCMicrobit.stopScanning(() => { |
There was a problem hiding this comment.
What's the minimum version of Node.js needed for the () => { syntax? I'm leaning towards just keeping it as an old school function here. Thoughts?
There was a problem hiding this comment.
AFAIK Node 4.0.0. for arrow functions. But happy to change it...
| this.emit('ready'); | ||
| this._microbit.connectAndSetUp(function(error) { | ||
| if (error) { | ||
| console.error(error); |
There was a problem hiding this comment.
Should we emit the error here instead of logging it?
There was a problem hiding this comment.
Whichever suits your style really :) I had a "throw" initially but felt that was too strong, but thought an emit might be "too weak". Can change.
There was a problem hiding this comment.
I think emit is the J5 way, it should bubble up the error.
|
|
||
| this._microbit = microbit; | ||
| //Certain Intel BT Chipsets seem to require a scan stop before connections are allowed, otherwise they'll throw a: Connection Rejected due to Limited Resources | ||
| BBCMicrobit.stopScanning(() => { |
There was a problem hiding this comment.
Isn't stop scanning called here in noble-device https://github.com/sandeepmistry/noble-device/blob/master/lib/util.js#L72-L77
There was a problem hiding this comment.
It's been a few days so I can't visualise the entire call stack anymore. But if I remember correctly, the discover event listeners are triggered before scanning actually stops on a device level (the actual race condition). The proper solution would probably be to wait for a stopScanning-callback within util.js and then emit discover. But I was hesitant to change a lot in util.js because I didn't have the time to fully understand the effects on other libs depending on noble-device. Also this seems to be an Intel-specific problem, so even more limited scope.
There was a problem hiding this comment.
I think I see what's needed, we're not waiting for the stop scan to complete.
I think this change would be better to have in noble-device.
Change
constructor.discover = function(callback) {
var onDiscover = function(device) {
constructor.stopDiscoverAll(onDiscover);
callback(device);
};
callback._nobleDeviceOnDiscover = onDiscover;
constructor.discoverAll(onDiscover);
};to something like
constructor.discover = function(callback) {
var onDiscover = function(device) {
constructor.stopDiscoverAll(onDiscover, function() {
callback(device);
).bind(this));
};
callback._nobleDeviceOnDiscover = onDiscover;
constructor.discoverAll(onDiscover);
};We'll need to change stopDiscoverAll to include an additional callback parameter. What do you think? Do you have time to PR this?
|
|
||
| this._microbit = microbit; | ||
| //Certain Intel BT Chipsets seem to require a scan stop before connections are allowed, otherwise they'll throw a: Connection Rejected due to Limited Resources | ||
| BBCMicrobit.stopScanning(() => { |
There was a problem hiding this comment.
What's the minimum version of Node.js required to use the () => { syntax? I'm leaning towards just using the old school function syntax here.
| this.emit('ready'); | ||
| this._microbit.connectAndSetUp(function(error) { | ||
| if (error) { | ||
| console.error(error); |
There was a problem hiding this comment.
Shouldn't we emit the error here instead of logging it?
|
@steinerj thanks for submitting! I've made a few review comments above. |
2 participants
There seems to be a bug in certain Intel BT chipsets which require a LE device scan to be stopped before being able to connect to a device.
Discovered in this issue: #1