Skip to content
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

nanocoap_sock: implement observe (Client-Side) #21160

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

benpicco
Copy link
Contributor

@benpicco benpicco commented Jan 23, 2025

Contribution description

Testing procedure

A new observe command was added to tests/net/nanocoap_cli:

> observe coap://[fe80::d07c:7cff:fe6d:9441]/time
2025-01-23 16:25:24,793 # 00000000  32  37  36  32  30  0A
2025-01-23 16:25:25,175 # 00000000  32  38  30  30  32  0A
2025-01-23 16:25:26,174 # 00000000  32  39  30  30  31  0A
2025-01-23 16:25:27,175 # 00000000  33  30  30  30  31  0A
2025-01-23 16:25:28,175 # 00000000  33  31  30  30  32  0A
2025-01-23 16:25:29,173 # 00000000  33  32  30  30  31  0A
2025-01-23 16:25:30,173 # 00000000  33  33  30  30  31  0A
2025-01-23 16:25:31,175 # 00000000  33  34  30  30  31  0A
2025-01-23 16:25:32,175 # 00000000  33  35  30  30  32  0A
2025-01-23 16:25:33,175 # 00000000  33  36  30  30  32  0A
2025-01-23 16:25:34,173 # 00000000  33  37  30  30  31  0A
2025-01-23 16:25:35,173 # 00000000  33  38  30  30  31  0A

> observe coap://[fe80::d07c:7cff:fe6d:9441]/time off
2025-01-23 16:25:36,148 # gnrc_sock: timeout != 0 within the asynchronous callback lead to unexpected delays within the asynchronous handler.
2025-01-23 16:25:36,148 # 00000000  33  38  39  37  36  0A
> 

Issues/PRs references

based on #21147

@github-actions github-actions bot added Area: network Area: Networking Area: tests Area: tests and testing framework Area: CoAP Area: Constrained Application Protocol implementations Area: sys Area: System Area: examples Area: Example Applications labels Jan 23, 2025
@benpicco benpicco requested a review from maribu January 23, 2025 15:28
Copy link
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

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

Looks quite good to me :)

I have been thinking for ages whether we should go for async sockets for nanocoap (both client and server), and provide the synchronous sock API on top of that.

IMO the synchronous interface is optimized for the rather synthetic "shell app use case". Any "headless" MCU would more likely want to use an async API and the ability to run server and client on the same endpoint.

But that is orthogonal to this PR.

tests/net/nanocoap_cli/nanocli_client.c Outdated Show resolved Hide resolved
tests/net/nanocoap_cli/nanocli_client.c Outdated Show resolved Hide resolved
sys/net/application_layer/nanocoap/sock.c Outdated Show resolved Hide resolved
sys/net/application_layer/nanocoap/sock.c Outdated Show resolved Hide resolved
sys/net/application_layer/nanocoap/sock.c Outdated Show resolved Hide resolved
@benpicco benpicco force-pushed the sys/net/nanocoap/observe-client branch 2 times, most recently from 8d0a4e9 to dcded9d Compare January 23, 2025 18:29
examples/nanocoap_server/coap_handler.c Outdated Show resolved Hide resolved
sys/include/net/nanocoap_sock.h Outdated Show resolved Hide resolved
sys/include/net/nanocoap_sock.h Outdated Show resolved Hide resolved
sys/include/net/nanocoap_sock.h Outdated Show resolved Hide resolved
sys/net/application_layer/nanocoap/sock.c Outdated Show resolved Hide resolved
sys/net/application_layer/nanocoap/sock.c Outdated Show resolved Hide resolved
tests/net/nanocoap_cli/nanocli_client.c Outdated Show resolved Hide resolved
tests/net/nanocoap_cli/nanocli_client.c Outdated Show resolved Hide resolved
@benpicco benpicco force-pushed the sys/net/nanocoap/observe-client branch from 5a58280 to 73f41af Compare January 28, 2025 13:02
@benpicco benpicco marked this pull request as ready for review January 28, 2025 13:05
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 28, 2025
@benpicco benpicco requested review from maribu and chrysn January 28, 2025 13:05
@riot-ci
Copy link

riot-ci commented Jan 28, 2025

Murdock results

FAILED

99a66d5 examples/nanocoap_server: add output if registration fails

Success Failures Total Runtime
1102 1 9371 01m:48s
Build failures (1)
Application Target Toolchain Runtime (s) Worker
tests/net/nanocoap_cli hifive1b gnu 3.46 mobi3

Artifacts

Copy link
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

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

lgtm, some more comments inline and the CI also has comments.

sys/net/application_layer/nanocoap/sock.c Outdated Show resolved Hide resolved
sys/net/application_layer/nanocoap/sock.c Outdated Show resolved Hide resolved
sys/include/net/nanocoap_sock.h Outdated Show resolved Hide resolved
@benpicco benpicco force-pushed the sys/net/nanocoap/observe-client branch from 73f41af to 99a66d5 Compare January 28, 2025 15:03
@maribu maribu enabled auto-merge January 28, 2025 15:05
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 Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants