net: added CoAP RD client for simple node registration by haukepetersen · Pull Request #7406 · RIOT-OS/RIOT

haukepetersen · 2017-07-25T14:43:30Z

~~based on #7402~~
also needs

This PR provides a CoAP resource directory (RD) client implementation, that uses the simplified client registration as described in section 5.3.2 in draft-ietf-core-resource-directory-11. The client implementation can be used in two different ways:

standalone (USEMODULE += rdcli_simple_standalone): in this mode the module is running in its own thread, which is started automatically on system startup via auto-init. In this mode, nothing has to be done by the user.
'manual' (USEMODULE += rdcli_simple): in this mode, the user has to take care of calling rdcli_simple_register() for initial registration and then periodically or on certain events to refresh the registration and to reset the lifetime counter in the resource directory.

The rdcli_simple client is statically configured through the sys/net/include/rdcli_config.h header.

smlng · 2017-07-27T09:44:12Z

just a quick one on first sight: I find the naming (rdcli) a bit vague, i.e. its the CoAP Resource Directory, hence I would expect coap to be part of the name/prefix?

haukepetersen · 2017-07-27T11:24:32Z

but then the name gets too long... Also, I just saw, the official name is CoRE Resource Directory, so the natural name would be corerdcli?! But a quick google search for resource directory points very reliable to the CoRE RD, so IMHO it is fine to omit the CoRE from the name of the module and stick to rdcli. I could however change the name of the example project to something more meaningful...

smlng · 2017-07-27T11:28:26Z

mhm, I thought RD is intrinsically connected to/with CoAP - but as I just read, its not [;] CoAP is just one way to access resources. So never mind, leave naming as is.

haukepetersen · 2017-07-27T12:13:27Z

awesome. Would you be inclined to maybe have a look at the underlying PR by any chance?!

bergzand · 2017-07-27T15:01:24Z

sys/net/application_layer/rdcli_simple/rdcli_simple_standalone.c

+    (void)arg;
+
+    /* wait some seconds to give the address configuration some time to settle */
+    xtimer_sleep(3);


Isn't more useful to have a define for this delay? Are there situations where 3 seconds is insufficient?

Probably yes. Will add this.

haukepetersen · 2017-08-28T12:41:46Z

startup delay is now configurable. Still waiting for kaspar030/sock#17, though.

haukepetersen · 2017-09-06T13:59:12Z

kaspar030/sock#17 was merged, so rebased and updated the nanocoap pkg

haukepetersen · 2017-09-07T13:02:20Z

Did some additional enhancements and fixes:

removed all dependencies for hwrng from example application
improved resources in example
fixed doxygen in many places
fixed gcoap to accept NULL as response handler
renamed example to rdcli_simple

bergzand · 2017-10-08T11:15:16Z

Is there any advantage to having the separate gcoap RD examples instead of integrating them in the gcoap example? As a test, I think it is useful to have a minimal CoAP RD test, but I think it would be better to have the example integrated in the gcoap example.

haukepetersen · 2017-10-09T08:25:24Z

Yes, so far it's separation of concerns... The example of introduced by this PR is to demonstrate the simplified registration as most likely used by very minimal setups, and this is IMHO best demonstrated by a minimal demo application. I would then rather integrate the full RD client as part of the gcoap example...

bergzand

I've added a number of remarks based on a scan through the code. Example looks and works good.

bergzand · 2017-10-28T18:39:36Z

Makefile.dep

+
+ifneq (,$(filter rdcli_common,$(USEMODULE)))
+  USEMODULE += luid
+  USEMODULE += fmt


wouldn't it be better if the rdcli_common module depends on gcoap? It is the first module in this module dependency string that requires coap.

bergzand · 2017-10-28T18:42:15Z

examples/rdcli_simple/Makefile

+USEMODULE += shell_commands
+USEMODULE += ps
+
+FEATURES_REQUIRED += periph_hwrng


I'm don't see why a hwrng required for this example.

ups, leftover from a previous example, will drop

bergzand · 2017-10-28T18:47:52Z

examples/rdcli_simple/main.c

+ * @{
+ *
+ * @file
+ * @brief       CoRE RD simple registration test application


Only a minor remark, but maybe it is possible to add here that this is a CoAP CoRE RD example, as this example is a CoAP specific example of the CoRE RD rfc.

bergzand · 2017-10-29T09:28:26Z

sys/net/application_layer/rdcli_simple/rdcli_simple.c

+        return res;
+    }
+    /* finish, we don't have any payload */
+    ssize_t len = gcoap_finish(&pkt, 0, COAP_FORMAT_TEXT);


