search

energostack / bisquitt

Bisquitt, a transparent MQTT-SN gateway
Eclipse Public License 2.0
21 stars 3 forks source link

packets2: Add register and subscribe-related packets #58

Closed mprymek closed 1 year ago

mprymek commented 1 year ago

PR adds register and subscribe-related packets: Register, Regack, Subscribe, Suback, Unsubscribe, Unsuback.

Failed tests are OK. They will pass as long as all packets are merged in.

mprymek commented 1 year ago

Changes look good. I’m still curious about the answer to my question above:

I have a more general question though: Handling of Topic Alias Type, Topic Alias, Topic Name and Topic Filter is done in multiple packets. The code dealing with packing, unpacking, computing packet length, and stringification is highly repetitive. Have you considered abstracting it a bit?

You are right that handling of these properties is much more unified and sane in MQTT-SN v.2 than in v.1 (e.g. the "long" topic ID type has the same flag value as the registered topic ID type in v.1 [1]) and there's probably room for some abstraction. Nevertheless, there are some minor deviations in particular packets and I must carefully investigate if it's worth it and the abstraction will not just complicate the code without much benefit.

I think it's out of scope of this PR. I made an issue https://github.com/energomonitor/bisquitt/issues/64 and will take a look later.

[1] https://github.com/energomonitor/bisquitt/blob/main/packets1/packets1.go#L44

dmajda commented 1 year ago

I think it's out of scope of this PR. I made an issue #64 and will take a look later.

Makes sense, thanks.