net: added emCute - introducing MQTT-SN support #6633

haukepetersen · 2017-02-20T23:19:36Z

Finally found some time last weekend to look into MQTT-SN... I am happy to introduce emCute, my 'from-scratch' approach to implementing MQTT-SN. I tried to make it very user-friendly and leight-weight -> on the iotlab-m3 it takes currently ~2.1k of ROM and 1245b RAM (of which are 1180b buffers + sock).

The implementation supports most major features defined for MQTT-SN except gateway discovery and QoS > 0. The doxygen in sys/include/net/emcute.h should give a pretty good overview on supported, missing, and planned features.

Currently, there are two major things left, that have to be fixed until this PR can be merged (all other things can be handled in follow up PRs):

the message re-send is broken. This is most probably caused by a bug in thread_flags -> see core: thread_flags seem to be broken on native #6624
sending of periodic ping request is currently commented out. I am not sure if I did something wrong here (its late...), or if sock_udp_recv() has a bug when setting the timeout value...

aabadie · 2017-02-21T08:57:00Z

That looks awesome. I would be happy to give a try.

miri64 · 2017-02-27T13:14:52Z

sys/net/application_layer/emcute/emcute.c

+//         uint8_t buf[2] = { 2, PINGREQ };
+//         sock_udp_send(&sock, &buf, 2, &gateway);
+//     }
+// }


please remove or use c-style comments

haukepetersen · 2017-02-27T13:52:59Z

Periodic sending of ping messages and handling of topics with QoS_1 are now also working.

This PR is now ready for review (and hopefully for merge). All open todos will/should be addressed in follow up PRs... Happy testing :-)

miri64 · 2017-02-27T14:04:02Z

sys/net/application_layer/emcute/emcute.c

+static volatile uint16_t waitonid = 0;
+static volatile int result;
+
+static inline uint16_t get_u16(const uint8_t *buf)


Why not use byteorder.h?

no reason -> fixed.

miri64 · 2017-02-27T14:04:11Z

sys/net/application_layer/emcute/emcute.c

+#endif
+}
+
+static inline void set_u16(uint8_t *buf, uint16_t val)


Why not use byteorder.h?

miri64 · 2017-02-27T14:07:49Z

examples/emcute/README.md

+./RIOTDIR/dist/tools/tapsetup/tapsetup
+```
+
+2. Assign a site-global prefix to the `tabbr0` interface:


The bridge is named differently under OSX and FreeBSD.

How should I know :-) Will fix

There was just a PR about this: #6647 ;-)

miri64 · 2017-02-27T14:08:52Z

examples/emcute/README.md

+3. Assign a site-global address with the same prefix to the RIOT `native`
+   instance:
+```
+ifconfig 5 add fec0:affe:99


Colon missing

miri64 · 2017-02-27T14:12:12Z

sys/include/net/emcute.h

+ */
+
+/**
+ * @defgroup    net_emcute emCute (MQTT-SN)


Would prefer the name "MQTT-SN client (emCute)" so it jumps into the eyes of newbies ;-).

miri64 · 2017-02-27T14:13:17Z

sys/include/net/emcute.h

+ * - handling re-transmits
+ *
+ * The following features are however still missing (but planned):
+ * @todo        Geteway discovery (so far there is no support for handling


Don't see it :-/

miri64 · 2017-02-27T14:25:04Z

sys/net/application_layer/emcute/emcute_str.c

+const char *emcute_type_str(uint8_t type)
+{
+    switch (type) {
+        case ADVERTISE:     return "ADVERTISE";


I think a lookup-table is way more efficient here.

actually, gcc should convert this to a lookup table, as the type IDs are up-counting without gaps... Also its just for debugging, so efficiency is not uttermost important...

miri64 · 2017-02-27T14:25:43Z

sys/net/application_layer/emcute/include/emcute_internal.h

+ */
+enum {
+    ADVERTISE       = 0x00,     /**< advertise message */
+    SEARCHGW        = 0x01,     /**< search gateway message */


Why enum if you enumerate manually anyways?

Didn't we have discussions some time ago, that this is the preferred way to define flags like this? Apart from that, I think this looks much nicer than having a bunch of defines...

nope it was the other way around, that flags shouldn't be defined as enums, but defines. But these are no flags, but field values.

miri64 · 2017-02-27T14:25:54Z

sys/net/application_layer/emcute/include/emcute_internal.h

+ * @brief   MQTT-SN return codes
+ */
+enum {
+    ACCEPT          = 0x00,     /**< all good */


kaspar030 · 2017-02-27T14:56:24Z

sys/net/application_layer/emcute/emcute.c

-#else
-    return (uint16_t)((buf[1] << 8) | buf[0]);
-#endif
+    network_uint16_t ns = { .u16 = *((uint16_t *)buf) };


nope, like this it leads to alignment problems.

hmm, I knew there was something off... I was looking for something that is as efficient as my manual solution I had here before. Any tips?

haukepetersen · 2017-02-27T16:19:38Z

@miri64: @kaspar030 had a point, the code using byteorder.h was prone to alignment faults. Switched back to my original code -> I will change this only if somebody can show me a at least as efficient solution...

haukepetersen · 2017-02-28T10:53:34Z

rebased

miri64 · 2017-02-28T15:26:52Z

sys/Makefile.include

@@ -32,6 +32,10 @@ ifneq (,$(filter oneway_malloc,$(USEMODULE)))
    USEMODULE_INCLUDES += $(RIOTBASE)/sys/oneway-malloc/include
 endif

+ifneq (,$(filter emcute,$(USEMODULE)))
+    USEMODULE_INCLUDES += $(RIOTBASE)/sys/net/application_layer/emcute/include


git mv sys/net/application_layer/emcute/include/emcute_internal.h sys/net/application_layer/emcute/

then this isn't necessary.

miri64

Tested on native and samr21-xpro. ACK. Please squash.

@kaspar030

Was addressed and dismissal sanctioned by @kaspar030

haukepetersen · 2017-02-28T16:18:43Z

rebased and squashed

haukepetersen · 2017-02-28T17:54:59Z

all green: go go go

keestux · 2020-09-05T19:28:35Z

sys/net/application_layer/emcute/emcute.c

+    mutex_lock(&txlock);
+
+    int pos = set_len(tbuf, (len + 1));
+    len += (pos + 1);


@haukepetersen
This len += (pos + 1); looks suspicious. Coverity Scan tells us that there is a potential buffer overflow.

If you ask me, I would say that this statement should not there. And if it is intended then the if condition above needs to be changed.

len comes from a local user call. This could just be solved by an appropriate assert().

Maybe I wasn't clear. My point is that this statement should not be there at all.

len is the length of the data. It does not make sense to increase it by 4. The three bytes for the length field are already copied into tbuf through set_len. Then WILLMSGUPD is copied into the buffer. What is left to do with memcpy is to copy the data with length len. Notice the &tbuf[pos] being the destination of memcpy.

@haukepetersen any comment to this?

See issue #15028 to discuss this further

haukepetersen added Impact: major The PR changes a significant part of the code base. It should be reviewed carefully Area: network Area: Networking Type: new feature The issue requests / The PR implemements a new feature for RIOT labels Feb 20, 2017

haukepetersen added this to the Release 2017.04 milestone Feb 20, 2017

haukepetersen assigned mfrey and miri64 Feb 20, 2017

miri64 requested changes Feb 27, 2017

View reviewed changes

miri64 added the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Feb 27, 2017

haukepetersen force-pushed the add_mqttsn branch from 8a71690 to 080045a Compare February 27, 2017 13:51

haukepetersen removed the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Feb 27, 2017

miri64 reviewed Feb 27, 2017

View reviewed changes

miri64 requested changes Feb 27, 2017

View reviewed changes

kaspar030 previously requested changes Feb 27, 2017

View reviewed changes

haukepetersen force-pushed the add_mqttsn branch from 2bfee99 to e98e64b Compare February 28, 2017 10:53

miri64 requested changes Feb 28, 2017

View reviewed changes

miri64 approved these changes Feb 28, 2017

View reviewed changes

miri64 added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Feb 28, 2017

haukepetersen added 2 commits February 28, 2017 16:58

net: added emCute - introducing MQTT-SN support

bb71986

examples: added emCute (MQTT-SN) example

25b6138

haukepetersen force-pushed the add_mqttsn branch from a6c2ce8 to 25b6138 Compare February 28, 2017 16:18

haukepetersen added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 28, 2017

haukepetersen removed the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Feb 28, 2017

miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Feb 28, 2017

haukepetersen merged commit 9b7489b into RIOT-OS:master Feb 28, 2017

haukepetersen deleted the add_mqttsn branch February 28, 2017 17:55

keestux reviewed Sep 5, 2020

View reviewed changes

keestux mentioned this pull request Sep 15, 2020

Suspicious code in emCute #15028

Closed

net: added emCute - introducing MQTT-SN support #6633

net: added emCute - introducing MQTT-SN support #6633

Conversation

haukepetersen commented Feb 20, 2017 • edited Loading

aabadie commented Feb 21, 2017

Choose a reason for hiding this comment

haukepetersen commented Feb 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haukepetersen commented Feb 27, 2017

haukepetersen commented Feb 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miri64 left a comment

Choose a reason for hiding this comment

haukepetersen commented Feb 28, 2017

haukepetersen commented Feb 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haukepetersen commented Feb 20, 2017 •

edited

Loading