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

GNRC implementation of CoAP #5598

Merged
merged 3 commits into from Nov 1, 2016

Conversation

@kb2ma
Member

kb2ma commented Jul 4, 2016

See README.md in examples/gcoap for an overview. Includes a CLI coap command.

Why a GNRC implementation when libcoap and microcoap are out there? Well, it started for personal reasons, as an experiment with the network stack, and I have kept on hacking at it. Certainly CoAP is a key IoT protocol. IMO it's worth the effort to have a well-documented LGPL implementation based on RIOT's messaging architecture, that can be tuned to the project's needs.

To make the implementation more useful, I plan to implement Observe soon. If others agree the the goal is worthwhile and the work looks promising, I'll add unit tests for what I have so far.

@kb2ma kb2ma changed the title from WIP for a GNRC implementation of COAP to WIP for a GNRC implementation of CoAP Jul 5, 2016

@BytesGalore

This comment has been minimized.

Member

BytesGalore commented Jul 5, 2016

@kb2ma nice one, I remember a discussion (but no details) with @smlng some Hack'n'ACKs ago about the shortcomings of our used *CoAP packages.
@smlng could be interested in this one.

@gebart

This comment has been minimized.

Member

gebart commented Jul 5, 2016

I certainly think it is worthwhile if we get a RIOT coap implementation which uses the riot IPC to make it easier to get started on IoT projects.
What is the current state of this implementation? Does it provide both server and client?
Block1, block2?

@kb2ma

This comment has been minimized.

Member

kb2ma commented Jul 5, 2016

Thanks for the feedback. @gebart, the implementation includes client and server, but I have focused mostly on client side so far. No block support yet. RIOT's generic IP fragmentation makes block less pressing, although I have not compared the per-packet payload size between those two.

Here is a summary of what's available. I would describe it as minimally useful.

  • Server listens on a port, and provides a callback for generation of an application-specific response.
  • Client uses gnrc_coap_send() for a request, and listens for a response via a callback. Matches response on IP port and CoAP token.
  • Message Type: Supports non-confirmable (NON) messaging.
  • Options: Supports URI-Path (ASCII, no percent-encoding) and Content-Format for payload.
@gebart

This comment has been minimized.

Member

gebart commented Jul 5, 2016

In my opinion there are two issues with relying on the 6lo fragmentation for block transfers: the size is limited to the maximum size of a IP packet, <1280 bytes, and if one fragment is lost, the whole packet needs to be retransmitted, unless other methods such as the link-layer retransmissions manage to catch the missing fragment frame.
Using a block transfer is usually more costly in terms of bytes total for the message, but more reliable.

@kb2ma

This comment has been minimized.

Member

kb2ma commented Jul 17, 2016

I have posted the slides and accompanying audio overview of gcoap that I presented Saturday at the Web and IoT discussion for the RIOT Summit.

@OlegHahm

This comment has been minimized.

Member

OlegHahm commented Jul 17, 2016

@kb2ma, thanks a lot!

@gebart

This comment has been minimized.

Member

gebart commented Jul 17, 2016

@kb2ma thank you for recording and posting your presentation! I wish I had been able to attend the summit. I will listen to this later tonight!

@OlegHahm

This comment has been minimized.

Member

OlegHahm commented Jul 17, 2016

@kb2ma, can you point me to the function/data structure one needs to implement in order to generate a resource and give it a name?

@kb2ma

This comment has been minimized.

Member

kb2ma commented Jul 18, 2016

@OlegHahm, good to hear from you! It sounds like you want to implement a server, and register a resource to some path. I have not implemented this registration, but gcoap does have a proto-solution.

All of the structs for gcoap are defined in coap.h, and there is some introductory documentation at the top of that file. See especially the Server Use section. The 'server struct' referenced there is gnrc_coap_server_t.

So, the proto-solution is a callback that allows a user of gcoap to implement their own resource server. A super-simple server could be implemented in a single function. See the gcoap example for a server that only provides an empty response to /.well-known/core. Now that's simple. :-) The actual resource transferred is the gnrc_coap_transfer_t.

At this point, anything more sophisticated is up to you. To date, I personally have been more focused on the client side of CoAP. However, I am starting on the goal of implementing the Observe protocol. For a working example, I plan to start with your netstats work (yes!) as a resource to serve, as I am interested in network monitoring.

