From 896b9f39186b21608e2a08a543f2d16613bcbcbf Mon Sep 17 00:00:00 2001 From: Benjamin Valentin Date: Thu, 14 Apr 2022 15:10:55 +0200 Subject: [PATCH] MERGECOMMIT: nanocoap: follow-up fixes #17950 --- sys/net/application_layer/nanocoap/nanocoap.c | 3 +- sys/net/application_layer/nanocoap/sock.c | 111 +++++++++--------- 2 files changed, 59 insertions(+), 55 deletions(-) diff --git a/sys/net/application_layer/nanocoap/nanocoap.c b/sys/net/application_layer/nanocoap/nanocoap.c index bc04d68bb94fc..290e91bef3708 100644 --- a/sys/net/application_layer/nanocoap/nanocoap.c +++ b/sys/net/application_layer/nanocoap/nanocoap.c @@ -812,13 +812,12 @@ size_t coap_opt_put_uri_pathquery(uint8_t *buf, uint16_t *lastonum, const char * size_t bytes_out = coap_opt_put_string_with_len(buf, *lastonum, COAP_OPT_URI_PATH, uri, len, '/'); - if (query) { buf += bytes_out; bytes_out += coap_opt_put_uri_query(buf, COAP_OPT_URI_PATH, query + 1); *lastonum = COAP_OPT_URI_QUERY; } - else { + else if (bytes_out) { *lastonum = COAP_OPT_URI_PATH; } diff --git a/sys/net/application_layer/nanocoap/sock.c b/sys/net/application_layer/nanocoap/sock.c index da72be37f434b..9343a293112e5 100644 --- a/sys/net/application_layer/nanocoap/sock.c +++ b/sys/net/application_layer/nanocoap/sock.c @@ -25,9 +25,11 @@ #include #include +#include "atomic_utils.h" #include "net/nanocoap_sock.h" #include "net/sock/util.h" #include "net/sock/udp.h" +#include "random.h" #include "sys/uio.h" #include "timex.h" @@ -45,6 +47,13 @@ typedef struct { bool more; } _block_ctx_t; +static uint16_t _get_id(void) +{ + __attribute__((section(".noinit"))) + static uint16_t id; + return atomic_fetch_add_u16(&id, 1); +} + int nanocoap_sock_connect(nanocoap_sock_t *sock, sock_udp_ep_t *local, sock_udp_ep_t *remote) { if (!remote->port) { @@ -81,53 +90,63 @@ 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; unsigned state = STATE_SEND_REQUEST; - /* TODO: timeout random between between ACK_TIMEOUT and (ACK_TIMEOUT * ACK_RANDOM_FACTOR) */ - uint32_t timeout = CONFIG_COAP_ACK_TIMEOUT_MS * US_PER_MS; - + uint32_t timeout = random_uint32_range(CONFIG_COAP_ACK_TIMEOUT_MS * US_PER_MS, + CONFIG_COAP_ACK_TIMEOUT_MS * CONFIG_COAP_RANDOM_FACTOR_1000); /* 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: + DEBUG("nanocoap: waiting for response (timeout: %"PRIu32" µs)\n", timeout); tmp = sock_udp_recv_buf(sock, &payload, &ctx, timeout, NULL); if (tmp == 0) { /* no more data */ + if (retry_rx) { + retry_rx = false; + continue; + } return res; } res = tmp; if (res == -ETIMEDOUT) { DEBUG("nanocoap: timeout\n"); timeout *= 2; - tries_left--; state = STATE_SEND_REQUEST; continue; } @@ -139,13 +158,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; + 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? */ @@ -159,7 +181,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; } @@ -221,31 +243,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, 1); + 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, @@ -294,38 +308,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 > 0; 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, - num); - pktpos += coap_opt_put_uri_pathquery(pktpos, &lastonum, path); - pktpos += coap_opt_put_uint(pktpos, lastonum, COAP_OPT_BLOCK2, - (num << 4) | blksize); + 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); - pkt->payload = pktpos; - pkt->payload_len = 0; + (void)len; + assert((uintptr_t)buf - (uintptr_t)pkt.hdr < len); - int res = nanocoap_sock_request_cb(sock, pkt, _block_cb, ctx); - if (res < 0) { - return res; - } - - 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, @@ -342,16 +351,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;