search

phoddie / node-red-mcu

Node-RED for microcontrollers
124 stars 18 forks source link

MQTTBrokerNode flattens first element of topic by shifting #59

Closed ralphwetzel closed 1 year ago

ralphwetzel commented 1 year ago

Given the following flow fragment:

image

As labeled, the two MQTT In nodes should answer to different topics. Yet they don't (on a MCU). Sending a message of topic check/set triggers an answer of both nodes.

This seems to be due to the fact that the first element of the topic is shift()ed from the array created by topic.split() (line 1020/1021 & again line 1025/1026).

https://github.com/phoddie/node-red-mcu/blob/b92c35703a836ae2511027f13cfb0acea6db12c2/nodered.js#L1017-L1041

Could you please verify?

phoddie commented 1 year ago

Curious. I had tested situations like that. Apparently something got broken along the way. Will take a look.

phoddie commented 1 year ago

It is a real bug. (Thank you for the report!) I always test with a leading slash for the topics ("/test/set") so the initial shift removed an empty element. But that was incorrect because topics do not always begin with a slash. Your proposed fix works well.

In looking into this, I noticed that if two nodes subscribe to the same topic, the broker node can get confused about the type of the payload. That's fixed now.

Sineos commented 1 year ago

Although permitted a leading / is not recommended as MQTT defines each / as topic level separator. This basically creates a empty level as first element:

null/Level1/Level2/...

Some MQTT clients / implementations even react kind of allergic to this.

phoddie commented 1 year ago

@Sineos – Thank you for the comment. That sent me back to the MQTT specification, which confirms what you say. The relevant text is in Section 4.7:

Topic level separators can appear anywhere in a Topic Filter or Topic Name. Adjacent Topic level separators indicate a zero length topic level

I should stop using / at the start of a topic by default, since it is bizarre, even if allowed. I believe the Node-RED MCU Edition implementation now correctly handles topics starting with and without/. Since it is allows, we shouldn't reject such topics though.