net: add Asymcute, an asynchronous MQTT-SN client implementation by haukepetersen · Pull Request #9464 · RIOT-OS/RIOT

haukepetersen · 2018-06-29T15:45:05Z

Contribution description

This PR adds a new MQTT-SN client implementation called Asymcute. In contrary to the exising emCute implementation, this one is completely asynchronous, thus allowing for a higher throughput and for more flexibility to the cost of increased memory usage (both RAM and ROM).

Asymcute is not only able to handle multiple pending requests concurrently, it is also able to connect to more than one gateway at a time.

The motivation for implementing a second MQTT-SN client comes mainly from some application layer protocol experiments that we were running earlier this year in the context of our 'I3' research project, where we were measuring certain metrics for MQTT-SN, CoAP and some ICN-based approaches. For this, the existing synchronous emCute library was not well suited, so hence the need for a fully asynchronous MQTT-SN client library.

Testing

Easiest way (using native):

run a local mqtt-sn broker using the build in rsmb make target (see tools: add mosquitto.rsmb MQTT-SN broker #9459)
run the examples/asymcute_mqttsn application under native
play, also hard-kill (kill -9) the rsmb instance to see the effect due to time out on keep-alive messages. For this it makes sense though to reduce the keep alive interval (override ASYMCUTE_KEEPALIVE).

Issues/PRs references

rebased/depending on #9459

waehlisch · 2018-06-29T15:57:17Z

Just to clarify the motivation for this PR: The reason of having asynchronous MQTT-SN support is not because of a research project but because RIOT should support MQTT-SN 1.2 more completely.

The MQTT spec,, which is the base for MQTT-SN, says explicitly in Section 4.3.2: Note that a Sender is permitted to send further PUBLISH Packets with different Packet Identifiers while it is waiting to receive acknowledgements.

miri64 · 2018-07-02T11:23:31Z

I wait with my proper review until #9459 is merged, but please fix the typo in a33f95c (s/clien/client/) until then

haukepetersen · 2018-07-04T07:45:34Z

The MQTT-SN spec says explicitly in Section 4.3.2: Note that a Sender is permitted to send further PUBLISH Packets with different Packet Identifiers while it is waiting to receive acknowledgements.

But to underline the beauty of the 'standard': Section 6.6 also states At any point in time a client may have only one QoS level 1 or 2 PUBLISH message outstanding, i.e. it has to wait for the termination of this PUBLISH message exchange before it could start a new level 1 or 2 transaction

So with the two implementations now available, we are complying to either statement and have it all covered :-)

haukepetersen · 2018-07-04T09:43:36Z

fixed the PR title...

miri64 · 2018-07-04T10:14:11Z

Needs rebase.

haukepetersen · 2018-07-04T10:18:57Z

rebased (and fixed typo in commit title)

miri64

~~Somehow GitHub dropped a lot of my review comments (after 4 hours of intense review :(), so here's what's left for now. Will look at it tomorrow again.~~ Mh now they all here again...

miri64 · 2018-07-04T10:19:14Z

examples/asymcute_mqttsn/README.md

+```
+
+This will download, build, and run the Eclipse Mosquitto.rsmb `Really Small
+Message Broker` [(found here)](https://github.com/eclipse/mosquitto.rsmb).


Nit-picky: Any reason for the monospace format for "Really Small Message Broker"?

miri64 · 2018-07-04T11:09:24Z

examples/asymcute_mqttsn/main.c

+#include "net/ipv6/addr.h"
+
+#ifndef REG_CTX_NUMOF
+#define REG_CTX_NUMOF       (8U)


miri64 · 2018-07-04T11:13:30Z

sys/include/net/asymcute.h

+ * @brief   Asymcute connection context
+ */
+struct asymcute_con {
+    uint8_t state;              /**< connection state */


Can you group this with last_id and keepalive_retry_cnt please, to save some RAM by packing the struct?

IMO this would be premature optimization, as the size of this struct is quite significant anyway (and contains 'wasted' fill bytes depending on the configuration)... In this case I actually think that there is a better gain by logically grouping the fields in this struct.

At what point becomes sensible optimization premature. I actually don't see the logical connection of state to lock or last_id (message ID) to client_id (except for the fact that they are both called ID). The only field I agree with is keepalive_retry_cnt, but if you move state below and last_id below that, you still have a logical grouping and waste-less packing ;-).

miri64 · 2018-07-04T11:18:01Z

sys/include/net/asymcute.h

+ * @param[out] topic        topic to initialize
+ * @param[in] topic_name    topic name (ASCII), may be NULL if topic should use
+ *                          a pre-shared topic ID
+ * @param[in] topic_id      pre-shared topic ID, or don't care if @p topic_name


At other places you use "pre-defined topic ID". Is this the same concept? If yes, please change here and in the line above.

miri64 · 2018-07-04T11:20:47Z

sys/include/net/asymcute.h

+/**
+ * @brief   Start a listener thread
+ *
+ * @note    Must have higher priority then the handler thread


Since asymcute_handler_run() does not receive a priority, it would be really helpful if it would be stated here what the priority of the handler thread is. So please reference ASYMCUTE_HANDLER_PRIO here.

miri64 · 2018-07-04T13:52:06Z

examples/asymcute_mqttsn/main.c

+    }
+    if (argc >= 4) {
+        ep.port = (uint16_t)atoi(argv[3]);
+    }


Use sock_udp_str2ep() in net/sock/util.h instead?

could, if the sock_util module would be compiling and working...

See #9464 (comment)

Fix in #9498

miri64 · 2018-07-04T13:56:32Z

examples/asymcute_mqttsn/main.c

+
+static uint16_t _topic_parse_pre(const char *name, size_t len)
+{
+    if ((len > 4) && memcmp(name, "pre_", 4) == 0) {


strncmp()

miri64 · 2018-07-04T13:57:02Z

examples/asymcute_mqttsn/main.c

+
+static void _topics_clear(void)
+{
+    memset(_topics, 0, (sizeof(asymcute_topic_t) * TOPIC_BUF_NUMOF));


Use sizeof(_topics) instead.

miri64 · 2018-07-04T13:59:03Z

examples/asymcute_mqttsn/main.c

+        return 1;
+    }
+    if (_topic_init(t, argv[1]) != 0) {
+        puts("error: unable to parse topic");


Since _topic_init() returns 1 when asymcute_topic_init() returns an error, this error message seems to be misleading.

not sure I understand.

Just use the same output as in _cmd_reg(). An error in _topic_init() not only can mean a parsing error.

miri64 · 2018-07-04T14:12:48Z

examples/asymcute_mqttsn/main.c

+    for (; i < TOPIC_BUF_NUMOF; i++) {
+        if (strcmp(argv[1], _topics[i].name) == 0) {
+            for (unsigned s = 0; s < SUB_CTX_NUMOF; s++) {
+                printf("cmp: %p vs %p\n", (void *)_subscriptions[i].topic, (void *)&_topics[i]);


Debug output?

miri64 · 2018-07-04T14:21:19Z

sys/net/application_layer/asymcute/asymcute.c

+    if (con->subscriptions == sub) {
+        con->subscriptions = sub->next;
+    }
+    for (asymcute_sub_t *e = con->subscriptions; e->next; e = e->next) {


Getting a segmentation fault when trying to unsubscribe from a (subscribed to) topic in the example application, since e == NULL, when e->next is checked.

haukepetersen · 2018-07-04T16:32:57Z

addressed most comments, but did not have to really test my fixes again... Will look into this first thing tomorrow morning once more.

miri64 · 2018-07-05T09:02:15Z

Both #9497 and #9498 are merged. Please rebase.

miri64 · 2018-07-05T09:32:05Z

examples/asymcute_mqttsn/main.c

+    }
+
+    /* get sock ep */
+    sock_udp_ep_t ep = { .family = AF_INET6, .port = MQTTSN_DEFAULT_PORT};;


Missing space before }, double ;

kaspar030 · 2018-07-05T09:39:24Z

sys/net/sock/sock_util.c

+
+    /* compare family and port */
+    if ((a->family != b->family) || (a->port != b->port)) {
+        return 0;


kaspar030 · 2018-07-05T09:40:16Z

sys/net/sock/sock_util.c

+        case AF_INET:
+            return (memcmp(a->addr.ipv4, b->addr.ipv4, 4) == 0);
+        default:
+            return true;


this returns true if a->family is unknown.

what should it return? My take was that on Family unknown we simply ignore the address field. Maybe not the smartest thing?!

false, otherwise it returns true for different addresses of not compiled-in family.

best would be -ENOTSUP.

could go for: int sock_udp_ep_equal(...)
@return 0 -> equal
@return -ENOTSUP if family unknown

but what for not equal?

hm.

if this is a three-state (equal, nonequal, error), "equal" is probably best represented as zero, because otherwise people write if (sock_udp_ep_equal(a, b)) and never catch the error. In that case, renaming to "sock_udp_ep_cmp()" (as strcmp()) would be most ideomatic C, and only >0 (or 1) is left for non-equal...

Why not simply "raise AddressFamilyNotSupportedException()"?

just played with it, going back to an int return value with a potential negative return value is not very practical, considering the most common use case for this function is probably something like

if (sock_udp_ep_equal(a, b)) { ....

-> I now changed the return value for the default path in the switch statement to false and added a half-sentence on this to the return value desc in the doc

This helped me a lot while testing RIOT-OS#9464 to interact with the broker using the mosquitto shell command clients.

miri64 · 2018-07-05T11:37:25Z

examples/asymcute_mqttsn/main.c

+#include "mutex.h"
+#include "thread.h"
+#include "net/asymcute.h"
+#include "net/ipv6/addr.h"


Include of net/sock/udp.h an net/sock/util.h missing? I think in an example this is important to have.

miri64

Some minor stuff left. I've tested this already and it works! :-)

miri64 · 2018-07-05T11:39:33Z

examples/asymcute_mqttsn/main.c

+{
+    if (argc < 3) {
+        printf("usage: %s <topic> <data> [QoS level]\n", argv[0]);
+          puts("        topic can be\n"


Alignments weird here (I don't mind having it this way, but then please be consistent and do it like this in _cmd_reg() as well.

miri64 · 2018-07-05T11:40:20Z

examples/asymcute_mqttsn/main.c

+{
+    if (argc < 3) {
+        printf("usage: %s <topic> <data> [QoS level]\n", argv[0]);
+          puts("        topic can be\n"


Since this explanation is also used in _cmd_reg and _cmd_sub, maybe have a function _topic_help() that prints that?

miri64 · 2018-07-05T11:48:10Z

sys/include/net/asymcute.h

+    uint8_t keepalive_retry_cnt;        /**< keep alive transmission counter */
+    uint8_t state;              /**< connection state */
+    uint8_t rxbuf[ASYMCUTE_BUFSIZE];    /**< connection specific receive buf */
+    char cli_id[ASYMCUTE_ID_MAXLEN + 1];    /**< buffer to store client ID */


Comment alignment in this struct is pretty wild ;-).

miri64 · 2018-07-05T11:48:58Z

sys/include/net/asymcute.h

+/**
+ * @brief   Reset the given topic
+ *
+ * @note    Make sure that the given topic is not used by any subscription or


Nit-picky: I would rather make this a @warning

miri64 · 2018-07-05T12:01:03Z

sys/include/net/asymcute.h

+static inline bool asymcute_topic_is_init(const asymcute_topic_t *topic)
+{
+    assert(topic);
+    return (strlen(topic->name) > 0);


You can safe a few byte of ROM (on nrf52dk and samr21-xpro I've got 16 bytes, 22 bytes on iotlab-m3) and the <string.h> include by doing topic->name[0] != '\0' here.

miri64 · 2018-07-05T12:06:30Z

sys/net/application_layer/asymcute/asymcute.c

+    (void)con;
+
+    /* reset the subscription context */
+    asymcute_sub_t *sub = (asymcute_sub_t *)req->arg;


miri64 · 2018-07-05T12:11:50Z

sys/net/application_layer/asymcute/asymcute.c

+    for (asymcute_sub_t *cur = con->subscriptions; cur; cur = cur->next) {
+        if (cur->topic->id == topic_id) {
+            sub = cur;
+            break;


miri64 · 2018-07-05T12:12:50Z

sys/net/application_layer/asymcute/asymcute.c

+    }
+    if ((topic_name && ((len == 0) || (len > ASYMCUTE_TOPIC_MAXLEN))) ||
+        (!topic_name && ((topic_id == 0) || (topic_id == UINT16_MAX)))) {
+        return ASYMCUTE_OVERFLOW;


Still confusing, but ok. As long as it's documented.

miri64 · 2018-07-05T12:13:41Z

sys/net/gnrc/sock/udp/gnrc_sock_udp.c

 */

 #include <errno.h>
+#include <string.h>


You can remove these changes now.

miri64 · 2018-07-05T12:16:52Z

examples/asymcute_mqttsn/main.c

+    return NULL;
+}
+
+static int _qos_parse(int argc, char **argv, int pos, unsigned *flags)


Any reason why not just hand over the argument string here instead of the whole array?

yes, because I 'outsource' the detection if enough parameters are passed to the shell command to this function. Else I would have to check for argc > something always before calling this function.

Ah, okay. Missed that part

haukepetersen · 2018-07-05T13:01:18Z

addressed comments and rebased.

miri64 · 2018-07-05T13:06:21Z

sys/net/gnrc/sock/udp/gnrc_sock_udp.c

 *
- * @author  Martine Lenders <[email protected]>
+ * @author      Martine Lenders <[email protected]>
+ * @author      Hauke Petersen <[email protected]>


This can be removed ;-).

miri64

Apart from the minor change to gnrc_sock_udp.c I'm fine with the state of this PR. Please squash

miri64 · 2018-07-05T13:09:53Z

sys/include/net/asymcute.h

+/**
+ * @brief   Buffer size used for emCute's transmit and receive buffers
+ *
+ * The overall buffer size used by Asymcute is this value time two (Rx + Tx).


haukepetersen · 2018-07-05T13:17:37Z

addressed last comments and squashed.

miri64

ACK. Let's give Murdock a run.

miri64 · 2018-07-05T13:31:30Z

sys/net/application_layer/asymcute/asymcute.c

+        return ASYMCUTE_REGERR;
+    }
+    if ((topic_name && ((len == 0) || (len > ASYMCUTE_TOPIC_MAXLEN))) ||
+        (!topic_name && ((topic_id == 0) || (topic_id == UINT16_MAX)))) {


~~cppcheck reports the check for !topic_name to be redundant, and since you can, due to lazy evaluation only land here when topic_name is true in the line above, I say its right. So please remove.~~ bullshit, let me think about it.

Ah, it's your usage of strlen(topic_name) in https://github.com/RIOT-OS/RIOT/pull/9464/files#diff-640646a2b9d2e083e6a60251fdfd4d99R652.

haukepetersen · 2018-07-05T13:44:56Z

blacklisted boards for missing memory and fixed a NULL pointer issue in asymcute_topic_init that cppcheck pointed out.

miri64

Re-ACK

miri64 · 2018-07-05T14:00:40Z

sys/net/application_layer/asymcute/asymcute.c

+
+    if (topic_name == NULL) {
+        if ((topic_id == 0) || (topic_id == UINT16_MAX)) {
+            return ASYMCUTE_OVERFLOW;


I guess now you can have a return value for that equates to invalid, but let's not split hairs over that.

kb2ma · 2018-07-05T14:53:01Z

@haukepetersen, @kaspar030, @miri64

Thanks for sock_udp_ep_equals(), I can reuse that in gcoap. 👍

This helped me a lot while testing RIOT-OS#9464 to interact with the broker using the mosquitto shell command clients.

haukepetersen assigned miri64, cgundogan, kb2ma and PeterKietzmann Jun 29, 2018

waehlisch requested a review from miri64 June 29, 2018 15:57

haukepetersen changed the title ~~Add asymcute~~ net: add Asymcute, an asynchronous MQTT-SN client implementation Jul 4, 2018

haukepetersen force-pushed the add_asymcute branch from a07124b to 08f04a2 Compare July 4, 2018 10:18

miri64 requested changes Jul 4, 2018

View reviewed changes

miri64 reviewed Jul 4, 2018

View reviewed changes

haukepetersen force-pushed the add_asymcute branch from 08f04a2 to 4b9e9fe Compare July 4, 2018 16:31

miri64 mentioned this pull request Jul 4, 2018

sock_util: add posix dependency #9498

Merged

haukepetersen force-pushed the add_asymcute branch from 18e8c8a to e76a812 Compare July 5, 2018 09:31

miri64 requested changes Jul 5, 2018

View reviewed changes

kaspar030 reviewed Jul 5, 2018

View reviewed changes

miri64 added a commit to miri64/RIOT that referenced this pull request Jul 5, 2018

mosquitto_rsmb: add MQTT listener to config

49036c7

This helped me a lot while testing RIOT-OS#9464 to interact with the broker using the mosquitto shell command clients.

miri64 mentioned this pull request Jul 5, 2018

mosquitto_rsmb: add MQTT listener to config #9501

Merged

miri64 reviewed Jul 5, 2018

View reviewed changes

miri64 requested changes Jul 5, 2018

View reviewed changes

haukepetersen force-pushed the add_asymcute branch from 33f96ed to b67ecc5 Compare July 5, 2018 13:01

miri64 reviewed Jul 5, 2018

View reviewed changes

haukepetersen added 2 commits July 5, 2018 15:13

net/sock_udp: add sock_udp_ep_equal()

bf8f8df

net: add header with generic MQTT-SN defines

ed01bf0

haukepetersen force-pushed the add_asymcute branch from b67ecc5 to d1b4194 Compare July 5, 2018 13:17

miri64 approved these changes Jul 5, 2018

View reviewed changes

miri64 reviewed Jul 5, 2018

View reviewed changes

haukepetersen added 3 commits July 5, 2018 15:44

net: add Asymcute (asynchronous MQTT-SN client)

ad20c4f

sys/auto_init: add init for asymcute

fb184a3

examples: add asymcute_mqttsn example application

901ec82

haukepetersen force-pushed the add_asymcute branch from d1b4194 to 901ec82 Compare July 5, 2018 13:44

miri64 approved these changes Jul 5, 2018

View reviewed changes

miri64 merged commit 2e717b2 into RIOT-OS:master Jul 5, 2018

miri64 deleted the add_asymcute branch July 5, 2018 14:01

cladmi added this to the Release 2018.07 milestone Jul 10, 2018

pekkanikander pushed a commit to AaltoNEPPI/RIOT that referenced this pull request Aug 11, 2018

mosquitto_rsmb: add MQTT listener to config

ac7e9dd

This helped me a lot while testing RIOT-OS#9464 to interact with the broker using the mosquitto shell command clients.

pekkanikander pushed a commit to AaltoNEPPI/RIOT that referenced this pull request Aug 28, 2018

mosquitto_rsmb: add MQTT listener to config

e6fa9c9

This helped me a lot while testing RIOT-OS#9464 to interact with the broker using the mosquitto shell command clients.

danpetry pushed a commit to danpetry/RIOT that referenced this pull request Sep 5, 2018

mosquitto_rsmb: add MQTT listener to config

d8cedc6

This helped me a lot while testing RIOT-OS#9464 to interact with the broker using the mosquitto shell command clients.

Conversation

haukepetersen commented Jun 29, 2018

Contribution description

Testing

Issues/PRs references

Uh oh!

waehlisch commented Jun 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miri64 commented Jul 2, 2018

Uh oh!

haukepetersen commented Jul 4, 2018

Uh oh!

haukepetersen commented Jul 4, 2018

Uh oh!

miri64 commented Jul 4, 2018

Uh oh!

haukepetersen commented Jul 4, 2018

Uh oh!

miri64 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

haukepetersen commented Jul 4, 2018

Uh oh!

miri64 commented Jul 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

waehlisch commented Jun 29, 2018 •

edited

Loading

miri64 left a comment •

edited

Loading