Regarding DS<->DS TCP slowness (and DSi XL hang)

[sgstair]

Well, as you alluded to earlier, UDP is your best option, as it doesn't have any of this mess
and it's not -that- hard to synchronize and make your own datastream reliable.

I have a good number of ideas for how I want the new implementation to work, the problem is mainly that the existing one was written with a bad set of core ideas, and has pretty much been hacked into working correctly since then... Have learned quite a lot since then. So, I am pretty reluctant to go back and rediscover all the hidden issues and try to modify it further at this point, as I do intend to direct some effort to rewriting it in the (hopefully near) future.

-Stephen

[bengtj]

Yeah, I know the feeling. And I didn't really expect you to be too happy about me mucking around in there trying to get you to fix things instead of working on the next version

But if I submit a patch with my changes will you consider checking if it's worth applying? I think that with my small changes things will work a lot better, at least for the DS<->DS communication case. And I'm trying to not change too much... (I'm considering to not switch to UDP.)

Have you considered using uIP/iwIP for the rewrite? The latter has some bsd-like sockets. But maybe the license isn't good enough for the lib? (It requires a text in the documentation of a binary redistribution.)
http://savannah.nongnu.org/projects/lwip/

Regards,
Bengt

[sgstair]

Will consider a patch yes, though messing with those paths could break something and it's not like there's an easy way to test and be sure :)

I would like to rewrite the lib, with the point of essentially rewriting it, not just getting it to work. Oddly enough when I started all of this I tried to get lwIP working - I still don't know exactly what was wrong but at the time nothing really worked ;) I may have made some really stupid error at that point. Nowadays though, I know what I'm doing enough that I would be able to make a new version pretty awesome without that much trouble. And it is a problem space I am pretty interested in building solutions for.

The "interface" by which sgIP connects to the ds wifilib is actually intentionally really trivial to break. There's a single point which is called for received packets, and a single function which transmits a packet intended for an ethernet interface. Then also the timer call which is set up for sgIP stuff by default.
I think that it's a little bit nontrivial to port lwIP to work well on this platform though, notably due to the absence of unix-like timers and a lot of other stuff. Synchronization is also an interesting issue, the DS architecture (and dealing with the flexibility of homebrew) presents unique challenges, like making sockets work asynchronously in a non-threaded environment.
...Otherwise some enterprising sort would have already kluged it in, as it's not like I have been deterring this.

-Stephen

[bengtj]

Ok, things are looking very good now. I want to thank you for dswifi, after my small changes it works very good for me. Now will I have to iron out the bugs in the networking code of my game...

One problem I had at the end was that I included the "sgIP_errno.h" file instead of "errno.h" and error-checking did not work at all. It defines an error variable that wasn't in use in this environment. Would it be possible to remove the file "sgIP_errno.h" from the distribution to avoid this confusion?

A summary of my changes:
1. Stopped replying to every packet causing packet flooding in DS<->DS communication. Now replying to every packet with data payload instead to ack directly. This is actually quite nice for games.
2. Stopped replying to every packed that acked sent data. Now replying if data was acked and there is data to send.
3. Changed timer from 50ms to 25ms to make SGIP_TCP_TRANSMIT_DELAY=25 work more as intended. As we send the same data until we get an ack for this data point 2&3 should hopefully increase send performance. My theory: A packet sent every 25ms means timer driven 40 pkt/s and if the receiving side acks every other packet point 2 will double this to 80pkt/s.
(I will pad my message to 40 bytes or more to send it directly. This means that the timer does not need to be faster for my case.)
4. Added test in send() for closed connection, sets errno=EPIPE. To be able to detect broken connections without calling recv().

My next post will be my patch. Do you want me to submit it somewhere else?

Thank you,
Bengt

[bengtj]

Code: Select all

Index: arm9/source/sgIP_TCP.c
===================================================================
--- arm9/source/sgIP_TCP.c	(revision 4063)
+++ arm9/source/sgIP_TCP.c	(working copy)
@@ -316,6 +316,8 @@
    tcpseq=htonl(tcp->seqnum);
    datalen=mb->totallength-(tcp->dataofs_>>4)*4;
    shouldReply=0;
+   if (datalen>0) // Ack all incoming packets containing payload data, piggybacking data if available.
+	  shouldReply=1;
    if(tcp->tcpflags&SGIP_TCP_FLAG_RST) { // verify if rst is legit, and act on it.
       // check seq against receive window
       delta1=(int)(tcpseq-rec->ack);
@@ -344,7 +346,8 @@
 	  delta2+=rec->buf_tx_in;
 	  if(delta2>=SGIP_TCP_TRANSMITBUFFERLENGTH) delta2-=SGIP_TCP_TRANSMITBUFFERLENGTH;
 	  rec->buf_tx_in=delta2;
-      if(delta1>0) shouldReply=1;
+      if(delta1>0 && rec->buf_tx_out!=rec->buf_tx_in)
+    	  shouldReply=1; // Send new packet when other side acked sent data. If data is available to send.
    }
    rec->txwindow=rec->sequence+htons(tcp->window);
 
@@ -386,7 +389,6 @@
 				}
 				// copy data into the fifo
 				rec->ack+=datalen;
-				delta1=datalen;
 				while(datalen>0) { // don't actually need to check the rx buffer length, if the ack check approved it, it will be in range (not overflow) by default
 					delta2=SGIP_TCP_RECEIVEBUFFERLENGTH-rec->buf_rx_out; // number of bytes til the end of the buffer
 					if(datalen<delta2) delta2=datalen;
@@ -397,7 +399,7 @@
 					if(rec->buf_rx_out>=SGIP_TCP_RECEIVEBUFFERLENGTH) rec->buf_rx_out-=SGIP_TCP_RECEIVEBUFFERLENGTH;
 				}
 				if(rec->tcpstate==SGIP_TCP_STATE_FIN_WAIT_1 || rec->tcpstate==SGIP_TCP_STATE_FIN_WAIT_2) break;
-				if(shouldReply || delta1>=0) { // send a packet in reply, ha!
+				if(shouldReply) { // send a packet in reply, ha!
 					delta1=rec->buf_tx_out-rec->buf_tx_in;
 					if(delta1<0) delta1+=SGIP_TCP_TRANSMITBUFFERLENGTH;
 					delta2=(int)(rec->txwindow-rec->sequence);
@@ -841,6 +843,7 @@
 int sgIP_TCP_Send(sgIP_Record_TCP * rec, const char * datatosend, int datalength, int flags) {
 	if(!rec || !datatosend) return SGIP_ERROR(EINVAL);
 	if(rec->want_shutdown) return SGIP_ERROR(ESHUTDOWN);
+	if(rec->tcpstate>SGIP_TCP_STATE_ESTABLISHED) return SGIP_ERROR(EPIPE); // Connection closed?
 	SGIP_INTR_PROTECT();
 	int bufsize;
 	bufsize=rec->buf_tx_out-rec->buf_tx_in;
Index: arm9/source/wifi_arm9.c
===================================================================
--- arm9/source/wifi_arm9.c	(revision 4063)
+++ arm9/source/wifi_arm9.c	(working copy)
@@ -1002,9 +1002,9 @@
 
 // wifi timer function, to update internals of sgIP
 //---------------------------------------------------------------------------------
-void Timer_50ms(void) {
+void Timer_25ms(void) {
 //---------------------------------------------------------------------------------
-	Wifi_Timer(50);
+	Wifi_Timer(25);
 }
 
 // notification function to send fifo message to arm7
@@ -1036,13 +1036,13 @@
 
 	if(!wifi_pass) return false;
 
-	irqSet(IRQ_TIMER3, Timer_50ms); // setup timer IRQ
+	irqSet(IRQ_TIMER3, Timer_25ms); // setup timer IRQ
 	irqEnable(IRQ_TIMER3);
 
 	Wifi_SetSyncHandler(arm9_synctoarm7); // tell wifi lib to use our handler to notify arm7
 
 	// set timer3
-	TIMER3_DATA = -6553; // 6553.1 * 256 cycles = ~50ms;
+	TIMER3_DATA = -3276; // 3276 * 256 cycles = ~25ms;
 	TIMER3_CR = 0x00C2; // enable, irq, 1/256 clock
 
 	fifoSendAddress(FIFO_DSWIFI, (void *)wifi_pass);

[bengtj]

Hmmm, I now see that setting the timer to 25ms will not work as thought. The time difference is compared using ">" and not ">=" which means that timer packet sending will only be triggered every other timer interrupt. So with my patch timer based packets will still be sent every 50ms.

However, as the time measured is reset every time a packet is received or sent the change will do some good. Previously a packet would be sent on every timer interrupt which could happen any time between 0 or 50ms after a sent/received packet. If this happens too close to when a packet was sent the same data would be sent again. Now it will send a timer packet between 25ms and 50ms after last send/receive action. This improves the chance that this packet contains useful data I think.

Regards,
Bengt

[PypeBros]

One problem I had at the end was that I included the "sgIP_errno.h" file instead of "errno.h" and error-checking did not work at all. It defines an error variable that wasn't in use in this environment. Would it be possible to remove the file "sgIP_errno.h" from the distribution to avoid this confusion?

Seconding the request. Having sgIP_errno.h still present but unused by dswifi itself makes porting of old code buggy as there will be a mismatch between error codes and checked variables.