[riot-notifications] [RIOT-OS/RIOT] gnrc_lorawan: add initial support for GNRC based LoRaWAN stack (v2) (#11022)

Sebastian Meiling notifications at github.com
Tue May 21 14:59:13 CEST 2019


smlng requested changes on this pull request.

first review round, no testing yet

> +
+#define GNRC_LORAWAN_REQ_STATUS_SUCCESS (0)     /**< MLME or MCPS request successful status */
+#define GNRC_LORAWAN_REQ_STATUS_DEFERRED (1)    /**< the MLME or MCPS confirm message is asynchronous */
+
+/**
+ * @brief MAC Information Base attributes
+ */
+typedef enum {
+    MIB_ACTIVATION_METHOD
+} mlme_mib_type_t;
+
+/**
+ * @brief MAC Information Base descriptor for MLME Request-Confirm
+ */
+typedef struct {
+    mlme_mib_type_t type; /**< MIB attribute identifier */

where do the following struct definitions come from? Because they look a bit inconsistent in style, i.e., some use an `enum` for `type` other generic `uint8_t`. Also the order is different, i.e., `param` after `type` or before. If possible, this should be made consistent.

> +#define GNRC_LORAWAN_REQ_STATUS_DEFERRED (1)    /**< the MLME or MCPS confirm message is asynchronous */
+
+/**
+ * @brief MAC Information Base attributes
+ */
+typedef enum {
+    MIB_ACTIVATION_METHOD
+} mlme_mib_type_t;
+
+/**
+ * @brief MAC Information Base descriptor for MLME Request-Confirm
+ */
+typedef struct {
+    mlme_mib_type_t type; /**< MIB attribute identifier */
+    union {
+        int activation;

also using a named `union` with only a single attribute seams unnecessary. Why not use anonymous `union` in all structs instead?

> +
+/**
+ * @brief Indicate the MAC layer when the transmission finished
+ *
+ * @param[in] mac pointer to the MAC descriptor
+ */
+void gnrc_lorawan_event_tx_complete(gnrc_lorawan_t *mac);
+
+/**
+ * @brief Init GNRC LoRaWAN
+ *
+ * @param[in] mac pointer to the MAC descriptor
+ * @param[in] nwkskey buffer to store the NwkSKey. Should be at least 16 bytes long
+ * @param[in] appskey buffer to store the AppsKey. Should be at least 16 bytes long
+ */
+void gnrc_lorawan_init(gnrc_lorawan_t *mac, uint8_t *nwkskey, uint8_t *appskey);

can (some) parameters be made `const`? Check for all functions please.

> +            if (mac->mcps.nb_trials-- == 0) {
+                _end_of_tx(mac, MCPS_CONFIRMED, -ETIMEDOUT);
+            }
+        }
+        else {
+            _end_of_tx(mac, state, GNRC_LORAWAN_REQ_STATUS_SUCCESS);
+        }
+
+        mac->msg.type = MSG_TYPE_MCPS_ACK_TIMEOUT;
+        if (mac->mcps.outgoing_pkt) {
+            xtimer_set_msg(&mac->rx, 1000000 + random_uint32_range(0, 2000000), &mac->msg, thread_getpid());
+        }
+    }
+}
+
+void gnrc_lorawan_mcps_request(gnrc_lorawan_t *mac, mcps_request_t *mcps_request, mcps_confirm_t *mcps_confirm)

to me it looks like you can omit the `mcps_confirm_t` param and make this function `int` (instead of `void`) and return the status code instead of using `mcps_confirm->status`. Because the only usage of this function is [here](https://github.com/RIOT-OS/RIOT/pull/11022/files#diff-385d2a03ea8a97a25dbb4c48d127beb1R210) where you pass in an empty `mcps_confirm_t` just to use its status variable for return afterwards.

> +    if (!buf || !data || !length) {
+        return -EINVAL;
+    }
+
+    buf->data = data;
+    buf->size = length;
+    buf->index = 0;
+    return 0;
+}
+
+static gnrc_pktsnip_t *_build_join_req_pkt(uint8_t *appeui, uint8_t *deveui, uint8_t *appkey, uint8_t *dev_nonce)
+{
+    gnrc_pktsnip_t *pkt = gnrc_pktbuf_add(NULL, NULL, sizeof(lorawan_join_request_t), GNRC_NETTYPE_UNDEF);
+
+    if (!pkt) {
+        goto out;

avoid `goto` by rewriting into
```
if (pkt) {
<do stuff>
}
return pkt;
```

or 
```
if (!pkt) {
   return NULL;
}
<do stuff>
return pkt;
```

> +    mlme_confirm->param.link_req.margin = fopt->data[fopt->index++];
+    mlme_confirm->param.link_req.num_gateways = fopt->data[fopt->index++];
+
+    mlme_confirm->type = MLME_LINK_CHECK;
+    mlme_confirm->status = GNRC_LORAWAN_REQ_STATUS_SUCCESS;
+    mac->netdev.event_callback(&mac->netdev, NETDEV_EVENT_MLME_CONFIRM);
+
+    mac->mlme.pending_mlme_opts &= ~GNRC_LORAWAN_MLME_OPTS_LINK_CHECK_REQ;
+
+    return 0;
+}
+
+void gnrc_lorawan_process_fopts(gnrc_lorawan_t *mac, uint8_t *fopts, size_t size)
+{
+    if (!fopts || !size) {
+        goto out;

use `return` instead, there nothing done at `out`

> +    mac->mlme.pending_mlme_opts &= ~GNRC_LORAWAN_MLME_OPTS_LINK_CHECK_REQ;
+
+    return 0;
+}
+
+void gnrc_lorawan_process_fopts(gnrc_lorawan_t *mac, uint8_t *fopts, size_t size)
+{
+    if (!fopts || !size) {
+        goto out;
+    }
+
+    lorawan_buffer_t buf;
+    _buffer_reset(&buf, fopts, size);
+
+    while (buf.index < buf.size) {
+        switch (*(buf.data)) {

switch looks like overkill here, `if ... else` looks cleaner (and avoids `goto`):
```
while (buf.index < buf.size) {
    if ((*(buf.data) != GNRC_LORAWAN_CID_LINK_CHECK_REQ_ANS) ||
        (_mlme_link_check_ans(mac, &buf) < 0)) {
        break;
    }
}
```

> +    mac->mlme.pending_mlme_opts &= ~GNRC_LORAWAN_MLME_OPTS_LINK_CHECK_REQ;
+
+    return 0;
+}
+
+void gnrc_lorawan_process_fopts(gnrc_lorawan_t *mac, uint8_t *fopts, size_t size)
+{
+    if (!fopts || !size) {
+        goto out;
+    }
+
+    lorawan_buffer_t buf;
+    _buffer_reset(&buf, fopts, size);
+
+    while (buf.index < buf.size) {
+        switch (*(buf.data)) {

I believe the code above should do the same as your `switch` + `goto`🤞 

> +                    goto out;
+                }
+                break;
+            default:
+                goto out;
+        }
+    }
+out:
+    return;
+}
+
+uint8_t gnrc_lorawan_build_options(gnrc_lorawan_t *mac, lorawan_buffer_t *buf)
+{
+    size_t size = 0;
+
+    size += (mac->mlme.pending_mlme_opts & GNRC_LORAWAN_MLME_OPTS_LINK_CHECK_REQ) ?

Why not simplify to:

```
return (mac->mlme.pending_mlme_opts & GNRC_LORAWAN_MLME_OPTS_LINK_CHECK_REQ) ?
        _fopts_mlme_link_check_req(mac, buf) : 0;
```

> +
+    if (!gnrc_lorawan_validate_dr(datarate)) {
+        return -EINVAL;
+    }
+    uint8_t bw = dr_bw[datarate];
+    uint8_t sf = dr_sf[datarate];
+
+    dev->driver->set(dev, NETOPT_BANDWIDTH, &bw, sizeof(bw));
+    dev->driver->set(dev, NETOPT_SPREADING_FACTOR, &sf, sizeof(sf));
+
+    return 0;
+}
+
+uint8_t gnrc_lorawan_rx1_get_dr_offset(uint8_t dr_up, uint8_t dr_offset)
+{
+    return dr_up > dr_offset ? dr_up - dr_offset : 0;

please use parentheses for readability 
```
return (dr_up > dr_offset) ? (dr_up - dr_offset) : 0;
```

> + * @author  José Ignacio Alamos <jose.alamos at haw-hamburg.de>
+ */
+#ifndef NET_GNRC_LORAWAN_REGION_H
+#define NET_GNRC_LORAWAN_REGION_H
+
+#include "net/gnrc/lorawan.h"
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#define GNRC_LORAWAN_LC_1 (868100000)   /**< First default EU868 channel */
+#define GNRC_LORAWAN_LC_2 (868300000)   /**< Second default EU868 channel */
+#define GNRC_LORAWAN_LC_3 (868500000)   /**< Third default EU868 channel */
+
+#define GNRC_LORAWAN_DEFAULT_CHANNELS 3 /**< Number of default channels for the current region */

this should be named `GNRC_LORAWAN_DEFAULT_CHANNELS_NUMOF` to be consistent with other similar names in RIOT like `I2C_NUMOF` and so on

> + *
+ * @file
+ * @brief   GNRC LoRaWAN region specific functions
+ *
+ * @author  José Ignacio Alamos <jose.alamos at haw-hamburg.de>
+ */
+#ifndef NET_GNRC_LORAWAN_REGION_H
+#define NET_GNRC_LORAWAN_REGION_H
+
+#include "net/gnrc/lorawan.h"
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#define GNRC_LORAWAN_LC_1 (868100000)   /**< First default EU868 channel */

maybe use a array here, which might simplify usage and extension, i.e.:
```
static const uint32_t gnrc_lorawan_default_channels[] = {
    868100000UL,
    868300000UL,
    868500000UL
}
```

> + * @author  José Ignacio Alamos <jose.alamos at haw-hamburg.de>
+ */
+#ifndef NET_GNRC_LORAWAN_REGION_H
+#define NET_GNRC_LORAWAN_REGION_H
+
+#include "net/gnrc/lorawan.h"
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#define GNRC_LORAWAN_LC_1 (868100000)   /**< First default EU868 channel */
+#define GNRC_LORAWAN_LC_2 (868300000)   /**< Second default EU868 channel */
+#define GNRC_LORAWAN_LC_3 (868500000)   /**< Third default EU868 channel */
+
+#define GNRC_LORAWAN_DEFAULT_CHANNELS 3 /**< Number of default channels for the current region */

and using an array as suggested above this will be:
```
#define GNRC_LORAWAN_DEFAULT_CHANNELS_NUMOF (sizeof(gnrc_lorawan_default_channels)/sizeof(uint32_t))
```

> +    int i = 0;
+    uint32_t channel = 0;
+
+    while (n) {
+        if (mac->channel[i]) {
+            n--;
+            channel = mac->channel[i];
+            i++;
+        }
+    }
+    return channel;
+}
+
+void gnrc_lorawan_channels_init(gnrc_lorawan_t *mac)
+{
+    mac->channel[0] = GNRC_LORAWAN_LC_1;

using the array here this can be converted into a loop as well.

> +    int i = 0;
+    uint32_t channel = 0;
+
+    while (n) {
+        if (mac->channel[i]) {
+            n--;
+            channel = mac->channel[i];
+            i++;
+        }
+    }
+    return channel;
+}
+
+void gnrc_lorawan_channels_init(gnrc_lorawan_t *mac)
+{
+    mac->channel[0] = GNRC_LORAWAN_LC_1;

or maybe even put into a single loop like:
```
for (unsigned i = 0; i < GNRC_LORAWAN_MAX_CHANNELS; i++) {
    if (i < GNRC_LORAWAN_DEFAULT_CHANNELS_NUMOF) {
        mac->channel[i] = gnrc_lorawan_default_channels[i];
    }
    else {
        mac->channel[i] = 0;
    }
}
```

which would also avoid an error if `GNRC_LORAWAN_MAX_CHANNELS < GNRC_LORAWAN_DEFAULT_CHANNELS`

> +        case MLME_SET:
+            _mlme_set(mac, mlme_request, mlme_confirm);
+            break;
+        case MLME_GET:
+            _mlme_get(mac, mlme_request, mlme_confirm);
+            break;
+        case MLME_RESET:
+            gnrc_lorawan_reset(mac);
+            mlme_confirm->status = GNRC_LORAWAN_REQ_STATUS_SUCCESS;
+            break;
+        default:
+            break;
+    }
+}
+
+int _fopts_mlme_link_check_req(gnrc_lorawan_t *mac, lorawan_buffer_t *buf)

looks like an internal function, so you can just drop the `mac` param here

> +USEMODULE += od
+USEMODULE += shell
+USEMODULE += shell_commands
+USEMODULE += ps
+USEMODULE += xtimer
+USEMODULE += random
+USEMODULE += gnrc_netdev_default
+USEMODULE += auto_init_gnrc_netif
+USEMODULE += gnrc_lorawan
+USEMODULE += gnrc_pktbuf_cmd
+USEMODULE += gnrc_netapi_callbacks
+USEMODULE += netdev_layer
+USEMODULE += gnrc_neterr
+USEMODULE += gnrc_netreg
+USEMODULE += hashes
+USEMODULE += crypto

I guess some of the modules above should be made dependencies of `gnrc_lorawan` and thus are not required here, i.e. `crypto` is not used in the test anywhere

-- 
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/11022#pullrequestreview-239996644
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190521/583c806f/attachment-0001.html>


More information about the notifications mailing list