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: add support for no-response option #18154

Merged
merged 3 commits into from
Oct 15, 2022

Conversation

benpicco
Copy link
Contributor

@benpicco benpicco commented Jun 1, 2022

Contribution description

This adds support for the no-response option (RFC 7968) in the nanoCoAP server.

To allow for testing and easy use, the two client functions nanocoap_sock_put_non() and nanocoap_sock_post_non() are also added which will add the no-response option if no payload response is requested.

Testing procedure

A new put_non command has been added to tests/nanocoap_cli:

> put_non coap://[fe80::58b0:8ff:fe67:ad82]/value 42
> ncget coap://[fe80::58b0:8ff:fe67:ad82]/value -
42

Verify in Wireshark that indeed no response was generated to the PUT request

image

Issues/PRs references

@benpicco benpicco requested a review from chrysn June 1, 2022 10:06
@github-actions github-actions bot added Area: CoAP Area: Constrained Application Protocol implementations Area: network Area: Networking Area: sys Area: System labels Jun 1, 2022
@benpicco benpicco requested review from kaspar030 and maribu June 7, 2022 09:14
Copy link
Member

@chrysn chrysn left a comment

Choose a reason for hiding this comment

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

I've pushed a suggestion (GitHub doesn't let me add suggestions to files you didn't touch) to reflect the response code in the documentation.

An aspect this misses on the CoAP side is that No-Response acts on the request-response sublayer, but leaves the reliability sublayer intact. IOW: When a CON request is sent with No-Response, there still needs to be an ACK (but if the response is actually suppressed, it would be an empty ACK). In the implementation, that'd mean building a response header with a zero-length token and a zero code, and returning the header length.

@benpicco
Copy link
Contributor Author

I suppose it makes sense to limit no-response to NON requests then, when we are sending a response anyway we might was well include the payload.

@benpicco benpicco force-pushed the nanocoap_no-response branch 2 times, most recently from 2059803 to 1f27d10 Compare August 29, 2022 13:35
@benpicco benpicco marked this pull request as ready for review August 29, 2022 13:35
sys/include/net/coap.h Outdated Show resolved Hide resolved
@miri64
Copy link
Member

miri64 commented Aug 30, 2022

I suppose it makes sense to limit no-response to NON requests then, when we are sending a response anyway we might was well include the payload.

As far as I understood @chrysn, CON requests are also possible to have no-response. They would just an empty ACK. Shouldn't be too difficult to implement.

@benpicco
Copy link
Contributor Author

But does it make sense? The RFC says

Using this option with CON requests may not serve the desired purpose if piggybacked responses are triggered. But, if the server responds with a separate response (which, perhaps, the client does not care about), then this option can be useful.

Since we are not doing separate response, I don't think this is useful for CON messages.

@miri64
Copy link
Member

miri64 commented Aug 30, 2022

Since we are not doing separate response, I don't think this is useful for CON messages.

What even are "separate responses" in this context? Late CON responses after the server already ACK'd the request with an empty ACK? In this case, I think it does all the more sense to save traffic from the server.

@benpicco
Copy link
Contributor Author

benpicco commented Aug 30, 2022

Yes - but we don't have this implemented AFAIK (and it would also not be relevant here since the payload length of the ACK would then be 0 anyway)

@miri64
Copy link
Member

miri64 commented Aug 30, 2022

Yes - but we don't have this implemented AFAIK (and it would also not be relevant here since the payload length of the ACK would then be 0 anyway)

We do have empty ACKs implemented. In gcoap at least (also see #18386 for a server application that generates "separate responses").

switch (coap_get_type(&pdu)) {
case COAP_TYPE_CON:
messagelayer_emptyresponse_type = COAP_TYPE_ACK;
DEBUG("gcoap: Answering CON response with ACK\n");
/* fall through */

@chrysn
Copy link
Member

chrysn commented Aug 30, 2022

Just because we don't have them implemented (well) doesn't mean that the client can't send a CON request with a No-Response option. Sending the full answer (as we do now) is not wrong, as the option is not critical.

@benpicco benpicco changed the title nanocoap: add support for no-response option nanocoap: add support for no-response option for non-confirmable messages Sep 14, 2022
@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 Sep 25, 2022
@benpicco
Copy link
Contributor Author

Adding no-response support for NON messages is easy (less than 10 lines in nanocoap.c), adding no-response support for CON messages is a whole different effort that I'm still not sure if it's justified. Could we relegate that to a separate PR?

@benpicco benpicco removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 13, 2022
@benpicco benpicco changed the title nanocoap: add support for no-response option for non-confirmable messages nanocoap: add support for no-response option Oct 13, 2022
Copy link
Member

@chrysn chrysn left a comment

Choose a reason for hiding this comment

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

I'm happy with this now, thanks for sticking with it.

Not too much has changed since the version I've tested. Two notes on documentation in the usual mixture of "I think this needs to be documented" and "If this is inaccurate I'd have misunderstood something" -- but unless the latter triggers, it's fine without them as well.

Please squash.

sys/include/net/nanocoap.h 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
@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 Oct 13, 2022
@maribu maribu added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Oct 13, 2022
@benpicco benpicco merged commit 4d0c533 into RIOT-OS:master Oct 15, 2022
@benpicco benpicco deleted the nanocoap_no-response branch October 15, 2022 19:16
@kaspar030 kaspar030 added this to the Release 2023.01 milestone Jan 19, 2023
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: 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.

6 participants