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

haukepetersen · 2018-04-11T13:11:55Z

Contribution description

Many string-based options (e.g. COAP_OPT_URI, COAP_OPT_LOCATION_PATH) share a similar structure: they consist of string 'snippets', that are split using some kind of separation character, typically /. This PR adds a more generic function for setting and reading these kind of options to nanocoap.

Please let me know what you think of this approach - once consensus is reached I will spend the time to add the missing doxygen...

Issues/PRs references

Follow-up on #8772

haukepetersen · 2018-04-11T13:12:47Z

sys/include/net/nanocoap.h

+}
+
+/**
+ * @brief    Get the packet's request URI


this is of course copy/paste bla bla, will adjust once the code is approved...

kaspar030 · 2018-04-11T13:18:19Z

ACK for the idea!

haukepetersen · 2018-04-11T13:23:30Z

Cool.

haukepetersen · 2018-04-12T07:20:25Z

alright, updated the doxygen accordingly.

haukepetersen · 2018-04-12T07:20:42Z

should be ready for review

kaspar030

Minors, feel free to squash after.

kaspar030 · 2018-04-11T13:26:24Z

sys/include/net/nanocoap.h

+ * @return      -ENOSPC if the complete option does fit into @p target
+ * @return      nr of bytes written to @p target (including '\0')
+ */
+int coap_get_opt_string(const coap_pkt_t *pkt, uint16_t optnum,


probably change to size_t return type for consistency

ssize_t since error codes are negative?

kaspar030 · 2018-04-12T08:23:28Z

sys/net/application_layer/nanocoap/nanocoap.c

@@ -504,6 +512,8 @@ size_t coap_put_option_uri(uint8_t *buf, uint16_t lastonum, const char *uri, uin
    return bufpos - buf;
 }

+


kaspar030 · 2018-04-12T08:25:08Z

@kb2ma I'm about to ACK, would you like to take a quick look?

bergzand

Same as @kaspar030, nothing big, feel free to squash after these too.

bergzand · 2018-04-12T08:49:48Z

sys/include/net/nanocoap.h

@@ -434,7 +494,32 @@ unsigned coap_get_content_type(coap_pkt_t *pkt);
 * @returns     -ENOSPC     if URI option is larger than NANOCOAP_URI_MAX
 * @returns     nr of bytes written to @p target (including '\0')
 */
-int coap_get_uri(coap_pkt_t *pkt, uint8_t *target);
+static inline int coap_get_uri(const coap_pkt_t *pkt, uint8_t *target)


If @kaspar030 comment is applied, the return type also propagates to this function.

bergzand · 2018-04-12T08:49:59Z

sys/include/net/nanocoap.h

+ * @returns     -ENOSPC     if URI option is larger than @p max_len
+ * @returns     nr of bytes written to @p target (including '\0')
+ */
+static inline int coap_get_location_path(const coap_pkt_t *pkt,


And to this one

bergzand · 2018-04-12T08:53:52Z

sys/net/application_layer/nanocoap/nanocoap.c

 {
-    coap_optpos_t *optpos = pkt->options;
+    coap_optpos_t *optpos = (coap_optpos_t *)pkt->options;


If you mark *optpos as const you don't need the cast here.

haukepetersen · 2018-04-12T09:43:20Z

Just a remark before I forget it again: somebody would need to go through all of the nanocoap code to check/fix the usage of const for function parameters. In this PR, I only touched the occurrences that were 'in my path'...

Comments look all valid, will address them ASAP

bergzand · 2018-04-12T09:46:33Z

There is a typo in the commit message of the second commit "LOCACTION_PATH" -> "LOCATION_PATH".

kb2ma · 2018-04-12T10:18:36Z

I'm about to ACK, would you like to take a quick look?

Yes, thanks. I'll post my feedback later today.

kb2ma · 2018-04-12T10:29:56Z

sys/include/net/nanocoap.h

+ * @param[in]   max_len     size of @p target
+ * @param[in]   separator   character used for separating the option parts
+ *
+ * @return      -ENOSPC if the complete option does fit into @p target


Should be "does not fit".

haukepetersen · 2018-04-12T11:44:38Z

rebased, squashed, and fixed comments:

renamed 'get' function to coap_get_option_string() -> more consistent with coap_put_option_ functions
changed return type for coap_get_option_string() and its users to ssize_t
make optpos in coap_find_option const
fixed some typos in doxygen

haukepetersen · 2018-04-12T11:45:14Z

and fixed the typo in the commit message :-)

kb2ma · 2018-04-13T04:46:28Z

I'm going to put my review here to keep it all together, rather than put some of it in the code.

coap_put_option_location_path() -- shouldn't this be coap_put_option_location() to include Location-Path and Location-Query? This approach would be consistent with coap_put_option_uri(), which includes Uri-Path and Uri-Query.

I have a couple of higher level concerns. Why do coap_[get|put]_option_string() require the separator as an argument? Isn't this implicit in the option number argument? For example, Uri-Path always uses '/', and Uri-Query uses '&' except for the initial '?'.

What about options with an opaque data type, like ETag? WIll this require a coap_[get|put]_option_buf()? Why not roll up string and opaque options into a single function? Once again, the option number defines the behavior.

I think the fundamental API is:

coap_opt_get_buf(pkt, optnum, uint8_t *target, max_len)
coap_opt_put_buf(buf, lastonum, optnum, target, uint8_t *source, source_len)
coap_opt_get_uint(pkt, optnum, uint32_t *target)
coap_opt_put_uint(buf, lastonum, optnum, value)

Any other option-specific functions likely are inline on top of those.

gcoap then simplifies the put options by removing the requirement to specify them in order, as shown in #8831.

kaspar030 · 2018-04-16T19:39:29Z

Why not roll up string and opaque options into a single function?

Isn't the difficulty with string functions that one e.g, path is split into multiple occurences of the option?

kb2ma · 2018-04-17T03:17:13Z

I have been experimenting with the adaptation of gcoap with the API I described above. The problem with this approach is that it means nanocoap would need to maintain lookup tables for the configuration of an option. Examples -- is it repeatable, what is the separator character, how to match the option number to a position in the table.

This approach would allow nanocoap to validate options more carefully. However, I think this work is two steps away from where we are today. The next step is just to get options working based on #8772. So, I agree that the API needs functions to get/set string values. If/when we get to option definitions, the string functions can be deprecated.

So how about this for an updated API. I do think the use of 'coap_opt' for the prefix improves usability, as @kaspar030 suggested.

coap_opt_get_string(pkt, optnum, uint8_t *target, max_len, separator)
coap_opt_put_string(buf, lastonum, optnum, char *source, separator)
coap_opt_get_buf(pkt, optnum, uint8_t *target, max_len)
coap_opt_put_buf(buf, lastonum, optnum, target, uint8_t *source, source_len)
coap_opt_get_uint(pkt, optnum, uint32_t *target)
coap_opt_put_uint(buf, lastonum, optnum, value)

haukepetersen · 2018-04-20T07:48:50Z

coap_put_option_location_path() -- shouldn't this be coap_put_option_location()

yes, that's better indeed. Will fix.

I have a couple of higher level concerns. Why do coap_[get|put]_option_string() require the separator as an argument? Isn't this implicit in the option number argument? For example, Uri-Path always uses '/', and Uri-Query uses '&' except for the initial '?'.

As you stated below, making this explicit depending on the option number implies that we need to keep some kind of a lookup table to map the behavior. This is IMHO overkill (especially considering nanocaop's scope to be as lightweight as possible). Further, the separator character in the string representation is something specific chosen by the nanocoap user, and is not really for making the string nice to read for humans, so there is IMHO no need for having e.g. a query string start with ? instead of starting it also with an &...

So how about this for an updated API.

I like it. I will adapt the functions touched by this PR and propose that we open an issue for adapting the rest of the option handling functions in separate PR(s). How does that sound?

kaspar030 · 2018-04-20T08:09:09Z

So how about this for an updated API.

Thinking about this:

coap_opt_get_string(pkt, optnum, uint8_t *target, max_len, separator)
coap_opt_put_string(buf, lastonum, optnum, char *source, separator)

... the get functions get a packet, the put functions a buffer.
@kb2ma intends to add higher level "put" type functions that work on a pkt, into gcoap. IMO, those can actually reside in nanocoap.

I propose using coap_opt_put_<type>_raw() for the put functions that write directly into a buffer, then introduce higher-level functions without _raw as described in #8831.

haukepetersen · 2018-08-23T07:35:02Z

Thanks @kb2ma! Now I start to remember/understand what the option handling concept was all about :-) I agree with your proposed changes and merged them in, will do one more verification round on my side now and then I hope this PR should be ready to go.

kb2ma · 2018-08-23T08:56:39Z

Excellent! Since you're working with Uri-Query options, remember that coap_parse() currently does not set the coap_pkt_t.qs attribute for gcoap. I'll watch for your OK for final review.

haukepetersen · 2018-08-23T09:52:55Z

alright, fixed one more alignment issue and did some (successful) testing. Also cleaned up the commits and squashed. On my part this PR should be ready for final review!

kb2ma

Also, please take a look at sys/net/application_layer/nanocoap/sock.c. It uses the obsolete coap_put_option_uri(). One more minor issue, the commit comment includes the word 'genering' :-)

kb2ma · 2018-08-27T05:56:38Z

sys/include/net/nanocoap.h

+}
+
+/**
+ * @brief    Convenience function for getting the packet's LOCATION_PATH option


Should be LOCATION_QUERY

kb2ma · 2018-08-27T05:57:35Z

sys/net/application_layer/nanocoap/nanocoap.c

@@ -296,7 +304,7 @@ ssize_t coap_handle_req(coap_pkt_t *pkt, uint8_t *resp_buf, unsigned resp_buf_le
    uint8_t *uri = pkt->url;
 #else
    uint8_t uri[NANOCOAP_URI_MAX];
-    if (coap_get_uri(pkt, uri) <= 0) {
+    if (coap_get_uri_query(pkt, uri) <= 0) {


Should be coap_get_uri_path()

kb2ma · 2018-08-28T11:47:47Z

Tested OK on native and samr21-xpro, with the code updates in my review. Test included use of rdcli-simple example to register with libcoap coap-rd server. Good stuff!

haukepetersen · 2018-08-29T11:06:15Z

addressed comments and squashed. Thanks for the testing!

haukepetersen · 2018-08-29T11:07:21Z

@kaspar030, @bergzand: are your change requests satisfies as well?

kb2ma · 2018-08-29T16:45:57Z

Still don't see the fix to sock.c and Murdock is unhappy with documentation.

haukepetersen · 2018-08-30T08:17:12Z

you are right, I missed that one

- add generic string put and get functions - add location path and location query options - add dedicated functions for getting and setting URI query, URI path, location query, and location path options

haukepetersen · 2018-08-30T08:31:23Z

ok, should be fixed now.

kb2ma · 2018-08-30T10:30:33Z

Looks good to me! Since you've got an open question out there, I'll leave it to you to merge when ready.

haukepetersen · 2018-08-31T07:26:06Z

Lets wait for the other two to approve or dismiss their review -> @kaspar030, @bergzand

bergzand · 2018-08-31T09:09:32Z

I'll try to take a look at this PR today to see if I can spot any remaining issues.

kaspar030

LGTM

bergzand

ACK

haukepetersen · 2018-08-31T10:05:25Z

awesome, thanks guys!

-> let's go!

haukepetersen · 2018-08-31T10:05:43Z

dammit, 10 seconds too late :-)

haukepetersen added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: network Area: Networking labels Apr 11, 2018

haukepetersen assigned kb2ma, kaspar030 and bergzand Apr 11, 2018

haukepetersen requested a review from kaspar030 April 11, 2018 13:11

haukepetersen commented Apr 11, 2018

View reviewed changes

haukepetersen force-pushed the add_nanocaop_urilocationoption branch from b8e8ca7 to c58df24 Compare April 11, 2018 13:22

bergzand changed the title ~~net/nanocaop: add generic handling for string-based options~~ net/nanocoap: add generic handling for string-based options Apr 11, 2018

kaspar030 requested changes Apr 12, 2018

View reviewed changes

bergzand requested changes Apr 12, 2018

View reviewed changes

kb2ma reviewed Apr 12, 2018

View reviewed changes

haukepetersen force-pushed the add_nanocaop_urilocationoption branch from 5768a81 to 5ff4745 Compare April 12, 2018 11:42

kb2ma mentioned this pull request Apr 14, 2018

[RFC] gcoap adaptation to #8772 #8831

Closed

haukepetersen force-pushed the add_nanocaop_urilocationoption branch from 5ff4745 to c6b9051 Compare April 20, 2018 07:38

haukepetersen force-pushed the add_nanocaop_urilocationoption branch from cb5cc7d to 31273af Compare August 23, 2018 09:51

kb2ma mentioned this pull request Aug 23, 2018

nanocoap: add server-side block2 support #8932

Merged

kb2ma requested changes Aug 27, 2018

View reviewed changes

haukepetersen force-pushed the add_nanocaop_urilocationoption branch 2 times, most recently from 90fab04 to 2cc095d Compare August 29, 2018 11:05

haukepetersen added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Aug 29, 2018

haukepetersen added 3 commits August 30, 2018 10:31

net/nanocoap: improve option handling

4c08e77

- add generic string put and get functions - add location path and location query options - add dedicated functions for getting and setting URI query, URI path, location query, and location path options

net/gcoap: adjust to nanocoap changes

a3b8c90

unittests/nanocoap: adjust to api changes

04bc6ee

haukepetersen force-pushed the add_nanocaop_urilocationoption branch from 2cc095d to 04bc6ee Compare August 30, 2018 08:31

kb2ma approved these changes Aug 30, 2018

View reviewed changes

kaspar030 approved these changes Aug 31, 2018

View reviewed changes

bergzand approved these changes Aug 31, 2018

View reviewed changes

bergzand merged commit e8dfabd into RIOT-OS:master Aug 31, 2018

haukepetersen deleted the add_nanocaop_urilocationoption branch August 31, 2018 10:05

kb2ma mentioned this pull request Sep 7, 2018

net/nanocoap: allow empty uri/location path option #9872

Merged

		@@ -504,6 +512,8 @@ size_t coap_put_option_uri(uint8_t buf, uint16_t lastonum, const char uri, uin
		return bufpos - buf;
		}

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

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

Conversation

haukepetersen commented Apr 11, 2018

Contribution description

Issues/PRs references

Choose a reason for hiding this comment

kaspar030 commented Apr 11, 2018

haukepetersen commented Apr 11, 2018

haukepetersen commented Apr 12, 2018

haukepetersen commented Apr 12, 2018

kaspar030 left a 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

kaspar030 commented Apr 12, 2018

bergzand left a 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

haukepetersen commented Apr 12, 2018

bergzand commented Apr 12, 2018

kb2ma commented Apr 12, 2018

Choose a reason for hiding this comment

haukepetersen commented Apr 12, 2018

haukepetersen commented Apr 12, 2018

kb2ma commented Apr 13, 2018 • edited Loading

kaspar030 commented Apr 16, 2018

kb2ma commented Apr 17, 2018

haukepetersen commented Apr 20, 2018

kaspar030 commented Apr 20, 2018

haukepetersen commented Aug 23, 2018

kb2ma commented Aug 23, 2018

haukepetersen commented Aug 23, 2018

kb2ma 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 Aug 28, 2018

haukepetersen commented Aug 29, 2018

haukepetersen commented Aug 29, 2018

kb2ma commented Aug 29, 2018

haukepetersen commented Aug 30, 2018

haukepetersen commented Aug 30, 2018

kb2ma commented Aug 30, 2018

haukepetersen commented Aug 31, 2018

bergzand commented Aug 31, 2018

kaspar030 left a comment

Choose a reason for hiding this comment

bergzand left a comment

Choose a reason for hiding this comment

haukepetersen commented Aug 31, 2018

haukepetersen commented Aug 31, 2018

kb2ma commented Apr 13, 2018 •

edited

Loading