node-openzwave-shared icon indicating copy to clipboard operation
node-openzwave-shared copied to clipboard

Pass object including the home id as callback arg

Open s0meone opened this issue 8 years ago • 6 comments

❗ 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! 😎

s0meone avatar Jun 28 '17 18:06 s0meone

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?

ekarak avatar Jul 14 '17 17:07 ekarak

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.

s0meone avatar Jul 14 '17 18:07 s0meone

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 avatar Aug 12 '17 07:08 ekarak

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 avatar Aug 12 '17 17:08 ekarak

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.

s0meone avatar Aug 15 '17 09:08 s0meone

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 avatar Aug 24 '17 09:08 ekarak