sys: Add Link Format module

[leandrolanzieri]

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]

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.

[kaspar030]

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

[leandrolanzieri]

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]

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...

[haukepetersen]

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

[haukepetersen]

or core_lf_?

[leandrolanzieri]

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]

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]

+1 for clif, I like it!

[kb2ma]

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.

[leandrolanzieri]

Sounds reasonable, I will change this.

[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]

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]

@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

[leandrolanzieri]

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

Revising review after fixups

[kb2ma]

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]

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

[leandrolanzieri]

@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.

[kb2ma]

@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]

@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]

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.

[leandrolanzieri]

@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]

please squash