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.

Sign up for GitHub

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

Jump to bottom

examples: add TCP echo server & client from documentation #16739

Merged
merged 1 commit into from
Nov 12, 2024
Merged

examples: add TCP echo server & client from documentation #16739

merged 1 commit into from
Nov 12, 2024

Conversation

benpicco
Copy link
Contributor

@benpicco benpicco commented Aug 15, 2021 •
edited
Loading

Contribution description

The TCP sock API comes with an TCP echo example server & client.
But since this is never compiled and run it contained hidden bugs and is generally inconvenient as no Makefile is included.

Move this code to a proper example to it can be easily build & run for LWIP and GNRC TCP implementations.

Testing procedure

You can run all the tests on native using nc as the other end:

server mode

> ifconfig
Iface  5  HWaddr: 7A:37:FC:7D:1A:AF 
          L2-PDU:1500  MTU:1500  HL:64  Source address length: 6
          Link type: wired
          inet6 addr: fe80::7837:fcff:fe7d:1aaf  scope: link  VAL
          inet6 group: ff02::1
          inet6 group: ff02::1:ff7d:1aaf

> listen 1234
Listening on port 1234

Send some data from Linux

echo -n "Hello from Linux" | nc fe80::7837:fcff:fe7d:1aaf%tapbr0 1234

Reading data
Read: "Hello from Linux"
> Disconnected

client mode

Start a server on Linux

% ip a s tapbr0
3: tapbr0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP group default qlen 1000
    link/ether 92:a7:a6:4b:2e:32 brd ff:ff:ff:ff:ff:ff
    inet6 fe80::90a7:a6ff:fe4b:2e32/64 scope link 
       valid_lft forever preferred_lft forever
% echo -n "Hello Back" | nc -6 -l 1234

Send some data in RIOT

> send fe80::90a7:a6ff:fe4b:2e32 1234 "Hello from RIOT"
Read: "Hello Back"

Receive it on Linux

Hello from RIOT

Issues/PRs references

depend on and includes #16741
#16494

@github-actions github-actions bot added the Area: examples Area: Example Applications label Aug 15, 2021
This was referenced Aug 15, 2021
Merged
Merged
@github-actions github-actions bot added Area: network Area: Networking Area: pkg Area: External package ports labels Aug 15, 2021
@benpicco benpicco mentioned this pull request Aug 15, 2021
Merged
@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 Aug 15, 2021
@benpicco benpicco mentioned this pull request Aug 15, 2021
Merged
@benpicco benpicco requested a review from fjmolinas August 22, 2021 16:49
@miri64
Copy link
Member

miri64 commented Aug 23, 2021

Why single out the TCP example? Why not the UDP, IP, or DTLS examples?

@benpicco
Copy link
Contributor Author

Well we should have those as runnable code too, but we got to start somewhere 😉

@github-actions github-actions bot removed Area: network Area: Networking Area: pkg Area: External package ports labels Aug 26, 2021
miri64
miri64 reviewed Aug 26, 2021
View reviewed changes
Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

IMHO this could also be one application. The client could go in one C-file and the server in another. Sure, the resulting binary then has both functionalities, but I don't think that is harmful and maybe even a bit easier for newcomers to grasp.

examples/tcp_echo_server/main.c Outdated Show resolved Hide resolved
@miri64
Copy link
Member

miri64 commented Aug 26, 2021

IMHO this could also be one application.

(and we should call it sock_tcp_echo to distiguish it from posix_sockets)

@benpicco benpicco force-pushed the examples/tcp_echo branch 3 times, most recently from ddfc5d9 to 7350366 Compare September 6, 2021 21:19
miri64
miri64 reviewed Sep 7, 2021
View reviewed changes
Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

This is missing a README ;-)

miri64
miri64 previously requested changes Sep 7, 2021
View reviewed changes
examples/sock_tcp_echo/Makefile Outdated Show resolved Hide resolved
@benpicco benpicco requested a review from jia200x as a code owner September 8, 2021 15:04
@github-actions github-actions bot added the Area: doc Area: Documentation label Sep 8, 2021
@benpicco
Copy link
Contributor Author

benpicco commented Sep 8, 2021

@brummer-simon I just noticed something now that client and server are combined into the same application:
After starting the server, I can no longer open any client connections:

2021-09-08 17:02:30,857 # listen 1234
2021-09-08 17:02:30,859 # Listening on port 1234

> send 2001:16b8:45bb:3d00:b8f3:77d9:fa25:2b01 1234 Test
2021-09-08 17:02:46,663 # send 2001:16b8:45bb:3d00:b8f3:77d9:fa25:2b01 1234 Test
2021-09-08 17:02:46,667 # Error connecting sock: Not enough space

With LWIP=1 this is working fine:

> listen 1234
2021-09-08 17:06:24,483 # listen 1234
2021-09-08 17:06:24,485 # Listening on port 1234

> send 2001:16b8:45bb:3d00:b8f3:77d9:fa25:2b01 1234 Test
2021-09-08 17:07:09,175 # send 2001:16b8:45bb:3d00:b8f3:77d9:fa25:2b01 1234 Test
2021-09-08 17:07:09,178 # Read: "Hello from Linux
2021-09-08 17:07:09,178 # "

@brummer-simon
Copy link
Member

brummer-simon commented Sep 9, 2021 •
edited
Loading

@brummer-simon I just noticed something now that client and server are combined into the same application:
After starting the server, I can no longer open any client connections:

2021-09-08 17:02:30,857 # listen 1234
2021-09-08 17:02:30,859 # Listening on port 1234

> send 2001:16b8:45bb:3d00:b8f3:77d9:fa25:2b01 1234 Test
2021-09-08 17:02:46,663 # send 2001:16b8:45bb:3d00:b8f3:77d9:fa25:2b01 1234 Test
2021-09-08 17:02:46,667 # Error connecting sock: Not enough space

With LWIP=1 this is working fine:

> listen 1234
2021-09-08 17:06:24,483 # listen 1234
2021-09-08 17:06:24,485 # Listening on port 1234

> send 2001:16b8:45bb:3d00:b8f3:77d9:fa25:2b01 1234 Test
2021-09-08 17:07:09,175 # send 2001:16b8:45bb:3d00:b8f3:77d9:fa25:2b01 1234 Test
2021-09-08 17:07:09,178 # Read: "Hello from Linux
2021-09-08 17:07:09,178 # "

This makes sence. By default GNRC_TCP has a preallocated memory for a single receive buffer. If you want to maintain multiple connections at the same time you must increase CONFIG_GNRC_TCP_RCV_BUFFERS to something above 1.

With IPv6 usage each receive buffer in use tries to allocate have at least 1220 Byte from the preallocated memory. To keep memory consumption low GNRC_TCP preallocates only enough space for a single connection by default.

@benpicco
Copy link
Contributor Author

Shouldn't gnrc_pktbuf take care of holding the memory of the packet? Why do we need to copy it to a second buffer?

@miri64
Copy link
Member

miri64 commented Sep 15, 2021

Shouldn't gnrc_pktbuf take care of holding the memory of the packet? Why do we need to copy it to a second buffer?

IIRC, the current gnrc_tcp implementation is not zero-copy. At some point you need to reassemble the TCP segments, so I guess this is where the second buffer comes in. Why gnrc_pktbuf is not used as the space where this happens, I cannot remember.

@benpicco benpicco added the State: stale State: The issue / PR has no activity for >185 days label Feb 9, 2023
@benpicco benpicco requested a review from bergzand February 10, 2023 14:10
@stale stale bot removed the State: stale State: The issue / PR has no activity for >185 days label Dec 1, 2023
@github-actions github-actions bot removed Area: network Area: Networking Area: sys Area: System labels Dec 1, 2023
@riot-ci
Copy link

riot-ci commented Dec 2, 2023 •
edited
Loading

Murdock results

✔️ PASSED

f2d5a4c examples/sock_tcp_echo: add TCP echo client / server

Success Failures Total Runtime
19 0 19 01m:57s

Artifacts

@benpicco benpicco requested a review from maribu December 2, 2023 00:23
maribu
maribu approved these changes Dec 2, 2023
View reviewed changes
Copy link
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.

lgtm, I trust your testing.

Maybe convert to XFA style shell commands? (Sorry for the suggestions not being multiline, I somehow cannot select multiple lines on the phone.)

examples/sock_tcp_echo/main.c Outdated Show resolved Hide resolved
examples/sock_tcp_echo/main.c Outdated Show resolved Hide resolved
examples/sock_tcp_echo/main.c Outdated Show resolved Hide resolved
examples/sock_tcp_echo/main.c Outdated Show resolved Hide resolved
examples/sock_tcp_echo/main.c Outdated Show resolved Hide resolved
examples/sock_tcp_echo/main.c Outdated Show resolved Hide resolved
@Teufelchen1
Copy link
Contributor

Ping @benpicco

@kfessel
Copy link
Contributor

kfessel commented Mar 26, 2024

seems like this is mergable just some small improvements to the coding style

@maribu
Copy link
Member

maribu commented Nov 11, 2024

Ping?

@benpicco benpicco force-pushed the examples/tcp_echo branch 3 times, most recently from 16c6ba5 to 23b1398 Compare November 11, 2024 16:15
@benpicco benpicco added this pull request to the merge queue Nov 11, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 11, 2024
@benpicco benpicco enabled auto-merge November 11, 2024 16:22
@benpicco benpicco added this pull request to the merge queue Nov 11, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 11, 2024
@maribu
Copy link
Member

maribu commented Nov 11, 2024

Looks like the Makefile.ci needs bumping

@benpicco benpicco enabled auto-merge November 11, 2024 23:23
@benpicco benpicco added this pull request to the merge queue Nov 12, 2024
Merged via the queue into RIOT-OS:master with commit 1976e68 Nov 12, 2024
25 checks passed
@benpicco benpicco deleted the examples/tcp_echo branch November 12, 2024 13:06
@MrKevinWeiss MrKevinWeiss added this to the Release 2025.01 milestone Jan 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leandrolanzieri leandrolanzieri leandrolanzieri left review comments

@kfessel kfessel kfessel left review comments

@miri64 miri64 miri64 left review comments

@maribu maribu maribu approved these changes

@brummer-simon brummer-simon Awaiting requested review from brummer-simon

@fjmolinas fjmolinas Awaiting requested review from fjmolinas

@jia200x jia200x Awaiting requested review from jia200x jia200x 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

@bergzand bergzand Awaiting requested review from bergzand

Assignees
No one assigned
Labels
Area: doc Area: Documentation Area: examples Area: Example Applications 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 2025.01
Development

Successfully merging this pull request may close these issues.
10 participants
@benpicco @miri64 @brummer-simon @chrysn @kfessel @riot-ci @Teufelchen1 @maribu @leandrolanzieri @MrKevinWeiss