gcoap: Observe extension server #6469

kb2ma · 2017-01-24T16:34:01Z

Observe (RFC 7641) provides a push mechanism for a CoAP server to notify clients of changes to a resource. Elimination of polling for this data is valuable in a constrained environment.

Initial work in this PR is on the server side. Presently the implementation is able to accept a client's registration for a resource, and then include an Observe option in the response. No ongoing notifications yet. Depends on PR #6243.

The PR uses my fork of @kaspar030's sock repository to add common data and utility code for Observe. This work then can be used for an Observe implementation for nanocoap. Of course, I'll work to get these additions upstream.

I see a clear path to implementation of ongoing notifications. This mechanism will work in two phases:

An application (outside gcoap's thread) notifies the server that one of its resources has changed.
The notification handler marks the Observe memos (if any) for the resource as pending, and sends a message to the sock mbox to allow the gcoap thread to take action.
gcoap then cycles through its _eventLoop(), which includes searching for pending Observe memos.
The server builds the notification/response via the resource's callback, and then sends it.

Once I have implemented notifications, the PR should be useful. I'll refresh when it's ready.

kaspar030 · 2017-01-24T20:22:03Z

The PR uses my fork of @kaspar030's sock repository to add common data and utility code for Observe.

FYI: I just merged your gcoap/observe branch. Thanks!

kb2ma · 2017-01-25T18:22:40Z

Thanks for merging, and for the refactor of the uint parse code! I updated my nanocoap pkg to use your repository again.

kb2ma · 2017-01-29T17:10:22Z

After reviewing the nanocoap update and thinking about the rest of Observe, it seemed time for the kind of reassurance unit tests can bring. So, now we have sock PR #11! Back to Observe itself now.

kb2ma · 2017-02-10T12:23:26Z

Added generation of Observe notifications. The implementation follows the outline in the first posting for this PR. See the new "Observe Server" section in the source documentation to give it a try. The most recent commit shows how I added Observe support for the CLI example. It's a one-liner!

It took a while to figure out how to generate the option value (serial number) for the notification. I like the outcome -- simple and stateless, and allows the user to minimize the length of the option.

I think client deregistration is all that's left for this PR. I think it would be simpler to implement the client side of Observe in a separate PR, so I renamed this PR to be more specific. But even before I work on the client side, I need to add Confirmable messaging to make the server side compliant. Sec. 4.5 says a notification must be sent confirmably at least once a day to be sure the client still exists.

kb2ma · 2017-02-11T12:36:26Z

Updated yesterday's commits to fix static test failures.

kb2ma · 2017-02-17T18:19:10Z

Added Observe deregistration. I plan to automate the functional tests, rebase and squash, and then should be ready to remove the WIP label.

kb2ma · 2017-02-27T03:44:12Z

Code complete, squashed. Ready for review. I removed 'WIP' from the title; please remove the label as well.

Merge for this PR is blocked on merge for #6243. @kaspar030, I'd be grateful if you would review that one first. As described there, I updated the unit tests to show that it works.

kb2ma · 2017-03-06T16:24:14Z

I am reconsidering this PR's readiness for merge. My concern is the implementation of gcoap_resource_changed(), which results in asynchronously calling (from the gcoap thread) the resource handler's function to generate the resource representation. Instead, I am thinking that the caller should generate the resource representation before notifying gcoap. In other words, observations will be implemented more like originating a POST/PUT request rather than responding to a GET request. This approach means the gcoap user will not be required to buffer data that it receives from an event.

Next, I plan to work through the alternative implementation to be sure it works better. Of course, any comments are welcome.

kb2ma · 2017-03-15T03:20:41Z

Finished with rework. Ready for review.

Removed gcoap_resource_changed(), and added gcoap_obs_init() and gcoap_obs_send() to create and send an Observe notification. See the Observe Server Operation section in the module documentation.

The new approach provides for more control over messaging and data buffering. The value of this approach should become clearer with the implementation of confirmable messaging, which I plan to work on soon.

KBraham · 2017-03-30T12:10:36Z

Created a test application to send notifications from a thread running on the native platform, the observe GET model seems to work brilliantly.

Copper uses by default the lazy subscribe and cancels the notification via a reset message. This results in a 4.04 returned by RIOT and not canceling the subscription. Should this approach work for non-confirmable messages?

kb2ma · 2017-03-30T16:27:02Z

Thanks for the report on using Observe, @KBraham! No, the lazy/reset mechanism is not supported. Presently the implementation relies on receipt of an explicit cancellation GET with the Observe option value of 1. I leaned on the Implementation Note at the end of Sec. 4.5 in the RFC which says the lazy/reset mechanism isn't required for non-confirmable notifications.

On the other hand, it's pretty uncool to respond to a RST message, but gcoap does not process them yet. :-/ Handling CON/ACK/RST is the next significant feature I plan to add. See the status page for the roadmap.

I definitely will add a note to the source documentation on how to cancel an Observe session. I also will play more with Copper. It will be good to verify against another implementation besides libcoap.

aabadie · 2017-04-06T08:25:17Z

Is this still WIP ? Would be great to have it in 2017.04.

kb2ma · 2017-04-06T09:39:31Z

@aabadie, thanks for checking in. This PR is no longer WIP. The last commit needs to be squashed in, but I thought I would wait for review first. Also, it's not clear why the latest build failed. Please try running that again to confirm.

This PR depends on #6243, which should be straightforward. Also, #6786 reduces memory use for gcoap, and I would appreciate review for that one, too.

KBraham · 2017-04-06T10:16:43Z

I would like to add another remark on the observe implementation, it seems clients which simultaneously subscribe a resource will only allow one to reserve an observe registration. I was expecting that multiple clients (in this case originating from the same source machine) should register multiple active sessions.

I have traced this using the debug prints to "gcoap: can't register observer" which fails on the resource_memo == NULL check as a memo is registered for the coap resource by the first client. Is each resource only observable by one client at most when using concurrency? They do provide different tokens for the session and thus should be separate entries? After releasing the session another client can claim the resource to observe.

kb2ma · 2017-04-06T17:27:12Z

Thanks again for your feedback! Yes, there is a limitation of one observer per resource. I did actually document this one under "Observe Server Operation" in the module docs. I view this limitation as tentative -- maybe it should be removed in the future, but I wanted to be conservative for these reasons:

It increases the registration code complexity
It increases the complexity of gcoap_obs_send() and its semantics, which now may send more than one notification at a time. I was particularly concerned here with the overhead of confirmable messaging, which isn't implemented yet.
For me, the implementation is intended for constrained nodes, like a wireless sensor mote sending readings or management stats to a server. This use case doesn't really need multiple observers per resource.

As you can see on the Memory Use page, I have been measuring lately. Observe already adds 800 B of text. If there needs to be multiple clients per resource, maybe that should be handled by a single, more capable client.

This tension between features and memory use makes me crazy at times. It really depends on use cases, and I couldn't justify this one, at least right now. So, I'd be very interested to know what you're trying to accomplish with this scenario.

aabadie · 2017-04-28T07:07:04Z

I believe I've found a corner case where the observer is not cleared. I plan to write a test and a fix for this condition.

Ok, let me know when you are done. I'll be quite busy before 11th of may so I probably won't find time for testing before.

kb2ma · 2017-05-04T16:31:26Z

Fixed a logic error to ensure an observer record is allocated only if a memo record is allocated. The diff looks extensive, but mostly I just reordered the steps. You can see the added test, 'reg-cleanup', in commit 0336aa8 of my gcoap-test project.

kb2ma · 2017-05-04T16:36:24Z

Also fixed a logic error (3a9582b) to ensure the observe memo lookup matches the requested endpoint. Did not add a test because the error would occur in a situation that is difficult to create -- when the tokens match for the endpoints.

@aabadie, done with fixes. I'm happy to respond to any further questions or concerns.

kb2ma · 2017-05-15T17:17:10Z

Created a temperature collection app to exercise gcoap and Observe. Nothing like an example to help others get started. :-)

haukepetersen

Played around with this and found everything to work as expected so far. ACK from my side.

@aabadie are you happy as well?
@kb2ma would you be so nice to rebase and squash, thx

aabadie

Didn't find time for testing but if @haukepetersen is happy, so am I.

haukepetersen · 2017-05-23T08:33:52Z

:-)

miri64 · 2017-05-23T08:36:34Z

Can the WIP label be removed then?

haukepetersen · 2017-05-23T08:44:59Z

IMHO yes. I'd say we merge this in the current state and enhance/fix as we go. This will simplify and speed up things, right?

kb2ma · 2017-05-23T10:31:21Z

Glad to hear Observe worked for you @haukepetersen, and thanks to all for moving this forward. Will squash and rebase ASAP.

kb2ma · 2017-05-24T04:40:47Z

Squashed and rebased. My functional tests pass on native and samr21-xpro.

haukepetersen · 2017-05-24T07:11:12Z

I can confirm.

Murdock is happy, ACKs are there -> let's go!

@kb2ma thanks for the great work, looking forward to support for CON and blockwise-transfer (hopefully in a joint effort) :-)

emmanuelsearch · 2017-05-24T07:35:41Z

@kb2ma great!

kb2ma · 2017-05-24T15:03:59Z

Milestone logged on the status page!

jnohlgard · 2017-07-27T06:40:02Z

I don't see why the CI let this PR pass. I'm getting build errors on current master for tests/unittests with BOARD=mulle:

Building application "tests_unittests" for "mulle" with MCU "k60".

"make" -C /data/riotbuild/riotbase/pkg/heatshrink
cp Makefile.heatshrink /data/riotbuild/riotproject/tests/unittests/bin/pkg/mulle/heatshrink/Makefile
"make" -C /data/riotbuild/riotproject/tests/unittests/bin/pkg/mulle/heatshrink
"make" -C /data/riotbuild/riotbase/pkg/nanocoap
"make" -C /data/riotbuild/riotproject/tests/unittests/bin/pkg/mulle/nanocoap/nanocoap
"make" -C /data/riotbuild/riotbase/pkg/spiffs
"make" -C /data/riotbuild/riotproject/tests/unittests/bin/pkg/mulle/spiffs/riotbuild
"make" -C /data/riotbuild/riotbase/pkg/tweetnacl
"make" -C /data/riotbuild/riotproject/tests/unittests/bin/pkg/mulle/tweetnacl
"make" -C /data/riotbuild/riotbase/core
"make" -C /data/riotbuild/riotbase/drivers
"make" -C /data/riotbuild/riotbase/drivers/mtd
"make" -C /data/riotbuild/riotbase/drivers/mtd_spi_nor
"make" -C /data/riotbuild/riotbase/drivers/nvram
"make" -C /data/riotbuild/riotbase/drivers/nvram_spi
"make" -C /data/riotbuild/riotbase/drivers/periph_common
"make" -C /data/riotbuild/riotbase/drivers/saul
"make" -C /data/riotbuild/riotbase/pkg/spiffs/fs
"make" -C /data/riotbuild/riotbase/sys
"make" -C /data/riotbuild/riotbase/sys/base64
"make" -C /data/riotbuild/riotbase/sys/bitfield
"make" -C /data/riotbuild/riotbase/sys/bloom
"make" -C /data/riotbuild/riotbase/sys/cbor
"make" -C /data/riotbuild/riotbase/sys/checksum
"make" -C /data/riotbuild/riotbase/sys/color
"make" -C /data/riotbuild/riotbase/sys/crypto
"make" -C /data/riotbuild/riotbase/sys/crypto/modes
"make" -C /data/riotbuild/riotbase/sys/div
"make" -C /data/riotbuild/riotbase/sys/ecc/hamming256
"make" -C /data/riotbuild/riotbase/sys/embunit
"make" -C /data/riotbuild/riotbase/sys/fmt
"make" -C /data/riotbuild/riotbase/sys/fs/constfs
"make" -C /data/riotbuild/riotbase/sys/fs/devfs
"make" -C /data/riotbuild/riotbase/sys/hashes
"make" -C /data/riotbuild/riotbase/sys/isrpipe
"make" -C /data/riotbuild/riotbase/sys/net/application_layer/coap
/data/riotbuild/riotbase/sys/net/application_layer/coap/gcoap.c: In function '_handle_req':
/data/riotbuild/riotbase/sys/net/application_layer/coap/gcoap.c:193:9: error: implicit declaration of function 'coap_get_observe' [-Werror=implicit-function-declaration]
     if (coap_get_observe(pdu) == COAP_OBS_REGISTER) {
         ^~~~~~~~~~~~~~~~
/data/riotbuild/riotbase/sys/net/application_layer/coap/gcoap.c:193:34: error: 'COAP_OBS_REGISTER' undeclared (first use in this function)
     if (coap_get_observe(pdu) == COAP_OBS_REGISTER) {
                                  ^~~~~~~~~~~~~~~~~
/data/riotbuild/riotbase/sys/net/application_layer/coap/gcoap.c:193:34: note: each undeclared identifier is reported only once for each function it appears in
/data/riotbuild/riotbase/sys/net/application_layer/coap/gcoap.c:214:17: error: implicit declaration of function 'coap_clear_observe' [-Werror=implicit-function-declaration]
                 coap_clear_observe(pdu);
                 ^~~~~~~~~~~~~~~~~~
/data/riotbuild/riotbase/sys/net/application_layer/coap/gcoap.c:228:16: error: 'coap_pkt_t {aka struct <anonymous>}' has no member named 'observe_value'
             pdu->observe_value = (now >> GCOAP_OBS_TICK_EXPONENT) & 0xFFFFFF;
                ^~
/data/riotbuild/riotbase/sys/net/application_layer/coap/gcoap.c:231:41: error: 'COAP_OBS_DEREGISTER' undeclared (first use in this function)
     } else if (coap_get_observe(pdu) == COAP_OBS_DEREGISTER) {
                                         ^~~~~~~~~~~~~~~~~~~
/data/riotbuild/riotbase/sys/net/application_layer/coap/gcoap.c:248:16: error: implicit declaration of function 'coap_has_observe' [-Werror=implicit-function-declaration]
     } else if (coap_has_observe(pdu)) {
                ^~~~~~~~~~~~~~~~
/data/riotbuild/riotbase/sys/net/application_layer/coap/gcoap.c: In function '_write_options':
/data/riotbuild/riotbase/sys/net/application_layer/coap/gcoap.c:445:35: error: 'coap_pkt_t {aka struct <anonymous>}' has no member named 'observe_value'
         uint32_t nval  = htonl(pdu->observe_value);
                                   ^~
/data/riotbuild/riotbase/sys/net/application_layer/coap/gcoap.c:454:56: error: 'COAP_OPT_OBSERVE' undeclared (first use in this function)
         bufpos += coap_put_option(bufpos, last_optnum, COAP_OPT_OBSERVE,
                                                        ^~~~~~~~~~~~~~~~
/data/riotbuild/riotbase/sys/net/application_layer/coap/gcoap.c: In function 'gcoap_obs_init':
/data/riotbuild/riotbase/sys/net/application_layer/coap/gcoap.c:778:12: error: 'coap_pkt_t {aka struct <anonymous>}' has no member named 'observe_value'
         pdu->observe_value = (now >> GCOAP_OBS_TICK_EXPONENT) & 0xFFFFFF;
            ^~
cc1: all warnings being treated as errors
/data/riotbuild/riotbase/Makefile.base:81: recipe for target '/data/riotbuild/riotproject/tests/unittests/bin/mulle/gcoap/gcoap.o' failed
make[3]: *** [/data/riotbuild/riotproject/tests/unittests/bin/mulle/gcoap/gcoap.o] Error 1
/data/riotbuild/riotbase/Makefile.base:20: recipe for target 'ALL--/data/riotbuild/riotbase/sys/net/application_layer/coap' failed
make[2]: *** [ALL--/data/riotbuild/riotbase/sys/net/application_layer/coap] Error 2
/data/riotbuild/riotbase/Makefile.base:20: recipe for target 'ALL--/data/riotbuild/riotbase/sys' failed
make[1]: *** [ALL--/data/riotbuild/riotbase/sys] Error 2
/data/riotbuild/riotbase/Makefile.include:303: recipe for target 'link' failed
make: *** [link] Error 2

jnohlgard · 2017-07-27T06:42:49Z

Sorry for the noise, got the compilation working again after rm -rf bin to get the clone of pkg/nanocoap updated.

miri64 assigned kaspar030 Jan 24, 2017

miri64 added this to the Release 2017.04 milestone Jan 24, 2017

miri64 added Area: network Area: Networking Type: new feature The issue requests / The PR implemements a new feature for RIOT labels Jan 24, 2017

kb2ma changed the title ~~gcoap: Observe implementation WIP~~ gcoap: Observe extension WIP Jan 25, 2017

emmanuelsearch added the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Jan 25, 2017

kb2ma changed the title ~~gcoap: Observe extension WIP~~ gcoap: Observe extension server WIP Feb 10, 2017

kb2ma force-pushed the gcoap/observe branch from a77f889 to 36a41d8 Compare February 11, 2017 12:01

kb2ma mentioned this pull request Feb 16, 2017

gcoap: broken handling of ACK messages? #6620

Closed

kb2ma mentioned this pull request Feb 18, 2017

gcoap: Fix compilation and unit test errors for zero-length token #6243

Merged

kb2ma force-pushed the gcoap/observe branch from 1140b5d to b7e25dc Compare February 27, 2017 03:22

kb2ma changed the title ~~gcoap: Observe extension server WIP~~ gcoap: Observe extension server Feb 27, 2017

kb2ma force-pushed the gcoap/observe branch from b7e25dc to 41b8e28 Compare March 15, 2017 02:49

kb2ma mentioned this pull request Mar 25, 2017

gcoap/nanocoap: Exclude unused nanocoap messaging functions #6786

Closed

kaspar030 removed this from the Release 2017.04 milestone Apr 21, 2017

haukepetersen approved these changes May 23, 2017

View reviewed changes

aabadie approved these changes May 23, 2017

View reviewed changes

miri64 added CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable and removed State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet labels May 23, 2017

kb2ma added 3 commits May 23, 2017 17:47

nanocoap: Update pkg for Observe commits.

05fe852

gcoap: Add server-side Observe registration and notifications.

9d37be2

gcoap: Update example for Observe, including documentation.

3abff6d

kb2ma force-pushed the gcoap/observe branch from 3a9582b to 3abff6d Compare May 24, 2017 04:35

haukepetersen merged commit 7a1fcdf into RIOT-OS:master May 24, 2017

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

aabadie added this to the Release 2017.07 milestone Jun 30, 2017

kYc0o mentioned this pull request Oct 30, 2017

Network Stack High layers #7894

Closed

8 tasks

kYc0o modified the milestone: Release 2017.07 Oct 31, 2017

kb2ma deleted the gcoap/observe branch February 4, 2019 13:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gcoap: Observe extension server #6469

gcoap: Observe extension server #6469

kb2ma commented Jan 24, 2017

kaspar030 commented Jan 24, 2017

kb2ma commented Jan 25, 2017

kb2ma commented Jan 29, 2017

kb2ma commented Feb 10, 2017

kb2ma commented Feb 11, 2017

kb2ma commented Feb 17, 2017 •

edited

Loading

kb2ma commented Feb 27, 2017 •

edited

Loading

kb2ma commented Mar 6, 2017 •

edited

Loading

kb2ma commented Mar 15, 2017

KBraham commented Mar 30, 2017

kb2ma commented Mar 30, 2017

aabadie commented Apr 6, 2017

kb2ma commented Apr 6, 2017

KBraham commented Apr 6, 2017

kb2ma commented Apr 6, 2017

aabadie commented Apr 28, 2017

kb2ma commented May 4, 2017 •

edited

Loading

kb2ma commented May 4, 2017

kb2ma commented May 15, 2017

haukepetersen left a comment

aabadie left a comment

haukepetersen commented May 23, 2017

miri64 commented May 23, 2017

haukepetersen commented May 23, 2017

kb2ma commented May 23, 2017

kb2ma commented May 24, 2017

haukepetersen commented May 24, 2017

emmanuelsearch commented May 24, 2017

kb2ma commented May 24, 2017

jnohlgard commented Jul 27, 2017

jnohlgard commented Jul 27, 2017

gcoap: Observe extension server #6469

gcoap: Observe extension server #6469

Conversation

kb2ma commented Jan 24, 2017

kaspar030 commented Jan 24, 2017

kb2ma commented Jan 25, 2017

kb2ma commented Jan 29, 2017

kb2ma commented Feb 10, 2017

kb2ma commented Feb 11, 2017

kb2ma commented Feb 17, 2017 • edited Loading

kb2ma commented Feb 27, 2017 • edited Loading

kb2ma commented Mar 6, 2017 • edited Loading

kb2ma commented Mar 15, 2017

KBraham commented Mar 30, 2017

kb2ma commented Mar 30, 2017

aabadie commented Apr 6, 2017

kb2ma commented Apr 6, 2017

KBraham commented Apr 6, 2017

kb2ma commented Apr 6, 2017

aabadie commented Apr 28, 2017

kb2ma commented May 4, 2017 • edited Loading

kb2ma commented May 4, 2017

kb2ma commented May 15, 2017

haukepetersen left a comment

Choose a reason for hiding this comment

aabadie left a comment

Choose a reason for hiding this comment

haukepetersen commented May 23, 2017

miri64 commented May 23, 2017

haukepetersen commented May 23, 2017

kb2ma commented May 23, 2017

kb2ma commented May 24, 2017

haukepetersen commented May 24, 2017

emmanuelsearch commented May 24, 2017

kb2ma commented May 24, 2017

jnohlgard commented Jul 27, 2017

jnohlgard commented Jul 27, 2017

kb2ma commented Feb 17, 2017 •

edited

Loading

kb2ma commented Feb 27, 2017 •

edited

Loading

kb2ma commented Mar 6, 2017 •

edited

Loading

kb2ma commented May 4, 2017 •

edited

Loading