Packet size too big? - SimpleClient

[Dutrow]

I'm trying to get SimpleClient working on an Actiontec MI424 Verizon FIOS Router. Some issues have been found with this router regarding packet sizes being too big, but some code was posted to resolve the problem:
viewtopic.php?f=13&t=275&hilit=fios&start=20

The problem I am having is that most of the time, the board is never ever to make even one GETRequest, if it is able to make one, then it is never able to make another. However the feedback I am getting is confusing. I have "verbose" or "DEBUG" mode enabled in WiServer.cpp and each time I try to make a request, it reports that it gets a connection and then a few seconds later moments later (maybe 10-20 seconds) that the connection ended. Then after the connection is ended, "printData(char* data, int len) " in SimpleClient.pde prints an empty buffer. The connection is always ended due to a timeout, I was able to determine this by modifying the following code in "g2100.c":

Code: Select all

   if (uip_aborted() || uip_timedout() || uip_closed()) {
      if (req != NULL) {
         if (verbose) {
            Serial.print("\nEnded connection with ");
            Serial.print(req->hostName);
            Serial.print("\n  Reason: ");
            if( uip_aborted() )
               Serial.print("\n    aborted");
            if( uip_timedout() )
               Serial.print("\n    timedout");
            if( uip_closed() )
               Serial.print("\n    closed");
         }

         if (req->returnFunc) {
            // Call the sketch's callback function with 0 bytes to indicate End Of Data
            req->returnFunc((char*)uip_appdata, 0);
         }
         // Remove the request from the connection
         app->request = NULL;
         // Request is no longer active
         req->active = false;
      }
   }

The above mentioned fix to resolve the problem with the Actiontec MI424 Verizon FIOS Router sending packets through that are too big is to ignore the packet:
From "g2100.c"

Code: Select all

      case ZG_INTR_ST_RD_CTRL_REG:
      {
         U16 rx_byte_cnt = (0x0000 | (hdr[1] << 8) | hdr[2]) & 0x0fff;

         if ( rx_byte_cnt < (U16)UIP_BUFSIZE ){ // Check if our buffer is large enough for packet
            zg_buf[0] = ZG_CMD_RD_FIFO;
            spi_transfer(zg_buf, rx_byte_cnt + 1, 1); // copy ZG2100 buffer contents into zg_buf (uip_buf)
   
            hdr[0] = ZG_CMD_RD_FIFO_DONE;  // Tell ZG2100 we're done reading from it's buffer
            spi_transfer(hdr, 1, 1);
   
            intr_valid = 1;
   
            intr_state = 0;
            break;
         }{ // Too Big, ignore it and continue
                 intr_valid = 0;
                 intr_state = 0;
                 packet_too_big++;    // Log how many times this part of the code is ever reached
                 break;
            }
      }

I was able to determine that once a packet comes through that is too large and the part of the code is triggered that ignores the packet, this whole block of code is never called again. This seems to indicating that despite this "fix", the application is still failing.

I'm guessing that something more needs to be done about these oversized packets, perhaps they need to be caught sooner and/or ignored more gracefully?

Edit::::::::::::
I just logged into my neighbor's unsecured linksys router. I had basically the same problem there, except I had a much higher instance of successfully completing the GETRequest() once. But even with the linksys, after a packet comes in that is too big nothing works.

[Dutrow]

Ok, it looks like I just fixed this (I think posting the question may have helped organize my thoughts).

I took another look that the modified code in "g2100.c" and guessed that when the packet was being "ignored", some other stuff was being left out as well that caused the driver to hang, specifically, my guess is that one or more of these lines needs to exccute either way, which wasn't happening in the previous code:

Code: Select all

hdr[0] = ZG_CMD_RD_FIFO_DONE;  // Tell ZG2100 we're done reading from it's buffer
spi_transfer(hdr, 1, 1);
intr_valid = 1;


here is the complete modified version, this works with both my Actiontec Verizon Fios router as well as my neighbor's unsecured linksys router. I could see this causing some other problems because all I did was neglect to copy some code that I figured would overrun the buffer size, there is probably a better way to handle this, but its an improvement over what was happening before which was the the GETRequest() not working at all:

Code: Select all

      case ZG_INTR_ST_RD_CTRL_REG:
      {
         U16 rx_byte_cnt = (0x0000 | (hdr[1] << 8) | hdr[2]) & 0x0fff;

         zg_buf[0] = ZG_CMD_RD_FIFO;
         if ( rx_byte_cnt < (U16)UIP_BUFSIZE ) // Check if our buffer is large enough for packet
            spi_transfer(zg_buf, rx_byte_cnt + 1, 1); // copy ZG2100 buffer contents into zg_buf (uip_buf)
            
         hdr[0] = ZG_CMD_RD_FIFO_DONE;  // Tell ZG2100 we're done reading from it's buffer
         spi_transfer(hdr, 1, 1);
         intr_valid = 1;
         intr_state = 0;
         break;
      }

[GregEigsti]

Good stuff! Should be simple enough to repro and check out - when I get some time I repro'ed the problem a while back with a simple UDP app that blasted out large packets but have never tried this fix as I don't have the problem I will give it a try because it is part of the user contrib branch and I want to make sure that it is working as expected.

Greg

[GregEigsti]

Was playing with the user contrib stack tonight and ran into what appears to be the same (or related) hang; weird thing is that the latest AsyncLab's stack does not exhibit the hang (and does not have to too large of a packet fix). Only the original fix exhibits this problem. So I updated the user contrib stack to contain Dutrow's updated fix.

Thanks!
Greg

[spidermonkey04]

Good catch Dutrow! It seems like something still isn't quite right though.

Code: Select all

intr_valid = 1;


In doing this, we're telling the driver it needs to process an incoming packet, even though it was too big and should be ignored. So when it returns to zg_drv_process(), case DRV_STATE_PROCESS_RX: will be executed and then uIP will try to process that "incoming" packet.

I think you're right in that ZG_CMD_RD_FIFO_DONE needs to be executed either way.

---Jared

[spidermonkey04]

So I did a bit of testing and that was indeed a "bad" fix. Only small changes to what Dutrow said.

Code: Select all

case ZG_INTR_ST_RD_CTRL_REG:
      {
         U16 rx_byte_cnt = (0x0000 | (hdr[1] << 8) | hdr[2]) & 0x0fff;

            if ( rx_byte_cnt+1 < (U16)UIP_BUFSIZE ){ // Check if our buffer is large enough for packet
             zg_buf[0] = ZG_CMD_RD_FIFO;
             spi_transfer(zg_buf, rx_byte_cnt + 1, 1);  // Copy ZG2100 buffer contents into zg_buf (uip_buf)
             intr_valid = 1;  // interrupt from zg2100 was meaningful and requires further processing
            }else{ // Too Big, ignore it and continue
                 intr_valid = 0;  //
                 }
            hdr[0] = ZG_CMD_RD_FIFO_DONE;  // Tell ZG2100 we're done reading from its buffer
            spi_transfer(hdr, 1, 1);
           
            intr_state = 0; // Done reading interrupt from ZG2100
            break;
      }

I tested this with a simple ping command. With the "bad" fix, a 1400 byte ping packet would still cause a lockup. With this new fix a 1400 byte ping packet is ignored and the board keeps on truckin'. Maybe I should test things before posting them... Live and learn.

---Jared

[GregEigsti]

From a quick and tired perusal of spidermonkey04's last post I think his handling of intr_valid is correct; if the incoming data is within size limits intr_valid should be set to non-zero to allow further processing (indicating a potentially valid payload in zg_buf); otherwise it should be set to 0 to stop any further processing of the data (because it was too big, was not copied into zg_buf and therefore no further processing of zg_buf is in order).

However I think the following line should be changed from:

Code: Select all

if ( rx_byte_cnt+1 < (U16)UIP_BUFSIZE ){ // Check if our buffer is large enough for packet

To:

Code: Select all

if ( rx_byte_cnt < (U16)UIP_BUFSIZE ){ // Check if our buffer is large enough for packet

Because "rx_byte_cnt + 1" bytes will be copied into zg_buf. spidermonkey04's last change would allow two extra bytes to be safely copied into zg_buf but we only need room for the one additional byte. Make sense or am I just too tire for my own good?

So how about this...

Code: Select all

      case ZG_INTR_ST_RD_CTRL_REG:
      {
         U16 rx_byte_cnt = (0x0000 | (hdr[1] << 8) | hdr[2]) & 0x0fff;
         // Check if our buffer is large enough for packet
         if(rx_byte_cnt < (U16)UIP_BUFSIZE) {
            // copy ZG2100 buffer contents into zg_buf (uip_buf)
            zg_buf[0] = ZG_CMD_RD_FIFO;
            spi_transfer(zg_buf, rx_byte_cnt + 1, 1);
            intr_valid = 1;
         }
         else {
            // incoming data too large, ignore it and continue
            intr_valid = 0;
         }

         // Tell ZG2100 we're done reading from it's buffer
         hdr[0] = ZG_CMD_RD_FIFO_DONE;
         spi_transfer(hdr, 1, 1);
         intr_state = 0;
         break;
      }


Whatcha think? Of course I have not tested this out but wondered if we could come to some sort of consensus. BTW - thanks very much to spidermonkey04 and Dutrow for your time and effort on this!
Greg

[spidermonkey04]

Hows about we just make it exact and use "rx_byte_cnt < (U16)UIP_BUFSIZE+1" or "rx_byte_cnt <= (U16)UIP_BUFSIZE". I prefer the former, but only because I'm not familiar enough with AVR assembly to know which is better. In other ASM I've used, a "<=" would produce two machine instructions but a "<" was only one. The "+1" doesn't change things because UIP_BUFSIZE is a constant, so it's loaded as an immediate value and doesn't need any more machine instructions.

I think this could avoid some confusion in the future. Let's say someone NEEDS to accept an incoming 750 byte packet, so they set their UIP_BUFSIZE to 750. When their code gets to "if ( 750 < (U16)750){", the packet will NOT be accepted, and they will cry...

Besides, we already have a two byte "safety net" when uIP initializes the buffer.

Code: Select all

u8_t uip_buf[UIP_BUFSIZE + 2];   /* The packet buffer that contains incoming packets. */

Just my $.02

---Jared

[GregEigsti]

Good thoughts - thanks! I'll try to grok the implications later on tonight. As you said in a recent post - we (or I) really need to test the code and see what _really_ happens All of this has been theoretical so far. But I do appreciate your willingness to work on these issues and I do want to see the proper fix make it into the source.

Thanks! Now off to a meeting
Greg

[GregEigsti]

Ok, but the spi_transfer() copies "rx_byte_cnt + 1" bytes so we have to ensure that "rx_byte_cnt < (U16)UIP_BUFSIZE". Agreed? Wonder why "rx_byte_cnt + 1" bytes are copied???

Testing the following code now; works for a TCP app but want to try with a WiServer app which seems to be more susceptible to the crash.

Code: Select all

      case ZG_INTR_ST_RD_CTRL_REG:
      {
            // Get the size of the incoming packet
         U16 rx_byte_cnt = (0x0000 | (hdr[1] << 8) | hdr[2]) & 0x0fff;

         // Check if our buffer is large enough for packet
            if(rx_byte_cnt < (U16)UIP_BUFSIZE) {
            zg_buf[0] = ZG_CMD_RD_FIFO;
            // Copy ZG2100 buffer contents into zg_buf (uip_buf)             
            spi_transfer(zg_buf, rx_byte_cnt + 1, 1);
            // interrupt from zg2100 was meaningful and requires further processing
            intr_valid = 1;
         }
         else {
            // Too Big, ignore it and continue
            intr_valid = 0;
         }

         // Tell ZG2100 we're done reading from its buffer
         hdr[0] = ZG_CMD_RD_FIFO_DONE;
         spi_transfer(hdr, 1, 1);
           
         // Done reading interrupt from ZG2100
         intr_state = 0;
         break;
      }


Greg