Skip to content
This repository has been archived by the owner on Dec 20, 2019. It is now read-only.

Fixes connection error handling (cleaner PR) #57

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fixes connection error handling (cleaner PR) #57

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
809202c
merge jamuus changes
Jan 16, 2016
243eb2e
Merge branch 'master' into connection-error-handling-pr-fix
Feb 27, 2016
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
4 changes: 2 additions & 2 deletions lib/adaptors/serialport.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,13 +52,13 @@ util.inherits(Adaptor, EventEmitter);
*/
Adaptor.prototype.open = function open(callback) {
var self = this,
port = this.serialport = new serialport.SerialPort(this.conn, {});
port = this.serialport = new serialport.SerialPort(this.conn, {}, false);

function emit(name) {
return self.emit.bind(self, name);
}

port.on("open", function(error) {
port.open(function(error) {
Copy link
Author

Choose a reason for hiding this comment

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

This change makes the first test fail.

@jamuus seems like SerialPort doesn't support open()

  1) Serialport Adaptor #open "before each" hook:
     TypeError: port.open is not a function
      at Adaptor.open (lib/adaptors/serialport.js:61:8)
      at Context.<anonymous> (spec/lib/adaptors/serialport.spec.js:41:15)

Copy link
Contributor

Choose a reason for hiding this comment

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

@olcar is exactly correct, Serialport fires open event if/when the port is opened after it has been created. Why change this?

Copy link

Choose a reason for hiding this comment

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

The reason for this change is described in #30. I'm not sure why the test fails, the code has been successfully working for me since the change.

Copy link
Contributor

Choose a reason for hiding this comment

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

You probably need to stub the open method like port.open = stub(); inside the beforeEach code here https://github.com/orbotix/sphero.js/blob/master/spec/lib/adaptors/serialport.spec.js#L12-L16

if (error) {
callback(error);
return;
Expand Down
31 changes: 30 additions & 1 deletion lib/devices/core.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,36 @@ module.exports = function core(device) {
* @return {void}
*/
device.getPowerState = function(callback) {
command(commands.getPwrState, null, callback);
command(commands.getPwrState, null, function(err, data) {
if (err) { return callback(err); }
var batteryState = "";
switch (data.data[1]) {
case 0x01:
batteryState = "Charging";
break;
}
if (data.data[1] === 0x01) {
batteryState = "Charging";
} else if (data.data[1] === 0x02) {
batteryState = "OK";
} else if (data.data[1] === 0x03) {
batteryState = "Low";
} else if (data.data[1] === 0x04) {
batteryState = "Critical";
}

var batteryVoltage = data.data.slice(2, 4).readUInt16BE(0) / 100;
var numCharges = parseInt(data.data[4], 16);
var obj = {
recVer: data.data[0],
batteryState: data.data[1],
batteryStateS: batteryState,
batteryVoltage: batteryVoltage,
chargeCount: numCharges,
secondsSinceCharge: data.data[4],
};
callback(null, obj);
});
Copy link
Author

Choose a reason for hiding this comment

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

This is why the second test fails.

@jamuus not sure why you are doing this, doesn't seem related to the PR title

  2) Core commands #getPowerState calls #command with params:
     AssertionError: expected command to have been called with arguments 0, 32, null, function spy() {}
    command(0, 32, null, function () {})
      at Context.<anonymous> (spec/lib/devices/core.spec.js:66:34)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, this looks like a cool feature, but that should be in a different PR altogether.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jamuus could you please remove this, and submit as part of a separate PR? Thanks.

};

/**
Expand Down
6 changes: 5 additions & 1 deletion lib/sphero.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,11 @@ Sphero.prototype.connect = function(callback) {

connection.on("open", emit("open"));

connection.open(function() {
connection.open(function(error) {
if (error) {
callback(error);
return;
}
self.ready = true;

connection.onRead(function(payload) {
Expand Down