Skip to content

nanocoap: make coap_get_block2() actually fill struct#11802

Merged
kb2ma merged 1 commit intoRIOT-OS:masterfrom
kaspar030:fix_coap_get_block2
Jul 8, 2019
Merged

nanocoap: make coap_get_block2() actually fill struct#11802
kb2ma merged 1 commit intoRIOT-OS:masterfrom
kaspar030:fix_coap_get_block2

Conversation

@kaspar030
Copy link
Copy Markdown
Contributor

@kaspar030 kaspar030 commented Jul 5, 2019

Contribution description

Previously, coap_get_block2() did not fill the passed structure, making it unusable.

This refactors coap_get_block1() into a shared coap_get_block() function and implements both coap_get_block1() and coap_get_block2() using that.

Testing procedure

  • ckeck if the coap test applications still work as expected

Issues/PRs references

@kaspar030 kaspar030 added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR labels Jul 5, 2019
@kaspar030 kaspar030 requested review from fjmolinas and kb2ma July 5, 2019 13:41
@kaspar030
Copy link
Copy Markdown
Contributor Author

kaspar030 commented Jul 5, 2019

@kb2ma I hope this doesn't collide too much with your stack of CoAP PRs. Could we get this in first? It is a dependency of SUIT updates.

@kb2ma
Copy link
Copy Markdown
Member

kb2ma commented Jul 6, 2019

This commit in this PR is similar to 0ff2856 as described in #11057, which is the last of the sequence to completely implement Block. Functionally, I have no concerns here. Unfortunately the implementations are not identical, so merging this PR adds to the bitrot there.

@kaspar030
Copy link
Copy Markdown
Contributor Author

kaspar030 commented Jul 8, 2019

This commit in this PR is similar to 0ff2856 as described in #11057, which is the last of the sequence to completely implement Block. Functionally, I have no concerns here. Unfortunately the implementations are not identical, so merging this PR adds to the bitrot there.

I'm fine with reverting this PR's commit in #11057 as first step. That should prevent the bitrot?

@kb2ma
Copy link
Copy Markdown
Member

kb2ma commented Jul 8, 2019

Sounds good to me.

kb2ma
kb2ma approved these changes Jul 8, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@kb2ma kb2ma left a comment

Choose a reason for hiding this comment

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

This PR is a straightforward extension of the existing block1 getter (server request handler) to also get block2 (client response handler). Tested block2 by running block client test app. Confirmed no regression with block1 by running nanocoap server for /sha256 resource.

In coap_get_block(), the blknum and szx variables are not necessary. However, this PR is intended to be a minimal impact stopgap until we merge #11057 in 2019.10, which addresses this issue.

@kb2ma kb2ma 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 Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines labels Jul 8, 2019
kaspar030 added a commit to kaspar030/RIOT that referenced this pull request Jul 8, 2019
@kaspar030
DEPENDENCY MERGE: fix_coap_get_block2 (RIOT-OS#11802)
aaebdfa
@kaspar030 kaspar030 mentioned this pull request Jul 8, 2019
Merged
@kb2ma kb2ma merged commit 210265c into RIOT-OS:master Jul 8, 2019
@kaspar030 kaspar030 deleted the fix_coap_get_block2 branch July 8, 2019 13:37
@kaspar030
Copy link
Copy Markdown
Contributor Author

kaspar030 commented Jul 8, 2019

Thanks for the quick review!

@kb2ma kb2ma mentioned this pull request Jul 25, 2019
Merged
@kb2ma kb2ma added this to the Release 2019.10 milestone Sep 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kb2ma kb2ma kb2ma approved these changes

@fjmolinas fjmolinas Awaiting requested review from fjmolinas

Assignees

No one assigned

Labels

Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware 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 Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Projects

None yet

Milestone

Release 2019.10

Development

Successfully merging this pull request may close these issues.

2 participants

@kaspar030 @kb2ma