Component/net connect #939
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
abhik-roy85
force-pushed
the
component/net_connect
branch
from
24d6b84 to
712046b
Compare
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| #if CONFIG_NET_CONNECT_CONNECT_IPV6 | ||
| xSemaphoreTake(s_semph_get_ip6_addrs, portMAX_DELAY); | ||
| #endif | ||
| return ESP_OK; |
There was a problem hiding this comment.
Bug: Ethernet connection hangs indefinitely if no IP
The net_connect_ethernet_connect function waits on s_semph_get_ip_addrs with portMAX_DELAY. If the Ethernet link is down or DHCP fails, the semaphore is never given, causing the application to hang indefinitely without a timeout or error return.
Key features: - Port network connection logic from protocol_examples_common into a standalone net_connect component. - Provide unified APIs for WiFi, Ethernet, Thread, and PPP connectivity. - Add comprehensive Kconfig options and example usage. - Introduce programmatic WiFi STA configuration APIs: - net_configure_wifi_sta() - net_connect_wifi() - net_disconnect_wifi() - Simplify WiFi connection flow and remove redundant config prefixes. - Add backward-compatibility mappings via sdkconfig.rename. Ethernet migration & cleanup: - Migrate Ethernet setup to the new ethernet_init component. - Simplify Kconfig by delegating Ethernet options to the main config. - Add interface init/deinit logging. - Fix missing Thread/PPP shutdown handlers. - Remove obsolete net_get_eth_handle() API. Misc: - Update component structure, CMake files, and general cleanup.
abhik-roy85
force-pushed
the
component/net_connect
branch
from
c94572e to
c18c062
Compare
abhik-roy85
requested review from
alisitsyn,
bogdankolendovskyy,
david-cermak,
euripedesrocha,
gabsuren,
glmfe and
kostaond
abhik-roy85
force-pushed
the
component/net_connect
branch
from
f614be6 to
695a0cb
Compare
david-cermak
left a comment
david-cermak
left a comment
There was a problem hiding this comment.
Adding a few high-level topics to talk about, no need to be addressed or fixed, just related to the structural/modular ideas of this component
| void net_connect_wifi_start(void); | ||
| void net_connect_wifi_stop(void); | ||
| esp_err_t net_connect_wifi_sta_do_connect(wifi_config_t wifi_config, bool wait); | ||
| esp_err_t net_connect_wifi_sta_do_disconnect(void); | ||
| bool net_connect_is_our_netif(const char *prefix, esp_netif_t *netif); | ||
| void net_connect_print_all_netif_ips(const char *prefix); | ||
| void net_connect_wifi_shutdown(void); | ||
| void net_connect_ethernet_shutdown(void); | ||
| esp_err_t net_connect_ethernet_connect(void); | ||
| void net_connect_thread_shutdown(void); | ||
| esp_err_t net_connect_thread_connect(void); | ||
| esp_err_t net_connect_ppp_connect(void); | ||
| void net_connect_ppp_start(void); | ||
| void net_connect_ppp_shutdown(void); |
There was a problem hiding this comment.
Should we somehow unify these methods across interfaces?
Matrix of wifi/ethernet/thead/ppp vs start/stop/connect/disconnect
| } | ||
|
|
||
|
|
||
| esp_err_t net_disconnect(void) |
There was a problem hiding this comment.
How do we handle lifecycle of different interfaces? Do we support re-connect after disconnection for various interfaces?
i.e. Ethernet shutdown and reconnect while the Wi-Fi is connected?
| #if CONFIG_NET_CONNECT_ETHERNET | ||
| ESP_LOGI(TAG, "Deinitializing Ethernet interface..."); | ||
| net_connect_ethernet_shutdown(); | ||
| ESP_ERROR_CHECK(esp_unregister_shutdown_handler(&net_connect_ethernet_shutdown)); |
There was a problem hiding this comment.
Should we handle errors in more graceful way?
Applies to other places as well, uses ESP_ERROR_CHECK() in some places returns errors or error goto macros in others.
| net_connect_wifi_stop(); | ||
| } | ||
|
|
||
| net_iface_handle_t net_configure_wifi_sta(net_wifi_sta_config_t *config) |
There was a problem hiding this comment.
Now wifi provides ample runtime configuration, while others are more or less Kconfig options.
should we unify this, or do we prefer runtime to compile-time config?
| static const char *TAG = "ethernet_connect"; | ||
| static SemaphoreHandle_t s_semph_get_ip_addrs = NULL; | ||
| #if CONFIG_NET_CONNECT_IPV6 | ||
| static SemaphoreHandle_t s_semph_get_ip6_addrs = NULL; |
There was a problem hiding this comment.
should the IPv6 be handled somehow globally, not per interface? Does it make sense to rework the architecture to provide netif-agnostic event/handlers, while keeping lifecycle netif/driver specific?
| @@ -0,0 +1,66 @@ | |||
| # net_connect | |||
There was a problem hiding this comment.
Should we also consider design for testablity? Suggest adding a simple unit test, just the see how the current design is modular/testable
| #include "driver/uart.h" | ||
| #include "sdkconfig.h" | ||
|
|
||
| esp_err_t net_configure_stdin_stdout(void) |
There was a problem hiding this comment.
Does this API logically belong to this component?
| } | ||
| } | ||
|
|
||
| static void ot_task_worker(void *aContext) |
There was a problem hiding this comment.
This uses worker thread, while other interfaces use sync calls. Should be use sync/async approach here?
|
@abhik-roy85 General note: I didn’t see any CI or testing coverage mentioned in the PR or workflows. |
abhik-roy85
force-pushed
the
component/net_connect
branch
from
695a0cb to
a8cef47
Compare
| { | ||
| if (s_semph_get_ip_addrs == NULL) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Bug: Ethernet IPv6-only shutdown skips resource cleanup
The net_connect_ethernet_shutdown() function checks if s_semph_get_ip_addrs == NULL to determine whether to proceed with cleanup and returns early if it's NULL. However, when only CONFIG_NET_CONNECT_IPV6 is enabled without CONFIG_NET_CONNECT_IPV4, s_semph_get_ip_addrs is never created and remains NULL. This causes the function to return early without cleaning up the IPv6 semaphore or stopping ethernet, leading to resource leaks. This scenario is common when Thread is enabled, since NET_CONNECT_IPV4 defaults to n for Thread configurations.
| #elif CONFIG_NET_CONNECT_PPP_DEVICE_UART | ||
| s_stop_task = true; | ||
| vTaskDelay(pdMS_TO_TICKS(1000)); // wait for the ppp task to stop | ||
| #endif |
There was a problem hiding this comment.
Bug: PPP UART missing cleanup on shutdown
When using PPP over UART, ppp_task() registers an event handler at line 163 (esp_event_handler_register(IP_EVENT, IP_EVENT_PPP_GOT_IP, esp_netif_action_connected, s_netif)), but net_connect_ppp_shutdown() only unregisters the on_ip_event handler and never unregisters esp_netif_action_connected. Additionally, the UART driver installed in ppp_task() via uart_driver_install() is never deleted during shutdown, causing a resource leak.
Additional Locations (1)
| esp_vfs_eventfd_unregister(); | ||
| vSemaphoreDelete(s_semph_thread_set_dns_server); | ||
| vSemaphoreDelete(s_semph_thread_attached); | ||
| } |
There was a problem hiding this comment.
Bug: Thread event handler never unregistered on shutdown
In net_connect_thread_connect(), an event handler is registered with esp_event_handler_register(OPENTHREAD_EVENT, ESP_EVENT_ANY_ID, thread_event_handler, NULL) at line 120. However, net_connect_thread_shutdown() never calls esp_event_handler_unregister() to remove this handler. This leaves a dangling event handler registered after shutdown that could cause issues if the semaphores are deleted but events are still dispatched.
Additional Locations (1)
| xSemaphoreTake(s_semph_get_ip_addrs, portMAX_DELAY); | ||
| vSemaphoreDelete(s_semph_get_ip_addrs); | ||
| s_semph_get_ip_addrs = NULL; | ||
| #endif |
There was a problem hiding this comment.
Bug: WiFi semaphore leak when IPv4 disabled
In net_connect_wifi_sta_do_connect(), s_semph_get_ip_addrs is created unconditionally at line 188 when wait=true, but it's only deleted inside the #if CONFIG_NET_CONNECT_IPV4 block at lines 218-220. When CONFIG_NET_CONNECT_IPV4 is disabled (which is the default when Thread is enabled), the semaphore is created but never deleted, causing a memory leak. The creation should also be conditional on CONFIG_NET_CONNECT_IPV4, matching the deletion logic.
| memset(wifi_config.sta.password, 0, sizeof(wifi_config.sta.password)); | ||
| temp = strtok_r(NULL, " ", &rest); | ||
| if (temp) { | ||
| strncpy((char*)wifi_config.sta.password, temp, sizeof(wifi_config.sta.password)); |
There was a problem hiding this comment.
Bug: WiFi stdin path missing null-termination for SSID/password
The strncpy calls at lines 312 and 316 use the full buffer size (sizeof(wifi_config.sta.ssid) and sizeof(wifi_config.sta.password)) without ensuring null-termination. This is inconsistent with convert_to_wifi_config() which correctly uses size - 1 and explicitly null-terminates. If a user enters an SSID of 32+ characters or password of 64+ characters from stdin, the resulting strings won't be null-terminated, potentially causing buffer over-reads when the WiFi stack processes these values.
| endif() | ||
|
|
||
|
|
||
| idf_component_register(SRCS "${srcs}" |
There was a problem hiding this comment.
Are all these dependencies necessary?
| @@ -0,0 +1,316 @@ | |||
| menu "Net Connect Configuration" | |||
|
|
|||
| orsource "$IDF_PATH/examples/common_components/env_caps/$IDF_TARGET/Kconfig.env_caps" | |||
There was a problem hiding this comment.
Why source from the example?
| Choose the preferred interface (WiFi, Ethernet, Thread, PPPoS) to connect to the network and configure the interface. | ||
|
|
||
| It is possible to enable multiple interfaces simultaneously making the connection phase to block until all the chosen interfaces acquire IP addresses. | ||
| It is also possible to disable all interfaces, skipping the connection phase altogether. |
There was a problem hiding this comment.
So what is the use of the component in this case?
|
|
||
| ### WiFi | ||
|
|
||
| Choose WiFi connection method (for chipsets that support it) and configure basic WiFi connection properties: |
There was a problem hiding this comment.
If the device doesn't support it, we should provide the means for it, using esp_wifi_remote
|
|
||
| #define HOST_IP_SIZE 128 | ||
|
|
||
| esp_err_t get_addr_from_stdin(int port, int sock_type, int *ip_protocol, int *addr_family, struct sockaddr_storage *dest_addr) |
There was a problem hiding this comment.
This function seems to be a good fit for having a single point of return cleaning up allocated resources.
| return strncmp(prefix, esp_netif_get_desc(netif), strlen(prefix) - 1) == 0; | ||
| } | ||
|
|
||
| static bool netif_desc_matches_with(esp_netif_t *netif, void *ctx) |
There was a problem hiding this comment.
| static bool netif_desc_matches_with(esp_netif_t *netif, void *ctx) | |
| static bool netif_desc_matches_with(esp_netif_t *netif, char *ctx) |
| extern "C" { | ||
| #endif | ||
|
|
||
| #if !CONFIG_IDF_TARGET_LINUX |
There was a problem hiding this comment.
I don't think the guard are necessary for the defines.
| #endif | ||
|
|
||
| #if CONFIG_NET_CONNECT_WIFI_SCAN_METHOD_FAST | ||
| #define NET_CONNECT_WIFI_SCAN_METHOD WIFI_FAST_SCAN |
There was a problem hiding this comment.
Do we need to redefine WIFI related configs? Seems that this might lead us to compatibility issues in the future.
| */ | ||
| esp_netif_t *net_get_netif_from_desc(const char *desc); | ||
|
|
||
| #if CONFIG_NET_CONNECT_PROVIDE_WIFI_CONSOLE_CMD |
There was a problem hiding this comment.
This should be only available if WIFI is also enabled.
| switch (event_id) { | ||
| case ETHERNET_EVENT_CONNECTED: | ||
| ESP_LOGI(TAG, "Ethernet Link Up"); | ||
| ESP_ERROR_CHECK(esp_netif_create_ip6_linklocal(esp_netif)); |
There was a problem hiding this comment.
Since Net Connect going to be a common component, we should stop using ESP_ERROR_CHECK (which asserts) and return error gracefully.
| description: Network connection component providing WiFi, Ethernet, Thread, and PPP connection methods for ESP32 boards. | ||
| dependencies: | ||
| idf: | ||
| version: '>=5.0' |
There was a problem hiding this comment.
ethernet_init is compatible starting with IDF ver 5.4.3 (or more precisely '>=5.4.3,!=5.5.0,!=5.5.1'). Does it make sense to be this component compatible starting with 5.0?
| #define NGX_UNESCAPE_REDIRECT (2) | ||
|
|
||
|
|
||
| uintptr_t ngx_escape_uri(u_char *dst, u_char *src, size_t size, unsigned int type) |
There was a problem hiding this comment.
This should not be part of net_connect component. It could rather be separate component.
|
|
||
| #define HOST_IP_SIZE 128 | ||
|
|
||
| esp_err_t get_addr_from_stdin(int port, int sock_type, int *ip_protocol, int *addr_family, struct sockaddr_storage *dest_addr) |
There was a problem hiding this comment.
This function is not used by Net connect. It seems to be a common function not related to connect hence it should be separate component.
| */ | ||
|
|
||
| /** | ||
| * @file net_connect_example.c |
There was a problem hiding this comment.
Is this example built in CI?
| esp_netif_action_connected(s_netif, 0, 0, 0); | ||
| #else // DEVICE is UART | ||
| s_stop_task = false; | ||
| if (xTaskCreate(ppp_task, "ppp connect", 4096, NULL, 5, NULL) != pdTRUE) { |
There was a problem hiding this comment.
Should the ppp task stack be configurable like for thread connection? May be other parameters can be configurable as well.
| ESP_ERROR_CHECK(uart_driver_install(UART_NUM_1, BUF_SIZE, 0, 16, &event_queue, 0)); | ||
| ESP_ERROR_CHECK(uart_param_config(UART_NUM_1, &uart_config)); | ||
| ESP_ERROR_CHECK(uart_set_pin(UART_NUM_1, CONFIG_NET_CONNECT_UART_TX_PIN, CONFIG_NET_CONNECT_UART_RX_PIN, UART_PIN_NO_CHANGE, UART_PIN_NO_CHANGE)); | ||
| ESP_ERROR_CHECK(uart_set_rx_timeout(UART_NUM_1, 1)); |
There was a problem hiding this comment.
Is the UART RX timeout feature used for PPP to separate the frames for netif? This should generate timeout event after 1 symbol silence time on current baudrate but also on fifo full event because the timeout flag on data event is not checked. Is this expected behavior? The data received on DATA event will never reach 1024 bytes of buffer allocated because the DATA event will come every fifo full event (usually 120 bytes). It is better to check the timeout flag of the event structure to handle the timeout event correctly instead of every fifo full event and then use the whole allocated buffer to transfer the data from UART to netif more effectively.
abhik-roy85
force-pushed
the
component/net_connect
branch
from
a8cef47 to
76a84fa
Compare
| s_semph_get_ip6_addrs = NULL; | ||
| #endif | ||
| eth_stop(); | ||
| } |
There was a problem hiding this comment.
Bug: Ethernet shutdown skipped when only IPv6 enabled
The net_connect_ethernet_shutdown() function uses s_semph_get_ip_addrs == NULL as its guard condition to return early. However, when only IPv6 is enabled (not IPv4), this semaphore is never created because its creation is guarded by #if CONFIG_NET_CONNECT_IPV4. This causes the shutdown function to return immediately without calling eth_stop(), leaving the ethernet interface and resources unreleased in IPv6-only configurations.
| } | ||
| free(buffer); | ||
| vTaskDelete(NULL); | ||
| } |
There was a problem hiding this comment.
Bug: UART driver and event handler leaked in PPP shutdown
When using UART for PPP, the ppp_task installs the UART driver at line 150 and registers an event handler at line 163, but neither is cleaned up on shutdown. When s_stop_task is set and the task exits, it only frees the buffer and deletes itself without calling uart_driver_delete(). The net_connect_ppp_shutdown() function also doesn't unregister the esp_netif_action_connected handler. This causes resource leaks and prevents proper re-initialization.
Additional Locations (1)
| return ESP_FAIL; | ||
| } | ||
| for (cur = addr_list; cur != NULL; cur = cur->ai_next) { | ||
| memcpy(dest_addr, cur->ai_addr, sizeof(*dest_addr)); |
There was a problem hiding this comment.
Bug: Buffer over-read when copying address from getaddrinfo result
The memcpy copies sizeof(*dest_addr) bytes (typically 128 bytes for struct sockaddr_storage) from cur->ai_addr, but getaddrinfo allocates only enough memory for the actual address type - 16 bytes for sockaddr_in (IPv4) or 28 bytes for sockaddr_in6 (IPv6). This reads beyond the allocated memory bounds, causing undefined behavior. The copy size should use cur->ai_addrlen instead of sizeof(*dest_addr).
| esp_vfs_eventfd_unregister(); | ||
| vSemaphoreDelete(s_semph_thread_set_dns_server); | ||
| vSemaphoreDelete(s_semph_thread_attached); | ||
| } |
There was a problem hiding this comment.
Bug: Thread event handler never unregistered on shutdown
The thread_event_handler registered via esp_event_handler_register(OPENTHREAD_EVENT, ...) at line 120 in net_connect_thread_connect() is never unregistered in net_connect_thread_shutdown(). This causes a resource leak and the handler could be invoked after shutdown, potentially accessing deleted semaphores s_semph_thread_attached and s_semph_thread_set_dns_server.
Addresses code review feedback from GitHub cursor bot.
Add comprehensive unit test suite for net_get_netif_from_desc() function with 11 test cases covering: - Successful lookup for different netif types (STA, ETH, Thread, PPP, custom) - Error handling for NULL, empty string, and non-existent descriptions - Edge cases including case-sensitive matching, exact match requirements, and consistency across multiple calls The test suite uses Unity test framework with proper setup/teardown for netif creation and memory leak detection. Includes CMakeLists.txt files for build configuration and pytest integration for CI/CD.
abhik-roy85
force-pushed
the
component/net_connect
branch
from
76a84fa to
5211944
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| wifi_init_config_t cfg = WIFI_INIT_CONFIG_DEFAULT(); | ||
| ESP_ERROR_CHECK(esp_wifi_init(&cfg)); | ||
|
|
||
| esp_netif_inherent_config_t esp_netif_config = ESP_NETIF_INHERENT_DEFAULT_WIFI_STA(); |
Check warning
Code scanning / clang-tidy
The value '153' provided to the cast expression is not in the valid range of values for 'esp_netif_flags' [clang-analyzer-optin.core.EnumCastOutOfRange] Warning
| } | ||
| } | ||
| free(buffer); | ||
| vTaskDelete(NULL); |
There was a problem hiding this comment.
Bug: PPP UART task leaks UART driver on exit
The ppp_task function installs the UART driver at line 150 and registers an event handler at line 163, but when the task exits normally (lines 181-182), neither resource is cleaned up. The UART driver is never deleted with uart_driver_delete() and the IP_EVENT_PPP_GOT_IP event handler registered with esp_netif_action_connected is never unregistered. This causes resource leaks that prevent proper re-initialization of the PPP connection.
Additional Locations (1)
| esp_vfs_eventfd_unregister(); | ||
| vSemaphoreDelete(s_semph_thread_set_dns_server); | ||
| vSemaphoreDelete(s_semph_thread_attached); | ||
| } |
There was a problem hiding this comment.
Bug: Thread event handler never unregistered during shutdown
In net_connect_thread_connect(), an event handler for OPENTHREAD_EVENT is registered at line 120, but net_connect_thread_shutdown() never calls esp_event_handler_unregister() to remove it. This causes the thread_event_handler callback to remain active after shutdown, potentially leading to crashes or undefined behavior if OpenThread events are received after resources are freed.
Additional Locations (1)
| ESP_LOGI(TAG, "WiFi shutdown handler called"); | ||
| net_connect_wifi_sta_do_disconnect(); | ||
| net_connect_wifi_stop(); | ||
| } |
There was a problem hiding this comment.
Bug: WiFi state flag not reset after shutdown
The s_wifi_sta_configured flag is set to true in net_configure_wifi_sta() but never reset to false in either net_connect_wifi_shutdown() or net_disconnect_wifi(). This means after a disconnect-reconnect cycle, the code incorrectly believes WiFi is already configured, and subsequent calls to net_connect() will skip reconfiguration and attempt to use stale state, potentially causing connection failures.
Additional Locations (1)
| char *temp = strtok_r(buf, " ", &rest); | ||
| if (temp == NULL) { | ||
| ESP_LOGE(TAG, "SSID is empty or invalid"); | ||
| return ESP_ERR_INVALID_ARG; |
There was a problem hiding this comment.
Bug: WiFi resources leaked when stdin input fails
When CONFIG_NET_CONNECT_WIFI_SSID_PWD_FROM_STDIN is enabled, net_connect_wifi_start() is called at line 291 to initialize the WiFi driver and create the netif. However, if fgets() fails at line 301 or strtok_r() returns NULL at line 312, the function returns early without calling net_connect_wifi_stop(). This leaves the WiFi driver initialized and the netif allocated, causing a resource leak that prevents proper re-initialization on subsequent connection attempts.
6 participants
Description
Helper Note for Reviewers
This PR ports
protocol_examples_commoninto a standalonenet_connectcomponent. Most diffs are renames or file moves. The key areas that deserve focused review are listed below.✅ Important Areas to Review:
net_configure_wifi_sta()net_connect_wifi()net_wifi_sta_config_t,net_ip_config_t,net_iface_handle_t.net_connect()now checksnet_connect_wifi_is_configured().s_wifi_sta_config,s_wifi_sta_handle,s_wifi_sta_configuredexample_*→net_*across the component.include/net_connect_wifi_config.hYou can skip reviewing pure renames, file moves, minor formatting or logging changes, and straightforward Kconfig or metadata updates.
Related
Testing
Checklist
Before submitting a Pull Request, please ensure the following:
Note
Introduce
components/net_connectproviding unified connect/disconnect across WiFi, Ethernet, Thread, and PPP, plus a two-phase WiFi configuration API, example, and tests.components/net_connect(new)net_connect(),net_disconnect(),net_get_netif_from_desc(); prints IPs vianet_connect_print_all_netif_ips().net_configure_wifi_sta(),net_connect_wifi()), console commands, IPv6 events, stdin SSID/pwd option, helpers for start/stop and STA connect/disconnect.eth_connect.cwith event-driven IP acquisition and shutdown.thread_connect.c) with eventfd, attach/DNS wait, netif glue.addr_from_stdin().Kconfig.projbuildwith WiFi/Ethernet/PPP/Thread and IPv4/IPv6 options;sdkconfig.renamemappings.CMakeLists.txt/idf_component.ymland optional deps (console, esp_eth, openthread, tinyusb).examples/net_connect_exampleusing unified API.net_get_netif_from_desc()and pytest runner.net_connectcommit scope.Written by Cursor Bugbot for commit 5211944. This will update automatically on new commits. Configure here.