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

José Alamos notifications at github.com
Mon Jul 12 15:56:27 CEST 2021


@jia200x requested changes on this pull request.



> @@ -154,6 +155,12 @@ void gnrc_lorawan_mcps_process_downlink(gnrc_lorawan_t *mac, uint8_t *psdu,
         return;
     }
 
+    /* Check if downlink was received after an uplink that had `ADRACKReq` bit set */
+    if (mac->mlme.adr_ack_cnt >= CONFIG_LORAMAC_DEFAULT_ADR_ACK_LIMIT) {

The ADR ACK counter is reset always if a downlink is received, according to the standard (page 17)

> @@ -251,6 +258,15 @@ size_t gnrc_lorawan_build_uplink(gnrc_lorawan_t *mac, iolist_t *payload,
     lw_hdr->addr = mac->dev_addr;
     lw_hdr->fctrl = 0;
 
+    lorawan_hdr_set_adr(lw_hdr,mac->mlme.adr);
+
+    /* Set `ADRACKReq` bit and ADR_ACK_CNT */
+    if (mac->mlme.adr) {
+        lorawan_hdr_set_adr_ack_req(lw_hdr,

This snippet doesn't reflect what the standard specified.

>From the standard:
```
In   uplink   transmissions   the ADRACKReqbit   is   set   if ADR_ACK_CNT  >=  ADR_ACK_LIMIT and  the  current  data-rate  is  greater than the  device  defined minimum data rate, it is cleared in other conditions
```

The bit shouldn't be set if the DR cannot be increased.

> +                                    mac->mlme.adr_ack_cnt >= CONFIG_LORAMAC_DEFAULT_ADR_ACK_LIMIT ?
+                                    true : false);

```suggestion
                                    mac->mlme.adr_ack_cnt >= CONFIG_LORAMAC_DEFAULT_ADR_ACK_LIMIT);
```

`>=` already resolves to boolean

> +    if (mac->mlme.adr_ack_cnt  > (CONFIG_LORAMAC_DEFAULT_ADR_ACK_LIMIT +
+         CONFIG_LORAMAC_DEFAULT_ADR_ACK_DELAY)) {
+             if (mac->last_dr) {
+                DEBUG("gnrc_lorawan_mcps: ADRACKReq: Decrement DR\n");
+                mac->last_dr--;
+                mac->mlme.adr_ack_cnt = CONFIG_LORAMAC_DEFAULT_ADR_ACK_LIMIT;
+             }
+    }

This should work, but we shouldn't reset the counter to `CONFIG_LORAMAC_DEFAULT_ADR_ACK_LIMIT`.
The counter should keep increasing.

You can use what was proposed in This was already proposed in https://github.com/RIOT-OS/RIOT/pull/15995#discussion_r583743697

We should enforce these CONFIGs to be power of 2. Then, the code can be written with bit operations.

You could define conditions that you should reuse in other parts of the code and reflect the standard with those.

> +           p[1] >> 4, p[1] & 0x0f);
+    DEBUG("gnrc_lorawan_mlme: LinkADRReq ChMask : %u\n", (p[3] << 8 ) | p[2]);
+    DEBUG("gnrc_lorawan_mlme: LinkADRReq Redundancy : %u\n", p[4]);
+
+    mac->last_dr = p[1] >> 4;
+    mac->channel_mask = (p[3] << 8 ) | p[2];
+    // mac->mcps.redundancy = p[4];
+
+    /* Set `LinkADRAns` for next uplink */
+    mac->mlme.pending_mlme_opts |=  GNRC_LORAWAN_MLME_OPTS_LINK_ADR_ANS;
+
+    DEBUG("gnrc_lorawan_mlme: Lastdr : %u\n",mac->last_dr); // to be removed
+    DEBUG("gnrc_lorawan_mlme: Channel Mask : %u\n",mac->channel_mask); // to be removed
+
+    /* Reset `ADR_ACK_CNT` counter */
+    mac->mlme.adr_ack_cnt = 0;

As stated before, this must be reset on every downlink. Therefore this shouldn't be here

> @@ -230,6 +230,7 @@ static void _reset(gnrc_netif_t *netif)
 {
     netif->lorawan.otaa = CONFIG_LORAMAC_DEFAULT_JOIN_PROCEDURE ==
                           LORAMAC_JOIN_OTAA ? NETOPT_ENABLE : NETOPT_DISABLE;
+    netif->lorawan.adr = IS_ACTIVE(CONFIG_LORAMAC_DEFAULT_ADR);

as discussed, `gnrc_netif_lorawan_t` shouldn't have ADR members

>      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;
 }
 
+static void _mlme_link_adr_req(gnrc_lorawan_t *mac, uint8_t *p)
+{
+    DEBUG("gnrc_lorawan_mlme: LinkADRReq DataRate_TXPower : DR%u TX%u\n",
+           p[1] >> 4, p[1] & 0x0f);
+    DEBUG("gnrc_lorawan_mlme: LinkADRReq ChMask : %u\n", (p[3] << 8 ) | p[2]);
+    DEBUG("gnrc_lorawan_mlme: LinkADRReq Redundancy : %u\n", p[4]);
+
+    mac->last_dr = p[1] >> 4;
+    mac->channel_mask = (p[3] << 8 ) | p[2];

where's the code that handles multiple LinkADRReq commands, as specified in the standard? (page 25)

>      mac->mcps.ack_requested = false;
 
     mac->mcps.nb_trials = CONFIG_LORAMAC_DEFAULT_RETX;
 
     mac->mcps.msdu = pkt;
-    mac->last_dr = mcps_request->data.dr;
+
+    DEBUG("LAST DR%d \n",mac->last_dr); //to be removed

ping

> +    DEBUG("gnrc_lorawan_mlme: LinkADRReq DataRate_TXPower : DR%u TX%u\n",
+           mlme_link_adr_req.dr_txpwr >> 4, mlme_link_adr_req.dr_txpwr & 0x0f);
+    DEBUG("gnrc_lorawan_mlme: LinkADRReq ChMask : %u\n",mlme_link_adr_req.chmsk);
+    DEBUG("gnrc_lorawan_mlme: LinkADRReq Redundancy : %u\n",mlme_link_adr_req.redncy);
+
+    mac->mlme.pending_mlme_opts |=  GNRC_LORAWAN_MLME_OPTS_LINK_ADR_ANS;
+    mac->last_dr = mlme_link_adr_req.dr_txpwr >> 4;
+    DEBUG("gnrc_lorawan_mlme: Lastdr : %u\n",mac->last_dr); // to be removed
+}
+
+int _fopts_mlme_link_adr_ans(lorawan_buffer_t *buf)
+{
+    if (buf) {
+        assert(buf->index + GNRC_LORAWAN_CID_SIZE <= buf->size);
+        buf->data[buf->index++] = GNRC_LORAWAN_CID_LINK_ADR_ANS;
+        buf->data[buf->index++] = 0x7;

ping

> +    DEBUG("gnrc_lorawan_mlme: LinkADRReq Redundancy : %u\n",mlme_link_adr_req.redncy);
+
+    mac->mlme.pending_mlme_opts |=  GNRC_LORAWAN_MLME_OPTS_LINK_ADR_ANS;
+    mac->last_dr = mlme_link_adr_req.dr_txpwr >> 4;
+    DEBUG("gnrc_lorawan_mlme: Lastdr : %u\n",mac->last_dr); // to be removed
+}
+
+int _fopts_mlme_link_adr_ans(lorawan_buffer_t *buf)
+{
+    if (buf) {
+        assert(buf->index + GNRC_LORAWAN_CID_SIZE <= buf->size);
+        buf->data[buf->index++] = GNRC_LORAWAN_CID_LINK_ADR_ANS;
+        buf->data[buf->index++] = 0x7;
+    }
+
+    return GNRC_LORAWAN_CID_SIZE + 1;

ping

> @@ -345,6 +346,10 @@ static int _set(gnrc_netif_t *netif, const gnrc_netapi_opt_t *opt)
 
     gnrc_netif_acquire(netif);
     switch (opt->opt) {
+        case NETOPT_LORAWAN_ADR:
+            assert(opt->data_len == sizeof(netopt_enable_t));
+            netif->lorawan.adr = *((netopt_enable_t *)opt->data);

also, the adr_ack_counter should be reset when ADR is turned on.

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


More information about the notifications mailing list