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

gcoap+nanocoap: Missing parameter docs for function pointer types #10080

Closed
jnohlgard opened this issue Sep 29, 2018 · 4 comments
Closed

gcoap+nanocoap: Missing parameter docs for function pointer types #10080

jnohlgard opened this issue Sep 29, 2018 · 4 comments
Assignees
Labels
Area: CoAP Area: Constrained Application Protocol implementations Area: doc Area: Documentation State: stale State: The issue / PR has no activity for >185 days Type: new feature The issue requests / The PR implemements a new feature for RIOT

Comments

@jnohlgard
Copy link
Member

Description

There is no documentation for the function parameters for the handler function pointer types used in nanocoap and gcoap: coap_handler_t in nanocoap and gcoap_resp_handler_t in gcoap

This should be trivial to add for someone with a good knowledge of nanocoap and gcoap internals, (@kb2ma or @haukepetersen?)

/**
* @brief Resource handler type
*/
typedef ssize_t (*coap_handler_t)(coap_pkt_t *pkt, uint8_t *buf, size_t len, void *context);

/**
* @brief Handler function for a server response, including the state for the
* originating request
*
* If request timed out, the packet header is for the request.
*/
typedef void (*gcoap_resp_handler_t)(unsigned req_state, coap_pkt_t* pdu,
sock_udp_ep_t *remote);

My original question is how do I, as an application writer in a CoAP server, use the buf and len parameters in the handler? My current assumption is that buf is a pointer to somewhere in the packet buffer and len is the length of the buffer. How can I be certain that my application will be given enough space to write its response payload?
The CLI example only forwards the buf and len parameters directly to gcoap_resp_init, and then just assumes there is enough space to write RIOT_BOARD at pdu->payload, but this assumption seems like a bad idea in a real world application.

Useful links

Generated API docs:

@jnohlgard jnohlgard added Area: doc Area: Documentation Feature Request Area: CoAP Area: Constrained Application Protocol implementations labels Sep 29, 2018
@miri64 miri64 added the Type: new feature The issue requests / The PR implemements a new feature for RIOT label Sep 30, 2018
@kb2ma
Copy link
Member

kb2ma commented Oct 1, 2018

Good question @gebart, and yes more documentation is a good idea. Sorry for the long winded response.

Just to be clear, both nanocoap and gcoap use the same handler type, coap_handler_t, for a server to respond to a request. This is documented in the first sentence of Creating a response. For a client, gcoap_resp_handler_t is a gcoap-specific handler for client reception of a server response. gcoap runs in its own thread and notifies the client asynchronously, while nanocoap expects the client to react synchronously when nanocoap_request() or nanocoap_get() return.

Also, #10036 provides much needed module level documentation for nanocoap. Please review that if you haven't seen it.

So, let me document coap_handler_t, and eventually we can add it to the source file.

/**
 * @brief   Resource handler type
 * 
 * param[in]  pkt      CoAP PDU data for request
 * param[out] buf      buffer on which to write the response
 * param[out] len      length of buf
 * param[in] context   context provided by resource
 */
typedef ssize_t (*coap_handler_t)(coap_pkt_t *pkt, uint8_t *buf, size_t len, void *context);

This should explain the basic nature of buf and len. As a practical matter, buf also is the buffer used for reading the request as well. This dual use is not a requirement, but both gcoap and nanocoap use buf this way. This is pointed out in the module documentation both for gcoap and nanocoap. gcoap and nanocoap write the response differently, so I'll describe them one at a time below.

gcoap reuses pkt to store data as it builds the response. In particular gcoap_resp_init() updates the payload and payload_len attributes. These attributes also are updated if you add an option via the coap_opt_add_xxx() functions. So,the use model is that an application handler uses these attributes to know where to write the payload and how much space is available. The application handler also must report the length of the payload they wrote in gcoap_finish().

Yes, you are correct that _riot_board_handler() in the gcoap CLI example is sloppy about the payload length. Certainly as an example this should be fixed. gcoap_finish() also should assert (or maybe return -ENOSPC) if the provided payload length is too great.

Historically, nanocoap has differed from gcoap. It does not use the pkt to store data as it builds the response, only the buffer. In this case, the coap_opt_put_xxx() functions return the number of bytes written. It is up the application handler to track the length remaining in the buffer. I say "historically" because we recently added coap_pkt_init(), which allows nanocoap also to use the pkt when writing the response, like gcoap. This is part of a drive to unite/reuse APIs between nanocoap and gcoap.

gcoap specifies the size of the response buffer in GCOAP_PDU_BUF_SIZE, and nanocoap accepts the buffer in nanocoap_server(). The CoAP block extension provides a way to split a large payload across packets. Block1 support (for a PUT/POST request) was added in #8788, and Block2 (for a GET response) is pending in #8932. By the way, I mentioned in the Block2 PR that it would be useful to have a generic function to accept and write payload buffer content so that we can validate the size before writing.

@stale
Copy link

stale bot commented Aug 10, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

@stale stale bot added the State: stale State: The issue / PR has no activity for >185 days label Aug 10, 2019
@stale
Copy link

stale bot commented Aug 10, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

@miri64 miri64 removed the State: stale State: The issue / PR has no activity for >185 days label Aug 10, 2019
@stale
Copy link

stale bot commented Feb 11, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

@stale stale bot added the State: stale State: The issue / PR has no activity for >185 days label Feb 11, 2020
@stale stale bot closed this as completed Mar 13, 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 Area: doc Area: Documentation State: stale State: The issue / PR has no activity for >185 days Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Development

No branches or pull requests

3 participants