gnrc_tcp: initial implementation#4744
gnrc_tcp: initial implementation#4744smlng merged 1 commit intoRIOT-OS:masterfrom brummer-simon:devel-gnrc_tcp
Conversation
|
nice! Is there any chance that an initial support version of that makes into the next release? (feature freeze planned on March 30th) |
|
I don't think TCP will be part of the next major release. There is still a lot to do and can't work on TCP further until 8.4.2016. |
|
Feature freeze -> postponed. |
|
Awesome! Congrats! |
|
if i use arduino-due, what setting should i do? |
|
@dondonprad That depends on your usecase. The current default settings work fine with a single TCP connection. If you want multiple parallel connections you must increase the number of preallocated receive buffers. (See the example applications Makefile) If you want three or more parallel connections you might increase the networkstacks pktbuf size as well. |
|
Short Update: TLDR: TCP works fine with multiple parallel connections, everybody is welcome to try it. Example Application: My last bulk test worked on two nodes with the following configuration:
|
|
@brummer-simon one question, what different between gnrc_tcpcli and gnrc_tcpsrv? and if using 2 nodes, one upload gnrc_tcpsrv, the other one upload gnrc_tcpcli? |
|
Yes the gnrc_tcp_srv is the examples server component waiting for an One node has to Run the Server and the other node has to run the Client. The Server prints on startup a list of connectable IP-Adresses. One of I am not sure how to setup the link-layer component for the arduino-uno, Am 02.09.2016 5:03 nachm. schrieb "Doni Pradana" notifications@github.com:
|
|
can this program find time delay TCP? if can how? |
|
Do you mean measuring the timespan between Sendung Data and the time the Am 14.09.2016 10:40 vorm. schrieb "Doni Pradana" notifications@github.com:
|
|
yes, whether using command "ping" or using third party softwere like wireshark? |
|
Okay first of all: Ping is not based on TCP, and there is currently no shell command using TCP. You can build a measure probe based on a raspberry pi with wireshark. There If you just want to measure the transmission time of a single tcp packet, Am 14.09.2016 11:24 vorm. schrieb "Doni Pradana" notifications@github.com:
|
|
@brummer-simon i tried compiled and flash using arduino due and it works, but did not get an ip address.. |
|
The IP-Address is generated by the IP Layer. The first thing the server Here is an example: Am Samstag, den 24.09.2016, 01:09 -0700 schrieb Doni Pradana:
|
DipSwitch
left a comment
There was a problem hiding this comment.
It looks good and I'm currently testing it a bit. Below are some pointers that might need addressing, depending on what others think.
Some overall points:
- The -EPERM could be removed I think, this makes it hard for other threads to send and receive on the given TCP connection. Don't know if this is possible.
- I personally would like to see some non-blocking I/O features. Like the ability to not use the USER_TIMEOUT message. And when calling
recvbe able to see if there is data available. Or by adding aconn_tcp_ioctlcall or by adding flags torecvfor example which can disable the retry mechanism. - The use of the name
erras return value variable is misleading. I thinkresorretwould be better since we don't know if it's an error or not. - There is not yet a hard border between the conn API and the TCP interface itself. If you would use TCP without conn API you would have to copy some logic from conn. I think this should be handled by the fsm instead.
This was my first impression of the code. Job well done! :)
sys/include/net/gnrc/tcp.h
Outdated
| * @return Not NULL on success | ||
| */ | ||
| gnrc_pktsnip_t *gnrc_tcp_hdr_build(gnrc_pktsnip_t *payload, uint8_t *src, size_t src_len, | ||
| uint8_t *dst, size_t dst_len); |
There was a problem hiding this comment.
This can be changed to gnrc_tcp_hdr_build(gnrc_pktsnip_t *payload, uint16_t src, uint16_t dst) just like it was changed in UDP.
| * @return -EADDRNOTAVAIL if the given address is not assigned to an interface. | ||
| * @return -EAFNOSUPPORT given address family is not supported. | ||
| */ | ||
| int conn_tcp_create(conn_tcp_t *conn, const void *addr, size_t addr_len, int family, |
There was a problem hiding this comment.
addr_len could be removed since the family defines the length.
examples/gnrc_tcp_srv/main.c
Outdated
| uint8_t bufs[BACKLOG][NBYTE]; | ||
| uint8_t stacks[BACKLOG][THREAD_STACKSIZE_DEFAULT + THREAD_EXTRA_STACKSIZE_PRINTF]; | ||
|
|
||
| void *srv_thread(__attribute__((unused)) void *arg); |
examples/gnrc_tcp_cli/main.c
Outdated
| return 0; | ||
| } | ||
|
|
||
| void *cli_thread(__attribute__((unused)) void *arg) |
| #define MSG_TYPE_PROBE_TIMEOUT (GNRC_NETAPI_MSG_TYPE_ACK + 2) | ||
| #define MSG_TYPE_RETRANSMISSION (GNRC_NETAPI_MSG_TYPE_ACK + 3) | ||
| #define MSG_TYPE_TIMEWAIT (GNRC_NETAPI_MSG_TYPE_ACK + 4) | ||
| #define MSG_TYPE_NOTIFY_USER (GNRC_NETAPI_MSG_TYPE_ACK + 5) |
There was a problem hiding this comment.
please make this define public so you can create a thread which listens to multiple message sources and is also using the TCP conn API. So it knows when to handle the TCP connection or do other logic.
static void* _at_tcp_server(void *args)
{
assert(args);
ATDataHandler *handler = (ATDataHandler*)args;
at_conn_tcp_t *hdl = (at_conn_tcp_t*)handler->iface_args;
at_conn_tcp_t clients[TCP_CLIENTS_PER_SERVER];
at_conn_tcp_t *c;
/* create a listening TCP socket */
conn_tcp_listen(&hdl->tcp_conn, clients, TCP_CLIENTS_PER_SERVER);
/* try accepting new TCP clients */
msg_t m;
while (msg_receive(&m) > 0)
{
switch (m.content.value) {
case MSG_TYPE_NOTIFY_USER: // TODO export this define so you can make a parent thread which
int ret = conn_tcp_accept(&hdl->tcp_conn, &c);
// TODO create a new handler thread for the conn
break;
case TCP_MSG_CLOSE:
// TODO close all clients
// TODO close the server socket
break;
case TCP_MSG_QUIT_THREAD:
// TODO do actions of: TCP_MSG_CLOSE
// TODO close the thread
return NULL;
}
}
}
| if (conn->status & GNRC_TCP_STATUS_PASSIVE && !(conn->status & GNRC_TCP_STATUS_ACCEPTED)) { | ||
| return -EBADF; | ||
| } | ||
|
|
There was a problem hiding this comment.
All -EPERM, -EALREADY and -EBADF could (and maybe should) be thrown from the state machine since this is essential to the state process itself.
| /* This call is only allowed on active Connections*/ | ||
| if (conn->status & GNRC_TCP_STATUS_PASSIVE) { | ||
| return -EOPNOTSUPP; | ||
| } |
There was a problem hiding this comment.
All -EISCONN, -EOPNOTSUPP could (and maybe should) be thrown from the state machine since this is essential to the state process itself.
| /* Guard: Specifing struct must be passive(listen was called on it). */ | ||
| if (!(conn->status & GNRC_TCP_STATUS_PASSIVE)) { | ||
| return -EOPNOTSUPP; | ||
| } |
There was a problem hiding this comment.
This could also be handled by the state process.
| && conn->state != FIN_WAIT_2 && conn->state != CLOSE_WAIT) { | ||
| return -EALREADY; | ||
| } | ||
|
|
There was a problem hiding this comment.
All -EPERM, -EBADF and -EALREADY could (and maybe should) be thrown from the state machine since this is essential to the state process itself.
| /* Guard: Connection is in an illegal state for this operation */ | ||
| if (conn->state != ESTABLISHED && conn->state != CLOSE_WAIT) { | ||
| return -EALREADY; | ||
| } |
There was a problem hiding this comment.
All -EPERM, -EBADF and -EALREADY could (and maybe should) be thrown from the state machine since this is essential to the state process itself.
| probing = false; | ||
| xtimer_remove(&tout_probe); | ||
| } | ||
| break; |
There was a problem hiding this comment.
The logic of MSG_TYPE_PROBE_TIMEOUT and MSG_TYPE_NOTIFY_USER could also be implemented in the fsm.
DipSwitch
left a comment
There was a problem hiding this comment.
With this changes and disabling the -EPERM tests and removing msg_tout and MSG_TYPE_USER_TIMEOUT from the in the conn API I was able to run the following two threads and connect to and from Linux:
I would say TCP is working, tomorrow the memory management and regression testing :)
static void* _at_tcp_server(void *args)
{
assert(args);
ATDataHandler *handler = (ATDataHandler*)args;
at_conn_tcp_t *hdl = (at_conn_tcp_t*)handler->iface_args;
conn_tcp_t clients[TCP_CLIENTS_PER_SERVER];
conn_tcp_t *c;
/* handle the client connections */
at_conn_tcp_client_t holders[TCP_CLIENTS_PER_SERVER];
at_conn_tcp_client_t *free_clients = NULL;
at_conn_tcp_client_t *used_clients = NULL;
for (int idx = 0; idx < TCP_CLIENTS_PER_SERVER; ++idx)
LL_PREPEND(free_clients, holders + idx);
/* create a listening TCP socket */
conn_tcp_create(&hdl->tcp_conn, NULL, sizeof(ipv6_addr_t), AF_INET6, handler->src_port);
conn_tcp_listen(&hdl->tcp_conn, clients, TCP_CLIENTS_PER_SERVER);
DEBUG("_at_tcp_server(): returned from TCP listen\n");
/* try accepting new TCP clients */
int ret;
msg_t m;
while (msg_receive(&m) > 0)
{
DEBUG("_at_tcp_server(): received a new message\n");
switch (m.type)
{
// TODO export this define so you can make a parent thread which
case GNRC_NETAPI_MSG_TYPE_ACK + 5:
/* accept a new client if we have to */
ret = conn_tcp_accept(&hdl->tcp_conn, &c);
if (ret == -EAGAIN) {
/* no new clients are waiting nothing to do here */
} else if (ret == 0) {
DEBUG("_at_tcp_server(): received new connection\n");
/* create a new client handler object */
at_conn_tcp_client_t *client = free_clients;
/* create a new data handler object */
client->tcp_conn = c;
client->identifier = TCP_CLIENT_IDENTIFIER;
client->handler = ATAllocDataHandler(handler->src_port, c->peer_port, (ipv6_addr_t*)c->peer_addr, false, &_at_tcp_interface, client);
assert(client->handler); // TODO close the client connection without saying anything
/* move to the used list */
LL_DELETE(free_clients, client);
LL_PREPEND(used_clients, client);
/* TODO inform the host that a new connection is available */
ATResponse resp = ATRegisterDataHandler(client->handler);
assert(resp == OK);
} else {
switch (ret) {
case -EOPNOTSUPP:
DEBUG("conn_tcp_accept() : -EOPNOTSUPP : Operation not supported on active Connection\n");
//return NULL;
default:
DEBUG("conn_tcp_accept() : %d : Unexpected return value: Go fix it!\n", ret);
//return NULL;
}
}
/* reserve a base packet to store the data */
gnrc_pktsnip_t *snip = gnrc_pktbuf_add(NULL, NULL, TCP_MAX_PACKET_SIZE, GNRC_NETTYPE_UNDEF);
if (!snip)
DEBUG("_at_tcp_server(): no packet buffer allocated");
/* check if any data is received on the client connections */
at_conn_tcp_client_t *cli, *tmp;
LL_FOREACH_SAFE(used_clients, cli, tmp)
{
/* try to read data from the connection */
int read;
while ((read = conn_tcp_recv(cli->tcp_conn, snip->data, TCP_MAX_PACKET_SIZE)) > 0) {
/* we have read data, create a new data interrupt */
if (read < TCP_MAX_PACKET_SIZE)
gnrc_pktbuf_realloc_data(snip, read);
/* queue the incomming packet in the data buffer */
DEBUG("_at_tcp_server(): received packet containing %d bytes\n", read);
if (!ATReceivedPacket(snip, &_at_tcp_interface, &cli->handler->addr, cli->handler->dst_port, cli->handler->src_port))
assert(false);
/* create a new packet snippet */
snip = gnrc_pktbuf_add(NULL, NULL, TCP_MAX_PACKET_SIZE, GNRC_NETTYPE_UNDEF);
}
/* check if the connection has closed */
DEBUG("_at_tcp_server(): read has returned with: %d\n", read);
if (read == 0 && cli->tcp_conn->state == CLOSE_WAIT)
{
/* TODO notify the host */
/* free the data handler */
ATFreeDataHandler(cli->handler);
/* the connection has closed, we must free it */
LL_DELETE(used_clients, cli);
LL_PREPEND(free_clients, cli);
}
}
/* free the temporary buffer */
gnrc_pktbuf_release(snip);
break;
case TCP_MSG_CLOSE:
// TODO close all clients
// close the server socket
ret = conn_tcp_close(&hdl->tcp_conn);
switch (ret) {
case 0:
DEBUG("conn_tcp_close() : 0 : ok\n");
break;
default:
DEBUG("conn_tcp_close() : %d : Unexpected return value: Go fix it!\n", ret);
}
break;
case TCP_MSG_QUIT_THREAD:
// TODO do actions of: TCP_MSG_CLOSE
// TODO close the thread
break;
}
}
return NULL;
}
static void* _at_tcp_client(void *args)
{
assert(args);
ATDataHandler *handler = (ATDataHandler*)args;
at_conn_tcp_t *hdl = (at_conn_tcp_t*)handler->iface_args;
/* create a listening TCP socket */
conn_tcp_create(&hdl->tcp_conn, NULL, sizeof(ipv6_addr_t), AF_INET6, handler->src_port);
int ret = conn_tcp_connect(&hdl->tcp_conn, &handler->addr, sizeof(ipv6_addr_t), handler->dst_port);
DEBUG("_at_tcp_client(): returned from TCP connect: %d\n", ret);
/* try accepting new TCP clients */
msg_t m;
while (msg_receive(&m) > 0)
{
DEBUG("_at_tcp_client(): received a new message\n");
switch (m.type)
{
// TODO export this define so you can make a parent thread which
case GNRC_NETAPI_MSG_TYPE_ACK + 5:
{
/* reserve a base packet to store the data */
gnrc_pktsnip_t *snip = gnrc_pktbuf_add(NULL, NULL, TCP_MAX_PACKET_SIZE, GNRC_NETTYPE_UNDEF);
if (!snip)
DEBUG("_at_tcp_client(): no packet buffer allocated");
/* try to read data from the connection */
int read;
while ((read = conn_tcp_recv(&hdl->tcp_conn, snip->data, TCP_MAX_PACKET_SIZE)) > 0) {
/* we have read data, create a new data interrupt */
if (read < TCP_MAX_PACKET_SIZE)
gnrc_pktbuf_realloc_data(snip, read);
/* queue the incomming packet in the data buffer */
DEBUG("_at_tcp_client(): received packet containing %d bytes\n", read);
if (!ATReceivedPacket(snip, &_at_tcp_interface, &handler->addr, handler->dst_port, handler->src_port))
assert(false);
/* create a new packet snippet */
snip = gnrc_pktbuf_add(NULL, NULL, TCP_MAX_PACKET_SIZE, GNRC_NETTYPE_UNDEF);
}
/* check if the connection has closed */
DEBUG("_at_tcp_client(): read has returned with: %d\n", read);
if (read == 0 && hdl->tcp_conn.state == CLOSE_WAIT)
{
/* TODO notify the host */
/* free the data handler */
ATFreeDataHandler(handler);
}
/* free the temporary buffer */
gnrc_pktbuf_release(snip);
} break;
case TCP_MSG_CLOSE:
{
// TODO close all clients
// close the server socket
ret = conn_tcp_close(&hdl->tcp_conn);
switch (ret) {
case 0:
DEBUG("conn_tcp_close() : 0 : ok\n");
break;
default:
DEBUG("conn_tcp_close() : %d : Unexpected return value: Go fix it!\n", ret);
break;
}
} break;
case TCP_MSG_QUIT_THREAD:
{
// TODO do actions of: TCP_MSG_CLOSE
// TODO close the thread
} break;
}
}
return NULL;
}
And then from another thread I was able to call these callbacks.
/**
* Send the UDP packet and return to command mode
*/
static ATResponse _at_tcp_finished_write(ATDataHandler *handler, size_t len)
{
assert(handler);
assert(handler->iface_args);
/* if no data in the buffer just free it */
if (!len) {
gnrc_pktbuf_release(handler->output);
return OK;
}
/* resize the TCP pay-load */
if (gnrc_pktbuf_realloc_data(handler->output, len) != 0) {
gnrc_pktbuf_release(handler->output);
return ATSetErrorMessage(ErrorInternal, "unable to reallocate the TCP header");
}
data = (uint8_t*)handler->output->data;
/* send data to the given TCP connection */
int res = 0;
uint8_t *data;
uint32_t ident = *(uint32_t*)handler->iface_args;
for (int idx = 0; idx < len; idx += res)
{
/* find the object that is stored in the interface arguments */
if (ident == TCP_MAIN_IDENTIFIER) {
/* we use a main TCP object created by the AT command set */
at_conn_tcp_t *hdl = (at_conn_tcp_t*)handler->iface_args;
res = conn_tcp_send(&hdl->tcp_conn, data + res, len - res);
} else if (ident == TCP_CLIENT_IDENTIFIER) {
/* we use a TCP client object create by a TCP server */
at_conn_tcp_client_t *hdl = (at_conn_tcp_client_t*)handler->iface_args;
res = conn_tcp_send(hdl->tcp_conn, data + res, len - res);
}
}
return OK;
}
/**
* Unregister the UDP listening connection
*/
static ATResponse _at_tcp_conn_close(ATDataHandler *handler)
{
assert(handler);
assert(handler->iface_args);
/* close the connection */
int ret = -1234567;
uint32_t identity = *(uint32_t*)handler->iface_args;
if (identity == TCP_MAIN_IDENTIFIER) {
at_conn_tcp_t *hdl = (at_conn_tcp_t*)handler->iface_args;
ret = conn_tcp_close(&hdl->tcp_conn);
} else if (identity == TCP_CLIENT_IDENTIFIER) {
at_conn_tcp_client_t *hdl = (at_conn_tcp_client_t*)handler->iface_args;
ret = conn_tcp_close(hdl->tcp_conn);
/* TODO how to free the client object in the server ? Does this happen automatically due to the messages ? */
}
return OK;
}
| mutex_unlock(&_list_conn_tcp_lock); | ||
|
|
||
| /* Mark Connection as passive */ | ||
| conn->status |= GNRC_TCP_STATUS_PASSIVE; |
There was a problem hiding this comment.
Please also add:
/* The server thread is also the owner of this conn object */
conn->owner = thread_getpid();
| return -EAFNOSUPPORT; | ||
| #endif | ||
| /* Mark Connections as passive */ | ||
| queue[i].status |= GNRC_TCP_STATUS_PASSIVE; |
There was a problem hiding this comment.
Please also add this:
/* Since the server will be owner anyway we can already set this thread as the owning thread */
queue[i].owner = thread_getpid();
| mutex_lock(&_list_conn_tcp_lock); | ||
| iter = _list_conn_tcp_head; | ||
| while (iter) { | ||
| if (conn->local_port == iter->local_port && !(iter->status & GNRC_TCP_STATUS_ACCEPTED)) { |
There was a problem hiding this comment.
This should probably be changed to: if (conn->local_port == iter->local_port && !(iter->status & GNRC_TCP_STATUS_ACCEPTED) && iter->state == SYN_RCVD) {
| /* Clear Accepted Flag */ | ||
| conn->status &= ~GNRC_TCP_STATUS_ACCEPTED; | ||
|
|
||
| *notify_owner = true; |
There was a problem hiding this comment.
Add so you can remove the _fsm(CALL_OPEN) on passive TCP conn objects from the conn API:
/* if this is a passive connection, we must reopen it in the listening state */
if (conn->status & GNRC_TCP_STATUS_PASSIVE) {
_fsm_unprotected(conn, CALL_OPEN, NULL, NULL, 0, notify_owner);
exit 0;
}
|
|
||
| case CLOSE_WAIT: *notify_owner = true; | ||
| break; | ||
|
|
There was a problem hiding this comment.
Please add here:
case SYN_RCVD: *notify_owner = true;
break;
|
Jenkins is not happy, but is just RAM overflow on board nucleo32-f303, add to insufficient mem list |
|
From my Point of view, gnrc_tcp is ready for its initial release (as soon as the build is successful of course). The only thing I would add until release are bug fixes. Any Objections? |
|
ping @miri64 happy? |
|
;-) |
|
Could anyone tell me what happend to "SEC_IN_USEC" ? That seems to be missing since rebase ;) |
|
@brummer-simon see #6399 |
|
@miri64 Thanks, i changed it |
|
@PeterKietzmann I'm just doing same final tests, can you wait until lunch (CET) with merge? |
|
Would you keep me updated on your test results (via Mail or something)? I am interested in it. |
|
No worries, I won't hit the button. Just wanted raise attention. |
| mutex_lock(&_list_gnrc_tcp_tcb_lock); | ||
| tcb = _list_gnrc_tcp_tcb_head; | ||
| while (tcb) { | ||
| #ifdef MODULE_GNRC_IPV6 |
There was a problem hiding this comment.
something is wrong with the TCB matching here. I wrote a small tcp server to listen on a certain port on any address and prints incoming messages. I sniffed with wireshark, and saw that handshake was successful. But when I send data to the server the server replies with a RST package (and win=0).
I enable debug output and saw that no matching TCB was found here. I then commented this whole section out, effectively having tcb = _list_gnrc_tcp_tcb_head;, which is okay because my test server can only handle 1 connection. And that worked!
In conclusion, the checks below do not find the correct tcb, that is none in my test case 😞
I'll try to find the error and provide a fix ...
There was a problem hiding this comment.
Okay I found the issue: parsing of the destination port fails, because you change pointers.
Move:
src = byteorder_ntohs(hdr->src_port);
dst = byteorder_ntohs(hdr->dst_port);
upwards, e.g. directly below
hdr = (tcp_hdr_t *)tcp->data;
ctl = byteorder_ntohs(hdr->off_ctl);
The problem is that hdr = tcp->data hence hdr->dst_port = tcp->data->dst_port, but when you mark the tcp header in L138 you change the pointer, that way hdr isn't where it should be anymore.
|
btw. my test was RIOT native gnrc_tcp against |
|
@smlng : I've added your changes and interop between Arch Linux and RIOT works as well. One last thing. I'll try to test between two samr21-xpro boards. With the given Makefiles I can't seem to aquire a link-local IP Address. Any Idea what additional modules I have to include? |
|
Will look into that on Monday, don't have the boards on hand atm. Please ping me in case I forget. |
|
@brummer-simon: the Makefiles of your examples look fine, I use the same modules for tcp_listen (see my last comment) and it gets a link local address via auto init. Maybe your code for printing the address in tcp_test_srv L58 is buggy, the following works for me: |
@brummer-simon you do not use the |
|
Update: I am not sure what exactly went wrong yesterday, but today I get valid ipv6 adresses. I can confirm that gnrc_tcp works between two samr21 boards. :) |
|
@brummer-simon Just tested between samr21-xpro and a RaspberryPi + OpenLabs, works like charm! Great job! |
|
Only took as next to a year but now we finally have TCP 🎆 🎉 |
|
@miri64: Jeah I clearly underestimated TCPs complexity ^^ |

TCP implementation for gnrc.
There are still a lot do, so this pull request is for referential purposes.
Current todo list:
Zero Window ProbingAdaptive Transamission Times (Retransmission time is currently a constant)Sequence Number Overflow detectionPiggybagging pending data on acknowledgements(Not part of initial release)Implementation of conn interface(Not part of initial release)Final overhaul (Doc and Code)Testing (native and hardware)