net: add full CoRE Resource Directory endpoint implementation by haukepetersen · Pull Request #7428 · RIOT-OS/RIOT

haukepetersen · 2017-07-28T12:14:44Z

Waiting on and rebased on a couple of PRs, e.g. ~~#7402~~, ~~#7406~~, ~~#7407~~, ~~#7408~~

This PR adds complete CoRE Resource Directory (RD) functionality for RIOT. The code should be fully functional and is basically done, though it needs some more documentation and some cleanups (and proper squashing of course). Will take that on once all the dependencies are merged...

PeterKietzmann

I do not intend to provide a full review but rather focus on testing. In a first step I analyzed sent packets via rdcli reg command on native an on a samr21-xpro which looks "good" at first sight, comparing to the RD draft. This applies for both provided applications. With examples/coap_rdcli it appeared that ping6 raises a failed assertion. Not quite sure I got the correct assertion but most likely it is one of these. Is there a GNRC relevant thread without a message queue?
Could you add a description about how to set up an RD server for easier testing of the actual protocol?

PeterKietzmann · 2017-08-03T15:10:19Z

examples/coap_rdcli/README.md

+redefine `RDCLI_SERVER_ADDR` and `RDCLI_SERVER_PORT`, e.g.:
+```
+CFLAGS="-DRDCLI_SERVER_ADDR=IPV6_ADDR_ALL_ROUTERS_LINK_LOCAL" make all
+```


Currently this README equals that of the other application. Would be great to have a short introduction what the difference is.

This is because I have not written an readme for this example, yet :-)

PeterKietzmann · 2017-08-03T15:13:40Z

sys/shell/commands/sc_rdcli.c

+        rdcli_dump_status();
+    }
+    else {
+        printf("usage: %s <reg [addr [port]]|up|rem|info>\n", argv[0]);


I would prefer update as command. up reminds me of initiating something.

PeterKietzmann · 2017-08-03T15:16:13Z

sys/net/application_layer/rdcli/rdcli.c

+    sock_udp_ep_t remote;
+} rd_entry_t;
+
+static rd_entry_t active_rd = { 0 };


I'm using a rather old compiler. arm-none-eabi-gcc 4.9.3 complains error: missing braces around initializer

PeterKietzmann · 2017-08-03T15:17:39Z

sys/net/application_layer/rdcli/rdcli.c

+    /* build CoAP request packet */
+    int res = gcoap_req_init(&pkt, buf, sizeof(buf), COAP_METHOD_POST, "/rd");
+    if (res < 0) {
+        goto rdcli_register;


Didn't we decide against "goto" some time ago?

AFAIK not in general, and though I am still not a huge fan of this, I think for certain error handling cases it can make the code more readable, if used with caution...

haukepetersen · 2017-08-04T08:21:04Z

With examples/coap_rdcli it appeared that ping6 raises a failed assertion

Yepp, thats the missing message queue (once more)... Will fix.

Could you add a description about how to set up an RD server for easier testing of the actual protocol?

quick version: compile nanocoap and run the coap-rd command, then watch with wireshark. But I have not really played with the lookup API (if it is even implenented in libcoap) to see if entries are made correctly.

Alternative: You could also use the javscript RD Pekka wrote in Dagstuhl (https://github.com/Ell-i/coap-rd):

install node 8.x on your host
run it using node coap-rd
watch the debug output...

haukepetersen · 2018-05-09T08:14:43Z

just as update: currently waiting on #8920

haukepetersen · 2018-09-12T09:53:06Z

finally, no more dependencies to wait for. Heavily rebased, and partly reworked this PR, now it is ready for (final) review and testing.

I tested this mainly by registering it with the RD provided by aiocoap (https://github.com/chrysn/aiocoap). For this I always used the latest master of aiocoap-rd, as the latest release does not include many features of the latest draft version...

haukepetersen · 2018-09-14T15:06:50Z

rebased and fixed some issues that Murdock pointed out.

haukepetersen · 2018-09-26T07:52:31Z

@bergzand @PeterKietzmann anyone available to test and review this PR?!

PeterKietzmann · 2018-09-27T12:18:43Z

Unfortunately I won't find time for this soon. @pokgak are you familiar with the idea of "Resource Directory"? Isn't this somewhat interesting for you?

pokgak · 2018-09-27T15:42:57Z

Perfect timing. Currently reading on this topic to use it in my project. Will test it next week and provide feedback.

haukepetersen · 2018-09-28T09:02:37Z

@pokgak awesome to hear, looking forward to your feedback!

bergzand · 2018-09-28T09:32:26Z

@haukepetersen I hope to have some time for a review next week somewhere.

pokgak · 2018-10-02T11:39:39Z

Tested this using native with PORT=tap0, aiocoap-rd bind to global address of tapbr0 and Wireshark listens on tap0.

Here is what I've tested:

Running discover, reg, update, info, rem directly after each other work as expected OK
rdcli update and rdcli rem when not registered does not do anything. OK
Client updates automatically once before the lifetime is over, but not anymore after that.
Steps to reproduce:
- rdcli reg [rd_ip], then wait and see wireshark log
- will auto update once, after that no more
Even when the lifetime is over, rdcli info still says that it is registered
Step to reproduce:
- rdcli reg [rd_ip] and wait until lifetime over
- rdcli info still says registered to RD
Calling update after lifetime is over make program hang
Step to reproduce:
- rdcli reg [rd_ip] and wait until lifetime over. Note: the client auto updates once, so wait at least 2 * lifetime
- rdcli update and program hangs
Registering to new RD, while already registered to another, the old registration will be deleted even though the new registration failed. I could not find about this in the draft.

Feedback:

When using discover/reg with random ip, it takes too long until the shell command returns. Maybe more verbose output like ping6 helps, so that the user does not think that the program hangs somewhere
Output of rdcli rem when not registered can be misleading. It says node successfully removed from RD when there are actually no nodes removed.

pokgak · 2018-10-11T13:29:18Z

ACK from my side =)

haukepetersen · 2018-10-12T07:39:55Z

@pokgak very nice, thanks for the testing!

@bergzand I will squash now, hope thats ok with you...

bergzand · 2018-10-12T08:09:17Z

@bergzand I will squash now, hope thats ok with you...

Go ahead, I didn't have time to look at this yet unfortunately :(

haukepetersen · 2018-10-12T10:24:18Z

rebased and squased.

haukepetersen · 2018-10-12T10:26:38Z

@bergzand no problem. But maybe you want to give a proxy-ACK for @pokgak? :-)

haukepetersen · 2018-10-12T10:36:04Z

blacklisted boards with insufficient memory

haukepetersen · 2018-10-12T11:51:57Z

fixed a parenthesis issues that clang did not like...

haukepetersen · 2018-10-12T12:42:03Z

more blacklisting...

jia200x · 2018-10-12T14:55:42Z

as far as I see this PR has chances to be merged for the Release, right?

miri64

I scanned through the code and didn't find anything obviously wrong with it and moreover @pokgak gave this a review as well. Regarding testing I know (and @chrysn probably can confirm) that this implementation already went through 2 plugtests against other implementations (also @haukepetersen gave me a little demo offline as well).

Let's get this into the release!

ACK.

miri64 · 2018-10-12T15:55:56Z

Codacy has a problem, but I don't see what it is (it just tells "1 warning" but not showing the actual warning). To my knowledge however, Murdock is still the canonical check (and also the only required one).

miri64 · 2018-10-12T15:56:01Z

So go!

haukepetersen · 2018-10-12T16:01:49Z

Finally :-) Thanks everyone!

kb2ma · 2018-10-12T16:44:28Z

Nice, Hauke! I just updated the Standards Based Application Development wiki page so hopefully the implementation and apps will be easy to find.

waehlisch · 2018-10-14T20:09:46Z

But we still should work on aligning naming with the RFC.

jia200x · 2018-10-15T07:35:42Z

awesome! Thanks to everyone involved! 🎉

haukepetersen · 2018-10-15T10:11:37Z

But we still should work on aligning naming with the RFC.

done: #10163

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

PeterKietzmann reviewed Aug 3, 2017

View reviewed changes

haukepetersen mentioned this pull request Mar 19, 2018

net/gcoap: allow to not define a response handler #8800

Merged

kb2ma mentioned this pull request Mar 27, 2018

net: added CoAP RD client for simple node registration #7406

