Skip to content

Conversation

benpicco
Copy link
Contributor

Contribution description

This adds two functions coap_payload_add() and coap_payload_advance().

  • coap_payload_add() will add n bytes to the payload buffer and advance
    payload pointer accordingly.
    const char hello[] = "Hello CoAP!";
    coap_payload_add(pkt, hello, sizeof(hello));
  • coap_payload_advance() will advance the payload buffer after data
    has been added to it.
    int len = snprintf(pkt->payload, pkt->payload_len, "%s %s!", "Hello", "CoAP");
    coap_payload_advance(pkt, len);

I considered adding an additional parameter to keep track of the total request size
(returned size from coap_opt_finish() incremented by each added payload fragment),
but decided against it to keep consistency with the existing API.

Testing procedure

Issues/PRs references

@benpicco benpicco added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: network Area: Networking labels Mar 26, 2020
@benpicco benpicco requested review from kb2ma and bergzand March 26, 2020 13:44
@benpicco benpicco force-pushed the nanocoap-payload_helper branch from 7ab647d to 743134e Compare March 26, 2020 13:50
@benpicco benpicco added Area: CoAP Area: Constrained Application Protocol implementations and removed Area: network Area: Networking labels Mar 29, 2020
@kb2ma
Copy link
Member

kb2ma commented Mar 31, 2020

Conceptually, these additions are a great idea. I'd like to think a little about how these functions relate to our mechanism for blockwise payload writing. See the coap_blockwise_put... functions. Writing a full payload is like a special case of writing a block.

At the same time, the use of Block options could be a semi-automatic mechanism to handle a big payload. It could be a kind of contract between (g|nano)coap and the application. The application's responsibility is to write the bytes, and (g|nano)coap handles slicing as needed.

We don't need to get there in a single step, but it would useful to have a vision. Of course it also should be the user's choice whether to take on this overhead.

cc: @bergzand, @kaspar030, @leandrolanzieri, @haukepetersen

@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 Apr 6, 2020
@benpicco benpicco force-pushed the nanocoap-payload_helper branch from 743134e to f444f81 Compare April 6, 2020 13:57
@bergzand
Copy link
Member

I would prefer coap_payload_put_bytes to match the block function names.

I do see some use in unifying the functions for blockwise and regular transfers and update the block option if it is already present in the packet.

While it would be nice to automatically start a blockwise transfer when required, I think it is an edge case that we should not try to cover. I expect that most of the time, the rough size (a few bytes or a large amount of bytes) is known at development time and that automatically switching between the two is never used.

For now I think this PR is a small step in the right direction and covers a currently used pattern.

@benpicco
Copy link
Contributor Author

Alright - I also replaces coap_payload_advance with coap_payload_advance_bytes.

@kaspar030
Copy link
Contributor

👍 in general. I'm a bit confused on when this can be used, though.

@benpicco
Copy link
Contributor Author

I'm a bit confused on when this can be used, though.

Whenever a user wants to add payload to a CoAP request - or am I getting the CoAP API wrong here?

@kb2ma
Copy link
Member

kb2ma commented Apr 22, 2020

I do see some use in unifying the functions for blockwise and regular transfers and update the block option if it is already present in the packet.

An example of the value of this ability is for gcoap_get_resource_list() and gcoap_encode_link(), which don't have the context to know the nature of the buffer to which they are writing.

While it would be nice to automatically start a blockwise transfer when required, I think it is an edge case that we should not try to cover.

+1

I have an outline for an implementation that I'll put in an RFC issue.

@bergzand
Copy link
Member

@kb2ma should we wait for your RFC issue before merging here?

@kb2ma
Copy link
Member

kb2ma commented Apr 24, 2020

@kb2ma should we wait for your RFC issue before merging here?

Yes, please. I should have something later today.

@kb2ma
Copy link
Member

kb2ma commented Apr 24, 2020

Please also add the function below to provide equivalent functionality to blockwise.

ssize_t coap_payload_put_char(coap_pkt_t *pkt, char c);

@kb2ma
Copy link
Member

kb2ma commented Apr 24, 2020

#13942 proposes reuse of the functions declared here as-is for blockwise. If that approach looks reasonable, there is no impact on this PR.

@benpicco
Copy link
Contributor Author

I've added coap_payload_put_char(), but I'm not sure when that would be used.

@kb2ma
Copy link
Member

kb2ma commented Apr 28, 2020

Thanks for the additional function. It's part of the diabolical plan hatched in #13942 to make it easy to write a blockwise payload in gcoap. ;-) I'll be back with a formal review in the next day or so.

Copy link
Member

@kb2ma kb2ma left a comment

Choose a reason for hiding this comment

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

See inline comments for mostly mechanical and consistency issues.

Copy link
Member

@kb2ma kb2ma left a comment

Choose a reason for hiding this comment

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

Fixups look good. Please squash!

This adds two functions `coap_payload_add()` and `coap_payload_advance()`.

 - `coap_payload_add()` will add n bytes to the payload buffer and advance
    payload pointer accordingly.

    const char hello[] = "Hello CoAP!";
    coap_payload_add(pkt, hello, sizeof(hello));

 - `coap_payload_advance()` will advance the payload buffer after data
    has been added to it.

    int len = snprintf(pkt->payload, pkt->payload_len, "%s %s!", "Hello", "CoAP");
    coap_payload_advance(pkt, len);

I considered adding an additional parameter to keep track of the total request size
(returned size from coap_opt_finish() incremented by each added payload fragment),
but decided against it to keep consistency with the existing API.
@benpicco benpicco force-pushed the nanocoap-payload_helper branch from ac3e8e2 to e5c20b1 Compare May 1, 2020 11:58
@kb2ma kb2ma merged commit 595e8c6 into RIOT-OS:master May 3, 2020
@benpicco benpicco deleted the nanocoap-payload_helper branch May 3, 2020 13:22
@miri64 miri64 added this to the Release 2020.07 milestone Jun 24, 2020
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 CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants