Dynamic modes

[aileo]

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:

• Add autoParse boolean to PoweredUp which enable parsing from port mode information, disabled by default
• Attach event is emitted when the device is "ready", meaning when it knows how to parse (immediately when auto parsing is disabled and when all modes are ok when auto parsing is enabled
• devices receive method call super.receive when auto parsing
• devices expose events get method to retrieve an array of available events, as it uses the mode names as events
• devices expose setModes method to allow hubs to set modes configuration

Thought:

• All port specific messages (except attach/detach) parsing may be deferred to the device class or at least mode ones.
• I have to admit that it takes a while to get all modes on some sensors
• using mode names for events is not nice, also values are provided as an Array to handle all cases (RGB...)
• It needs some conversion from raw to SI

[aileo]

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.

[nathankellenicki]

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.

[aileo]

I added autoparseWriteDirect (bad naming) which will use output modes definitions to send data.

To test, I removed the TECHNIC_LARGE_LINEAR_MOTOR from basehub and I was able to control one with this:

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 autoparseWriteDirect like this:

public autoparseWriteDirect (mode: string, ...data: number[])
// instead of
public autoparseWriteDirect (mode: string, data: number[])

So it does not need an array of values...

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.

Thank you, I hope it will be up to it.

Perhaps if this makes it in I can modify my dev branch that implements combined sensor modes in.

Cherry on the cake

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 think it is the best compromise, as it could also allow some special conversion (like distance).

Still in my TODO:

• [ ] enable autoparse by default for unknown device
• [ ] some JSON declaration to use the autoparse without port mode information messages

[aileo]

• All port specific messages (except attach/detach) parsing may be deferred to the device class or at least mode ones.

@nathankellenicki , your thoughts on this ?

[nathankellenicki]

Ok so I had some time to test this this afternoon, overall, it seems to work well. Great work :)

• I like that this doesn't override any of the existing methods - I feel those are more user friendly than autoParsing. autoParsing seems like it would mainly be used as a fallback or for debugging a non-implemented device.

• Saying that, I see the potential in the future to move methods such as setPower/rotateByAngle etc away from sending byte arrays, to writing values via mode names (ie. POWER/POS etc), meaning the modes wouldn't need to be hardcoded. To do this though we'd really need to make sure that mode names and values are consistent between all devices - I don't trust Lego. :D

I spotted a couple of bugs and inconsistencies.

1. When using waitForDeviceByType after connect,, and the device is already plugged in at connection time, the promise is resolved, returning a device without any modes. Seems like this should wait for autoParsing to complete too?

2. Some naming conventions I have/will comment on separately.

[nathankellenicki]

• All port specific messages (except attach/detach) parsing may be deferred to the device class or at least mode ones.

@nathankellenicki , your thoughts on this ?

I'm not sure I follow what you mean here - message parsing is already deferred to the device class?

[nathankellenicki]

• All port specific messages (except attach/detach) parsing may be deferred to the device class or at least mode ones.

@nathankellenicki , your thoughts on this ?

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 _parseModeInformationResponse, and potentially moving it to the device class?

Yes, I think I agree with this. I found it somewhat jarring at first that setModes is called by the hub - it seems like something that should be handled by the device. But this becomes more appropriate if mode parsing is deferred to the device class.

[nathankellenicki]

I don't know what can be done about it, but I noticed that the emitted values for POS on Technic motors (and likely others) aren't consistent with the pct/si values they claim. For example:

This is the emitted POS values when setting power to 30:

