Skip to content

net/nanocoap: Buffer API Block helper functions#11024

Merged
kb2ma merged 4 commits intoRIOT-OS:masterfrom
kb2ma:coap/add_block_helpers
Jul 31, 2019
Merged

net/nanocoap: Buffer API Block helper functions#11024
kb2ma merged 4 commits intoRIOT-OS:masterfrom
kb2ma:coap/add_block_helpers

Conversation

@kb2ma
Copy link
Copy Markdown
Member

@kb2ma kb2ma commented Feb 15, 2019

Contribution description

#11002 added client side implementations for Block for the Buffer API. This PR refines the implementation with helper functions.

Function Description
coap_block2_finish() Refactored as inline, added coap_block1_finish() as inline for client side, and coap_block_finish() implementation.
coap_block_object_init() Added for client to init a block2 control request, and refactored coap_opt_put_block() to use it.
coap_block_slicer_init() Added for client to init a block1 descriptive request, and refactored coap_block2_init() to use it.

You can see the client side functions in action in block_client.c in my RIOT apps repostiory.

Testing procedure

The nano-block-client RIOT app provides a command line with commands to generate CoAP requests for nanocoap_server example resources. So you can verify both client and server side.

  • get creates a block2 request for /riot/ver
  • post creates a block1 request for /sha256

Issues/PRs references

Partially implements #10732. Depends on #11002.

@kb2ma kb2ma added Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Area: CoAP Area: Constrained Application Protocol implementations labels Feb 15, 2019
@kb2ma kb2ma requested review from bergzand and kaspar030 February 15, 2019 16:30
@kb2ma kb2ma mentioned this pull request Feb 15, 2019
Closed
6 tasks
This was referenced Feb 24, 2019
Merged
Merged
@kb2ma kb2ma added this to the Release 2019.04 milestone Feb 24, 2019
@benpicco
Copy link
Copy Markdown
Contributor

benpicco commented Jun 24, 2019

needs rebasing.

@kb2ma
Copy link
Copy Markdown
Member Author

kb2ma commented Jun 24, 2019

This PR is part of a sequence, as shown in a comment on #10732. The next PR in the sequence is #11002, which was rebased at the beginning of May. I would be happy to work through merging that one, and then move on to this one.

@benpicco
Copy link
Copy Markdown
Contributor

benpicco commented Jun 24, 2019

Ah sorry, I was just trying to figure out why lost Confirmable messages were not send again, so I started looking if there was still something missing in RIOT's CoAP implementation.

@kb2ma
Copy link
Copy Markdown
Member Author

kb2ma commented Jun 24, 2019

@benpicco, if you still have concerns about Confirmable retries, please open an issue. I'd be happy to work through it with you.

@benpicco
Copy link
Copy Markdown
Contributor

benpicco commented Jun 24, 2019

Sorry for the confusion, it turned out the problem was entirely unrelated to CoAP.

fjmolinas
fjmolinas reviewed Jul 3, 2019
View reviewed changes
@kb2ma kb2ma force-pushed the coap/add_block_helpers branch from 30dfd55 to 54d3898 Compare July 25, 2019 11:10
@kb2ma
Copy link
Copy Markdown
Member Author

kb2ma commented Jul 25, 2019

Rebased

@fjmolinas fjmolinas added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines labels Jul 25, 2019
@fjmolinas
Copy link
Copy Markdown
Contributor

fjmolinas commented Jul 25, 2019

@kb2ma with testing I have the same issue than in #11002

@fjmolinas fjmolinas added the Reviewed: 3-testing The PR was tested according to the maintainer guidelines label Jul 29, 2019
@fjmolinas
Copy link
Copy Markdown
Contributor

fjmolinas commented Jul 29, 2019

@kb2ma with testing I have the same issue than in #11002

Tested server and client side. I only had a problem when using libcoap where the shah didn't get printed. But as in #11002 that is attributed to a problem with libcoap and not this PR.

fjmolinas
fjmolinas reviewed Jul 29, 2019
View reviewed changes
sys/net/application_layer/nanocoap/nanocoap.c
unsigned blknum = _slicer_blknum(slicer);
coap_block1_t block;

coap_block_object_init(&block, _slicer_blknum(slicer),
Copy link
Copy Markdown
Contributor

@fjmolinas fjmolinas Jul 29, 2019

Choose a reason for hiding this comment

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

Here coap_block_object_init receives a bool more but in it declaration its type is int. Shouldn't it be a bool in both cases? Change the documentation accordingly if its the case.

Copy link
Copy Markdown
Member Author

@kb2ma kb2ma Jul 30, 2019

Choose a reason for hiding this comment

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

Thanks for reading closely. We specifically use coap_opt_put_block() when writing a block, so it's only useful for its more parameter to be a bool. At the same time, coap_block_object_init() directly sets the struct attribute values, and the more attribute can take on a value of -1, so it makes sense for the function to support that. I think this is a case where the declaration of coap_opt_put_block() and coap_block_object_init() make sense as they are, and it's OK to let the definition of bool as 0 or 1 bridge the gap.

However, the doc comment for coap_block_object_init() only explicitly refers to use of 0 if we don't know if there are more blocks. I will extend the comment to be more precise about use of 1 and 0. However, I'd like to leave the use of -1 as unspecified because we don't expect anyone to use it for this purpose. That may change in the future, and if it does we won't need to change the API to accommodate it.

Copy link
Copy Markdown
Contributor

@fjmolinas fjmolinas Jul 30, 2019

Choose a reason for hiding this comment

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

OK!

@fjmolinas
Copy link
Copy Markdown
Contributor

fjmolinas commented Jul 29, 2019

I only have one comment regarding documentation, see above.

fjmolinas
fjmolinas approved these changes Jul 30, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@fjmolinas fjmolinas left a comment

Choose a reason for hiding this comment

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

@kb2ma This looks good to me, I would say squash directly unless you want to wait for someone else to take a look. But ACK on my side.

@fjmolinas
Copy link
Copy Markdown
Contributor

fjmolinas commented Jul 30, 2019

Also please trigger the ci build once squashed.

@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jul 30, 2019
@kb2ma kb2ma force-pushed the coap/add_block_helpers branch from b72ff0b to 64b4e0a Compare July 30, 2019 17:12
@kb2ma kb2ma merged commit b5200e9 into RIOT-OS:master Jul 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fjmolinas fjmolinas fjmolinas approved these changes

@kaspar030 kaspar030 Awaiting requested review from kaspar030

@bergzand bergzand Awaiting requested review from bergzand

Assignees

No one assigned

Labels

Area: CoAP Area: Constrained Application Protocol implementations CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation

Projects

None yet

Milestone

Release 2019.10

Development

Successfully merging this pull request may close these issues.

5 participants

@kb2ma @benpicco @fjmolinas @miri64 @MrKevinWeiss