Discussion surrounding a new paradigm for talking to attached devices

[nathankellenicki]

Something which has been on my mind for a while (which recently came up in a not-entirely-unrelated issue) has been a new paradigm for talking to devices attached to hubs.

Problem Space

Here are a few examples of the current design, and where it falls short.

hub.setMotorSpeed("A", 50);

This method allows you to start running the motor attached to port "A" at a speed 50 constantly. It does no checking against the type of device actually attached to the port, and therefore may or may not work. For example, it will work just fine with an attached light (setting the brightness to half), but will fail against a color sensor.

If device type checking would be added, it would be a lengthy list of compatible devices, which could potentially get out of date as new devices are added.

Similarly:

hub.setLightBrightness("A", 50);

Pretty much does the same as the above, but with less logic. For example setMotorSpeed() changes the underlying command depending on the attached motor and hub combination. However this doesn't - so it may set a motor running at half speed, it may not.

This method is used for rotating a tacho motor to a specified angle:

hub.setMotorAngle("A", 50, 100);

It won't work against non-tacho motors, so a device type check is done at the start of the method (see above comment). As new devices are released with this capability, the method needs to be changed to add more devices to the device check. A similar problem exists with setAbsoluteSpeed().

Another example is for setting the color of the LED on the hub. This is done like so:

hub.setLEDColor(Consts.Color.PINK);

However the color of the LED on attached color sensor can also be changed, which this library doesn't support at the moment. Were it to support it, what would the solution be? Do we add an optional port argument to the method? Or do we create an entirely new method named something along the lines of setLEDColorOnColorSensor()? Neither seem ideal.

Current Proposal

Having thought about this for a little while, an approach that keeps coming to me is to create an individual class for each device, such as SimpleMotor, TrainMotor, BoostMotor, Light, DistanceSensor, TiltSensor, etc.

When one is attached to a hub, an "attach" event is emitted as normal, however instead of having the type id as a parameter, it would contain an instance of the device class, like so:

hub.on("attach", (port, device) => {
    console.log(`Device attached - port ${port}, type ${device.type}`);
    if (device instanceof Light) {
        const light = device;
        light.setBrightness(50); // Set half brightness
    } else if (device instanceof BoostMotor) {
        const motor = device;
        motor.rotateByAngle(66, 100); // Rotate by 66 degrees at full speed
    }
});

Or, you could retrieve the device from the hub:

hub.getDeviceAtPort("A").setBrightness(50); // Assuming the device on port A is a light

Having an individual class for each device or device type would also allow greater control of the various modes each device supports, which while already supported by this library through the subscribe method, is not at all bug free.

For example, the following examples could be implemented:

colorSensor.enableProximityMode();

Which would result in proximity events being emitted. Or:

colorSensor.enableCountMode();

Which would result in count events being emitted.

Or even combining them (and subsequently separating them) using the ability of the LEGO Wireless Protocol to combine modes:

colorSensor.enableProximityMode();
colorSensor.enableCountMode();

Which could result in both events being emitted, or even a new combined event (proximityAndCount) with the values of both.

And then:

colorSensor.disableCountMode();

Dynamically reverting to proximity events only.

Closing

This solution doesn't solve all problems, and could be quite tricky to implement. For example taking care to prevent modes being combined that can't be combined. However any further thoughts or suggestions by anyone is interested is welcome. :)

(cc'ing previous participants @nutki, @aileo, @dlech)

[aileo]

About attach event, I would prefer having the port as a private attribute of device with a getter, then the only parameter whould be the device:

hub.on("attach", (device) => {
    console.log(`Device attached - port ${device.port}, type ${device.type}`);
    if (device instanceof Light) {
        const light = device;
        light.setBrightness(50); // Set half brightness
    } else if (device instanceof BoostMotor) {
        const motor = device;
        motor.rotateByAngle(66, 100); // Rotate by 66 degrees at full speed
    }
});

Events

I think combined events should not take over the single events as the actual features using values of the sensor could be unrelated to each other:

// some feature require proximity
colorSensor.enableProximityMode();
// from now we emit only proximity events
...
// some other feature require count
colorSensor.enableCountMode();
// from now we emit proximity, count and proximityAndCount events
...
// proximity not required anymore
colorSensor.disableProximityMode();
// from now we emit only count events

I think it would be nice to have events at all levels of the "tree":

PoweredUp.on("tilt", (hub, device, x, y, z) => { ... });
hub.on("tilt", (device, x, y, z) => { ... });
device.on("tilt", (x, y, z) => { ... });

// or, if we can access hub from device, an unified version:
PoweredUp.on("tilt", (device, x, y, z) => { ... });
hub.on("tilt", (device, x, y, z) => { ... });
device.on("tilt", (device, x, y, z) => { ... });

And maybe some object for event value so every event looks the same:

hub.on("tilt", (device, { x, y, z }) => { ... });
hub.on("speed", (device, { speed }) => { ... });

The plus on this one is the values are named so you cant take a angle for a speed.

Devices

As discussed in #39 , we must define what should be a device and what should not. Obviously, every physicaly plugable device would be a device... But what about built-in leds, buttons, tilt sensors ? I would say everything that is part of the physical UX should be a device, i.e:

• plugables (motors, sensors)
• internal motors (boost A & B)
• internal buttons (green, PUPRemote)
• internal tilt & accel
• internal led

It excludes mostly the "system" information like battery level, current, voltage, CPU temp.

Getters

Based on already implemented methods:

poweredUp.getHubByUuid(uuid: string);
> hub

poweredUp.getHubByPrimaryMACAddress(address: string);
> hub

poweredUp.getHubs();
> hub[]

poweredUp.getHubsByName(name: String);
> hub[]

poweredUp.getHubsByType(type: HubType);
> hub[]

poweredUp.getDevices();
hub.getDevices();
> Device[]

poweredUp.getDevicesByType(type: DeviceType);
hub.getDevicesByType(type: DeviceType);
> Device[]

// Use case for the last one: you want to light up everything as the night has come.
// then you make some night mode function to call:
function nightMode() {
  poweredUp.getDevicesByType(DeviceType.LED_LIGHTS).forEach((light) => {
    light.setBrightness(100);
  });
}

removed the Connected part of method names as you can't get not connected hubs.

Virtual ports

What about virtual ports with this new paradigm ? I don't know if they are really used from a developer perspective...

Unknow device

With new products, it is possible to have unknown yet connected device to a port. I think it should create a Device instance (I assume all device type classes herit from a Device class). To enable user to manage not implemented devices, PoweredUp could allow to set the unknown device class:

import { PoweredUp, Device } from 'node-poweredup';

class CustomDevice extends Device {...}

const poweredUp = new PoweredUp({ unknownDeviceClass: CustomDevice });

Maybe I am going too far, but you asked for suggestions :)

[nutki]

Thank you for the write up @nathankellenicki and @aileo and inviting me to comment. I had quite some thoughts about the improvements of the library myself.

I don't have time to put all that in writing, so my comments may be a bit incoherent, sorry for that.

Device discovery

We are all seem to agree on the usefulness of an object representation of LPF devices (both internal and external). Nathan and Leo are suggesting the following approach:

  const dev = hub.getDeviceAtPort("A");

This is a fair way to do it and I can see where this is coming from, but I will argue that the following would be more convenient in many cases (of course the both could coexist in the API):

  const dev = hub.getMotorAtPort("A");

There seem to be little difference between them, but my assumption is that if I am trying to get a device handle I will typically know what to expect there because this is part of my own build. This is how I see those approaches in actual code:

  const motor = hub.getMotorAtPort("A");
  await motor.speed(100); // This would do nothing or throw an error if no motor is connected to port A

  const motor = hub.getDeviceAtPort("A");
  if (!(motor instanceof BoostMotor)) throw "Error"; // Or return
  await motor.speed(100);

The essential part of my proposal is that the checks to actual device port type are done per request not on the getMotor call. I will show how this affects simple controller programs. I am basing this on my understanding of getDeviceAtPort semantics, I hope I am not misrepresenting it.

The current state:

  someEmitter.on("key1", async () => await hub.brakeMotor("A"));
  someEmitter.on("key2", async () => await hub.setMotorSpeed("A", -100));

With my proposal this requires just one extra line:

  const motor = hub.getMotorAtPort("A");
  someEmitter.on("key1", async () => await motor.brake());
  someEmitter.on("key2", async () => await motor.speed(-100));

Note that both in the current state and my proposed implementation the motor can be connected/reconnected at any time before or after the program starts (there may be a runtime exception when a key is pressed and no motor is connected).

With generic getDevice:

  const motor = hub.getDeviceAtPort("A") as BoostMotor; // In TS this type assertion is now neccessary bacause the type check below will not cover the asynchronous event handlers
  if (!(motor instanceof BoostMotor)) throw "Error"; // Or return
  someEmitter.on("key1", async () => await motor.brake());
  someEmitter.on("key2", async () => await motor.speed(-100));

