Skip to content

sys/net/nanocoap: Implement Observe (Server-Side) #21147

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

Merged
merged 2 commits into from
Jan 23, 2025

Conversation

maribu
Copy link
Member

@maribu maribu commented Jan 20, 2025

Contribution description

  • Clean up and extend the separate response feature of nanocoap
  • Add a bit of glue to use the separate response feature to implement observe

Testing procedure

Starting the Server

$ sudo ip tuntap add tap0 mode tap user $(whoami)
$ sudo ip link set tap0 up
$ make BOARD=native64 -C examples/nanocoap_server -j flash term

Running a Client

$ coap-client-notls -s 60 -m get 'coap://[fe80::9826:30ff:feb8:31f4%tap0]/time'
8326
9000
10001
11000
12001
13000
14000
15001
16000
17001
18000
19000
20001
21000
22000
[...]

Issues/PRs references

None

@maribu maribu added Type: new feature The issue requests / The PR implemements a new feature for RIOT CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jan 20, 2025
@github-actions github-actions bot added Area: network Area: Networking Area: CoAP Area: Constrained Application Protocol implementations Area: sys Area: System Area: examples Area: Example Applications labels Jan 20, 2025
@riot-ci
Copy link

riot-ci commented Jan 21, 2025

Murdock results

✔️ PASSED

feeb684 sys/net/nanocoap: implement observe

Success Failures Total Runtime
10271 0 10271 11m:03s

Artifacts

@maribu maribu force-pushed the sys/net/nanocoap/observe branch 2 times, most recently from f0a998f to 124752f Compare January 21, 2025 08:06
@maribu maribu marked this pull request as ready for review January 21, 2025 08:06
@maribu maribu force-pushed the sys/net/nanocoap/observe branch 2 times, most recently from a3e9b1c to c4de045 Compare January 21, 2025 08:41
@MrKevinWeiss MrKevinWeiss added the Process: blocked by feature freeze Integration Process: The impact of this PR is too high for merging it during soft feature freeze. label Jan 21, 2025
Copy link
Contributor

@mguetschow mguetschow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just had a high-level look at it and have some nit's below. In general I don't feel confident enough in nanocoap_sock to tell whether this takes all nanocoap quirks into account, but the implementation looks good to me in general.

@MrKevinWeiss MrKevinWeiss removed the Process: blocked by feature freeze Integration Process: The impact of this PR is too high for merging it during soft feature freeze. label Jan 21, 2025
@MrKevinWeiss
Copy link
Contributor

Freeze is over so if anyone wants to pick this up...

@maribu maribu force-pushed the sys/net/nanocoap/observe branch from c4de045 to 26f4f06 Compare January 21, 2025 15:38
@maribu
Copy link
Member Author

maribu commented Jan 22, 2025

Time for squash?

squash player dancing with racket in his hand

@benpicco
Copy link
Contributor

Please squash!

maribu and others added 2 commits January 23, 2025 14:25
This allows sending a separate response with CoAP Options and adds a
helper to detect duplicate requests, so that resource handlers can
repeat their empty ACK on duplicates.
This adds the new `nanocoap_server_observe` module that implements the
server side of the CoAP Observe option. It does require cooperation
from the resource handler to work, though.

Co-Authored-By: mguetschow <mikolai.guetschow@tu-dresden.de>
Co-authored-by: benpicco <benpicco@googlemail.com>
@maribu maribu force-pushed the sys/net/nanocoap/observe branch from 88ab5fd to feeb684 Compare January 23, 2025 13:27
@maribu maribu requested a review from mguetschow January 23, 2025 13:36
@benpicco benpicco added this pull request to the merge queue Jan 23, 2025
Merged via the queue into RIOT-OS:master with commit 139404d Jan 23, 2025
25 checks passed
@maribu maribu deleted the sys/net/nanocoap/observe branch January 23, 2025 17:37
@benpicco
Copy link
Contributor

An issue I see is that observe registrations never time out. So The server can easily fill up if clients do not unregister properly (which can easily happen when they get out of range/crash/reboot/…)

Should nanocoap_notify_observers() send CONfirmable messages and remove clients on timeout?

@maribu
Copy link
Member Author

maribu commented Jan 23, 2025

We sadly cannot send fully standard compliant CONs as separate response; which we use for the notification. (In fact, there is an assert that will block.)

One could probably send the notifications as CON without doing retransmissions, breaking the standard. (I think aborting retransmissions when a more recent notification is available is probably the sane thing to do anyway, as the stated goal of the observe RFC is to get the client as closely in sync with the server state as possible; blocking a more fresh update by retransmissions of stale data is not well aligned with this.)

Another approach is to just let observations time out anyway. After all, clients can in fact occasionally re-affirm their active observe registration by registering with the same token at the same resource again. This could be used to reset the timeout.

@miri64
Copy link
Member

miri64 commented Feb 5, 2025

feeb684 introduced a regression that makes the release tests fail for

  • make -C tests/unittests/ tests-nanocoap test and
  • make -C tests/unittests/ tests-nanocoap_cache test

(part of task 1.3)

In file included from /home/mlenders/Repositories/RIOT-OS/RIOT/sys/include/net/nanocoap_sock.h:146,
                 from /home/mlenders/Repositories/RIOT-OS/RIOT/sys/net/application_layer/nanocoap/nanocoap.c:31:
/home/mlenders/Repositories/RIOT-OS/RIOT/sys/include/net/sock/udp.h:293:28: error: conflicting types for ‘sock_udp_ep_t’; have ‘struct _sock_tl_ep’
  293 | typedef struct _sock_tl_ep sock_udp_ep_t;   /**< An end point for a UDP sock object */
      |                            ^~~~~~~~~~~~~
In file included from /home/mlenders/Repositories/RIOT-OS/RIOT/sys/net/application_layer/nanocoap/nanocoap.c:30:
/home/mlenders/Repositories/RIOT-OS/RIOT/sys/include/net/nanocoap.h:104:14: note: previous declaration of ‘sock_udp_ep_t’ with type ‘sock_udp_ep_t’ {aka ‘void’}
  104 | typedef void sock_udp_ep_t;
      |              ^~~~~~~~~~~~~
/home/mlenders/Repositories/RIOT-OS/RIOT/sys/include/net/sock/udp.h: In function ‘sock_udp_ep_is_multicast’:
/home/mlenders/Repositories/RIOT-OS/RIOT/sys/include/net/sock/udp.h:809:15: error: request for member ‘family’ in something not a structure or union
  809 |     switch (ep->family) {
      |               ^~
/home/mlenders/Repositories/RIOT-OS/RIOT/sys/include/net/sock/udp.h: At top level:
/home/mlenders/Repositories/RIOT-OS/RIOT/sys/include/net/sock/udp.h:845:10: fatal error: sock_types.h: No such file or directory
  845 | #include "sock_types.h"
      |          ^~~~~~~~~~~~~~

@mguetschow mguetschow added this to the Release 2025.04 milestone Apr 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: CoAP Area: Constrained Application Protocol implementations Area: examples Area: Example Applications Area: network Area: Networking Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants