Skip to content

Wait for le scan to stop before connecting to device #2

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
steinerj wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Wait for le scan to stop before connecting to device #2

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 17 additions & 11 deletions lib/bbc-microbit-io.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,21 +137,27 @@ BBCMicrobitIO.prototype.digitalRead = function(pin, handler) {
BBCMicrobitIO.prototype._onDiscover = function(microbit) {
debug('on discover', microbit);

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(() => {

@sandeepmistry sandeepmistry Feb 21, 2017

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@steinerj steinerj Feb 22, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK Node 4.0.0. for arrow functions. But happy to change it...

@sandeepmistry sandeepmistry Feb 27, 2017

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, please.

@sandeepmistry sandeepmistry Feb 21, 2017

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't stop scanning called here in noble-device https://github.com/sandeepmistry/noble-device/blob/master/lib/util.js#L72-L77

@steinerj steinerj Feb 22, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@sandeepmistry sandeepmistry Feb 27, 2017

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@sandeepmistry sandeepmistry Feb 21, 2017

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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._microbit = microbit;

this._microbit.on('pinDataChange', this._onPinDataChange.bind(this));
this._microbit.on('pinDataChange', this._onPinDataChange.bind(this));

this._microbit.once('disconnect', function() {
this.emit('disconnect');
}.bind(this));

this._microbit.connectAndSetUp(function() {
this.emit('connect');
this._microbit.once('disconnect', function() {
this.emit('disconnect');
}.bind(this));

this._microbit.subscribePinData(function() {
this.emit('ready');
this._microbit.connectAndSetUp(function(error) {
if (error) {
console.error(error);

@sandeepmistry sandeepmistry Feb 21, 2017

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we emit the error here instead of logging it?

@steinerj steinerj Feb 22, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@sandeepmistry sandeepmistry Feb 27, 2017

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think emit is the J5 way, it should bubble up the error.

@sandeepmistry sandeepmistry Feb 21, 2017

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we emit the error here instead of logging it?

}
this.emit('connect');
this._microbit.subscribePinData(function() {
this.emit('ready');
}.bind(this));

}.bind(this));
}.bind(this));
});
};

BBCMicrobitIO.prototype._onPinDataChange = function(pin, value) {
Expand Down