nanocoap: add serverside block1 support by kaspar030 · Pull Request #8788 · RIOT-OS/RIOT

kaspar030 · 2018-03-16T08:43:29Z

Contribution description

This PR makes use of the new option handling (introduced in #8772) and adds support for server-side blockwise (block1) support.

Issues/PRs references

~~Waiting for #8772.~~

bergzand

Initial review, I'll do a bit more thorough review as soon as #8772 is merged

bergzand · 2018-04-09T07:54:17Z

sys/include/net/nanocoap.h

+    unsigned szx;                   /**< szx value                          */
+    int more;                       /**< -1 for no option, 0 for last block,
+                                          1 for more blocks coming          */
+} coap_block1_t;


Can be reused for block2? If so, can it be changed to coap_block_t?

Probably. I propose renaming at that point.

kaspar030 · 2018-04-10T19:16:42Z

rebased after net/nanocoap: rework option handling #8772 merge, ready for review

bergzand · 2018-04-12T09:26:32Z

I'm doing some extensive testing (feeding gigabyte size data to the sha256 tester) and I'm seeing some weird things:

_sha256_handler(): received data: offset=6080 len=64 blockwise=1 more=1
_sha256_handler(): received data: offset=6144 len=64 blockwise=1 more=1
_sha256_handler(): received data: offset=6208 len=65440 blockwise=1 more=1
_sha256_handler(): received data: offset=6272 len=64 blockwise=1 more=1
_sha256_handler(): received data: offset=6336 len=64 blockwise=1 more=1
_sha256_handler(): received data: offset=6400 len=65440 blockwise=1 more=1
_sha256_handler(): received data: offset=6464 len=64 blockwise=1 more=1
_sha256_handler(): received data: offset=6528 len=64 blockwise=1 more=1

snip

_sha256_handler(): received data: offset=52160 len=64 blockwise=1 more=1
_sha256_handler(): received data: offset=52224 len=26111 blockwise=1 more=1
_sha256_handler(): received data: offset=52288 len=64 blockwise=1 more=1
_sha256_handler(): received data: offset=52352 len=26111 blockwise=1 more=1
_sha256_handler(): received data: offset=52416 len=64 blockwise=1 more=1
_sha256_handler(): received data: offset=52480 len=26111 blockwise=1 more=1
_sha256_handler(): received data: offset=52544 len=64 blockwise=1 more=1

This is produced with the nanocoap_server example on native using libcoaps coap-client to feed it data:
coap-client coap://[fe80::c0dd:c1ff:fe29:833b%tapbr0]/sha256 -m POST -f ~/$LARGE_FILE -b 64

I've wiresharked the packets, they all have size 64 there.

bergzand · 2018-04-12T07:02:19Z

examples/nanocoap_server/coap_handler.c

 /* must be sorted by path (alphabetically) */
 const coap_resource_t coap_resources[] = {
    COAP_WELL_KNOWN_CORE_DEFAULT_HANDLER,
+    { "/sha256", COAP_POST | COAP_PUT, _sha256_handler, NULL },


Comparing 5.8.2 and 5.8.3 of the CoAP rfc, I think this should only have COAP_POST as COAP_PUT is specified that the payload is stored as is and COAP_POST is used to indicate that the payload should be processed.

bergzand · 2018-04-12T07:04:49Z

sys/include/net/nanocoap.h

+int coap_get_blockopt(coap_pkt_t *pkt, uint16_t option, uint32_t *blknum, unsigned *szx);
+
+/**
+ * @brief    Block1 helper


"Block1 helper" sounds a bit too generic to me. Maybe "Block1 option getter" is more clear.

bergzand · 2018-04-12T07:05:22Z

sys/include/net/nanocoap.h

+ * @param[out]  block1  ptr to preallocated coap_block1_t structure
+ *
+ * @returns     0 if block1 option not present
+ * @returns     1 if structure has been filles


filles -> filled

bergzand · 2018-04-12T07:06:09Z

sys/include/net/nanocoap.h

+int coap_get_block1(coap_pkt_t *pkt, coap_block1_t *block1);
+
+/**
+ * @brief   Insert block1 option into buffer


"into the buffer"?

To me "the buffer" implies a specific buffer, but the function can insert the option in any buffer.

bergzand · 2018-04-12T07:08:35Z

sys/include/net/nanocoap.h

+size_t coap_put_option_block1(uint8_t *buf, uint16_t lastonum, unsigned blknum, unsigned szx, int more);
+
+/**
+ * @brief   Insert block1 option into buffer (from coap_block1_t)


"into the buffer"?

bergzand · 2018-04-12T09:05:55Z

sys/include/net/nanocoap.h

+ *
+ * @param[in]   szx     SZX value to convert
+ *
+ * @returns     size


Maybe expand a bit to "size in bytes" to clarify a bit.

bergzand · 2018-04-12T09:36:01Z

sys/include/net/nanocoap.h

+ * @param[in]   pkt     pkt to work on
+ * @param[in]   option  actual block option number to get
+ * @param[out]  blknum  block number
+ * @param[out]  szx     SZX value


Maybe clarify here that these parameters are modified to zero if the option is not found.

kaspar030 · 2018-04-16T09:33:04Z

I'm doing some extensive testing (feeding gigabyte size data to the sha256 tester) and I'm seeing some weird things:

Does this happen a lot? I've been feeding q couple hundred jiggabytes through the tester and so far, I cannot reproduce.

bergzand · 2018-04-16T10:11:52Z

Does this happen a lot? I've been feeding q couple hundred jiggabytes through the tester and so far, I cannot reproduce.

After testing a bit more this appears to be a variant of #6123, except that I'm hammering the instance here with CoAP packets instead of ping floods. This results in sock returning a far too large packet size (several times more than the mtu size). So this issue is unrelated to this PR.

bergzand · 2018-04-16T10:25:26Z

Ack, please squash

bergzand · 2018-04-16T10:35:36Z

@kaspar030 It looks like the commits are in the wrong order, could you rebase it to first the block1 support and then the commit adding example. Otherwise people are going to complain when bisecting.

kaspar030 · 2018-04-16T10:46:56Z

@bergzand github often messes up the order. the actual commit order in the branch is as you suggested.

bergzand · 2018-04-16T10:48:25Z

Ah, you're right, never mind then

…e_block1 nanocoap: add serverside block1 support

kaspar030 requested review from emmanuelsearch and kb2ma March 16, 2018 08:43

kaspar030 mentioned this pull request Mar 16, 2018

net/nanocoap: rework option handling #8772

Merged

kaspar030 force-pushed the nanocoap_add_serverside_block1 branch from ad95389 to 9558aab Compare April 4, 2018 14:56

bergzand requested changes Apr 9, 2018

View reviewed changes

kaspar030 mentioned this pull request Apr 9, 2018

sys: provide over-the-air update mechanism #8902

Closed

kaspar030 force-pushed the nanocoap_add_serverside_block1 branch from bea5296 to eaea004 Compare April 10, 2018 19:14

kaspar030 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 Apr 10, 2018

bergzand mentioned this pull request Apr 11, 2018

nanocoap: add server-side block2 support #8932

Merged

bergzand requested changes Apr 12, 2018

View reviewed changes

kaspar030 added 2 commits April 16, 2018 12:31

nanocoap: add server-side block1 support

4897222

examples/nanocoap: add blockwise handler example

b42159d

kaspar030 force-pushed the nanocoap_add_serverside_block1 branch from 8aef938 to b42159d Compare April 16, 2018 10:32

bergzand approved these changes Apr 16, 2018

View reviewed changes

bergzand merged commit 635e72d into RIOT-OS:master Apr 16, 2018

maxvankessel pushed a commit to maxvankessel/RIOT that referenced this pull request Apr 20, 2018

Merge pull request RIOT-OS#8788 from kaspar030/nanocoap_add_serversid…

273898b

…e_block1 nanocoap: add serverside block1 support

kb2ma mentioned this pull request Oct 1, 2018

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

Closed

kaspar030 deleted the nanocoap_add_serverside_block1 branch January 22, 2019 10:22

danpetry modified the milestone: Release 2019.04 Mar 12, 2019

Conversation

kaspar030 commented Mar 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Issues/PRs references

Uh oh!

bergzand left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaspar030 commented Apr 10, 2018

Uh oh!

bergzand commented Apr 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaspar030 commented Apr 16, 2018

Uh oh!

bergzand commented Apr 16, 2018

Uh oh!

bergzand commented Apr 16, 2018

Uh oh!

bergzand commented Apr 16, 2018

Uh oh!

kaspar030 commented Apr 16, 2018

Uh oh!

bergzand commented Apr 16, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kaspar030 commented Mar 16, 2018 •

edited

Loading