d-a-v / W5500lwIP

W5100, W5500 and ENC28J60 for esp8266 and lwIP (or any other uC using lwIP)
43 stars 11 forks source link

Can't get library really working #6

Open emelianov opened 5 years ago

emelianov commented 5 years ago

Probably i missing something but was able to get code below running only by patching esp8266\libraries\ESP8266WiFi\src\include\ClientContext.h (i know, delay doesn't implement early exit but it's just test)

diff --git a/libraries/ESP8266WiFi/src/include/ClientContext.h b/libraries/ESP8266WiFi/src/include/ClientContext.h
index 2f28915..55c4651 100644
--- a/libraries/ESP8266WiFi/src/include/ClientContext.h
+++ b/libraries/ESP8266WiFi/src/include/ClientContext.h
@@ -20,6 +20,7 @@
  */
 #ifndef CLIENTCONTEXT_H
 #define CLIENTCONTEXT_H
+#include <Schedule.h>

 class ClientContext;
 class WiFiClient;
@@ -131,7 +132,11 @@ public:
         _connect_pending = 1;
         _op_start_time = millis();
         // This delay will be interrupted by esp_schedule in the connect callback
-        delay(_timeout_ms);
+       //delay(_timeout_ms);
+       for (uint16_t i = 0; i < _timeout_ms; i++) {
+               delay(1);
+               run_scheduled_functions();
+       }
         _connect_pending = 0;
         if (!_pcb) {
             DEBUGV(":cabrt\r\n");

That case connect() is success. Don't perform more detailed checks.

#include <SPI.h>
#include <ESP8266WiFi.h>

#include <W5500lwIP.h>
#include <Schedule.h>

Wiznet5500lwIP eth(SPI, D4);
byte mac[] = {0x00, 0xAA, 0xBB, 0xCC, 0xDE, 0x02};

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

  SPI.begin();
  SPI.setBitOrder(MSBFIRST);
  SPI.setDataMode(SPI_MODE0);
  SPI.setFrequency(40000000);
  int present = eth.begin(mac);
  Serial.println("present= " + String(present, HEX));

  WiFi.mode(WIFI_OFF);

  while (!eth.connected()) {
    Serial.print(".");
    delay(1000);
    run_scheduled_functions();
  }
  Serial.println(eth.localIP());
  eth.setDefault(); // use ethernet for default route

  WiFiClient* client;
  client = new WiFiClient();
  Serial.println(client->connect(IPAddress(192, 168, 30, 133), 80));
}

void loop ()
{
    yield();
}

So it looks like that's enough to make run_scheduled_functions() called from yield()/delay() is that possible?

d-a-v commented 5 years ago

So it looks like that's enough to make run_scheduled_functions() called from yield()/delay() is that possible?

This is the purpose of the 1-2 months old patches in the core. delay() and yield() both call esp_yield() which calls run_scheduled_functions()

With your patch you call it more often (2*timeout_ms) times instead of once. This particular line you changed has changed on May 19 and I haven't been working on ethernet since then. https://github.com/esp8266/Arduino/commit/912c0db0910752d029d329ad20cbff8316a1db08#diff-3f3ed90c696a8411853b28574edb12d8 So it appears this change has broken ethernet.

Can you check the old fashion, with esp_yield() instead of delay() without your loop ?

Thanks for debugging this !

d-a-v commented 5 years ago

Sorry I was not talking about the same delay() but they are very similar (in the same file, the other delay() recently updated). You don't have to try anything (except trying to not call run_scheduled_functions()) and thanks again for debugging this issue.

Your fix is good and I think also is worth being applied to the other function.

To make it not a test, just add a test on _connect_pending:

       for (decltype(_timeout_ms) i = 0; _connect_pending && i < _timeout_ms; i++) {
               // let a chance to recurrent scheduled functions (ethernet)
               delay(1);
       }

Similarly on the other delay() with the same comment (the one I'm talking about above) should become:

       for (decltype(_timeout_ms) i = 0; _send_waiting && i < _timeout_ms; i++) {
               // let a chance to recurrent scheduled functions (ethernet)
               delay(1);
       }

It those changes work for you, would you propose a PR on the arduino core with them and a link to this issue ?

emelianov commented 5 years ago

I've looked dipper to Arduino core code and it looks like it's better to refactor delay(). Main idea is to call esp_yield() periodically during delay not only once. I'll try to create PR after comprehensive testing. In combination with #7 it should make your library working with no WiFi* classes modification.

Nevertheless above mentioned

for (decltype(_timeout_ms) i = 0; _connect_pending && i < _timeout_ms; i++) {
               // let a chance to recurrent scheduled functions (ethernet)
               delay(1);
       }

modifications are also required. I'll create separate PR for them too.

emelianov commented 5 years ago

Just built rather complex project (client/server both) with ethernet (WiFi off) -- it's working. No issue except connect delay and wdt reset on first start (coldstart?). Thanks for your great work on ethernet implementation!

d-a-v commented 5 years ago

except connect delay and wdt reset on first start (coldstart?).

This wdreset should be identified.

Thanks for your great work on ethernet implementation!

As you probably saw it, it's rather simple and only connecting lwIP with a new link layer. The main issue is the need for proper transparent polling, which you are also currently addressing.

What's left is

And afterward

emelianov commented 5 years ago

except connect delay and wdt reset on first start (coldstart?).

This wdreset should be identified.

Steps to reproduce WDT reset:

  1. Connect to Wi-Fi
  2. Disconnect from Wi-Fi (WiFi.mode(WIFI_OFF) )
  3. Connect to Ehernet
  4. Initialize mDNS. At step 4 got wdtreset. As firmware restores previousWi-Fi connection state on boot getting the same error on first boot with WiFi.mode(WIFI_OFF) (without explicit Wi-Fi connection from code). It's not looks like common situation, so can be left as is.

Hope to have some spare time to play with W5500 interrupt usage next days. But i'm not sure it can be really helpful.

emelianov commented 5 years ago

David, By the way is the any particular reason to not implement handlePacket() using os_timer API (os_timer_arm) ?

d-a-v commented 5 years ago

I will try to reproduce ASAP (currently finishing to update recurrent scheduled functions in the arduno core).

By the way is the any particular reason to not implement handlePacket() using os_timer API (os_timer_arm) ?

Yes because minimum timing resolution is 1ms and I don't know whether performances need lower value yet. Yes because I try to stay out from SYS stack (Ticker can use os_timer_arm and use scheduled functions. But in that case every timer create a new scheduled function, and they re executed only on loop()). Yes because I know that current lwIP configuration is not protected against concurrency.

#define SYS_ARCH_DECL_PROTECT(x)
#define SYS_ARCH_PROTECT(x)
#define SYS_ARCH_UNPROTECT(x)

Once it works reliably in CONT stack (= not in ISR) we can try to make it work from interrupts (timer or external int) without recurrent/scheduled functions (= in ISR).