net/nanocoap: rework option handling #8772

kaspar030 · 2018-03-13T14:06:02Z

Contribution description

This PR reworks nanocoap's option handling. Instead of having a field in coap_pkt_t for each supported CoAP option, the parsing step now just notes down the options with option number and offset in an array. Furthermore, some layers of parsing/iterating functions are provided.

As gcoap is not yet adapted, some compatibility layer makes it still work.

As a first user for the new option handling, this PR also adds block1 support to nanocoap, with an example, blockwise-capable /sha256 enpoint for examples/nanocoap_server.

pekkanikander · 2018-03-13T15:00:43Z

sys/net/application_layer/nanocoap/nanocoap.c

    pkt->hdr = hdr;

    uint8_t *pkt_pos = hdr->data;
    uint8_t *pkt_end = buf + len;

-    memset(pkt->url, '\0', NANOCOAP_URL_MAX);


Is this change documented? If not, may it cause nasty surprises to existing code?

Or does the change on line #L119 override always the URL field?

coap->pkt does not exist anymore, unless gcoap is used. In that case, #L119 does overwrite the field.

Anyhow, I've added another null-termination for the case that the option is not present.

Ah, you mean whether removing the field might cause nasty surprises? Very probably. :)

pekkanikander · 2018-03-13T15:01:28Z

sys/net/application_layer/nanocoap/nanocoap.c

@@ -67,9 +65,13 @@ int coap_parse(coap_pkt_t *pkt, uint8_t *buf, size_t len)
        pkt->token = NULL;
    }

+    coap_optpos_t *optpos = pkt->options;
+    unsigned option_count = 0;


Is using just unsigned instead of the "more standard" unsigned int according to the used style guide?�

In RIOT just unsigned seems to be "more standard". Personally I prefer less typing and one-word type identifiers...

kb2ma · 2018-03-15T12:17:33Z

Thanks for this work, Kaspar! I have browsed through it, and just from a structural perspective:

I would separate Block support into another PR. It seems too much to refactor coap_pkt_t and option parsing -- and to add support for a CoAP extension RFC in a single PR.
With work at this level, I would expect to see at least some basic unit tests to validate reading options.

Naturally, I want to adapt gcoap to use this work ASAP. This will be a good way for me to get familiar with the implementation, and provide more detailed comments.

kaspar030 · 2018-03-16T08:45:02Z

I would separate Block support into another PR. It seems too much to refactor coap_pkt_t and option parsing -- and to add support for a CoAP extension RFC in a single PR.

Done -> #8788

With work at this level, I would expect to see at least some basic unit tests to validate reading options.

No time. :) Maybe sometimes extensive testing is enough?

kaspar030 · 2018-03-16T09:04:37Z

No time. :) Maybe sometimes extensive testing is enough?

Ok, that's no excuse. I've added a (basic) test for get_option_uri(), which tests parsing options into the array, iterating through multi options and finally also the get_option_uri().

kb2ma · 2018-03-16T10:33:25Z

Excellent, thanks a lot for that rework. I understand the ambivalence about unit tests, and appreciate sinking the extra time into it. We can build on it as needed.

Now it's time for me to get to work and provide some feedback. :-)

kb2ma · 2018-03-17T16:26:34Z

sys/net/application_layer/nanocoap/nanocoap.c

+
+#ifdef MODULE_GCOAP
+    coap_get_uri(pkt, pkt->url);
+    pkt->observe_value = UINT32_MAX;


Must update this attribute if observe option is present.

This idea points to a more general question -- should the get/set option functions be based on the type of data format rather than the specific option?

Yeah, very good idea. I've started in this direction with a coap_get_option_uint() for getting the observe value.

(that reply belongs to the getting/setting options comment below)

kb2ma

Overall, this work is nice improvement to remove the url and qs attributes in coap_pkt_t. Tested on native for nanocoap and gcoap. Works OK except for the specific comment in the code for reading an Observe option.

Getting/Setting option values

If you add an option number parameter to coap_get_option_uri(), you could retrieve a Uri-Query option, as is done when setting these options in coap_put_option_uri(). This idea points to a more general question -- should the get/set option functions be based on the type of data format rather than the specific option? Sec. 3.2 in RFC 7252 shows there are just four possibilities for data format -- string, uint, opaque (byte array), and empty. String and byte array can be handled similarly. Specific option numbers could then be get/set by an inline function that just sets the parameters for the data-format based get/set functions.

Maybe this work is a follow-on PR, but it seems like a small step. Certainly gcoap will need to read the Observe value when it is adapted to this PR.

coap_hdr_... functions

This is more of a general/FYI comment. I see you have removed coap_hdr_get_ver(). gcoap does not need this function specifically, but it does need coap_hdr_ functions for message type and token length. msg_type is used in gcoap_req_send2() to determine if confirmable. token_len is used when reading a confirmable observe ack in open PR #7548. In both cases no coap_pkt_t is available. Presently I manually inline the bit manipulation. So, a PR to add these functions is high on my list.

kb2ma · 2018-03-17T17:20:23Z

Shouldn't coap_find_option(), coap_get_content_type(), coap_get_len(), and coap_iterate_option() be declared in nanocoap.h?

kb2ma · 2018-03-22T10:13:01Z

sys/net/application_layer/nanocoap/nanocoap.c

+    uint8_t *opt_pos = coap_find_option(pkt, COAP_OPT_URI_PATH);
+    if (!opt_pos) {
+        DEBUG("nanocoap: no COAP_OPT_URI_PATH option\n");
+        *target = '\0';


The absence of a Uri-Path option means the path is "/". See Item 7 in sec. 6.5 of 7252. This actually is a bug in the current implementation as well.

kb2ma · 2018-03-25T12:42:42Z

I just added my thoughts on adaptation of gcoap to this PR in #8831. No concerns beyond the issues I raised above.

kaspar030 · 2018-04-04T14:42:39Z

I see you have removed coap_hdr_get_ver(). gcoap does not need this function specifically, but it does need coap_hdr_ functions for message type and token length.

Removal of coap_hdr_get_ver() was an accident. Probably happened when we moved nanocoap into main while adding doxygen. I intend to keep the coap_hdr_* functions.

kaspar030 · 2018-04-04T14:44:58Z

Shouldn't coap_find_option(), coap_get_content_type(), coap_get_len(), and coap_iterate_option() be declared in nanocoap.h?

They should... Can we postpone that? I hoped to clean up the naming consistency a little.
IMO, the prefixing is a little off, and there should be clear coap_pkt_get/set(), coap_hdr_get/set(), coap_opt_X_get/set() ... naming conventions. I think they are a off sometimes.

kb2ma · 2018-04-04T17:05:24Z

I hoped to clean up the naming consistency a little.

No worries. I'm definitely in favor of standardizing the function names to make it easier to use them. I can experiment with the declarations as needed to prototype the gcoap adaptation.

bergzand · 2018-04-09T07:24:22Z

sys/include/net/nanocoap.h

+ * @param[in]   pkt     pkt to work on
+ * @param[out]  target  buffer for target URI
+ *
+ * @returns     -EBADMSG    if no URI option in packet


With the comment from @kb2ma resolved, this return error code is not valid anymore for the specified situation.

fixed. I also fixed the return value for the case where there's no URI option. It returned 1, but doc and other cases return 2, because the '\0' gets counted.

This makes me wonder - what's more appropriate from an API perspective? Return the number of bytes written to the output buffer, or return the length of the extracted path string? We can change that now, not so much later on...

bergzand · 2018-04-09T07:50:22Z

sys/net/application_layer/nanocoap/nanocoap.c


    return 0;
 }

+uint8_t *coap_find_option(coap_pkt_t *pkt, unsigned opt_num)


Is there a reason for not marking the coap_pkt_t *pkt as const (const coap_pkt_t *pkt) here and in the other getters below?

No reason. folo?

This function should be exported in the header, right?!

kb2ma · 2018-04-10T03:22:01Z

sys/include/net/nanocoap.h

+ *
+ * @param[in]   pkt     packet to work on
+ *
+ * @returns     the packet's content type value


To be clear, specify value is COAP_FORMAT_NONE if packet does not include a content type.

good idea, done

kb2ma · 2018-04-10T03:24:06Z

sys/net/application_layer/nanocoap/nanocoap.c

+        uint16_t delta;
+        int option_len = 0;
+        uint8_t *pkt_pos = _parse_option(pkt, opt_pos, &delta, &option_len);
+        if (option_len) {


This test also should be true when option_len is zero.

yes, fixed.

kb2ma · 2018-04-10T03:27:57Z

Just added a couple of inline comments. My Observe tests passed after I updated coap_get_option_uint(). When these comments are addressed, I'm ready to approve.

kaspar030 · 2018-04-10T08:06:24Z

@kb2ma addressed your comments

haukepetersen

Did not test in detail yet, trying to map the functionality of this PR to what I PRed in https://github.com/kaspar030/sock/pull/18/files.

On first sight, it seems to me, that coap_get_option_uri() is very similar to what would be coap_get_option_query_string() and coap_get_option_location_path() (and possibly others). It seems to me, that we can probably handle all these multi-field, string type options with a more generic function.

So should we merge this PR as is and add this after?

haukepetersen · 2018-04-10T13:09:12Z

sys/net/application_layer/nanocoap/nanocoap.c


    return 0;
 }

+uint8_t *coap_find_option(coap_pkt_t *pkt, unsigned opt_num)


This function should be exported in the header, right?!

haukepetersen · 2018-04-10T13:23:27Z

sys/net/application_layer/nanocoap/nanocoap.c

+    return -1;
+}
+
+uint8_t *coap_iterate_option(coap_pkt_t *pkt, uint8_t **optpos, int *opt_len, int first)


same here: should be exported, right?!

kb2ma · 2018-04-10T14:05:43Z

@haukepetersen, see the comments above starting here, which match your comments. I agree on more generic functions. See my ideas for gcoap in #8331.

haukepetersen · 2018-04-10T15:15:21Z

sorry, the comments above slipped through.

@kb2ma are you referring to #7700? #8331 seems to be something quite different :-)

For further proceedings, I suggest we go with this PR as is - after another thorough round of testing of course. Then I could prepare a first step of generalization when adding the 'LOCATION_PATH' option needed for the RD client.

kb2ma · 2018-04-10T15:35:19Z

are you referring to #7700? #8331 seems to be something quite different :-)

Whoops, meant #8831, for gcoap adaptation to these changes. It includes similar thoughts on data-centric option functions.

For further proceedings, I suggest we go with this PR as is.

I agree. I plan to test latest fixups today.

kaspar030 · 2018-04-10T18:58:51Z

squashed

@bergzand @haukepetersen you good for now?

bergzand

All OK, Unless you want me to start nagging about function parameters that could have been declared as const :)

comments addressed / postponed

kaspar030 · 2018-04-10T19:15:45Z

Nice, thanks for the reviews!

haukepetersen · 2018-04-11T11:56:30Z

Thanks for the actual work :-)

kaspar030 added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Mar 13, 2018

kaspar030 requested review from haukepetersen, kb2ma and emmanuelsearch March 13, 2018 14:06

pekkanikander reviewed Mar 13, 2018

View reviewed changes

kaspar030 force-pushed the rework_nanocoap_option_handling branch from 969fb19 to 7a32be1 Compare March 14, 2018 13:39

kaspar030 force-pushed the rework_nanocoap_option_handling branch from b42a1a3 to c44a9c6 Compare March 16, 2018 08:38

kaspar030 mentioned this pull request Mar 16, 2018

nanocoap: add serverside block1 support #8788

Merged

kb2ma reviewed Mar 17, 2018

View reviewed changes

kb2ma requested changes Mar 17, 2018

View reviewed changes

kb2ma reviewed Mar 22, 2018

View reviewed changes

kb2ma mentioned this pull request Mar 25, 2018

[RFC] gcoap adaptation to #8772 #8831

Closed

kb2ma mentioned this pull request Mar 29, 2018

sys/include/net/gcoap.h: improve documentation #8843

Merged

bergzand requested changes Apr 9, 2018

View reviewed changes

kaspar030 force-pushed the rework_nanocoap_option_handling branch from 6dfcaa0 to da6ca3d Compare April 9, 2018 08:19

kaspar030 mentioned this pull request Apr 9, 2018

sys: provide over-the-air update mechanism #8902

Closed

kb2ma reviewed Apr 10, 2018

View reviewed changes

kaspar030 force-pushed the rework_nanocoap_option_handling branch from da6ca3d to 6417167 Compare April 10, 2018 08:02

haukepetersen previously requested changes Apr 10, 2018

View reviewed changes

kb2ma approved these changes Apr 10, 2018

View reviewed changes

kaspar030 added 2 commits April 10, 2018 20:58

nanocoap: rework option handling

dee793d

unittests/tests-nanocoap: add coap_get_uri() test

b33ae7d

kaspar030 force-pushed the rework_nanocoap_option_handling branch from 6417167 to b33ae7d Compare April 10, 2018 18:58

bergzand approved these changes Apr 10, 2018

View reviewed changes

kaspar030 merged commit 012c016 into RIOT-OS:master Apr 10, 2018

kaspar030 deleted the rework_nanocoap_option_handling branch April 10, 2018 19:11

This was referenced Apr 11, 2018

net/nanocoap: fix indention of doxy comments #8919

Merged

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

Merged

kb2ma mentioned this pull request Apr 19, 2018

examples: gcoap: stack overflow when NON times out #8981

Closed

kb2ma mentioned this pull request Apr 29, 2018

[RFC] CoAP API updates for options #9048

Closed

kb2ma mentioned this pull request May 12, 2018

net/nanocoap: Build message with coap_pkt_t #9085

Merged

This was referenced Jun 27, 2018

net/gcoap: Use nanocoap pkt/options API #9156

Merged

CoAP struct-based Options API #9309

Closed

net/nanocoap: rework option handling #8772

net/nanocoap: rework option handling #8772

Conversation

kaspar030 commented Mar 13, 2018

Contribution description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kb2ma commented Mar 15, 2018

kaspar030 commented Mar 16, 2018

kaspar030 commented Mar 16, 2018

kb2ma commented Mar 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kb2ma left a comment • edited Loading

Choose a reason for hiding this comment

Getting/Setting option values

coap_hdr_... functions

kb2ma commented Mar 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kb2ma commented Mar 25, 2018 • edited Loading

kaspar030 commented Apr 4, 2018

kaspar030 commented Apr 4, 2018

kb2ma commented Apr 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kb2ma commented Apr 10, 2018

kaspar030 commented Apr 10, 2018

haukepetersen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kb2ma commented Apr 10, 2018

haukepetersen commented Apr 10, 2018 • edited Loading

kb2ma commented Apr 10, 2018

kaspar030 commented Apr 10, 2018

bergzand left a comment

Choose a reason for hiding this comment

kaspar030 commented Apr 10, 2018

haukepetersen commented Apr 11, 2018

kb2ma left a comment •

edited

Loading

kb2ma commented Mar 25, 2018 •

edited

Loading

haukepetersen commented Apr 10, 2018 •

edited

Loading