From 9eaa959a633afb6672b099a8a6b3e1505720c8da Mon Sep 17 00:00:00 2001 From: chrysn Date: Mon, 29 Jun 2020 17:47:19 +0200 Subject: [PATCH] gcoap: Suppress retransmissions when ACK was received This introduces an additional state to the COAP_MEMO_* series to avoid enlarging the memo struct needlessly. While they are documented publicly, practically only the COAP_MEMO_TIMEOUT and COAP_MEMO_RESPONSE are used in communication with the application, as a gcoap_request_memo_t is only handed out in that state. --- sys/include/net/gcoap.h | 9 +++++---- sys/net/application_layer/gcoap/gcoap.c | 27 ++++++++++++++++++++++--- 2 files changed, 29 insertions(+), 7 deletions(-) diff --git a/sys/include/net/gcoap.h b/sys/include/net/gcoap.h index f0d550775f4c1..7b87cef560924 100644 --- a/sys/include/net/gcoap.h +++ b/sys/include/net/gcoap.h @@ -459,10 +459,11 @@ extern "C" { * @{ */ #define GCOAP_MEMO_UNUSED (0) /**< This memo is unused */ -#define GCOAP_MEMO_WAIT (1) /**< Request sent; awaiting response */ -#define GCOAP_MEMO_RESP (2) /**< Got response */ -#define GCOAP_MEMO_TIMEOUT (3) /**< Timeout waiting for response */ -#define GCOAP_MEMO_ERR (4) /**< Error processing response packet */ +#define GCOAP_MEMO_RETRANSMIT (1) /**< Request sent, retransmitting until response arrives */ +#define GCOAP_MEMO_WAIT (2) /**< Request sent; awaiting response */ +#define GCOAP_MEMO_RESP (3) /**< Got response */ +#define GCOAP_MEMO_TIMEOUT (4) /**< Timeout waiting for response */ +#define GCOAP_MEMO_ERR (5) /**< Error processing response packet */ /** @} */ /** diff --git a/sys/net/application_layer/gcoap/gcoap.c b/sys/net/application_layer/gcoap/gcoap.c index 18eb2d0095712..5f52f629990c6 100644 --- a/sys/net/application_layer/gcoap/gcoap.c +++ b/sys/net/application_layer/gcoap/gcoap.c @@ -279,6 +279,12 @@ static void _on_resp_timeout(void *arg) { #endif event_timeout_set(&memo->resp_evt_tmout, timeout); + if (memo->state == GCOAP_MEMO_WAIT) { + /* See _cease_retransmission: Still going through the timeouts and + * rescheduling, but not actually sending any more */ + return; + } + ssize_t bytes = sock_udp_send(&_sock, memo->msg.data.pdu_buf, memo->msg.data.pdu_len, &memo->remote_ep); if (bytes <= 0) { @@ -292,12 +298,26 @@ static void _on_resp_timeout(void *arg) { * * This is used in response to an empty ACK. * + * The current implementation does not touch the timers, but merely sets the + * memo's state to GCOAP_MEMO_WAIT. This approach needs less complex code at + * the cost of the remaining `send_limit` timers firing and some memory not + * being freed until the actual response arrives. + * + * An alternative implementation would stop the timeouts, and either free the + * whole memo if it has no response handler, or calculate the remaining timeout + * from `send_limit` to set a final timeout then. In that case, it might also + * free the gcoap_resend_t data and move it back into hdr_buf (along with a + * change in the discriminator for that). (That's not an option with the + * current design because the discriminator is the send_limit field, which is + * still used to count down). + * * @param[inout] memo The memo indicating the pending request * - * @pre The @p memo is active, and its send_limit is not GCOAP_SEND_LIMIT_NON. + * @pre The @p memo is GCOAP_MEMO_RETRANSMIT or GCOAP_MEMO_WAIT, and its + * send_limit is not GCOAP_SEND_LIMIT_NON. */ static void _cease_retransmission(gcoap_request_memo_t *memo) { - /* FIXME This needs to do something, although not doing anything is not terrible. */ + memo->state = GCOAP_MEMO_WAIT; } /* @@ -520,7 +540,7 @@ static void _find_req_memo(gcoap_request_memo_t **memo_ptr, coap_pkt_t *src_pdu, static void _expire_request(gcoap_request_memo_t *memo) { DEBUG("coap: received timeout message\n"); - if (memo->state == GCOAP_MEMO_WAIT) { + if ((memo->state == GCOAP_MEMO_RETRANSMIT) || (memo->state == GCOAP_MEMO_WAIT)) { memo->state = GCOAP_MEMO_TIMEOUT; /* Pass response to handler */ if (memo->resp_handler) { @@ -775,6 +795,7 @@ size_t gcoap_req_send(const uint8_t *buf, size_t len, #if CONFIG_COAP_RANDOM_FACTOR_1000 > 1000 timeout = random_uint32_range(timeout, TIMEOUT_RANGE_END * US_PER_SEC); #endif + memo->state = GCOAP_MEMO_RETRANSMIT; } else { memo->state = GCOAP_MEMO_UNUSED;