However this code requires motor to be connected at the program start. To make it really equivalent to the original code, more asynchronous handling is required:

  let motor: BoostMotor;
  hub.on("attach", (device) => {
    if (device instanceof BoostMotor && device.port === "A") {
      motor = device;
    }
  })
  someEmitter.on("key1", async () => await motor.brake());
  someEmitter.on("key2", async () => await motor.speed(-100));

I am also not sure how the generic device idea would cope with changes to port connectivity during execution, for example:

  const motor = hub.getDeviceAtPort("A"); // motor is connected to A
  if (!(motor instanceof Motor)) return;
  await motor.speed(100);
  // Now motor gets disconnected from A
  await motor.speed(0); // What happens here?
  // Now LED Lights are connected to A
  await motor.speed(100); // Will this turn on the lights?
  // Same type of motor is reconnected to A
  await motor.speed(100); // Does this work again?
  // Another type of motor is connected to A
  await motor.speed(100); // How about this?

The getDeviceAtPort could be implemented as follows to reconcile both:

getDeviceAtPort(port: string): Device | undefined {
  const t = this.getPortDeviceType(port);
  switch(t) {
    case DeviceType.BASIC_MOTOR: return this.getMotorAt(port);
    case DeviceType.LED_LIGHTS: return this.getLightAt(port);
    ...
    default: return undefined;
  }
}

[aileo]

About reconnections, following example should work in any case:

someEmitter.on("key1", async () => await hub.getMotorAtPort("A").brake());
someEmitter.on("key2", async () => await hub.getMotorAtPort("A").speed(-100));
// or
someEmitter.on("key1", async () => await hub.getDeviceAtPort("A").brake());
someEmitter.on("key2", async () => await hub.getDeviceAtPort("A").speed(-100));

I am also not sure how the generic device idea would cope with changes to port connectivity during execution

As you get the same object from both methods, it should react the same way. getDeviceAtPort, from my understanding, would return a device specific object depending on actual connected device.

If Device objects are valide over multiple reconnections, they may have some "offline" flag to know if the actual device connected to the port is, at least, from the expected type of device. This could allow error handling on calling speed on a disconnected motor for example.

My first thought was a Device instance was created on attach events but "destroyed" on detach (but I don't think it is really possible in JS/TS).

What bother me with the device objects working between reconnections is that the hubs does not. I mean, if you disconnect your hub and reconnect, you have to use the new instance to interract whith it.

---

I will typically know what to expect there because this is part of my own build

One of my use case is a control app for multiple trains for my daughter and she really does not care about where she plugs motors and lights so the app handle device mapping but I can understand this is not the main usage :)

[nutki]

Commenting @aileo suggestions:

I think it would be nice to have events at all levels of the "tree":

PoweredUp.on("tilt", (hub, device, x, y, z) => { ... });
hub.on("tilt", (device, x, y, z) => { ... });
device.on("tilt", (x, y, z) => { ... });

// or, if we can access hub from device, an unified version:
PoweredUp.on("tilt", (device, x, y, z) => { ... });
hub.on("tilt", (device, x, y, z) => { ... });
device.on("tilt", (device, x, y, z) => { ... });

I am not convinced this is necessary (how much value this adds for the added complexity), but if we go for it I much prefer the latter approach.

And maybe some object for event value so every event looks the same:

hub.on("tilt", (device, { x, y, z }) => { ... });
hub.on("speed", (device, { speed }) => { ... });

Agreed, I think it is better with structured values.

I would say everything that is part of the physical UX should be a device, i.e: ...

* internal buttons (green, PUPRemote)

This is one case in which I would hesitate to map these to devices. Note that the PUP Remote has 7 independent physical buttons where 6 are grouped in 2 ports, while the green one is not using the port/device abstraction at all (like on the other hubs). How many API devices would you map them to? 7, 3, 2, or 1? We should also take into account that it is likely that a single binary push button will be released as an external device at some point (It does have a device number assigned in the LWP spec, but on the other hand the push button coming soon in Spike sets is supposedly non-binary).

What about virtual ports with this new paradigm ? I don't know if they are really used from a developer perspective...

They can be used to have perfectly synchronized motors in driving bases. The official LWP specification refers to "drive base" multiple times when talking about virtual ports, so I guess this is their intended purpose. I did not verify how well you can synchronize the motors using separate BT messages, but we should not exclude this functionality just because we have difficulty expressing it in an API. That being said I don't see why we couldn't have:

  interface DualMotor extends Motor {
    async speeds(speedA: number, speedB: number): void;
    ...
  }
  getDualMotorAtPort(portPair: string): DualMotor;
  getDeviceAtPort(portPair: string): Device; // Returning DualMotor for boostHub.getDeviceAtPort("AB") for example

