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

net/coap: Block optimizations #11057

Merged
merged 7 commits into from
Oct 4, 2019
Merged

net/coap: Block optimizations #11057

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
233dd31
net/nanocoap: add generic uint block option
kb2ma Feb 24, 2019
dae9c4a
net/nanocoap: use coap_opt_add_uint() and remove unused
kb2ma Feb 24, 2019
efb75c5
net/coap: improve slicer to block helper
kb2ma Feb 24, 2019
5eaa51e
net/nanocoap: remove unused function
kb2ma Feb 24, 2019
2abcbc0
net/coap: add generic get block function
kb2ma Feb 24, 2019
e22d472
net/nanocoap: initialize offset attribute
kb2ma Feb 24, 2019
2dc4209
net/nanocoap: optimize determination of exponent for block szx
kb2ma Sep 30, 2019
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
70 changes: 54 additions & 16 deletions sys/include/net/nanocoap.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -717,6 +717,26 @@ size_t coap_blockwise_put_bytes(coap_block_slicer_t *slicer, uint8_t *bufpos,
*/
size_t coap_blockwise_put_char(coap_block_slicer_t *slicer, uint8_t *bufpos, char c);

/**
* @brief Block option getter
*
* This function gets a CoAP packet's block option and parses it into a helper
* structure.
*
* If no block option is present in @p pkt, the values in @p block will be
* initialized with zero. That implies both block->offset and block->more are
* also valid in that case, as packet with offset==0 and more==0 means it contains
* all the payload for the corresponding request.
*
* @param[in] pkt pkt to work on
* @param[out] block ptr to preallocated coap_block1_t structure
* @param[in] option block1 or block2
*
* @returns 0 if block option not present
* @returns 1 if structure has been filled
*/
int coap_get_block(coap_pkt_t *pkt, coap_block1_t *block, uint16_t option);

/**
* @brief Block1 option getter
*
Expand All @@ -729,23 +749,29 @@ size_t coap_blockwise_put_char(coap_block_slicer_t *slicer, uint8_t *bufpos, cha
* all the payload for the corresponding request.
*
* @param[in] pkt pkt to work on
* @param[out] block1 ptr to preallocated coap_block1_t structure
* @param[out] block ptr to preallocated coap_block1_t structure
*
* @returns 0 if block1 option not present
* @returns 1 if structure has been filled
*/
int coap_get_block1(coap_pkt_t *pkt, coap_block1_t *block1);
static inline int coap_get_block1(coap_pkt_t *pkt, coap_block1_t *block)
{
return coap_get_block(pkt, block, COAP_OPT_BLOCK1);
}

/**
* @brief Block2 option getter
*
* @param[in] pkt pkt to work on
* @param[out] block2 ptr to preallocated coap_block1_t structure
* @param[out] block ptr to preallocated coap_block1_t structure
*
* @returns 0 if block2 option not present
* @returns 1 if structure has been filled
*/
int coap_get_block2(coap_pkt_t *pkt, coap_block1_t *block2);
static inline int coap_get_block2(coap_pkt_t *pkt, coap_block1_t *block)
{
return coap_get_block(pkt, block, COAP_OPT_BLOCK2);
}

/**
* @brief Generic block option getter
Expand Down Expand Up @@ -1039,17 +1065,18 @@ static inline size_t coap_opt_put_block2(uint8_t *buf, uint16_t lastonum,
}

/**
* @brief Insert block option into buffer from block struct
* @brief Encode the given uint option into buffer
*
* @param[in] buf buffer to write to
* @param[in] lastonum last option number (must be < @p option)
* @param[in] block block option attribute struct
* @param[in] option option number (block1 or block2)
* @param[out] buf buffer to write to
* @param[in] lastonum number of previous option (for delta calculation),
* or 0 for first option
* @param[in] onum number of option
* @param[in] value value to encode
*
* @returns amount of bytes written to @p buf
*/
size_t coap_opt_put_block_object(uint8_t *buf, uint16_t lastonum,
coap_block1_t *block, uint16_t option);
size_t coap_opt_put_uint(uint8_t *buf, uint16_t lastonum, uint16_t onum,
uint32_t value);
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this also pass the remaining space available in buf so we don't silently overflow?


/**
* @brief Insert block1 option into buffer in control usage
Expand All @@ -1063,7 +1090,8 @@ size_t coap_opt_put_block_object(uint8_t *buf, uint16_t lastonum,
static inline size_t coap_opt_put_block1_control(uint8_t *buf, uint16_t lastonum,
coap_block1_t *block)
{
return coap_opt_put_block_object(buf, lastonum, block, COAP_OPT_BLOCK1);
return coap_opt_put_uint(buf, lastonum, COAP_OPT_BLOCK1,
(block->blknum << 4) | block->szx | (block->more ? 0x8 : 0));
}

/**
Expand All @@ -1080,8 +1108,9 @@ static inline size_t coap_opt_put_block1_control(uint8_t *buf, uint16_t lastonum
static inline size_t coap_opt_put_block2_control(uint8_t *buf, uint16_t lastonum,
coap_block1_t *block)
{
block->more = 0;
return coap_opt_put_block_object(buf, lastonum, block, COAP_OPT_BLOCK2);
/* block.more must be zero, so no need to 'or' it in */
return coap_opt_put_uint(buf, lastonum, COAP_OPT_BLOCK2,
(block->blknum << 4) | block->szx);
}

