nanocoap: add server-side block2 support

[bergzand]

Contribution description

Making use of the new option handling and the work done by @kaspar030 with block1 support, this PR extends that to block2 support. The /.well-known/core URI is also adapted to support block2.

commit 54b275c (block2 support, not the example) adds 412 bytes to the text size of the nanocoap example on a samr21-xpro

Issues/PRs references

Depends on #8788

[kaspar030]

mishap

[bergzand]

Yeah, I also noticed that I still have to squash in a bit of doxygen

[miri64]

Another one ;-).

[bergzand]

First ☕, otherwise half this PR is going to consist of me removing merge conflict indicators 😞

[bergzand]

I hope they're all gone now

[bergzand]

Rebased because block1 support got merged.

[kb2ma]

Making progress on review. See WIP as C-style comments in code. Overall approach and structure seem fine. Right now I'm thinking about how the block payload is written.

I plan to finish the merge for #8920, and then finish this review. That PR provides examples for the naming of option functions, which I think we should follow here.

[kb2ma]

As I mentioned before, I think the overall approach here makes sense. I like the switch for writing the payload contents depending on location within the overall transfer. Happy to provide more details and talk through my suggestions and comments.

[bergzand]

Thanks for the review @kb2ma, I'll try to fix and rebase this PR asap.

[bergzand]

@kb2ma I think I've fixed all of your issues besides your suggestion for a generic put_bytes. I have to take a look at that one later and see how the additional checks would change things.

[kb2ma]

We're getting there. :-) I think all of the previous discussion is done except for the name/purpose of NANOCOAP_BLOCK_SZX_MAX. See the discussion.

Toward the goal of getting there, I'm OK with putting off a decision on use of 'coap_blockwise_put_xxx()' vs. 'coap_payload_put_[char|bytes]()'. coap_payload_put_xxx() is really additional to this PR rather than essential to it. The implementation of the function could probably call coap_blockwise_put_xxx() depending on whether the coap_blockbuilder_t parameter in coap_payload_put_xxx() is NULL. I can see how eventually it might be handy to have such a function, but let's see how use of block develops.

By the way, in the specific issues below, I mention use of 'coap_opt_put_xxx()' vs. 'coap_opt_add_xxx()'. I'm not asking for implementations of 'coap_opt_add_xxx()' for this PR! That's also follow-on work. ;-)

[kb2ma]

Actually, I thought this function name was OK as coap_get_block2(). I understand that this choice may seem confusing. You'll see that 'coap_get_block2()' is consistent with #8920. Let me explain.

I did originally think it best to include the 'opt' in all the 'get' functions. But once I started to do that, I had a hard time deciding where to stop. For example, should we use 'coap_opt_block2_init()'? Does 'coap_get_observe()' need to change to 'coap_opt_get_observe()'?

So, then I took minimalist approach -- only change what it necessary for clarity. The main necessity I see is to distinguish between building options directly with the byte array or indirectly with a coap_pkt_t. So then, use of byte array means to name the function 'coap_opt_put_xxx()', and use of the coap_pkt_t means to name the function 'coap_opt_add_xxx()'. I think others may not see this distinction as the basis for the names because we have only a few of the 'coap_opt_add_xxx()' functions. It will become clear as we add them though.

There is one additional use for a 'coap_opt' name, and that is for data-type based retrieval, like 'coap_opt_get_string()'. I think just 'coap_get_string()' would be too confusing (as in "really, any string?").

I think this confusion has helped me decide the next PR I'm going to write -- and that is to add header documentation to nanocoap.h to explain use of a byte buffer or a coap_pkt_t to build a packet. I can revive what I started in #9474.

[bergzand]

So if I understand it correctly, you rather have me change this back to coap_get_block2?

[kb2ma]

Yes, please. Apologies for my contribution to any misunderstanding.

[bergzand]

No worries :)

[kb2ma]

Following up on my comments above. I created #10036 with nanocoap module documentation. Might be worth a look.

[kb2ma]

Still using old name. I fixed in 7a67661.

[kb2ma]

Now that I look more closely, I see this function needs to be refactored to work when building a message with a coap_pkt_t (coap_opt_add_xxx(), usually for gcoap) rather than a byte array (coap_opt_put_xxx()).

So, how about this declaration:
void coap_block2_init(coap_pkt_t *pkt, coap_block1_t *blk_opt, coap_blockbuilder_t *blk_builder);

The implementation of the function changes to set blknum and szx in blk_opt, and removes the call to coap_opt_put_block2(), which is incompatible with bulding a message with a coap_pkt_t.

Then, for example, coap_well_known_core_default_handler() begins like below:

    (void)context;
    coap_block1_t blk_opt;
    coap_blockbuilder_t blk_builder;
    uint8_t *payload = buf + coap_get_total_hdr_len(pkt);
    uint8_t *bufpos = payload;

    bufpos += coap_put_option_ct(bufpos, 0, COAP_CT_LINK_FORMAT);
    coap_block2_init(pkt, blk_opt, blk_builder);
    bufpos += coap_opt_put_block2(bufpos, COAP_OPT_CONTENT_FORMAT, blk_opt.blknum, blk_opt.szx);

Finally, there is not a similar compatibility problem (uint8_t* vs. coap_pkt_t) with coap_block_finish() because this function is called after the option has been added to message buffer, so we're just editing what's already there.

[bergzand]

Let me try something here, I think I understand what you want and it makes sense to me.

[bergzand]

@kb2ma I've reworked it to use only a coap_block_slicer_t here. It required 2 helper function which added 20 bytes of additional flash compared to previous fixup state. This prevents us from having to put that coap_block1_t struct on the stack during the full handler processing.

Please let me know if this is workable for gcoap since I didn't completely follow your instructions :)

[bergzand]

