-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
gcoap: rebase networking on sock #6117
Conversation
I probably can find this out reading the code, but can you also highlight how this approach now differs from using pure nanocoap+sock? AFAIK it also brings client and server functionalities with it, so what does this implementation now bring that the other solution does not. |
Rebased and compressed commits. Updated the overview documentation to focus on gcoap features . For reviewers, the core changes are in commit eb6b87e. You'll see the documentation updates in gcoap.h, and the implementation changes in gcoap.c. Thanks for asking about gcoap vs. nanocoap. This is similar to the issue #6038 that @cgundogan raised about the number of CoAP examples. So, maybe we can reuse this response there. @kaspar030, please jump in if I have described nanocoap incorrectly. I'm just trying to provide a clear comparison. :-) The description below is copied from the gcoap Status page. It's kind of long -- focus on the bullets if it's too much. FeaturesIn general, gcoap provides a higher-level interface to CoAP than nanocoap. The main differences are gcoap's single CoAP messaging hub approach, and also a consistent, well-documented API for client requests and server responses. Single CoAP messaging hub:
In other words, gcoap provides a single, reusable access point for CoAP messaging. On the server side, I was inspired by the Python microframeworks like Bottle or Flask. gcoap provides gcoap_register_listener() for an application to register resources. In contrast, as a server nanocoap appears to be designed for a server per application. All resource callbacks must be specified within a single const array in the nanocoap_server example in RIOT. On the client side, gcoap tracks a configurable number of open requests in an array. In contrast, nanocoap is focused on handling one request at a time in the nanocoap_get() function in nanocoap_sock.c. gcoap's timeout capability for a client request provides for lightweight confirmable messaging without specifically supporting the CON message type yet. nanocoap already provides CON messaging. Simple high-level API
The contrast between gcoap and nanocoap APIs is not that great. However, gcoap is well-documented in the RIOT Modules>Networking>CoAP source documentation (moved there in this PR). nanocoap recently added a coap_reply_simple(), which also writes the content format option. |
You summed it up pretty well. ;) I designed nanocoap to be pretty minimal. So it'll never have any My plans for the future of nanocoap are (so far):
|
I refined the introductory documentation in gcoap.h, squashed, and rebased. I also have been testing the implementation, and it works as expected. I think it's ready to merge, so I removed the WIP in the title. Please remove that label, too. |
@miri64, is there an issue blocking merge of this PR? FWIW, I created some functional tests with Pexpect to convince myself that gcoap works with ongoing client and server exchanges. |
At the moment just time to review it. Will try to find some. |
* @return < 0 on error | ||
*/ | ||
int gcoap_req_init(coap_pkt_t *pdu, uint8_t *buf, size_t len, unsigned code, | ||
char *path); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here and below: I don't understand your indentation pattern. But since we seem to have no convention about this, I'm fine in general
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main goal is readability. If the declaration extends beyond approx. 80 chars, I right-align the following parameters below the last one on the first line. I think this block-like look is easier to read than wrapping the following parameters starting under the first parameter on the first line.
#ifdef __cplusplus | ||
} | ||
#endif | ||
#include "net/gcoap.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this file still required? I would completely remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, coap.h is not required any longer -- it's only there for backwards compatibility. I didn't want to break code for existing users out of the box. I included a 'deprecated' comment above, but I'm happy to remove the file. I don't expect that there are hordes of gcoap users, but is there a policy on when to deprecate?
FWIW, for the same reason I also retained gcoap_req_send(), which uses an ipv6_addr_t and an int port for the destination. I created gcoap_req_send2(), which uses a sock_udp_ep_t. Let me know if I should make any changes there.
fe53fd2
to
ef2d660
Compare
@miri64, I removed net/gnrc/coap.h in commit ef2d660. I'm glad you brought up this subject. You'll see in that commit that I also changed several #include's that referenced the old header file. I should have done that with the original commits. I also would appreciate any insights on deprecation policy, as I mentioned above in my inline responses to your review. Maybe it would be a better question for the riot-devel list. |
Still WIP? Otherwise, I'm fine with the code and want to test sometime this week (most likely at the interim H&A on Wednesday). |
Glad to hear it! No longer WIP, as mentioned above. I'll keep in mind that you might be testing Wednesday so I can respond quickly if questions arise. |
@miri64 any news? |
Sorry, didn't think about testing this at H'n'A. Will do, but needs rebase ;-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was able to send GET requests and receive replies from another examples/gcoap
application. Since this only changes how gcoap accesses GNRC this is sufficient enough for me
Excellent! In a few hours I will rebase and verify all is well. Unless I hear otherwise, I do not plan to squash the latest commit, which removes the obsolete coap.h include file. Leaving this commit will help make the change explicit to any current gcoap users. |
Also change references in dependencies to the net/gcoap.h include.
Murdock and CI are happy now. Rebased, fixed and squashed a few issues:
|
and go |
gcoap has been based on the GNRC stack, but sock is the preferred, stack-agnostic API for applications. This PR updates gcoap to use it.
Highlights:
_event_loop()
to create a remote-less sock. Listen on this sock indefinitely if not expecting a response. Otherwise, periodically poll for an IPC msg to timeout waiting for a response. I left a debug message in_listen()
for now to make it easy to see whensock_udp_recv()
is called.gcoap_req_send2()
, with the sock remote endpoint parameter. Post a msg to the sock mbox to interrupt listening, to set the response timeout.gcoap_req_send()
now is deprecated, and simply creates the sock endpoint from the provided address and port, and forwards togcoap_req_send2()
.sys/net
. Moved and renamedcoap.h
tonet/application_layer/coap/gcoap.h
. Keptnet/gnrc/application_layer/coap/coap.h
as a shim that references the new header file. I need to mark it as deprecated though.Let me know if there is a better way to reach the goal. Otherwise, it needs a little polishing and documentation update. I also need to figure out what the 'g' in gcoap stands for now. :-)
Thanks to @miri64 for the reference in the code docs to her thesis and defense slides. They helped a lot to understand the networking stack rationale.