[riot-notifications] [RIOT-OS/RIOT] pkg/wakaama: Add basic LWM2M client implementation (#11036)

Juan I Carrano notifications at github.com
Tue Jun 4 16:54:27 CEST 2019


jcarrano requested changes on this pull request.

First, I don't know LWM2M, or waakama. My comments are thus only related to general coding stuff and the RIOT integration.

The most important thing here is that this is ALOT of code, so I probably missed many things. At the same time, I feel its size is not so much because of what it does, but how it does it:

- A pooled allocator implementation that AFAICT does not belong here.
- Bit switch...case that can be replaced by table lookups.
- A big URI parsing routine.
- The lights stuff does not belong to the package.

Because of the last point, I did not go deep into the lights code.

> +#ifndef LWM2M_BSSERVER_PORT
+#define LWM2M_BSSERVER_PORT "5685"
+#endif
+
+/**
+ * @brief Default port for the local LWM2M instance
+ */
+#ifndef LWM2M_LOCAL_PORT
+#define LWM2M_LOCAL_PORT    "5683"
+#endif
+
+/**
+ * @brief Device name used to register at the LWM2M server
+ */
+#ifndef LWM2M_DEVICE_NAME
+#define LWM2M_DEVICE_NAME "testRIOTDevice"

Is it a good idea to give default values for parameters other than port? I can imagine loads of devices showing up as "testRIOTDevice".

> @@ -0,0 +1,4 @@
+USEMODULE += wakaama_contrib
+USEMODULE += wakaama_object_device
+USEMODULE += wakaama_object_lightctrl
+USEMODULE += memarray

newline.

> @@ -0,0 +1,4 @@
+USEMODULE += wakaama_contrib
+USEMODULE += wakaama_object_device
+USEMODULE += wakaama_object_lightctrl

Why is it the default to include a light control?

> @@ -0,0 +1,362 @@
+/*
+ * Copyright (C) 2019 HAW Hamburg
+ *
+ * This file is subject to the terms and conditions of the GNU Lesser
+ * General Public License v2.1. See the file LICENSE in the top level
+ * directory for more details.
+ */
+
+/**
+ * @ingroup     lwm2m_objects_light_control
+ * @{
+ * @brief       Light control object implementation for LWM2M client using

This should be in an example, not in the package.

> @@ -0,0 +1,155 @@
+/*
+ * Copyright (C) 2019 HAW Hamburg
+ *
+ * This file is subject to the terms and conditions of the GNU Lesser
+ * General Public License v2.1. See the file LICENSE in the top level
+ * directory for more details.
+ */
+
+/**
+ * @ingroup     lwm2m_objects
+ * @defgroup    lwm2m_objects_light_control Light Control LWM2M object
+ * @brief       Light Control object implementation for LWM2M client using

Same as with the .c, it should be in the example, not the package.

> +    DEBUG("cnt512: %"PRIu16"/%u max: %"PRIu16"\n", cnt512, MAX_512BYTE_BLOCKS,
+          cnt512_max);
+#endif /* DEBUG_MALLOC */
+    return;
+}
+
+void lwm2m_memarray_init(void)
+{
+    memarray_init(&b16_storage, b16_data, 16, MAX_16BYTE_BLOCKS);
+    memarray_init(&b32_storage, b32_data, 32, MAX_32BYTE_BLOCKS);
+    memarray_init(&b64_storage, b64_data, 64, MAX_64BYTE_BLOCKS);
+    memarray_init(&b192_storage, b192_data, 192, MAX_192BYTE_BLOCKS);
+    memarray_init(&b512_storage, b512_data, 512, MAX_512BYTE_BLOCKS);
+}
+
+void *lwm2m_malloc(size_t s)

What's the justification for using this sort of home-brewed pooled allocator?
Why not just use malloc? Are the blocks requested by waakama always a power of two?

> +    char *uri;
+    char uri_buf[URI_LENGTH + 1];
+
+    memset(uri_buf, 0, sizeof(uri_buf));
+    DEBUG("Creating connection\n");
+    /* get the server URI from the requested instance */
+    uri = _get_uri_from_security_obj(client_data->obj_security, instance_id,
+                                     uri_buf, sizeof(uri_buf) - 1);
+
+    if (!uri) {
+        DEBUG("[_connection_create] Could not get URI of instance\n");
+        goto out;
+    }
+
+    /* parse the URI in the form "coaps://[host]:port" */
+    if (!strncmp(uri, SCHEME_COAPS, strlen(SCHEME_COAPS))) {

You know SCHEME_COAPS is null terminated, so there is no point in using `strlen(SCHEME_COAPS)` as a limit. What you should use here is the size of the uri buffer (or, better, the number of bytes actually loaded in the buffer).

> +    DEBUG("Creating connection\n");
+    /* get the server URI from the requested instance */
+    uri = _get_uri_from_security_obj(client_data->obj_security, instance_id,
+                                     uri_buf, sizeof(uri_buf) - 1);
+
+    if (!uri) {
+        DEBUG("[_connection_create] Could not get URI of instance\n");
+        goto out;
+    }
+
+    /* parse the URI in the form "coaps://[host]:port" */
+    if (!strncmp(uri, SCHEME_COAPS, strlen(SCHEME_COAPS))) {
+        host = uri + strlen(SCHEME_COAPS);
+        default_port = LWM2M_DTLS_PORT;
+    }
+    else if (!strncmp(uri, SCHEME_COAP, strlen(SCHEME_COAP))) {

Same comment as above.

> +
+static int _connection_send(lwm2m_client_connection_t *conn, uint8_t *buffer,
+                            size_t buffer_size,
+                            lwm2m_client_data_t *client_data)
+{
+    ssize_t sent_bytes = sock_udp_send(&(client_data->sock), buffer,
+                                       buffer_size, &(conn->remote));
+    if (sent_bytes <= 0) {
+        DEBUG("[_connection_send] Could not send UDP packet: %d\n", sent_bytes);
+        return -1;
+    }
+    conn->last_send = lwm2m_gettime();
+    return 0;
+}
+
+static lwm2m_client_connection_t *_connection_create(int instance_id,

Do we really need to roll out a URI parser from scratch specifically for this module?

> @@ -0,0 +1,4 @@
+USEMODULE += wakaama_contrib

You are using xtimer and gettimeofday(). Add the dependency.

> +            case LWM2M_RES_BINDINGS:
+                lwm2m_data_encode_string(LWM2M_DEVICE_BINDINGS, *data_arrayP + i);
+                result = COAP_205_CONTENT;
+                break;
+            case LWM2M_RES_TYPE:
+                lwm2m_data_encode_string(LWM2M_DEVICE_TYPE, *data_arrayP + i);
+                result = COAP_205_CONTENT;
+                break;
+            /* Error code is mandatory */
+            case LWM2M_RES_ERROR_CODE:
+                /* TODO: Do this properly */
+                lwm2m_data_encode_int(0, *data_arrayP + i);
+                result = COAP_205_CONTENT;
+                break;
+            /* Time resources */
+            case LWM2M_RES_TIME:

Why not delete all these cases and leave the default?

> +                                lwm2m_object_t *objectP)
+{
+    int i;
+    uint8_t result = COAP_404_NOT_FOUND;
+    dev_data_t *data = (dev_data_t *)objectP->userData;
+
+    (void)data;
+
+    (void)instance_id;
+    (void)num_data;
+    (void)data_array;
+
+    for (i = 0; i < num_data; i++) {
+        switch (data_array[i].id) {
+            /* Writable */
+            case LWM2M_RES_TIME:

Same thing here, is there a more sensible way to write this that does not involve listing everything? Is it possible to just check a range?

> +        *num_dataP = cnt;
+        for (i = 0; i < cnt; i++) {
+            (*data_arrayP)[i].id = resList[i];
+        }
+    }
+
+    for (i = 0; i < *num_dataP; i++) {
+        switch ((*data_arrayP)[i].id) {
+            /* Exec resources */
+            case LWM2M_RES_REBOOT:
+            case LWM2M_RES_FRESET:
+            case LWM2M_RES_ERROR_CODE_RESET:
+                result = COAP_405_METHOD_NOT_ALLOWED;
+                break;
+            /* Statically defined resources */
+            case LWM2M_RES_MANUFACTURER:

Can't this be done with a string table lookup instead of code?

> +    DEBUG("cnt512: %"PRIu16"/%u max: %"PRIu16"\n", cnt512, MAX_512BYTE_BLOCKS,
+          cnt512_max);
+#endif /* DEBUG_MALLOC */
+    return;
+}
+
+void lwm2m_memarray_init(void)
+{
+    memarray_init(&b16_storage, b16_data, 16, MAX_16BYTE_BLOCKS);
+    memarray_init(&b32_storage, b32_data, 32, MAX_32BYTE_BLOCKS);
+    memarray_init(&b64_storage, b64_data, 64, MAX_64BYTE_BLOCKS);
+    memarray_init(&b192_storage, b192_data, 192, MAX_192BYTE_BLOCKS);
+    memarray_init(&b512_storage, b512_data, 512, MAX_512BYTE_BLOCKS);
+}
+
+void *lwm2m_malloc(size_t s)

If you have reasons to use a dedicated heap instead of the global system heap, you can use TLSF.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/RIOT-OS/RIOT/pull/11036#pullrequestreview-245340894
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190604/85958511/attachment.html>


More information about the notifications mailing list