net/coap: Block optimizations by kb2ma · Pull Request #11057 · RIOT-OS/RIOT

kb2ma · 2019-02-24T16:56:12Z

Contribution description

While working on the Block implementation (#10732) we found several opportunities for optimization. However, we decided to wait until the implementation was complete before implementing them. This PR includes the improvements listed below.

This PR also completes the Block implementation!

Commit	Notes
`c02872f`	Adds coap_opt_put_uint() to write a generic uint-based option for the Buffer API. The Packet API already includes coap_opt_add_uint(). This function allows the implementation of simple options, like Content-Format, to inline this function. It also allows a user to easily add an option to a message for which there is not a specific implementation presently, like Max-Age or Size1.
`1420c8e`	Reimplements coap_opt_put_blockN(), coap_put_option_ct(), and coap_put_option_block1() to inline coap_opt_put_uint(). Removes implementations of those functions.
`d90f022`	Reworks the static _slicer_blknum() into the more useful _slicer2blkopt(). The former function only calculated the block number from a slicer struct. The latter adds a more parameter to completely calculate the uint for the block option from a slicer struct. Also replaces three uses of _slicer_blknum() with _slicer2blkopt(), which also removes the duplicate calculation of the block option uint value.
`d8ccda5`	The previous commit removed the only use of coap_opt_put_block_object(), which was added during recent development for completion of block. This commit removes coap_opt_put_block_object().
`0ff2856`	The coap_get_blockN() functions were implemented separately. Only the defintiion of coap_get_block1() calculated the offset attribute for the coap_block1_t struct. This commit reworks the implementation to a single definition that calculates the offset, with inlines for the specific "blockN" functions.
`789646e`	Fixes coap_block_object_init() to also calculate the offset attribute for coap_block1_t struct.

Testing procedure

Verify no regression by rerunning the test procedure in #11024 and #11056.

Issues/PRs references

Completes #10732.

kb2ma · 2019-07-25T13:57:06Z

Rebased, including replacement of functionality in #11802 with the implementation here.

sys/net/application_layer/nanocoap/nanocoap.c

benpicco · 2019-09-24T19:45:01Z

This one needs a rebase.

fjmolinas

Some comments, otherwise I ran the functional tests and there seems to be no regression

Details

sudo -E pytest block1_server_test.py 
========================================================= test session starts ==========================================================
platform linux -- Python 3.6.8, pytest-5.1.3, py-1.8.0, pluggy-0.12.0
rootdir: /home/francisco/workspace/riot-coap-pytest, inifile: pytest.ini
collected 6 items                                                                                                                      

block1_server_test.py ......                                                                                                     [100%]

========================================================== 6 passed in 25.57s ==========================================================

sudo -E pytest block2_server_test.py
========================================================= test session starts ==========================================================
platform linux -- Python 3.6.8, pytest-5.1.3, py-1.8.0, pluggy-0.12.0
rootdir: /home/francisco/workspace/riot-coap-pytest, inifile: pytest.ini
collected 4 items

block2_server_test.py .... [100%]

========================================================== 4 passed in 16.94s ==========================================================

sudo -E pytest block1_client_test.py 
========================================================= test session starts ==========================================================
platform linux -- Python 3.6.8, pytest-5.1.3, py-1.8.0, pluggy-0.12.0
rootdir: /home/francisco/workspace/riot-coap-pytest, inifile: pytest.ini
collected 1 item                                                                                                                       

block1_client_test.py .                                                                                                          [100%]

========================================================== 1 passed in 0.84s ===========================================================

sudo -E pytest block2_client_test.py 
========================================================= test session starts ==========================================================
platform linux -- Python 3.6.8, pytest-5.1.3, py-1.8.0, pluggy-0.12.0
rootdir: /home/francisco/workspace/riot-coap-pytest, inifile: pytest.ini
collected 3 items                                                                                                                      

block2_client_test.py ...                                                                                                        [100%]

========================================================== 3 passed in 2.62s ===========================================================

sys/net/application_layer/nanocoap/nanocoap.c

fjmolinas · 2019-10-01T07:25:38Z

sys/include/net/nanocoap.h

 * @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,


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

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.

fjmolinas · 2019-10-01T12:58:00Z

@kb2ma once the final two comments are addressed feel free to squash :) and trigger the ci build

kb2ma

Blocking until I get a fix in for _size2szx().

kb2ma · 2019-10-02T11:25:15Z

sys/net/application_layer/nanocoap/nanocoap.c

-    /* Size exponent + 1 */
-    assert(szx >= 5);
-    return szx - 5;
+    return bitarithm_lsb(size) - 4;


I think I was too aggressive by returning here. I'd like to restore the assert on the szx value. I'll add in a fix.

Apologies for the churn.

