pkg/libcoap: Add in libcoap CoAP library support

[mrdeep1]

Contribution description

Adds in support for using the libcoap CoAP library with version 4.3.4, as per https://github.com/obgm/libcoap with documentation at https://libcoap.net/doc/reference/develop/ .

pkg/libcoap contains the information for the libcoap library to be included and built as appropriate.
examples/libcoap-client provides an example CoAP client.
examples/libcoap-server provides an example CoAP server.
pkg/Kconfig updated to include libcoap.

Testing procedure

test/pkg/libcoap provided which does a loop-back test with libcoap client logic sending request to libcoap server logic and sanity checks the response.

Issues/PRs references

None, other than adding in new functionality.

[JKRhb]

Just a couple of minor suggestions below to get rid of the warnings.

[riot-ci]

Murdock results

✔️ PASSED

4a73a8d pkg/libcoap: Add in libcoap CoAP library support

Success	Failures	Total	Runtime
11166	0	11166	10m:37s

Artifacts

• Documentation preview

[miri64]

@chrysn could you maybe have a look?

[mrdeep1]

I guess I do not understand why the Murdock test build failed for libcoap - msb-430 is definitely defined in tests/pkg/libcoap/Makefile.ci as a BOARD_INSUFFICIENT_MEMORY entry. It appears that size_t in this case is only 2 bytes wide.

[maribu]

This is correct behavior. The BOARD_INSUFFICIENT_MEMORY is used to flag boards that have too little RAM and/or flash for the linking step to succeed. Hence, the CI will do all compilation steps (to still confirm that the code is otherwise valid and portable C code), but skip the final linking step.

We do have two platforms where size_t is 16 bits wide: AVR and MSP430.

Note that size_t is defined to be able to hold any array index for an in-memory C array. So it may even not be possible to cast any pointer to size_t and back without loosing information, as the address space may be larger than anything that can be stored in RAM in a contiguous fashion.

Hence, size_t is not a suitable type for lengths of files written to e.g. SD cards and transferred over network e.g. using CoAP blockwise transfers.

But given that this bug is likely upstream, you could just add FEATURES_REQUIRED_ANY += arch_32bit|arch_64bit to the Makefile.dep of the package and prevent it from being compiled on our 8 bit and 16 bit boards.

[mrdeep1]

Thanks for the information, It looks like only a compile issue in one place, which I will get fixed shortly.

[maribu]

I only took a quick peek, but this looks good to me. @chrysn might be interested in this, as this would also provide an alternative implementation of OSCORE.

One thing: Most (if not all) of the KConfig parts can be dropped now, (see https://forum.riot-os.org/t/last-chance-for-kconfig-dependency-modelling/4068 for details). Selection of modules and packages and the dependency resolution via KConfig has been removed from upstream; but you can still use KConfig to expose configuration knobs (such as buffer sizes, ports to listen on, etc.).

I can take an in-depth look tomorrow, if you think this is ready for review? Would be cool to get this back in :)

[mrdeep1]

I can take an in-depth look tomorrow, if you think this is ready for review?

I have just found an OSCORE issue that needs to get fixed. I will fix this and have everything pushed for Monday.

Most (if not all) of the KConfig parts can be dropped now

I will look at doing this, probably not by Monday though.

[mrdeep1]

I have just found an OSCORE issue that needs to get fixed. I will fix this and have everything pushed for Monday.

Just pushed the fix. Do you want all these commits squashed?

[maribu]

Just pushed the fix. Do you want all these commits squashed?

Do as you prefer. Some other maintainers are a bit picky about when to squash, but I only look at the diff to upstream (or to when I last checked). So as long as only I am reviewing, you can squash (or not squash) during the review as works best for you.

[mrdeep1]

@maribu I have just pushed libcoap v4.3.5rc1 for review.

I have left in the Kconfig stuff for now - certainly users can just modify app.config to get the same results for configuring things.

[Ivrawn1109]

Hi, I may have found a bug (or an issue easy to solve).

I was able to flash both examples for RIOT successfully, each one on a nRF52840DK console. Having libCoAP server started in a Pyterm console:

coaps start
INFO Server IP [fe80::64b3:7881:5633:4d32]
INFO libcoap server ready

While the libCoAP client is activated on another console. There is ping between them. But when I do the following request for resource time (which is defined in server-coap.c): coapc coap://[fe8064b3:7881:5633:4d32]/time

I get: WARN Failed to create options

I don’t know why the URI is not being submitted. This format doesn't work as well: coap://[ip]/time?ticks coap://[ip]/.well-known/core, which it’s specified in README.

Grateful for any clues!

[mrdeep1]

@Ivrawn1109

Looks like this change is needed - I will get it fixed

diff --git a/examples/libcoap-client/client-coap.c b/examples/libcoap-client/client-coap.c
index bdf8db7151..f3a636ac90 100644
--- a/examples/libcoap-client/client-coap.c
+++ b/examples/libcoap-client/client-coap.c
@@ -219,7 +220,7 @@ client_coap_init(int argc, char **argv)
     }

     res = coap_uri_into_optlist(&uri, &dst, &optlist, 1);
-    if (res) {
+    if (res != 1) {
         coap_log_warn("Failed to create options\n");
         goto fail;
     }

[maribu]

It looks like something has gone wrong in the latest rebase, as unrelated commits are now showing up. Could you just do another rebase on top of master and see if that fixes the issue?

[mrdeep1]

Just pulled latest master and rebased my PR against it. git log --oneline master and git log --oneline libcoap_add is only showing 1 commit difference (the addition of libcoap_add).

I see that Murdock has a failure that needs to be fixed, which I will do now.

[crasbe]

Thank you for picking up the PR again. Some things (style related) have changed since the original PR was submitted, so some updates and migrations have to be applied before merging.

The comments I gave are only exemplary, please adapt the remaining files yourself given the patterns.

[mrdeep1]

Is there a way to stop dist/tools/whitespacecheck/check.sh failing on a pkg .patch file where the external library follows a different coding standard, or the git diff adds a leading blank for a no change line?

As in

./dist/tools/whitespacecheck/check.sh x
  Command output:
  
  	pkg/libcoap/patches/0001-RIOT-Handle-ints-being-16-bits.patch:22: trailing whitespace.
  	+ 
  	pkg/libcoap/patches/0001-RIOT-Handle-ints-being-16-bits.patch:34: trailing whitespace.
  	+ 
  	pkg/libcoap/patches/0001-RIOT-Handle-ints-being-16-bits.patch:37: trailing whitespace.
  	+-- 
  	pkg/libcoap/patches/0001-RIOT-Handle-ints-being-16-bits.patch:39: new blank line at EOF.
  	ERROR: This change introduces new whitespace errors
  
  Error: trailing whitespace.
  Error: trailing whitespace.
  Error: trailing whitespace.
  Error: new blank line at EOF.

I have added pkg/*/patches/*.patch in dist/tools/whitespacecheck/ignore_list.txt for now. Doing this removes a significant number of warnings when just running dist/tools/whitespacecheck/check.sh with no parameter.

[Teufelchen1]

@crasbe seems to work fine.

@mrdeep1 Is it expected behavior that coapc does not respect interface ids? e.g. coap://[fe80::1%5]/
I also disliked that the coapc command is silent until timeout. This is an ux issue as someone not familiar with that tool will not understand what is going on while their riot node seemingly has frozen / became non-responsive. A little "trying coap get on " will be enough to fix that.

[mrdeep1]

@Teufelchen1

Is it expected behavior that coapc does not respect interface ids? e.g. coap://[fe80::1%5]/

This works for me provided the interface number is correct (was 5 also for me) and a valid IPv6 address is entered for where a server is running.

Brief Help documentation on usage has now been added to the client and server.

[crasbe]

Second (and last?) review round with more style related comments.