net/gcoap: Confirmable request with piggybacked ACK response by kb2ma · Pull Request #7337 · RIOT-OS/RIOT

kb2ma · 2017-07-10T03:58:13Z

This PR implements the third step in the confirmable messaging plan in #7192, and depends on #7336. The goal is to use the extended attributes in the request memo to send a confirmable request, including resends on timeout.

Presently, the PR sends the confirmable request using the extended attributes, including a buffer to store the PDU. The next steps are to validate the received response code and type, and then to resend on timeout.

kb2ma · 2017-08-15T17:11:33Z

No longer WIP. I plan to rebase ASAP.

kb2ma · 2017-08-18T04:17:23Z

Rebased. This PR is a pretty significant milestone for gcoap, and I look forward to getting it merged.

nmeum · 2017-11-03T21:50:26Z

sys/net/application_layer/gcoap/gcoap.c

+            if (!_coap_state.resend_bufs[i*GCOAP_PDU_BUF_SIZE]) {
+                memo->msg.data.pdu_buf = &_coap_state.resend_bufs[i*GCOAP_PDU_BUF_SIZE];
+                memcpy(memo->msg.data.pdu_buf, buf, GCOAP_PDU_BUF_SIZE);
+                memo->msg.data.pdu_len = len;


You forgot a break; here, didn't you?

Because if gcoap found a free receive buffer it doesn't need to look for an additional one, right? This is of cause not a problem when GCOAP_RESEND_BUFS_MAX == 1 (which is the default value) but if you increase that number the code doesn't seem to work properly.

Right on, @nmeum! I just pushed your fix to my repository.

See: RIOT-OS#7337 (comment)

smlng · 2017-12-01T10:19:28Z

please rebase as #7336 is merged

kb2ma · 2017-12-14T13:17:48Z

@smlng, I plan to rebase on #8250. Let me know of any concerns. Thanks.

smlng · 2017-12-18T12:18:15Z

please rebase with #8250 merged right now

kb2ma · 2017-12-20T10:04:52Z

Rebased; apologies for the delay. Thanks to @miri64 and @bergzand for #8268, which allowed me to run my pyexpect tests on a real board.

smlng · 2017-12-20T15:09:58Z

sys/include/net/gcoap.h

                                             observe memos */
    gcoap_observe_memo_t observe_memos[GCOAP_OBS_REGISTRATIONS_MAX];
                                        /**< Observed resource registrations */
+    uint8_t resend_bufs[GCOAP_RESEND_BUFS_MAX * GCOAP_PDU_BUF_SIZE];


why not uint8_t resend_bufs[GCOAP_RESEND_BUFS_MAX][GCOAP_PDU_BUF_SIZE]?

Good idea, makes the intent clearer.

smlng · 2017-12-20T15:12:07Z

sys/net/application_layer/gcoap/gcoap.c

+                /* retries remaining; double timeout and resend */
+                else {
+                    /* decrement send limit, and add 1 to advance the timeout */
+                    unsigned i       = COAP_MAX_RETRANSMIT - memo->send_limit-- + 1;


this looks IMHO a bit ugly, I would put the decrement as a separate instruction.

I agree. With this update, I removed the 'decrement' comment because it's not really necessary.

smlng · 2018-01-10T08:34:16Z

sys/net/application_layer/gcoap/gcoap.c

+                else {
+                    unsigned i       = COAP_MAX_RETRANSMIT - memo->send_limit + 1;
+                    uint32_t timeout = ((uint32_t)COAP_ACK_TIMEOUT << i) * US_PER_SEC;
+                    timeout = random_uint32_range(timeout, timeout * COAP_RANDOM_FACTOR);


mhm, I don't like that, bc COAP_RANDOM_FACTOR is 1.5 thereby introducing floating point maths here. We know that timeout is a multiple of COAP_ACK_TIMEOUT (which is two seconds). Hence, I would rather do something like

#define COAP_TIMEOUT_VARIANCE (500U * US_PER_MS) uint32_t timeout = ((uint32_t)COAP_ACK_TIMEOUT << i) * US_PER_SEC; timeout = random_uint32_range(timeout - COAP_TIMEOUT_VARIANCE, timeout + COAP_TIMEOUT_VARIANCE);

smlng · 2018-01-10T08:37:22Z

sys/net/application_layer/gcoap/gcoap.c

+                }
+                /* retries remaining; double timeout and resend */
+                else {
+                    unsigned i       = COAP_MAX_RETRANSMIT - memo->send_limit + 1;


if you put the decrement, i.e. memo->send_limit--, above this statement you can omit the + 1 at the end of this line.

I know its likely micro-optimisation, and the compiler will do that already. However, IMHO it makes the code also more readable.

Yes, it's fine and clear with the decrement first. I had it in my head to increase timeout first, but it doesn't matter.

smlng · 2018-01-10T08:38:59Z

sys/net/application_layer/gcoap/gcoap.c

+                gcoap_request_memo_t *memo = (gcoap_request_memo_t *)msg_rcvd.content.ptr;
+
+                /* no retries remaining */
+                if (memo->send_limit == GCOAP_SEND_LIMIT_NON


I would recommend to enclose each condition in parentheses to clearly separate them.

Yes, I try to do this with new code now.

smlng · 2018-01-10T08:40:06Z

sys/net/application_layer/gcoap/gcoap.c

+
+    /* validate class and type for incoming */
+    switch (coap_get_code_class(&pdu)) {



remove empty line

smlng · 2018-01-10T08:47:52Z

sys/net/application_layer/gcoap/gcoap.c

-            else {
-                memo->state = GCOAP_MEMO_UNUSED;
-                DEBUG("gcoap: can't wake up mbox; no timeout for msg\n");
+    unsigned msg_type  = (*buf & 0x30) >> 4;


why not use coap_get_type from nanocoap? Though the variable name indicates what you want here, its not clear why this works, the following should work (I hope):

unsigned msg_type = coap_get_type((coap_pkt_t *)buf);

I agree my solution was not clear. Unfortunately, your suggestion generates a segfault. I wonder if the best appoach is to define a second set of inline functions with the prefix 'coap_hdr_get', like

coap_hdr_get_type((coap_hdr_t *)buf);

Leave this for future work? Just define this one function for now?

ah, buf is not a full packet, hence coap_get_type fails, i.e., reads data from where it shouldn't -> segfault.

okay than leave as is (for now).

smlng · 2018-01-10T08:48:53Z

sys/net/application_layer/gcoap/gcoap.c

+        if (memo->msg.data.pdu_buf) {
+            memo->send_limit = COAP_MAX_RETRANSMIT;
+            timeout = (uint32_t)COAP_ACK_TIMEOUT * US_PER_SEC;
+            timeout = random_uint32_range(timeout, timeout * COAP_RANDOM_FACTOR);


same as above, i.e., introduces floating point maths.

smlng · 2018-01-15T09:37:31Z

please squash, I'll test in the mean time, so this can go into release 2018.01 as planned.

kb2ma · 2018-01-15T19:53:55Z

Thanks! I have squashed on my local workstation. I plan to test and upload later today.

smlng · 2018-01-15T19:57:30Z

Looks like you need to rebase (again) to resolve the conflict, too.

Also, create resend_bufs array to store PDU for resend.

kb2ma · 2018-01-16T05:10:02Z

Squashed and rebased.

kb2ma · 2018-01-16T06:45:56Z

The Murdock build failed after I squashed and rebased. It revealed a type issue in a debug statement on MIPS platforms. It turns out that I was expecting a size_t response from sock_udp_send(), but the function really returns an ssize_t. I created a fixup commit so you can review. I can quickly squash this on your approval.

It turns out that there are two other places in gcoap.c that expect sock_udp_send() to return a size_t, in gcoap_req_send2() and in gcoap_obs_send(). I think a fix for the return handling requires a separate PR. Let me know how you'd like to approach it.

How could this code have been wrong for so long. :-/

smlng · 2018-01-16T08:24:33Z

How could this code have been wrong for so long

well, we recently (yesterday) merged #5166 which changed the handling of debug messages, i.e., previously they were guarded by preprocessor #ifs and hence not checked if debug wasn't enabled. Now they are checked every time even if debug is disabled.

smlng · 2018-01-16T08:25:37Z

I think a fix for the return handling requires a separate PR

Yes, your minimal fixup seems to be enough to get this PR through Murdock. Anything on top, should be a separate PR.

smlng · 2018-01-16T08:31:54Z

sys/net/application_layer/gcoap/gcoap.c

+                    uint32_t variance = ((uint32_t)COAP_ACK_VARIANCE << i) * US_PER_SEC;
+                    timeout = random_uint32_range(timeout, timeout + variance);
+
+                    ssize_t res2 = sock_udp_send(&_sock, memo->msg.data.pdu_buf,


maybe bytes or bytes_sent would be a better name than res2? Otherwise, looks good, please squash the fixup.

kb2ma · 2018-01-16T13:43:48Z

Thanks for the review. Fixed variable name and squashed. Travis and Murdock are happy.

smlng

Re-ACK, still I'd like to run some short tests before merge. Nevertheless, this goes into the release, thx @kb2ma for acting quickly 😄

kb2ma · 2018-01-16T14:27:21Z

I really appreciate your feedback! Very happy to uncover the bug with handling sock_udp_send(). I'll make that my next priority.

smlng · 2018-01-16T19:29:02Z

I tested and saw CON and NON request on the wire(shark), looks good to me.

kb2ma mentioned this pull request Jul 10, 2017

net/gcoap: Create confirm request infrastructure and adapt existing messaging #7336

Merged

kb2ma changed the title ~~net/gcoap: WIP for confirmable piggybacked ACK request~~ net/gcoap: WIP for confirmable request with piggybacked ACK response Jul 10, 2017

kb2ma mentioned this pull request Jul 10, 2017

gcoap: confirmable messaging plan #7192

Open

6 tasks

aabadie modified the milestone: Release 2017.10 Jul 12, 2017

kb2ma force-pushed the gcoap/confirm_piggy_client branch from 15e7d3f to 20065c1 Compare July 25, 2017 22:27

kb2ma changed the title ~~net/gcoap: WIP for confirmable request with piggybacked ACK response~~ net/gcoap: Confirmable request with piggybacked ACK response Aug 15, 2017

kb2ma force-pushed the gcoap/confirm_piggy_client branch from fc8cd9f to e46d9c8 Compare August 18, 2017 04:11

kb2ma mentioned this pull request Aug 31, 2017

net/gcoap: Confirmable Observe notifications #7548

Closed

miri64 assigned smlng Sep 19, 2017

smlng added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: network Area: Networking State: waiting for other PR State: The PR requires another PR to be merged first labels Oct 14, 2017

nmeum reviewed Nov 3, 2017

View reviewed changes

nmeum added a commit to nmeum/RIOT that referenced this pull request Nov 3, 2017

Fix for working with GCOAP_RESEND_BUFS_MAX > 1

eb65791

See: RIOT-OS#7337 (comment)

smlng modified the milestones: Release 2017.10, Release 2018.01 Nov 16, 2017

kb2ma mentioned this pull request Nov 24, 2017

gcoap: can't build with network stacks other than GNRC #8130

Closed

smlng removed the State: waiting for other PR State: The PR requires another PR to be merged first label Dec 1, 2017

kb2ma force-pushed the gcoap/confirm_piggy_client branch from 3de19fd to f396227 Compare December 20, 2017 10:02

smlng added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Dec 20, 2017

smlng suggested changes Jan 9, 2018

View reviewed changes

smlng suggested changes Jan 10, 2018

View reviewed changes

smlng approved these changes Jan 15, 2018

View reviewed changes

kb2ma added 3 commits January 15, 2018 23:21

net/gcoap: populate confirm request attributes

4062b1f

Also, create resend_bufs array to store PDU for resend.

net/gcoap: improve validation of incoming message

d5e4da7

net/nanocoap: add macro for timeout calculation

87e334a

kb2ma force-pushed the gcoap/confirm_piggy_client branch from f9cc42f to d67dfe7 Compare January 16, 2018 04:54

smlng suggested changes Jan 16, 2018

View reviewed changes

kb2ma added 2 commits January 16, 2018 06:37

net/gcoap: automate retries for confirm timeout

4223de6

net/gcoap: Update doc for confirmable message

5e1d3fe

kb2ma force-pushed the gcoap/confirm_piggy_client branch from adf0a5a to 5e1d3fe Compare January 16, 2018 13:17

smlng approved these changes Jan 16, 2018

View reviewed changes

smlng merged commit 50f3d6a into RIOT-OS:master Jan 16, 2018

kb2ma mentioned this pull request Feb 16, 2018

net/gcoap: fix sock send return type #8580

Merged

kb2ma deleted the gcoap/confirm_piggy_client branch February 4, 2019 11:29


		/* validate class and type for incoming */
		switch (coap_get_code_class(&pdu)) {

Conversation

kb2ma commented Jul 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kb2ma commented Aug 15, 2017

Uh oh!

kb2ma commented Aug 18, 2017

Uh oh!

nmeum Nov 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smlng commented Dec 1, 2017

Uh oh!

kb2ma commented Dec 14, 2017

Uh oh!

smlng commented Dec 18, 2017

Uh oh!

kb2ma commented Dec 20, 2017

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!

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!

kb2ma Jan 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

smlng commented Jan 15, 2018

Uh oh!

kb2ma commented Jan 15, 2018

Uh oh!

smlng commented Jan 15, 2018

Uh oh!

kb2ma commented Jan 16, 2018

Uh oh!

kb2ma commented Jan 16, 2018

Uh oh!

smlng commented Jan 16, 2018

Uh oh!

smlng commented Jan 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kb2ma commented Jan 16, 2018

Uh oh!

smlng left a comment

Choose a reason for hiding this comment

Uh oh!

kb2ma commented Jan 16, 2018

Uh oh!

smlng commented Jan 16, 2018

Uh oh!

Reviewers

kb2ma commented Jul 10, 2017 •

edited

Loading

nmeum Nov 3, 2017 •

edited

Loading

kb2ma Jan 14, 2018 •

edited

Loading