-
Notifications
You must be signed in to change notification settings - Fork 61
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
Discussion surrounding a new paradigm for talking to attached devices #49
Comments
About 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
}
}); EventsI 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. DevicesAs 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 ?
It excludes mostly the "system" information like battery level, current, voltage, CPU temp. GettersBased 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);
});
}
Virtual portsWhat about virtual ports with this new paradigm ? Unknow deviceWith new products, it is possible to have unknown yet connected device to a port. 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 :) |
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 discoveryWe are all seem to agree on the usefulness of an object representation of LPF devices (both internal and external). 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 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 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(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;
}
} |
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));
As you get the same object from both methods, it should react the same way. 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 My first thought was a Device instance was created on 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.
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 :) |
Commenting @aileo suggestions:
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.
Agreed, I think it is better with structured values.
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).
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. 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
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? |
True, but this has to me serious drawbacks:
Yes, I also spotted this inconsistency. Ideally I would add reconnecting to hubs as well. It does not seem to be technically difficult.
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 |
For the top level, I don't think it is very usefull. Still I think emiting event from both device and hub is nice.
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).
Totally agree. Sometime messages get lost so separate messages could be disapointing.
Agreed, I was thinking about makers and their physical custom devices but now I think it is out of scope.
It would be very nice.
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 |
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:
In unrelated news, it would be great if this library worked on Node > 8 out of the box. |
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. |
Thanks for the reply!
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.
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.
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.
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 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?
Agreed. The
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").
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 |
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
I'd argue
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 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.
Well, this is a problem in general with devices, and not restricted to 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 |
I agree with this completely (barring previous discussion about
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 On detach, you are right, we can't destroy the device object, but we can set an internal property of it ( 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. |
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. |
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.
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.
That's not a bad compromise. I quite like it.
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).
This may be doable, but I think its a seperate topics of discussion (perhaps raise an issue to track it?).
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 |
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.
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
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. |
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. |
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! :) |
Implemented in #61. 🎉 |
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.
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:
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:
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:
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:
Or, you could retrieve the device from the hub:
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:
Which would result in
proximity
events being emitted. Or: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:
Which could result in both events being emitted, or even a new combined event (
proximityAndCount
) with the values of both.And then:
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)
The text was updated successfully, but these errors were encountered: