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
There was a problem hiding this comment.
For this release, the existing code and API behavior are intentionally kept unchanged. A unified interface (start, stop, connect, disconnect, reconnect) across Wi-Fi, Ethernet, Thread, and PPP is planned for a future release where architectural changes can be introduced safely.
| } | ||
|
|
||
|
|
||
| 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?
There was a problem hiding this comment.
This will be addressed together with the unified interface work in a future release. The current release does not introduce changes to interface lifecycle or reconnection logic to avoid architectural disruption.
| #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.
There was a problem hiding this comment.
Improved error handling (replacing ESP_ERROR_CHECK with proper error returns and cleanup paths) is planned for a future release. The current release intentionally avoids refactoring error-handling behavior.
| 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?
There was a problem hiding this comment.
A runtime configuration API has already been introduced (net_iface_handle_t net_configure_wifi_sta(net_wifi_sta_config_t *config)). Similar APIs will be added for other interfaces in a future release.
The existing net_connect() API will continue to work by internally invoking these configuration APIs using Kconfig values, ensuring full backward compatibility.
| 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
There was a problem hiding this comment.
Internal restructuring to improve mockability and testability is planned for a future release. I am considering a Hierarchical State Machine–based architecture.
| #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?
There was a problem hiding this comment.
The APIs in stdin_out.c are currently used within this component. They will be kept as-is for now, and revisited once the overall API surface is stabilized.
| } | ||
| } | ||
|
|
||
| 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?
There was a problem hiding this comment.
This is a valid observation. In a future release, the architecture will be reworked to ensure that initialization of one interface does not block others, and to standardize synchronous/asynchronous behavior across interfaces. No such changes are introduced in the current release.
|
@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
| 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?
There was a problem hiding this comment.
If all the interfaces are disabled the net_connect() api becomes a no-op api.
It all depends on the configuration.
|
|
||
| ### 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
| #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.
There was a problem hiding this comment.
The Kconfig choice menu generates boolean configs (CONFIG_NET_CONNECT_WIFI_SCAN_METHOD_FAST), but the WiFi API expects enum values (WIFI_FAST_SCAN). The mapping converts the Kconfig selection to the required enum.
| 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.
There was a problem hiding this comment.
This will be done in a future release.
| */ | ||
|
|
||
| /** | ||
| * @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.
There was a problem hiding this comment.
Making PPP task parameters configurable per interface is planned.
However, this requires a broader architectural redesign of interface initialization and task management, which will be addressed in a future release.
| 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
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
- Remove protocol_examples_utils and addr_from_stdin files - Add null checks in ethernet shutdown to prevent crashes - Add proper cleanup in wifi shutdown and error paths - Update CMakeLists to reflect removed source files
abhik-roy85
force-pushed
the
component/net_connect
branch
from
6ff42aa to
b20f0da
Compare
- Fix net_connect_is_our_netif() NULL check and strlen bug - Add proper cleanup/rollback on interface init failures - Add NULL checks for netif creation in Ethernet - Fix double-cleanup issue in Thread shutdown - Add missing cleanup in PPP and WiFi error paths
| goto cleanup; | ||
| } | ||
| ESP_ERROR_CHECK(esp_register_shutdown_handler(&net_connect_wifi_shutdown)); | ||
| wifi_initialized = true; |
Check warning
Code scanning / clang-tidy
Value stored to 'wifi_initialized' is never read [clang-analyzer-deadcode.DeadStores] Warning
- Fix empty string check in net_get_netif_from_desc() to handle both NULL and empty string cases - Add conditional compilation guards for initialization flags to avoid unused variable warnings when features are disabled - Add proper Ethernet resource cleanup (ethernet_deinit_all) when netif or netif glue creation fails in eth_connect - Signal connection failure event when PPP buffer allocation fails - Add esp_vfs_eventfd_unregister() cleanup when Thread task creation fails to prevent resource leaks These changes improve error handling and ensure proper resource cleanup in failure scenarios across all network connection types.
abhik-roy85
force-pushed
the
component/net_connect
branch
2 times, most recently
from
4e4d656 to
340ee46
Compare
abhik-roy85
force-pushed
the
component/net_connect
branch
from
340ee46 to
3ea7ca3
Compare
components/net_connect/examples/ppp_connect_example/main/ppp_connect_example.c
Outdated
Show resolved
Hide resolved
…pendency - Add PPP connection example with comprehensive README - Automate esp_tinyusb dependency via idf_component.yml - Move PPP setup instructions to example README - Fix header guards and improve code documentation - Add IPv6 support: wait for both IPv4 and IPv6 addresses when IPv6 is enabled before considering PPP connection successful - Make esp_tinyusb dependency unconditional (remove conditional rule) - Remove net_connect from example PRIV_REQUIRES as it's now implicit
abhik-roy85
force-pushed
the
component/net_connect
branch
from
3ea7ca3 to
4b05064
Compare
- Update esp_tinyusb dependency from 1.7.6~2 to 2.0.0 - Migrate PPP USB CDC ACM implementation to new API: * Replace tusb_cdc_acm.h with tinyusb_cdc_acm.h header * Add tinyusb_default_config.h include * Use TINYUSB_DEFAULT_CONFIG() macro for tinyusb_config_t initialization * Update function call from tusb_cdc_acm_init() to tinyusb_cdcacm_init() * Remove deprecated usb_dev parameter from acm_cfg configuration These changes ensure compatibility with esp_tinyusb v2.0.0 which introduced API changes including header renames, new default configuration macros, and updated function signatures.
| } | ||
| esp_event_handler_unregister(OPENTHREAD_EVENT, ESP_EVENT_ANY_ID, thread_event_handler); | ||
| vSemaphoreDelete(s_semph_thread_set_dns_server); | ||
| vSemaphoreDelete(s_semph_thread_attached); |
There was a problem hiding this comment.
Thread semaphores deleted unconditionally risking use-after-free
The vSemaphoreDelete() calls for s_semph_thread_set_dns_server and s_semph_thread_attached are placed outside the if (s_ot_task_handle != NULL) block, causing them to be called unconditionally. This is problematic when net_connect_thread_connect() was never called (semaphores are NULL) or when it failed after the first semaphore was created then deleted at line 113 (variable still holds a stale pointer). Additionally, at line 113, after vSemaphoreDelete(s_semph_thread_attached), the variable is not set to NULL, leaving a dangling pointer that could cause a double-free.
Additional Locations (1)
| #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.
Disconnect crashes if shutdown handlers were never registered
The net_disconnect() function uses ESP_ERROR_CHECK() on esp_unregister_shutdown_handler() for each interface type. If net_connect() was never called, failed before registering handlers, or only partially succeeded, these calls will return ESP_ERR_INVALID_STATE and ESP_ERROR_CHECK will abort the program. Unlike the cleanup code in net_connect() which tracks initialization state with boolean flags, net_disconnect() unconditionally attempts to unregister all handlers.
Additional Locations (2)
| void net_register_wifi_connect_commands(void) | ||
| { | ||
| ESP_LOGI(TAG, "Registering WiFi connect commands."); | ||
| net_connect_wifi_start(); |
There was a problem hiding this comment.
Console command registration causes crash on later net_connect
net_register_wifi_connect_commands() calls net_connect_wifi_start() which initializes the WiFi driver via esp_wifi_init(). If the user later calls net_connect(), it will invoke net_connect_wifi() which also calls net_connect_wifi_start(). Since net_connect_wifi_start() has no guard to check if WiFi is already initialized, the second call to esp_wifi_init() will fail and ESP_ERROR_CHECK will abort the program. Console commands are enabled by default when not using stdin for credentials, making this a likely usage scenario.
Additional Locations (1)
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
Adds new
net_connectcomponent with unified connect/disconnect APIs, Kconfig, console cmds, examples (generic and PPP), unit tests, and required dependencies; updates commit message scopes.net_connect(),net_disconnect(),net_get_netif_from_desc().net_configure_wifi_sta(),net_connect_wifi(),net_disconnect_wifi()withnet_wifi_sta_config_tandnet_ip_config_t.wifi_connect.c,eth_connect.c,thread_connect.c,ppp_connect.c; prints IPv4/IPv6 per netif.esp_tinyusb,ethernet_init, IDF constraints).examples/net_connect_example: demonstrates multi-interface connect/disconnect and netif retrieval.examples/ppp_connect_example: PPP over USB CDC/UART with README and defaults.net_get_netif_from_desc()behavior and test harness setup.net_connect.Written by Cursor Bugbot for commit 36c93fc. This will update automatically on new commits. Configure here.