-
Notifications
You must be signed in to change notification settings - Fork 59
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
Dynamic modes #97
base: master
Are you sure you want to change the base?
Dynamic modes #97
Conversation
Just made a small test: removing ColorDistanceSensor class and using autoparse to control the device, it works. I think the same kind of code could be done for output modes and auto parsing could be enabled by default for unknown devices. |
Wow nice, quite a lot to unpack here. :) I'll have to give it a good test at the weekend at some point, but its definitely promising. Perhaps if this makes it in I can modify my dev branch that implements combined sensor modes in. Regarding the mode names - I agree, they're not suited to event handlers. Perhaps a mapping of mode names to events could be implemented so that we could keep the existing event names. |
I added To test, I removed the hub.on("attach", (device) => {
const outputs = device.writeModes
if (outputs.includes('SPEED')) {
device.autoparseWriteDirect('SPEED', [50])
}
}); Now that I pushed, I notice that maybe I should define public autoparseWriteDirect (mode: string, ...data: number[])
// instead of
public autoparseWriteDirect (mode: string, data: number[]) So it does not need an array of values...
Thank you, I hope it will be up to it.
Cherry on the cake
I think it is the best compromise, as it could also allow some special conversion (like distance). Still in my TODO:
|
@nathankellenicki , your thoughts on this ? |
@@ -15,6 +15,10 @@ export class DuploTrainBaseSpeedometer extends Device { | |||
} | |||
|
|||
public receive (message: Buffer) { | |||
if (this.hub.autoParse) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are quite repetitive, and error prone when new devices are added (ie. could forget to do this). Could the check be done at hub level, calling either receive or autoParse?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am thinking about another way to do this:
- Define all known modes like auto parsed ones and add them in the constructor.
- always use
Device.receive
to parse incoming messages (including mode information) and emit lego named events - make the specific device classes to listen on their lego events to emit proper ones
No more receive
method in specific device classes, just event handlers that could use private methods to handle mode calculation and formating.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I totally forgot about the subscription logic in the comment above. It will need the correct mode map.
Ok so I had some time to test this this afternoon, overall, it seems to work well. Great work :)
I spotted a couple of bugs and inconsistencies.
|
I'm not sure I follow what you mean here - message parsing is already deferred to the device class? |
Actually I think I understand now, are you referring to Yes, I think I agree with this. I found it somewhat jarring at first that |
return; | ||
} | ||
const { name, raw, pct, si, values } = this._modes[mode]; | ||
const valueSize = Consts.ValueTypeSize[values.type]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't checked on all devices, but do all device modes only accept one type of value? Even for multiple values?
Are there any instances where a device could have ie. 3x int32, 1x uint8? I don't know if the UART protocol even supports that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say they mix everything as in the boost color and distance sensor:
[
{
"name": "COLOR",
"input": true,
"output": false,
"raw": { "min": 0, "max": 10 },
"pct": { "min": 0, "max": 100 },
"si": { "min": 0, "max": 10, "symbol": "IDX" },
"values": {
"count": 1,
"type": 0
}
},
{
"name": "PROX",
"input": true,
"output": false,
"raw": { "min": 0, "max": 10 },
"pct": { "min": 0, "max": 100 },
"si": { "min": 0, "max": 10, "symbol": "DIS" },
"values": {
"count": 1,
"type": 0
}
},
"[...]",
{
"name": "SPEC 1",
"input": true,
"output": false,
"raw": { "min": 0, "max": 255 },
"pct": { "min": 0, "max": 100 },
"si": { "min": 0, "max": 255, "symbol": "N/A" },
"values": {
"count": 4,
"type": 0
}
},
"[...]"
]
For SPEC 1, which includes values from COLOR and PROX modes, it use 0-255 as range instead of 0-10 to match other values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As an example, the tilt sensor of the TechnicMediumHub has different data types for different modes in use (Int8, Int32, Int16) but within one mode, the data type is always the same. At least, that is what the device/spec is self-declaring.
Port: 99
IOTypeId: TechnicMediumHubTiltSensor
HardwareRevision: 0.0.0.1
SoftwareRevision: 0.0.0.1
OutputCapability: True
InputCapability: True
LogicalCombinableCapability: True
LogicalSynchronizableCapability: True
ModeCombinations: []
UsedCombinationIndex: 0
MultiUpdateEnabled: False
ConfiguredModeDataSetIndex: []
Mode: 0
Name: POS
IsInput: True
IsOutput: False
RawMin: -180
RawMax: 180
PctMin: -100
PctMax: 100
SIMin: -180
SIMax: 180
Symbol: DEG
InputSupportsNull: False
InputSupportFunctionalMapping20: True
InputAbsolute: True
InputRelative: False
InputDiscrete: False
OutputSupportsNull: False
OutputSupportFunctionalMapping20: False
OutputAbsolute: False
OutputRelative: False
OutputDiscrete: False
NumberOfDatasets: 3
DatasetType: Int16
TotalFigures: 3
Decimals: 0
DeltaInterval: 0
NotificationEnabled: False
Mode: 1
Name: IMP
IsInput: True
IsOutput: False
RawMin: 0
RawMax: 100
PctMin: 0
PctMax: 100
SIMin: 0
SIMax: 100
Symbol: CNT
InputSupportsNull: False
InputSupportFunctionalMapping20: False
InputAbsolute: False
InputRelative: True
InputDiscrete: False
OutputSupportsNull: False
OutputSupportFunctionalMapping20: False
OutputAbsolute: False
OutputRelative: False
OutputDiscrete: False
NumberOfDatasets: 1
DatasetType: Int32
TotalFigures: 3
Decimals: 0
DeltaInterval: 0
NotificationEnabled: False
Mode: 2
Name: CFG
IsInput: False
IsOutput: True
RawMin: 0
RawMax: 255
PctMin: 0
PctMax: 100
SIMin: 0
SIMax: 255
Symbol:
InputSupportsNull: False
InputSupportFunctionalMapping20: False
InputAbsolute: False
InputRelative: False
InputDiscrete: False
OutputSupportsNull: False
OutputSupportFunctionalMapping20: False
OutputAbsolute: True
OutputRelative: False
OutputDiscrete: False
NumberOfDatasets: 2
DatasetType: SByte
TotalFigures: 3
Decimals: 0
DeltaInterval: 0
NotificationEnabled: False
I don't know what can be done about it, but I noticed that the emitted values for This is the emitted
As you can see, they claim the max value is 360, and that 0-100% = 0-360, however I believe the values can actually go -2147483646-2147483647 due to the Int32. Dammit Lego. >.< |
Exactly |
Is this case, maybe the code should use modulo instead of clamping... but there is no guarantee it would work for other metrics...
Consistency does not seem to be their first priority |
- also added all known modes definition
I think I almost broke everything with some improvments:
Problems:
As you can see, the "problems" part is bigger than the other one... |
export const modes = TachoMotorModes.concat([ | ||
// POWER | ||
// SPEED/speed | ||
// POS/rotate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It add mode to the existing ones in TachoMotor
* @param {number} absolute | ||
*/ | ||
this.notify("absolute", { angle }); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add handle for rotate
mode. Using raw instead of SI because we know it does not convert very well on angles
si: { min: -100, max: 100, symbol: "PCT" }, | ||
values: { count: 1, type: Consts.ValueType.Int8 } | ||
} | ||
]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mode definitions are exported to be used by TachoMotor
protected _mode: number | undefined; | ||
protected _modeMap: {[event: string]: number} = {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_modeMap
is now protected so it can be used in device class to subscribe to mode
this._isWeDo2SmartHub = (this.hub.type === Consts.HubType.WEDO2_SMART_HUB); | ||
this._isVirtualPort = this.hub.isPortVirtual(portId); | ||
|
||
if (!this.autoParse) { | ||
this._init(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_init
is call once device have all its mode definitions
this._modeMap = this._modes.reduce((map: {[name: string]: number}, mode, index) => { | ||
map[mode.name] = index; | ||
return map; | ||
}, {}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
generate modeMap
from mode definitions
|
||
if (mode === this._modeCount - 1) { | ||
this._init(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Call init
when we are done with mode information messages (and autoParse
enabled)
pct: { min: 0, max: 100 }, | ||
si: { min: 0, max: 5120, symbol: "mm" }, | ||
values: { count: 1, type: Consts.ValueType.Int16 } | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mode definition handle the * 10
in SI max.
constructor (hub: IDeviceInterface, portId: number, modeMap: {[event: string]: number} = {}, type: Consts.DeviceType = Consts.DeviceType.TECHNIC_MEDIUM_ANGULAR_MOTOR) { | ||
super(hub, portId, {}, type); | ||
constructor (hub: IDeviceInterface, portId: number) { | ||
super(hub, portId, [], Consts.DeviceType.TECHNIC_MEDIUM_ANGULAR_MOTOR); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other similar motor are defined this way so I though it would be better as it is shorter
pct: {min: -100, max: 100}, | ||
si: {min: -2000, max: 2000, symbol: "DPS"}, | ||
values: {count: 3, type: Consts.ValueType.Int16} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont understand how max of Int16 can be a decimal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the system communicates int16. rawMin and rawMax are not the boundaries of this values but the scaling baseline on the SI/pct min/max. That is the same as for the motors. their pos is a fully utilized int32 but rawMin/rawMax are -360 to 360.
just do not think of them as min and max of the related communicated raw value. But as an input for the formula to calculate SI and PCT.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, then I will remove the "clamping" logic of the normalize
function.
|
||
if (device) { | ||
device.receive(message); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
send all device messages to device
I am so glad you recognized this concept of pre-caching metadata on code level here. I also implemented it in A hint regards hacking all the port information into the code. In my lib I ...
While this may not work for devices not adhering to the protocol, it works quite nicely for the Technic ones I have ;). I think an adopted approach by dumping json from this tool instead of byte arrays is maybe the right thing here. Using an external tool to capture the self-description messages can also help when you do not personally own a device (e.g. duplo train). |
Regards the POS (if you have not clarified this yet) of the motor ... POS is the amount of accumulated/moved degrees relative to initial value/position ... so it is not 360 to -360. It is the amount of degrees the motor has turned. However, check this scaling code (and assume rawMin = min and rawMax = max) => the scaling result will be the same as the input value. So when Lego specs for "POS" Raw: -360 - 360 and SI -360 - 360 ... the effectively invalidate the scaling and any given raw value (e.g. int of any size) will just be passed through. internal static float Scale(float value, float rawMin, float rawMax, float min, float max)
{
var positionInRawRange = (value - rawMin) / (rawMax - rawMin);
return (positionInRawRange * (max - min)) + min;
} As stated above, do not think rawMin and rawMax as the boundary of the communicated values. Think of them as the baseline for the si/pct related scaling. |
An architecture decision I made, was to put the Protocol in an independent object. This object is not only doing the byte[] <-> message object conversion but also holds the complete relevant knowledge and state (independent whether there is a device connected on the other side) which the decoder need. Like that I can have fun on top (doing e.g. a hub/device-like programming model or just doing some discovery logic without a hub like concept) and work with already decoded data. I see here a splitting of the decoding between hub and base type of device. I do not use hub or device base for that but have decided to host it in the protocol layer below. You have a much more mature platform and have to deal with many more devices. So take this with a grain of salt. |
I finally found why: it iterate over connected devices and returns the first one matching type regardless of its "ready" state. I will fix that. |
super.receive(message); | ||
break; | ||
} | ||
this._eventHandlers.rotate = (data: IEventData) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo - this should be this._eventHandlers.absolute
raw: { min: 0, max: 512 }, | ||
pct: { min: 0, max: 100 }, | ||
si: { min: 0, max: 5120, symbol: "mm" }, | ||
values: { count: 1, type: Consts.ValueType.Int16 } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be an Int8, also the raw/si values are incorrect for this sensor:
lpf2hubmodeinfo Port 00, mode 0, name LPF2-DETECT +30ms
lpf2hubmodeinfo Port 00, mode 0, RAW min 0, max 10 +30ms
lpf2hubmodeinfo Port 00, mode 0, PCT min 0, max 100 +30ms
lpf2hubmodeinfo Port 00, mode 0, SI min 0, max 10 +46ms
lpf2hubmodeinfo Port 00, mode 0, SI symbol +30ms
lpf2hubmodeinfo Port 00, mode 0, Value 1 x 0, Decimal format
@aileo I spent a bit of time testing out the latest this evening, and for the most part, it works great. :) I made some smaller comments on a couple of typos and corrections that need made. You might also be pleased to hear that WeDo 2.0 still works great. :) I tested the SPIKE Prime Large Angular Motor, the Boost Motor, and the WeDo 2.0 Distance and Tilt sensors, and they all work great with the WeDo 2.0 Smart hub as well as the Boost Move Hub, Powered UP Hub, and the Technic Control+ hub. Where did you get most of the raw/pct/si values for the sensors/modes you implemented? When I get more time this week I plan on doing a more thorough test of all current sensor types with all hubs and all currently implemented modes (may take a wee while!), so I was thinking of double checking the mode definitions against those in this PR. |
I used the
That's why I started #100 |
This should replace #60 and #65 as they are both out of date.
It is just another attempt and is not meant to be merged as is.
Modifications:
autoParse
boolean to PoweredUp which enable parsing from port mode information, disabled by defaultreceive
method callsuper.receive
when auto parsingevents
get method to retrieve an array of available events, as it uses the mode names as eventssetModes
method to allow hubs to set modes configurationThought: