-
-
Notifications
You must be signed in to change notification settings - Fork 295
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
fix: use sparkplugb decoder only for spBv1.0 topic #793
Conversation
backend/src/index.ts
Outdated
@@ -47,9 +47,17 @@ export class ConnectionManager { | |||
buffer = buffer.slice(0, 20000) | |||
} | |||
|
|||
let decoded_payload = null | |||
// spell-checker: disable-next-line | |||
if (topic.match(/spBv1\.0\/[^/]+\/(DDATA|NDATA|NCMD|DCMD|NBIRTH|DBIRTH|NDEATH|DDEATH\/[^/]+\/)/u)) { |
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 first '/' seems to be wrong. Sparkplug topics don't start with '/'.
- The device topics have an additional topic level for the device id. The regex expression is missing that topic except for the DDEATH topic type.
You can try this expression: ^spBv1\.0\/[^\/]+\/(N(DATA|CMD|BIRTH|DEATH)\/[^\/]+|D(DATA|CMD|BIRTH|DEATH)\/[^\/]+\/[^\/]+)$
. I have tested that expression only manually.
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 first '/' seems to be wrong. Sparkplug topics don't start with '/'.
The first slash is just js notation for regexp, if you mean topice.match(/spBv1
.
- The device topics have an additional topic level for the device id. The regex expression is missing that topic except for the DDEATH topic type.
Simplified it a bit as ^spBv1\.0\/[^/]+\/[ND](DATA|CMD|DEATH|BIRTH)\/[^/]+(\/[^/]+)?$
Does it look correct?, have a look at https://regex101.com/r/wwzYQf/1
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.
Simplified it a bit as
^spBv1\.0\/[^/]+\/[ND](DATA|CMD|DEATH|BIRTH)\/[^/]+(\/[^/]+)?$
Does it look correct?, have a look at https://regex101.com/r/wwzYQf/1
Cool link! I think it's ok. The regex expression is also matching node topics with a device id element, like spBv1.0/Sparkplug Devices/NBIRTH/JavaScript Edge Node/Emulated Device
or device topics without a device id element like spBv1.0/Sparkplug Devices/DBIRTH/JavaScript Edge Node
. That is not allowed by the Sparkplug specification but I also don't think, this will cause any troubles.
@thomasnordquist I'll merge this until we get decoding sorted properly in frontend. |
I am almost done, still needs some testing and small fixes
… @thomasnordquist <https://github.com/thomasnordquist> I'll merge this
until we get decoding sorted properly in frontend.
—
Reply to this email directly, view it on GitHub
<#793 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AB25FGOLTU7YIORPPV5WMSDZDBVWDAVCNFSM6AAAAABHYGBNTKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMJZGE3DOMBRGA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Sweer, if you push something I can do some testing too |
Dirty fix #792 by inspecting topic as suggested by #792 (comment)
Tested with sparkplug client https://github.com/eclipse/tahu/blob/master/javascript/examples/simple/example.js
The whole decoder choice logic should probably be cleaned up later on to support #493 and the likes. I guess decoding should not be done in backend in order to choose decoder interactively.. See #655