gcoap: don't allocate memo for clients without response handlers #9310

miri64 · 2018-06-08T06:01:46Z

Contribution description

@haukepetersen and I were discussing this the other day offline: If the client doesn't want to handle the response we don't need to add a waiter for it (saving that space for other requests that do want to get a response + allowing for "spamming" NON requests, without much memory requirements¹). I'm not 100% sure if I did not miss anything, but this small fix should allow for that.

¹ This might be required e.g. for ergonomic applications, like a game controller, where the feedback of an action should be somewhat immediately.

Issues/PRs references

None

kb2ma

Sounds good to me, and simple to implement. This PR actually fits in with RFC 7967, which specifies a No-Response option to ask the server not to send a response to certain requests.

Also see l.890. Must check for NULL memo there, too. ;-)

kb2ma · 2018-06-11T06:30:41Z

sys/net/application_layer/gcoap/gcoap.c

@@ -796,71 +796,75 @@ size_t gcoap_req_send2(const uint8_t *buf, size_t len,
                       gcoap_resp_handler_t resp_handler)
 {
    gcoap_request_memo_t *memo = NULL;
+    unsigned msg_type  = (*buf & 0x30) >> 4;
+
    assert(remote != NULL);

    /* Find empty slot in list of open requests. */


Move this comment inside the conditional? I found it confusing to leave here. OTOH, it might be good to add a new comment here, like "Allocate memo if necessary"

kb2ma · 2018-06-11T06:32:16Z

sys/net/application_layer/gcoap/gcoap.c

-                memo->msg.data.pdu_buf = &_coap_state.resend_bufs[i][0];
-                memcpy(memo->msg.data.pdu_buf, buf, GCOAP_PDU_BUF_SIZE);
-                memo->msg.data.pdu_len = len;
+    if ((resp_handler != NULL) || (msg_type == COAP_TYPE_NON)) {


Shouldn't the check here be for COAP_TYPE_CON rather than NON?

Yes, you are right... This was probably a typo ^^"

miri64 · 2018-06-11T12:29:45Z

Addressed change request.

This PR actually fits in with RFC 7967, which specifies a No-Response option to ask the server not to send a response to certain requests.

Mh, but this option must be implemented first, right (+ its presence checked in https://github.com/RIOT-OS/RIOT/pull/9310/files#diff-9739e1bd2e91135c214745f71bc5c86aR805 then as well)?

kb2ma · 2018-06-11T17:11:44Z

Mh, but this option must be implemented first, right (+ its presence checked [in the code for this PR] )

Yes, this PR is a good first step down the road. First, we reduce processing on the client device in this PR, then we ask the server device to not send the response in the first place, by implementing 7967.

Changes look good. Give me a chance to run some tests.

kb2ma

My tests worked after fixing the two items in this review. Let's go through one more round before squashing.

kb2ma · 2018-06-14T05:01:56Z

sys/net/application_layer/gcoap/gcoap.c

        }
-        else {
+
+        uint32_t timeout   = 0;


Must move this initialization above the 'resp_handler != NULL' conditional. Otherwise, l.869 won't compile. ;-)

Oops, done (this time tested ;-))

kb2ma · 2018-06-14T05:05:11Z

sys/net/application_layer/gcoap/gcoap.c

@@ -883,7 +889,7 @@ size_t gcoap_req_send2(const uint8_t *buf, size_t len,
            DEBUG("gcoap: can't wake up mbox; no timeout for msg\n");
        }
    }
-    if (res <= 0) {
+    if ((memo != NULL) && (res <= 0)) {


Please rework this conditional so the DEBUG stmt on l.897 still is executed when memo is NULL.

kb2ma · 2018-06-15T04:05:34Z

Looks good and tests OK on native! Please squash.

miri64 · 2018-06-15T07:47:26Z

Squashed

Commit line length too long.

kb2ma · 2018-06-15T10:25:35Z

Apologies @miri64, I just noticed the commit message length. Would you please cut it back to 50 chars.

miri64 · 2018-06-15T11:32:18Z

Why? <72 chars is perfectly fine as well. That's why it is just a warning but not an error.

miri64 · 2018-06-15T11:40:07Z

I honestly don't think I can make it any shorter and still be meaningful

kb2ma · 2018-06-15T12:18:19Z

@miri64, maybe I'm just confused on this point. The PR guidelines page says:

Be aware that each commit title is only allowed to have 50 characters.

I thought the requirement was for the first line, i.e. the title, to be no longer than 50 characters, but subsequent lines in the commit message could be longer. I see that dist/tools/commit-msg/check.sh only errors when any line greater than 72 chars. OTOH, I recall being asked to cut back the first line myself on another PR.

I'm just trying to be a good (but not pedantic) maintainer here. :-) If the CI build is happy, I should be happy, too?

miri64 · 2018-06-15T13:19:14Z

I thought the requirement was for the first line, i.e. the title, to be no longer than 50 characters, but subsequent lines in the commit message could be longer. I see that dist/tools/commit-msg/check.sh only errors when any line greater than 72 chars. OTOH, I recall being asked to cut back the first line myself on another PR.

If the PR guidelines say that, they are wrong, I think. Normal git best practices (which I practice in both contribution and review) is that commit messages should be ideally <50 characters but no longer than 72 (which is also the cut GitHub makes in showing the summary line and is also the hard limit Git uses internally). So we should try to achieve <50 chars, but not enforce if, if the reasons for it are reasonable (which in my case I think they are).

We have the same "stretchy" conditional for our code line length (try for <80, but have 100 at max), so I think it is reasonable to have this for commit messages as well.

miri64 · 2018-06-15T13:25:22Z

Fixed the guidelines ;-)

kb2ma

Thanks for updating the PR guidelines!

kb2ma · 2018-06-15T14:28:23Z

We have a consensus of happiness, so let's go!

miri64 · 2018-06-15T14:29:49Z

Thanks! :-)

miri64 requested a review from kb2ma June 8, 2018 06:01

kb2ma requested changes Jun 11, 2018

View reviewed changes

kb2ma reviewed Jun 11, 2018

View reviewed changes

kb2ma requested changes Jun 14, 2018

View reviewed changes

gcoap: don't allocate memo for clients without response handlers

b03aa52

miri64 force-pushed the gcoap/enh/clients-without-response-handlers branch from b35a939 to b03aa52 Compare June 15, 2018 07:46

kb2ma previously approved these changes Jun 15, 2018

View reviewed changes

kb2ma added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jun 15, 2018

kb2ma approved these changes Jun 15, 2018

View reviewed changes

kb2ma merged commit 4f8c3b7 into RIOT-OS:master Jun 15, 2018

miri64 deleted the gcoap/enh/clients-without-response-handlers branch June 15, 2018 14:29

cladmi added this to the Release 2018.07 milestone Jul 10, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gcoap: don't allocate memo for clients without response handlers #9310

gcoap: don't allocate memo for clients without response handlers #9310

miri64 commented Jun 8, 2018

kb2ma left a comment •

edited

Loading

kb2ma Jun 11, 2018

kb2ma Jun 11, 2018

miri64 Jun 11, 2018

miri64 commented Jun 11, 2018 •

edited

Loading

kb2ma commented Jun 11, 2018 •

edited

Loading

kb2ma left a comment

kb2ma Jun 14, 2018

miri64 Jun 14, 2018

kb2ma Jun 14, 2018

miri64 Jun 14, 2018

kb2ma commented Jun 15, 2018

miri64 commented Jun 15, 2018

kb2ma commented Jun 15, 2018

miri64 commented Jun 15, 2018 •

edited

Loading

miri64 commented Jun 15, 2018

kb2ma commented Jun 15, 2018

miri64 commented Jun 15, 2018

miri64 commented Jun 15, 2018

kb2ma left a comment

kb2ma commented Jun 15, 2018

miri64 commented Jun 15, 2018

gcoap: don't allocate memo for clients without response handlers #9310

gcoap: don't allocate memo for clients without response handlers #9310

Conversation

miri64 commented Jun 8, 2018

Contribution description

Issues/PRs references

kb2ma left a comment • edited Loading

Choose a reason for hiding this comment

kb2ma Jun 11, 2018

Choose a reason for hiding this comment

kb2ma Jun 11, 2018

Choose a reason for hiding this comment

miri64 Jun 11, 2018

Choose a reason for hiding this comment

miri64 commented Jun 11, 2018 • edited Loading

kb2ma commented Jun 11, 2018 • edited Loading

kb2ma left a comment

Choose a reason for hiding this comment

kb2ma Jun 14, 2018

Choose a reason for hiding this comment

miri64 Jun 14, 2018

Choose a reason for hiding this comment

kb2ma Jun 14, 2018

Choose a reason for hiding this comment

miri64 Jun 14, 2018

Choose a reason for hiding this comment

kb2ma commented Jun 15, 2018

miri64 commented Jun 15, 2018

kb2ma commented Jun 15, 2018

miri64 commented Jun 15, 2018 • edited Loading

miri64 commented Jun 15, 2018

kb2ma commented Jun 15, 2018

miri64 commented Jun 15, 2018

miri64 commented Jun 15, 2018

kb2ma left a comment

Choose a reason for hiding this comment

kb2ma commented Jun 15, 2018

miri64 commented Jun 15, 2018

kb2ma left a comment •

edited

Loading

miri64 commented Jun 11, 2018 •

edited

Loading

kb2ma commented Jun 11, 2018 •

edited

Loading

miri64 commented Jun 15, 2018 •

edited

Loading