@kb2ma What makes more sense to you, passing the individual block values (size, block number and "more") to the coap_opt_put_block2 or passing the coap_blockbuilder_t struct?

[kb2ma]

I think passing individual values makes sense at the lowest levels -- coap_get_blockopt() and coap put_option_block() (eventually to become coap_opt_put_block()). However, above that it seems more convenient to use a single coap_block1_t rather than its attributes. (I assume you meant coap_block1_t rather then coap_blockbuilder_t.)

So, yes, I think it makes more sense to pass a coap_block1_t to coap_opt_put_block2().

[kb2ma]

At the risk of you throwing things at me, would coap_block_serializer_t or coap_block_payload_t be better than coap_blockbuilder_t? I'm not asking you to change this, just thinking about the best name as my understanding evolves. [ducking and running]

[bergzand]

At the risk of you throwing things at me, would coap_block_serializer_t or coap_block_payload_t be better than coap_blockbuilder_t? I'm not asking you to change this, just thinking about the best name as my understanding evolves. [ducking and running]

As long as this PR is not yet finalized I don't mind you suggesting new names. How about coap_block_slicer_t. I think serializer here is technically incorrect since the payload is not really transformed. I suggested slicer here since the payload is sliced into individual segments

[kb2ma]

How about coap_block_slicer_t.

+1! Nicely descriptive

[bergzand]

+1! Nicely descriptive

Done!

Now back to the other issues remaining :)

[bergzand]

@kb2ma I think I've fixed all your comments so far, please let me know if I skipped one.

[kb2ma]

Thanks for the update, @bergzand. I am reviewing as I write this. My goal is a concrete response as soon as possible.

[kb2ma]

Documentation out of sync with declaration. I fixed in b6b452d.

[kb2ma]

Besides the issues above, I also added module documentation in d1e999d. This documentation seems odd on its own, but I'll combine with #10036 when that's merged in the next few days. Also, I fixed some typos in c126ca8.

In the interest of time, I went ahead and pushed these fixes to @bergzand's repo. Besides, especially with coap_block2_finish() issue, writing the code helps convince yourself that it's the right thing to do. :-)

With these changes, no failures when testing. I had not really used block before this, but the design and @bergzand's implementation are well done! I used the nanocoap server example to test /.well-known/core and /riot/description. I tried adjusting the lengths of the output to catch off-by-1 problems. I tried various block lengths (16, 32, etc.).

I only tested on native, but it compiles and nanocoap server starts OK on samr21-xpro.

Let's wrap this up and get it merged! If @bergzand is OK with these fixups, from my perspective all we need is to rebase to get the new coap header file.

[kb2ma]

Found the issues above as I reviewed the maintainer guidelines. See #4 in @bergzand's repository. Also, added a commit based on uncrustify in 323e792.

Able to test on samr21-xpro. It works fine. Also checked executable size and found the block2 work excluding the example added 380 bytes to the text section of the executable. Max memory use for /.well-known/core did not increase according to ps(). Max memory use did increase by 68 bytes when running the /riot/description example.

My only suggestion for future work on memory/code space is to consider adding the block value to coap_block_slicer_t. This would remove the need for _size2szx() and _slicer_blknum() at the price of 4 bytes of RAM.

I'm going to check off all of the Reviewed labels now. Once we work through these changes, I'm happy to mark approved.

[kb2ma]

As @bergzand and I discussed, the block code needs some unit tests. For example, this will verify the fix for the issue with coap_get_block2(), which is not used in the nanocoap_server example. However, we agreed to add these tests in a follow-up.

[kb2ma]

Given Murdock issue, need to blacklist WSN430? Alternatively could remove the /riot/description example to free text space. Can reference coap_well_known_core_default_handler() to illustrate how to use block2.

[bergzand]

My only suggestion for future work on memory/code space is to consider adding the block value to coap_block_slicer_t. This would remove the need for _size2szx() and _slicer_blknum() at the price of 4 bytes of RAM.

I'm not sure what the correct tradeoff is here. If I compile the example with these functions not flagged with static, they occupy 20 and 16 bytes of flash, so we're talking about trading 36B of flash for 4B of stack usage.

Given Murdock issue, need to blacklist WSN430? Alternatively could remove the /riot/description example to free text space. Can reference coap_well_known_core_default_handler() to illustrate how to use block2.

I prefer to have an as simple as possible example for this, IMHO the coap_well_known_core_default_handler() is not exactly trivial. In this case I'd prefer to blacklist the two boards, there are still other 16 bit boards that should be able to compile/run the example

[kb2ma]

I'm not sure what the correct tradeoff is here.

I agree that given the relative byte sizes of the tradeoff (RAM vs flash) it's a tough call. At least we've flagged it for future reference.

In this case I'd prefer to blacklist the two boards

It really gets into larger questions about the content of examples, and it's definitely not worth holding up this PR. For example, it would be easy enough in the future to create a 'block' app in the applications repository for /riot/ver and other uses of block. OTOH, it was definitely worthwhile to look at the flash and RAM sizes before merging.

[kb2ma]

So, I'd say were there, and just in time given the soft freeze on 10/16. :-) Please squash.

[kb2ma]

OK to blacklist the WSN boards in the squash.

[bergzand]

So, I'd say were there, and just in time given the soft freeze on 10/16. :-) Please squash.

Squashed

OK to blacklist the WSN boards in the squash.

And added to the insufficient memory list.

[kb2ma]

Thanks for the tag, @bergzand. I was just focused on waiting for Murdock to finish. :-)

[bergzand]

🎉 thanks for the reviews @kb2ma!

[kYc0o]

Yaaay!!! Thanks o everyone involved here! This feature is really really appreciated!

[jia200x]

Awesome work! Thanks to everyone involved! :)