thomasnordquist / MQTT-Explorer

An all-round MQTT client that provides a structured topic overview
https://mqtt-explorer.com
Other
2.99k stars 287 forks source link

MQTT-Explorer 0.4.0-beta4 SparkplugB decode some strings as "{}" #792

Closed LoicGoulefert closed 4 months ago

LoicGoulefert commented 4 months ago

In the latest release of MQTT-Explorer, some payloads seem to be decoded by SparkplugB when they're not meant to be. This results in some topics to display "{}" instead of the actual payload.

@kc2zgu mentionned here that some strings (like 5000) are detected as SparkplugB payloads. I can't reproduce this in my application, but the string "empty" gets decoded by SparkplugB, and rendered as {} in MQTT-Explorer. Turns out that strings of length 5 starting with "e" are decoded wrongly, based on a few manual tests.

kc2zgu commented 4 months ago

5-character strings starting with "5" are what I noticed. So "50000" or "50.00" but not "5000"

LoicGoulefert commented 4 months ago

True, did some more tests and all the strings that you mentioned turned into {} in MQTT-Explorer. I can't count :disappointed:

bj00rn commented 4 months ago

@thomasnordquist this is a release stopper i guess, sparkplugb decoding "detection" seems to be flaky..

image

The decoder choice logic is pretty naive at the moment it seems, it simply tries to decode the payload as sparkplug and if the result is !== undefined we return it. Any suggestion on how to improve the logic is welcome.

https://github.com/thomasnordquist/MQTT-Explorer/blob/65b86ac5f665ef8522168b3787183097ada1d6c6/backend/src/index.ts#L52

A naive solution might be, to add a check to see if Object.keys(message).length === 0 here, and return undefined here? I lack deeper understanding of protobuf decoding, maybe there is a way to inspect the payload to determine if it's sparkplugb.

https://github.com/thomasnordquist/MQTT-Explorer/blob/65b86ac5f665ef8522168b3787183097ada1d6c6/backend/src/Model/sparkplugb.ts#L14

LoicGoulefert commented 4 months ago

Tried to run some more tests, here's what i found: Strings starting with any of the following characters: 'e', 'E', 'u', 'U', 'm', 'M', 5, with a length that is a multiple of 5, will be decoded as {}.

thomasnordquist commented 4 months ago

Thank you, I consider this rather critical. Maybe we can use part of this implementation https://github.com/thomasnordquist/MQTT-Explorer/pull/493 to make the decoding customizable.

I believe currently we run a decoder chain which is kind of broken

tryToDecodeAsJson else tryToDecodeAsSparkplugB else ....

bj00rn commented 4 months ago

Thank you, I consider this rather critical. Maybe we can use part of this implementation #493 to make the decoding customizable.

I believe currently we run a decoder chain which is kind of broken

tryToDecodeAsJson else tryToDecodeAsSparkplugB else ....

Thank you, I consider this rather critical. Maybe we can use part of this implementation #493 to make the decoding customizable.

I believe currently we run a decoder chain which is kind of broken

tryToDecodeAsJson else tryToDecodeAsSparkplugB else ....

Would be awesome to prepare for image support to while we are at it. https://github.com/thomasnordquist/MQTT-Explorer/issues/734

jst-mbs commented 4 months ago

I lack deeper understanding of protobuf decoding, maybe there is a way to inspect the payload to determine if it's sparkplugb.

As someone that has knowledge about Sparkplug but not about JS/TS, I would suggest to inspect the topic structure to detemine if the message is sparkplugb. In my opinion, relying only on the message payload is a bad idea. The sparkplug specifikation can be found here: https://www.eclipse.org/tahu/spec/sparkplug_spec.pdf. The chapter 4.1 is describing the topic structure:

All MQTT clients using the Sparkplug specification MUST use the following topic namespace structure:

namespace/group_id/message_type/edge_node_id/[device_id]

The namespace element must be spBv1.0.

tobehn commented 4 months ago

I think @jst-mbs has a pretty good point. spBv1.0 will always be in the topic of the message. It is at least a good way to double check if decoding might need to be used. Though in some cases people send uncompressed messages to the spBv1.0 namespace. For example i do not know if all STATE messages in SparkplugB are send protobuf compressed.

jst-mbs commented 4 months ago

Though in some cases people send uncompressed messages to the spBv1.0 namespace. For example i do not know if all STATE messages in SparkplugB are send protobuf compressed.

You are right: The state messages of the 'Host application' is JSON encoded, see chapter '4.2 Sparkplug Host Application'.

bj00rn commented 4 months ago

Sidenote, there is a package sparkplug-payload that can do decoding/encoding. Might be worth looking into instead of rolling own protobuf decoding.

bj00rn commented 4 months ago

I think @jst-mbs has a pretty good point. spBv1.0 will always be in the topic of the message. It is at least a good way to double check if decoding might need to be used. Though in some cases people send uncompressed messages to the spBv1.0 namespace. For example i do not know if all STATE messages in SparkplugB are send protobuf compressed.

@tobehn @jst-mbs have a look at #793, Will (topic.match(/spBv1\.0\/[^/]+\/(DDATA|NDATA|NCMD|DCMD|NBIRTH|DBIRTH|NDEATH|DDEATH\/[^/]+\/)/u)) regexp suffice to identify encoded sparkplug payloads?

tlifschitz commented 4 months ago

Just for the record, in our application we use custom protobuf messages and the wrongly decoded messages we are seeing are not empty but with random invalid data, as shown below.

Hope @jst-mbs proposal and @bj00rn implementation fixes this issue, thanks!

Payload 26 bytes b'*\x18\x1a\x16\x08\x0e\xf2\x01\x11\x08\x01\x12\r\x08\x03\x10\x01\x18\x02 \x02HUP\xf0.' Decoded Sparkplug(?) {"body":{"0":26,"1":22,"2":8,"3":14,"4":242,"5":1,"6":17,"7":8,"8":1,"9":18,"10":13,"11":8,"12":3,"13":16,"14":1,"15":24,"16":2,"17":32,"18":2,"19":72,"20":85,"21":80,"22":240,"23":46}}

Payload 36 bytes b'\x12\x0fwmod-din-v0.08b*\x11f/wmod-din-v0.08b' Decoded Sparkplug(?) {"metrics":[{"isHistorical":true,"isNull":true,"doubleValue":3.0644533518218016e-75,"booleanValue":true}],"body":{"0":102,"1":47,"2":119,"3":109,"4":111,"5":100,"6":45,"7":100,"8":105,"9":110,"10":45,"11":118,"12":48,"13":46,"14":48,"15":56,"16":98}}

image

bj00rn commented 4 months ago

Just for the record, in our application we use custom protobuf messages and the wrongly decoded messages we are seeing are not empty but with random invalid data, as shown below.

Hope @jst-mbs proposal and @bj00rn implementation fixes this issue, thanks!

Payload 26 bytes b'*\x18\x1a\x16\x08\x0e\xf2\x01\x11\x08\x01\x12\r\x08\x03\x10\x01\x18\x02 \x02HUP\xf0.' Decoded Sparkplug(?) {"body":{"0":26,"1":22,"2":8,"3":14,"4":242,"5":1,"6":17,"7":8,"8":1,"9":18,"10":13,"11":8,"12":3,"13":16,"14":1,"15":24,"16":2,"17":32,"18":2,"19":72,"20":85,"21":80,"22":240,"23":46}}

Payload 36 bytes b'\x12\x0fwmod-din-v0.08b*\x11f/wmod-din-v0.08b' Decoded Sparkplug(?) {"metrics":[{"isHistorical":true,"isNull":true,"doubleValue":3.0644533518218016e-75,"booleanValue":true}],"body":{"0":102,"1":47,"2":119,"3":109,"4":111,"5":100,"6":45,"7":100,"8":105,"9":110,"10":45,"11":118,"12":48,"13":46,"14":48,"15":56,"16":98}}

image

Thanks, there is clearly a bug with sparkplug decoding. Hopefully we'll get a new beta sorted soon that you can try.