Skip to content

gcoap: Send separate response #14395

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
chrysn wants to merge 7 commits into
base: master
Choose a base branch
from
+206 −28
Draft

gcoap: Send separate response #14395

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
148633b
gcoap: Process CON responses
chrysn May 30, 2020
9f5b4a7
gcoap: Cleanup around empty responses
chrysn Jun 2, 2020
32622a9
fixup! gcoap: Process CON responses
chrysn Jun 29, 2020
69b5bd6
gcoap: Prepare to cease retransmission
chrysn Jun 29, 2020
32e51c0
gcoap: Suppress retransmissions when ACK was received
chrysn Jun 29, 2020
c4f778f
[WIP] gcoap separate responses
chrysn Jun 30, 2020
816434a
typo fix
chrysn Oct 14, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
114 changes: 114 additions & 0 deletions examples/gcoap/gcoap_cli.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
#include "net/gcoap.h"
#include "od.h"
#include "fmt.h"
#include "xtimer.h"

#define ENABLE_DEBUG (0)
#include "debug.h"
Expand All @@ -40,11 +41,15 @@ static void _resp_handler(const gcoap_request_memo_t *memo, coap_pkt_t* pdu,
const sock_udp_ep_t *remote);
static ssize_t _stats_handler(coap_pkt_t* pdu, uint8_t *buf, size_t len, void *ctx);
static ssize_t _riot_board_handler(coap_pkt_t* pdu, uint8_t *buf, size_t len, void *ctx);
static ssize_t _riot_board_handler_blocking(coap_pkt_t* pdu, uint8_t *buf, size_t len, void *ctx);
static ssize_t _riot_board_handler_nonblocking(coap_pkt_t* pdu, uint8_t *buf, size_t len, void *ctx);

/* CoAP resources. Must be sorted by path (ASCII order). */
static const coap_resource_t _resources[] = {
{ "/cli/stats", COAP_GET | COAP_PUT, _stats_handler, NULL },
{ "/riot/board", COAP_GET, _riot_board_handler, NULL },
{ "/riot/board+block", COAP_GET, _riot_board_handler_blocking, NULL },
{ "/riot/board+nonblock", COAP_GET, _riot_board_handler_nonblocking, NULL },
};

static const char *_link_params[] = {
Expand Down Expand Up @@ -230,6 +235,115 @@ static ssize_t _riot_board_handler(coap_pkt_t *pdu, uint8_t *buf, size_t len, vo
}
}

static ssize_t _riot_board_handler_blocking(coap_pkt_t *pdu, uint8_t *buf, size_t len, void *ctx)
{
(void)ctx;
gcoap_resp_init(pdu, buf, len, COAP_CODE_CONTENT);
coap_opt_add_format(pdu, COAP_FORMAT_TEXT);
size_t resp_len = coap_opt_finish(pdu, COAP_OPT_FINISH_PAYLOAD);

xtimer_sleep(3);

/* write the RIOT board name in the response buffer */
if (pdu->payload_len >= strlen(RIOT_BOARD)) {
memcpy(pdu->payload, RIOT_BOARD, strlen(RIOT_BOARD));
return resp_len + strlen(RIOT_BOARD);
}
else {
puts("gcoap_cli: msg buffer too small");
return gcoap_response(pdu, buf, len, COAP_CODE_INTERNAL_SERVER_ERROR);
}
}
Comment on lines +238 to +256
Copy link
Copy Markdown
Member

@cgundogan cgundogan Jul 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is almost identical to the handler in https://github.com/RIOT-OS/RIOT/pull/14395/files#diff-1ad46e1aa545fa54a91a5563e03a4ee8R220. The only addition is the sleep call .. can we deduplicate?

Copy link
Copy Markdown
Member Author

@chrysn chrysn Oct 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably, yes -- but this now serves primarily for demonstration during development, we may want to have different examples anyway on the long run.


/* This is the information needed when the operation is known to take some time
* and a separate response is sent unconditionally. See the proxy application
* for an example of how to use a timer to send an empty ACK when the operation
* takes too long. */
#define NONBLOCKING_READY (-1)
static struct nonblocking_info {
/** A value of NONBLOCKING_READY indicates that none of this is initialized */
ssize_t token_length;
uint8_t token[GCOAP_TOKENLEN_MAX];
sock_udp_ep_t remote;
xtimer_t response_timer;
} nonblocking = { .token_length = NONBLOCKING_READY };

