[eppp]: Support for transport via Ethernet (phy or emac) #622

david-cermak · 2024-07-25T11:35:31Z

Using a real phy chip, via a standard Ethernet cable
~~Using a dummy phy, by means of EMAC to EMAC connection~~
Added emac2emac example which demonstrates overriding (standard) Ethernet drivers with the custom one: eth_dummy_phy

kostaond · 2024-07-26T06:03:14Z

components/eppp_link/README.md

+
+- Internal EMAC with real PHY chip
+    * TCP - 5Mbits/s
+    * UDP - 8Mbits/s


I'm quite surprised with such poor performance. Do you have idea what could be the bottle neck and why it isn't at least as good as SDIO?

would probably get similar performance as SDIO, after some optimizations; tested just briefly with all defaults.
I'm planning on looking into it at some point.

kostaond · 2024-07-26T06:24:35Z

components/eppp_link/eppp_eth.c

+    };
+    gpio_config(&gpio_sink_cfg);
+    gpio_set_level(CONFIG_EPPP_LINK_ETHERNET_SINK_READY_GPIO, 0);
+    vTaskDelay(pdMS_TO_TICKS(STARTUP_DELAY_MS));


I'm the author of the simple EMAC 2 EMAC example but I'm not sure about this delay now :) I need to revisit.

I think it's a good workaround but doesn't really fit into component definition. Thinking of dissecting that and moving to example(s)

kostaond · 2024-07-26T06:27:21Z

components/eppp_link/eppp_eth.c

+
+static esp_err_t eth_init(void)
+{
+#if CONFIG_EPPP_LINK_ETHERNET_CLK_SOURCE


We need to clearly distinguish that this is only ESP32 workaround. I'll try to come up with something. I'm currently thinking to wrap all this workaround code to specific functions with prefix like esp32_gpio0_workaround. Users then can easily remove them if they don't need them.

100% agree, I'll also think about it.

kostaond · 2024-07-26T06:29:41Z

components/eppp_link/eppp_eth.c

+    ESP_RETURN_ON_ERROR(esp_eth_driver_install(&config, &s_eth_handles[0]), TAG, "Ethernet driver install failed");
+#endif // CONFIG_EPPP_LINK_ETHERNET_CLK_SINK
+    return ESP_OK;
+}


esp_restart(); of CLK source is missing when sink is restart. That could cause the sink may not boot correctly when restarted.

Good point. In case of wifi_remote would could leave this logic on the host (master) side and let this side control the slave. But this is a generic component (on level lower than wifi-remote) and each node (server/client) is more or less generic too (peer nodes).

Cannot wait in this task (could be user's task or eppp-task), would have to create a small task to handle this case...

kostaond · 2024-07-26T06:32:52Z

components/eppp_link/eppp_eth.c

+    s_eth_handles = NULL;
+}
+
+#else


What about adding a comment that here we use PHY chip to make it clearer to users? And also add comment at the start of #ifdef CONFIG_EPPP_LINK_ETHERNET_USE_DUMMY_PHY. Or rename it to EMAC to EMAC?

kostaond · 2024-07-26T06:34:48Z

components/eppp_link/README.md

+             SLAVE micro                                     HOST micro
+    \|/  +----------------+                               +----------------+
+     |   |                |       (serial) line           |                |
+     +---+ WiFi NAT PPPoS |=== UART / SPI / SDIO / ETH ===| PPPoS client   |


We may need to improve the README with instruction how to use EMAC to EMAC. Or at least provide link to the component?

Yes, I'll improve the docs and add sections for each transport.

david-cermak · 2024-07-26T09:10:41Z

@kostaond Thanks taking a look at the emac2emac option. I must say that I concur with your assessment and think that this option is probably not a viable transport option for eppp at this moment.

My suggestion is that we drop the emac2emac option in this PR, but allow the Ethernet init/start functionality to be extended (with callbacks or __weak functions).
Then we can add emac2emac as an example and add appropriate comments about the GPIO0 workaround and the synchronization and reset stuff...

kostaond · 2024-07-26T09:24:21Z

@kostaond Thanks taking a look at the emac2emac option. I must say that I concur with your assessment and think that this option is probably not a viable transport option for eppp at this moment.

My suggestion is that we drop the emac2emac option in this PR, but allow the Ethernet init/start functionality to be extended (with callbacks or __weak functions). Then we can add emac2emac as an example and add appropriate comments about the GPIO0 workaround and the synchronization and reset stuff...

Agree, if you need to move forward, drop the emac2emac. It's currently too much polluted with the GPIO0 workaround. Presence or non presence of the workaround must be super clear to users because the most probable use case is to use ESP32 with WiFi connected to some other chip without WiFi.

david-cermak · 2024-07-26T12:18:23Z

It's currently too much polluted with the GPIO0 workaround. Presence or non presence of the workaround must be super clear to users because the most probable use case is to use ESP32 with WiFi connected to some other chip without WiFi.

Removed the emac2emac from the component and added it as an extra example with the specific configuration, so the GPIO0 workaround is in the app code only. (Moreover It demonstrates using any custom Ethernet driver)

euripedesrocha · 2024-08-01T10:40:55Z

components/eppp_link/eppp_eth.c

+    return ESP_FAIL;
+}
+
+__attribute__((weak)) esp_err_t eppp_transport_ethernet_init(esp_eth_handle_t *handle_array[])


What is the use case here for having the init function as a customization point?

euripedesrocha · 2024-08-01T10:42:16Z

components/eppp_link/eppp_eth.c

+    sscanf(CONFIG_EPPP_LINK_ETHERNET_THEIR_ADDRESS, "%2" PRIu8 ":%2" PRIu8 ":%2" PRIi8 ":%2" PRIu8 ":%2" PRIu8 ":%2" PRIu8,
+           &s_their_mac[0], &s_their_mac[1], &s_their_mac[2], &s_their_mac[3], &s_their_mac[4], &s_their_mac[5]);
+    esp_eth_ioctl(s_eth_handles[0], ETH_CMD_S_MAC_ADDR, s_our_mac);
+    ESP_RETURN_ON_ERROR(esp_event_handler_register(ETH_EVENT, ESP_EVENT_ANY_ID, event_handler, NULL), TAG, "Failed to register Ethernet handlers");


Should we open space for a custom event handler?

euripedesrocha · 2024-08-01T10:43:53Z

components/eppp_link/eppp_eth.c

+esp_err_t eppp_transport_tx(void *h, void *buffer, size_t len)
+{
+    if (!s_is_connected) {
+        return ESP_OK;


Should this be a failure?

euripedesrocha · 2024-08-01T11:26:58Z

components/eppp_link/examples/emac2emac/README.md

+
+## How to use this example
+
+* Choose `CONFIG_EXAMPLE_NODE_SERVER` for the device connected as **RMII CLK Source Device**


We should add some advice here regarding clk source.
@kostaond the conditions for clk generation if Wi-Fi is used in the device applies here as well, right?

euripedesrocha · 2024-08-01T11:39:36Z

components/eppp_link/examples/emac2emac/main/app_main.c

+
+esp_err_t eppp_transport_ethernet_init(esp_eth_handle_t *handle[])
+{
+    *handle = malloc(sizeof(esp_eth_handle_t));


Isn't esp_eth_handle_t a type alias for void*? Do we need to allocate here?

euripedesrocha · 2024-08-01T11:41:59Z

components/eppp_link/examples/emac2emac/main/app_main.c

+    TaskHandle_t task_handle = xTaskGetHandle(pcTaskGetName(NULL));
+    gpio_isr_handler_add(CONFIG_EXAMPLE_RMII_CLK_READY_GPIO, gpio_isr_handler, task_handle);
+    ESP_LOGW(TAG, "waiting for RMII CLK sink device interrupt");
+    ESP_LOGW(TAG, "if RMII CLK sink device is already running, reset it by `EN` button");


Should we make it part of the source device to automatically reset the device if the user enables it?

feat(eppp): Add support for transport via Ethernet link

776e40c

david-cermak added the eppp label Jul 25, 2024

david-cermak self-assigned this Jul 25, 2024

david-cermak requested review from euripedesrocha and kostaond July 25, 2024 14:42

david-cermak force-pushed the feat/eppp_emac2emac branch from 150f1b3 to 001855e Compare July 25, 2024 14:46

kostaond reviewed Jul 26, 2024

View reviewed changes

david-cermak force-pushed the feat/eppp_emac2emac branch from 001855e to da05de2 Compare July 26, 2024 08:45

feat(eppp): Add demo with custom eth-driver using emac2emac

35f9579

david-cermak force-pushed the feat/eppp_emac2emac branch from da05de2 to 35f9579 Compare July 26, 2024 12:14

euripedesrocha reviewed Aug 1, 2024

View reviewed changes

tomassebestik removed the eppp label Sep 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[eppp]: Support for transport via Ethernet (phy or emac) #622

[eppp]: Support for transport via Ethernet (phy or emac) #622

david-cermak commented Jul 25, 2024 •

edited

Loading

kostaond Jul 26, 2024

david-cermak Jul 26, 2024

kostaond Jul 26, 2024

david-cermak Jul 26, 2024

kostaond Jul 26, 2024

david-cermak Jul 26, 2024

kostaond Jul 26, 2024

david-cermak Jul 26, 2024

kostaond Jul 26, 2024

kostaond Jul 26, 2024

david-cermak Jul 26, 2024

david-cermak commented Jul 26, 2024

kostaond commented Jul 26, 2024

david-cermak commented Jul 26, 2024

euripedesrocha Aug 1, 2024

euripedesrocha Aug 1, 2024

euripedesrocha Aug 1, 2024

euripedesrocha Aug 1, 2024

euripedesrocha Aug 1, 2024

euripedesrocha Aug 1, 2024


		## How to use this example

		* Choose `CONFIG_EXAMPLE_NODE_SERVER` for the device connected as RMII CLK Source Device

[eppp]: Support for transport via Ethernet (phy or emac) #622

Are you sure you want to change the base?

[eppp]: Support for transport via Ethernet (phy or emac) #622

Conversation

david-cermak commented Jul 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-cermak commented Jul 26, 2024

kostaond commented Jul 26, 2024

david-cermak commented Jul 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-cermak commented Jul 25, 2024 •

edited

Loading