Implement buffered RX for HardwareSerial #2237

Closed
YannikW opened this Issue Jul 6, 2016 · 17 comments

Comments

Projects
None yet
7 participants
@YannikW

YannikW commented Jul 6, 2016

Basic Infos

Hardware

Hardware: ESP-02 + Arduino Nano
Core Version: 2.3.0

Description

The ESP is working as a Serial<->TCP bridge. Code mostly like the Telnet example.

When sending data from the Arduino Nano over Serial to the ESP I'm losing data.
When I let the ESP Send the same String to the client, everyting comes to the client.

But receiving the same string over Serial and foreward it to the clients, sometimes bytes are missing, see following picture.
2016-07-06 07 52 49

It looks like a buffer overflow, because always the last bytes are missing.. But the whole message is < 256 Bytes.

I tried different baud rates from 57600 up to 500000, but the problem is always the same.

Sketch

Arduino Nano

uint16_t index = 0;

void setup() 
{
  Serial.begin(115200);
}

void loop() 
{
  Serial.println(index++);
  delay(5);
  Serial.println("#############################################");
  delay(5);
  Serial.println("#############################################");
  delay(5);
  Serial.println("#############################################");
  delay(5);
  Serial.println("#############################################");
  delay(5);
  Serial.println("#############################################");

  delay(5000);
}

ESP

#include <ESP8266WiFi.h>
#include <WiFiClient.h>

#define SERIALBAUD    115200  //Baud rate for communication
#define MAXCLIENTS         4

//Access Point data
const char* ssid = "OpenRF";
const char* password = "12345678";


//TCP Server on Port 333
WiFiServer server(333);
//Server clients
WiFiClient serverClients[MAXCLIENTS];

void setup()
{
  Serial.begin(SERIALBAUD);

  //Start Access Point
  WiFi.disconnect(true);
  WiFi.mode(WIFI_AP);
  WiFi.softAP(ssid, password);

  //Start Server
  server.begin();
  server.setNoDelay(true);

  //Initialization finished
  Serial.println();Serial.println();
  Serial.println("ESP ready");
  Serial.print("Server IP is : "); Serial.println(WiFi.softAPIP());
  Serial.println("Port is : 333");
}

void loop()
{
  uint8_t i;
  //check if there are any new clients
  if (server.hasClient())
  {
    for(i = 0; i < MAXCLIENTS; i++)
    {
      //find free/disconnected spot
      if (!serverClients[i] || !serverClients[i].connected())
      {
        if(serverClients[i]) 
          serverClients[i].stop();

        serverClients[i] = server.available();
        //Serial.print("New client: "); Serial.print(i);
        continue;
      }
    }
    //no free/disconnected spot so reject
    WiFiClient serverClient = server.available();
    serverClient.stop();
    //Serial.println("No client slot available");
  }

  //check clients for data
  for(i = 0; i < MAXCLIENTS; i++)
  {
    if (serverClients[i] && serverClients[i].connected())
    {
      if(serverClients[i].available())
      {
        //get data from the telnet client and push it to the UART
        while(serverClients[i].available()) 
          Serial.write(serverClients[i].read());
      }
    }
  }

  //check UART for data
  if(Serial.available())
  {
    size_t len = Serial.available();
    uint8_t sbuf[len];
    Serial.readBytes(sbuf, len);
    //push UART data to all connected telnet clients
    for(i = 0; i < MAXCLIENTS; i++)
    {
      if (serverClients[i] && serverClients[i].connected())
      {
        serverClients[i].write(sbuf, len);
        delay(1);
      }
    }
  }
}
@igrr

This comment has been minimized.

Show comment
Hide comment
@igrr

igrr Jul 6, 2016

Member

Can you check if Serial.available() returns 128? This would indicate UART FIFO overflow.

Member

igrr commented Jul 6, 2016

Can you check if Serial.available() returns 128? This would indicate UART FIFO overflow.

@YannikW

This comment has been minimized.

Show comment
Hide comment
@YannikW

YannikW Jul 6, 2016

Yeah, I check this. You're right. Always returning 128.
I read somewhere it is 256, but it seems this is wrong :D

Any chance to bump up the buffer to 256 bytes? Can't find any #define in HardwareSerial.h..

YannikW commented Jul 6, 2016

Yeah, I check this. You're right. Always returning 128.
I read somewhere it is 256, but it seems this is wrong :D

Any chance to bump up the buffer to 256 bytes? Can't find any #define in HardwareSerial.h..

@igrr

This comment has been minimized.

Show comment
Hide comment
@igrr

igrr Jul 6, 2016

Member

UART FIFO is in hardware, and it's 128 bytes. It's not possible to bump it using defines...

Member

igrr commented Jul 6, 2016

UART FIFO is in hardware, and it's 128 bytes. It's not possible to bump it using defines...

@me-no-dev

This comment has been minimized.

Show comment
Hide comment
@me-no-dev

me-no-dev Jul 6, 2016

Collaborator

I have done some thinking recently about the UART RX scheme we currently use, and I kinda lean towards returning ONLY the RX data interrupt routine and use larger buffer to store incoming uart data.
Reason is that with higher baudrates and busy network, you can often end up in RX overflow.

Collaborator

me-no-dev commented Jul 6, 2016

I have done some thinking recently about the UART RX scheme we currently use, and I kinda lean towards returning ONLY the RX data interrupt routine and use larger buffer to store incoming uart data.
Reason is that with higher baudrates and busy network, you can often end up in RX overflow.

@igrr

This comment has been minimized.

Show comment
Hide comment
@igrr

igrr Jul 6, 2016

Member

That's a good option. Another one is to implement serialEvent, which unlike AVR Arduino core we don't have.

Member

igrr commented Jul 6, 2016

That's a good option. Another one is to implement serialEvent, which unlike AVR Arduino core we don't have.

@YannikW

This comment has been minimized.

Show comment
Hide comment
@YannikW

YannikW Jul 6, 2016

Would be very cool to have a bigger buffer..

Any chance this will be implemented the next time? :D

YannikW commented Jul 6, 2016

Would be very cool to have a bigger buffer..

Any chance this will be implemented the next time? :D

@me-no-dev

This comment has been minimized.

Show comment
Hide comment
@me-no-dev

me-no-dev Jul 6, 2016

Collaborator

serialEvent runs from the loop, no? even if run in the system context, you can have enough time between calls to overflow the buffer. Only option I see is to use a buffer and fill it with the uart interrupt. I would opt out for all-c and in-ram isr to help with speed and not trigger exceptions

Collaborator

me-no-dev commented Jul 6, 2016

serialEvent runs from the loop, no? even if run in the system context, you can have enough time between calls to overflow the buffer. Only option I see is to use a buffer and fill it with the uart interrupt. I would opt out for all-c and in-ram isr to help with speed and not trigger exceptions

@me-no-dev

This comment has been minimized.

Show comment
Hide comment
@me-no-dev

me-no-dev Jul 6, 2016

Collaborator

you need less than 900 micro seconds to fill the 128 bytes at 115200 baud

Collaborator

me-no-dev commented Jul 6, 2016

you need less than 900 micro seconds to fill the 128 bytes at 115200 baud

@igrr

This comment has been minimized.

Show comment
Hide comment
@igrr

igrr Jul 6, 2016

Member

Sure, ISR has to go into RAM.
I'm fine with keeping the implementation in UART HAL, which is in C. This way HardwareSerial class will not need any changes, except passing the size of software buffer into uart_init function.

Member

igrr commented Jul 6, 2016

Sure, ISR has to go into RAM.
I'm fine with keeping the implementation in UART HAL, which is in C. This way HardwareSerial class will not need any changes, except passing the size of software buffer into uart_init function.

@me-no-dev

This comment has been minimized.

Show comment
Hide comment
@me-no-dev

me-no-dev Jul 6, 2016

Collaborator

I'll get on it. 1K default good?

Collaborator

me-no-dev commented Jul 6, 2016

I'll get on it. 1K default good?

@igrr

This comment has been minimized.

Show comment
Hide comment
@igrr

igrr Jul 6, 2016

Member

I would go with the old default of 256 bytes, and add HardwareSerial::setRxBufferSize method, which will basically call uart_init with new value of buffer size.

Member

igrr commented Jul 6, 2016

I would go with the old default of 256 bytes, and add HardwareSerial::setRxBufferSize method, which will basically call uart_init with new value of buffer size.

@igrr

This comment has been minimized.

Show comment
Hide comment
@igrr

igrr Jul 6, 2016

Member

Linking #1683, #2037 here.

Member

igrr commented Jul 6, 2016

Linking #1683, #2037 here.

@igrr igrr changed the title from Losing Serial data to Implement buffered RX for HardwareSerial Jul 6, 2016

@igrr igrr added this to the 2.4.0 milestone Jul 6, 2016

me-no-dev added a commit to me-no-dev/Arduino that referenced this issue Jul 6, 2016

igrr added a commit that referenced this issue Jul 8, 2016

@ffsi

This comment has been minimized.

Show comment
Hide comment
@ffsi

ffsi Jul 9, 2016

Playing with Arduino EasyTransfer (https://github.com/madsci1016/Arduino-EasyTransfer), I can not run in NodeMCU (Arduino to Arduino working fine). Maybe could be related?

ffsi commented Jul 9, 2016

Playing with Arduino EasyTransfer (https://github.com/madsci1016/Arduino-EasyTransfer), I can not run in NodeMCU (Arduino to Arduino working fine). Maybe could be related?

@me-no-dev

This comment has been minimized.

Show comment
Hide comment
@me-no-dev

me-no-dev Jul 11, 2016

Collaborator

What do you mean by "I can not run in NodeMCU"? You get resets or what is happening exactly?

Collaborator

me-no-dev commented Jul 11, 2016

What do you mean by "I can not run in NodeMCU"? You get resets or what is happening exactly?

@amardhore

This comment has been minimized.

Show comment
Hide comment
@amardhore

amardhore Nov 15, 2016

Great Job guys.. I was struggling with my serial to WiFi bridge.

  1. I was waiting for a serial data and then a open connection to a server.
  2. By the time I had a connection, I already had 128 bytes in the buffer and then I was loosing few bytes after that.

But with your new changes I was able to increase my buffer to 512 and 1024. It works so great. Thank you for you time and efforts. Really appreciate it.

-Amar

Great Job guys.. I was struggling with my serial to WiFi bridge.

  1. I was waiting for a serial data and then a open connection to a server.
  2. By the time I had a connection, I already had 128 bytes in the buffer and then I was loosing few bytes after that.

But with your new changes I was able to increase my buffer to 512 and 1024. It works so great. Thank you for you time and efforts. Really appreciate it.

-Amar

@themindfactory

This comment has been minimized.

Show comment
Hide comment
@themindfactory

themindfactory Jan 18, 2017

Contributor

I have 2.3.0 loaded and I can not use Serial.setRxBufferSize(1024); I thought it had been added in #2239 ?

Contributor

themindfactory commented Jan 18, 2017

I have 2.3.0 loaded and I can not use Serial.setRxBufferSize(1024); I thought it had been added in #2239 ?

@rndme

This comment has been minimized.

Show comment
Hide comment
@rndme

rndme Mar 13, 2017

2.3.0 came out after this fix. Is there anyway to release this yet? I'm in a complicated position where i can't run python, git, and ardunio on the same box, but could really use this "new" feature...

rndme commented Mar 13, 2017

2.3.0 came out after this fix. Is there anyway to release this yet? I'm in a complicated position where i can't run python, git, and ardunio on the same box, but could really use this "new" feature...

@igrr igrr closed this Jan 2, 2018

@igrr igrr removed the staged-for-release label Jan 2, 2018

@mareksugar mareksugar referenced this issue in bbqkees/Nefit-Buderus-EMS-bus-Arduino-Domoticz Mar 19, 2018

Closed

support for ESP8266 #7

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