Skip to content

nanocoap_sock: add nanocoap_sock_block_request()#17958

Merged
benpicco merged 4 commits intoRIOT-OS:masterfrom
benpicco:nanocoap-blockwise_put
May 18, 2022
Merged

nanocoap_sock: add nanocoap_sock_block_request()#17958
benpicco merged 4 commits intoRIOT-OS:masterfrom
benpicco:nanocoap-blockwise_put

Conversation

@benpicco
Copy link
Copy Markdown
Contributor

@benpicco benpicco commented Apr 16, 2022 •
edited
Loading

Contribution description

This implements a nanocoap_sock_block_request() function for block-wise (block1) requests.
The function is expected to be called in a loop and will transmit a block each time it's called.

As an alternative to #17544 I simple added a iolist_t *snips to coap_pkt_t so we don't have to copy the payload into a temporary buffer and can leverage sock_udp_sendv() instead.

Testing procedure

The tests/nanocoap_cli has been extended with a (crude) put command.
It will simply upload the lyrics of the Free Software Song via blockwise PUT, it can be used with

put coap://[fe80::90a7:a6ff:fe4b:2e32%5]/song.txt

with

aiocoap-fileserver --write .

on the other end.
This will be changed to instead integrate with nanocoap_vfs to upload files and integrate with the url put/post command, but I wanted to get some feedback on the API first.

Issues/PRs references

@benpicco benpicco added the State: waiting for other PR State: The PR requires another PR to be merged first label Apr 16, 2022
@github-actions github-actions bot added Area: CoAP Area: Constrained Application Protocol implementations Area: network Area: Networking Area: sys Area: System Area: tests Area: tests and testing framework labels Apr 16, 2022
@benpicco benpicco requested a review from chrysn April 16, 2022 12:50
@benpicco benpicco force-pushed the nanocoap-blockwise_put branch 2 times, most recently from 3d23871 to 2688862 Compare April 17, 2022 23:01
@benpicco benpicco removed the State: waiting for other PR State: The PR requires another PR to be merged first label Apr 17, 2022
@benpicco benpicco force-pushed the nanocoap-blockwise_put branch from c255b51 to a7f71d8 Compare April 18, 2022 01:14
@benpicco benpicco mentioned this pull request Apr 18, 2022
Merged
@miri64 miri64 mentioned this pull request Apr 19, 2022
Open
@benpicco benpicco force-pushed the nanocoap-blockwise_put branch 2 times, most recently from 6a6d87a to 54a4272 Compare April 22, 2022 21:20
@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 Apr 30, 2022
@benpicco benpicco force-pushed the nanocoap-blockwise_put branch 3 times, most recently from 153819f to d2e5d9f Compare May 1, 2022 23:03
@benpicco benpicco requested review from kaspar030, kfessel and maribu May 10, 2022 10:56
maribu
maribu reviewed May 17, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

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

Code looks good to me.

I think that the semantics of coap_get_total_len are unclear when snips in a pkt are used. With the current documentation, I would assume that it would also take any payload snips into account, but the implementation doesn't. (In fact, the code here relies on the fact that it doesn't take the snips size into account.)

It is not directly related, but I am also a bit unhappy with the documentation of coap_pkt_t::payload. As I understand it, it really points to the end of the header (included the 0xff end of options marker, if present). So even when there is no payload at all, it must still not be NULL.

I also would like the documentation to have an opinion on whether (pkt->payload_len > 0) && (pkt->snips != NULL) is valid or not. And if it is valid, how the payload when assembled back into a single continuous buffer would look like. (E.g. would pkt->payload be put in front of pkt->snips, or the other way round.)

@benpicco benpicco removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label May 17, 2022
@benpicco
Copy link
Copy Markdown
Contributor Author

benpicco commented May 17, 2022

I added some documentation to coap_pkt_t. I also don't like it's API, but changing it turned out to be a larger undertaking than I had anticipated, so I turned to this less invasive approach.

coap_get_total_len() is only used by nanocoap_sock, I wonder if we should just make that an internal function.

maribu
maribu approved these changes May 17, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

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

ACK. Code looks good and I trust your testing. Please squash the documentation nitpicks right in.

@benpicco benpicco force-pushed the nanocoap-blockwise_put branch from f98913a to c20a1d0 Compare May 17, 2022 21:07
@benpicco benpicco added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels May 17, 2022
@benpicco benpicco merged commit 7c62c89 into RIOT-OS:master May 18, 2022
@benpicco benpicco deleted the nanocoap-blockwise_put branch May 18, 2022 08:40
This was referenced May 18, 2022
Closed
Closed
chrysn pushed a commit to RIOT-OS/rust-riot-sys that referenced this pull request May 31, 2022
@benpicco @chrysn
gcoap: cast payload to void*
aab605f 
This accounts for a C-nonbreaking-Rust-breaking change in RIOT APIs.

Merges: https://gitlab.com/etonomy/riot-wrappers/-/merge_requests/10
See-Also: RIOT-OS/RIOT#17958
@chrysn chrysn added this to the Release 2022.07 milestone Aug 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@maribu maribu maribu approved these changes

@smlng smlng Awaiting requested review from smlng

@leandrolanzieri leandrolanzieri Awaiting requested review from leandrolanzieri leandrolanzieri is a code owner

@aabadie aabadie Awaiting requested review from aabadie aabadie is a code owner

@MichelRottleuthner MichelRottleuthner Awaiting requested review from MichelRottleuthner MichelRottleuthner is a code owner

@fjmolinas fjmolinas Awaiting requested review from fjmolinas

@miri64 miri64 Awaiting requested review from miri64 miri64 is a code owner

@haukepetersen haukepetersen Awaiting requested review from haukepetersen

@PeterKietzmann PeterKietzmann Awaiting requested review from PeterKietzmann

@cgundogan cgundogan Awaiting requested review from cgundogan

@chrysn chrysn Awaiting requested review from chrysn

@kaspar030 kaspar030 Awaiting requested review from kaspar030

@kfessel kfessel Awaiting requested review from kfessel

Assignees

No one assigned

Labels

Area: CoAP Area: Constrained Application Protocol implementations Area: network Area: Networking Area: sys Area: System Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR

Projects

None yet

Milestone

Release 2022.07

Development

Successfully merging this pull request may close these issues.

3 participants

@benpicco @maribu @chrysn