JiriBilek / WiFiSpi

SPI library for Arduino AVR and STM32F1 to connect to ESP8266
GNU Lesser General Public License v3.0
62 stars 13 forks source link

Problem with 3 second delay when calling WiFiSpiServer::avaliable() #13

Closed m0rt3nlund closed 3 years ago

m0rt3nlund commented 5 years ago

Hi!

I am having a problem that creates a 3000ms delay when calling WiFiSpiServer::avaliable() in my STM32 run loop. Is this a known problem with the ESP? I have traced it all the way to ServerSpiDrv::getClientState and it is in the EspSpiDrv::waitResponseCmd the delay is created. I would guess the ESP does not respond before 3 seconds inside the function ServerSpiDrv::cmdGetClientStateTcp. This creates a 3 second delay every time my loop is ran so the application gets a "bit" slow :-P

Hope to here from you as besides this the library is getting really good! :-D

Thank you, Morten Lund

m0rt3nlund commented 5 years ago

Its waiting for this command to finish: waitForSlaveTxReady

And it is returning due to timeout since the timeout is 3000ms, same as my measured delay.

JiriBilek commented 5 years ago

Are you trying the newest WifiSpiESP commit? There was bad timing on the ESP side that could cause communication problems.

m0rt3nlund commented 5 years ago

I am using 0.1.4. I also have ESP Arduino platform 2.5.0

I just tried 2.5.0-beta 2 but that did not work at all :-)

I have enabled debugging in the arduino and the esp and there is no debug output after the initialization of the esp :-)

JiriBilek commented 5 years ago

Could you please create a MCVE sketch and post it here with the information about your master hardware?

m0rt3nlund commented 5 years ago

Yes! I will do that tonight!

JiriBilek commented 5 years ago

I'm using 2.5.0-dev from June 2018, not for any particular reason :)

m0rt3nlund commented 5 years ago

I am now using 2.5.0-beta1 I am using 0.1.4 with only one change WifiSpiCmdConnection:80 is changed to WiFi.mode(WIFI_AP_STA); to make it available as AP aswell as this is my primary use for it.

#include <WiFiSpi.h>

int wifiCSPin = PA4;
int wifiResetPin = PB12;
int wifiStatus = WL_IDLE_STATUS;

WiFiSpiClient clients[4];
WiFiSpiServer TCPserver(80);

void setup() {
  // put your setup code here, to run once:
  pinMode(wifiCSPin, OUTPUT);
  digitalWrite(wifiCSPin, LOW);

  pinMode(wifiResetPin, OUTPUT);
  digitalWrite(wifiResetPin, LOW);
  delay(500);
  digitalWrite(wifiResetPin, HIGH);
  delay(1000);

  WiFiSpi.init(wifiCSPin,5000000);

  delay(500);

  if (WiFiSpi.status() != WL_NO_SHIELD) {
    TCPserver.begin();
    if ( TCPserver.status() )
    {
      Serial.println(PSTR("Started server"));
    }

    wifiStatus = WiFiSpi.begin("MogH", "10042057");

    // wait 1 second for connection:
    delay(1000);

    if ( wifiStatus == WL_CONNECTED )
    {
      Serial.println(PSTR("Connected"));
    }
  }
}

long nextUpdate = 0;

void loop() {
  long startTime = millis();
  WiFiSpiClient client = TCPserver.available(); 

  long calcTime = millis() - startTime;
  if ( calcTime > 100 )
  {
    Serial.print("Used time: ");
    Serial.println(calcTime);
  }

  bool clientIsNew = true;

  for (byte i= 0 ; i < 4; i++) 
  { 
    if(client == clients[i]) 
    {
      clientIsNew = false;
    }
  }

  if ( clientIsNew )
  {
    for (byte i= 0 ; i < 4; i++) 
    { 
      if(clients[i].connected() == false ) 
      {
        Serial.print("New client! : ");
        Serial.println(i);
        client.write("<<hello;->>" + '\n');
        clients[i]=client;
        break;
      }
    }
  }

  if ( nextUpdate == 0 || millis() > nextUpdate )
  {
    for (byte i=0; i<4; i++)
    { 
      if (clients[i].connected()) 
      { 
        char Data[200];
        snprintf(Data, sizeof Data, "<<data;0:%u|1:%u|2:%u>>\n", 0,1,2);

        clients[i].write(Data);
      }
    }

    nextUpdate = millis() + 500;
  }
}
m0rt3nlund commented 5 years ago