Just pushed the fixup. Also added a comment to explain the sequencing of the offset subtraction.

fjmolinas · 2019-10-03T16:21:36Z

@kb2ma Tests are still passing, please squash.

fjmolinas

There are no new changes since last I tested. Some of my comments are still pending, but will be addressed in follow-up PR's, namely consistency across block function names.

There is no regression regarding #11024 and #11056. ACK!

fjmolinas · 2019-10-04T07:42:58Z

@kb2ma I would add some doc cleanup to the list #10732 to complete it :)

kb2ma · 2019-10-04T12:43:24Z

Thanks for all the testing and feedback with these PRs, @fjmolinas!

benpicco · 2024-01-23T15:23:00Z

sys/include/net/nanocoap.h

-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);


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

kb2ma added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: CoAP Area: Constrained Application Protocol implementations labels Feb 24, 2019

kb2ma added this to the Release 2019.04 milestone Feb 24, 2019

kb2ma requested review from bergzand and haukepetersen February 24, 2019 16:56

kb2ma mentioned this pull request Feb 24, 2019

Completing CoAP Block implementation #10732

Closed

6 tasks

kb2ma mentioned this pull request Mar 11, 2019

sys/net/application_layer/nanocoap: Add path prefix option #11098

Merged

miri64 modified the milestones: Release 2019.04, Release 2019.07 May 7, 2019

kb2ma mentioned this pull request May 9, 2019

Poor results with coap/block_optimization branch kb2ma/RIOT#29

Closed

kb2ma mentioned this pull request Jul 6, 2019

nanocoap: make coap_get_block2() actually fill struct #11802

Merged

MrKevinWeiss modified the milestones: Release 2019.07, Release 2019.10 Jul 16, 2019

kb2ma force-pushed the coap/block_optimization branch from 789646e to b2c9080 Compare July 25, 2019 13:41

fjmolinas reviewed Aug 8, 2019

View reviewed changes

sys/net/application_layer/nanocoap/nanocoap.c Show resolved Hide resolved

kb2ma mentioned this pull request Sep 13, 2019

nanocoap docs: buffer/packet API distinction not referenced when used #12201

Closed

kb2ma force-pushed the coap/block_optimization branch from b2c9080 to c1aa872 Compare September 28, 2019 15:06

fjmolinas reviewed Oct 1, 2019

View reviewed changes

kb2ma added 6 commits October 2, 2019 05:51

net/nanocoap: add generic uint block option

233dd31

net/nanocoap: use coap_opt_add_uint() and remove unused

dae9c4a

net/coap: improve slicer to block helper

efb75c5

net/nanocoap: remove unused function

5eaa51e

net/coap: add generic get block function

2abcbc0

net/nanocoap: initialize offset attribute

e22d472

kb2ma force-pushed the coap/block_optimization branch from 12518ae to 5018e2c Compare October 2, 2019 09:58

kb2ma added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 2, 2019

kb2ma commented Oct 2, 2019

View reviewed changes

net/nanocoap: optimize determination of exponent for block szx

2dc4209

kb2ma force-pushed the coap/block_optimization branch from c49c8f4 to 2dc4209 Compare October 3, 2019 17:19

fjmolinas approved these changes Oct 4, 2019

View reviewed changes

fjmolinas merged commit 7d7596e into RIOT-OS:master Oct 4, 2019

fjmolinas mentioned this pull request Oct 4, 2019

sys/suit: initial support for SUIT firmware updates #11818

Merged

benpicco reviewed Jan 23, 2024

View reviewed changes

Conversation

kb2ma commented Feb 24, 2019

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

kb2ma commented Jul 25, 2019

Uh oh!

Uh oh!

benpicco commented Sep 24, 2019

Uh oh!

fjmolinas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fjmolinas Oct 1, 2019

Choose a reason for hiding this comment

Uh oh!

kb2ma Oct 1, 2019

Choose a reason for hiding this comment

Uh oh!

fjmolinas Oct 1, 2019

Choose a reason for hiding this comment

Uh oh!

fjmolinas commented Oct 1, 2019

Uh oh!

kb2ma left a comment

Choose a reason for hiding this comment

Uh oh!

kb2ma Oct 2, 2019

Choose a reason for hiding this comment

Uh oh!

fjmolinas Oct 2, 2019

Choose a reason for hiding this comment

Uh oh!

kb2ma Oct 2, 2019

Choose a reason for hiding this comment

Uh oh!

fjmolinas commented Oct 3, 2019

Uh oh!

fjmolinas left a comment

Choose a reason for hiding this comment

Uh oh!

fjmolinas commented Oct 4, 2019

Uh oh!

kb2ma commented Oct 4, 2019

Uh oh!

benpicco Jan 23, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants