New paradigm for handling attached devices #61

nathankellenicki · 2019-12-07T01:36:42Z

This is a work in progress PR to implement some of the changes discussed in #49.

Currently, large swathes of code are commented out. The approach I'm taking for this is a bottom up approach - starting with minimal functionality and adding upon it, until feature parity is nearly reached.

Currently implemented:

Attach/detach events
BasicMotor setSpeed and brake (to demonstrate devices sending data to the hub)
Basic hierarchy of Device/BasicMotor/ControlPlusLargeMotor etc.

In addition, this starts to implement an auto-subscribe mechanism (originally suggested by @nutki in #60). Currently it is a Big Ugly Switch Statement(TM), however my hope is that this can be reduced and automated with some of the logic implemented by @aileo in #60.

I've added a temporary example at examples/new_device_test.js to demonstrate how the new paradigm would work.

aileo · 2019-12-07T12:23:52Z

src/colordistancesensor.ts

+        this.on("newListener", (event) => {
+            switch (event) {
+                case "color":
+                    this.subscribe(0x00);


I wonder if we should avoid using hex notation as all const are in decimal and I see modes as almost const.

In this case I don't mind either way. But for Consts.DeviceType I would actually prefer to use hex literals as this is how they are listed in LWP doc.

It's a good question. I think for the moment having the values hardcoded is a placeholder, I'd like to put all consts in a file somewhere. So - hex or decimal? Perhaps hex as @nutki suggests?

Yes, like @nutki wrote hex make more sense regarding the doc. Even more if we had some const with all message headers and all.

Plus, the end users can use whatever format they want when they call methods.

@aileo I've started getting rid of the hex notation (and decimals too) and implementing modes as enums. And as suggested by @nutki somewhere else, subscriptions are automatically made when attaching the associated event handler. It's much cleaner, thanks!

nutki · 2019-12-07T14:22:56Z

src/basicmotor.ts

+     * @param {number} speed For forward, a value between 1 - 100 should be set. For reverse, a value between -1 to -100. Stop is 0.
+     * @returns {Promise} Resolved upon successful completion of command.
+     */
+    public setSpeed (speed: number) {


Since basic motors don't have a tachometer, this is actually setPower. I guess this makes it easier for the API, but would you then have setSpeed and setPower for tacho motors or just setSpeed?

Good point. I guess I haven't liked power as I think for most users speed is a more appropriate term for what they want, even if it isn't exactly the same. I don't this library should attempt to exactly emulate LWP functionally, instead adding a user friendly layer on top of it.

Perhaps we could have setSpeed(50) set power by default, however with an options object (such as setSpeed(50, { maxPower: 50 }) to switch to maintaining the speed with power?

@nutki I've gone with setPower and done a 180 on this. :) Will implement setSpeed appropriately later.

nutki · 2019-12-07T14:59:42Z

src/device.ts

+        this._portId = portId;
+        this._type = type;
+        this.hub.on("detach", (device) => {
+            if (device.portId === this.portId) {


Wouldn't device === this be better? This way old disconnected devices would not generate detach events.

Good catch. Will change this.

Maybe a more straightforward approach would be to remove this listener on detach.

@nutki Listener is now removed on detach.

nutki · 2019-12-07T15:05:53Z

src/device.ts

+    }
+
+    public send (data: Buffer, characteristic: string = Consts.BLECharacteristic.LPF2_ALL, callback?: () => void) {
+        this.hub.send(data, characteristic, callback);


Doesn't this send LPF2_ALL characteristic messages to WeDo2 hub? As a side note, I am not sure if continued support of the WeDo hub makes sense. The new devices will not be supported on it anyway since the FW is not updatable (according to a Lego designer) and it has limited availability (which also makes it expensive).

Right now, yes, eventually I want to have each device method (ie. setSpeed) switch characteristics and message based on hub type.

Regarding the WeDo 2.0 support - I've also been thinking about this recently, but I think I want to keep support personally, as I actually use a few WeDo 2.0 hubs myself. :) They are currently the only hub type that offers an AC adapter so it can run forever.

On the topic of firmware support, that's correct - however most newer devices can be supported on it in some way. See the compatibility matrix in the readme of this project.

Eventually I might drop it though - especially if Lego releases a hub with an AC adapter.

Yeah, I understand that WeDo support is mostly for people who already own one. Iteresting about the AC adapter though, I have not heard about it. Is this stand alone or the one with rechargeable battery allows to bypass the battery? Long time ago I especially for that purpose bought Lego RCX with an AC plug, even though I already had 2 without :)

Yeah, I understand that WeDo support is mostly for people who already own one. Iteresting about the AC adapter though, I have not heard about it. Is this stand alone or the one with rechargeable battery allows to bypass the battery? Long time ago I especially for that purpose bought Lego RCX with an AC plug, even though I already had 2 without :)

It's the rechargeable battery pack for the WeDo 2.0 hub - it has a built in port to plug in an AC adapter. :)

https://education.lego.com/en-us/products/wedo-2-0-add-on-power-pack-by-lego-education/5004838

That will allow you to keep a WeDo 2.0 hub running forever.

I've implemented some WeDo2 support into some devices - it's actually on par with master feature wise, but I may implement some more modes, once I test to see what works and doesn't!

It's much simpler to implement WeDo2 support with each motor/sensor having its own device class.

nutki · 2019-12-07T15:58:55Z

src/hub.ts


-    protected _ports: {[port: string]: Port} = {};
-    protected _virtualPorts: {[port: string]: Port} = {};
+    protected _attachedDevices: Device[] = [];


Given the current use I would say a map of portId to Device would make more sense than a flat array for _attachedDevices.

I think so too. But object literals don't accept numbers as key type, and I was hoping not to use ES6 maps. I guess we could though ("Just shut up and do it, Nathan!")

I'm totally wrong - they do support numbers as keys/property names, just not in initializers. I'll change to use this. :)

Do you mean initializes as in: ({0:'Test'})[0]? It does work for me. What's wrong with ES6 Maps, btw? I strongly prefer them to using POJOs for maps, especially in Typescript.

aileo · 2019-12-09T08:15:00Z

src/pupremote.ts

-        this.type = Consts.HubType.POWERED_UP_REMOTE;
-        this._ports = {
-            "LEFT": new Port("LEFT", 0),
-            "RIGHT": new Port("RIGHT", 1)
+        this._type = Consts.HubType.POWERED_UP_REMOTE;
+        this._portNames = {
+            "LEFT": 0,
+            "RIGHT": 1


This looks repetitive over hubs, does it make sense to use the same approach you have on devices ?

super(device, autosubscribe, Consts.HubType.POWERED_UP_REMOTE, { "LEFT": 0, "RIGHT": 1 });

Yeah, perhaps so. To be honest I’ve barely touched the hub setup, I’ve been focused on the device abstraction. But they will get a revamp too.

This is also now done - #61 (comment)

aileo · 2019-12-09T08:19:03Z

examples/winter_village_train.js

+        sensor = hub;
+
+        sensor.on("distance", (_, distance) => {
+            if (distance < 5 && !ramping) {
+                await stopTrain();
+            }
+        });


This listen a device (sensor) event on the hub, does it mean we support device events at the hub level ?

If it does not call the same method on the device, it does not subscribe to the correct mode.

Oops, I shouldn’t have committed that! :) It’s for my house - designed to run on the old code (master). But I haven’t though more about supporting that as of now.

It makes a nice example !

the new waitForDeviceByType should allow you to migrate this to the new paradigm

Yeah - it was an idea that only occurred to me last night while thinking about other stuff, but it solves a problem that many people have, ie. not knowing when to call getDeviceByType without watching for an attach event, by which point it's provided to you anyway. It just makes it easier to maintain a more sequential coding pattern, which is easier for a lot of people.

It's also how I was thinking this could support internal motors/functions on a hub rather on device. ie. If a boost internal motor has a method setSpeed(speed), I can create a lightweight wrapper on the hub called setSpeed(port, speed), which simply passes through the values to the method on the device after using waitForDeviceByPort.

aileo · 2019-12-12T14:05:10Z

src/boostmovehub.ts

+        this._portNames = {
+            "A": 0,
+            "B": 1,
+            "C": 2,
+            "D": 3,
+            "TILT": 58


I think port names/id should appear in consts as they are constant and the same over same type hubs. It would also help developers like @ractive and me determine which port we should look at in our projects.

this comes from here and here

I've now created a const (exported under the hub namespace) for each hub, accessible via PoweredUP.WeDo2SmartHub.PortMap, etc. This is also passed to the root hub object at constructor time. It's also available at runtime via a ports getter, ie. hub.ports = ["A", "B", ...].

…weredup into feature/devices

aileo · 2020-01-12T09:14:41Z

src/utils.ts

+    angle = normalizeAngle(angle);
+    if (angle < -135) {
+        return -180;
+    }
+    if (angle < -45) {
+        return -90;
+    }
+    if (angle < 45) {
+        return 0;
+    }
+    if (angle < 135) {
+        return 90;
+    }
+    return -180;


I would go for a more mathematical approach :

export const roundAngleToNearest90 = (angle: number) => { return normalizeAngle(Math.round(angle/90)*90); }

nathankellenicki · 2020-01-13T19:04:42Z

I think at this point this PR has reached feature parity with what was there before. I'd love some help testing it before I end up approving it and merging it to master and pushing out a new release.

I'm hoping to do this soon - especially as my copy of Spike Prime is scheduled to be delivered today, so I'll have some new devices to add support for shortly! :)

… bluetooth example

…pike Prime)

…(Boost Motor)

aileo · 2020-01-30T07:31:24Z

I'd love some help testing it before I end up approving it and merging it to master and pushing out a new release.

Do you have any testing plan ? I have no idea where to start.

Aslo, I can only test the web-bluetooth part with my current setup.

nathankellenicki · 2020-02-06T17:58:28Z

I'd love some help testing it before I end up approving it and merging it to master and pushing out a new release.

Do you have any testing plan ? I have no idea where to start.

Aslo, I can only test the web-bluetooth part with my current setup.

My testing plan up until now has been quite simple - for every hub, try every sensor in every mode, and every motors output. :D It's not especially quick.

Thus far I'm happy with this PR, though more testing (both similar to the plan and real-life application testing) I'm always thankful for!

I think the main things I need to do now are update the documentation and JSDoc comments, and update the samples.

nathankellenicki added 2 commits December 6, 2019 11:31

Basic implementation of BasicMotor types

d453fe5

Added autosubscribe when adding eventlisteners

4af1d3d

nathankellenicki added WIP Do Not Merge labels Dec 7, 2019

aileo reviewed Dec 7, 2019

View reviewed changes

nutki reviewed Dec 7, 2019

View reviewed changes

nathankellenicki added 5 commits December 7, 2019 13:19

attachedDevices now a map

8ffc60b

Basic code for passing messages through to the device

93a1369

Basic ColorDistanceSensor and TachoMotor handling

d382c3d

Implemented basic lights, device busy-ness

a02249f

Yay, port object is no longer a thing

f9cea9a

aileo reviewed Dec 9, 2019

View reviewed changes

nathankellenicki added 4 commits December 10, 2019 07:23

Start of hub refactor

7fdc754

More refactoring

5c03e5c

Removing commented code

1793eb0

Implemented methods for retrieving hubs and devices

6beeda5

aileo reviewed Dec 12, 2019

View reviewed changes

nathankellenicki and others added 9 commits December 15, 2019 14:07

Added more motor types

6a7c489

Renamed devices

198d637

Merge branch 'feature/devices' of github.com:nathankellenicki/node-po…

81cbdac

…weredup into feature/devices

Motion and tilt sensor

31ac932

Removed some commented code from hubs

2a67242

Better handling of device initialisation, implemented more modes

023b141

Rename

37be7f8

Rename again

406e9e9

Better directory structure

225f331

nathankellenicki and others added 9 commits January 8, 2020 14:38

Start of port combining

673f5ff

Merge branch 'feature/devices' of github.com:nathankellenicki/node-po…

afa7c74

…weredup into feature/devices

Fixed linting errors and removed namespaces

875293a

Virtual port attachment and detachment

bf721ed

Virtual ports tested

b43b59e

Added setSpeed, virtual port multiparam options

f7e86d3

Initial absolute motor support

74be8a7

Fixed parts of absolute angle

56eaa7c

Servo calibration and reset

63f1c10

aileo reviewed Jan 12, 2020

View reviewed changes

Adding power and brightness ramping back in

5fde49c

nathankellenicki added 9 commits January 13, 2020 12:17

Can not retrieve human readable device type from devices. Updated web…

ea20b1b

… bluetooth example

Can not retrieve human readable device type from devices. Updated web…

dfd22b1

… bluetooth example

Devices now cache notified values for easy retrieval without events

0c3ff8b

Devices now cache notified values for easy retrieval without events

225b60f

Added support for Spike Prime motors

d329ce6

Added support for Technic Color Sensor and Technic Distance Sensor (S…

ef31567

…pike Prime)

Added Technic Force Sensor (Spike Prime)

407acce

Added example of slow speed train control with a Medium Linear Motor …

c6f8015

…(Boost Motor)

Removed examples with old API

e34eea9

nathankellenicki and others added 3 commits February 7, 2020 14:28

Updated vernie sample, added braking style

c0009f8

Updated docs, readme, version

e721e0f

Added announement to readme

a34c422

nathankellenicki merged commit a34c422 into master Feb 10, 2020

nathankellenicki removed Do Not Merge WIP labels Feb 10, 2020

nathankellenicki mentioned this pull request Feb 10, 2020

Discussion surrounding a new paradigm for talking to attached devices #49

Closed

New paradigm for handling attached devices #61

New paradigm for handling attached devices #61

Conversation

nathankellenicki commented Dec 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nathankellenicki Dec 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aileo Dec 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nathankellenicki Dec 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aileo Dec 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nathankellenicki commented Jan 13, 2020

aileo commented Jan 30, 2020

nathankellenicki commented Feb 6, 2020

nathankellenicki commented Dec 7, 2019 •

edited

Loading

nathankellenicki Dec 17, 2019 •

edited

Loading

aileo Dec 9, 2019 •

edited

Loading

nathankellenicki Dec 18, 2019 •

edited

Loading

aileo Dec 12, 2019 •

edited

Loading