Dynamic modes #97
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 ? |
| } | ||
| | ||
| public receive (message: Buffer) { | ||
| if (this.hub.autoParse) { |
There was a problem hiding this comment.
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.
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.receiveto 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.
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.
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.
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.
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.
It add mode to the existing ones in TachoMotor
| * @param {number} absolute | ||
| */ | ||
| this.notify("absolute", { angle }); | ||
| }; |
There was a problem hiding this comment.
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.
mode definitions are exported to be used by TachoMotor
| protected _modeCount: number = 0; | ||
| protected _modes: IMode[] = []; | ||
| protected _mode: number | undefined; | ||
| protected _modeMap: {[event: string]: number} = {}; |
There was a problem hiding this comment.
_modeMap is now protected so it can be used in device class to subscribe to mode
| | ||
| if (!this.autoParse) { | ||
| this._init(); | ||
| } |
There was a problem hiding this comment.
_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.
generate modeMap from mode definitions
| | ||
| if (mode === this._modeCount - 1) { | ||
| this._init(); | ||
| } |
There was a problem hiding this comment.
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.
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.
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.
I dont understand how max of Int16 can be a decimal.
There was a problem hiding this comment.
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.
Oh, then I will remove the "clamping" logic of the normalize function.
| | ||
| if (device) { | ||
| device.receive(message); | ||
| } |
There was a problem hiding this comment.
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.
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.
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 |
3 participants
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:
autoParseboolean to PoweredUp which enable parsing from port mode information, disabled by defaultreceivemethod callsuper.receivewhen auto parsingeventsget method to retrieve an array of available events, as it uses the mode names as eventssetModesmethod to allow hubs to set modes configurationThought: