sys: Add Link Format module by leandrolanzieri · Pull Request #11189 · RIOT-OS/RIOT

leandrolanzieri · 2019-03-14T17:11:42Z

Contribution description

This PR adds a simple CoRE Link Format module to encode and decode strings, together with a set of unit test cases.

It can be used for describing the registered CoAP resources (#11171) or registering them on a resource directory. It can be also useful for parsing the responses when looking up on a resource directory (#10222).

~~In this PR also the resource listing of gcoap is modified to use some of the basic functionalities of the module.~~

Testing procedure

Go to tests/unittests and run make tests-link_format test. For a detailed output add CFLAGS=-DTESTS_LINK_FORMAT_PRINT.
Run any application that uses gcoap and list the resources by sending a GET request to /.well-known/core.

Issues/PRs references

See also #11171
Could be of use in #10222

leandrolanzieri · 2019-03-14T17:13:42Z

Maybe @pokgak want to take a look?

pokgak

Looks good to me. Left some comment on the changes in gcoap. Also maybe the behaviour of the module when passing NULL as output buffer should be mentioned. Had to look in gcoap for that one.

sys/net/application_layer/gcoap/gcoap.c

kaspar030 · 2019-03-19T15:39:56Z

Hm, "link format" is not specific enough as a name, but "core_link_format" is difficult in RIOT...

leandrolanzieri · 2019-03-20T09:26:51Z

Also maybe the behaviour of the module when passing NULL as output buffer should be mentioned

@pokgak Added documentation on functions. Also fixed link_format_encode_link to follow that behavior, add added it on the unit test.

leandrolanzieri · 2019-03-25T17:43:11Z

Hm, "link format" is not specific enough as a name, but "core_link_format" is difficult in RIOT...

What about "coap_core_link_format"? @smlng @kb2ma any opinion on that?

haukepetersen

Just did a quick scroll through the code, so no proper review. But I think this looks quite good to me.

One remark on the PR structure: it might make sense to cut the changes to gcoap out and PR them separately...

sys/include/link_format.h

haukepetersen · 2019-03-26T16:33:00Z

on the naming: how about clf_ (serious)? Or by my personal naming scheme it would be rosalinde (not) :-)

haukepetersen · 2019-03-26T16:33:32Z

or core_lf_?

leandrolanzieri · 2019-03-26T19:52:01Z

One remark on the PR structure: it might make sense to cut the changes to gcoap out and PR them separately...

Removed changes to gcoap, will open a separate PR

leandrolanzieri · 2019-03-26T19:52:31Z

on the naming: how about clf_ (serious)? Or by my personal naming scheme it would be rosalinde (not) :-)

mmm Now I kind of like clif

haukepetersen · 2019-03-26T20:29:11Z

+1 for clif, I like it!

kb2ma · 2019-04-01T00:34:33Z

sys/include/clif.h

+ * @brief Link format parameter descriptor
+ */
+typedef struct {
+    clif_param_type_t type;        /**< type of parameter */


Is it worthwhile to include this here and generate it with each call to clif_get_param()? Maybe create a separate unsigned clif_get_param_type(const char *key) and let the user decide when to use it.

In this case the char *ext attribute becomes char *key.

Sounds reasonable, I will change this.

sys/include/clif.h

kb2ma

I agree that the additions to the link format implementation warrant the separate module in this PR. It also makes sense for gcoap and nanocoap to use this API, and it's proably worth a separate PR for that work.

I added a comment to #11171 on extending CoAP resources. Given the use cases there, I favor the lower level aspects of the clif API. The functions provide a stream-like API where the client provides or collects the input, and then loops through it to execute the functions to read/write the target and params.

For encoding, clif_add_target(), clif_add_param(), and clif_add_link_separator() are necessary. gcoap_get_resource_list() provides an example of looping through the resources, but it needs clif_add_param_list(const char *list) since it likely already is encoded for each coap_resource_t as suggested in #11171.

For decoding, use clif_get_target() and clif_get_param() over an input string. For resource lookup in #10222, the source string could be collected with cord_lc_res_raw().

Beyond these functions, for clif_decode_links(), how to predict the number of links and params to expect, and why use all that memory up front? Maybe just a singular clif_decode_link() is sufficient. This logically matches to clif_encode_link().

See other inline comments, too.

leandrolanzieri · 2019-04-01T13:59:04Z

but it needs clif_add_param_list(const char *list)

Would this only append to a buffer the ';' character and the given string?

@kb2ma I removed the type from the structure, also modified the decoding function do it one link at a time and accept a NULL as param array and return the needed amount of parameters.

leandrolanzieri · 2019-05-16T15:37:56Z

@kb2ma are you ok with the changes?

smlng

first review round mostly on documentation, there should also be a reference to RFC 6690 which this is implementing

sys/clif/Makefile

sys/clif/include/clif_internal.h

leandrolanzieri · 2019-06-13T07:30:52Z

first review round mostly on documentation, there should also be a reference to RFC 6690 which this is implementing

@smlng Thanks for reviewing, I addressed the comments and added the RFC 6690 reference.

smlng

some more reviewing

sys/clif/clif.c

Revising review after fixups

sys/include/clif.h

kb2ma · 2019-09-23T03:15:45Z

This all looks good now, @leandrolanzieri! The decoding logic is easy to follow. If you can address the const parameters and the parameter documentation, I'm ready to approve. Let's try to get this in for 2019.10 if you've got the time.

kb2ma · 2019-09-23T03:17:41Z

@smlng, would you please confirm your concerns have been addressed.

leandrolanzieri · 2019-09-23T08:05:32Z

@kb2ma Thanks for the review! I went over the API fixing the constant parameters, also I fixed doc errors I found, including the example snippet.

sys/clif/clif.c

sys/include/clif.h

kb2ma · 2019-09-26T10:38:02Z

@leandrolanzieri, I apologize for not being more specific in my earlier comments about making elements const. My thinking was that for clif_decode_link() it was reasonable to say we would not modify the input buffer. I agree this reasoning carries through to clif_get_attr().

I don't think we can carry this through to the clif_attr_t struct in general through because it may be created or used in other contexts besides reading an input string. I'm OK with the const on the 'key' attribute for reasons we discussed earlier.

Honestly I am not an expert at use of const though, so let me know if you agree.

leandrolanzieri · 2019-09-26T11:33:14Z

@leandrolanzieri, I apologize for not being more specific in my earlier comments about making elements const. My thinking was that for clif_decode_link() it was reasonable to say we would not modify the input buffer. I agree this reasoning carries through to clif_get_attr().

I don't think we can carry this through to the clif_attr_t struct in general through because it may be created or used in other contexts besides reading an input string. I'm OK with the const on the 'key' attribute for reasons we discussed earlier.

Honestly I am not an expert at use of const though, so let me know if you agree.

@kb2ma Sure, sorry about that. It makes sense that the value can be created by the application so it should not be const. Changed!

kb2ma · 2019-09-26T12:16:54Z

Looks good to me! Please squash. I really like the detailed unit tests. Let's get some real world use now and see how it works.

ping @smlng to reconsider review

smlng

I would generally opt for single condition in assert not combining 2 (or more), also would avoid goto especially where I pointed out there is no specific benefit.

sys/clif/clif.c

sys/include/clif.h

leandrolanzieri · 2019-10-02T14:01:38Z

@smlng thanks for the review.

I would generally opt for single condition in assert not combining 2 (or more),

Changed.

also would avoid goto especially where I pointed out there is no specific benefit.

I usually tend to prioritize having a single point of return and the flow seemed clear to me to use the gotos, but I don't really have a strong opinion in this case.

All comments should be addressed now.

smlng · 2019-10-02T14:23:45Z

please squash

This module implements a simple encoder and decoder of CoRE Link Format (RFC 6690).

leandrolanzieri added the Type: new feature The issue requests / The PR implemements a new feature for RIOT label Mar 14, 2019

leandrolanzieri requested review from haukepetersen, kaspar030 and kb2ma March 14, 2019 17:11

pokgak suggested changes Mar 19, 2019

View reviewed changes

sys/net/application_layer/gcoap/gcoap.c Outdated Show resolved Hide resolved

sys/net/application_layer/gcoap/gcoap.c Outdated Show resolved Hide resolved

smlng added the Area: CoAP Area: Constrained Application Protocol implementations label Mar 21, 2019

haukepetersen reviewed Mar 26, 2019

View reviewed changes

sys/include/link_format.h Outdated Show resolved Hide resolved

leandrolanzieri force-pushed the gcoap_link_format_options branch from 4967976 to 7ec6868 Compare March 26, 2019 19:49

kb2ma reviewed Apr 1, 2019

View reviewed changes

sys/include/clif.h Outdated Show resolved Hide resolved

kb2ma previously requested changes Apr 1, 2019

View reviewed changes

kb2ma mentioned this pull request Apr 1, 2019

net: add CoRE RD lookup client implementation #10222

Merged

leandrolanzieri force-pushed the gcoap_link_format_options branch from cba6bef to 88dd550 Compare April 1, 2019 13:57

leandrolanzieri mentioned this pull request Apr 2, 2019

net/coap: Add Link Format attributes to resources #11328

Closed

leandrolanzieri requested a review from kb2ma April 26, 2019 13:46

smlng suggested changes Jun 12, 2019

View reviewed changes

sys/clif/Makefile Outdated Show resolved Hide resolved

sys/clif/include/clif_internal.h Outdated Show resolved Hide resolved

smlng suggested changes Jun 13, 2019

View reviewed changes

kb2ma added Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines labels Sep 23, 2019

kb2ma reviewed Sep 23, 2019

View reviewed changes

sys/include/clif.h Outdated Show resolved Hide resolved

kb2ma reviewed Sep 23, 2019

View reviewed changes

sys/include/clif.h Outdated Show resolved Hide resolved

kb2ma reviewed Sep 23, 2019

View reviewed changes

sys/include/clif.h Outdated Show resolved Hide resolved

kb2ma reviewed Sep 23, 2019

View reviewed changes

sys/include/clif.h Outdated Show resolved Hide resolved

kb2ma added Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines labels Sep 23, 2019

kb2ma added this to the Release 2019.10 milestone Sep 23, 2019

kb2ma reviewed Sep 26, 2019

View reviewed changes

sys/clif/clif.c Outdated Show resolved Hide resolved

kb2ma reviewed Sep 26, 2019

View reviewed changes

sys/include/clif.h Outdated Show resolved Hide resolved

kb2ma approved these changes Sep 26, 2019

View reviewed changes

leandrolanzieri force-pushed the gcoap_link_format_options branch from 1ecd1ea to d1ad8e1 Compare September 26, 2019 13:13

smlng suggested changes Oct 2, 2019

View reviewed changes

sys/clif/clif.c Outdated Show resolved Hide resolved

sys/clif/clif.c Outdated Show resolved Hide resolved

sys/clif/clif.c Outdated Show resolved Hide resolved

sys/include/clif.h Outdated Show resolved Hide resolved

smlng approved these changes Oct 2, 2019

View reviewed changes

leandrolanzieri force-pushed the gcoap_link_format_options branch from bcc8819 to 5f76c11 Compare October 2, 2019 14:49

leandrolanzieri added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 2, 2019

leandrolanzieri force-pushed the gcoap_link_format_options branch from 5f76c11 to fc92bf1 Compare October 4, 2019 07:03

leandrolanzieri added 2 commits October 4, 2019 11:35

sys: Add clif (CoRE Link Format) module

5febec8

This module implements a simple encoder and decoder of CoRE Link Format (RFC 6690).

tests: Add unit tests for clif module

3ca6a28

leandrolanzieri force-pushed the gcoap_link_format_options branch from fc92bf1 to 3ca6a28 Compare October 4, 2019 09:36

kb2ma merged commit 033b60b into RIOT-OS:master Oct 5, 2019

Conversation

leandrolanzieri commented Mar 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

leandrolanzieri commented Mar 14, 2019

Uh oh!

pokgak left a comment • 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!

Uh oh!

Uh oh!

kaspar030 commented Mar 19, 2019

Uh oh!

leandrolanzieri commented Mar 20, 2019

Uh oh!

leandrolanzieri commented Mar 25, 2019

Uh oh!

haukepetersen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

haukepetersen commented Mar 26, 2019

Uh oh!

haukepetersen commented Mar 26, 2019

Uh oh!

leandrolanzieri commented Mar 26, 2019

Uh oh!

leandrolanzieri commented Mar 26, 2019

Uh oh!

haukepetersen commented Mar 26, 2019

Uh oh!

kb2ma Apr 1, 2019

Choose a reason for hiding this comment

Uh oh!

leandrolanzieri Apr 1, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kb2ma left a comment

Choose a reason for hiding this comment

Uh oh!

leandrolanzieri commented Apr 1, 2019

Uh oh!

leandrolanzieri commented May 16, 2019

Uh oh!

smlng left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

leandrolanzieri commented Jun 13, 2019

Uh oh!

smlng left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kb2ma commented Sep 23, 2019

Uh oh!

kb2ma commented Sep 23, 2019

Uh oh!

leandrolanzieri commented Sep 23, 2019

Uh oh!

Uh oh!

Uh oh!

kb2ma commented Sep 26, 2019

leandrolanzieri commented Mar 14, 2019 •

edited

Loading

pokgak left a comment •

edited

Loading