nanocoap: follow-up fixes

sys/Makefile.dep

@@ -658,6 +658,7 @@ endif
 ifneq (,$(filter nanocoap_sock,$(USEMODULE)))
   USEMODULE += sock_udp
   USEMODULE += sock_util
+  USEMODULE += ztimer_msec
 endif
 
 ifneq (,$(filter nanocoap_%,$(USEMODULE)))

sys/net/application_layer/nanocoap/sock.c

@@ -32,6 +32,7 @@
 #include "random.h"
 #include "sys/uio.h"
 #include "timex.h"
+#include "ztimer.h"
 
 #define ENABLE_DEBUG 0
 #include "debug.h"
@@ -80,61 +81,121 @@ static int _send_ack(nanocoap_sock_t *sock, coap_pkt_t *pkt)
     coap_hdr_t ack;
     unsigned tkl = coap_get_token_len(pkt);
 
-    coap_build_hdr(&ack, COAP_TYPE_ACK, pkt->token, tkl,
+    coap_build_hdr(&ack, COAP_TYPE_ACK, coap_get_token(pkt), tkl,
                    COAP_CODE_VALID, ntohs(pkt->hdr->id));
 
     return sock_udp_send(sock, &ack, sizeof(ack), NULL);
 }
 
+static bool _id_or_token_missmatch(const coap_pkt_t *pkt, unsigned id,
+                                   const void *token, size_t token_len)
+{
+    switch (coap_get_type(pkt)) {
+    case COAP_TYPE_RST:
+    case COAP_TYPE_ACK:
+        return coap_get_id(pkt) != id;
+    default:
+        if (coap_get_token_len(pkt) != token_len) {
+            return true;
+        }
+        return memcmp(coap_get_token(pkt), token, token_len);
+    }
+}
+
+static uint32_t _deadline_from_interval(uint32_t interval)
+{
+    return US_PER_MS * ztimer_now(ZTIMER_MSEC) + interval;
+}
+
+static uint32_t _deadline_left_us(uint32_t deadline)
+{
+    uint32_t now = ztimer_now(ZTIMER_MSEC) * US_PER_MS;
+    if (now > deadline) {
+        return 0;
+    }
+
+    return deadline - now;
+}
+
 ssize_t nanocoap_sock_request_cb(nanocoap_sock_t *sock, coap_pkt_t *pkt,
                                  coap_request_cb_t cb, void *arg)
 {
     ssize_t tmp, res = 0;
-    size_t pdu_len = (pkt->payload - (uint8_t *)pkt->hdr) + pkt->payload_len;
-    uint8_t *buf = (uint8_t*)pkt->hdr;
-    uint32_t id = coap_get_id(pkt);
+    const void *pdu = pkt->hdr;
+    const size_t pdu_len = coap_get_total_len(pkt);
+    const unsigned id = coap_get_id(pkt);
     void *payload, *ctx = NULL;
+    const uint8_t *token = coap_get_token(pkt);
+    uint8_t token_len = coap_get_token_len(pkt);
 
     unsigned state = STATE_SEND_REQUEST;
 
     uint32_t timeout = random_uint32_range(CONFIG_COAP_ACK_TIMEOUT_MS * US_PER_MS,
                                            CONFIG_COAP_ACK_TIMEOUT_MS * CONFIG_COAP_RANDOM_FACTOR_1000);
+    uint32_t deadline = _deadline_from_interval(timeout);
+
     /* add 1 for initial transmit */
     unsigned tries_left = CONFIG_COAP_MAX_RETRANSMIT + 1;
 
     /* check if we expect a reply */
     const bool confirmable = coap_get_type(pkt) == COAP_TYPE_CON;
 
+    /* try receiving another packet without re-request */
+    bool retry_rx = false;
+
     while (1) {
         switch (state) {
         case STATE_SEND_REQUEST:
+            DEBUG("nanocoap: send %u bytes (%u tries left)\n", (unsigned)pdu_len, tries_left);
             if (--tries_left == 0) {
                 DEBUG("nanocoap: maximum retries reached\n");
                 return -ETIMEDOUT;
             }
 
-            res = sock_udp_send(sock, buf, pdu_len, NULL);
+            res = sock_udp_send(sock, pdu, pdu_len, NULL);
             if (res <= 0) {
                 DEBUG("nanocoap: error sending coap request, %d\n", (int)res);
                 return res;
             }
 
-            if (confirmable || cb) {
-                state = STATE_AWAIT_RESPONSE;
-            } else {
+            /* no response needed and no response handler given */
+            if (!confirmable && !cb) {
                 return 0;
             }
+
+            /* ctx must have been released at this point */
+            assert(ctx == NULL);
+            state = STATE_AWAIT_RESPONSE;
             /* fall-through */
         case STATE_AWAIT_RESPONSE:
-            tmp = sock_udp_recv_buf(sock, &payload, &ctx, timeout, NULL);
+            DEBUG("nanocoap: waiting for response (timeout: %"PRIu32" µs)\n",
+                  _deadline_left_us(deadline));
+            tmp = sock_udp_recv_buf(sock, &payload, &ctx, _deadline_left_us(deadline), NULL);
+            /* sock_udp_recv_buf() is supposed to return multiple packet fragments
+             * when called multiple times with the same context.
+             * In practise, this is not implemented and it will always return a pointer
+             * to the whole packet on the first call and NULL on the second call, which
+             * releases the packet.
+             * This assertion will trigger should the behavior change in the future.
+             */
+            if (retry_rx) {
+                assert(tmp == 0 && ctx == NULL);
+            }
             if (tmp == 0) {
                 /* no more data */
+                /* sock_udp_recv_buf() needs to be called in a loop until ctx is NULL again
+                 * to release the buffer */
+                if (retry_rx) {
+                    retry_rx = false;
+                    continue;
+                }
                 return res;
             }
             res = tmp;
             if (res == -ETIMEDOUT) {
                 DEBUG("nanocoap: timeout\n");
                 timeout *= 2;
+                deadline = _deadline_from_interval(timeout);
                 state = STATE_SEND_REQUEST;
                 continue;
             }
@@ -146,13 +207,16 @@ ssize_t nanocoap_sock_request_cb(nanocoap_sock_t *sock, coap_pkt_t *pkt,
             /* parse response */
             if (coap_parse(pkt, payload, res) < 0) {
                 DEBUG("nanocoap: error parsing packet\n");
-                res = -EBADMSG;
+                retry_rx = true;
+                continue;
             }
-            else if (coap_get_id(pkt) != id) {
-                res = -EBADMSG;
+            else if (_id_or_token_missmatch(pkt, id, token, token_len)) {
+                DEBUG("nanocoap: ID mismatch %u != %u\n", coap_get_id(pkt), id);
+                retry_rx = true;
                 continue;
             }
 
+            DEBUG("nanocoap: response code=%i\n", coap_get_code(pkt));
             switch (coap_get_type(pkt)) {
             case COAP_TYPE_RST:
                 /* TODO: handle different? */
@@ -166,7 +230,7 @@ ssize_t nanocoap_sock_request_cb(nanocoap_sock_t *sock, coap_pkt_t *pkt,
                 if (cb) {
                     res = cb(arg, pkt);
                 } else {
-                    res = 0;
+                    res = _get_error(pkt);
                 }
                 break;
             }
@@ -228,31 +292,23 @@ static int _get_cb(void *arg, coap_pkt_t *pkt)
 
 ssize_t nanocoap_sock_get(nanocoap_sock_t *sock, const char *path, void *buf, size_t len)
 {
-    ssize_t res;
-    coap_pkt_t pkt;
     uint8_t *pktpos = buf;
+    coap_pkt_t pkt = {
+        .hdr = buf,
+    };
 
     struct iovec ctx = {
         .iov_base = buf,
         .iov_len  = len,
     };
 
-    pkt.hdr = buf;
     pktpos += coap_build_hdr(pkt.hdr, COAP_TYPE_CON, NULL, 0, COAP_METHOD_GET, _get_id());
     pktpos += coap_opt_put_uri_path(pktpos, 0, path);
+
     pkt.payload = pktpos;
     pkt.payload_len = 0;
 
-    res = nanocoap_sock_request_cb(sock, &pkt, _get_cb, &ctx);
-    if (res < 0) {
-        return res;
-    }
-
-    if (coap_get_code(&pkt) != 205) {
-        return -ENOENT;
-    }
-
-    return res;
+    return nanocoap_sock_request_cb(sock, &pkt, _get_cb, &ctx);
 }
 
 ssize_t nanocoap_request(coap_pkt_t *pkt, sock_udp_ep_t *local,
@@ -301,37 +357,33 @@ static int _block_cb(void *arg, coap_pkt_t *pkt)
     /* response was not block-wise */
     if (!coap_get_block2(pkt, &block2)) {
         block2.offset = 0;
-        ctx->more = false;
-    } else {
-        ctx->more = block2.more;
+        block2.more = false;
     }
 
+    ctx->more = block2.more;
     return ctx->callback(ctx->arg, block2.offset, pkt->payload, pkt->payload_len, block2.more);
 }
 
-static int _fetch_block(coap_pkt_t *pkt, sock_udp_t *sock,
+static int _fetch_block(nanocoap_sock_t *sock, uint8_t *buf, size_t len,
                         const char *path, coap_blksize_t blksize, unsigned num,
                         _block_ctx_t *ctx)
 {
-    uint8_t *pktpos = (void *)pkt->hdr;
+    coap_pkt_t pkt = {
+        .hdr = (void *)buf,
+    };
     uint16_t lastonum = 0;
 
-    pktpos += coap_build_hdr(pkt->hdr, COAP_TYPE_CON, NULL, 0, COAP_METHOD_GET, _get_id());
-    pktpos += coap_opt_put_uri_pathquery(pktpos, &lastonum, path);
-    pktpos += coap_opt_put_uint(pktpos, lastonum, COAP_OPT_BLOCK2,
-                                (num << 4) | blksize);
-
-    pkt->payload = pktpos;
-    pkt->payload_len = 0;
+    buf += coap_build_hdr(pkt.hdr, COAP_TYPE_CON, NULL, 0, COAP_METHOD_GET, _get_id());
+    buf += coap_opt_put_uri_pathquery(buf, &lastonum, path);
+    buf += coap_opt_put_uint(buf, lastonum, COAP_OPT_BLOCK2, (num << 4) | blksize);
 
-    int res = nanocoap_sock_request_cb(sock, pkt, _block_cb, ctx);
-    if (res < 0) {
-        return res;
-    }
+    (void)len;
+    assert((uintptr_t)buf - (uintptr_t)pkt.hdr < len);
 
-    DEBUG("code=%i\n", coap_get_code(pkt));
+    pkt.payload = buf;
+    pkt.payload_len = 0;
 
-    return _get_error(pkt);
+    return nanocoap_sock_request_cb(sock, &pkt, _block_cb, ctx);
 }
 
 int nanocoap_sock_get_blockwise(nanocoap_sock_t *sock, const char *path,
@@ -348,16 +400,12 @@ int nanocoap_sock_get_blockwise(nanocoap_sock_t *sock, const char *path,
 
     unsigned num = 0;
     while (ctx.more) {
-        coap_pkt_t pkt = {
-            .hdr = (void *)buf,
-        };
-
         DEBUG("fetching block %u\n", num);
-        int res = _fetch_block(&pkt, sock, path, blksize, num, &ctx);
 
+        int res = _fetch_block(sock, buf, sizeof(buf), path, blksize, num, &ctx);
         if (res) {
             DEBUG("error fetching block %u: %d\n", num, res);
-            return -1;
+            return res;
         }
 
         num += 1;

tests/nanocoap_cli/Makefile

@@ -6,7 +6,12 @@ USEMODULE += netdev_default
 USEMODULE += auto_init_gnrc_netif
 # Specify the mandatory networking modules
 USEMODULE += gnrc_ipv6_default
-USEMODULE += sock_udp
+
+# Optionally include DNS support. This includes resolution of names at an
+# upstream DNS server and the handling of RDNSS options in Router Advertisements
+# to auto-configure that upstream DNS server.
+# USEMODULE += sock_dns              # include DNS client
+# USEMODULE += gnrc_ipv6_nib_dns     # include RDNSS option handling
 
 USEMODULE += nanocoap_sock