net/gcoap: Use nanocoap pkt/options API by kb2ma · Pull Request #9156 · RIOT-OS/RIOT

kb2ma · 2018-05-19T05:08:12Z

Contribution description

Updates gcoap to use the nanocoap pkt/options API in #9085. This work allows removal of the gcoap-specific attributes in coap_pkt_t, which add a painful 130 bytes. (Now you know why I've been working so hard lately!)

Use of this API allows gcoap to fully build on nanocoap as a higher-level API. gcoap has always used a coap_pkt_t to build a message, and now nanocoap does too. See how gcoap_req_init() prepares for coap_build_hdr(), and then calls coap_pkt_init(), coap_opt_add_string() for the path, and coap_opt_add_uint() for the content format.

2018-10-23 Update

Simplified implementation of gcoap_finish(), which results in a minor API change. gcoap_finish() adds a Content-Format option to the packet after writing the payload. The original implementation of this PR accommodated options with any other option number that may have been added to the packet. Today I updated the implementation to accept options only with an option number less than Content-Format.

Fundamentally, the gcoap_finish() function has become obsolete as we have increased the use of CoAP options. The updated CoAP API now uses coap_opt_finish() to finalize the packet header before writing the payload. We plan to deprecate gcoap_finish() soon.

This change significantly simplifies gcoap_finish() until we deprecate it, and reduces the text space in the executable for this function by half, around 120 bytes. The cost is that existing code breaks when setting a Uri-Query option and then setting the format in gcoap_finish(). (Uri-Query is the only option with an option number greater than Content-Format that gcoap used before this PR.) In fact, the new Resource Directory implementation, cord_ep, had this problem, so I added a commit to this PR to fix it.

Issues/PRs references

Depends on #10223

See CoAP API Options Update wiki page for background, including how gcoap uses it.

kb2ma · 2018-05-20T01:07:33Z

I think code complete, but I'll give it a little time before removing WIP. I was able to remove most of the gcoap attributes in coap_pkt_t, with the nice reduction in size. I partially converted use of the observe value attribute, but removal will require a little work in nanocoap. I'll leave that for a follow-on PR.

kb2ma · 2018-06-12T04:58:36Z

Rebased and ready for review now that #9085 has been merged.

bergzand · 2018-06-12T07:46:03Z

sys/include/net/nanocoap.h

+ * Not intended for use in a transmitted packet. Must be a 3-byte unsigned
+ * value.
+ */
+#define COAP_FORMAT_NO_PAYLOAD  (65536)


Could you please right align all format values here?

Sure, no problem.

bergzand · 2018-06-12T07:50:14Z

sys/net/application_layer/gcoap/gcoap.c

            }

-            int res = strcmp((char *)&pdu->url[0], resource->path);
+            int res = strcmp((char *)&uri[0], resource->path);


Does it make sense to use strncmp here with NANOCOAP_URI_MAX as an extra safety precaution?

coap_get_uri() inserts the terminator, but I agree it's better to be safe.

Small note here, I'm not sure whether strncmp is in the C99 libc. (It's not the case with strnlen by default)

Use of strncmp() adds 40 bytes to the samr21-xpro executable text space. Since coap_get_uri() documentation asserts that it inserts the terminator, would you be OK with beefing up the unit tests for that function as an alternative way to accomplish the goal?

Agreed, especially since it is a string build in our own functions.

Perfect. I'm working up a PR in my nanocoap/test_get_uri_path branch.

Merged in tests in #9444.

bergzand · 2018-06-13T08:42:53Z

sys/net/application_layer/gcoap/gcoap.c

 int gcoap_add_qstring(coap_pkt_t *pdu, const char *key, const char *val)
 {
-    size_t qs_len = strlen((char *)pdu->qs);
+    char qs[NANOCOAP_URI_MAX];


Is there a reason for reusing NANOCOAP_URI_MAX and not keeping and using the NANOCOAP_QS_MAX here?

NANOCOAP_QS_MAX definition goes away in nanocoap.h.

Why not keep NANOCOAP_QS_MAX and use it here? To me it makes sense to separate these two defines, even if they have the same value.

I am inclined to leave as-is until we have a reason to differentiate max length of query string. Why add a constant we don't need? The use is internal/invisible to the user, so we can add it whenever necessary.

To me it feels as if the define is forcefully removed with this PR while there was still a use case for the define. For me defines are a way to make code more readable by giving a meaning to a constant.

Using the NANOCOAP_URI_MAX define here where the use case is not to fix the size of a uri array feels dirty.

So my opinion would be to keep the NANOCOAP_QS_MAX, maybe define the value as NANOCOAP_URI_MAX, but to at least keep it here for clarity reasons.

I understand your motivation. I removed the macro because nanocoap removed it from generic use in #8772, and left if for gcoap for historical reasons. I just pushed a fixup for gcoap only, as it was before this PR. We can reinstate NANOCOAP_QS_MAX generically for nanocoap when we work on merging #8920.

bergzand · 2018-06-13T08:54:13Z

sys/net/application_layer/gcoap/gcoap.c


-    /* make sure if url_len + the new query string fit into the url buffer */
-    if ((qs_len + key_len + val_len + 2) >= NANOCOAP_QS_MAX) {
+    if ((key_len + val_len + 2) >= NANOCOAP_URI_MAX) {


Could you please add a comment what the + 2 here is for. The NULL terminator and something else I'm missing I guess :)

Actually it's the query punctuation, but yes, let's describe the magic.

bergzand · 2018-06-13T09:05:41Z

sys/net/application_layer/gcoap/gcoap.c

-     * length in the buffer. Allows us to reconstruct buffer length later. */
-    pdu->payload_len  = len - (pdu->payload - buf);
-    pdu->content_type = COAP_FORMAT_NONE;
+    unsigned header_len  = sizeof(coap_hdr_t) + coap_get_token_len(pdu);


This looks like a coap_get_total_hdr_len(pdu);

I agree. It looks like I don't even need the header_len variable itself. I'll take a closer look.

bergzand · 2018-06-13T09:09:04Z

sys/net/application_layer/gcoap/gcoap.c

+
+    pdu->options_len = 0;
+    pdu->payload     = buf + header_len;
+    pdu->payload_len = len - (pdu->payload - buf);


pdu->payload - buf is equal to header_len right?

That looks right to me. I'll work to simplify this section.

bergzand · 2018-06-13T09:22:38Z

sys/net/application_layer/gcoap/gcoap.c

+        coap_pkt_init(pdu, buf, len, hdrlen);
+        coap_opt_add_string(pdu, COAP_OPT_URI_PATH, path, '/');
+        /* placeholder until gcoap_finish() */
+        coap_opt_add_uint(pdu, COAP_OPT_CONTENT_FORMAT, COAP_FORMAT_NO_PAYLOAD);


Is this to guarantee space for the content format in the buffer?

Yes, I'll make that clearer.

bergzand · 2018-06-13T09:35:22Z

sys/net/application_layer/gcoap/gcoap.c

 {
-    /* reconstruct full PDU buffer length */
-    size_t len = pdu->payload_len + (pdu->payload - (uint8_t *)pdu->hdr);
+    unsigned format_offset = UINT_MAX;


The use of this variable confuses me. It looks to me as if it is only used to remember if there was a content-format option

Yes, that's right. I will generally add more comments to make the logic in gcoap_finish() clearer. This tortured nature of this code is why I plan to deprecate gcoap_finish() in favor of gcoap_opt_format_finish(), which is called after all options have been entered but before the payload has been written. See API Options page for details.

bergzand · 2018-06-13T09:46:12Z

sys/net/application_layer/gcoap/gcoap.c

+            if (format == COAP_FORMAT_NONE) {
+                if (!move_pos) {
+                    move_pos = (uint8_t *)pdu->hdr + pdu->options[i].offset;
+                }


These snippet of 3 lines is in both branches of the if. Is it possible to move it outside of the if statement

I agree -- looks like an oversight.

bergzand · 2018-06-13T09:52:05Z

sys/net/application_layer/gcoap/gcoap.c

+    uint8_t *move_pos = 0;      /* buffer position to move forward */
+
+    /* Remove or adjust Content-Format option for request, if any.
+     * Assumes current payload format is COAP_FORMAT_NO_PAYLOAD */


I had a hard time understanding the code below. Maybe some additional comments would help the next person. Something like:

/* Remove or adjust Content-Format option for request, if any. * Assumes current payload format is COAP_FORMAT_NO_PAYLOAD * First the content format is updated and the option size difference is * stored in len_delta. Then the Option array is updated. Finally the options as * placed in the buffer are moved to their updated position (offset) */

Feel free to adjust to your liking

Yes, as I described in a reply above.

kb2ma · 2018-06-14T16:57:27Z

Thanks for the detailed review, @bergzand! I plan to implement the adjustments above soon.

kb2ma · 2018-06-26T05:54:16Z

Added fixups, except for a couple of items with replies above.

kb2ma · 2018-06-27T06:16:41Z

Addressed latest comments, including a fixup.

kb2ma · 2018-07-02T08:04:32Z

I plan to rebase on #9474. Actually I have rebased locally with no conflicts. I'd like to test a little to be sure there are no regressions.

kb2ma · 2018-07-03T04:59:55Z

Rebased; tests OK.

kb2ma · 2018-11-22T09:06:32Z

sys/net/application_layer/gcoap/gcoap.c

 {
-    /* reconstruct full PDU buffer length */
-    size_t len = pdu->payload_len + (pdu->payload - (uint8_t *)pdu->hdr);
+    if (pdu->options_len && payload_len) {


Actually, we can make this friendlier by adding another condition here. We won't check the assertion if the user specifies there is not a Content-Format option (format is COAP_FORMAT_NONE). Let me know if you disagree.

sounds valid, so +1

Side note: I think this check should be written as a single logical expression inside the assert() (intentional bad formatting...:

assert( !(pdu->options_len) || !(payload_len) || (pdu->options[pdu->options_len-1].opt_num < COAP_OPT_CONTENT_FORMAT));

hope I did the logic correctly here...

OK, I'll rework the assert. Your construct is more idiomatic although as you say the logic is a little harder to follow.

Fixed in commit e60663a, and added test in 7859556.

kb2ma · 2018-11-22T09:12:06Z

Thanks, @haukepetersen. Let me know your reactions on GCOAP_X_OPTIONS_BUF and any concerns about the change to the assertion check in gcoap_finish().

haukepetersen · 2018-11-22T10:06:12Z

done.

kb2ma · 2018-11-22T22:45:12Z

Rebased on master for #10223.

kb2ma · 2018-11-23T15:06:31Z

All comments addressed.

Observe still uses coap_pkt_t attribute.

haukepetersen

Nothing to add -> ACK

haukepetersen · 2018-11-26T09:23:31Z

feel free to squash :-)

kb2ma · 2018-11-26T10:00:32Z

Done, and all code bits accounted for!

haukepetersen · 2018-11-26T10:10:47Z

@bergzand:

@haukepetersen Feel free to dismiss my review if it is blocking

done that :-)

@haukepetersen

quote "@haukepetersen Feel free to dismiss my review if it is blocking"

haukepetersen · 2018-11-26T10:12:01Z

now let's see what Murdock is saying, but seems we are pretty much ready to go (fingers crossed).

haukepetersen · 2018-11-26T10:20:24Z

all green -> let's go!

kb2ma · 2018-11-26T10:27:51Z

That was a big one. Thanks @haukepetersen and @bergzand!

kb2ma added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet labels May 19, 2018

kb2ma requested review from bergzand, haukepetersen and kaspar030 May 19, 2018 05:08

kb2ma added State: waiting for other PR State: The PR requires another PR to be merged first Area: network Area: Networking labels May 19, 2018

kb2ma force-pushed the gcoap/use_opt_add_api branch from ecc7cc4 to 2aea999 Compare May 20, 2018 01:00

kb2ma mentioned this pull request May 20, 2018

net/nanocoap: add generic handling for string-based options #8920

Merged

kb2ma force-pushed the gcoap/use_opt_add_api branch from 2aea999 to 1bdc3f8 Compare May 22, 2018 10:28

bergzand added the Area: CoAP Area: Constrained Application Protocol implementations label May 25, 2018

kb2ma mentioned this pull request May 27, 2018

net/gcoap: Add detailed message init options #7700

Closed

kb2ma changed the title ~~[WIP] net/gcoap: Use nanocoap pkt/options API~~ net/gcoap: Use nanocoap pkt/options API May 30, 2018

kb2ma removed the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label May 30, 2018

kb2ma mentioned this pull request Jun 8, 2018

CoAP struct-based Options API #9309

Closed

kb2ma force-pushed the gcoap/use_opt_add_api branch from 1bdc3f8 to d047a58 Compare June 12, 2018 04:41

kb2ma removed the State: waiting for other PR State: The PR requires another PR to be merged first label Jun 12, 2018

bergzand previously requested changes Jun 13, 2018

View reviewed changes

kb2ma force-pushed the gcoap/use_opt_add_api branch from f556b98 to 7eb1ec6 Compare June 26, 2018 05:36

kb2ma mentioned this pull request Jun 28, 2018

net/nanocoap: test coap_get_uri() boundaries #9444

Merged

kb2ma force-pushed the gcoap/use_opt_add_api branch 2 times, most recently from c03b533 to 0078e6d Compare July 3, 2018 04:57

kb2ma mentioned this pull request Aug 23, 2018

net/nanocoap: add nanocoap_opt2 module #9474

Closed

kb2ma commented Nov 22, 2018

View reviewed changes

kb2ma mentioned this pull request Nov 22, 2018

net/nanocoap: fix string option separator write handling #10223

Merged

kb2ma force-pushed the gcoap/use_opt_add_api branch from 71c0269 to cd15b97 Compare November 22, 2018 22:43

kb2ma removed the State: waiting for other PR State: The PR requires another PR to be merged first label Nov 23, 2018

kb2ma added 6 commits November 24, 2018 07:09

net/gcoap: Add format option to nanocoap

fa5ab4e

net/gcoap: Use nanocoap options API

f0b966d

Observe still uses coap_pkt_t attribute.

net/gcoap: move macros to implementation file

034c78d

net/gcoap: update tests for nanocoap options API

f8e5b3d

net/gcoap: remove gcoap attributes from coap_pkt_t

424a01d

cord/ep: Update for gcoap API change

8fa6f10

haukepetersen approved these changes Nov 26, 2018

View reviewed changes

kb2ma force-pushed the gcoap/use_opt_add_api branch from 0e275f9 to 8fa6f10 Compare November 26, 2018 09:59

haukepetersen merged commit b603c29 into RIOT-OS:master Nov 26, 2018

kb2ma mentioned this pull request Nov 28, 2018

net/gcoap: allow NULL path in gcoap_req_init() #10501

Merged

aabadie added this to the Release 2019.01 milestone Dec 2, 2018

kb2ma mentioned this pull request Dec 31, 2018

net/nanocoap: remove obsolete macro #10687

Merged

kb2ma mentioned this pull request Jan 16, 2019

net/gcoap: make options buf macros configurable #10791

Merged

kb2ma deleted the gcoap/use_opt_add_api branch February 4, 2019 17:13

kb2ma mentioned this pull request Nov 28, 2019

net/gcoap: deprecate gcoap_finish() #12838

Merged

Conversation

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

Uh oh!

Contribution description

2018-10-23 Update

Issues/PRs references

Uh oh!

kb2ma commented May 20, 2018

Uh oh!

kb2ma commented Jun 12, 2018

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!

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!

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 commented Jun 14, 2018

Uh oh!

kb2ma commented Jun 26, 2018

Uh oh!

kb2ma commented Jun 27, 2018

Uh oh!

kb2ma commented Jul 2, 2018

Uh oh!

kb2ma commented Jul 3, 2018

Uh oh!

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

kb2ma commented May 19, 2018 •

edited

Loading

kb2ma Nov 22, 2018 •

edited

Loading