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_vfs: add nanocoap_vfs_put() #17962

Merged
merged 4 commits into from
Jun 16, 2022
Merged

Conversation

benpicco
Copy link
Contributor

@benpicco benpicco commented Apr 18, 2022

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 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 the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label May 24, 2022
@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
Copy link
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.

sys/shell/commands/sc_nanocoap_vfs.c Outdated Show resolved Hide resolved
sys/shell/commands/sc_nanocoap_vfs.c Outdated Show resolved Hide resolved
@benpicco benpicco force-pushed the nanocoap_vfs-put branch from 8c38feb to d30052a Compare June 5, 2022 14:59
Copy link
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

sys/shell/commands/sc_nanocoap_vfs.c Outdated Show resolved Hide resolved
@benpicco benpicco force-pushed the nanocoap_vfs-put branch from 7ebbe7d to 639dce8 Compare June 6, 2022 14:07
@benpicco
Copy link
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
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
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
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
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
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
Contributor Author

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
Contributor

fabian18 commented Jun 15, 2022

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
Contributor

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

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

Successfully merging this pull request may close these issues.

4 participants