With new products, it is possible to have unknown yet connected device to a port. I think it should create a Device instance (I assume all device type classes herit from a Device class). To enable user to manage not implemented devices, PoweredUp could allow to set the unknown device class:

Would this be for the users to implement their own device handlers? Seems unnecessary to me. Wouldn't it make more sense to contribute to the library itself so others could benefit from it as well?

[nutki]

About reconnections, following example should work in any case:

someEmitter.on("key1", async () => await hub.getMotorAtPort("A").brake());
someEmitter.on("key2", async () => await hub.getMotorAtPort("A").speed(-100));
// or
someEmitter.on("key1", async () => await hub.getDeviceAtPort("A").brake());
someEmitter.on("key2", async () => await hub.getDeviceAtPort("A").speed(-100));

True, but this has to me serious drawbacks:

• it is unintive to retrieve the device handle each time so this is probably not how the users will code it at first
• again users are required to store the hub object an port name of a given device somewhere in the code, so we are back to the old approach with slighlty more wordy syntax (hub.getDeviceAtPort("A").speed(-100) vs hub.setMotorSpeed("A", -100))

What bother me with the device objects working between reconnections is that the hubs does not. I mean, if you disconnect your hub and reconnect, you have to use the new instance to interract whith it.

Yes, I also spotted this inconsistency. Ideally I would add reconnecting to hubs as well. It does not seem to be technically difficult.

I will typically know what to expect there because this is part of my own build

One of my use case is a control app for multiple trains for my daughter and she really does not care about where she plugs motors and lights so the app handle device mapping but I can understand this is not the main usage :)

I don't think this is a far off use case, but even there, I think, more explicit apporach would result in cleaner code. In #39 you suggested:

const heA = horizonPupHub.getDeviceByPort("A");
const heB = horizonPupHub.getDeviceByPort("B");

const horizonExpress = {
  motor: heA.type === DeviceType.TRAIN_MOTOR ? heA : heB,
  lights: heA.type === DeviceType.LED_LIGHTS ? heA : heB,
}

Instead you could:

const horizonExpress = {
  motor: horizonPupHub.getMotorAtPort(horizonPupHub.findDeviceByType(DeviceType.TRAIN_MOTOR)),
  lights: horizonPupHub.getLightsAtPort(horizonPupHub.findDeviceByType(DeviceType.LED_LIGHTS)),
}

In my opinion this is easier to read. But also it is more robust if you accidently connect something that is not train motor or lights and is less error prone (for mixing heA and heB in the code).

[aileo]

I am not convinced this is necessary (how much value this adds for the added complexity), but if we go for it I much prefer the latter approach.

For the top level, I don't think it is very usefull. Still I think emiting event from both device and hub is nice. To limit implementation complexity, I think devices could listen to all relevant events (related to its type) from their hub and emit only those from its own port.

This is one case in which I would hesitate to map these to devices. Note that the PUP Remote has 7 independent physical buttons where 6 are grouped in 2 ports, while the green one is not using the port/device abstraction at all (like on the other hubs). How many API devices would you map them to? 7, 3, 2, or 1? We should also take into account that it is likely that a single binary push button will be released as an external device at some point (It does have a device number assigned in the LWP spec, but on the other hand the push button coming soon in Spike sets is supposedly non-binary).

I would say that as all tilt sensor have their own type, buttons from left and right port on the remote would be a RemoteButtonSet device or something like this. This makes me wonder if all motors would share the same object (I think not due to tacho/not tacho). Another way to put the limit between device and hub feature could be: Everything every hub have in common (green button, led, battery, ...) is a hub feature and the rest are devices. In other words, LWP3 hub properties are features, the rest are port devices.

They can be used to have perfectly synchronized motors in driving bases. The official LWP specification refers to "drive base" multiple times when talking about virtual ports, so I guess this is their intended purpose. I did not verify how well you can synchronize the motors using separate BT messages, but we should not exclude this functionality just because we have difficulty expressing it in an API.

Totally agree. Sometime messages get lost so separate messages could be disapointing. Should it work with a set of two differents types motor ? With different types of device ? I would say yes to both as it is in the LWP3 virtual port setup example but it could make the dual devices classes very complex. I also wonder if it is possible to setup a custom virtual port, I am not sure of my understanding of this part of the LWP3. If so, it could be nice to create it "on the fly" when calling getDualMotorAtPort if it does not already exists.

Would this be for the users to implement their own device handlers? Seems unnecessary to me. Wouldn't it make more sense to contribute to the library itself so others could benefit from it as well?

