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

Feature/devices: parsing sensor #65

Closed
wants to merge 12 commits into from

Conversation

aileo
Copy link
Contributor

@aileo aileo commented Dec 18, 2019

This is not tested, just some code to illustrate this discussion.

I only worked on the color and distance sensor so it breaks other devices. @nathankellenicki , I wanted to know if it is at your taste before going further.

Comment on lines 102 to 107
const data = super.receive(message) || [];

if (this._mode === "SPEC_1") {
this.emit("color", data[0]);
this.emit("distance", data[1]);
this.emit("reflectivity", data[3]);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The purpose of this is to defer the generic parsing to the device class then get back the data to handle only edge cases.

However, this is not the same as the current state of the lib in term of events and data but I think that if we decide to break some things, it is now.

Comment on lines 9 to 95
private static modes: { [name: string]: IDeviceMode } = {
COLOR: {
/**
* Emits when a color sensor is activated.
* @event ColorDistanceSensor#color
* @param {string} port
* @param {Color} color
*/
input: true,
event: "color",
values: { type: ValueType.UInt8, count: 1, min: 0, max: 255 },
num: {
[HubType.BOOST_MOVE_HUB]: 0x00,
[HubType.CONTROL_PLUS_HUB]: 0x00,
[HubType.POWERED_UP_HUB]: 0x00,
[HubType.WEDO2_SMART_HUB]: 0x00
}
},
PROX: {
input: true,
event: "distance",
values: { type: ValueType.UInt8, count: 1, min: 0, max: 10 },
num: {
[HubType.BOOST_MOVE_HUB]: 0x01,
[HubType.CONTROL_PLUS_HUB]: 0x01,
[HubType.POWERED_UP_HUB]: 0x01
}
},
COUNT: {
input: true,
event: "count",
values: { type: ValueType.UInt8, count: 1, min: 0 },
num: {
[HubType.BOOST_MOVE_HUB]: 0x02,
[HubType.CONTROL_PLUS_HUB]: 0x02,
[HubType.POWERED_UP_HUB]: 0x02
}
},
REFLT: {
input: true,
event: "reflectivity",
values: { type: ValueType.UInt8, count: 1, min: 0, max: 100 },
num: {
[HubType.BOOST_MOVE_HUB]: 0x03,
[HubType.CONTROL_PLUS_HUB]: 0x03,
[HubType.POWERED_UP_HUB]: 0x03
}
},
AMBI: {
input: true,
event: "luminosity",
values: { type: ValueType.UInt8, count: 1, min: 0, max: 100 },
num: {
[HubType.BOOST_MOVE_HUB]: 0x04,
[HubType.CONTROL_PLUS_HUB]: 0x04,
[HubType.POWERED_UP_HUB]: 0x04
}
},
SOME_OUTPUT_MODE: {
input: false,
num: {}
},
RGB_I: {
input: true,
event: "rgb",
values: { type: ValueType.UInt8, count: 3, min: 0, max: 255 },
num: {
[HubType.BOOST_MOVE_HUB]: 0x06,
[HubType.CONTROL_PLUS_HUB]: 0x06,
[HubType.POWERED_UP_HUB]: 0x06
}
},
OTHER_OUTPUT_MODE: {
input: false,
num: {}
},
SPEC_1: {
input: true,
event: "colorAndDistance",
values: { type: ValueType.UInt8, count: 4, min: 0, max: 255 },
num: {
[HubType.BOOST_MOVE_HUB]: 0x08,
[HubType.CONTROL_PLUS_HUB]: 0x08,
[HubType.POWERED_UP_HUB]: 0x08
}
}
};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes devices objects very declarative.

It handle compatibility matrix, value parsing description and event relation to mode.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could add here some transformation function that get data array in parameters and return transformed array. It could do some calculation for distance or simply remove some values (color and distance).

@@ -8,26 +8,38 @@ export class Device extends EventEmitter {

public autoSubscribe: boolean = true;

protected _mode: number | undefined;
protected _mode: string | undefined;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I use mode names instead of number because number is not reliable over different hubs.

Comment on lines 32 to 41
this._eventMap = Object.keys(modes).reduce(
(map: {[event: string]: string}, name) => {
const mode = modes[name];
if (mode.num[hub.type] !== undefined && mode.event) {
map[mode.event] = name;
}
return map;
},
{}
);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Create an event to mode name map to subscribe from event


const modeNum = this._modes[modeName].num[this.hub.type];
if (modeNum === undefined) {
// TODO : error handling -> unsupported mode
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Allow to handle hub related unsupported mode error

Comment on lines 126 to 155
switch (mode.values.type) {
case Consts.ValueType.UInt8: {
data.push(message.readUInt8(index));
break;
}
case Consts.ValueType.Int8: {
data.push(message.readInt8(index));
break;
}
case Consts.ValueType.UInt16: {
data.push(message.readUInt16LE(index));
break;
}
case Consts.ValueType.Int16: {
data.push(message.readInt16LE(index));
break;
}
case Consts.ValueType.UInt32: {
data.push(message.readUInt32LE(index));
break;
}
case Consts.ValueType.Int32: {
data.push(message.readInt32LE(index));
break;
}
case Consts.ValueType.Float: {
data.push(message.readFloatLE(index));
break;
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because "We All Love Switches (TM)" :)

Copy link
Owner

@nathankellenicki nathankellenicki Dec 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"This is the way."

Comment on lines 158 to 160
if (mode.event) {
this.emit(mode.event, ...data);
}
Copy link
Contributor Author

@aileo aileo Dec 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

emit related event if any.

This breaks the distance one as it does not apply calculation on the value and also the colorAndDistance which will receive color, distance (0-10), led color and reflectivity instead of only color and distance.

@@ -410,7 +410,8 @@ export class LPF2Hub extends Hub {
const device = this._getDeviceByPortId(portId);

if (device) {
device.receive(message);
// Forward message to device (without size, hub and port)
device.receive(message.slice(3));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Device does not need to get the full message, data is sufficient.
I did not test and I wonder if it should be 4 instead of 3.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, it seems the offset is not the same for WEDO2, it would remove specifique parsing from devices.

Leo Bonnargent added 5 commits December 18, 2019 20:23
if (typeof color === "boolean") {
color = 0;
}
this.send(Buffer.from([0x81, this.portId, 0x11, 0x51, 0x00, color]));
this.sendWithMode("COLOR", Buffer.from([0x81, this.portId, 0x11, 0x51, 0x00, color]));
Copy link
Contributor Author

@aileo aileo Dec 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 0x81, this.portId, 0x11 is common to all devices sent messages. I am not sure to understand that part of the protocol yet but I think it refers to output commands.

Device could have some wrapper method to prepend this part and remove repetitions.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed and good spot - this is known in the LWP3 documentation as WriteDirect mode, so perhaps I'll create a method called writeDirect.

Comment on lines +76 to +79
[HubType.MOVE_HUB]: 0x00,
[HubType.TECHNIC_MEDIUM_HUB]: 0x00,
[HubType.HUB]: 0x00,
[HubType.WEDO2_SMART_HUB]: 0x00
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could maybe define some shorthand like HubType.ALL to lighten the configuration objects when modes are supported by all and with the same number.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do this already in a few places - for example in currentsensor.ts there is a default set for HubType.UNKNOWN. Perhaps this could follow the same model.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be weird but I don't like using UNKNOW as the ALL placeholder, it may lead to misunderstanding while reading the code.

- common device linear power command for motor power and led brightness
- hub led herit from led for brightness
@nathankellenicki
Copy link
Owner

nathankellenicki commented Dec 19, 2019

This is excellent, thank you! I have a few thoughts.

Firstly, it seems each device has an attached firmware and hardware revision. I wonder if future device production runs may have different firmware, perhaps for bug fixes - or even if device firmware can even be updated through the hub. If so, then it's conceivable that modes might change, or at least mode values (ie. min/max).

Perhaps we should do some kind of hardware/firmware version check when first connecting a device, before the device object is attached to the hub? If we have a mode definition for that version combination, we use that, otherwise we use the commands to retrieve the definition?

This would add a small amount of latency before we attach the device, but only in scenarios where we don't already have the definition embedded, and it gives us an incentive to stay up to date.

Secondly, I do like the definition aspect of this, but I think it should be kept separate from the implementation, ie. out of the device classes. Perhaps we should store them as json files to be loaded at runtime from a definitions/ directory?

Regarding timelines for the feature/devices branch - you may have noticed it's been quite fast moving recently as I actively work on this. :) I hope to get an initial version out within the next few days, but I was planning to make a few small changes to devices to handle being able to attach events to the hub and not just the device. I appreciate the speediness of this PR - but perhaps it would be worth it to hold off until feature/devices branch is more stable, ie. on version release? But that's entirely up to you!

});
}

protected sendLinearPowerCommand (value: number) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 I like this - but not sure on the naming as this is DirectWrite mode and isn't just for setting power - ie. it's used for setting LED color/RGB on a hub.

for (let index = 0; index <= message.length; index += Consts.ValueBits[mode.values.type]) {
switch (mode.values.type) {
case Consts.ValueType.UInt8: {
data.push(message.readUInt8(index));
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the raw value be changed to take into account min/max value at this point?

const data = Buffer.from([this.portId, 0x01, 0x02, value]);
this.send(data, Consts.BLECharacteristic.WEDO2_MOTOR_VALUE_WRITE);
} else {
const data = Buffer.from([0x81, this.portId, 0x11, 0x51, 0x00, value]);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There can be multiple values - the LED light takes seperate R G B values, for instance.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not know it was the same header everywhere, I planned to add the mapSpeed logic directly to this method by adding min/max parameters and maybe the 127 thing.

Copy link
Owner

@nathankellenicki nathankellenicki Dec 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, see here for usage when setting hub LED RGB color (I just added writeDirect to my branch based on your other comment): https://github.com/nathankellenicki/node-poweredup/blob/feature/devices/src/devices/hubled.ts#L48

Here is the documentation for this command: https://lego.github.io/lego-ble-wireless-protocol-docs/index.html#writedirectmodedata

@aileo
Copy link
Contributor Author

aileo commented Dec 19, 2019

Firstly, it seems each device has an attached firmware and hardware revision. I wonder if future device production runs may have different firmware, perhaps for bug fixes - or even if device firmware can even be updated through the hub. If so, then it's conceivable that modes might change, or at least mode values (ie. min/max).

I would say it is not first priority, lets see when it happens.
Just like the mode number is defined per hub type, some part of the definition could be per firmware/hardware version.

Perhaps we should do some kind of hardware/firmware version check when first connecting a device, before the device object is attached to the hub? If we have a mode definition for that version combination, we use that, otherwise we use the commands to retrieve the definition?
This would add a small amount of latency before we attach the device, but only in scenarios where we don't already have the definition embedded, and it gives us an incentive to stay up to date.

This looks like a fair compromise.

Secondly, I do like the definition aspect of this, but I think it should be kept separate from the implementation, ie. out of the device classes. Perhaps we should store them as json files to be loaded at runtime from a definitions/ directory?

The definition object is not serializable since I added the transform method that handle special conversions.
In fact, I wonder if it is a weakness in the current model, raw min/max and SI min/max should solve this problem (at least for current/voltage sensor) if it was defined per hub type (like the mode numbers).
I tried the JSON approach at first, but loading JSON in typescript is not in my competencies. Maybe some TS namespaces files in definitions/ directory. I wonder how to handle the "loaded at runtime" for the browser. Maybe just some filtering at "constructor" time.

Regarding timelines for the feature/devices branch - you may have noticed it's been quite fast moving recently as I actively work on this. :) I hope to get an initial version out within the next few days, but I was planning to make a few small changes to devices to handle being able to attach events to the hub and not just the device. I appreciate the speediness of this PR - but perhaps it would be worth it to hold off until feature/devices branch is more stable, ie. on version release? But that's entirely up to you!

Absolutely no problem, I am just better at writing code than explanations :)
This is just pure theory as it does not even compile, I just wanted to see what we could do.
I don't really care about this code being thrown away as it serves as discussion support.

@nathankellenicki
Copy link
Owner

Well, if we go down the JSON file route then each JSON file could be a combination of firmware-hardware-hubtype-devicetype.json, which would allow for making it purely definition based, and allow for version specificity with little effort, just by checking for the appropriate filename.

I tried the JSON approach at first, but loading JSON in typescript is not in my competencies.

I'm sure we can work together on that. :)

I wonder how to handle the "loaded at runtime" for the browser.

Webpack can handle that automatically when bundling. It'll traverse the AST looking for import statements, and bundle the JSON files into the bundle. The json-loader Webpack plugin should handle it for us.

Absolutely no problem, I am just better at writing code than explanations :)
This is just pure theory as it does not even compile, I just wanted to see what we could do.
I don't really care about this code being thrown away as it serves as discussion support.

Don't throw it away! I'd love to get some of this in at some point. Code definitely makes clearer what we're thinking in these contexts. :)

@aileo
Copy link
Contributor Author

aileo commented Dec 20, 2019

[...] it seems each device has an attached firmware and hardware revision. I wonder if future device production runs may have different firmware, perhaps for bug fixes - or even if device firmware can even be updated through the hub [...]

I am not sure, but I have the intuition that this firmware version refer to some hub internal package to handle the device, like dependencies. Why I think this :

  • the file @nutki exposed here which does not mention any hub type related information
  • some videos of people who make poweredup/power function cables and control motors in train mode or linear depending on the wiring and I think power functions devices do not have firmware.

I will make some experiment to verify this.

Comment on lines +80 to +93
export const deviceType: {[typeName: string]: number} = {
UNKNOW: 0,
};
export const deviceTypeNames: {[typeName: number]: string } = {
0: "UNKNOW",
};

for (const type in devices) {
if (devices[+type]) {
const device = devices[+type];
deviceType[device.typeName] = device.type;
deviceTypeNames[device.type] = device.typeName;
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

they are no longer enum and I think it is not very good.

Comment on lines +108 to +115
public get type () {
return this.constructor._type;
}

public get typeName () {
return this.constructor._typeName;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Get type and type name from static attributes instead of consts

Comment on lines +7 to +10
export type DeviceVersion = {
hardware: string,
software: string,
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add hardware/software version to handle modes from "ability" configuration files

Comment on lines +13 to +14
protected static _type: number = 0;
protected static _typeName: string = "UNKNOW";
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add device type and name to static attribute to remove namespaces and consts

Comment on lines -324 to 322
protected _createDevice (deviceType: number, portId: number) {
let constructor;

// NK TODO: This needs to go in a better place
const deviceConstructors: {[type: number]: typeof Device} = {
[Consts.DeviceType.LIGHT]: Light,
[Consts.DeviceType.TRAIN_MOTOR]: TrainMotor,
[Consts.DeviceType.SIMPLE_MEDIUM_LINEAR_MOTOR]: SimpleMediumLinearMotor,
[Consts.DeviceType.MOVE_HUB_MEDIUM_LINEAR_MOTOR]: MoveHubMediumLinearMotor,
[Consts.DeviceType.MOTION_SENSOR]: MotionSensor,
[Consts.DeviceType.TILT_SENSOR]: TiltSensor,
[Consts.DeviceType.MOVE_HUB_TILT_SENSOR]: MoveHubTiltSensor,
[Consts.DeviceType.TECHNIC_MEDIUM_HUB_TILT_SENSOR]: TechnicMediumHubTiltSensor,
[Consts.DeviceType.TECHNIC_MEDIUM_HUB_GYRO_SENSOR]: TechnicMediumHubGyroSensor,
[Consts.DeviceType.TECHNIC_MEDIUM_HUB_ACCELEROMETER]: TechnicMediumHubAccelerometerSensor,
[Consts.DeviceType.MEDIUM_LINEAR_MOTOR]: MediumLinearMotor,
[Consts.DeviceType.TECHNIC_LARGE_LINEAR_MOTOR]: TechnicLargeLinearMotor,
[Consts.DeviceType.TECHNIC_XLARGE_LINEAR_MOTOR]: TechnicXLargeLinearMotor,
[Consts.DeviceType.COLOR_DISTANCE_SENSOR]: ColorDistanceSensor,
[Consts.DeviceType.VOLTAGE_SENSOR]: VoltageSensor,
[Consts.DeviceType.CURRENT_SENSOR]: CurrentSensor,
[Consts.DeviceType.REMOTE_CONTROL_BUTTON]: RemoteControlButton,
[Consts.DeviceType.HUB_LED]: HubLED,
[Consts.DeviceType.DUPLO_TRAIN_BASE_COLOR_SENSOR]: DuploTrainBaseColorSensor,
[Consts.DeviceType.DUPLO_TRAIN_BASE_MOTOR]: DuploTrainBaseMotor,
[Consts.DeviceType.DUPLO_TRAIN_BASE_SPEAKER]: DuploTrainBaseSpeaker,
[Consts.DeviceType.DUPLO_TRAIN_BASE_SPEEDOMETER]: DuploTrainBaseSpeedometer
};

constructor = deviceConstructors[deviceType as Consts.DeviceType];

protected _createDevice (deviceType: number, portId: number, versions: DeviceVersion) {
const constructor = devices[deviceType];

if (constructor) {
return new constructor(this, portId);
return new constructor(this, portId, versions);
} else {
return new Device(this, portId, undefined, deviceType);
return new Device(this, portId, versions, undefined);
}

}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if it's a better place but now it is in the devices.ts collection of devices constructors

Comment on lines -5 to +6
import { BaseHub } from "./hubs/basehub";
import { DuploTrainBase } from "./hubs/duplotrainbase";
import { Hub } from "./hubs/hub";
import { MoveHub } from "./hubs/movehub";
import { RemoteControl } from "./hubs/remotecontrol";
import { TechnicMediumHub } from "./hubs/technicmediumhub";
import { WeDo2SmartHub } from "./hubs/wedo2smarthub";

import { ColorDistanceSensor } from "./devices/colordistancesensor";
import { CurrentSensor } from "./devices/currentsensor";
import { Device } from "./devices/device";
import { DuploTrainBaseColorSensor } from "./devices/duplotrainbasecolorsensor";
import { DuploTrainBaseMotor } from "./devices/duplotrainbasemotor";
import { DuploTrainBaseSpeaker } from "./devices/duplotrainbasespeaker";
import { DuploTrainBaseSpeedometer } from "./devices/duplotrainbasespeedometer";
import { HubLED } from "./devices/hubled";
import { Light } from "./devices/light";
import { MediumLinearMotor } from "./devices/mediumlinearmotor";
import { MotionSensor } from "./devices/motionsensor";
import { MoveHubMediumLinearMotor } from "./devices/movehubmediumlinearmotor";
import { MoveHubTiltSensor } from "./devices/movehubtiltsensor";
import { PiezoBuzzer } from "./devices/piezobuzzer";
import { RemoteControlButton } from "./devices/remotecontrolbutton";
import { SimpleMediumLinearMotor } from "./devices/simplemediumlinearmotor";
import { TechnicLargeLinearMotor } from "./devices/techniclargelinearmotor";
import { TechnicMediumHubAccelerometerSensor } from "./devices/technicmediumhubaccelerometersensor";
import { TechnicMediumHubGyroSensor } from "./devices/technicmediumhubgyrosensor";
import { TechnicMediumHubTiltSensor } from "./devices/technicmediumhubtiltsensor";
import { TechnicXLargeLinearMotor } from "./devices/technicxlargelinearmotor";
import { TiltSensor } from "./devices/tiltsensor";
import { TrainMotor } from "./devices/trainmotor";
import { VoltageSensor } from "./devices/voltagesensor";
import * as devices from "./devices";
import * as hubs from "./hubs";
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

less imports in both index.

Comment on lines +6 to +7
export * from "./devices";
export * from "./hubs";
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

export all hubs and all devices

Comment on lines -194 to +189
switch (hubType) {
case Consts.HubType.WEDO2_SMART_HUB:
hub = new WeDo2SmartHub(device);
break;
case Consts.HubType.MOVE_HUB:
hub = new MoveHub(device);
break;
case Consts.HubType.HUB:
hub = new Hub(device);
break;
case Consts.HubType.REMOTE_CONTROL:
hub = new RemoteControl(device);
break;
case Consts.HubType.DUPLO_TRAIN_BASE:
hub = new DuploTrainBase(device);
break;
case Consts.HubType.TECHNIC_MEDIUM_HUB:
hub = new TechnicMediumHub(device);
break;
default:
return;
const constructor = hubs[type];

if (!constructor) {
return;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apply to hub creation the same process as the devices

Comment on lines +26 to +28
protected static _type: number = 0;
protected static _typeName: string = "UNKNOW";
protected static _portMap: {[portName: string]: number} = {};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add type, type name and port map to static attributes to remove namespace and related consts

@aileo
Copy link
Contributor Author

aileo commented May 27, 2020

am not sure, but I have the intuition that this firmware version refer to some hub internal package to handle the device, like dependencies. Why I think this :

  • the file @nutki exposed here which does not mention any hub type related information
  • some videos of people who make poweredup/power function cables and control motors in train mode or linear depending on the wiring and I think power functions devices do not have firmware.

I will make some experiment to verify this.

A few months later...

I tested mode/device information with a single Technic Large Linear Motor on three kind of hubs, using the web bluetooth example:

hub hardware revision software revision mode 0 mode 1 mode 2 mode 3 mode 4 mode 5
Technic Medium Hub 0.0.00.1000 0.0.00.1000 POWER SPEED POS APOS LOAD CALIB
Move Hub 0.0.00.0004 1.0.00.0000 POWER SPEED POS APOS CALIB STATS
Base Hub 0.0.00.1000 0.0.00.1000 POWER SPEED POS APOS LOAD CALIB

So the hardware revision can change between hubs for the same device...

the mode definition is stricly identical for Technic and Base hub so I think we can rely on version number regardless of the hub type.

I don't know how to structure mode configuration: one file per device ? one per device per versions ?
Should it be json file or typescript ? Typescript is my preferred, easier to write, format/type error on compilation instead of runtime...

If we do not have matching configuration, do we load from mode messages ? If yes, how do we map mode names to events ? If no, do we fall back to the nearest ?

It is a lot of questions, but I think it's worth a try.

@nathankellenicki
Copy link
Owner

nathankellenicki commented Jun 5, 2020

A week or so later...

Sorry I just remembered about this. :) A few months ago I started working on a branch to get sensor combined modes working. I forgot about it until yesterday, so today I started doing more work on it. But it got me thinking.

A lot of it is duplication about defining how many datasets per sensor mode etc, something which as you say may be better defined rather than manifested in code, perhaps through some kind of file you suggest. For this I think JSON would be better.

My worry is I don't really want to have to maintain a per-version list of modes per device, because inevitably it'll become out of date. When that happens, connections would have to become really slow as we start querying every device for their capabilities before you can use them. But I'm not really sure of a great solution for that. Thoughts?

It seems to be that the "main" (ie. non internal or calibration ones) are unlikely to change as they'd need to update many apps, so maybe we don't factor in versioning and just have a single file that we update?

@aileo
Copy link
Contributor Author

aileo commented Jun 5, 2020

My worry is I don't really want to have to maintain a per-version list of modes per device, because inevitably it'll become out of date. When that happens, connections would have to become really slow as we start querying every device for their capabilities before you can use them. But I'm not really sure of a great solution for that. Thoughts?

In #60 I was reading modes from the port information messages, here are pros and cons:

Pros:

  • It works whatever version the hubs/device is running
  • It will simplify code for parsing values and certainly help about mode combination

Cons:

  • Add latency on attach*
  • Maping mode names to events

*About latency, we are talking about startup latency, not running latency so it won't affect the main usage. This does not add complexity to the lib user as the 6.x design is totally event driven so you just wait for the attach (and the attach would be fired only when modes are all good).

It seems to be that the "main" (ie. non internal or calibration ones) are unlikely to change as they'd need to update many apps, so maybe we don't factor in versioning and just have a single file that we update?

I think lego apps are relying on the port information messages so they do not have to maintain consistent version peers. In my test above, the LOAD mode (which is a main sensor mode if you try to do some calibration step like for 42100) is missing on boost hub.

I can think of a compromise, using a flag we could tell the lib tu use either the internal definition or load them from port information messages:

const PoweredUP = require("node-poweredup");
const poweredUP = new PoweredUP.PoweredUP({ dynamicModes: true });

Not sure about the naming...

Some devices could just ignore this flag because of their simplicity: current, voltage, hub led, ...

@aileo aileo mentioned this pull request Jun 22, 2020
@aileo
Copy link
Contributor Author

aileo commented Jun 26, 2020

going further in #97

@aileo aileo closed this Jun 26, 2020
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