sys: net: add nanocoap #8123

kaspar030 · 2017-11-23T09:35:20Z

This PR removes the nanocoap package and imports the code instead.

haukepetersen · 2017-11-23T09:51:19Z

Awesome, will review later today. So as I was involved in getting the code ready to be PRed, I think it makes sense to have at least one other review...

kaspar030 · 2017-11-23T10:24:05Z

Hm. The imported nanocoap has some (minor) changes. One is that it now doesn't ignore the CoAP host option anymore (see kaspar030/sock#5, kaspar030/sock#22).
That is not a problem per-se, but the (binary) packet used in gcoap's unittests contains that option, thus the unittests fails.

Either we again ignore that option (as in current master, but against specs), or we change the packet in the unittests.

@kb2ma what do you think?

kaspar030 · 2017-11-23T10:29:40Z

Either we again ignore that option (as in current master, but against specs)

I've reinstated this for now, as we should discuss/fix that apart from this integration PR.

aabadie · 2017-11-23T10:56:07Z

one general question: why keeping the name nanocoap ? and not just simply rename to coap ?

kb2ma · 2017-11-23T11:46:34Z

IIRC, the unit test was written from libcoap as it worked at the time. I suggest removing the Uri-Host option from the unit test. I agree it's simplest to ignore Uri-Host and merge nanocoap first, and then fix this issue separately.

haukepetersen · 2017-11-27T11:13:29Z

one general question: why keeping the name nanocoap ? and not just simply rename to coap ?

Because it is good practice to give modules a unique name, independent of what they implement. So say there is a new coap implementation by someone popping up tomorrow, what reasoning should we have to call nanocaop coap, but force the new implementation to have a different name and not make that the coap named implementation?

By keeping unique names for modules (e.g. gcoap, nanocaop, or same for emcute), we allow for multiple implementations of the same protocol(s) without ever getting in trouble naming them...

haukepetersen · 2017-11-27T11:14:20Z

I've reinstated this for now, as we should discuss/fix that apart from this integration PR.

+1

haukepetersen

Mostly picky things. The only non-trivial thing I noticed concerns making nanocaop_sock a sub-module.

haukepetersen · 2017-11-27T11:17:07Z

sys/include/net/nanocoap_sock.h

+int nanocoap_server(sock_udp_ep_t *local, uint8_t *buf, size_t bufsize);
+
+/**
+ * @brief   simple synchronous CoAP get


haukepetersen · 2017-11-27T11:17:38Z

sys/include/net/nanocoap_sock.h

+ * @returns     length of response on success
+ * @returns     <0 on error
+ */
+ssize_t nanocoap_get(sock_udp_ep_t *remote, const char *path, uint8_t *buf, size_t len);


line length

haukepetersen · 2017-11-27T11:21:48Z

sys/net/application_layer/nanocoap/Makefile

@@ -0,0 +1 @@
+include $(RIOTBASE)/Makefile.base


Right now nanocoap_sock.c is always build, but not always needed (e.g. for gcoap), right? I think it would make much sense to make nanocoap_sock a sub-module of nanocoap, so that I must be enabled explicitly by using USEMODULE += nanocoap_sock e.g. in the example application above. Makes sense?

haukepetersen · 2017-11-27T11:23:02Z

sys/include/net/nanocoap.h

+#define COAP_PORT               (5683)
+
+/**
+ * @name    nanocoap specific maximum values


capitalize first word

haukepetersen · 2017-11-27T11:23:41Z

sys/include/net/nanocoap.h

+typedef ssize_t (*coap_handler_t)(coap_pkt_t* pkt, uint8_t *buf, size_t len);
+
+/**
+ * @brief    Type for CoAP resource entry


should be 3 spaces after @brief

haukepetersen · 2017-11-27T11:26:12Z

sys/include/net/nanocoap.h

+ *
+ * This function can be used to create a reply to any CoAP request packet.  It
+ * will create the reply packet header based on parameters from the request
+ * (e.g., id, token).  Passing a nonzero @p payload_len will ensure the payload


I think non-zero is the more common spelling (according to google...)

haukepetersen · 2017-11-27T11:26:38Z

sys/include/net/nanocoap.h

+        unsigned code,
+        uint8_t *buf, size_t len,
+        unsigned ct,
+        const uint8_t *payload, uint8_t payload_len);


haukepetersen · 2017-11-27T11:26:52Z

sys/include/net/nanocoap.h

+ *
+ * @returns      length of resulting header
+ */
+ssize_t coap_build_hdr(coap_hdr_t *hdr, unsigned type, uint8_t *token, size_t token_len, unsigned code, uint16_t id);


line length

haukepetersen · 2017-11-27T11:28:42Z

sys/include/net/nanocoap.h

+ * @param[in]   uri         ptr to source URI
+ * @param[in]   optnum      option number to use (e.g., COAP_OPT_URI_PATH)
+ *
+ * @returns     amount of bytes written to @p buf


Although @returns and @return do the same thing, at least inside a single file we should be consistent in the usage... Further check the indention after the @return(s) statements, we have also two styles in this file...

haukepetersen · 2017-11-27T11:30:32Z

sys/net/application_layer/nanocoap/nanocoap.c

+ * General Public License v2.1. See the file LICENSE in the top level
+ * directory for more details.
+ */
+


No doxygen block in this file. I know it is not parsed anyway, but still useful e.g. for the author tags etc and AFAIK it is standard that we put it in every C file, is it not?

aabadie · 2017-11-27T15:15:00Z

what reasoning should we have to call nanocaop coap, but force the new implementation to have a different name and not make that the coap named implementation?

I think this is a more global question. nanocoap is going in the sys/ of RIOT so for me it should be generic, e.g it should provide and be compatible with CoAP standard with core functionalities (type of messages, error codes, etc) and basic functions. Maybe we could also consider merging gcoap and nanocoap ?
nanocoap was initially designed to be a package on its own, why not providing it on a separate repo on GitHub: nanocoap/nanocoap => this may increase its visibility and it could still be integrated in RIOT as a package. The situation now is different since nanocoap is a subfolder of a personal repository, which gives less visibility IMHO.

kaspar030 · 2017-11-29T13:19:41Z

I think this is a more global question. nanocoap is going in the sys/ of RIOT so for me it should be generic

More generic than what? And where do you get the notion than stuff in "sys/" "should be generic"?

Maybe we could also consider merging gcoap and nanocoap ?

There's been a lot of discussion around this, both online and offline. Short answer: one stays with focus on minimal resource usage, the other re-uses as much code as possible but provides all features.

nanocoap was initially designed to be a package on its own, why not providing it on a separate repo on GitHub: nanocoap/nanocoap => this may increase its visibility and it could still be integrated in RIOT as a package. The situation now is different since nanocoap is a subfolder of a personal repository, which gives less visibility IMHO.

We decided to move it to the main RIOT repo because it sucks to keep PRs in sync with two interdependant repositories.

kaspar030 · 2017-11-29T13:20:34Z

@haukepetersen I think I got all the style comments, and changed _sock to be a submodule.

haukepetersen · 2017-11-30T20:46:12Z

except the missing doxygen blocks in the c-files...

haukepetersen

ACK anyway - fix doxygen blocks in the c files at will, I won't block this PR because of that...

benpicco · 2022-02-24T13:43:44Z

sys/include/net/nanocoap.h

+ * @name    Timing parameters
+ * @{
+ */
+#define COAP_ACK_TIMEOUT        (2U)


Why was such a large timeout value chosen?

From the RFC: https://datatracker.ietf.org/doc/html/rfc7252#section-4.8

benpicco · 2024-01-09T15:28:23Z

sys/net/application_layer/nanocoap/nanocoap.c

+    }
+
+    /* if code is COAP_CODE_EMPTY (zero), use RST as type, else RESP */
+    unsigned type = code ? COAP_RESP : COAP_RST;


What about separate response where we reply with an empty ACK?
This will always turn this into a RST.

haukepetersen requested changes Nov 27, 2017

View reviewed changes

haukepetersen approved these changes Nov 30, 2017

View reviewed changes

haukepetersen added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Nov 30, 2017

kaspar030 added 6 commits December 1, 2017 12:53

pkg: remove nanocoap

9fe9d1f

sys: net: add nanocoap

16bdbe5

examples/nanocoap_server: adapt to in-tree nanocoap

729cda6

sys/net/gcoap: adapt to in-tree nanocoap

7a636cb

sys: net: nanocoap: ignore Uri-Host option

84bc849

sys/net/nanocoap: use submodules

3367840

kaspar030 force-pushed the add_nanocoap branch from 185d925 to 3367840 Compare December 1, 2017 11:56

kaspar030 merged commit 830d968 into RIOT-OS:master Dec 1, 2017

kaspar030 deleted the add_nanocoap branch December 1, 2017 12:25

aabadie added this to the Release 2018.01 milestone Jan 18, 2018

benpicco reviewed Feb 24, 2022

View reviewed changes

benpicco reviewed Jan 9, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sys: net: add nanocoap #8123

sys: net: add nanocoap #8123

kaspar030 commented Nov 23, 2017

haukepetersen commented Nov 23, 2017

kaspar030 commented Nov 23, 2017

kaspar030 commented Nov 23, 2017

aabadie commented Nov 23, 2017

kb2ma commented Nov 23, 2017

haukepetersen commented Nov 27, 2017

haukepetersen commented Nov 27, 2017

haukepetersen left a comment

haukepetersen Nov 27, 2017

haukepetersen Nov 27, 2017

haukepetersen Nov 27, 2017

kaspar030 Nov 29, 2017

haukepetersen Nov 27, 2017

haukepetersen Nov 27, 2017

haukepetersen Nov 27, 2017

haukepetersen Nov 27, 2017

haukepetersen Nov 27, 2017

haukepetersen Nov 27, 2017

haukepetersen Nov 27, 2017

aabadie commented Nov 27, 2017

kaspar030 commented Nov 29, 2017

kaspar030 commented Nov 29, 2017

haukepetersen commented Nov 30, 2017

haukepetersen left a comment

benpicco Feb 24, 2022

kaspar030 Feb 24, 2022

benpicco Jan 9, 2024 •

edited

Loading

sys: net: add nanocoap #8123

sys: net: add nanocoap #8123

Conversation

kaspar030 commented Nov 23, 2017

haukepetersen commented Nov 23, 2017

kaspar030 commented Nov 23, 2017

kaspar030 commented Nov 23, 2017

aabadie commented Nov 23, 2017

kb2ma commented Nov 23, 2017

haukepetersen commented Nov 27, 2017

haukepetersen commented Nov 27, 2017

haukepetersen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aabadie commented Nov 27, 2017

kaspar030 commented Nov 29, 2017

kaspar030 commented Nov 29, 2017

haukepetersen commented Nov 30, 2017

haukepetersen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benpicco Jan 9, 2024 • edited Loading

Choose a reason for hiding this comment

benpicco Jan 9, 2024 •

edited

Loading