Experiment: read sensor data with mode informations #60

aileo · 2019-12-04T20:17:19Z

Once again this is not meant to be merged as it breaks everything.

Since the LWP3 exposes mode information with name and data format for parsing, I tried to implement some kind of "auto parsing":

it parse data using mode information
emit a sensor event with mode name and raw parsed data array (which allow to listen to events not handled by the lib)
emit almost the same events the library already have

BUT: It does not work very well as I don't know how to get the mode information before the attach event is emited. As is, it just ignore sensor events until the mode information got collected...

If anyone see value in this and wants to help, I would be very happy to know if there is a way to make it reliable.

aileo · 2019-12-04T20:19:15Z

src/hub.ts

-                this._activatePortDevice(port.value, type, this._getModeForDeviceType(type), 0x00);
+                this.subscribe(port.id, this._getModeForDeviceType(type));


This changed only to have the exact same result using manual subscribe or autosubscribe.

aileo · 2019-12-04T20:19:57Z

src/lpf2hub.ts

-            this._sendPortInformationRequest(data[3]);
        }
+        this._sendPortInformationRequest(data[3]);


Ask for port information even if no debug flag to get parsing format

I specifically removed it to avoid unnecessary radio static, of course since you actually use this information it cannot be avoided (though it could be trimmed to the messages you use - name and data type).

src/lpf2hub.ts

nathankellenicki · 2019-12-05T19:43:38Z

I love this - it's something I'd hoped to try experimenting with but again you guys beat me to it. :)

One thing I notice though is that the ginormous switch statement with emits looks very repetitive. One solution might be to have a mapping of modes to human-friendly emit types, and use the ES6 spread operator to spread out the values array into the different emit arguments, based on the reported length by the mode information? Do you think that's feasible?

aileo · 2019-12-06T07:35:29Z

One thing I notice though is that the ginormous switch statement with emits looks very repetitive. One solution might be to have a mapping of modes to human-friendly emit types, and use the ES6 spread operator to spread out the values array into the different emit arguments, based on the reported length by the mode information? Do you think that's feasible?

I tried to do this first but there is some special cases where it does not work very well :

POS mode exists for both motors and control+ gyro but it is obviously not the same event
SPEC_1 mode of boost tilt sensor emits multiple events

also, it miss the event documentation that should take place there. I will look for a good compromise.

aileo · 2019-12-06T12:03:09Z

I tried something to remove the event big switch. It hits me that with the #49 and specific class per device the edge cases would disappear.

I still don't understand how to read modes sequentially. I think Hub._registerDeviceAttachment could read all modes before emitting the attach event, but I don't know if it is possible to wait for a specific sequence of messages...

nutki · 2019-12-06T20:36:25Z

Nicely done @aileo. I was also thinking about reworking the sensor reading code. I had some similar ideas, but overall I would try to go more radical, but I am not sure how viable would that be. Specifically I would try to make subscribe unnecessary. Simply by registering a listener of an event with on the library should be able to switch to a certain mode automatically.
As for the issue of port information reporting delay, my main concern would be that the library would become very fragile to potential message drop. What if a port mode name message is missed for some internal device? Even in positive outcome the operation would be significantly delayed. Reading all port mode infos for just the internal devices on the Boost hub takes almost 2s.
I have some clue about how the official Lego SDK handles it since it leaves some files during the operation. For example a file 46-0.0.00.1000-0.0.00.1000 with contents:

CAPABILITIES:15
MODE_COUNT:6
INPUT_MODES:30
OUTPUT_MODES:31
ALLOWED_COMBINATIONS:DgA=
0/NAME:POWER
0/MIN_RAW:-100
0/MAX_RAW:100
0/MIN_PCT:-100
0/MAX_PCT:100
0/MIN_SI:-100
0/MAX_SI:100
0/SYMBOL:PCT
0/MAPPING_INPUT:16
0/MAPPING_OUTPUT:0
0/VALUE_FORMAT_COUNT:1
0/VALUE_FORMAT_DECIMALS:0
0/VALUE_FORMAT_FIGURES:1
0/VALUE_FORMAT_TYPE:0
1/NAME:SPEED
1/MIN_RAW:-100
1/MAX_RAW:100
1/MIN_PCT:-100
...

This is obviously port mode information and the file name contains the device type number and its software and hardware version. This suggests that Lego's official SDK:

uses this information is some way
the information can only change when the software or hardware version changes for the device (they are both present in the device attach message)

src/hub.ts

nutki · 2019-12-06T20:44:43Z

src/lpf2hub.ts

-            this._sendPortInformationRequest(data[3]);
        }
+        this._sendPortInformationRequest(data[3]);


I specifically removed it to avoid unnecessary radio static, of course since you actually use this information it cannot be avoided (though it could be trimmed to the messages you use - name and data type).

src/lpf2hub.ts

nutki · 2019-12-06T20:57:16Z

src/lpf2hub.ts

+             * @param {number} z
+             */
+            ACCEL: "acceleration",
+            ROT: "acceleration"


C+ hub 'ROT' mode is angular velocity (or rotation speed), acceleration is reported by the 'GRV' mode.

I totaly missed those, I did not see GRV mode.

It is on the previously not supported port 0x61 mode 0, I added it in a recently merged PR.

nutki · 2019-12-06T21:03:57Z

src/lpf2hub.ts

        }

    }

-
+    private _emitSensorEvent(port: Port, mode: IPortMode, values: number[]) {
+        const modeToEvent: { [key: string]: string } = {


Good idea with the event name map, but I think it would be cool it the users could also subscribe to ports using these names. Otherwise you need to remember that to fire distance events you need to use PROX or LPF2-DETECT depending on the device used.

There is aleady this.emit("sensor", port.id, mode, values); and it would make a lot of duplicated events, I don't know what approach should be adopted.

nutki · 2019-12-06T21:09:56Z

src/lpf2hub.ts

+        } else if (mode.name === "POS" && port.type === Consts.DeviceType.CONTROL_PLUS_TILT) {
+            this.emit("angle", port.id, ...values);
+        } else if (modeToEvent[mode.name]) {
+            this.emit(modeToEvent[mode.name], port.id, ...values);


This should ideally also normalize the values according to min/max raw and si unit. For example accel in boost hub 1G = 65 while in C+ hub 1G=4096.

Do you have any link to the device/firmware mode information files ? I would try to work on a normalization helper to convert values.

src/lpf2hub.ts

nathankellenicki · 2019-12-07T00:46:24Z

I tried something to remove the event big switch. It hits me that with the #49 and specific class per device the edge cases would disappear.

Agreed. I'm about to push my (very) WIP branch with these changes so you can take a look at the approach, but I think it should allow for per device mode types.

I still don't understand how to read modes sequentially. I think Hub._registerDeviceAttachment could read all modes before emitting the attach event, but I don't know if it is possible to wait for a specific sequence of messages...

I think some kind of counter would be required, ie. device reports 4 modes (totalModes), so increment a counter (reportedModes) whenever a piece of mode info comes in. When reportedModes => totalModes, resolve a promise so that higher order code can continue.

nathankellenicki · 2019-12-07T00:51:54Z

Nicely done @aileo. I was also thinking about reworking the sensor reading code. I had some similar ideas, but overall I would try to go more radical, but I am not sure how viable would that be. Specifically I would try to make subscribe unnecessary. Simply by registering a listener of an event with on the library should be able to switch to a certain mode automatically.

This is fantastic idea, I hadn't thought about that! I'm literally going to start implement this in my working branch for #49 - I was about to work in subscribe methods, but this is a step up.

The question of combined ports becomes an issue - ie. If you attach an event handler for "color", and one for "reflectivity", it should probably set up combined mode for the device so both can be reported. But this can be tackled later (using @aileo's code here in this PR as a base) - v1 can be one at a time.

As for the issue of port information reporting delay, my main concern would be that the library would become very fragile to potential message drop.

Can BLE drop messages? I thought it was a "reliable" transport protocol like TCP, however I understand that some devices drop mesages in low memory situations. However how common will this be with PUP devices?

This is obviously port mode information and the file name contains the device type number and its software and hardware version. This suggests that Lego's official SDK:

uses this information is some way

the information can only change when the software or hardware version changes for the device (they are both present in the device attach message)

This is actually a great idea (essentially caching the information for known firmware versions). We could probably follow this approach?

aileo · 2019-12-07T08:29:12Z

Thanks @nutki for the review, you always find all my mistakes, and they are numerous.

Nicely done @aileo. I was also thinking about reworking the sensor reading code. I had some similar ideas, but overall I would try to go more radical, but I am not sure how viable would that be. Specifically I would try to make subscribe unnecessary. Simply by registering a listener of an event with on the library should be able to switch to a certain mode automatically.

This is fantastic idea, I hadn't thought about that! I'm literally going to start implement this in my working branch for #49 - I was about to work in subscribe methods, but this is a step up.

The question of combined ports becomes an issue - ie. If you attach an event handler for "color", and one for "reflectivity", it should probably set up combined mode for the device so both can be reported. But this can be tackled later (using @aileo's code here in this PR as a base) - v1 can be one at a time.

I think it could be great as it makes events/mode relation very simple for the user. My concerns are over the complexity it adds to the lib:

As all modes are not compatible with others (even with combination), we have to manage off and it forces developers to keep a reference to the listener. removeAllListener can be the solution for lazy developers (me).
If the lib override on and off, it should also override addListener, once, removeListener, removeAllListener, prependListener and prependOnceListener. This could eventually be handled by using newListener and removeListener event to make subscribe/unsubscribe logic.

nathankellenicki · 2019-12-17T23:00:00Z

I think it could be great as it makes events/mode relation very simple for the user. My concerns are over the complexity it adds to the lib:

As all modes are not compatible with others (even with combination), we have to manage off and it forces developers to keep a reference to the listener. removeAllListener can be the solution for lazy developers (me).

If the lib override on and off, it should also override addListener, once, removeListener, removeAllListener, prependListener and prependOnceListener. This could eventually be handled by using newListener and removeListener event to make subscribe/unsubscribe logic.

@aileo I think you presented perfectly good solutions to those concerns. removeAllListeners can be used to remove any listeners the user has added, and the newListener can be used to hook into when new events are added (and removeListener when removed).

In the event the user isn't properly removing listeners, and until combined mode is added, the "last added wins" approach can work, as it's an existing paradigm that makes sense to users (up to now in this lib, you could only set one mode at a time).

@nutki I've actually implemented your suggestion in #61 and I think it's a great solution. Currently I'm not working on combined modes (this is MVP1) and still need to implement unsubscribe, but it's working great.

nathankellenicki · 2019-12-17T23:01:43Z

@aileo I'd actually love to know your thoughts on how to merge some of this with #61.

I've managed to eliminate the "Big Ugly Switch Statement(TM)" of mode numbers to event names by creating a map of them and having it handled automatically, but for parsing of the sensor messages, I still have a switch statement.

I think maybe some of this could work with that, by reverse looking up the event name from the currently set mode? Some special cases could still be required, but I wonder if it could simplify it greatly?

aileo · 2019-12-18T09:31:05Z

@aileo I'd actually love to know your thoughts on how to merge some of this with #61

I think we have to drop the part of this PR where it get parsing information from mode info as it lead to big latency (and it totally rely on not missing any message). Some sort of configuration could do the job (I don't think mode are updated that often and we could maintain some firmware related configuration like @nutki showed here if needed.

From what I investigated about modes in #56 , modes do not have the same position (number, id) on every hub (L/XL motors have load mode on PUP and C+ at 0x04 and CALIB at 0x05 but CALIB is at 0x04 and STATS at 0x05 on Boost).

I think we have a list of modes for each device type with :

a name
information about their parsing
- value type (8/16/32 bits, floats)
- number of values
- min/max
- unit
- all the fancy stuff we can imagine (conversion function, coefficient, ...)
their number/id depending on the hub type

We can map event name to mode name or add directly event name to the modes configuration.

I would make this configuration a static attribute of each device class instead of passing it to super. Maybe it could be a json file so we can track changes easily and generate it with some npm script using this lib (something like #51) ?

I would also defer the parsing to the device class when it comes to device sensor messages (or other pure device messages). This way the edge cases would be handle by the device itself and this could simplify some test suite implementation :) .

The first step is to collect all modes information for all devices on all hubs. I will do some as soon as I get my hands back on my hubs.

Side note : talking about "Big Ugly Switch Statement(TM)", a device type to constructor map would also remove the one in hub._createDevice

aileo · 2020-06-26T09:58:21Z

#97 is way better than this

Experiment: read sensor data with mode informations

0dc2310

aileo commented Dec 4, 2019

View reviewed changes

src/lpf2hub.ts Outdated Show resolved Hide resolved

Remove ginormous switch and add event doc

a0e16cf

aileo mentioned this pull request Dec 6, 2019

Attempt to read differents device modes #56

Closed

nutki reviewed Dec 6, 2019

View reviewed changes

nathankellenicki mentioned this pull request Dec 7, 2019

New paradigm for handling attached devices #61

Merged

nathankellenicki added the Do Not Merge label Dec 7, 2019

Leo Bonnargent added 3 commits December 7, 2019 21:01

Unsigned values until raw min < 0

7ebc1e0

refactor subscribeByModeName

740ea0c

minor fixes

77f1183

aileo mentioned this pull request Dec 18, 2019

Feature/devices: parsing sensor #65

Closed

aileo mentioned this pull request Jun 22, 2020

Dynamic modes #97

Draft

aileo closed this Jun 26, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Experiment: read sensor data with mode informations #60

Experiment: read sensor data with mode informations #60

aileo commented Dec 4, 2019

aileo Dec 4, 2019

aileo Dec 4, 2019

nutki Dec 6, 2019

nathankellenicki commented Dec 5, 2019

aileo commented Dec 6, 2019

aileo commented Dec 6, 2019

nutki commented Dec 6, 2019

nutki Dec 6, 2019

nutki Dec 6, 2019

aileo Dec 7, 2019

nutki Dec 7, 2019

nutki Dec 6, 2019

aileo Dec 7, 2019

nutki Dec 6, 2019 •

edited

Loading

aileo Dec 7, 2019

nathankellenicki commented Dec 7, 2019

nathankellenicki commented Dec 7, 2019

aileo commented Dec 7, 2019

nathankellenicki commented Dec 17, 2019

nathankellenicki commented Dec 17, 2019 •

edited

Loading

aileo commented Dec 18, 2019

aileo commented Jun 26, 2020

		this._activatePortDevice(port.value, type, this._getModeForDeviceType(type), 0x00);
		this.subscribe(port.id, this._getModeForDeviceType(type));

Experiment: read sensor data with mode informations #60

Experiment: read sensor data with mode informations #60

Conversation

aileo commented Dec 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nathankellenicki commented Dec 5, 2019

aileo commented Dec 6, 2019

aileo commented Dec 6, 2019

nutki commented Dec 6, 2019

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

nutki Dec 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nathankellenicki commented Dec 7, 2019

nathankellenicki commented Dec 7, 2019

aileo commented Dec 7, 2019

nathankellenicki commented Dec 17, 2019

nathankellenicki commented Dec 17, 2019 • edited Loading

aileo commented Dec 18, 2019

aileo commented Jun 26, 2020

nutki Dec 6, 2019 •

edited

Loading

nathankellenicki commented Dec 17, 2019 •

edited

Loading