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

[WIP]gnrc_tcp #2827

Closed
wants to merge 1 commit into from
Closed

[WIP]gnrc_tcp #2827

wants to merge 1 commit into from

Conversation

brummer-simon
Copy link
Member

Current State of ng_tcp. This Branch is currently under heavy development. Your comments on the current state are very welcome :).

This PR is intendend for reference in the NSTF Todo List.

@OlegHahm OlegHahm added Area: network Area: Networking State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet Type: new feature The issue requests / The PR implemements a new feature for RIOT NSTF labels Apr 18, 2015
OlegHahm
OlegHahm reviewed Apr 18, 2015
View reviewed changes
sys/include/net/ng_tcp.h
/**
* @brief States for the TCP State Maschine
*/
typedef enum {CLOSED, LISTEN, SYN_SENT, SYN_RCVD, ESTABLISHED} ng_tcp_states_t;
Copy link
Member

Choose a reason for hiding this comment

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

For readability and documentation I would prefer to put each enum into a single line.

@OlegHahm
Copy link
Member

@cgundogan, maybe you are interested, too.

@miri64
Copy link
Member

miri64 commented Apr 18, 2015

Please don't use sub-directories in tests (except unittests). See e.g. thread and vtimer tests.

miri64
miri64 reviewed Apr 18, 2015
View reviewed changes
sys/net/crosslayer/ng_netreg/ng_netreg.c
@@ -22,6 +22,10 @@
#include "net/ng_pkt.h"
#include "net/ng_ipv6.h"

#ifdef MODULE_NG_TCP
Copy link
Member

Choose a reason for hiding this comment

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

ifdefs should not be necessary here

@miri64 miri64 added this to the Network Stack Task Force milestone Apr 18, 2015
@miri64 miri64 mentioned this pull request Apr 18, 2015
Closed
36 tasks
@brummer-simon brummer-simon force-pushed the devel-ng_tcp branch 2 times, most recently from c888411 to d4d8d5d Compare April 23, 2015 13:58
@brummer-simon brummer-simon force-pushed the devel-ng_tcp branch 6 times, most recently from 5be55d5 to 0b8b7dc Compare May 4, 2015 12:43
@brummer-simon brummer-simon force-pushed the devel-ng_tcp branch 3 times, most recently from 5e92e94 to a863050 Compare May 12, 2015 16:24
@miri64 miri64 assigned cgundogan and unassigned miri64 Jun 2, 2015
@brummer-simon brummer-simon force-pushed the devel-ng_tcp branch 2 times, most recently from 641a619 to 8018107 Compare July 23, 2015 12:54
@brummer-simon
Copy link
Member Author

My original goal was to implement the connection termination first, to make sure the fsm work as it should be. But as i wrote yesterday, I focus the next months on my Bachelor Thesis.

@brummer-simon
Copy link
Member Author

Oh the merge included a lot files that aren't involved in the hole tcp thing. I'll fix that in the afternoon.

@DipSwitch
Copy link
Member

Both are even important :) good luck with your thesis!

@miri64
Copy link
Member

miri64 commented Aug 26, 2015

@DipSwitch please comment in the PR not the commit. Comments in commits get easily lost.

@miri64
Copy link
Member

miri64 commented Aug 26, 2015

You probably need to rebase to current master. There are a lot of changes in here now, that are actually a part of master already.

@brummer-simon brummer-simon force-pushed the devel-ng_tcp branch 3 times, most recently from ecda3aa to 5841c03 Compare August 26, 2015 11:51
@DipSwitch
Copy link
Member

I rebased and forced updated, sorry not the best way possible...

If you do close implementation, I'll see if I can start with send in my free time ^_^

@brummer-simon
Copy link
Member Author

Yes I'll implement the close operation. Over the next weeks.

@miri64 miri64 modified the milestones: Release 2015.08, Network Stack Task Force, Release NEXT MAJOR Sep 2, 2015
cgundogan
cgundogan reviewed Oct 8, 2015
View reviewed changes
sys/net/gnrc/transport_layer/tcp/gnrc_tcp.c
/**
* @brief Head of tcb linked list. The eventloop needs
* to look up the affected tcbs in case of an incomming paket.
* ng_tcp_tcb_init() adds tcb to this list.
Copy link
Member

Choose a reason for hiding this comment

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

s/ng/gnrc/

@cgundogan cgundogan changed the title [WIP]ng_tcp [WIP]gnrc_tcp Oct 8, 2015
@OlegHahm
Copy link
Member

The last time I tried (about 1 month ago), I was not able to get the test application to send anything (not even the handshake packets).
As far as I can see, what would be the "eventloop" (as it is called in other gnrc modules) corresponds to the _fsm function here. However, this seems not to implement a common event loop for all TCP connections, but just for one connection. This would require every TCP connection to run its own thread which is most likely a bad idea. I would advice to use a central eventloop (just similar to what other gnrc modules do) and make all calls to TCP based on gnrc netapi calls.

Some more "formal" comments:

  • Defining such a long function as the current _fsm() static inside a header is IMO not a good style. Please move this to a C file.
  • The code could need some uncrustifying

@brummer-simon
Copy link
Member Author

Hi Oleg,

currently i'am doing buzy spliting the static includes into thier own c-Files, and doing some renaming. For your concern with the _fsm corresponance.

TCPs statemaschine must be able to do translations, without the users interaction, in case a FIN-Packet was received. Moving from ESTABLISHED to CLOSE_WAIT. The Eventloop interacts only in case of Packet reception with the FSM. The Transmission control blocks are organized as a linked list. If the eventloop receives a packet, it looks for the receivers pid. The pid is part of an tcb as well. The eventloop searches for the receivers tcb and calls the fsm-function with this specific tcb.

Long story short, only one eventloop is needed for multiple connections.

@brummer-simon brummer-simon force-pushed the devel-ng_tcp branch 2 times, most recently from cecfc13 to 61040e5 Compare November 25, 2015 13:16
@OlegHahm OlegHahm removed this from the Release 2015.12 milestone Nov 28, 2015
@OlegHahm OlegHahm modified the milestone: Release 2016.03 Dec 8, 2015
@brummer-simon
Copy link
Member Author

Merge fuckup lead to this pull request: #4744

@cgundogan
Copy link
Member

closed in favor of #4744

@cgundogan cgundogan closed this Feb 4, 2016
@brummer-simon brummer-simon deleted the devel-ng_tcp branch February 4, 2016 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@cgundogan cgundogan

Labels
Area: network Area: Networking State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Milestone
Release 2016.04
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@brummer-simon @OlegHahm @miri64 @kaspar030 @jnohlgard @Kijewski @DipSwitch @cgundogan