Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

nanocoap: make coap_get_block2() actually fill struct #11802

Merged
merged 1 commit into from
Jul 8, 2019

Conversation

kaspar030
Copy link
Contributor

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 kb2ma and fjmolinas July 5, 2019 13:41
@kaspar030
Copy link
Contributor Author

@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
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
Contributor Author

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
Member

kb2ma commented Jul 8, 2019

Sounds good to me.

Copy link
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
@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
Contributor Author

Thanks for the quick review!

@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
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
Development

Successfully merging this pull request may close these issues.

2 participants