espressif / esp-lwip

Fork of lwIP (https://savannah.nongnu.org/projects/lwip/) with ESP-IDF specific patches
Other
79 stars 126 forks source link

Allow for slip input of longer than 256 bytes. (IDFGH-4019) #23

Closed webworxshop closed 3 years ago

webworxshop commented 3 years ago

Allows SLIP interface to accept input of longer than 256 bytes in one go. Relevant to https://github.com/espressif/esp-idf/pull/4985, which will perform UART reads with a user defined (32-bit buffer length) and then attempt to write them into slipif_received_bytes.

Alvin1Zhang commented 3 years ago

Thanks for your contribution.

david-cermak commented 3 years ago

Hi @webworxshop

Thanks for posting this fix, it makes perfect sense. The only problems that I see are:

With that said I think we could elaborate on possible options to fix this otherwise, e.g. calling the slipif_received_bytes() in batches, since the API says it can work max 255 bytes, so we're in fact violating this definition in esp_netif_lwip_slip module.

A quick idea to fix this in IDF is:

void esp_netif_lwip_slip_input(void *h, void *buffer, unsigned int len, void *eb)
{
 ...
  // Update slip netif with data
  const int max_batch = 255;
  while (len > 0) {
    int batch = len > max_batch?max_batch:len;
    slipif_received_bytes(netif->lwip_netif, buffer, batch);
    len -= batch;
  }
 ...

 }
webworxshop commented 3 years ago

Hi @david-cermak,

Thanks for your response, I understand the requirement to minimise changes to lwip and am happy to go for an alternate fix.

However, the code sample you provided seems incorrect to me, for two reasons:

I've submitted an alternate fix at https://github.com/espressif/esp-idf/pull/5928, which resolves these issues and works in my testing.

david-cermak commented 3 years ago

@webworxshop Of course you are correct, thanks for fixing this and posing the IDF PR! I've only meant to sketch up the idea.

Thanks again, closing this in favour of the https://github.com/espressif/esp-idf/pull/5928