Looking at the rfc draft section 5.3.2, this should be COAP_FORMAT_LINK.

haukepetersen · 2017-10-30T11:03:55Z

@bergzand addressed your comments, thanks for the review!

smlng · 2018-01-16T19:50:14Z

please rebase, also nanocoap is now part of RIOT, i.e. no external package anymore.

haukepetersen · 2018-03-26T07:29:12Z

reabsed and added the RD server address configuration to the example's Makefile.

@bergzand can I squash?

bergzand · 2018-03-26T07:37:15Z

ACK

@bergzand can I squash?

Yes, I'd say this is ready to get merged unless somebody else wants to start nitpicking over it at this point :)

kaspar030

minors, feel free to directly squash

kaspar030 · 2018-03-26T15:02:17Z

sys/net/application_layer/rdcli_simple/rdcli_simple_standalone.c

+#define PRIO                (THREAD_PRIORITY_MAIN - 1)
+#define TNAME               "rdcli_simple"
+
+char stack[STACKSIZE];


kaspar030 · 2018-03-26T15:04:14Z

sys/net/application_layer/rdcli_common/rdcli_common.c

+    }
+
+    luid_get(luid, sizeof(luid));
+    for (unsigned i = 0; i < sizeof(luid); i++) {


there's a fmt_bytes_hex now

kb2ma · 2018-03-26T21:58:25Z

@haukepetersen, @bergzand: What did you use as an RD server for development/testing?

kb2ma · 2018-03-27T14:44:05Z

@haukepetersen, @bergzand: What did you use as an RD server for development/testing?

Never mind. I see Hauke's comment in #7428 on @pekkanikander's JavaScript-based server.

kb2ma · 2018-03-27T15:10:30Z

I haven't actually tested the implementation but have a couple of comments. Overall, I think this work is a nice addition. A resource directory seems like a worthwhile service to reduce the burden on a constrained network.

I understand the need for flexibility at this point in the implementation. Let the user decide whether to run the registration themselves or in an external thread. However, registration management seems like an infrastructure task rather than something an application developer should care about in detail. gcoap itself seems well suited to this work. It runs in its own thread and is designed as a kind of messaging hub. In the future, it can provide resource registration as an optional service.

gcoap already has the gcoap_register_listener() function to trigger registration. Then it can use its thread message handler loop to react to a timer to re-register based on lifetime. Maybe this work could be the beginning of a plug-in mechanism for optional CoAP features.

Just to be clear, I'm envisioning gcoap-based registration as future work, and not blocking this PR. :-)

haukepetersen · 2018-04-06T09:40:17Z

rebased and squashed.

@kaspar030 fixed your remarks and merged them right in

@kb2ma Thanks for your thoughts! I think what you sketch makes a lot of sense, and reusing the gcoap's thread for this kind of tasks seems like a very valid approach. Being able to integrate this RD implementation into a 'larger' scope is exactly the reason, why I factored out the thread-stuff into an optional sub-module. Once this PR is merged, I will bring the 'full' RD client PR (#7428) back in a good shape. I would think that that PR is suited well to filter out the exact requirements on gcoap for running additionally 'CoAP based services'. So lets see were we can take that.

kaspar030 · 2018-04-09T08:17:28Z

ACK from my side. @bergzand all yours!

haukepetersen · 2018-04-09T08:37:45Z

fixed header guards and sqqashed.

bergzand · 2018-04-09T08:53:36Z

@haukepetersen Could you please add the failed boards from Murdock to the examples BOARD_INSUFFICIENT_MEMORY list? Feel free to keep it squashed :)

haukepetersen · 2018-04-09T09:05:52Z

done

haukepetersen · 2018-04-09T09:46:17Z

Travis seems to have a point here, do not merge yet!

haukepetersen · 2018-04-09T09:53:23Z

nope, false alarm, defining a custom value for RDCLI_EP does work as expected, I merely had some trouble escaping the " chars locally.

So until somebody can explain why Travis complains about RDCLI_EP being void, I'd say its save to merge this PR...

haukepetersen · 2018-04-09T10:30:22Z

