gcoap: Send separate response #14395

chrysn · 2020-06-30T08:33:13Z

Contribution description

This is currently a first sketch to get separate responses working, and will be evolved along the outline of #14393 later on.

Right now it is just application code (doing things manually), and while most of the following work will be finding suitable helpers to make that cleaner, the two wrong parts here are:

The handler signature is abused to feed the request address to the handler through the data pointer.
The generated message ID is 4, while it should really be counted through.

Testing procedure

Run the gcoap example as a server. From a client, GET the /riot/board+block and /riot/board+nonblock resources. (For now, running this more than two times in a row will fill up the retransmission buffers; that can be mitigated by enhancements to #14178 later.)

From a second client, GET /riot/board while one of the long-running requests is pending and observe that while the nonblocking is being requested, that new request can still be served. Request nonblocking in the same situation to see that this application limits the concurrent blocking requests to 1 with a 5.03 response.

Issues/PRs references

Requires: #14178
Closes: #14393

This generalizes the existing code for answering CoAP pings into general message-layer responses. Such responses are now also sent as a reaction to CON responses, which can otherwise follow the same code path as existing other responses. As a side effect, issues that would crop up when responding to odd empty requests that have token length set are resolved. Contributes-To: RIOT-OS#14169

Simplify the code path and give consistent debug messages.

The actual implementation will follow in a separate commit, this does the groundwork and sets the intention.

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.

cgundogan

Just a few minor remarks, since this PR is still in draft state. Do you envision the current design choice (separate responses on application level) to be merged, or would you work on a tighter integration with gcoap?

cgundogan · 2020-07-07T14:07:45Z

examples/gcoap/gcoap_cli.c

+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);
+    }
+}


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?

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

cgundogan · 2020-07-07T14:13:11Z

examples/gcoap/gcoap_cli.c

+    */
+    pdu->hdr = (coap_hdr_t *)buf;
+
+    /* generate token */


Suggested change

/* generate token */

/* generate message id */

?

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

cgundogan · 2020-07-07T14:14:20Z

examples/gcoap/gcoap_cli.c

+    pdu->hdr = (coap_hdr_t *)buf;
+
+    /* generate token */
+    uint16_t msgid = 4; /* FIXME determined by random dice roll as we can't access gcoap internals */


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.

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.

examples/gcoap/gcoap_cli.c

cgundogan · 2020-07-07T14:30:38Z

examples/gcoap/gcoap_cli.c

+        puts("gcoap_cli: msg buffer too small");
+        pdu->hdr->code = COAP_CODE_INTERNAL_SERVER_ERROR;
+        /* unwinding the TEXXT option*/
+        resp_len = resp_len - 1;


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); ... */

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.

chrysn · 2020-10-14T15:38:59Z

I think it's best if that stays an application choice. Those applications that take longer (than, say, a few milliseconds) to prepare their response need to hand off request processing to their own thread anyway, lest they block gcoap and thus any coap processing completely. (This is different from the large computer situation where the coap server would run on threads or async tasks, and messages would be processed concurrently with the handler). This is not ideal, but a consequence of how gcoap handles things. (Some automagic would make sense in a completely different CoAP server where requests are processed into IPC messages and handed to a thread that's not the server for processing, but gcoap is not such a server).

Co-authored-by: Cenk Gündoğan <mail+dev@gundogan.net>

stale · 2021-06-02T16:37:12Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

Jnae · 2021-06-08T13:28:48Z

Is this PR still active and if yes, what remains to be done? I would be interested in this feature.

chrysn · 2021-06-08T14:10:09Z

Is this PR still active and if yes, what remains to be done? I would be interested in this feature.

Active yes, not sure off my head what's missing (probably guidance as to how to actually do this as a server when you're not very deep inside the stack anyway) -- sorry, am currently in a completely different project. Do you have a use case that could be used to shape the guidance to users?

Jnae · 2021-06-08T14:53:34Z

Great! I think I don't have a very good use case - a blocking call is fine, only an empty ACK needs to be sent before processing. (Before I saw the PR I had the idea that the ACK could be sent before the actual handler is already called, to take some work from the application and to make sure it is sent as soon as possible).

chrysn · 2021-06-08T19:30:39Z

Yeah, a way for the application to say that this is gonna take a while, to send an empty ack and to block in the handler until a result is ready is probably a useful thing. (It's not the *best* thing, as any other requests coming in during that time do not get the same benefit, but then again it is an improvement over the current situation, and probably good enough for many a case).

stale · 2022-03-02T11:18:40Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

chrysn · 2022-03-02T13:31:05Z

Thanks for the reminder stalebot, I'll see when I can continue here.

stale · 2022-09-21T05:52:00Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

chrysn · 2022-09-21T19:03:05Z

This will need porting to ztimer.

chrysn added 6 commits May 30, 2020 15:56

gcoap: Cleanup around empty responses

9f5b4a7

Simplify the code path and give consistent debug messages.

fixup! gcoap: Process CON responses

32622a9

gcoap: Prepare to cease retransmission

69b5bd6

The actual implementation will follow in a separate commit, this does the groundwork and sets the intention.

[WIP] gcoap separate responses

c4f778f

chrysn requested review from cgundogan, haukepetersen, miri64 and PeterKietzmann as code owners June 30, 2020 08:33

chrysn marked this pull request as draft June 30, 2020 08:33

chrysn self-assigned this Jun 30, 2020

chrysn mentioned this pull request Jul 1, 2020

gcoap: confirmable messaging plan #7192

Open

6 tasks

cgundogan requested changes Jul 7, 2020

View reviewed changes

typo fix

816434a

Co-authored-by: Cenk Gündoğan <mail+dev@gundogan.net>

stale bot added the State: stale State: The issue / PR has no activity for >185 days label Jun 2, 2021

stale bot removed the State: stale State: The issue / PR has no activity for >185 days label Jun 8, 2021

MrKevinWeiss added this to the Release 2021.07 milestone Jun 21, 2021

Jnae mentioned this pull request Jun 22, 2021

sys/net/gcoap: allow seperate response #16578

Closed

MrKevinWeiss removed this from the Release 2021.07 milestone Jul 15, 2021

chrysn mentioned this pull request Aug 13, 2021

net/gcoap: add functions to read and set the next message id #16730

Closed

stale bot added the State: stale State: The issue / PR has no activity for >185 days label Mar 2, 2022

stale bot removed the State: stale State: The issue / PR has no activity for >185 days label Mar 2, 2022

chrysn added State: don't stale State: Tell state-bot to ignore this issue and removed State: don't stale State: Tell state-bot to ignore this issue labels Sep 21, 2022

gcoap: Send separate response #14395

Are you sure you want to change the base?

gcoap: Send separate response #14395

Conversation

chrysn commented Jun 30, 2020

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

cgundogan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrysn commented Oct 14, 2020

Uh oh!

stale bot commented Jun 2, 2021

Uh oh!

Jnae commented Jun 8, 2021

Uh oh!

chrysn commented Jun 8, 2021 via email

Uh oh!

Jnae commented Jun 8, 2021

Uh oh!

chrysn commented Jun 8, 2021 via email

Uh oh!

stale bot commented Mar 2, 2022

Uh oh!

chrysn commented Mar 2, 2022

Uh oh!

stale bot commented Sep 21, 2022

Uh oh!

chrysn commented Sep 21, 2022

Uh oh!

Uh oh!