static void _riot_board_handler_flush(void *arg)
{
struct nonblocking_info *nonblocking = (struct nonblocking_info *)arg;
uint8_t buf[64];

printf("Timer!\n");

coap_pkt_t pdu_backend;
/* Just to make copy-pasted code easier (and I trust the compiler
* simplifies this again) */
coap_pkt_t *pdu = &pdu_backend;

/* Can't do that as it won't expose the token; copy/pasting instead
gcoap_req_init(&pdu, &buf[0], sizeof(buf), COAP_CODE_CONTENT, NULL);
*/
pdu->hdr = (coap_hdr_t *)buf;

/* generate token */
Copy link
Copy Markdown
Member

@cgundogan cgundogan Jul 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/* generate token */
/* generate message id */

?

Copy link
Copy Markdown
Member Author

@chrysn chrysn Oct 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes (but I'm not pulling it in yet as this is from copy-pasting, so there's another occurrence somewhere...)

uint16_t msgid = 4; /* FIXME determined by random dice roll as we can't access gcoap internals */
Copy link
Copy Markdown
Member

@cgundogan cgundogan Jul 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would we need to expose the message id generation code in gcoap in order to fix this later? Then again, if we move the separate response generation into gcoap (if we decide to), then we do not need this line here anymore.

Copy link
Copy Markdown
Member Author

@chrysn chrysn Oct 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, and that's one of the open items. I don't expect gcoap to handle all of the separate response process as a framework, but we may instead of exposing the msgid-take-and-increment have a "prepare header with a new message ID"-ish function.

ssize_t res;
/* Responding CON because we can -- could also respond with whatever the
* original request came in had we persisted that */
res = coap_build_hdr(pdu->hdr, COAP_TYPE_CON, &nonblocking->token[0],
nonblocking->token_length, COAP_CODE_CONTENT, msgid);

coap_pkt_init(pdu, buf, sizeof(buf), res);

/* end actually gcoap_req_init */
coap_opt_add_format(pdu, COAP_FORMAT_TEXT);
size_t resp_len = coap_opt_finish(pdu, COAP_OPT_FINISH_PAYLOAD);
if (pdu->payload_len >= strlen(RIOT_BOARD)) {
memcpy(pdu->payload, RIOT_BOARD, strlen(RIOT_BOARD));
resp_len = resp_len + strlen(RIOT_BOARD);
} else {
puts("gcoap_cli: msg buffer too small");
pdu->hdr->code = COAP_CODE_INTERNAL_SERVER_ERROR;
/* unwinding the TEXT option*/
resp_len = resp_len - 1;
Copy link
Copy Markdown
Member

@cgundogan cgundogan Jul 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this is not very intuitive .. could you also add something along the lines:

/* ... unwinding the TEXT option added by coap_opt_finish(..., COAP_OPT_FINISH_PAYLOAD); ... */

Copy link
Copy Markdown
Member Author

@chrysn chrysn Oct 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's even wrong; as opt_finish adds one byte payload marker and there's another something to unwind from the coap_opt_add_format, it's gotta be more.

The better thing to do would be to get (extract, or better add a function to expose) the length that coap_opt_finish(pdu, COAP_OPT_FINISH_NO_PAYLOAD) would return at the current stage of writing, and store that to roll back.

}

if (gcoap_req_send(buf, resp_len, &nonblocking->remote, NULL, NULL) == 0) {
/* No (re)transmit slots, better keep occupying this slot and try later
* than leave the client all hanging. Try again soon. */
xtimer_set(&nonblocking->response_timer, 1000000);
return;
}

nonblocking->token_length = NONBLOCKING_READY;
}

static ssize_t _riot_board_handler_nonblocking(coap_pkt_t *pdu, uint8_t *buf, size_t len, void *ctx)
{
// (void)ctx;
/* hacked into gcoap.c */
const sock_udp_ep_t *remote = (sock_udp_ep_t*)ctx;

if (nonblocking.token_length != NONBLOCKING_READY) {
return gcoap_response(pdu, buf, len, COAP_CODE_SERVICE_UNAVAILABLE);
}

nonblocking.token_length = pdu->hdr->ver_t_tkl & 0x0f;
memcpy(&nonblocking.token[0], (uint8_t*)pdu->hdr + sizeof(coap_hdr_t), nonblocking.token_length);
nonblocking.remote = *remote;

coap_hdr_set_type(pdu->hdr, COAP_TYPE_ACK);
pdu->hdr->ver_t_tkl &= 0xf0;
coap_hdr_set_code(pdu->hdr, 0);

nonblocking.response_timer.callback = _riot_board_handler_flush;
nonblocking.response_timer.arg = &nonblocking;
xtimer_set(&nonblocking.response_timer, 3000000);

return sizeof(coap_hdr_t);
}


static bool _parse_endpoint(sock_udp_ep_t *remote,
char *addr_str, char *port_str)
{
Expand Down
9 changes: 5 additions & 4 deletions sys/include/net/gcoap.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -498,10 +498,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 */
/** @} */

/**
Expand Down
111 changes: 87 additions & 24 deletions sys/net/application_layer/gcoap/gcoap.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,18 +41,22 @@
#define GCOAP_RESOURCE_WRONG_METHOD -1
#define GCOAP_RESOURCE_NO_PATH -2

/* Sentinel value indicating that no immediate response is required */
#define NO_IMMEDIATE_REPLY (-1)

/* End of the range to pick a random timeout */
#define TIMEOUT_RANGE_END (CONFIG_COAP_ACK_TIMEOUT * CONFIG_COAP_RANDOM_FACTOR_1000 / 1000)

/* Internal functions */
static void *_event_loop(void *arg);
static void _on_sock_evt(sock_udp_t *sock, sock_async_flags_t type, void *arg);
static ssize_t _well_known_core_handler(coap_pkt_t* pdu, uint8_t *buf, size_t len, void *ctx);
static void _cease_retransmission(gcoap_request_memo_t *memo);
static size_t _handle_req(coap_pkt_t *pdu, uint8_t *buf, size_t len,
sock_udp_ep_t *remote);
static void _expire_request(gcoap_request_memo_t *memo);
static void _find_req_memo(gcoap_request_memo_t **memo_ptr, coap_pkt_t *pdu,
const sock_udp_ep_t *remote);
const sock_udp_ep_t *remote, bool by_mid);
static int _find_resource(coap_pkt_t *pdu, const coap_resource_t **resource_ptr,
gcoap_listener_t **listener_ptr);
static int _find_observer(sock_udp_ep_t **observer, sock_udp_ep_t *remote);
Expand Down Expand Up @@ -133,6 +137,15 @@ static void _on_sock_evt(sock_udp_t *sock, sock_async_flags_t type, void *arg)
coap_pkt_t pdu;
sock_udp_ep_t remote;
gcoap_request_memo_t *memo = NULL;
/* Code paths that necessitate a response on the message layer can set a
* response type here (COAP_TYPE_RST or COAP_TYPE_ACK). If set, at the end
* of the function there will be
* * that value will be put in the code field,
* * token length cleared,
* * code set to EMPTY, and
* * the message is returned with the rest of its header intact.
*/
int8_t messagelayer_emptyresponse_type = NO_IMMEDIATE_REPLY;

(void)arg;
if (type & SOCK_ASYNC_MSG_RECV) {
Expand All @@ -157,18 +170,18 @@ static void _on_sock_evt(sock_udp_t *sock, sock_async_flags_t type, void *arg)
if (coap_get_code_raw(&pdu) == COAP_CODE_EMPTY) {
/* ping request */
if (coap_get_type(&pdu) == COAP_TYPE_CON) {
coap_hdr_set_type(pdu.hdr, COAP_TYPE_RST);

ssize_t bytes = sock_udp_send(sock, _listen_buf,
sizeof(coap_hdr_t), &remote);
if (bytes <= 0) {
DEBUG("gcoap: ping response failed: %d\n", (int)bytes);
messagelayer_emptyresponse_type = COAP_TYPE_RST;
DEBUG("gcoap: Answering empty CON request with RST\n");
} else if (coap_get_type(&pdu) == COAP_TYPE_ACK) {
_find_req_memo(&memo, &pdu, &remote, true);
if (memo != NULL && memo->send_limit != GCOAP_SEND_LIMIT_NON) {
DEBUG("gcoap: empty ACK processed, stopping retransmissions\n");
_cease_retransmission(memo);
} else {
DEBUG("gcoap: empty ACK matches no known CON, ignoring\n");
}
} else if (coap_get_type(&pdu) == COAP_TYPE_NON) {
DEBUG("gcoap: empty NON msg\n");
}
else {
goto empty_as_response;
} else {
DEBUG("gcoap: Ignoring empty non-CON request\n");
}
}
/* normal request */
Expand All @@ -189,17 +202,16 @@ static void _on_sock_evt(sock_udp_t *sock, sock_async_flags_t type, void *arg)
}
break;

empty_as_response:
DEBUG("gcoap: empty ack/reset not handled yet\n");
return;

/* incoming response */
case COAP_CLASS_SUCCESS:
case COAP_CLASS_CLIENT_FAILURE:
case COAP_CLASS_SERVER_FAILURE:
_find_req_memo(&memo, &pdu, &remote);
_find_req_memo(&memo, &pdu, &remote, false);
if (memo) {
switch (coap_get_type(&pdu)) {
case COAP_TYPE_CON:
messagelayer_emptyresponse_type = COAP_TYPE_ACK;
/* fall through */
case COAP_TYPE_NON:
case COAP_TYPE_ACK:
if (memo->resp_evt_tmout.queue) {
Expand All @@ -215,9 +227,6 @@ static void _on_sock_evt(sock_udp_t *sock, sock_async_flags_t type, void *arg)
}
memo->state = GCOAP_MEMO_UNUSED;
break;
case COAP_TYPE_CON:
DEBUG("gcoap: separate CON response not handled yet\n");
break;
default:
DEBUG("gcoap: illegal response type: %u\n", coap_get_type(&pdu));
break;
Expand All @@ -231,6 +240,19 @@ static void _on_sock_evt(sock_udp_t *sock, sock_async_flags_t type, void *arg)
DEBUG("gcoap: illegal code class: %u\n", coap_get_code_class(&pdu));
}
}

if (messagelayer_emptyresponse_type != NO_IMMEDIATE_REPLY) {
coap_hdr_set_type(pdu.hdr, (uint8_t)messagelayer_emptyresponse_type);
coap_hdr_set_code(pdu.hdr, COAP_CODE_EMPTY);
/* FIXME make this a coap_hdr_set_token or set_token_length */
pdu.hdr->ver_t_tkl &= 0xf0;

ssize_t bytes = sock_udp_send(sock, _listen_buf,
sizeof(coap_hdr_t), &remote);
if (bytes <= 0) {
DEBUG("gcoap: empty response failed: %d\n", (int)bytes);
}
}
}

/* Handles response timeout for a request; resend confirmable if needed. */
Expand All @@ -256,6 +278,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) {
Expand All @@ -265,6 +293,32 @@ static void _on_resp_timeout(void *arg) {
}
}

/* Change the retransmission of the memo such that no requests are sent any more.
*
* 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 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 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) {
memo->state = GCOAP_MEMO_WAIT;
}

/*
* Main request handler: generates response PDU in the provided buffer.
*
Expand Down Expand Up @@ -369,7 +423,8 @@ static size_t _handle_req(coap_pkt_t *pdu, uint8_t *buf, size_t len,
return -1;
}

ssize_t pdu_len = resource->handler(pdu, buf, len, resource->context);
// HACK to get the remote into the handler (only works when the context is not really used)
ssize_t pdu_len = resource->handler(pdu, buf, len, (void*)remote);
if (pdu_len < 0) {
pdu_len = gcoap_response(pdu, buf, len,
COAP_CODE_INTERNAL_SERVER_ERROR);
Expand Down Expand Up @@ -440,9 +495,10 @@ static int _find_resource(coap_pkt_t *pdu, const coap_resource_t **resource_ptr,
* memo_ptr[out] -- Registered request memo, or NULL if not found
* src_pdu[in] -- PDU for token to match
* remote[in] -- Remote endpoint to match
* by_mid[in] -- true if matches are to be done based on Message ID, otherwise they are done by token
*/
static void _find_req_memo(gcoap_request_memo_t **memo_ptr, coap_pkt_t *src_pdu,
const sock_udp_ep_t *remote)
const sock_udp_ep_t *remote, bool by_mid)
{
*memo_ptr = NULL;
/* no need to initialize struct; we only care about buffer contents below */
Expand All @@ -463,7 +519,13 @@ static void _find_req_memo(gcoap_request_memo_t **memo_ptr, coap_pkt_t *src_pdu,
memo_pdu->hdr = (coap_hdr_t *) memo->msg.data.pdu_buf;
}

if (coap_get_token_len(memo_pdu) == cmplen) {
if (by_mid) {
if (src_pdu->hdr->id == memo_pdu->hdr->id
&& sock_udp_ep_equal(&memo->remote_ep, remote)) {
*memo_ptr = memo;
break;
}
} else if (coap_get_token_len(memo_pdu) == cmplen) {
memo_pdu->token = coap_hdr_data_ptr(memo_pdu->hdr);
if ((memcmp(src_pdu->token, memo_pdu->token, cmplen) == 0)
&& sock_udp_ep_equal(&memo->remote_ep, remote)) {
Expand All @@ -478,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) {
Expand Down Expand Up @@ -777,6 +839,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;
Expand Down