Skip to content

nanocoap_vfs: add nanocoap_vfs_put()#17962

Merged
benpicco merged 4 commits intoRIOT-OS:masterfrom
benpicco:nanocoap_vfs-put
Jun 16, 2022
Merged

nanocoap_vfs: add nanocoap_vfs_put()#17962
benpicco merged 4 commits intoRIOT-OS:masterfrom
benpicco:nanocoap_vfs-put

Conversation

@benpicco
Copy link
Copy Markdown
Contributor

@benpicco benpicco commented Apr 18, 2022 •
edited
Loading

Contribution description

This adds a function to upload a file from VFS to a CoAP server via block-wise PUT request.

Testing procedure

You can test this with aiocoap-fileserver -w:

download a file from the CoAP server

> ncget coap://[fe80::2454:cfff:fea7:4598]/LICENSE
Saved as /nvm0/LICENSE

upload the file again under a different name

> ncput /nvm0/LICENSE coap://[fe80::2454:cfff:fea7:4598]/LICENSE-2
Saved to coap://[fe80::2454:cfff:fea7:4598]/LICENSE-2

the files should be identical

$ md5sum LICENSE*
a4ce26a8dcc018faa99a6cd08cbcd1c3  LICENSE
a4ce26a8dcc018faa99a6cd08cbcd1c3  LICENSE-2

Issues/PRs references

depends on #17937, #17960, #17958

@benpicco benpicco added the State: waiting for other PR State: The PR requires another PR to be merged first label Apr 18, 2022
@github-actions github-actions bot added Area: CoAP Area: Constrained Application Protocol implementations Area: examples Area: Example Applications Area: network Area: Networking Area: OTA Area: Over-the-air updates Area: sys Area: System Area: tests Area: tests and testing framework labels Apr 18, 2022
@benpicco benpicco requested a review from chrysn April 18, 2022 01:15
@benpicco benpicco added the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Apr 18, 2022
@benpicco benpicco mentioned this pull request Apr 21, 2022
Merged
@fjmolinas fjmolinas mentioned this pull request Apr 22, 2022
Merged
@fabian18 fabian18 mentioned this pull request May 16, 2022
Merged
@benpicco benpicco force-pushed the nanocoap_vfs-put branch from 4129bbd to 2999b4c Compare May 24, 2022 14:45
@github-actions github-actions bot removed Area: examples Area: Example Applications Area: tests Area: tests and testing framework Area: OTA Area: Over-the-air updates labels May 24, 2022
@benpicco benpicco removed State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet State: waiting for other PR State: The PR requires another PR to be merged first labels May 24, 2022
benpicco
benpicco commented May 29, 2022
View reviewed changes
@chrysn chrysn mentioned this pull request May 29, 2022
Merged
@benpicco benpicco force-pushed the nanocoap_vfs-put branch from 5a63379 to 8c38feb Compare May 29, 2022 13:40
@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 May 29, 2022
@benpicco benpicco requested a review from fabian18 June 5, 2022 10:08
fabian18
fabian18 reviewed Jun 5, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@fabian18 fabian18 left a comment

Choose a reason for hiding this comment

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

Two nitpicks inline. PUTting and re-GETting a file works well, also with BWT.

@benpicco benpicco force-pushed the nanocoap_vfs-put branch from 8c38feb to d30052a Compare June 5, 2022 14:59
fabian18
fabian18 reviewed Jun 6, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@fabian18 fabian18 left a comment

Choose a reason for hiding this comment

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

Something I have not noticed before. The first transfer goes well, but during the second or sometimes the third, the client seems to send the blocks twice and sys/net/application_layer/nanocoap/sock.c:187 fires.
nanocoap_vfs-put.pcapng.txt

@benpicco benpicco force-pushed the nanocoap_vfs-put branch from 7ebbe7d to 639dce8 Compare June 6, 2022 14:07
@benpicco
Copy link
Copy Markdown
Contributor Author

benpicco commented Jun 6, 2022

Uh good catch!
Is that with coap-server and netdev_tap?
(btw: you can run Wireshark traces directly if you run them through gzip and Wireshark understands to open .pcapng.gz files)

@fabian18
Copy link
Copy Markdown
Contributor

fabian18 commented Jun 6, 2022

No netdev_tap but an Ethernet connection between board and PC and CFLAGS += -DCONFIG_GNRC_IPV6_NIB_ARSM=1. In nanocoap_sock_request_cb(), data is not sent after the first call to sock_udp_sendv() and sock_udp_recv_buf() returns a timeout. I don´t know why, yet.

@fabian18
Copy link
Copy Markdown
Contributor

fabian18 commented Jun 6, 2022

(btw: you can run Wireshark traces directly if you run them through gzip and Wireshark understands to open .pcapng.gz files)

Thanks for the hint.

@benpicco
Copy link
Copy Markdown
Contributor Author

benpicco commented Jun 7, 2022

Hm maybe we have a bug in gnrc_sock. I see something similar with SUIT update when routing it via an intermediate RIOT node, but that might also be something else - still investigating.

@benpicco
Copy link
Copy Markdown
Contributor Author

benpicco commented Jun 7, 2022

When I tried to reproduce this on same54-xpro it would crash because it ran out of stack space - might that be the issue here?

@github-actions github-actions bot added the Area: tests Area: tests and testing framework label Jun 7, 2022
@fabian18
Copy link
Copy Markdown
Contributor

fabian18 commented Jun 7, 2022

Stack size is not the problem. I also had to increased stack sizes with:

CFLAGS += -DTHREAD_STACKSIZE_MAIN=2*THREAD_STACKSIZE_LARGE
CFLAGS += -DGCOAP_STACK_SIZE=2*THREAD_STACKSIZE_LARGE

But this is just may configuration, and is probably not necessary for everyone.

@benpicco benpicco force-pushed the nanocoap_vfs-put branch from b2c9052 to 6447828 Compare June 8, 2022 15:22
@benpicco
Copy link
Copy Markdown
Contributor Author

benpicco commented Jun 12, 2022

I'm still looking for a way to reproduce this - do you also see this issue with the put command in tests/nanocoap_cli?

@fabian18
Copy link
Copy Markdown
Contributor

fabian18 commented Jun 15, 2022 •
edited
Loading

Yes it also happens with tests/nanocoap_cli.
The first transmission is ok but takes a while.
client put fe80::3478:9157:9fd5:81c3%5 5683 /data datendatendaten
And then, there (almost) always happens a retransmission if I repeat the command, but the retransmission seems to happen immediately and not after [ACK_TIMEOUT, ACK_TIMEOUT * RANDOM_FACTOR].
(look how close the retransmission is to the original transmission regarding the timestamp)

nanocoap_cli_put.pcapng.gz

@fabian18
Copy link
Copy Markdown
Contributor

fabian18 commented Jun 16, 2022

So what are we going to do? The error does not come from this PR. This PR looks fine. It is something with gnrc_sock and/or timers. I think it is annoying to stop this PR because of that. If you agree, we could merge this anyway.

@benpicco
Copy link
Copy Markdown
Contributor Author

benpicco commented Jun 16, 2022

Thank you!
Can you open an issue about this so we don't lose track?

@benpicco benpicco merged commit 0a16f82 into RIOT-OS:master Jun 16, 2022
@benpicco benpicco deleted the nanocoap_vfs-put branch June 16, 2022 09:53
@fabian18 fabian18 mentioned this pull request Jun 16, 2022
Closed
@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

@fabian18 fabian18 fabian18 approved these changes

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

@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

@kaspar030 kaspar030 Awaiting requested review from kaspar030

@bergzand bergzand Awaiting requested review from bergzand

@haukepetersen haukepetersen Awaiting requested review from haukepetersen

@PeterKietzmann PeterKietzmann Awaiting requested review from PeterKietzmann

@cgundogan cgundogan Awaiting requested review from cgundogan

@maribu maribu Awaiting requested review from maribu

@chrysn chrysn Awaiting requested review from chrysn

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.

5 participants

@benpicco @fabian18 @maribu @chrysn @kfessel