Ethernet macros changes is it a bug?

[Stanimir-Petev]

Board

Olimex ESP32-POE-ISO

Device Description

LAN cable, USB

Hardware Configuration

LAN cable

Version

latest master (checkout manually)

IDE Name

Arduino

Operating System

Windows 10

Flash frequency

80

PSRAM enabled

no

Upload speed

921600

Description

Hello!

I am an Olimex employee and since the release of 3.0.0 version of the package we have quite a few reports from our customers that the ethernet is not working. In particular about the ESP32-POE and ESP32-POE-ISO boards but the issue I am about to talk about is more general than just a one board or another.

The question/issue report I have is about the ethernet examples and some changes on the "ETH.h" file when the transition from 2.0.14 to 3.0.0 of the package happened.

In "ETH.h" file of the older version of the package lines 31-53 there are a bunch of #ifndef - #define - #endif preprocessor directives for each of the following ethernet macros: ETH_PHY_ADDR, ETH_PHY_TYPE, ETH_PHY_POWER, ETH_PHY_MDC, ETH_PHY_MDIO, ETH_CLK_MODE.
While in the latest development version (at the moment of writing 3.0.0-alpha3) they are defined multiple times but all of the commented (thus not defined at all). Instead all these macro defines are shifted into the Arduino sketch examples right before the include of the "ETH.h".

So here is my confusion.

In the past there was a way to define a macro(s) inside the "pins_arduino.h" of a board to match the specific hardware and as a result the define(s) from ETH.h was ignored since the condition is #ifndef. While if the macro isn't defined then it takes some default value from the "ETH.h". And that worked great because this way when a client opens an example from the Arduino it is ready to go (as long as the defines are done correctly of course). Compile and upload.

With the latest changes however since the macros are defined inside the sketch itself, everything (about the ethernet) that is inside "pins_arduino.h" is completely overwritten by the values in the sketch. Without any warnings or errors even when the values are different (which was very surprising to me). So now when an example is open from the Arduino, the client have to either input the correct values of the macros or to remove the macros (with conflicting values) in case they are defined "pins_arduino.h".
I even tried with to redefine the values inside the "pins_arduino.h" using the sequence #ifdef - #undef - #endif - #define with the hope to bypass the values inside the sketch but it seems they are defined after those in the "pins_arduino.h" so like I mentioned earlier those from the sketch overwrites the ones in the header.

Obviously both ways will work if everything is setup properly. But from my point of view that makes it less convenient for the client who just wants to open the example, compile, upload and have a working program.
So what is the point of that change? Is there anything that I am missing? Something that makes the change necessary, something that improves the overall performance of the code or anything like that.
If there is a reason - well I guess this is how it goes in future. Otherwise if it's just an oversight is there a chance that to be reverted to what it was in 2.0.14 (unless it will break other things)?

I don't mean to disrespect anyone who has worked on that update. Just genuinely wondering why it was done this way.

Sketch

/*
    This sketch shows the Ethernet event usage

*/

// Important to be defined BEFORE including ETH.h for ETH.begin() to work.
// Example RMII LAN8720 (Olimex, etc.)
#define ETH_PHY_TYPE        ETH_PHY_LAN8720
#define ETH_PHY_ADDR         0
#define ETH_PHY_MDC         23
#define ETH_PHY_MDIO        18
#define ETH_PHY_POWER       -1
#define ETH_CLK_MODE        ETH_CLOCK_GPIO0_IN

#include <ETH.h>

static bool eth_connected = false;

// WARNING: WiFiEvent is called from a separate FreeRTOS task (thread)!
void WiFiEvent(WiFiEvent_t event)
{
  switch (event) {
    case ARDUINO_EVENT_ETH_START:
      Serial.println("ETH Started");
      // The hostname must be set after the interface is started, but needs
      // to be set before DHCP, so set it from the event handler thread.
      ETH.setHostname("esp32-ethernet");
      break;
    case ARDUINO_EVENT_ETH_CONNECTED:
      Serial.println("ETH Connected");
      break;
    case ARDUINO_EVENT_ETH_GOT_IP:
      Serial.println("ETH Got IP");
      ETH.printInfo(Serial);
      eth_connected = true;
      break;
    case ARDUINO_EVENT_ETH_LOST_IP:
      Serial.println("ETH Lost IP");
      eth_connected = false;
      break;
    case ARDUINO_EVENT_ETH_DISCONNECTED:
      Serial.println("ETH Disconnected");
      eth_connected = false;
      break;
    case ARDUINO_EVENT_ETH_STOP:
      Serial.println("ETH Stopped");
      eth_connected = false;
      break;
    default:
      break;
  }
}

void testClient(const char * host, uint16_t port)
{
  Serial.print("\nconnecting to ");
  Serial.println(host);

  WiFiClient client;
  if (!client.connect(host, port)) {
    Serial.println("connection failed");
    return;
  }
  client.printf("GET / HTTP/1.1\r\nHost: %s\r\n\r\n", host);
  while (client.connected() && !client.available());
  while (client.available()) {
    Serial.write(client.read());
  }

  Serial.println("closing connection\n");
  client.stop();
}

void setup()
{
  Serial.begin(115200);
  WiFi.onEvent(WiFiEvent);  // Will call WiFiEvent() from another thread.
  ETH.begin();
}

void loop()
{
  if (eth_connected) {
    testClient("google.com", 80);
  }
  delay(10000);
}

Debug Message

ets Jul 29 2019 12:21:46

rst:0x1 (POWERON_RESET),boot:0x1b (SPI_FAST_FLASH_BOOT)
configsip: 0, SPIWP:0xee
clk_drv:0x00,q_drv:0x00,d_drv:0x00,cs0_drv:0x00,hd_drv:0x00,wp_drv:0x00
mode:DIO, clock div:1
load:0x3fff0030,len:1416
load:0x40078000,len:14804
load:0x40080400,len:4
load:0x40080404,len:3356
entry 0x4008059c
E (1010) esp.emac: emac_esp32_init(369): reset timeout
E (1010) esp_eth: esp_eth_driver_install(228): init mac failed

Other Steps to Reproduce

No response

I have checked existing issues, online documentation and the Troubleshooting Guide

• I confirm I have checked existing issues, online documentation and Troubleshooting guide.

[me-no-dev]

would checking in the sketch if values are already define help?

#include "pins_arduino.h"
#ifndef ETH_PHY_TYPE
#define ETH_PHY_TYPE        ETH_PHY_LAN8720
#define ETH_PHY_ADDR         0
#define ETH_PHY_MDC         23
#define ETH_PHY_MDIO        18
#define ETH_PHY_POWER       -1
#define ETH_CLK_MODE        ETH_CLOCK_GPIO0_IN
#endif

[me-no-dev]

the main problem with pre-defined defaults is that we now support both RMII and SPI Ethernet and in order to know which type is available, we can not have defaults. ETH.begin() now works regardless if you have SPI or RMII, but I agree that we could add a check and not overwrite pins_arduino.h

[DanKoloff]

Yes, that might be a solution. Because right now we have to force customers to edit the demo code, and it is pointless to have Ethernet defined in pins_arduino.h - doing so still requires changing the demo code.

[P-R-O-C-H-Y]

@me-no-dev Should I post a PR with your proposed fix?

[me-no-dev]

@P-R-O-C-H-Y please go ahead. #include "pins_arduino.h" should not be necessary.

[TD-er]

Yep checking for defines before (re)defining them is always a good thing.
IMHO the pin defines should be set in some board definition and only be defined in example code when not yet defined.

And for the Olimex employees, it would be great if you also would have boards with SPI Ethernet as it allows for more free usable GPIO pins. :)

[Jason2866]

Since the issue is about Ethernet changes in Core 3.0.0. The are working.
Tasmota built with latest Core 3.0.0 does work with SPI Ethernet :-)

+1 to see Olimex Ethernet boards with SPI PHY.