Skip to content

gnrc_tcp: initial implementation#4744

Merged
smlng merged 1 commit intoRIOT-OS:masterfrom
brummer-simon:devel-gnrc_tcp
Jan 23, 2017
Merged

gnrc_tcp: initial implementation#4744
smlng merged 1 commit intoRIOT-OS:masterfrom
brummer-simon:devel-gnrc_tcp

Conversation

@brummer-simon
Copy link
Copy Markdown
Member

@brummer-simon brummer-simon commented Feb 4, 2016

TCP implementation for gnrc.
There are still a lot do, so this pull request is for referential purposes.

Current todo list:

  1. Zero Window Probing
  2. Adaptive Transamission Times (Retransmission time is currently a constant)
  3. Sequence Number Overflow detection
  4. Piggybagging pending data on acknowledgements (Not part of initial release)
  5. Implementation of conn interface (Not part of initial release)
  6. Final overhaul (Doc and Code)
  7. Testing (native and hardware)

@brummer-simon brummer-simon mentioned this pull request Feb 4, 2016
@cgundogan cgundogan added Area: network Area: Networking Type: new feature The issue requests / The PR implemements a new feature for RIOT State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet labels Feb 4, 2016
@cgundogan cgundogan self-assigned this Feb 4, 2016
@emmanuelsearch
Copy link
Copy Markdown
Member

nice! Is there any chance that an initial support version of that makes into the next release? (feature freeze planned on March 30th)

@brummer-simon
Copy link
Copy Markdown
Member Author

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.

@miri64 miri64 modified the milestone: Release 2016.07 Mar 29, 2016
@kYc0o
Copy link
Copy Markdown
Contributor

kYc0o commented Jul 25, 2016

Feature freeze -> postponed.

@kYc0o kYc0o modified the milestones: Release 2016.10, Release 2016.07 Jul 25, 2016
@brummer-simon
Copy link
Copy Markdown
Member Author

I proudly present the first live capture of TCP on RIOT between samr21 boards.
There are a few things left to do but soon everybody is welcome play arround with it.

riot_tcp

@OlegHahm
Copy link
Copy Markdown
Member

OlegHahm commented Aug 9, 2016

Awesome! Congrats!

@OlegHahm OlegHahm added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Aug 15, 2016
@dondonprad
Copy link
Copy Markdown

if i use arduino-due, what setting should i do?

@brummer-simon
Copy link
Copy Markdown
Member Author

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

@brummer-simon
Copy link
Copy Markdown
Member Author

Short Update:

TLDR: TCP works fine with multiple parallel connections, everybody is welcome to try it.

Example Application:
The example application establishes a connection between two nodes, exchanges and verifies a transmission of 2kb in each direction and closes the connection. This procesdure is repeated in a loop.

My last bulk test worked on two nodes with the following configuration:

  • Nodes: samr21-xpro
  • Each Node started two threads, each processing a connection.
  • The example application completed 10000 test cycles for each connection.
  • In a few testcycles the connection broke down, meaning send and receive timedout, the problem
    vanished on entering the next testcycle, after a new connection was established.

@dondonprad
Copy link
Copy Markdown

@brummer-simon
my case single TCP connection and using 2 nodes (arduino due). arrangement i mean, how setting makefile, because your case using samr21-pro..

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?

@brummer-simon
Copy link
Copy Markdown
Member Author

Yes the gnrc_tcp_srv is the examples server component waiting for an
incomming connection and the gnrc_tcp_cli is the examples client.

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
those addresses needs to be added to the Makefile to set the according
macro.

I am not sure how to setup the link-layer component for the arduino-uno,
maybe ask on the devel mailinglist for that. The samr21-xpro has that
builtin.

Am 02.09.2016 5:03 nachm. schrieb "Doni Pradana" notifications@github.com:

@brummer-simon https://github.com/brummer-simon
my case single TCP connection and using 2 nodes (arduino due). arrangement
i mean, how setting makefile, because your case using samr21-pro..

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?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#4744 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ADxGJ--aBEMo0h7yWui4GdXp8Iy7C77Cks5qmDqmgaJpZM4HTYS5
.

@dondonprad
Copy link
Copy Markdown

can this program find time delay TCP? if can how?

@brummer-simon
Copy link
Copy Markdown
Member Author

Do you mean measuring the timespan between Sendung Data and the time the
Sender receives the according acknowlegement?

Am 14.09.2016 10:40 vorm. schrieb "Doni Pradana" notifications@github.com:

can this program find time delay TCP? if can how?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#4744 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ADxGJ75fmpnAhibXZA3nBOGAVmDTHPPMks5qp7LpgaJpZM4HTYS5
.

@dondonprad
Copy link
Copy Markdown

yes, whether using command "ping" or using third party softwere like wireshark?

@brummer-simon
Copy link
Copy Markdown
Member Author

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
is a Wiki entry how to do that.

If you just want to measure the transmission time of a single tcp packet,
just measure time before and after the conn_tcp_send call. The function
transmits a single packet and returns after receiving an acknowlegement for
the sent packet.

Am 14.09.2016 11:24 vorm. schrieb "Doni Pradana" notifications@github.com:

yes, whether using command "ping" or using third party softwere like
wireshark?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#4744 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ADxGJ_Q_HniVLrxGbHkauIwvWPPVNOQoks5qp71DgaJpZM4HTYS5
.

@dondonprad
Copy link
Copy Markdown

dondonprad commented Sep 24, 2016

@brummer-simon i tried compiled and flash using arduino due and it works, but did not get an ip address..
whether the ip address is derived from program with or from inculed by self?
one more question, how to connect device with wireshark for snnifing?

@brummer-simon
Copy link
Copy Markdown
Member Author

The IP-Address is generated by the IP Layer. The first thing the server
application does is printing its IP-Address. For the Demo, you can copy
this IP-Address and paste it into the client applications make-file.

Here is an example:
CFLAGS += -DPEER_ADDR="fe80::5855:6a41:9603:32c2"

Am Samstag, den 24.09.2016, 01:09 -0700 schrieb Doni Pradana:

i tried compiled and flash using arduino due and it works, but did
not get an ip address..
whether the ip address is derived from program with or from inculed
by self?

You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

Copy link
Copy Markdown
Member

@DipSwitch DipSwitch left a comment

Choose a reason for hiding this comment

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

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 recv be able to see if there is data available. Or by adding a conn_tcp_ioctl call or by adding flags to recv for example which can disable the retry mechanism.
  • The use of the name err as return value variable is misleading. I think res or ret would 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! :)

* @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);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

addr_len could be removed since the family defines the length.

uint8_t bufs[BACKLOG][NBYTE];
uint8_t stacks[BACKLOG][THREAD_STACKSIZE_DEFAULT + THREAD_EXTRA_STACKSIZE_PRINTF];

void *srv_thread(__attribute__((unused)) void *arg);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

arg is not unused anymore.

return 0;
}

void *cli_thread(__attribute__((unused)) void *arg)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

arg is not unused anymore

#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)
Copy link
Copy Markdown
Member

@DipSwitch DipSwitch Sep 28, 2016

Choose a reason for hiding this comment

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

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;
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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;
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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;
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This could also be handled by the state process.

&& conn->state != FIN_WAIT_2 && conn->state != CLOSE_WAIT) {
return -EALREADY;
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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;
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The logic of MSG_TYPE_PROBE_TIMEOUT and MSG_TYPE_NOTIFY_USER could also be implemented in the fsm.

Copy link
Copy Markdown
Member

@DipSwitch DipSwitch left a comment

Choose a reason for hiding this comment

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

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;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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)) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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;
Copy link
Copy Markdown
Member

@DipSwitch DipSwitch Sep 29, 2016

Choose a reason for hiding this comment

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

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;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please add here:

    case SYN_RCVD:      *notify_owner = true;
        break;

@smlng
Copy link
Copy Markdown
Member

smlng commented Jan 19, 2017

Jenkins is not happy, but is just RAM overflow on board nucleo32-f303, add to insufficient mem list

@brummer-simon
Copy link
Copy Markdown
Member Author

brummer-simon commented Jan 19, 2017

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?

@smlng
Copy link
Copy Markdown
Member

smlng commented Jan 19, 2017

ping @miri64 happy?

Copy link
Copy Markdown
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.

After a quick review I didn't see anything in the code that would justify a change request and I trust @smlng's review

@miri64 miri64 changed the title gnrc tcp gnrc_tcp: initial implementation Jan 19, 2017
@miri64
Copy link
Copy Markdown
Member

miri64 commented Jan 19, 2017

;-)

@brummer-simon
Copy link
Copy Markdown
Member Author

Could anyone tell me what happend to "SEC_IN_USEC" ? That seems to be missing since rebase ;)

@miri64
Copy link
Copy Markdown
Member

miri64 commented Jan 19, 2017

@brummer-simon see #6399

@brummer-simon
Copy link
Copy Markdown
Member Author

@miri64 Thanks, i changed it

@PeterKietzmann
Copy link
Copy Markdown
Member

@miri64, @smlng many green lights!!!

@smlng
Copy link
Copy Markdown
Member

smlng commented Jan 20, 2017

@PeterKietzmann I'm just doing same final tests, can you wait until lunch (CET) with merge?

@brummer-simon
Copy link
Copy Markdown
Member Author

brummer-simon commented Jan 20, 2017

Would you keep me updated on your test results (via Mail or something)? I am interested in it.
I am not available till tomorrow (my employer has its 20th anniversary * insert Beer Mug Emoji *)

@PeterKietzmann
Copy link
Copy Markdown
Member

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
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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 ...

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

@smlng smlng left a comment

Choose a reason for hiding this comment

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

please fix this

@smlng
Copy link
Copy Markdown
Member

smlng commented Jan 20, 2017

btw. my test was RIOT native gnrc_tcp against netcat on macOS. Which means interop RIOT macOS works! I put my test tool named tcp_listen online here

@brummer-simon
Copy link
Copy Markdown
Member Author

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

@miri64
Copy link
Copy Markdown
Member

miri64 commented Jan 21, 2017

Will look into that on Monday, don't have the boards on hand atm. Please ping me in case I forget.

@smlng
Copy link
Copy Markdown
Member

smlng commented Jan 21, 2017

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

    kernel_pid_t ifs[GNRC_NETIF_NUMOF];
    size_t numof = gnrc_netif_get(ifs);
    if (numof > 0) {
        gnrc_ipv6_netif_t *entry = gnrc_ipv6_netif_get(ifs[0]);
        char ipv6_addr_str[IPV6_ADDR_MAX_STR_LEN];
        for (int i = 0; i < GNRC_IPV6_NETIF_ADDR_NUMOF; i++) {
            if ((ipv6_addr_is_link_local(&entry->addrs[i].addr)) && !(entry->addrs[i].flags & GNRC_IPV6_NETIF_ADDR_FLAGS_NON_UNICAST)) {
                ipv6_addr_to_str(ipv6_addr_str, &entry->addrs[i].addr, IPV6_ADDR_MAX_STR_LEN);
                printf(" -- %s\n", ipv6_addr_str);
            }
        }
}

@miri64
Copy link
Copy Markdown
Member

miri64 commented Jan 21, 2017

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 ifconfig command provided by shell_commands? Why?

@brummer-simon
Copy link
Copy Markdown
Member Author

@smlng : Under native, I have get a link-local Address without any Problems. With the samr21 Boards it is a different story.

@miri64 : I don't know why I didn't do that, I changed it.

@brummer-simon
Copy link
Copy Markdown
Member Author

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. :)

@smlng
Copy link
Copy Markdown
Member

smlng commented Jan 23, 2017

@brummer-simon Just tested between samr21-xpro and a RaspberryPi + OpenLabs, works like charm! Great job!

@smlng smlng merged commit 7f7329e into RIOT-OS:master Jan 23, 2017
@miri64
Copy link
Copy Markdown
Member

miri64 commented Jan 23, 2017

Only took as next to a year but now we finally have TCP 🎆 🎉

@brummer-simon brummer-simon deleted the devel-gnrc_tcp branch January 23, 2017 08:53
@brummer-simon
Copy link
Copy Markdown
Member Author

brummer-simon commented Jan 23, 2017

@miri64: Jeah I clearly underestimated TCPs complexity ^^

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: new feature The issue requests / The PR implemements a new feature for RIOT

Projects

None yet

Development

Successfully merging this pull request may close these issues.