@bergzand what do you say, should we merge?

bergzand · 2018-04-09T11:25:46Z

Murdock is not complaining about it, so I blame the older cppcheck from travis.

haukepetersen added Area: network Area: Networking Type: new feature The issue requests / The PR implemements a new feature for RIOT State: waiting for other PR State: The PR requires another PR to be merged first labels Jul 25, 2017

haukepetersen added this to the Release 2017.10 milestone Jul 25, 2017

haukepetersen assigned aabadie, kb2ma, kaspar030 and kYc0o Jul 25, 2017

haukepetersen force-pushed the add_rdcli_simple branch from 0026499 to b082539 Compare July 27, 2017 09:14

bergzand reviewed Jul 27, 2017

View reviewed changes

haukepetersen mentioned this pull request Jul 28, 2017

net: add full CoRE Resource Directory endpoint implementation #7428

Merged

haukepetersen force-pushed the add_rdcli_simple branch from 8fe48f2 to da7ca54 Compare August 28, 2017 12:37

haukepetersen force-pushed the add_rdcli_simple branch 2 times, most recently from 4eb2070 to ea2af21 Compare September 6, 2017 13:59

haukepetersen removed the State: waiting for other PR State: The PR requires another PR to be merged first label Sep 6, 2017

bergzand requested changes Oct 29, 2017

View reviewed changes

haukepetersen force-pushed the add_rdcli_simple branch from 91fead5 to 6b4af09 Compare October 30, 2017 11:03

smlng modified the milestones: Release 2017.10, Release 2018.01 Nov 16, 2017

kaspar030 requested changes Mar 26, 2018

View reviewed changes

haukepetersen force-pushed the add_rdcli_simple branch 2 times, most recently from 96703af to a0373a5 Compare April 6, 2018 09:33

haukepetersen added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed State: waiting for other PR State: The PR requires another PR to be merged first labels Apr 9, 2018

kaspar030 approved these changes Apr 9, 2018

View reviewed changes

bergzand approved these changes Apr 9, 2018

View reviewed changes

haukepetersen added 3 commits April 9, 2018 10:37

net/rdcli: added common rdcli module

f12d85d

net: added RD client for simplified registration

1082846

examples: added rdcli_simple example application

fb4b622

haukepetersen force-pushed the add_rdcli_simple branch from a0373a5 to fb4b622 Compare April 9, 2018 08:37

examples/rdcli_simple: blacklisted boards for RAM

544ca46

bergzand merged commit 4b210bb into RIOT-OS:master Apr 9, 2018

haukepetersen deleted the add_rdcli_simple branch April 9, 2018 16:33

haukepetersen mentioned this pull request Apr 10, 2018

net/rdcli_common: fix EP name generation #8905

Merged

haukepetersen mentioned this pull request Apr 19, 2018

net/rdcli: fix doxygen groups for RD client code #8985

Merged

Conversation

haukepetersen commented Jul 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smlng commented Jul 27, 2017

Uh oh!

haukepetersen commented Jul 27, 2017

Uh oh!

smlng commented Jul 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

haukepetersen commented Jul 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

haukepetersen commented Aug 28, 2017

Uh oh!

haukepetersen commented Sep 6, 2017

Uh oh!

haukepetersen commented Sep 7, 2017

Uh oh!

bergzand commented Oct 8, 2017

Uh oh!

haukepetersen commented Oct 9, 2017

Uh oh!

bergzand left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

haukepetersen commented Oct 30, 2017

Uh oh!

smlng commented Jan 16, 2018

Uh oh!

haukepetersen commented Mar 26, 2018

Uh oh!

bergzand commented Mar 26, 2018

Uh oh!

kaspar030 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kb2ma commented Mar 26, 2018

Uh oh!

kb2ma commented Mar 27, 2018

Uh oh!

kb2ma commented Mar 27, 2018

Uh oh!

haukepetersen commented Apr 6, 2018

Uh oh!

kaspar030 commented Apr 9, 2018

Uh oh!

haukepetersen commented Apr 9, 2018

Uh oh!

bergzand commented Apr 9, 2018

Uh oh!

haukepetersen commented Apr 9, 2018

Uh oh!

haukepetersen commented Apr 9, 2018

haukepetersen commented Jul 25, 2017 •

edited

Loading

smlng commented Jul 27, 2017 •

edited

Loading