/**
Expand Down Expand Up @@ -1216,7 +1245,12 @@ size_t coap_put_option(uint8_t *buf, uint16_t lastonum, uint16_t onum, const uin
*
* @returns amount of bytes written to @p buf
*/
size_t coap_put_option_block1(uint8_t *buf, uint16_t lastonum, unsigned blknum, unsigned szx, int more);
static inline size_t coap_put_option_block1(uint8_t *buf, uint16_t lastonum,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we change this to coap_put_option_block()? and add option as a parameter?

Copy link
Member Author

Choose a reason for hiding this comment

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

How about actually deprecating this function in favor of using coap_opt_put_block1_control(), which already is used in the nanocoap_server example? One of my goals with this sequence of PRs was consistent names for block functions.

Either way, I would like to put the deprecation in a separate PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

unsigned blknum, unsigned szx, int more)
{
return coap_opt_put_uint(buf, lastonum, COAP_OPT_BLOCK1,
(blknum << 4) | szx | (more ? 0x8 : 0));
}

/**
* @brief Insert content type option into buffer
Expand All @@ -1228,7 +1262,11 @@ size_t coap_put_option_block1(uint8_t *buf, uint16_t lastonum, unsigned blknum,
*
* @returns amount of bytes written to @p buf
*/
size_t coap_put_option_ct(uint8_t *buf, uint16_t lastonum, uint16_t content_type);
static inline size_t coap_put_option_ct(uint8_t *buf, uint16_t lastonum,
uint16_t content_type)
{
return coap_opt_put_uint(buf, lastonum, COAP_OPT_CONTENT_FORMAT, content_type);
}
/**@}*/


Expand Down
106 changes: 25 additions & 81 deletions sys/net/application_layer/nanocoap/nanocoap.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
#include <stdio.h>
#include <string.h>

#include "bitarithm.h"
#include "net/nanocoap.h"

#define ENABLE_DEBUG (0)
Expand Down Expand Up @@ -653,35 +654,20 @@ size_t coap_put_option(uint8_t *buf, uint16_t lastonum, uint16_t onum, const uin
return (size_t)n;
}

size_t coap_put_option_ct(uint8_t *buf, uint16_t lastonum, uint16_t content_type)
{
if (content_type == 0) {
return coap_put_option(buf, lastonum, COAP_OPT_CONTENT_FORMAT, NULL, 0);
}
else if (content_type <= 255) {
uint8_t tmp = content_type;
return coap_put_option(buf, lastonum, COAP_OPT_CONTENT_FORMAT, &tmp, sizeof(tmp));
}
else {
return coap_put_option(buf, lastonum, COAP_OPT_CONTENT_FORMAT, (uint8_t *)&content_type, sizeof(content_type));
}
}

static unsigned _size2szx(size_t size)
kb2ma marked this conversation as resolved.
Show resolved Hide resolved
{
unsigned szx = 0;
assert(size <= 1024);

while (size) {
size = size >> 1;
szx++;
}
/* Size exponent + 1 */
assert(szx >= 5);
return szx - 5;
/* We must wait to subract the szx offset of 4 until after the assert below.
* Input should be a power of two, but if not it may have a stray low order
* '1' bit that would invalidate the subtraction. */
unsigned szx = bitarithm_lsb(size);

assert(szx >= 4);
return szx - 4;
}

static unsigned _slicer_blknum(coap_block_slicer_t *slicer)
static unsigned _slicer2blkopt(coap_block_slicer_t *slicer, bool more)
{
size_t blksize = slicer->end - slicer->start;
size_t start = slicer->start;
Expand All @@ -691,51 +677,23 @@ static unsigned _slicer_blknum(coap_block_slicer_t *slicer)
start -= blksize;
blknum++;
}
return blknum;
}

static size_t coap_put_option_block(uint8_t *buf, uint16_t lastonum, unsigned blknum, unsigned szx, int more, uint16_t option)
{
uint32_t blkopt = (blknum << 4) | szx | (more ? 0x8 : 0);
size_t olen = _encode_uint(&blkopt);

return coap_put_option(buf, lastonum, option, (uint8_t *)&blkopt, olen);
}

size_t coap_put_option_block1(uint8_t *buf, uint16_t lastonum, unsigned blknum, unsigned szx, int more)
{
return coap_put_option_block(buf, lastonum, blknum, szx, more, COAP_OPT_BLOCK1);
return (blknum << 4) | _size2szx(blksize) | (more ? 0x8 : 0);
}

int coap_get_block(coap_pkt_t *pkt, coap_block1_t *block, uint16_t blkopt)
int coap_get_block(coap_pkt_t *pkt, coap_block1_t *block, uint16_t option)
{
uint32_t blknum;
unsigned szx;

block->more = coap_get_blockopt(pkt, blkopt, &blknum, &szx);
block->more = coap_get_blockopt(pkt, option, &block->blknum, &block->szx);
if (block->more >= 0) {
block->offset = blknum << (szx + 4);
block->offset = block->blknum << (block->szx + 4);
}
else {
block->offset = 0;
}

block->blknum = blknum;
block->szx = szx;

return (block->more >= 0);
}

int coap_get_block1(coap_pkt_t *pkt, coap_block1_t *block)
{
return coap_get_block(pkt, block, COAP_OPT_BLOCK1);
}

int coap_get_block2(coap_pkt_t *pkt, coap_block1_t *block)
{
return coap_get_block(pkt, block, COAP_OPT_BLOCK2);
}

size_t coap_put_block1_ok(uint8_t *pkt_pos, coap_block1_t *block1, uint16_t lastonum)
{
if (block1->more >= 1) {
Expand All @@ -749,23 +707,9 @@ size_t coap_put_block1_ok(uint8_t *pkt_pos, coap_block1_t *block1, uint16_t last
size_t coap_opt_put_block(uint8_t *buf, uint16_t lastonum, coap_block_slicer_t *slicer,
bool more, uint16_t option)
{
coap_block1_t block;

coap_block_object_init(&block, _slicer_blknum(slicer),
slicer->end - slicer->start, more);

slicer->opt = buf;

return coap_opt_put_block_object(buf, lastonum, &block, option);
}

size_t coap_opt_put_block_object(uint8_t *buf, uint16_t lastonum,
coap_block1_t *block, uint16_t option)
{
uint32_t blkopt = (block->blknum << 4) | block->szx | (block->more ? 0x8 : 0);
size_t olen = _encode_uint(&blkopt);

return coap_put_option(buf, lastonum, option, (uint8_t *)&blkopt, olen);
return coap_opt_put_uint(buf, lastonum, option, _slicer2blkopt(slicer, more));
}

size_t coap_opt_put_string(uint8_t *buf, uint16_t lastonum, uint16_t optnum,
Expand Down Expand Up @@ -810,6 +754,14 @@ size_t coap_opt_put_string(uint8_t *buf, uint16_t lastonum, uint16_t optnum,
return bufpos - buf;
}

size_t coap_opt_put_uint(uint8_t *buf, uint16_t lastonum, uint16_t onum,
uint32_t value)
{
unsigned uint_len = _encode_uint(&value);

return coap_put_option(buf, lastonum, onum, (uint8_t *)&value, uint_len);
}

/* Common functionality for addition of an option */
static ssize_t _add_opt_pkt(coap_pkt_t *pkt, uint16_t optnum, const uint8_t *val,
size_t val_len)
Expand Down Expand Up @@ -900,13 +852,9 @@ ssize_t coap_opt_add_uint(coap_pkt_t *pkt, uint16_t optnum, uint32_t value)
ssize_t coap_opt_add_block(coap_pkt_t *pkt, coap_block_slicer_t *slicer,
bool more, uint16_t option)
{
uint32_t blkopt = (_slicer_blknum(slicer) << 4);
blkopt |= _size2szx(slicer->end - slicer->start);
blkopt |= (more ? 0x8 : 0);

slicer->opt = pkt->payload;

return coap_opt_add_uint(pkt, option, blkopt);
return coap_opt_add_uint(pkt, option, _slicer2blkopt(slicer, more));
}

ssize_t coap_opt_finish(coap_pkt_t *pkt, uint16_t flags)
Expand All @@ -932,6 +880,7 @@ void coap_block_object_init(coap_block1_t *block, size_t blknum, size_t blksize,
block->szx = _size2szx(blksize);
block->blknum = blknum;
block->more = more;
block->offset = block->blknum << (block->szx + 4);
}

void coap_block_slicer_init(coap_block_slicer_t *slicer, size_t blknum,
Expand Down Expand Up @@ -969,12 +918,7 @@ void coap_block_finish(coap_block_slicer_t *slicer, uint16_t option)
uint8_t *pos = slicer->opt + 1;
uint16_t delta = _decode_value(*slicer->opt >> 4, &pos, slicer->opt + 3);

/* Calculate the block uint value inline here rather than through
* coap_opt_put_block(). Conserves stack and avoids importing Buffer API
* functions when using Packet API. */
uint32_t blkopt = (_slicer_blknum(slicer) << 4);
blkopt |= _size2szx(slicer->end - slicer->start);
blkopt |= ((slicer->cur > slicer->end) ? 0x8 : 0);
uint32_t blkopt = _slicer2blkopt(slicer, slicer->cur > slicer->end);
size_t olen = _encode_uint(&blkopt);

coap_put_option(slicer->opt, option - delta, option, (uint8_t *)&blkopt, olen);
Expand Down