Skip to content
This repository was archived by the owner on Sep 6, 2023. It is now read-only.

Wired ethernet implementation #187

Closed
wants to merge 1 commit into from
Closed

Wired ethernet implementation #187

wants to merge 1 commit into from

Conversation

MrSurly
Copy link
Contributor

@MrSurly MrSurly commented Sep 27, 2017

Per #172

@MrSurly MrSurly mentioned this pull request Sep 27, 2017
@nickzoic
Copy link
Collaborator

nickzoic commented Oct 3, 2017

(still waiting on hardware, sorry)

@MrSurly
Copy link
Contributor Author

MrSurly commented Oct 3, 2017

No worries -- you know you'll likely have to modify your hardware, right? I used this guide

@nickzoic
Copy link
Collaborator

nickzoic commented Oct 3, 2017

Yep! Doesn't look too painful, and I've got the resistors on hand. In the article linked I love the combination of snotty perfboard and smd resistors, took me a while to notice them there!

@MrSurly
Copy link
Contributor Author

MrSurly commented Oct 3, 2017

I didn't bother with the surface mount, even though I have them on hand. This is ugly, but it works. However, I was more commenting on the wire that needed to be added to the 8720 board from the "NC" pin to the oscillator enable.

@nickzoic
Copy link
Collaborator

nickzoic commented Oct 6, 2017

My hardware arrived ("Wavelan LAN8720 Eth Board", it says) and I quickly wired it together.
I ended up soldering the pullup/pulldown resistors to the eth board and just connecting it to a DevKitC with wire jumpers. Much to my surprise, it worked first time!

I have some more testing to do, but in general this looks very good!

Quick comments, I'll look at this more closely over the weekend:

  • We probably shouldn't initialize the WiFi when we import network any more, instead wait until creating a network.WLAN() object.
  • Arguably LAN() should initially be inactive like the WLAN() objects are. It automatically DHCPs if it doesn't have an address already.
  • It should have a config() method even if that doesn't do anything as per AbstractNIC.
  • We should work out a .status() as per docs/library/network: Enhance AbstractNIC.status to take an optional argument. micropython#3351

@dpgeorge
Copy link
Member

dpgeorge commented Oct 6, 2017

This is a great addition! Just one comment at this point: it might be worth putting the LAN code in a separate file named network_lan.c (following classes like machine_pin.c) so the modnetwork.c file doesn't get overloaded (and then would make sense to eventually put the WLAN class code in network_wlan.c).

@nickzoic
Copy link
Collaborator

nickzoic commented Oct 6, 2017 via email

@MrSurly
Copy link
Contributor Author

MrSurly commented Oct 9, 2017

We probably shouldn't initialize the WiFi when we import network any more, instead wait until creating a network.WLAN() object.

I'm going to commit that change to this PR right now, since I'm doing a power-sensitive project, and just importing network means current consumption goes from ~35mA to ~115mA, even without connecting WiFi.

@MrSurly
Copy link
Contributor Author

MrSurly commented Oct 10, 2017

@nickzoic

This goes into an un-ending loop ("resetting mac" or some such) if you don't have a PHY attached. I think it's in the IDF, but noting here so someone remembers.

@mhanuel26
Copy link

Just want to know if this initiative of wired ethernet with native Phy is only for esp32 or if there is similar development for stm32?

Any comment will be appreciated!

@MrSurly MrSurly mentioned this pull request Oct 15, 2017
28 tasks
@MrSurly
Copy link
Contributor Author

MrSurly commented Oct 16, 2017

@mhanuel26 I don't believe there is any work in that area, but I'm sure a PR would be welcome? Which board/chip are you thinking of?

@MrSurly
Copy link
Contributor Author

MrSurly commented Oct 16, 2017

... putting the LAN code in a separate file named network_lan.c (following classes like machine_pin.c) so the modnetwork.c file doesn't get overloaded (and then would make sense to eventually put the WLAN class code in network_wlan.c).

@dpgeorge Is that something you want done before merging?

@mhanuel26
Copy link

@MrSurly I have STM32F769 board, just want to make sure I am not reinventing the wheel. I am not very familiar with esp32 but I guess a good option is to follow similar convention, would you mind to mention which files are involve and what are suppose to contain in a broad sense so I can start digging there and mimic that for the stm32 as much as possible?

@MrSurly
Copy link
Contributor Author

MrSurly commented Oct 16, 2017

@mhanuel26

I tried to mimic the functionality of network.WLAN() as closely as possible, only making changes as necessary for the things that were different. The actual changes are below; most of it is ESP32 specific, but the bits that create the new network.LAN() object would be similar. Most of the work would be in researching how to get the wired ethernet working on the STM32 platforms.

Since not every STM32 variant has a MAC, you might have to make this a config option. But then again, I don't know enough about the STM32 chips to be certain about this.

diff --git a/ports/esp32/Makefile b/ports/esp32/Makefile
index 2eae802..c3e2636 100644
--- a/ports/esp32/Makefile
+++ b/ports/esp32/Makefile
@@ -264,6 +264,9 @@ ESPIDF_CXX_O = $(addprefix $(ESPCOMP)/cxx/,\
 ESPIDF_ETHERNET_O = $(addprefix $(ESPCOMP)/ethernet/,\
 	emac_dev.o \
 	emac_main.o \
+	eth_phy/phy_tlk110.o \
+	eth_phy/phy_lan8720.o \
+	eth_phy/phy_common.o \
 	)
 
 $(BUILD)/$(ESPCOMP)/expat/%.o: CFLAGS += -Wno-unused-function
diff --git a/ports/esp32/modnetwork.c b/ports/esp32/modnetwork.c
index 59af7ae..0699db5 100644
--- a/ports/esp32/modnetwork.c
+++ b/ports/esp32/modnetwork.c
@@ -47,6 +47,9 @@
 #include "esp_log.h"
 #include "lwip/dns.h"
 #include "tcpip_adapter.h"
+#include "eth_phy/phy.h"
+#include "eth_phy/phy_tlk110.h"
+#include "eth_phy/phy_lan8720.h"
 
 #define MODNETWORK_INCLUDE_CONSTANTS (1)
 
@@ -98,15 +101,34 @@ static inline void esp_exceptions(esp_err_t e) {
 
 #define ESP_EXCEPTIONS(x) do { esp_exceptions(x); } while (0);
 
+enum { PHY_LAN8720, PHY_TLK110 };
+
 typedef struct _wlan_if_obj_t {
     mp_obj_base_t base;
     int if_id;
 } wlan_if_obj_t;
 
+typedef struct _lan_if_obj_t {
+    mp_obj_base_t               base;
+    int                         if_id; // MUST BE FIRST
+    bool                        initialized;
+    bool                        active;
+    uint8_t                     mdc_pin;
+    uint8_t                     mdio_pin;
+    int8_t                      phy_power_pin;
+    uint8_t                     phy_addr;
+    uint8_t                     phy_type;
+    eth_phy_check_link_func     link_func;
+    eth_phy_power_enable_func   power_func;
+} lan_if_obj_t;
+
 const mp_obj_type_t wlan_if_type;
 STATIC const wlan_if_obj_t wlan_sta_obj = {{&wlan_if_type}, WIFI_IF_STA};
 STATIC const wlan_if_obj_t wlan_ap_obj = {{&wlan_if_type}, WIFI_IF_AP};
 
+const mp_obj_type_t lan_if_type;
+STATIC lan_if_obj_t lan_obj = {{&lan_if_type}, ESP_IF_ETH, false, false};
+
 //static wifi_config_t wifi_ap_config = {{{0}}};
 static wifi_config_t wifi_sta_config = {{{0}}};
 
@@ -122,7 +144,7 @@ static esp_err_t event_handler(void *ctx, system_event_t *event) {
         ESP_LOGI("wifi", "STA_START");
         break;
     case SYSTEM_EVENT_STA_GOT_IP:
-        ESP_LOGI("wifi", "GOT_IP");
+        ESP_LOGI("network", "GOT_IP");
         break;
     case SYSTEM_EVENT_STA_DISCONNECTED: {
         // This is a workaround as ESP32 WiFi libs don't currently
@@ -161,7 +183,7 @@ static esp_err_t event_handler(void *ctx, system_event_t *event) {
         break;
     }
     default:
-        ESP_LOGI("wifi", "event %d", event->event_id);
+        ESP_LOGI("network", "event %d", event->event_id);
         break;
     }
     return ESP_OK;
@@ -182,6 +204,20 @@ STATIC void require_if(mp_obj_t wlan_if, int if_no) {
 }
 
 STATIC mp_obj_t get_wlan(size_t n_args, const mp_obj_t *args) {
+    static int initialized = 0;
+    if (!initialized) {
+        ESP_LOGD("modnetwork", "esp_event_loop_init done");
+        wifi_init_config_t cfg = WIFI_INIT_CONFIG_DEFAULT();
+        ESP_LOGD("modnetwork", "Initializing WiFi");
+        ESP_EXCEPTIONS( esp_wifi_init(&cfg) );
+        ESP_EXCEPTIONS( esp_wifi_set_storage(WIFI_STORAGE_RAM) );
+        ESP_LOGD("modnetwork", "Initialized");
+        ESP_EXCEPTIONS( esp_wifi_set_mode(0) );
+        ESP_EXCEPTIONS( esp_wifi_start() );
+        ESP_LOGD("modnetwork", "Started");
+        initialized = 1;
+    }
+
     int idx = (n_args > 0) ? mp_obj_get_int(args[0]) : WIFI_IF_STA;
     if (idx == WIFI_IF_STA) {
         return MP_OBJ_FROM_PTR(&wlan_sta_obj);
@@ -193,6 +229,129 @@ STATIC mp_obj_t get_wlan(size_t n_args, const mp_obj_t *args) {
 }
 STATIC MP_DEFINE_CONST_FUN_OBJ_VAR_BETWEEN(get_wlan_obj, 0, 1, get_wlan);
 
+STATIC void phy_power_enable(bool enable) {
+    lan_if_obj_t* self = MP_OBJ_TO_PTR(&lan_obj);
+
+    if (self->phy_power_pin != -1) {
+
+        if (!enable) {
+            /* Do the PHY-specific power_enable(false) function before powering down */
+            self->power_func(false);
+        }
+
+        gpio_pad_select_gpio(self->phy_power_pin);
+        gpio_set_direction(self->phy_power_pin,GPIO_MODE_OUTPUT);
+        if(enable == true) {
+            gpio_set_level(self->phy_power_pin, 1);
+        } else {
+            gpio_set_level(self->phy_power_pin, 0);
+        }
+
+        // Allow the power up/down to take effect, min 300us
+        vTaskDelay(1);
+
+        if (enable) {
+            /* Run the PHY-specific power on operations now the PHY has power */
+            self->power_func(true);
+        }
+    }
+}
+
+STATIC void init_lan_rmii() {
+    lan_if_obj_t* self = MP_OBJ_TO_PTR(&lan_obj);
+    phy_rmii_configure_data_interface_pins();
+    phy_rmii_smi_configure_pins(self->mdc_pin, self->mdio_pin);
+}
+
+STATIC void init_lan() {
+    lan_if_obj_t* self = MP_OBJ_TO_PTR(&lan_obj);
+    eth_config_t config;
+
+    switch (self->phy_type) {
+        case PHY_TLK110:
+            config = phy_tlk110_default_ethernet_config; 
+            break;
+        case PHY_LAN8720:
+            config = phy_lan8720_default_ethernet_config;
+            break;
+    }
+    
+    self->link_func = config.phy_check_link;
+
+    // Replace default power func with our own
+    self->power_func = config.phy_power_enable;
+    config.phy_power_enable = phy_power_enable;
+
+    config.phy_addr = self->phy_addr;
+    config.gpio_config = init_lan_rmii;
+    config.tcpip_input = tcpip_adapter_eth_input;
+
+    if (esp_eth_init(&config) == ESP_OK) {
+        esp_eth_enable();
+        self->active = true;
+    } else {
+        mp_raise_msg(&mp_type_OSError, "esp_eth_init() failed");
+    }
+}
+
+STATIC mp_obj_t get_lan(size_t n_args, const mp_obj_t *pos_args, mp_map_t *kw_args) {
+    lan_if_obj_t* self = MP_OBJ_TO_PTR(&lan_obj);
+
+    if (self->initialized) {
+        return MP_OBJ_FROM_PTR(&lan_obj);
+    }
+
+    enum { ARG_id, ARG_mdc, ARG_mdio, ARG_power, ARG_phy_addr, ARG_phy_type };
+
+    uint8_t default_pins[] = {23, 18, 17}; // mdc, mdio, power
+
+    static const mp_arg_t allowed_args[] = {
+        { MP_QSTR_id,           MP_ARG_OBJ, {.u_obj = mp_const_none} },
+        { MP_QSTR_mdc,          MP_ARG_OBJ, {.u_obj = MP_OBJ_NULL} },
+        { MP_QSTR_mdio,         MP_ARG_OBJ, {.u_obj = MP_OBJ_NULL} },
+        { MP_QSTR_power,        MP_ARG_OBJ, {.u_obj = MP_OBJ_NULL} },
+        { MP_QSTR_phy_addr,     MP_ARG_INT, {.u_int = 0x01} },
+        { MP_QSTR_phy_type,     MP_ARG_INT, {.u_int = PHY_LAN8720} },
+    };
+
+    mp_arg_val_t args[MP_ARRAY_SIZE(allowed_args)];
+    mp_arg_parse_all(n_args, pos_args, kw_args, MP_ARRAY_SIZE(allowed_args), allowed_args, args);
+
+    if (args[ARG_id].u_obj != mp_const_none) {
+        if (mp_obj_get_int(args[ARG_id].u_obj) != 0) {
+            mp_raise_ValueError("invalid LAN interface identifier");
+        }
+    }
+
+    for(int i = ARG_mdc; i <= ARG_power; ++i) {
+        if (args[i].u_obj == mp_const_none && i == ARG_power) {
+            args[i].u_int = -1;
+        } else if (args[i].u_obj != MP_OBJ_NULL) {
+            args[i].u_int = machine_pin_get_id(args[i].u_obj);
+        } else {
+            args[i].u_int = default_pins[i - ARG_mdc];
+        }
+    }
+
+    if (args[ARG_phy_addr].u_int < 0x00 || args[ARG_phy_addr].u_int > 0x1f) {
+        mp_raise_ValueError("invalid phy address");
+    }
+
+    if (args[ARG_phy_type].u_int != PHY_LAN8720 && args[ARG_phy_type].u_int != PHY_TLK110) {
+        mp_raise_ValueError("invalid phy type");
+    }
+
+    self->mdc_pin = args[ARG_mdc].u_int;
+    self->mdio_pin = args[ARG_mdio].u_int;
+    self->phy_power_pin = args[ARG_power].u_int;
+    self->phy_addr = args[ARG_phy_addr].u_int;
+    self->phy_type = args[ARG_phy_type].u_int;
+    self->initialized = true;
+    init_lan();
+    return MP_OBJ_FROM_PTR(&lan_obj);
+}
+STATIC MP_DEFINE_CONST_FUN_OBJ_KW(get_lan_obj, 0, get_lan);
+
 STATIC mp_obj_t esp_initialize() {
     static int initialized = 0;
     if (!initialized) {
@@ -200,15 +359,6 @@ STATIC mp_obj_t esp_initialize() {
         tcpip_adapter_init();
         ESP_LOGD("modnetwork", "Initializing Event Loop");
         ESP_EXCEPTIONS( esp_event_loop_init(event_handler, NULL) );
-        ESP_LOGD("modnetwork", "esp_event_loop_init done");
-        wifi_init_config_t cfg = WIFI_INIT_CONFIG_DEFAULT();
-        ESP_LOGD("modnetwork", "Initializing WiFi");
-        ESP_EXCEPTIONS( esp_wifi_init(&cfg) );
-        ESP_EXCEPTIONS( esp_wifi_set_storage(WIFI_STORAGE_RAM) );
-        ESP_LOGD("modnetwork", "Initialized");
-        ESP_EXCEPTIONS( esp_wifi_set_mode(0) );
-        ESP_EXCEPTIONS( esp_wifi_start() );
-        ESP_LOGD("modnetwork", "Started");
         initialized = 1;
     }
     return mp_const_none;
@@ -237,6 +387,28 @@ STATIC mp_obj_t esp_active(size_t n_args, const mp_obj_t *args) {
 
 STATIC MP_DEFINE_CONST_FUN_OBJ_VAR_BETWEEN(esp_active_obj, 1, 2, esp_active);
 
+STATIC mp_obj_t lan_active(size_t n_args, const mp_obj_t *args) {
+
+    lan_if_obj_t *self = MP_OBJ_TO_PTR(args[0]);
+
+    if (n_args > 1) {
+        if (mp_obj_is_true(args[1])) {
+            self->active = (esp_eth_enable() == ESP_OK);
+            if (!self->active) {
+                mp_raise_msg(&mp_type_OSError, "ethernet enable failed");
+            }
+        } else {
+            self->active = !(esp_eth_disable() == ESP_OK);
+            if (self->active) {
+                mp_raise_msg(&mp_type_OSError, "ethernet disable failed");
+            }
+        }
+    }
+    return self->active ? mp_const_true : mp_const_false;
+}
+
+STATIC MP_DEFINE_CONST_FUN_OBJ_VAR_BETWEEN(lan_active_obj, 1, 2, lan_active);
+
 STATIC mp_obj_t esp_connect(size_t n_args, const mp_obj_t *args) {
 
     mp_uint_t len;
@@ -273,6 +445,12 @@ STATIC mp_obj_t esp_status(mp_obj_t self_in) {
 
 STATIC MP_DEFINE_CONST_FUN_OBJ_1(esp_status_obj, esp_status);
 
+STATIC mp_obj_t lan_isconnected(mp_obj_t self_in) {
+    lan_if_obj_t *self = MP_OBJ_TO_PTR(self_in);
+    return self->link_func() ? mp_const_true : mp_const_false;
+}
+STATIC MP_DEFINE_CONST_FUN_OBJ_1(lan_isconnected_obj, lan_isconnected);
+
 STATIC mp_obj_t esp_scan(mp_obj_t self_in) {
     // check that STA mode is active
     wifi_mode_t mode;
@@ -358,10 +536,10 @@ STATIC mp_obj_t esp_ifconfig(size_t n_args, const mp_obj_t *args) {
         netutils_parse_ipv4_addr(items[2], (void*)&info.gw, NETUTILS_BIG);
         netutils_parse_ipv4_addr(items[3], (void*)&dns_addr, NETUTILS_BIG);
         // To set a static IP we have to disable DHCP first
-        if (self->if_id == WIFI_IF_STA) {
-            esp_err_t e = tcpip_adapter_dhcpc_stop(WIFI_IF_STA);
+        if (self->if_id == WIFI_IF_STA || self->if_id == ESP_IF_ETH) {
+            esp_err_t e = tcpip_adapter_dhcpc_stop(self->if_id);
             if (e != ESP_OK && e != ESP_ERR_TCPIP_ADAPTER_DHCP_ALREADY_STOPPED) _esp_exceptions(e);
-            ESP_EXCEPTIONS(tcpip_adapter_set_ip_info(WIFI_IF_STA, &info));
+            ESP_EXCEPTIONS(tcpip_adapter_set_ip_info(self->if_id, &info));
         } else if (self->if_id == WIFI_IF_AP) {
             esp_err_t e = tcpip_adapter_dhcps_stop(WIFI_IF_AP);
             if (e != ESP_OK && e != ESP_ERR_TCPIP_ADAPTER_DHCP_ALREADY_STOPPED) _esp_exceptions(e);
@@ -521,6 +699,21 @@ const mp_obj_type_t wlan_if_type = {
     .locals_dict = (mp_obj_t)&wlan_if_locals_dict,
 };
 
+STATIC const mp_map_elem_t lan_if_locals_dict_table[] = {
+    { MP_OBJ_NEW_QSTR(MP_QSTR_active), (mp_obj_t)&lan_active_obj },
+    { MP_OBJ_NEW_QSTR(MP_QSTR_isconnected), (mp_obj_t)&lan_isconnected_obj },
+    { MP_OBJ_NEW_QSTR(MP_QSTR_status), (mp_obj_t)&esp_status_obj },
+    { MP_OBJ_NEW_QSTR(MP_QSTR_ifconfig), (mp_obj_t)&esp_ifconfig_obj },
+};
+
+STATIC MP_DEFINE_CONST_DICT(lan_if_locals_dict, lan_if_locals_dict_table);
+
+const mp_obj_type_t lan_if_type = {
+    { &mp_type_type },
+    .name = MP_QSTR_LAN,
+    .locals_dict = (mp_obj_t)&lan_if_locals_dict,
+};
+
 STATIC mp_obj_t esp_phy_mode(size_t n_args, const mp_obj_t *args) {
     return mp_const_none;
 }
@@ -531,6 +724,7 @@ STATIC const mp_map_elem_t mp_module_network_globals_table[] = {
     { MP_OBJ_NEW_QSTR(MP_QSTR___name__), MP_OBJ_NEW_QSTR(MP_QSTR_network) },
     { MP_OBJ_NEW_QSTR(MP_QSTR___init__), (mp_obj_t)&esp_initialize_obj },
     { MP_OBJ_NEW_QSTR(MP_QSTR_WLAN), (mp_obj_t)&get_wlan_obj },
+    { MP_OBJ_NEW_QSTR(MP_QSTR_LAN), (mp_obj_t)&get_lan_obj },
     { MP_OBJ_NEW_QSTR(MP_QSTR_phy_mode), (mp_obj_t)&esp_phy_mode_obj },
 
 #if MODNETWORK_INCLUDE_CONSTANTS
@@ -558,6 +752,11 @@ STATIC const mp_map_elem_t mp_module_network_globals_table[] = {
         MP_OBJ_NEW_SMALL_INT(WIFI_AUTH_WPA_WPA2_PSK) },
     { MP_OBJ_NEW_QSTR(MP_QSTR_AUTH_MAX),
         MP_OBJ_NEW_SMALL_INT(WIFI_AUTH_MAX) },
+
+    { MP_OBJ_NEW_QSTR(MP_QSTR_PHY_LAN8720),
+        MP_OBJ_NEW_SMALL_INT(PHY_LAN8720) },
+    { MP_OBJ_NEW_QSTR(MP_QSTR_PHY_TLK110),
+        MP_OBJ_NEW_SMALL_INT(PHY_TLK110) },
 #endif
 };

@dpgeorge
Copy link
Member

@dpgeorge Is that something you want done before merging?

@MrSurly yes, please put the LAN code in a separate file.

@dpgeorge
Copy link
Member

I have STM32F769 board, just want to make sure I am not reinventing the wheel.

@mhanuel26 that MCU has a built-in Ethernet MAC peripheral so it's possible to add network support to it. The main two things that need to be done are: 1) integrate lwIP into the stm32 port to provide a TCP/IP stack; 2) add support for the STM32 ETH peripheral so that one can send and receive raw Ethernet frames.

@nickzoic
Copy link
Collaborator

I'll put in a PR to move the WLAN stuff out to network_wlan.c tomorrow (AEDT)

@MrSurly
Copy link
Contributor Author

MrSurly commented Oct 18, 2017

@nickzoic Some of my PR re-arranges the init code -- there's a separate wifi init bit now -- maybe take a look?

I'll try to do the wired Ethernet tonight It's currently 7P PST, I'll hit it in an hour or so.

@MrSurly
Copy link
Contributor Author

MrSurly commented Oct 18, 2017

@nickzoic Separated out into network_lan.c and network_lan.h

@dpgeorge Compiles, but not tested on hardware yet. I'll do that tomorrow, and update here. Alternatively, if Nick has time to test, that'd be great.

@@ -136,6 +136,7 @@ SRC_C = \
machine_uart.c \
modmachine.c \
modnetwork.c \
network_lan.c \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a tab/space inconsistency?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, looked weird in meld, so I "fixed" it.

@@ -34,6 +35,8 @@
#include <stdint.h>
#include <string.h>

#include "network_lan.h"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use modnetwork.h for a common header file. Since the header code is not expected to excessive in size it's better to put all the data for WLAN/LAN/etc in modnetwork.h.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, please move this include to after the core py/ includes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -50,6 +53,8 @@

#define MODNETWORK_INCLUDE_CONSTANTS (1)

MP_DECLARE_CONST_FUN_OBJ_KW(get_lan_obj);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should go in modnetwork.h

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -200,15 +219,6 @@ STATIC mp_obj_t esp_initialize() {
tcpip_adapter_init();
ESP_LOGD("modnetwork", "Initializing Event Loop");
ESP_EXCEPTIONS( esp_event_loop_init(event_handler, NULL) );
ESP_LOGD("modnetwork", "esp_event_loop_init done");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This log line should stay here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch.

{ MP_OBJ_NEW_QSTR(MP_QSTR_PHY_LAN8720),
MP_OBJ_NEW_SMALL_INT(PHY_LAN8720) },
{ MP_OBJ_NEW_QSTR(MP_QSTR_PHY_TLK110),
MP_OBJ_NEW_SMALL_INT(PHY_TLK110) },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the constants be prefixed with something like ETH or LAN to emphasise that they are for wired and not wifi?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, the hardware in question is a PHY, thus the PHY prefix -- this should stay.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it's specifically a wired LAN PHY, as opposed to a wireless/USB/etc PHY. So I was thinking something like ETH_PHY_LAN8720.

}
return self->active ? mp_const_true : mp_const_false;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blank line

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.


STATIC mp_obj_t lan_isconnected(mp_obj_t self_in) {
lan_if_obj_t *self = MP_OBJ_TO_PTR(self_in);
return self->link_func() ? mp_const_true : mp_const_false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mp_obj_new_bool

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

STATIC MP_DEFINE_CONST_FUN_OBJ_VAR_BETWEEN(lan_active_obj, 1, 2, lan_active);

STATIC mp_obj_t lan_status(mp_obj_t self_in) {
return mp_const_none;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indenting

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, should return an integer, just return 0 for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can change this, but it will differ from the WLAN implementation:

STATIC mp_obj_t esp_status(mp_obj_t self_in) {
    return mp_const_none;
}

STATIC MP_DEFINE_CONST_FUN_OBJ_1(esp_status_obj, esp_status);

Can you confirm?

.locals_dict = (mp_obj_t)&lan_if_locals_dict,
};


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No trailing blank lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

const mp_obj_type_t lan_if_type = {
{ &mp_type_type },
.name = MP_QSTR_LAN,
.locals_dict = (mp_obj_t)&lan_if_locals_dict,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cast should be mp_obj_dict_t*

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing, but the WLAN implementation has this same issue.

@nickzoic
Copy link
Collaborator

My branch to split network_wlan off is on top of this PR, so either I'll hold my horsies until it is slightly more ready.

Also once the various comments above are addressed I'll do some more hardware testing.

@MrSurly
Copy link
Contributor Author

MrSurly commented Oct 19, 2017

@dpgeorge Committed those changes commented on above, still not tested on physical hardware.

@nickzoic There are a couple of points above we need to hammer out, but none of them will affect your changes. Any HW testing will be welcome.

@MrSurly
Copy link
Contributor Author

MrSurly commented Oct 24, 2017

I'll probably have an opportunity to test this week.

self->mdio_pin = args[ARG_mdio].u_int;
self->phy_power_pin = args[ARG_power].u_int;
self->phy_addr = args[ARG_phy_addr].u_int;
self->phy_type = args[ARG_phy_type].u_int;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need to store phy_addr and phy_type in the self structure. Just pass them straight through as args to init_lan().

@MrSurly
Copy link
Contributor Author

MrSurly commented Oct 26, 2017

@dpgeorge Awaiting a connector to re-wire my PHY to test. I should have bought more PHYs.

@nickzoic
Copy link
Collaborator

nickzoic commented Oct 26, 2017 via email

@MrSurly
Copy link
Contributor Author

MrSurly commented Oct 26, 2017

Can I help test?

By all means! I'm having a literal wire issue. I'd solder it temporarily, but then I'd have to un-solder it, and that board has been through a lot already -- afraid I'm going to lift the pads.

@nickzoic
Copy link
Collaborator

nickzoic commented Oct 26, 2017 via email

@MrSurly
Copy link
Contributor Author

MrSurly commented Oct 26, 2017

@dpgeorge Tested this build today; works ok.

@dpgeorge
Copy link
Member

Looking really clean @MrSurly!

@nickzoic do you want to test this yourself to double-check things?

@dpgeorge
Copy link
Member

@MrSurly can you please rebase and squash this into a small number of commits, probably just 1 commit is good enough?

@MrSurly
Copy link
Contributor Author

MrSurly commented Oct 30, 2017

@dpgeorge Done.

@nickzoic
Copy link
Collaborator

nickzoic commented Oct 30, 2017 via email

@dpgeorge
Copy link
Member

@nickzoic I'll wait for your testing before merging.

@MrSurly
Copy link
Contributor Author

MrSurly commented Oct 30, 2017

I might've borked the rebase -- it's my Kryptonite. But I did do a diff against the esp32 branch, and it seemed ok.

@nickzoic
Copy link
Collaborator

nickzoic commented Oct 31, 2017

OK, so I checked out 523f122 but that's fallen behind esp32 again :-/ so I re-rebased it onto esp32 at a6566fc and got b77b41e
( nickzoic/micropython-esp32@b77b41e5 )

Updated ESP-IDF to espressif/esp-idf@2c95a77cf and recursive submodules. make clean & make deploy to a DevKitC with rev 1 chip, wired up to a modified Wavelan LAN8720 as per https://sautter.com/blog/ethernet-on-esp32-using-lan8720/

Ran a quick test script (attached below) and did some manual poking around and everything looks good to merge. @dpgeorge, let me know if you want me to push the button :-)

Comments:

  • It does indeed crash if the hardware isn't set up. I removed the 'power' pin and it wedges into a blind look of "emac: emac resetting ...." ditto if you pick the wrong pin numbers. The watchdog timer doesn't seem to catch it either. This is is an ESP-IDF bug though (related: [TW#13762] emac: emac resetting error espressif/esp-idf#768 ) so I don't think it should stop us merging this.

  • I'd still rather the default state of the interface was inactive. The behaviour on activation, if no address has been configured, is to DHCP for an address. By starting deactivated you don't get a race condition between configuring the interface and setting its IP address. This would also make it the same as network.WLAN. On the other hand, hitting lan.active(False) immediately works for now.

Stuff which should become separate issues:

  • Once you lan.ifconfig(( ... )) there's no way to set the interface back to "dhcp mode" (this is also true for the WLAN interface ifconfig ...

  • It handles routing packets to the two interfaces (LAN & WLAN) correctly, but by default it doesn't route (eg: IP forwarding) between them ... to make it act as an AP we'd have to expose a 'network.Route' interface or similar.

test-wlan.py.txt

@MrSurly
Copy link
Contributor Author

MrSurly commented Oct 31, 2017

I'd still rather the default state of the interface was inactive. The behaviour on activation, if no address has been configured, is to DHCP for an address. By starting deactivated you don't get a race condition between configuring the interface and setting its IP address. This would also make it the same as network.WLAN. On the other hand, hitting lan.active(False) immediately works for now.

Hmm, I agree. Should this be changed?

Once you lan.ifconfig(( ... )) there's no way to set the interface back to "dhcp mode" (this is also true for the WLAN interface ifconfig ...

Perhaps calling ifconfig with an empty sequence? Or a special parameter of network.DHCP?

@dpgeorge
Copy link
Member

I'd still rather the default state of the interface was inactive.

Yse I agree that should be changed so it matches network.WLAN. @MrSurly is that an easy fix for you to do?

Once you lan.ifconfig(( ... )) there's no way to set the interface back to "dhcp mode"

The way the cc3200 does it is by passing the string "dhcp" to ifconfig: nic.ifconfig("dhcp")

@MrSurly
Copy link
Contributor Author

MrSurly commented Oct 31, 2017

@MrSurly is that an easy fix for you to do?

Yes, changed, squashed, and pushed. Not tested; my PHY is at work. @nickzoic You up?

@nickzoic
Copy link
Collaborator

Sadly, if I run lan.isconnected() before lan.active(True) it hangs indefinitely (no watchdog, no logging)
I'm guessing something which should be being woken up in components/ethernet/emac_main.c : emac_start isn't happening and causing the link_func to spin indefinitely.

Anyway, easily fixed by checking self->active before calling link_func ...
return self->active ? mp_obj_new_bool(self->link_func()) : mp_const_false;

@MrSurly
Copy link
Contributor Author

MrSurly commented Oct 31, 2017

Oops! Yeah, that makes sense. The PHY will be inactive. Lemme fix.

Updates to Makefile, modnetwork.c, and addition of network_lan.c to
implement `network.LAN()` object for wired PHY objects.
@MrSurly
Copy link
Contributor Author

MrSurly commented Oct 31, 2017

Updated, squashed, pushed.

Thanks, @nickzoic.

@nickzoic
Copy link
Collaborator

Tested, perfect! And thanks for sticking at it ... @dpgeorge we're all ready to merge ...

@dpgeorge
Copy link
Member

Thanks guys, great work! I merged it in 236297f with some minor last-minute edits: removed the "Based on the ESP IDF example code..." line from modnetwork.h (but not network_lan.c) because that header is just bare minimum and didn't take anything from the IDF; removed a few whitespace at end of lines; changed lan_if_locals_dict_table to a ROM dict and used MP_ROM_QSTR and MP_ROM_PTR macros (we should change the WLAN object to prevent more copy-paste errors).

@dpgeorge dpgeorge closed this Oct 31, 2017
@nickzoic
Copy link
Collaborator

Thanks Damien!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants