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.

Sign up for GitHub

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

Jump to bottom

sys/net/nanocoap: Introduce coap_build_pkt_t and use it #17544

Closed
wants to merge 3 commits into from
Closed

sys/net/nanocoap: Introduce coap_build_pkt_t and use it #17544

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4b71cd2
sys/net/nanocoap: Introduce coap_build_pkt_t and use it
benpicco Jan 20, 2022
dc7df25
net/gcoap: introduce and use gcoap_resource_t
benpicco Jan 22, 2022
e2ab8a1
sys/net/nanocoap: use coap_build_pkt_t in coap_blockwise_put_bytes()
benpicco Jan 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions examples/cord_ep/main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,14 +75,14 @@ static ssize_t _handler_info(coap_pkt_t *pdu,
return resp_len + slen;
}

static const coap_resource_t _resources[] = {
static const gcoap_resource_t _resources[] = {
{ "/node/info", COAP_GET, _handler_info, NULL },
{ "/sense/hum", COAP_GET, _handler_dummy, NULL },
{ "/sense/temp", COAP_GET, _handler_dummy, NULL }
};

static gcoap_listener_t _listener = {
.resources = (coap_resource_t *)&_resources[0],
.resources = _resources,
.resources_len = ARRAY_SIZE(_resources),
.next = NULL
};
Expand Down
4 changes: 2 additions & 2 deletions examples/cord_epsim/main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,14 +57,14 @@ static ssize_t handler_text(coap_pkt_t *pdu, uint8_t *buf, size_t len, void *ctx
return text_resp(pdu, buf, len, (char *)ctx, COAP_FORMAT_TEXT);
}

static const coap_resource_t resources[] = {
static const gcoap_resource_t resources[] = {
{ "/riot/bar", COAP_GET, handler_text, "foo" },
{ "/riot/foo", COAP_GET, handler_text, "bar" },
{ "/riot/info", COAP_GET, handler_info, NULL }
};

static gcoap_listener_t listener = {
.resources = &resources[0],
.resources = resources,
.resources_len = ARRAY_SIZE(resources),
.next = NULL
};
Expand Down
6 changes: 3 additions & 3 deletions examples/gcoap/server.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,13 +56,13 @@ static const credman_credential_t credential = {
};
#endif

static ssize_t _encode_link(const coap_resource_t *resource, char *buf,
static ssize_t _encode_link(const gcoap_resource_t *resource, char *buf,
size_t maxlen, coap_link_encoder_ctx_t *context);
static ssize_t _stats_handler(coap_pkt_t* pdu, uint8_t *buf, size_t len, void *ctx);
static ssize_t _riot_board_handler(coap_pkt_t* pdu, uint8_t *buf, size_t len, void *ctx);

/* CoAP resources. Must be sorted by path (ASCII order). */
static const coap_resource_t _resources[] = {
static const gcoap_resource_t _resources[] = {
{ "/cli/stats", COAP_GET | COAP_PUT, _stats_handler, NULL },
{ "/riot/board", COAP_GET, _riot_board_handler, NULL },
};
Expand All @@ -83,7 +83,7 @@ static gcoap_listener_t _listener = {


/* Adds link format params to resource list */
static ssize_t _encode_link(const coap_resource_t *resource, char *buf,
static ssize_t _encode_link(const gcoap_resource_t *resource, char *buf,
size_t maxlen, coap_link_encoder_ctx_t *context) {
ssize_t res = gcoap_encode_link(resource, buf, maxlen, context);
if (res > 0) {
Expand Down
64 changes: 29 additions & 35 deletions examples/nanocoap_server/coap_handler.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,62 +22,57 @@ static const uint8_t block2_intro[] = "This is RIOT (Version: ";
static const uint8_t block2_board[] = " running on a ";
static const uint8_t block2_mcu[] = " board with a ";

static ssize_t _echo_handler(coap_pkt_t *pkt, uint8_t *buf, size_t len, void *context)
static ssize_t _echo_handler(coap_pkt_t *pkt, coap_build_pkt_t *response, void *context)
{
(void)context;
char uri[CONFIG_NANOCOAP_URI_MAX];

if (coap_get_uri_path(pkt, (uint8_t *)uri) <= 0) {
return coap_reply_simple(pkt, COAP_CODE_INTERNAL_SERVER_ERROR, buf,
len, COAP_FORMAT_TEXT, NULL, 0);
return coap_reply_simple(pkt, COAP_CODE_INTERNAL_SERVER_ERROR, response,
COAP_FORMAT_TEXT, NULL, 0);
}
char *sub_uri = uri + strlen("/echo/");
size_t sub_uri_len = strlen(sub_uri);
return coap_reply_simple(pkt, COAP_CODE_CONTENT, buf, len, COAP_FORMAT_TEXT,
return coap_reply_simple(pkt, COAP_CODE_CONTENT, response, COAP_FORMAT_TEXT,
(uint8_t *)sub_uri, sub_uri_len);
}

static ssize_t _riot_board_handler(coap_pkt_t *pkt, uint8_t *buf, size_t len, void *context)
static ssize_t _riot_board_handler(coap_pkt_t *pkt, coap_build_pkt_t *response, void *context)
{
(void)context;
return coap_reply_simple(pkt, COAP_CODE_205, buf, len,
COAP_FORMAT_TEXT, (uint8_t*)RIOT_BOARD, strlen(RIOT_BOARD));
return coap_reply_simple(pkt, COAP_CODE_205, response, COAP_FORMAT_TEXT,
RIOT_BOARD, strlen(RIOT_BOARD));
}

static ssize_t _riot_block2_handler(coap_pkt_t *pkt, uint8_t *buf, size_t len, void *context)
static ssize_t _riot_block2_handler(coap_pkt_t *pkt, coap_build_pkt_t *response, void *context)
{
(void)context;
coap_block_slicer_t slicer;
coap_block2_init(pkt, &slicer);
uint8_t *payload = buf + coap_get_total_hdr_len(pkt);

uint8_t *bufpos = payload;

bufpos += coap_put_option_ct(bufpos, 0, COAP_FORMAT_TEXT);
bufpos += coap_opt_put_block2(bufpos, COAP_OPT_CONTENT_FORMAT, &slicer, 1);
*bufpos++ = 0xff;
response->cur += coap_put_option_ct(response->cur, 0, COAP_FORMAT_TEXT);
response->cur += coap_opt_put_block2(response->cur, COAP_OPT_CONTENT_FORMAT, &slicer, 1);
*response->cur++ = 0xff;

/* Add actual content */
bufpos += coap_blockwise_put_bytes(&slicer, bufpos, block2_intro, sizeof(block2_intro)-1);
bufpos += coap_blockwise_put_bytes(&slicer, bufpos, (uint8_t*)RIOT_VERSION, strlen(RIOT_VERSION));
bufpos += coap_blockwise_put_char(&slicer, bufpos, ')');
bufpos += coap_blockwise_put_bytes(&slicer, bufpos, block2_board, sizeof(block2_board)-1);
bufpos += coap_blockwise_put_bytes(&slicer, bufpos, (uint8_t*)RIOT_BOARD, strlen(RIOT_BOARD));
bufpos += coap_blockwise_put_bytes(&slicer, bufpos, block2_mcu, sizeof(block2_mcu)-1);
bufpos += coap_blockwise_put_bytes(&slicer, bufpos, (uint8_t*)RIOT_MCU, strlen(RIOT_MCU));
coap_blockwise_put_bytes(&slicer, response, block2_intro, sizeof(block2_intro)-1);
Copy link
Member

Choose a reason for hiding this comment

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

This clearly is an improvement in regard to usability :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And it can be used to avoid having to copy the payload into a separate buffer #17559

coap_blockwise_put_bytes(&slicer, response, (uint8_t*)RIOT_VERSION, strlen(RIOT_VERSION));
coap_blockwise_put_char(&slicer, response, ')');
coap_blockwise_put_bytes(&slicer, response, block2_board, sizeof(block2_board)-1);
coap_blockwise_put_bytes(&slicer, response, (uint8_t*)RIOT_BOARD, strlen(RIOT_BOARD));
coap_blockwise_put_bytes(&slicer, response, block2_mcu, sizeof(block2_mcu)-1);
coap_blockwise_put_bytes(&slicer, response, (uint8_t*)RIOT_MCU, strlen(RIOT_MCU));
/* To demonstrate individual chars */
bufpos += coap_blockwise_put_char(&slicer, bufpos, ' ');
bufpos += coap_blockwise_put_char(&slicer, bufpos, 'M');
bufpos += coap_blockwise_put_char(&slicer, bufpos, 'C');
bufpos += coap_blockwise_put_char(&slicer, bufpos, 'U');
bufpos += coap_blockwise_put_char(&slicer, bufpos, '.');

unsigned payload_len = bufpos - payload;
return coap_block2_build_reply(pkt, COAP_CODE_205,
buf, len, payload_len, &slicer);
coap_blockwise_put_char(&slicer, response, ' ');
coap_blockwise_put_char(&slicer, response, 'M');
coap_blockwise_put_char(&slicer, response, 'C');
coap_blockwise_put_char(&slicer, response, 'U');
coap_blockwise_put_char(&slicer, response, '.');

return coap_block2_build_reply(pkt, COAP_CODE_205, response, &slicer);
}

static ssize_t _riot_value_handler(coap_pkt_t *pkt, uint8_t *buf, size_t len, void *context)
static ssize_t _riot_value_handler(coap_pkt_t *pkt, coap_build_pkt_t *response, void *context)
{
(void) context;

Expand Down Expand Up @@ -108,11 +103,10 @@ static ssize_t _riot_value_handler(coap_pkt_t *pkt, uint8_t *buf, size_t len, vo
}
}

return coap_reply_simple(pkt, code, buf, len,
COAP_FORMAT_TEXT, (uint8_t*)rsp, p);
return coap_reply_simple(pkt, code, response, COAP_FORMAT_TEXT, (uint8_t*)rsp, p);
}

ssize_t _sha256_handler(coap_pkt_t* pkt, uint8_t *buf, size_t len, void *context)
ssize_t _sha256_handler(coap_pkt_t* pkt, coap_build_pkt_t *response, void *context)
{
(void)context;

Expand Down Expand Up @@ -149,7 +143,7 @@ ssize_t _sha256_handler(coap_pkt_t* pkt, uint8_t *buf, size_t len, void *context
result_len = SHA256_DIGEST_LENGTH * 2;
}

ssize_t reply_len = coap_build_reply(pkt, result, buf, len, 0);
ssize_t reply_len = coap_build_reply(pkt, result, response);
uint8_t *pkt_pos = (uint8_t*)pkt->hdr + reply_len;
if (blockwise) {
pkt_pos += coap_opt_put_block1_control(pkt_pos, 0, &block1);
Expand Down
4 changes: 2 additions & 2 deletions examples/suit_update/coap_handler.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,10 @@
#include "suit/transport/coap.h"
#include "kernel_defines.h"

static ssize_t _riot_board_handler(coap_pkt_t *pkt, uint8_t *buf, size_t len, void *context)
static ssize_t _riot_board_handler(coap_pkt_t *pkt, coap_build_pkt_t *response, void *context)
{
(void)context;
return coap_reply_simple(pkt, COAP_CODE_205, buf, len,
return coap_reply_simple(pkt, COAP_CODE_205, response,
COAP_FORMAT_TEXT, (uint8_t*)RIOT_BOARD, strlen(RIOT_BOARD));
}

Expand Down
42 changes: 34 additions & 8 deletions sys/include/net/gcoap.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@
* this by uncommenting the appropriate lines in gcoap's make file.
*
* gcoap allows an application to specify a collection of request resource paths
* it wants to be notified about. Create an array of resources (coap_resource_t
* it wants to be notified about. Create an array of resources (gcoap_resource_t
Copy link
Contributor

@kfessel kfessel Mar 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

could you move that coap_resource_t -> gcoap_resource_t to a seperate PR ?
(i seems to already be a separate commit)

Copy link
Contributor

Choose a reason for hiding this comment

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

the same might be done for the link_encoder

(i like these changes as they clearup where a structure is defined)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#17804 there you go

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately this causes some issues with the Rust wrappers which in turn require a CI upgrade :(

So if you don't insist on splitting this, could we move forward with this as a single PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

wouldn't this PR trigger the same issue (if we restart murdock now) until rust is upgraded

Copy link
Contributor

Choose a reason for hiding this comment

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

gcoap.h:210-308 explains how to do block transfers with gcoap:
the nanocoap functions are used i think they are missing the initialization for the respose structure after this PR is mergerd.

This section also provides a link to an example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wouldn't this PR trigger the same issue (if we restart murdock now) until rust is upgraded

no, the rust wrapper uses the presence of coap_build_pkt_t to detect whether to use the old or the new API.
It has now been fixed to check for gcoap_resource_t instead.

the nanocoap functions are used i think they are missing the initialization for the respose structure after this PR is mergerd.

Maybe I miss something, but those are not using any of the functions that makes use of coap_build_pkt_t?
The new type is not used by gcoap (that's why I introduced gcoap_resource_t) - it should be, but I was hoping this could be done as a follow-up.

Copy link
Contributor

@kfessel kfessel Mar 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

gcoap is using the nanocoap functions to generate the header coap_pkgs, while most of these generator functions do not make any use of buf and len/coap_build_pkt_t, block transfer does -> with this pr gcoap would loose nanocoaps block-transfer abilities (which is described in gcoaps documentation (gcoap.h:210-308) (also featuring an external example).

I am no sure about the state of #17168 and #16855 especial in looking at block-transfers. May be this API change is also welcome to gcoap.

While this PR tries to break just nanocoaps-API it implicitly also breaks gcoaps-API (@block-transfers).

maybe @chrysn or @miri64 have some insight into what changes are going to happen in gcoap maybe this API-change is welcome and should just be forwarded to gcoap ( which might be easier since it may not need the introduction of 'gcoap_rescource_t').

Copy link
Contributor

Choose a reason for hiding this comment

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

#17801 implements caching for gcoap and somehow also for nanocoap (using the old buf an len but introducing a nanocoap_-prefix)

Copy link
Contributor Author

@benpicco benpicco Mar 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

while most of these generator functions do not make any use of buf and len/coap_build_pkt_t, block transfer does -> with this pr gcoap would loose nanocoaps block-transfer abilities (which is described in gcoaps documentation (gcoap.h:210-308)

Please explain to me which function(s) you mean. If Gcoap would use on of the converted nanocoap functions, that would be a compilation error as Gcoap does not yet make use of coap_build_pkt_t.

There are functions in nanocoap.c that are exclusively used by Gcoap, but as far as I can tell those are not touched by this PR.

Blockwise GET is still working in examples/gcoap, not sure if we also have a server example for Gcoap, but I expect no surprises there.

* structs) ordered by the resource path, specifically the ASCII encoding of
* the path characters (digit and capital precede lower case). Use
* gcoap_register_listener() at application startup to pass in these resources,
Expand Down Expand Up @@ -649,6 +649,32 @@ extern "C" {

/** @} */

/**
* @brief Resource handler type
*
* Functions that implement this must be prepared to be called multiple times
* for the same request, as the server implementations do not perform message
* deduplication. That optimization is [described in the CoAP
* specification](https://tools.ietf.org/html/rfc7252#section-4.5).
*
* This should be trivial for requests of the GET, PUT, DELETE, FETCH and
* iPATCH methods, as they are defined as idempotent methods in CoAP.
*
* For POST, PATCH and other non-idempotent methods, this is an additional
* requirement introduced by the contract of this type.
*/
typedef ssize_t (*gcoap_handler_t)(coap_pkt_t *pkt, uint8_t *rbuf, size_t rlen, void *context);

/**
* @brief Type for CoAP resource entry
*/
typedef struct {
const char *path; /**< URI path of resource */
coap_method_flags_t methods; /**< OR'ed methods this resource allows */
gcoap_handler_t handler; /**< ptr to resource handler */
void *context; /**< ptr to user defined context data */
} gcoap_resource_t;

/**
* @brief Context information required to write a resource link
*/
Expand All @@ -670,7 +696,7 @@ typedef struct {
* @return count of bytes written to @p buf (or writable if @p buf is null)
* @return -1 on error
*/
typedef ssize_t (*gcoap_link_encoder_t)(const coap_resource_t *resource, char *buf,
typedef ssize_t (*gcoap_link_encoder_t)(const gcoap_resource_t *resource, char *buf,
size_t maxlen, coap_link_encoder_ctx_t *context);

/**
Expand Down Expand Up @@ -701,7 +727,7 @@ typedef struct gcoap_listener gcoap_listener_t;
* @return GCOAP_RESOURCE_ERROR on processing failure of the request
*/
typedef int (*gcoap_request_matcher_t)(gcoap_listener_t *listener,
const coap_resource_t **resource,
const gcoap_resource_t **resource,
coap_pkt_t *pdu);

/**
Expand Down Expand Up @@ -738,7 +764,7 @@ typedef struct {
* @brief A modular collection of resources for a server
*/
struct gcoap_listener {
const coap_resource_t *resources; /**< First element in the array of
const gcoap_resource_t *resources; /**< First element in the array of
* resources; must order alphabetically */
size_t resources_len; /**< Length of array */
/**
Expand Down Expand Up @@ -816,7 +842,7 @@ struct gcoap_request_memo {
*/
typedef struct {
sock_udp_ep_t *observer; /**< Client endpoint; unused if null */
const coap_resource_t *resource; /**< Entity being observed */
const gcoap_resource_t *resource; /**< Entity being observed */
uint8_t token[GCOAP_TOKENLEN_MAX]; /**< Client token for notifications */
unsigned token_len; /**< Actual length of token attribute */
gcoap_socket_t socket; /**< Transport type to observer */
Expand Down Expand Up @@ -1020,7 +1046,7 @@ static inline ssize_t gcoap_response(coap_pkt_t *pdu, uint8_t *buf,
* @return GCOAP_OBS_INIT_UNUSED if no observer for resource
*/
int gcoap_obs_init(coap_pkt_t *pdu, uint8_t *buf, size_t len,
const coap_resource_t *resource);
const gcoap_resource_t *resource);

/**
* @brief Sends a buffer containing a CoAP Observe notification to the
Expand All @@ -1036,7 +1062,7 @@ int gcoap_obs_init(coap_pkt_t *pdu, uint8_t *buf, size_t len,
* @return 0 if cannot send
*/
size_t gcoap_obs_send(const uint8_t *buf, size_t len,
const coap_resource_t *resource);
const gcoap_resource_t *resource);

/**
* @brief Provides important operational statistics
Expand Down Expand Up @@ -1112,7 +1138,7 @@ static inline int gcoap_get_resource_list(void *buf, size_t maxlen, uint8_t cf)
* @return count of bytes written to @p buf (or writable if @p buf is null)
* @return -1 on error
*/
ssize_t gcoap_encode_link(const coap_resource_t *resource, char *buf,
ssize_t gcoap_encode_link(const gcoap_resource_t *resource, char *buf,
size_t maxlen, coap_link_encoder_ctx_t *context);

#if IS_USED(MODULE_GCOAP_DTLS) || defined(DOXYGEN)
Expand Down
Loading