Agreed, I was thinking about makers and their physical custom devices but now I think it is out of scope.

Yes, I also spotted this inconsistency. Ideally I would add reconnecting to hubs as well. It does not seem to be technically difficult.

It would be very nice.

const horizonExpress = {
  motor: horizonPupHub.getMotorAtPort(horizonPupHub.findDeviceByType(DeviceType.TRAIN_MOTOR)),
  lights: horizonPupHub.getLightsAtPort(horizonPupHub.findDeviceByType(DeviceType.LED_LIGHTS)),
}

It looks good to me, I would just handle that there could be multiple devices of the same type on the hub and as you precise the device type, you could receive the device object instread of its port, like:

const horizonExpress = {
  motor: horizonPupHub.getDevicesByType(DeviceType.TRAIN_MOTOR)[0], // I assume the first one is the good one
  lights: horizonPupHub.getDevicesByType(DeviceType.LED_LIGHTS)[0],
}

I tend to store only scalars/serializables in my applications state due to some framework limitations (like usage of Object.assign in state managment precisely to remove external references). It influence a lot the way I see events handling and objects references retrieval.

[MrShinyAndNew]

I'm not a Node expert so forgive me if what I suggest isn't idiomatic. But I can describe some issues I have with my code:

1. I have a program that needs to be generic for any connected devices (it's used for diagnostics) so it has a lot of "if this device type, do that" code. I need to be able to "get device at port X" and then do something useful without knowing ahead of time what hubs are connected or what devices are connected.
2. In the same program I have 'moc' scripts which attach behaviour to some of the hubs/devices. These scripts are written with a specific configuration in mind, but for flexibility I typically assume "if there is a motor on this hub, it is the train motor" and "if there is a handset, it is the train handset". This allows me to reconnect the motor to either port after changing batteries.
3. In combination, the diagnostic control panel and the behavriour script can step on each other's toes because they don't have a way of finding out the state of the system. For example, if the diagnostic control panel sets the motor speed, the train system won't know the motor speed was changed, because there is no place where the speed is stored globally (that I know of). So it would be nice if all commands to modify a device went through a single api that could keep track of the expected state of the device.

In unrelated news, it would be great if this library worked on Node > 8 out of the box.

[MrShinyAndNew]

I'd like to amend my previous comment, it does appear to work on Node 12 out of the box now, but I'm still concerned about the long-term dependency on the abandoned bluetooth library.

[nathankellenicki]

Thanks for the reply!

About attach event, I would prefer having the port as a private attribute of device with a getter, then the only parameter whould be the device:

Can you explain what the downside to be of including the port as an event parameter? It would allow for cleaner code in situations where the application doesn't need to use the device immediately - it. add the device to the port map, without needing to touch the device object.

I think combined events should not take over the single events as the actual features using values of the sensor could be unrelated to each other:

That's absolutely fair. Certain situations may not care about the other events/features, and making that code unsubscribe and resubscribe to a different event wouldn't be ideal. I think I agree with you on this.

I think it would be nice to have events at all levels of the "tree":

PoweredUp.on("tilt", (hub, device, x, y, z) => { ... });
hub.on("tilt", (device, x, y, z) => { ... });
device.on("tilt", (x, y, z) => { ... });

Hmm, I struggle with this one. This would mean that there is a tight coupling between the library, hubs and device objects, leading to a mess of circular references. It also would lead to a lot more events being emitted, which adds a performance tax onto the Node.js event loop - especially I would assert its rare a user would need to be subscribed to more than one of them at the time. It also adds code complexity and could lead to bugs introducing dangling references, affecting GC.

I think part of the point of this initiative is to decouple the hubs and devices, so coupling them back together like this defeats the purpose a bit.

And maybe some object for event value so every event looks the same:

hub.on("tilt", (device, { x, y, z }) => { ... });
hub.on("speed", (device, { speed }) => { ... });

This is interesting and something I've been considering doing in general across my personal and work related projects. It certainly has some advantages - as you state, values would be named, which is nice, and in TypeScript it would allow for each event to emit an object that adheres to an interface, which would also cut down bugs.

On the other hand, it leads to a lot more short lived object initializations, which would be a problem in more traditional languages, but I think Node handles object lifetimes better, so this may be moot.

I would say everything that is part of the physical UX should be a device, i.e:

• plugables (motors, sensors)
• internal motors (boost A & B)
• internal buttons (green, PUPRemote)
• internal tilt & accel
• internal led

I still can't make my mind up on this one. I can perhaps see the argument for things like tilt sensors as they have external counterparts that behave similarly, but I think I lose it when I think of internal LED's and buttons. I think the counterargument are that these things aren't removable, so fundamentally they are part of the hub, and exposing them as a device adds unnecessary complexity for users using this library.

I wonder if they were exposed as separate devices, whether we could compose the objects (perhaps using mixins, or something else) to create a hub object that contains all the hub functions and device functions?

Getters

Based on already implemented methods:

poweredUp.getHubsByType(type: HubType);
> hub[]

poweredUp.getDevices();
hub.getDevices();
> Device[]

poweredUp.getDevicesByType(type: DeviceType);
hub.getDevicesByType(type: DeviceType);
> Device[]

// Use case for the last one: you want to light up everything as the night has come.
// then you make some night mode function to call:
function nightMode() {
  poweredUp.getDevicesByType(DeviceType.LED_LIGHTS).forEach((light) => {
    light.setBrightness(100);
  });
}

Agreed. The byType getters might seem a little over the top, but I can see advantages to them. But I don't think they're a first priority feature.

What about virtual ports with this new paradigm ? I don't know if they are really used from a developer perspective...

I'd argue they're definitely used - especially in the case of trains and remote control cars. Being able to control two motors with a single command is extremely useful as it stops them getting out of sync, even by a single millisecond.

I think new methods would be provided for combining ports as virtual ports, but perhaps not as initial release. I can see the result being a new "attach" event with the combined device and port ("AB").

With new products, it is possible to have unknown yet connected device to a port. I think it should create a Device instance (I assume all device type classes herit from a Device

That's interesting, I hadn't thought about that. I can see the advantage, for example I may not be the first to get my hands on a new device to add support. :)

Currently this library provides a sendRaw function to allow arbitrary commands, so perrhaps this BaseDevice class can provide a send() method and a "receive" event, which is only emitted when the device doesn't know how to handle it (ie. For a BaseDevice, this would be always)

[nathankellenicki]

This is a fair way to do it and I can see where this is coming from, but I will argue that the following would be more convenient in many cases (of course the both could coexist in the API):

  const dev = hub.getMotorAtPort("A");

There seem to be little difference between them, but my assumption is that if I am trying to get a device handle I will typically know what to expect there because this is part of my own build. This is how I see those approaches in actual code:

I'm not sure I see the value here. If you know you're gonna have a motor at that port, then you can call getDeviceAtPort and treat the resulting device as a motor. Otherwise a simple type check would allow you to verify. This seems like it's unnecessary. In addition, if it was implented:

  const motor = hub.getMotorAtPort("A");
  await motor.speed(100); // This would do nothing or throw an error if no motor is connected to port A

I'd argue getMotorAtPort should be the one to throw the error, or return null, if the device isn't a motor. You shouldn't receive a device that isn't a motor - it isn't what you requested.

With my proposal this requires just one extra line:

  const motor = hub.getMotorAtPort("A");
  someEmitter.on("key1", async () => await motor.brake());
  someEmitter.on("key2", async () => await motor.speed(-100));

Note that both in the current state and my proposed implementation the motor can be connected/reconnected at any time before or after the program starts (there may be a runtime exception when a key is pressed and no motor is connected).

With generic getDevice:

  const motor = hub.getDeviceAtPort("A") as BoostMotor; // In TS this type assertion is now neccessary bacause the type check below will not cover the asynchronous event handlers
  if (!(motor instanceof BoostMotor)) throw "Error"; // Or return
  someEmitter.on("key1", async () => await motor.brake());
  someEmitter.on("key2", async () => await motor.speed(-100));

I still don't see the value unfortunately. You state the benefit of this is that if the application knows it expects a motor on the port it won't need to do a type check, however the same is true of getDeviceAtPort:

const motor = hub.getDeviceAtPort("A") as BoostMotor;
someEmitter.on("key1", async () => await motor.brake());
someEmitter.on("key2", async () => await motor.speed(-100));

Either way, without catching any errors, you are making a conscious assertion as the application developer to assume the device at the port is what you expect.

I am also not sure how the generic device idea would cope with changes to port connectivity during execution, for example:

  const motor = hub.getDeviceAtPort("A"); // motor is connected to A
  if (!(motor instanceof Motor)) return;
  await motor.speed(100);
  // Now motor gets disconnected from A
  await motor.speed(0); // What happens here?
  // Now LED Lights are connected to A
  await motor.speed(100); // Will this turn on the lights?
  // Same type of motor is reconnected to A
  await motor.speed(100); // Does this work again?
  // Another type of motor is connected to A
  await motor.speed(100); // How about this?

Well, this is a problem in general with devices, and not restricted to getDeviceAtPort/getMotorAtPort. I'd argue it should be a different device object - it may very well be. We shouldn't try to transfer it between ports. As a developer you should listen to the "detach" event and handle it appropriately.

If you still have a reference to a device object that is no longer attached, it should throw an error when you try to do anything on it (perhaps based on an internal private connected boolean which gets set on detach.

[nathankellenicki]

About reconnections, following example should work in any case:

someEmitter.on("key1", async () => await hub.getMotorAtPort("A").brake());
someEmitter.on("key2", async () => await hub.getMotorAtPort("A").speed(-100));
// or
someEmitter.on("key1", async () => await hub.getDeviceAtPort("A").brake());
someEmitter.on("key2", async () => await hub.getDeviceAtPort("A").speed(-100));

I am also not sure how the generic device idea would cope with changes to port connectivity during execution

As you get the same object from both methods, it should react the same way. getDeviceAtPort, from my understanding, would return a device specific object depending on actual connected device.

If Device objects are valide over multiple reconnections, they may have some "offline" flag to know if the actual device connected to the port is, at least, from the expected type of device. This could allow error handling on calling speed on a disconnected motor for example.

My first thought was a Device instance was created on attach events but "destroyed" on detach (but I don't think it is really possible in JS/TS).

What bother me with the device objects working between reconnections is that the hubs does not. I mean, if you disconnect your hub and reconnect, you have to use the new instance to interract whith it.

I will typically know what to expect there because this is part of my own build

One of my use case is a control app for multiple trains for my daughter and she really does not care about where she plugs motors and lights so the app handle device mapping but I can understand this is not the main usage :)

I agree with this completely (barring previous discussion about getMotorAtPort/getDeviceAtPort). Attempting to maintain the device object between attach/detach events makes several assumptions that may or may not be true.

1. If I detach the motor and plug it back in, to the same port - So logically, this may be exactly the same device, but how do we know?
2. If I detach the motor and plug another motor of exactly the same type, to the same port - Related to the first, but can we assume its the same type? What if its a different firmware revision (Scenario: Lego adds absolute positioning support to the Boost Motor).
3. If I detach the motor and plug it (or another motor of the same type), to a different port - should we make the same assumptions?
4. If we plug in two motors of the same type (eg. to ports A and B), unplug both of them, and plug them both back in to different ports (to C and D), what should we do? Which do we transfer and to where? Does it matter?

I think adding transferring adds needless complexity. I'd argue if you want to handle this properly either you listen to attach and detach events and update your reference to the device appropriately, or you call getDeviceAtPort each time as you suggest.

On detach, you are right, we can't destroy the device object, but we can set an internal property of it (_connected = false) so that if you try to call any method on it, an error is thrown. Then you can update your reference with the device emitted by a new attach.

The developer knows better than this library about the types of devices that should/will be used, and therefore it should be left up to them. Introducing this kind of "magic" I'd argue introduces unnecessary complexity and can't handle all edge cases.

[nathankellenicki]

One of my use case is a control app for multiple trains for my daughter and she really does not care about where she plugs motors and lights so the app handle device mapping but I can understand this is not the main usage :)

Hah! It's a valid use case - of course your daughter shouldn't care, but you, as the developer writing the app for her to use, should implement this! :) Definitely an application level design decision I'd argue.

[nathankellenicki]

I am not convinced this is necessary (how much value this adds for the added complexity), but if we go for it I much prefer the latter approach.

For the top level, I don't think it is very usefull. Still I think emiting event from both device and hub is nice. To limit implementation complexity, I think devices could listen to all relevant events (related to its type) from their hub and emit only those from its own port.

I think the problem here is that this makes the hub responsible for the logic again. Unless the hub listens to the event from the device which listens to the event from the hub which emits the event, which - shudder.

I would say that as all tilt sensor have their own type, buttons from left and right port on the remote would be a RemoteButtonSet device or something like this. This makes me wonder if all motors would share the same object (I think not due to tacho/not tacho).

The left and right sets of buttons on the remote are treated as two seperate devices by Lego, so I think we should do the same. But I think deriving all motors from the same base class is nice, I agree - all motors have the same basic functionality, such as starting/stopping, setting speed, etc.

Another way to put the limit between device and hub feature could be: Everything every hub have in common (green button, led, battery, ...) is a hub feature and the rest are devices. In other words, LWP3 hub properties are features, the rest are port devices.

That's not a bad compromise. I quite like it.

Totally agree. Sometime messages get lost so separate messages could be disapointing. Should it work with a set of two differents types motor ? With different types of device ? I would say yes to both as it is in the LWP3 virtual port setup example but it could make the dual devices classes very complex. I also wonder if it is possible to setup a custom virtual port, I am not sure of my understanding of this part of the LWP3. If so, it could be nice to create it "on the fly" when calling getDualMotorAtPort if it does not already exists.

Yes it is possible to create them on the fly, and yes it can be done with multiple device types. However as Lego says in the docs, its up to the application developer to decide if that pairing makes sense, so I'm inclined to follow the same approach. However I'm not sure how to implement it yet. Perhaps a first pass restricts devices to the same type (as this library currently does).

Yes, I also spotted this inconsistency. Ideally I would add reconnecting to hubs as well. It does not seem to be technically difficult.

This may be doable, but I think its a seperate topics of discussion (perhaps raise an issue to track it?).

const horizonExpress = {
  motor: horizonPupHub.getMotorAtPort(horizonPupHub.findDeviceByType(DeviceType.TRAIN_MOTOR)),
  lights: horizonPupHub.getLightsAtPort(horizonPupHub.findDeviceByType(DeviceType.LED_LIGHTS)),
}

So, there's an assumption being made here that there's only one device of that type attached, which can't be made. However this can be solved by having it return an array, and the application developer can decide to always use the device at index 0 if they so choose.

I think this can be simplified like this which could be useful:

horizonPupHub.getDevicesByType(Consts.DeviceType.TRAIN_MOTOR)[0];
horizonPupHub.getDevicesByType(Consts.DeviceType.LED_LIGHTS)[0];
// PS. Check the length of arrays to make sure there's anything of that device type plugged in at all

[nathankellenicki]

@MrShinyAndNew

1. I have a program that needs to be generic for any connected devices (it's used for diagnostics) so it has a lot of "if this device type, do that" code. I need to be able to "get device at port X" and then do something useful without knowing ahead of time what hubs are connected or what devices are connected.

So, as you likely know, you can currently do this by watching for discover/connect/disconnect events on the library. This is unlikely to change, and in fact with the new paradigm being discussed, you'd watch for attach/detach events of devices.

2. In the same program I have 'moc' scripts which attach behaviour to some of the hubs/devices. These scripts are written with a specific configuration in mind, but for flexibility I typically assume "if there is a motor on this hub, it is the train motor" and "if there is a handset, it is the train handset". This allows me to reconnect the motor to either port after changing batteries.

Currently you would do this by watching for attach events of type motor, and updating a reference to the motor somewhere in your application. However there is some discussion in this thread of a hub.getDevicesByType(DeviceType.TRAIN_MOTOR)[0] type of interface. It really depends on what you're looking for.

3. In combination, the diagnostic control panel and the behavriour script can step on each other's toes because they don't have a way of finding out the state of the system. For example, if the diagnostic control panel sets the motor speed, the train system won't know the motor speed was changed, because there is no place where the speed is stored globally (that I know of). So it would be nice if all commands to modify a device went through a single api that could keep track of the expected state of the device.

I actually want to avoid storing any kind of cached state in the library at all, and leave it up to you, as the developer using the library, to do what you want. My reasoning is that external factors can influence the state in a way that this library may not know about without having knowledge of what the application itself wants to do. For example, with the Duplo train hub, if you stall the train with your hand, protection will kick in and stop the motor. The only way to know this is to a) Watch for the current usage spiking, or b) Watching the seperate speedometer sensor dropping to 0.

I'd argue that if you were developing an app designed to control Duplo trains, then you should be watching those events and update the UI accordingly. Perhaps store your own global variable somewhere with the speed of the motor. Many UI frameworks (such as React and Redux) automatically rerender UI based on the state of the global variable store. See the observer pattern.

[nathankellenicki]

@MrShinyAndNew

I'd like to amend my previous comment, it does appear to work on Node 12 out of the box now, but I'm still concerned about the long-term dependency on the abandoned bluetooth library.

I agree, the state of the Noble library isn't ideal, however it remains the only comprehensive BLE library available for Node.js. Until an alternative comes around this library's usage of it is unlikely to change.

The Abandonware project seems to have done a decent job of taking the Noble library under its wing in keeping it up-to date, as you noticed, by fixing issues and bringing compatibility back to Node.js >8. My hope is that it can be continued. Of course, I'll reevaluate that should that situation ever change.

[nathankellenicki]

In other news, I've started a first pass at this stuff. I had the foresight to pack a small handful of LPF2 devices in the hope I'd have some spare work trip hotel room coding time! :)

[nathankellenicki]

Implemented in #61. 🎉