commschamp / cc.mqttsn.libs

CommsChampion Ecosystem MQTT-SN client / gateway libraries and applications
Mozilla Public License 2.0
63 stars 16 forks source link

Fix subscription of long topic names #5

Closed a1lu closed 4 years ago

a1lu commented 4 years ago

Hi, If one subscribe a topic with more than 2 characters length, the wrong flags are used and it is subscribed as short topic name. This fix checks if a topic name length is more than 2 characters or includes wildcards and uses TopicIdTypeVal::Normal in this case. The MQTTSN spec states that it is not useful to subscribe short topic names with wildcards, they are always handled like long topic names.

Also it registers short topic names in RegInfo, so that we can receive messages, otherwise a PUBACK with invalid topic id flag was returned.

The same was applied to unsubscribe. Since I could not unsubscribe topics by ID that were subscribed by name, I had to reset the topic id if a topic name is available to not trigger the asserts.

I thinks this could not merged like this, since I did not fix the tests and some other issues:

arobenko commented 4 years ago

It looks like I completely misread / misunderstood the MQTT-SN spec and probably didn't have proper PAHO implementation at that time to compare and test my client against their work.

My understanding of the spec was based on the actual TopicIdType description:

TopicIdType: indicates whether the field TopicId or TopicName included in this message contains a normal topic id (set to “0b00”), a pre-defined topic id (set to “0b01”), or a short topic name (set to “0b10”). The value “0b11” is reserved.

So, my interpretation was "0b00" is using known topic id of previously registered topic string, while "0b10" is actual string of topic name (regardless of its length). Based on what you describe and now after I re-read the spec I came to the conclusion that SUBSCRIBE message cannot use known topic ID of previously registered topic (not pre-defined one). It looks like it needs to be as following (correct me if you understand it differently)

My personal opinion using separate handling of "short" (2 bytes) topic name strings (instead of allowing handling of already registered topic IDs) is dumb, but spec is the spec and there is need to comply. It looks I need to invest some significant effort to rework the existing logic in both client and gateway. It may take me some time (maybe even weeks, not days). Sorry about that.

a1lu commented 4 years ago

Hi,

Based on what you describe and now after I re-read the spec I came to the conclusion that SUBSCRIBE message cannot use known topic ID of previously registered topic (not pre-defined one). It looks like it needs to be as following (correct me if you understand it differently)

  • 0b00 - Topic ID string
  • 0b01 - Predefined topic ID of 2 bytes
  • 0b10 - Short topic name of 2 bytes.

I think you are correct, I understand it the same. Regarding the flags in SUBSCRIBE the spec says (5.4.15 SUBSCRIBE):

TopicIdType: indicates the type of information included at the end of the message, namely “0b00” topic name, “0b01” pre-defined topic id, “0b10” short topic name, and “0b11” reserved.

BTW pahos implementation also assigns the same topic Id to the same topic name in a subscription.

IMO the advantage of the short topic names is that you don't have to register / pre-define them and you can publish to this topics without connecting to a gateway / broker.

arobenko commented 4 years ago

Hi @a1lu, Just FYI, "develop" branch should have the relevant "client" code fixes (not gateway yet). Note, that at this stage I haven't yet tested it myself with paho gateway (only unittests). I'll proceed to the gateway part implementation as the next stage. If you have time and desire you can try to test my current client implementation with paho gateway and let me know if you experience any problems.

Regards, Alex

a1lu commented 4 years ago

Hi @arobenko , develop has no new commits yet? Thanks for your time.

arobenko commented 4 years ago

Sorry, forgot to push. Try now

arobenko commented 4 years ago

I've tested it myself and it looks like the "develop" branch client works fine with paho gateway now. My gateway still needs some work though. I'll try to fix it during the next week and then make an official release (merge to master).

arobenko commented 4 years ago

Fixed in v0.8.5 release

arobenko commented 4 years ago

Fixed in v0.8.5 release