This area of resources is tricky, and really server-specific. Certainly we don't want unnecessary storage at the CoAP layer. At a high level, what are your expectations from a server for resource registration -- pass a pointer, a callback, store some data?

Monday is RIOT day for me this week, so let me try to come up with some concrete ideas for serving up the netstats data.

coap_type = (hdr->ver_type_tkl & 0x30) >> 4;
msg_meta->tokenlen = hdr->ver_type_tkl & 0x0F;
if (coap_ver != GNRC_COAP_VERSION
|| coap_type != GNRC_COAP_TYPE_NON

This comment has been minimized.

@OlegHahm

OlegHahm Jul 18, 2016

Member

So, only non-confirmable messages are supported right now?

@smlng

This comment has been minimized.

Member

smlng commented Jul 18, 2016

@OlegHahm: yes, only non-con - as Ken mentioned above:

Message Type: Supports non-confirmable (NON) messaging.

and during his presentation at the summit.

@OlegHahm

This comment has been minimized.

Member

OlegHahm commented Jul 18, 2016

Oops, sorry, too little sleep during the last days. ;)

@kb2ma

This comment has been minimized.

Member

kb2ma commented Jul 18, 2016

No worries, @OlegHahm, and thanks for stepping in, @smlng. Yes, this PR definitely is WIP. I have added timeouts in my working branch, which are essential for confirmable messaging. BTW, is there a best practice for refreshing the PR? I was planning to wait until I have some significant feature ready.

My thinking on confirmable has been that any failures in the mesh mean that some link layer retries already failed. Besides, if the message is really important, an app needs a fallback strategy anyway, in case confirmable retries fail, too. I definitely agree though that the ACK confirmation is nice to have.

@gebart

This comment has been minimized.

Member

gebart commented Jul 18, 2016

@kb2ma while you have the WIP tag on the PR you should be able to push whatever you want to whenever you want to. It's always nice to see things moving ahead. You should probably rebase regularily while working on it to stay up to date with the master branch.
When you are ready for a review you should let us know in the comments and edit the title to remove the "WIP". After that you should avoid squashing without first asking the reviewers that it is OK.

@kaspar030

This comment has been minimized.

Contributor

kaspar030 commented Jul 18, 2016

I must say that I'm not sure the tight coupling of CoAP protocol handling and gnrc is good design. A proper CoAP library should sit below any gnrc layer.

(I'm also working on a CoAP library here [1], functionality wise it's not further than this PR.)

[1] https://github.com/kaspar030/sock/tree/master/nanocoap

@kb2ma

This comment has been minimized.

Member

kb2ma commented Jul 19, 2016

Thanks for the feedback and link to nanocoap, @kaspar030. Have you defined features and timeline for it? Are you planning to add Observe, Confirmable, or Block in the near term?

@gebart

This comment has been minimized.

Member

gebart commented Jul 19, 2016

@kaspar030 @kb2ma you should join forces, and try to unify your implementations.

I think using conn for the network layer interactions makes sense, CoAP is a session layer protocol (OSI) and it makes sense to try to use the abstractions provided by conn or sock (hopefully, conn and sock will be the same thing after #5533 is resolved)

@OlegHahm

This comment has been minimized.

Member

OlegHahm commented Jul 20, 2016

While I agree with @kaspar030 and @gebart on the one hand (putting CoAP on top of conn/sock/sockets increases flexibility a lot), I see problems with that:

  1. You would have either to wait until our discussion in #5533 has concluded (and maybe provide input) or use the current conn interface and update it later.
  2. GNRC has IMO a very nice interface and sticking to it may lead to a better design in the end.

Moreover, CoAP cannot only run over UDP, but a plethora of protocols (the two craziest things I heard about were CoAP-over-SMS and CoAP-over-WebRTC). Hence, netapi might be a better match to enable this flexibility than conn.

@kb2ma

This comment has been minimized.

Member

kb2ma commented Jul 20, 2016

I agree that refitting this PR to work on conn/sock is hung up on #5533. Having said that, I expect refitting will be easier if conn/sock adds an async event loop, as is being discussed now. For my implementation, this feature (plus message passing) is the main appeal of GNRC. I can live with the ambiguity for now, and will see how the other PR resolves.

@kaspar030

This comment has been minimized.

Contributor

kaspar030 commented Jul 21, 2016

Thanks for the feedback and link to nanocoap, @kaspar030. Have you defined features and timeline for it? Are you planning to add Observe, Confirmable, or Block in the near term?

Those features are planned, but I don't know when I find time.
The rough roadmap is:

  • get conn/sock to a usable state
  • replace microcoap with nanocoap
  • then gradually add the missing features

I don't see nanocoap tied to conn/sock though. It's parsing and state handling will be independent from the used networking. I think it would make sense to at least share that code, if possible.

@kb2ma

This comment has been minimized.

Member

kb2ma commented Jul 22, 2016

Sounds like a good plan. I reviewed the nanocoap code, and I think it works well as a microcoap replacement. It definitely is the thinnest veneer over the packet data.

I agree that the packet handling code should be isolated from the networking code. For example, my _coap_parse() expects a gnrc_pktsnip_t, but it just needs a buffer and length as you have done.

It looks like nanocoap really is a few things:

  1. a network/server application encoded in main.c and handler.c
  2. a server library encoded in the endpoint struct/array and coap_build_reply(), as used in handler.c
  3. a packet library in coap_parse(), and the static getters and setters, as well as the hdr and pkt structs

Let me think on this list from the bottom up, for how these things can be integrated with gcoap.

@kb2ma

This comment has been minimized.

Member

kb2ma commented Jul 28, 2016

I see a way forward now with use of nanocoap for gcoap. I plan to make these migrations for the structs in gcoap. The structs don't match one-to-one, but this is the basic idea.

nanocoap gcoap/description
coap_hdr_t migrate from gnrc_coap_meta_t
coap_pkt_t migrate from gnrc_coap_transfer_t
coap_endpoint_t add use; provides a way to specify local resources or to access a remote server
n/a retain gnrc_coap_sender_t to track sent requests; add a pointer for the target endpoint; add a pointer for the buffer backing coap_hdr_t, probably a pktsnip_t

At least for development purposes, I plan to make nanocoap, specifically nanocoap.c/.h, into a module at the 'net' level, in sys/net/application_layer/coap. It will be a dependency for gcoap. I'll make any changes required by gcoap to that module, and then Kaspar and I can work out the differences with the nanocoap in his sock repository. I'll bring up specific issues as they arise.

This refactoring is substantial, and I plan to take some vacation time soon, so expect a few weeks for some results.

@miri64

This comment has been minimized.

Member

miri64 commented Oct 31, 2016

\o/ Maybe we can merge it for this release?

@kb2ma

This comment has been minimized.

Member

kb2ma commented Oct 31, 2016

That's my goal! Working on it now.

@miri64

This comment has been minimized.

Member

miri64 commented Oct 31, 2016

(feature freeze is today/tomorrow-ish ;-))

@miri64

This comment has been minimized.

Member

miri64 commented Oct 31, 2016

(I'm willing to wait a little for this PR (and others) but won't overstretch the timetable)

USEMODULE += gnrc_netdev_default
USEMODULE += auto_init_gnrc_netif
# Specify the mandatory networking modules
USEMODULE += gnrc_ipv6_router_default

This comment has been minimized.

@miri64

miri64 Oct 31, 2016

Member

Maybe you can remove the router requirements on this, too? RPL you already commented out below anyways ;-) (see #6007).

This comment has been minimized.

@kb2ma

kb2ma Oct 31, 2016

Member

Done on top of rebase.

@kb2ma kb2ma force-pushed the kb2ma:gcoap/master branch from 85fc229 to d917065 Oct 31, 2016

@kb2ma

This comment has been minimized.

Member

kb2ma commented Oct 31, 2016

@miri64, I just read your 'One more day' message. To be clear, I addressed the issue you flagged about configuring the example as a host. I'm not sure what is next, but I expect that you will ask me to squash that. I will do that, and then either we are done or there will be more review. Let me know if there is anything I should do to move the process along.

@miri64

This comment has been minimized.

Member

miri64 commented Oct 31, 2016

Just let me give this a quick review before squashing. Also it might be that you will require #6030 to pass Murdock :-/

@miri64

Just some questions left ;-)

USEMODULE += gcoap
USEMODULE += gnrc_udp
USEMODULE += gnrc_ipv6

This comment has been minimized.

@miri64

miri64 Oct 31, 2016

Member

Why are these modules included?

This comment has been minimized.

@kb2ma

kb2ma Oct 31, 2016

Member
  1. gcoap is necessary because we are testing that module. :-)
  2. gnrc_ipv6 is needed at least due to code in gcoap's _event_loop(), which references GNRC_NETTYPE_IPV6. This dependency is my assumption. Is there a better alternative?
  3. gnrc_udp is not needed, and I will fix that. It is an implicit (Makefile.dep) dependency for gcoap. Thanks for that.

This comment has been minimized.

@miri64

miri64 Oct 31, 2016

Member

I think there are other tests already that include gnrc_ipv6 so I'm fine with that (gcoap was clear ^^")

# Redefine request token length, max 8.
#GCOAP_TOKENLEN = 2
#CFLAGS += -DGCOAP_TOKENLEN=$(GCOAP_TOKENLEN)

This comment has been minimized.

@miri64

miri64 Oct 31, 2016

Member

Can those be removed or do you plan to uncomment them?

This comment has been minimized.

@kb2ma

kb2ma Oct 31, 2016

Member

I left those in there intentionally to help a user. It seemed like the message token length, like the CoAP port commented out above it, is something that an implementation might want to adjust. I have an #ifndef check in coap.h for GCOAP_TOKENLEN.

This comment has been minimized.

@miri64

miri64 Oct 31, 2016

Member

Could you maybe add some comment about than (like "uncomment if needed" like in the gnrc_minimal example)?

This comment has been minimized.

@kb2ma

kb2ma Oct 31, 2016

Member

Updated; I understand the confusion.

@kb2ma

This comment has been minimized.

Member

kb2ma commented Oct 31, 2016

Thanks. Regarding #6030, the Makefile explicitly imports the posix module due to #5959, and I expect this gives the build what it needs.

@miri64

This comment has been minimized.

Member

miri64 commented Oct 31, 2016

Yes :-).

@miri64

miri64 approved these changes Oct 31, 2016

ACK, please squash!

@kb2ma kb2ma force-pushed the kb2ma:gcoap/master branch from 34dde30 to 277998e Oct 31, 2016

@kb2ma

This comment has been minimized.

Member

kb2ma commented Oct 31, 2016

Squashed, with gusto!

@miri64

There are still some issues picked up by Murdock. Please address and squash immediately.

/**
* @brief The entry point of this test suite.
*

This comment has been minimized.

@miri64

miri64 Oct 31, 2016

Member

Please remove trailing whitespace.

This comment has been minimized.

@kb2ma

kb2ma Nov 1, 2016

Member

Fixed, apologies for the oversight.

argv[4]);
}
printf("gcoap_cli: sending msg ID %u, %u bytes\n", coap_get_id(&pdu),
len);

This comment has been minimized.

@miri64

miri64 Oct 31, 2016

Member

len needs casting to (unsigned)

This comment has been minimized.

@kb2ma

kb2ma Nov 1, 2016

Member

Addressed and rebased.

/* Content-Format */
if (pdu->content_type != COAP_FORMAT_NONE) {
bufpos += coap_put_option_ct(bufpos, last_optnum, pdu->content_type);
last_optnum = COAP_OPT_CONTENT_FORMAT;

This comment has been minimized.

@miri64

miri64 Oct 31, 2016

Member

last_optnum is assigned but never used according to cppcheck.

This comment has been minimized.

@kb2ma

kb2ma Nov 1, 2016

Member

Addressed and rebased.

@kb2ma kb2ma force-pushed the kb2ma:gcoap/master branch from 277998e to 88a3eeb Nov 1, 2016

@kb2ma

This comment has been minimized.

Member

kb2ma commented Nov 1, 2016

Now seeing an error in Murdock with dtls-echo:nrf52dk. I don't understand this, as nrf52dk is blacklisted in Makefile.

@miri64

This comment has been minimized.

Member

miri64 commented Nov 1, 2016

Nothing to do with your PR, so I would say ACK!

@miri64

miri64 approved these changes Nov 1, 2016

@miri64

This comment has been minimized.

Member

miri64 commented Nov 1, 2016

(See #6032)

@miri64 miri64 merged commit 5084d8c into RIOT-OS:master Nov 1, 2016

1 check failed

Murdock The build failed. runtime: 25m:4s
Details

@smlng smlng referenced this pull request Nov 2, 2016

Closed

Too many coap examples? #6038

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment