Skip to content
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.

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

Pass object including the home id as callback arg #179

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

Pass object including the home id as callback arg #179

wants to merge 1 commit into from

Conversation

s0meone
Copy link
Contributor

@s0meone s0meone commented Jun 28, 2017

❗ NOT FINISHED, JUST A PROPOSAL

I would like to add support for multiple drivers in node-openzwave because the underlaying OpenZWave stack supports this easily. The main reason we want this is that we want to bridge multiple ZWave networks and we can work around the limit of 232 ZWave devices per network. As you can see from my previous PRs, I'm working my way up to this change.

The remaining problem is that we'll need the current homeid in every event so we can distinguish the different ZWave networks. We can do this easily by just adding the homeid to the end of the argument list for each event. This doesn't break the current API, but it also doesn't feel right.

My proposal is to break the current API now in a constructive manner to allow future expansions more easily. So instead of just adding another argument, pass every event handler an object including all arguments. And now we can also apply the destructuring assignment syntax to only get the arguments we're interested in.

{
  homeid: 123123,
  nodeid: 1,
  ...
}

The following code example is functional within this PR:

var OpenZWave = require("./lib/openzwave-shared.js");

const zwave = new OpenZWave({ ConsoleOutput: false });

// let's start two zwave usb controllers
zwave.connect("/dev/cu.usbmodem1411");
zwave.connect("/dev/cu.usbmodem1451");

// new syntax
zwave.on("node added", ({homeid, nodeid}) => {
  console.log(`Node Added '${nodeid}' in home '0x${homeid.toString(16)}'`);
});

// or the old
zwave.on("node added", function(event) {
  console.log("Node Added '" + event.nodeid + "' in home '0x" + event.homeid.toString(16) + "'");
});

Let me know what you think! 😎

@ekarak
Copy link
Member

ekarak commented Jul 14, 2017

We probably need to refactor this in a way that doesn't break existing clients. This project already has 16 or so dependants. Maybe pass in the required API version upon initialisation?

@s0meone
Copy link
Contributor Author

s0meone commented Jul 14, 2017

Supporting multiple APIs would be a lot of work, to build, but also to maintain.

I'll start with the refactor without breaking the existing API and will see what problems arise. Will keep you updated.

@ekarak
Copy link
Member

ekarak commented Aug 12, 2017

How about creating an interim object encapsulating the homeId via the connect() return value? something like:

const zwave = new OpenZWave({ ConsoleOutput: false });

// let's start two zwave usb controllers
var home1 = zwave.connect("/dev/cu.usbmodem1411");
var home2 = zwave.connect("/dev/cu.usbmodem1451");

home1.on("node added", (nodeid) => { ...
home2.on("node added", (nodeid) => { ...

The main eventEmitter on the OZW singleton would be able to proxy the event api to the last connect() object instantiated, so the legacy code should just work.

By this approach we maintain API compatibility, what do you think?

@ekarak
Copy link
Member

ekarak commented Aug 12, 2017

I've started working on a new branch to enable multiple drivers. I'm going to do this in a way outlined above, namely without breaking the existing API, and using Connect() to create multiple Driver instances. Thank you for your stimulus, I've always wanted to refactor this singleton-driver thing!

@ekarak ekarak closed this Aug 12, 2017
@s0meone
Copy link
Contributor Author

s0meone commented Aug 15, 2017

Sounds great, much better than the solution I had planned. Looking forward to the end result. Let me know if you want me to test things.

@ekarak
Copy link
Member

ekarak commented Aug 24, 2017

ok, I've got something on a branch that you can test:
https://github.com/OpenZWave/node-openzwave-shared/tree/feature/multiple-drivers

I've also changed the API to reflect the ability to connect() to multiple controllers, so take a look at https://github.com/OpenZWave/node-openzwave-shared/blob/feature/multiple-drivers/README-api.md

Let me know how it behaves.

@ekarak ekarak reopened this Aug 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants