[riot-notifications] [RIOT-OS/RIOT] gnrc_lorawan : Add ADR support (#15995)

Leandro Lanzieri notifications at github.com
Tue Jul 20 11:37:38 CEST 2021


@leandrolanzieri commented on this pull request.



> + * @return true if TX power is valid
+ * @return false otherwise

```suggestion
 * @retval true if TX power is valid
 * @retval false otherwise
```

>  
 /**
- * @brief   Default adaptive datarate ACK limit (in s)
+ * @brief   Default adaptive datarate ACK limit

Is there a reason to remove the unit of this configuration from the documentation? Also, what is `ADR_ACK_CNT`? It does not seem so clear to me from this sentence.

>  #endif
 
 /**
- * @brief   Default adaptive datarate ACK delay (in s)
+ * @brief   Default adaptive datarate ACK delay

Same question for this

>      mlme_confirm_t mlme_confirm;
 
-    mlme_confirm.link_req.margin = p[1];
-    mlme_confirm.link_req.num_gateways = p[2];
+    assert(p[index] == GNRC_LORAWAN_CID_LINK_CHECK_ANS);

I would also check boundaries using `len`, so we make sure we are not accessing something outside `p` later.

>      return GNRC_LORAWAN_CID_SIZE;
 }
 
-static void _mlme_link_check_ans(gnrc_lorawan_t *mac, uint8_t *p)
+static int _mlme_link_check_ans(gnrc_lorawan_t *mac, uint8_t *p, size_t len, uint8_t index)

I think we can do:
```suggestion
static int _mlme_link_check_ans(gnrc_lorawan_t *mac, const uint8_t *p, size_t len, uint8_t index)
```

>  
     mlme_confirm.type = MLME_LINK_CHECK;
     mlme_confirm.status = GNRC_LORAWAN_REQ_STATUS_SUCCESS;
     gnrc_lorawan_mlme_confirm(mac, &mlme_confirm);
 
     mac->mlme.pending_mlme_opts &= ~GNRC_LORAWAN_MLME_OPTS_LINK_CHECK_REQ;
+
+    return GNRC_LORAWAN_FOPT_LINK_CHECK_ANS_SIZE;
+}
+
+static int _mlme_link_adr_req(gnrc_lorawan_t *mac, uint8_t *p, size_t len, uint8_t index)
+{
+    int consumed_bytes = 0;

Also here we can check boundaries and make `p` `const uint8_t *` I believe.

>  
     mlme_confirm.type = MLME_LINK_CHECK;
     mlme_confirm.status = GNRC_LORAWAN_REQ_STATUS_SUCCESS;
     gnrc_lorawan_mlme_confirm(mac, &mlme_confirm);
 
     mac->mlme.pending_mlme_opts &= ~GNRC_LORAWAN_MLME_OPTS_LINK_CHECK_REQ;
+
+    return GNRC_LORAWAN_FOPT_LINK_CHECK_ANS_SIZE;
+}
+
+static int _mlme_link_adr_req(gnrc_lorawan_t *mac, uint8_t *p, size_t len, uint8_t index)
+{
+    int consumed_bytes = 0;
+    assert(p[index] == GNRC_LORAWAN_CID_LINK_ADR_REQ);
+
+    /* Check whether this is the first block. */
+    if (mac->mlme.adr_flags & 0x8) {
+        /* Indicate error */
+        return 0;

Is it correct that `0` indicates an error?

> +
+static int _mlme_link_adr_req(gnrc_lorawan_t *mac, uint8_t *p, size_t len, uint8_t index)
+{
+    int consumed_bytes = 0;
+    assert(p[index] == GNRC_LORAWAN_CID_LINK_ADR_REQ);
+
+    /* Check whether this is the first block. */
+    if (mac->mlme.adr_flags & 0x8) {
+        /* Indicate error */
+        return 0;
+    }
+
+    /* Mark the first contiguous block */
+    mac->mlme.adr_flags |= 0x8;
+    uint8_t c = index;
+    while (c < len) {

Probably this could be a `for`.

> +    uint8_t nbtrans = p[req_index + 4] & 0xF;
+
+    int status = 0;
+    gnrc_lorawan_set_uncnf_redundancy(mac, ((nbtrans != 0) ? (uint8_t)(nbtrans - 1) \
+                                             : CONFIG_LORAMAC_DEFAULT_REDUNDANCY));
+
+    if (gnrc_lorawan_set_tx_power(mac, tx_power) >= 0) {
+        status |= 0x4;
+    }
+
+    if (gnrc_lorawan_validate_dr(dr) == true) {
+        mac->last_dr = dr;
+        status |= 0x2;
+    }
+
+    if (chmask_ctrl == 6) {

What is this number? 

> +
+    if (gnrc_lorawan_validate_dr(dr) == true) {
+        mac->last_dr = dr;
+        status |= 0x2;
+    }
+
+    if (chmask_ctrl == 6) {
+        channel_mask = 0;
+        for (unsigned i = 0; i < GNRC_LORAWAN_MAX_CHANNELS; i++) {
+            if (mac->channel[i]) {
+                channel_mask |= 1;
+            }
+            channel_mask = channel_mask << 1;
+        }
+        int res = gnrc_lorawan_phy_set_channel_mask(mac, channel_mask);
+        (void) res;

Is this required?

> +        channel_mask = 0;
+        for (unsigned i = 0; i < GNRC_LORAWAN_MAX_CHANNELS; i++) {
+            if (mac->channel[i]) {
+                channel_mask |= 1;
+            }
+            channel_mask = channel_mask << 1;
+        }
+        int res = gnrc_lorawan_phy_set_channel_mask(mac, channel_mask);
+        (void) res;
+        assert(res >= 0);
+        status |= 0x1;
+    }
+    else if (chmask_ctrl == 0) {
+        /* Try to apply channel mask */
+        if (gnrc_lorawan_phy_set_channel_mask(mac, channel_mask) >= 0) {
+            status |= 0x1;

Why is here OK to fail? (i.e. no `assert`)

> +    /* Use the last 3 bytes for the status */
+    mac->mlme.adr_flags |= status;

Maybe also mask it to only use the three bits
```suggestion
    /* Use the last 3 bits for the status */
    mac->mlme.adr_flags |= status;
```

>  
     mac->mcps.waiting_for_ack = waiting_for_ack;
+

ping?

> +void gnrc_lorawan_set_adr(gnrc_lorawan_t *mac, bool adr)
+{
+    if ((adr == true) && (mac->mlme.adr == false)) {
+        mac->mlme.adr_ack_cnt = 0;
+    }
+    mac->mlme.adr = adr;
+}
+

ping?

> @@ -21,6 +21,7 @@
 #include "debug.h"
 
 #define GNRC_LORAWAN_DATARATES_NUMOF (6U)
+#define GNRC_LORAWAN_TX_POWER_NUMOF  (8U)

ping?

> +        channel_mask = 0;
+        for (unsigned i = 0; i < GNRC_LORAWAN_MAX_CHANNELS; i++) {
+            if (mac->channel[i]) {
+                channel_mask |= 1;
+            }
+            channel_mask = channel_mask << 1;
+        }
+        int res = gnrc_lorawan_phy_set_channel_mask(mac, channel_mask);
+        (void) res;
+        assert(res >= 0);
+        status |= 0x1;
+    }
+    else if (chmask_ctrl == 0) {
+        /* Try to apply channel mask */
+        if (gnrc_lorawan_phy_set_channel_mask(mac, channel_mask) >= 0) {
+            status |= 0x1;

If in both cases the channel mask is set, we can deduplicate this code.

-- 
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/15995#pullrequestreview-710245995
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20210720/b7226039/attachment-0001.htm>


More information about the notifications mailing list