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

José Alamos notifications at github.com
Thu Jul 15 03:07:00 CEST 2021


@jia200x requested changes on this pull request.



> @@ -43,6 +43,9 @@ static inline void gnrc_lorawan_mlme_reset(gnrc_lorawan_t *mac)
     mac->mlme.pending_mlme_opts = 0;
     mac->rx_delay = (CONFIG_LORAMAC_DEFAULT_RX1_DELAY / MS_PER_SEC);
     mac->mlme.nid = CONFIG_LORAMAC_DEFAULT_NETID;
+    mac->mlme.adr = IS_ACTIVE(CONFIG_LORAMAC_DEFAULT_ADR) ? true : false;
+    mac->mlme.adr_ack = false;

we don't need to store this, since this can be calculated from the ADR counter. Then we save RAM



> @@ -43,6 +43,9 @@ static inline void gnrc_lorawan_mlme_reset(gnrc_lorawan_t *mac)
     mac->mlme.pending_mlme_opts = 0;
     mac->rx_delay = (CONFIG_LORAMAC_DEFAULT_RX1_DELAY / MS_PER_SEC);
     mac->mlme.nid = CONFIG_LORAMAC_DEFAULT_NETID;
+    mac->mlme.adr = IS_ACTIVE(CONFIG_LORAMAC_DEFAULT_ADR) ? true : false;

```suggestion
    mac->mlme.adr = IS_ACTIVE(CONFIG_LORAMAC_DEFAULT_ADR);
```

> +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;
+}
+

```suggestion
int gnrc_lorawan_set_adr(gnrc_lorawan_t *mac, bool adr)
{
    if (adr == mac->mlme.adr) {
        return -EALREADY;
    }
    
    mac->mlme.adr_ack_cnt = 0;
    mac->mlme.adr = adr;
    return 0;
}
```

Then you can implement the NETOPT to read the output of this function.

> @@ -358,7 +373,14 @@ void gnrc_lorawan_event_no_rx(gnrc_lorawan_t *mac)
         return;
     }
 
-    /* Otherwise check if retransmission should be handled */
+    if (mac->mlme.adr && (mac->last_dr != 0)) {
+        mac->mlme.adr_ack = mac->mlme.adr_ack_cnt >= CONFIG_LORAMAC_DEFAULT_ADR_ACK_LIMIT;

the condition is ok, but as stated before, this shouldn't be stored.

> @@ -358,7 +373,14 @@ void gnrc_lorawan_event_no_rx(gnrc_lorawan_t *mac)
         return;
     }
 
-    /* Otherwise check if retransmission should be handled */
+    if (mac->mlme.adr && (mac->last_dr != 0)) {
+        mac->mlme.adr_ack = mac->mlme.adr_ack_cnt >= CONFIG_LORAMAC_DEFAULT_ADR_ACK_LIMIT;
+        if (mac->mlme.adr_ack && ((mac->mlme.adr_ack - CONFIG_LORAMAC_DEFAULT_ADR_ACK_LIMIT) \

The DR should be decrease for the first time when `mac->mlme.adr_ack >= ADR_ACK_LIMIT + ADR_ACK_DELAY`. The snippet still doesn't reflect that.

> @@ -107,6 +108,7 @@ void gnrc_lorawan_reset(gnrc_lorawan_t *mac)
 
     dev->driver->set(dev, NETOPT_RX_TIMEOUT, &rx_timeout, sizeof(rx_timeout));
 
+    mac->last_dr = CONFIG_LORAMAC_DEFAULT_DR;

actually we shouldn't change the `last_dr` here. This would break e.g `ifconfig 3 set rx2_dr 5`, since it would change the main DR!

-- 
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-706796925
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20210714/e54d8a66/attachment-0001.htm>


More information about the notifications mailing list