I also appears it is not always just WiFiSpiServer::avaliable() but it is also WifiSpiClient::status, but if I am correct both these functions uses the same function: ServerSpiDrv::getClientState to retrive the client status?

It also easier to reproduce if there is no clients connected, right after boot.

m0rt3nlund commented 5 years ago

I updated the test code.

JiriBilek commented 5 years ago

Hi, some investigation made, some progress made as well but there are more issues. So far:

  1. The 3 sec. problem: the app calls TCPserver.available() too often. I put a 50 ms delay in the loop() and the problem is gone. It's strange and I'll deal with it further.

  2. I have to look at the sources why simple WiFiSpiClient client = TCPserver.available() didn't work for me. Finally, I ended with:

    uint8_t clientStatus;
    WiFiSpiClient client = TCPserver.available(&clientStatus); 
    
    if (clientStatus != CLOSED) {
    ....
    }
  3. As I tested with curl and it always sends some http data, I observed that reading input data (whatever bogus are they) is necessary:

    while (true) {
        int16_t intVal = client.read();
        if (intVal < 0)
            break;
    }
  4. For curl to be happy I needed to send client.write("HTTP/1.1 200 OK\r\n\r\n"); on a successful connection.

And now the bad news:

  1. I wasn't able to make more than one connection. If I tried to connect from two terminals, the second one was waiting until I killed the first one :(

  2. I was convinced the function TCPserver.available() returns a new connection on every call. It seems not to work so, I was receiving the first established connection on every call (and other connections were meantime on hold, see point 5 above).

The main loop I am testing is:

void loop() {
  long startTime = millis();
  delay(50);
  uint8_t clientStatus;
  WiFiSpiClient client = TCPserver.available(&clientStatus); 

  long calcTime = millis() - startTime;
  if ( calcTime > 100 )
  {
    Serial.print("Used time: ");
    Serial.println(calcTime);
  }

  if (clientStatus != CLOSED) {
    Serial.println("Request dump:");
    while (true) {
        int16_t intVal = client.read();
        if (intVal < 0)
            break;

        Serial.write(intVal);
    }
    Serial.println();

    bool clientIsNew = true;

    for (byte i= 0 ; i < 4; i++) 
    { 
      if(client == clients[i]) 
      {
        clientIsNew = false;
      }
    }

    if ( clientIsNew )
    {
      for (byte i= 0 ; i < 4; i++) 
      { 
        if(clients[i].connected() == false ) 
        {
          Serial.print("New client! : ");
          Serial.println(i);
          client.write("HTTP/1.1 200 OK\r\n\r\n");
          client.write("<<hello;->>\r\n");
          clients[i]=client;
          break;
        }
      }
    }
  }

  if ( nextUpdate == 0 || millis() > nextUpdate )
  {
    for (byte i=0; i<4; i++)
    { 
      if (clients[i].connected()) 
      { 
        char Data[200];
        snprintf(Data, sizeof Data, "<<data;%d:%lu>>\n", i, millis());

        clients[i].write(Data);
      }
    }

    nextUpdate = millis() + 500;
  }
}
m0rt3nlund commented 5 years ago

Hi! Thanks for looking into this! :)

I was looking at the mcu side of the library and see that the server->avaliable function calls the client->status function? Is this correct? I also see that on the esp side there is not really a server-> avaliable function? It uses server->avaliable in cmdGetClientStateTcp but not in a way that would return a new client?

JiriBilek commented 5 years ago

As I see, there is only one incoming connection per server allowed in this library. Thus my points 4 and 5 are an expected behaviour. The library maintains 4 sockets. They can be either client or server in the meaning of outgoing or ingoing traffic. For sending data you have to use client api, whatever it is called, because you need to send data from the master MCU to the internet and the client api does it. Maybe it's strange but I believe this is how the original Wifi library for Arduino works. Will look at it further.

JiriBilek commented 5 years ago

I made a simple ESP8266 sketch for testing how it is really with the TCP server. I can't get more than one connection at a time as well. Therefore I doubt I can add the multiclient feature to the WifiSPI library.

The source: https://gist.github.com/JiriBilek/9d2b6c1578ba7cf7a869e5577564dfe3

Edit: not correct, can make more connections. Edit2: the first revision of the gist file does not run for more than one client (is buggy - bad comparison of objects client), the second revision runs fine for 8 connections.

m0rt3nlund commented 5 years ago

Cool! I thought it was strange that the ESP8266 Wifi code did not support more than 1 client so its nice you figured it out! :-) But how can we make this available in your library? :-) I looked at the wifi code for the ESP and they use some clever flags to check if there is any new "unclaimed" clients avaliable in their Server:available function. Maybe that could work somehow? ESP8266 Wifi library

I also checked the Arduino Wifi library and it was the same as your implementation, as you said. But as stated by the Arduino guys the Arduino Wifi library actually does not support more than one client, but the ESP does :-)

I hope you/we can figure this out as the library is really getting great and I think this will be awesome for everyone that uses the ESP with Arduino! Great work!

JiriBilek commented 5 years ago

The _unclaimed variable in WiFiServer.cpp (ESP8266) serves only to create a linked list (FIFO) of accepted connections. Every call to WiFiServer::available takes the first one from the list. No help for us.

m0rt3nlund commented 5 years ago

But it takes the first unclaimed client? I did not mean to use the variable in this library, but the concept, or actually replace the current server:avaliable code to use the ESP Wifi Server:available function and not how they did in the Arduino wifi library :-) Is there any reason for the WifiSpiESP server code to not just use the available functionand return the same status as it does when running on the ESP? Like in your GIST? :-)

JiriBilek commented 5 years ago

It would require to rework the part of the library. There is a concept of _sock variables that are used to bind connections between master and slave app. The WiFiSPIServer class uses only one _sock position both for identification of server and for identification of client that is opened on incoming connection. Being only on ESP8266 there is no such problem as we can work with WiFiClient object directly.

Edit: There is a possibility the server will keep one _sock for its incoming communication and will create clients on another _socks. But obviously, it will break compatibility with the original Arduino library.

m0rt3nlund commented 5 years ago

Hmm, why would it break compatibility? As I have understood, the problem with the Arduino library only supporting one client connection at a time is actually a bug, and it was not the intention.

Let me know if there is anything I could do to help if you are interested in making these changes! :-)

JiriBilek commented 5 years ago

By compatibility I meant the binding between the master and slave. But maybe you're right - the binding (the _sock stuff) may be hidden from the user. I will have to think it over after refreshing my knowledge from the code.

JiriBilek commented 5 years ago

I am leaving this open because the 3 sec delay caused by communication error between master and slave is worth further investigation.

xelll commented 5 years ago

hi, i also have this problem. have somebody fix it?

JiriBilek commented 5 years ago

Are you trying the newest git version?

xelll commented 5 years ago

Are you trying the newest git version?

yes

JiriBilek commented 5 years ago

And could you post me a MCVE sketch?

xelll commented 5 years ago

And could you post me a MCVE sketch?

~I rewrote this library with C, the latest commit of the master, running the mqtt demo, and always calling valid data.~ i use MQTT_Publish example,and meet this problem

xelll commented 5 years ago

~https://github.com/esp8266/Arduino/blob/72c21feff6e53e273fde99862baf086137cc98e7/libraries/ESP8266WiFi/src/WiFiClient.cpp#L255-L266~

~I guess this optimistic_yield(100); let we can not pool fast than 100ms, now i sleep 100ms in loop, No problems encountered~

JiriBilek commented 5 years ago

I am sorry, I don't understand. Could you please explain the previous post to me?

JiriBilek commented 3 years ago

The issue is too old and no progress for a long time, closing.