{ raw: [ 0 ], pct: [ 0 ], si: [ 0 ] }
{ raw: [ 1 ], pct: [ 0.27777777777777146 ], si: [ 1 ] }
{ raw: [ 11 ], pct: [ 3.055555555555543 ], si: [ 10.999999999999943 ] }
{ raw: [ 27 ], pct: [ 7.5 ], si: [ 27 ] }
{ raw: [ 45 ], pct: [ 12.5 ], si: [ 45 ] }
{ raw: [ 62 ], pct: [ 17.22222222222223 ], si: [ 62 ] }
{ raw: [ 82 ], pct: [ 22.777777777777786 ], si: [ 82 ] }
{ raw: [ 99 ], pct: [ 27.499999999999986 ], si: [ 98.99999999999994 ] }
{ raw: [ 120 ], pct: [ 33.333333333333314 ], si: [ 120 ] }
{ raw: [ 141 ], pct: [ 39.16666666666666 ], si: [ 141 ] }
{ raw: [ 161 ], pct: [ 44.72222222222223 ], si: [ 161 ] }
{ raw: [ 181 ], pct: [ 50.27777777777777 ], si: [ 181 ] }
{ raw: [ 200 ], pct: [ 55.55555555555557 ], si: [ 200 ] }
{ raw: [ 219 ], pct: [ 60.83333333333334 ], si: [ 219 ] }
{ raw: [ 239 ], pct: [ 66.38888888888889 ], si: [ 239 ] }
{ raw: [ 257 ], pct: [ 71.38888888888889 ], si: [ 257 ] }
{ raw: [ 276 ], pct: [ 76.66666666666666 ], si: [ 276 ] }
{ raw: [ 295 ], pct: [ 81.94444444444443 ], si: [ 295 ] }
{ raw: [ 315 ], pct: [ 87.5 ], si: [ 315 ] }
{ raw: [ 334 ], pct: [ 92.77777777777777 ], si: [ 334 ] }
{ raw: [ 353 ], pct: [ 98.05555555555557 ], si: [ 353 ] }
{ raw: [ 373 ], pct: [ 100 ], si: [ 360 ] }
{ raw: [ 392 ], pct: [ 100 ], si: [ 360 ] }
{ raw: [ 411 ], pct: [ 100 ], si: [ 360 ] }
{ raw: [ 430 ], pct: [ 100 ], si: [ 360 ] }
{ raw: [ 450 ], pct: [ 100 ], si: [ 360 ] }
{ raw: [ 469 ], pct: [ 100 ], si: [ 360 ] }
{ raw: [ 491 ], pct: [ 100 ], si: [ 360 ] }
{ raw: [ 511 ], pct: [ 100 ], si: [ 360 ] }
{ raw: [ 531 ], pct: [ 100 ], si: [ 360 ] }

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. >.<

[aileo]

• All port specific messages (except attach/detach) parsing may be deferred to the device class or at least mode ones.

@nathankellenicki , your thoughts on this ?

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 _parseModeInformationResponse, and potentially moving it to the device class?

Yes, I think I agree with this. I found it somewhat jarring at first that setModes is called by the hub - it seems like something that should be handled by the device. But this becomes more appropriate if mode parsing is deferred to the device class.

Exactly

[aileo]

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.

Is this case, maybe the code should use modulo instead of clamping... but there is no guarantee it would work for other metrics...

Dammit Lego. >.<

• I don't trust Lego. :D

Consistency does not seem to be their first priority

[aileo]

I think I almost broke everything with some improvments:

• added mode definitions to each specific device class (I dont have duplo and spike ones so I made it up)
• device classes have per mode specific functions to emit related event. If not, it emit the full raw+pct+si event using mode name.
• devices are in charge of messages they are related to (except attach/detach):
  • port information
  • mode information
  • sensor message
  • port action
• modeMap is built from mode definitions

Problems:

• I dont think it is still compatible with the wedo2 but I dont think it is a lot of work to get it back:
  • All sensor parsing is done in Device.receive
  • mode definitions could have an optional flag to specify if mode is compatible with wedo2
  • autoParse may (should?) be disabled on wedo2
• I wanted to export the mode definitions from each device file but the current and voltage sensor mode definitions are related to the hub type (max values). I wonder if this devices should be on autoParse by default like unknown? (Not so much modes and accuracy on future values)
• Some mode definitions are missing, I dont have the related devices:
  • mode 0x01 on DuploTrainBaseColorSensor (I assume it would be count or ambient)
  • modes 0x02, 0x03 and 0x04 on TechnicDistanceSensor

As you can see, the "problems" part is bigger than the other one...

[tthiery]

I am so glad you recognized this concept of pre-caching metadata on code level here. I also implemented it in SharpBrick.PoweredUp. Once you think about Port Value and CombinedMode decoding in a uniform way, the fact that protocol actually has state (combined mode) and needs knowledge (what are the data types, how many, ranges for raw, SI, PCT, etc) the need becomes obvious.

A hint regards hacking all the port information into the code. In my lib I ...

1. trace a port discovery logic against a port id and dump the received messages in the output (independent exe basically)
2. I create a new device class in code (naming it ... mostly stealing good names from here)
3. Add the traced output into this class as a resource string
4. My device initializer, then replays this output against my protocol stack (basically avoiding the length bluetooth based discovery) which then builds up its internal knowledge in the protocol.

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).

[tthiery]

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.

[tthiery]

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.

[aileo]

1. When using waitForDeviceByType after connect,, and the device is already plugged in at connection time, the promise is resolved, returning a device without any modes. Seems like this should wait for autoParsing to complete too?

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.

[nathankellenicki]

@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.

[aileo]

Where did you get most of the raw/pct/si values for the sensors/modes you implemented?

I used the lpf2hubmodeinfo messages for devices I have and guessed for the others

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.

That's why I started #100