Merged

bergzand added the Area: CoAP Area: Constrained Application Protocol implementations label May 25, 2018

haukepetersen mentioned this pull request Aug 17, 2018

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

Merged

haukepetersen force-pushed the add_rdcli branch from b46fc94 to e56292b Compare September 12, 2018 09:49

bergzand self-requested a review September 12, 2018 15:39

haukepetersen mentioned this pull request Sep 13, 2018

coap-rd: update process fails obgm/libcoap#254

Closed

haukepetersen force-pushed the add_rdcli branch from e56292b to e1e1e37 Compare September 14, 2018 15:06

haukepetersen force-pushed the add_rdcli branch from e36d109 to 55a7936 Compare October 12, 2018 10:24

haukepetersen force-pushed the add_rdcli branch from 71b658c to 0bb69df Compare October 12, 2018 11:51

haukepetersen added 6 commits October 12, 2018 14:41

net/rdcli_common: added RES_LOC_LEN

314dfcb

net: added full RD endpoint implementation

9acce40

sys/auto_init: added rdcli_standalon initialization

49f21b8

shell: add shell command for rdcli

899d7fe

examples: add example for rdcli

c9c7bf8

examples/rdcli: blacklist low memory boards

cba288b

haukepetersen force-pushed the add_rdcli branch from 0bb69df to cba288b Compare October 12, 2018 12:41

miri64 approved these changes Oct 12, 2018

View reviewed changes

miri64 merged commit 1095ac2 into RIOT-OS:master Oct 12, 2018

miri64 deleted the add_rdcli branch October 12, 2018 15:56

miri64 mentioned this pull request Oct 12, 2018

RFC: CoAP release specs RIOT-OS/Release-Specs#73

Closed

haukepetersen mentioned this pull request Oct 15, 2018

net/rdcli*: restructure and rename RD related modules #10163

Merged

Conversation

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

Uh oh!

Uh oh!

PeterKietzmann left a comment

Choose a reason for hiding this comment

Uh oh!

PeterKietzmann Aug 3, 2017

Choose a reason for hiding this comment

Uh oh!

haukepetersen Aug 7, 2017

Choose a reason for hiding this comment

Uh oh!

PeterKietzmann Aug 3, 2017

Choose a reason for hiding this comment

Uh oh!

haukepetersen Aug 7, 2017

Choose a reason for hiding this comment

Uh oh!

PeterKietzmann Aug 3, 2017

Choose a reason for hiding this comment

Uh oh!

PeterKietzmann Aug 3, 2017

Choose a reason for hiding this comment

Uh oh!

haukepetersen Aug 7, 2017

Choose a reason for hiding this comment

Uh oh!

haukepetersen commented Aug 4, 2017

Uh oh!

haukepetersen commented May 9, 2018

Uh oh!

haukepetersen commented Sep 12, 2018

Uh oh!

haukepetersen commented Sep 14, 2018

Uh oh!

haukepetersen commented Sep 26, 2018

Uh oh!

PeterKietzmann commented Sep 27, 2018

Uh oh!

pokgak commented Sep 27, 2018

Uh oh!

haukepetersen commented Sep 28, 2018

Uh oh!

bergzand commented Sep 28, 2018

Uh oh!

pokgak commented Oct 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pokgak commented Oct 11, 2018

Uh oh!

haukepetersen commented Oct 12, 2018

Uh oh!

bergzand commented Oct 12, 2018

Uh oh!

haukepetersen commented Oct 12, 2018

Uh oh!

haukepetersen commented Oct 12, 2018

Uh oh!

haukepetersen commented Oct 12, 2018

Uh oh!

haukepetersen commented Oct 12, 2018

Uh oh!

haukepetersen commented Oct 12, 2018

Uh oh!

jia200x commented Oct 12, 2018

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

miri64 commented Oct 12, 2018

Uh oh!

miri64 commented Oct 12, 2018

Uh oh!

haukepetersen commented Oct 12, 2018

Uh oh!

kb2ma commented Oct 12, 2018

Uh oh!

waehlisch commented Oct 14, 2018

Uh oh!

haukepetersen commented Jul 28, 2017 •

edited

Loading

pokgak commented Oct 2, 2018 •

edited

Loading