Update IP addresses in ESP_NETIF to stop static addresses being overwritten #3133
Conversation
nfbot
changed the title
WalkthroughThis update integrates new ESP32 network configuration functionality. A new source file is added to the build list and implemented with functions that retrieve network configuration blocks, configure network settings, and apply settings by index. Additionally, the Ethernet module now sets a static IP by invoking the new configuration function, while the wireless module replaces a waiting function with a dedicated station configuration function. Corresponding header declarations have also been updated. Changes
Suggested labels
Suggested reviewers
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Automated fixes for code style.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
targets/ESP32/_Network/NF_ESP32_Ethernet.cpp (1)
222-223: Implementation looks good for configuring static addressesThe new code addresses the PR objective by calling
NF_ESP32_ConfigureNetworkByIndexto configure static IP addresses before starting the Ethernet driver, which should prevent them from being overwritten by DHCP.Consider adding error logging here similar to what's in the Wireless implementation for consistency:
- ESP_ERROR_CHECK(NF_ESP32_ConfigureNetworkByIndex(IDF_ETH_DEF, eth_netif)); + esp_err_t ec = NF_ESP32_ConfigureNetworkByIndex(IDF_ETH_DEF, eth_netif); + if (ec != ESP_OK) + { + ESP_LOGE(TAG, "Unable to configure Ethernet - result %d", ec); + }targets/ESP32/_Network/NF_ESP32_Network.cpp (1)
38-42: Improve error handling in default caseThe comment "can't reach here" is misleading since the default case can be reached with an invalid network interface number. Consider logging the invalid number and returning an error code.
default: - // can't reach here - HAL_AssertEx(); + // Invalid network interface number + DEBUG_PRINT("Invalid network interface number: %d\n", num); + return -1; break;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
targets/ESP32/_Network/CMakeLists.txt(1 hunks)targets/ESP32/_Network/NF_ESP32_Ethernet.cpp(1 hunks)targets/ESP32/_Network/NF_ESP32_Network.cpp(1 hunks)targets/ESP32/_Network/NF_ESP32_Wireless.cpp(2 hunks)targets/ESP32/_include/NF_ESP32_Network.h(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: nf-interpreter (Check_Code_Style)
- GitHub Check: nf-interpreter (Check_Build_Options)
- GitHub Check: nf-interpreter (Nightly build) (Check_Build_Options)
🔇 Additional comments (6)
targets/ESP32/_Network/CMakeLists.txt (1)
14-14: Correctly adds the new network file to the buildThis change properly adds the new
NF_ESP32_Network.cppfile to the build, aligning with the PR objective of moving common code into a dedicated file.targets/ESP32/_include/NF_ESP32_Network.h (1)
63-65: New function declarations look goodThese new function declarations support the PR objective by providing proper interfaces for retrieving network configuration blocks and applying network settings to prevent static addresses from being overwritten.
targets/ESP32/_Network/NF_ESP32_Wireless.cpp (2)
115-118: Helper function implementation looks goodThis new function provides a clean, dedicated way to configure the Wi-Fi station with the correct network index.
163-169: Error handling for network configuration looks goodThis implementation properly checks for errors when configuring the Wi-Fi station and logs them appropriately, helping with debugging if there are issues with static IP configuration.
targets/ESP32/_Network/NF_ESP32_Network.cpp (2)
55-69: Memory allocation handling is well implementedThe function properly checks for memory allocation failures and configuration retrieval errors, returning NULL in both cases. This allows the caller to handle errors appropriately.
103-118: Proper memory management in NF_ESP32_ConfigureNetworkByIndexThe function correctly frees the allocated memory for the network configuration block after use, which prevents memory leaks.
| ec = esp_netif_set_ip_info(netIf, &ip_info); | ||
|
|
||
| // Make sure DHCP client is disabled | ||
| netIf->flags = (esp_netif_flags_t)(netIf->flags & ~ESP_NETIF_DHCP_CLIENT); |
There was a problem hiding this comment.
Avoid direct flag manipulation to stop DHCP client
Directly modifying netIf->flags may be fragile if the ESP-IDF API changes. Use the official ESP-IDF API function esp_netif_dhcpc_stop() to disable the DHCP client.
- netIf->flags = (esp_netif_flags_t)(netIf->flags & ~ESP_NETIF_DHCP_CLIENT);
+ esp_err_t dhcp_stop_result = esp_netif_dhcpc_stop(netIf);
+ // It's okay if DHCP was already stopped
+ if (dhcp_stop_result != ESP_OK && dhcp_stop_result != ESP_ERR_ESP_NETIF_DHCP_ALREADY_STOPPED)
+ {
+ return dhcp_stop_result;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| netIf->flags = (esp_netif_flags_t)(netIf->flags & ~ESP_NETIF_DHCP_CLIENT); | |
| esp_err_t dhcp_stop_result = esp_netif_dhcpc_stop(netIf); | |
| // It's okay if DHCP was already stopped | |
| if (dhcp_stop_result != ESP_OK && dhcp_stop_result != ESP_ERR_ESP_NETIF_DHCP_ALREADY_STOPPED) | |
| { | |
| return dhcp_stop_result; | |
| } |
| // | ||
| // Configure network settings for a espressif network interface | ||
| // | ||
| esp_err_t NF_ESP32_ConfigureNetwork(esp_netif_t *netIf, HAL_Configuration_NetworkInterface *config) | ||
| { | ||
| esp_err_t ec; | ||
| esp_netif_ip_info_t ip_info; | ||
|
|
||
| ec = esp_netif_get_ip_info(netIf, &ip_info); | ||
| if (ec != ESP_OK) | ||
| { | ||
| return ec; | ||
| } | ||
|
|
||
| bool enableDHCP = (config->StartupAddressMode == AddressMode_DHCP); | ||
|
|
||
| // Set static addresses | ||
| if (config->IPv4Address != 0) | ||
| { | ||
| ip_info.ip.addr = config->IPv4Address; | ||
| ip_info.netmask.addr = config->IPv4NetMask; | ||
| ip_info.gw.addr = config->IPv4GatewayAddress; | ||
|
|
||
| ec = esp_netif_set_ip_info(netIf, &ip_info); | ||
|
|
||
| // Make sure DHCP client is disabled | ||
| netIf->flags = (esp_netif_flags_t)(netIf->flags & ~ESP_NETIF_DHCP_CLIENT); | ||
| } | ||
|
|
||
| return ec; | ||
| } |
There was a problem hiding this comment.
Missing DHCP client enablement when configured
The function calculates whether DHCP should be enabled but only handles disabling it for static IP configurations. When enableDHCP is true, the code should enable the DHCP client.
esp_err_t NF_ESP32_ConfigureNetwork(esp_netif_t *netIf, HAL_Configuration_NetworkInterface *config)
{
esp_err_t ec;
esp_netif_ip_info_t ip_info;
ec = esp_netif_get_ip_info(netIf, &ip_info);
if (ec != ESP_OK)
{
return ec;
}
bool enableDHCP = (config->StartupAddressMode == AddressMode_DHCP);
+ // Handle DHCP configuration
+ if (enableDHCP)
+ {
+ // Enable DHCP client
+ ec = esp_netif_dhcpc_start(netIf);
+ return ec;
+ }
+ // Static IP configuration
// Set static addresses
if (config->IPv4Address != 0)
{
ip_info.ip.addr = config->IPv4Address;
ip_info.netmask.addr = config->IPv4NetMask;
ip_info.gw.addr = config->IPv4GatewayAddress;
ec = esp_netif_set_ip_info(netIf, &ip_info);
// Make sure DHCP client is disabled
netIf->flags = (esp_netif_flags_t)(netIf->flags & ~ESP_NETIF_DHCP_CLIENT);
+ ec = esp_netif_dhcpc_stop(netIf);
+ if (ec != ESP_OK && ec != ESP_ERR_ESP_NETIF_DHCP_ALREADY_STOPPED)
+ {
+ return ec;
+ }
}
return ec;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // | |
| // Configure network settings for a espressif network interface | |
| // | |
| esp_err_t NF_ESP32_ConfigureNetwork(esp_netif_t *netIf, HAL_Configuration_NetworkInterface *config) | |
| { | |
| esp_err_t ec; | |
| esp_netif_ip_info_t ip_info; | |
| ec = esp_netif_get_ip_info(netIf, &ip_info); | |
| if (ec != ESP_OK) | |
| { | |
| return ec; | |
| } | |
| bool enableDHCP = (config->StartupAddressMode == AddressMode_DHCP); | |
| // Set static addresses | |
| if (config->IPv4Address != 0) | |
| { | |
| ip_info.ip.addr = config->IPv4Address; | |
| ip_info.netmask.addr = config->IPv4NetMask; | |
| ip_info.gw.addr = config->IPv4GatewayAddress; | |
| ec = esp_netif_set_ip_info(netIf, &ip_info); | |
| // Make sure DHCP client is disabled | |
| netIf->flags = (esp_netif_flags_t)(netIf->flags & ~ESP_NETIF_DHCP_CLIENT); | |
| } | |
| return ec; | |
| } | |
| // | |
| // Configure network settings for a espressif network interface | |
| // | |
| esp_err_t NF_ESP32_ConfigureNetwork(esp_netif_t *netIf, HAL_Configuration_NetworkInterface *config) | |
| { | |
| esp_err_t ec; | |
| esp_netif_ip_info_t ip_info; | |
| ec = esp_netif_get_ip_info(netIf, &ip_info); | |
| if (ec != ESP_OK) | |
| { | |
| return ec; | |
| } | |
| bool enableDHCP = (config->StartupAddressMode == AddressMode_DHCP); | |
| // Handle DHCP configuration | |
| if (enableDHCP) | |
| { | |
| // Enable DHCP client | |
| ec = esp_netif_dhcpc_start(netIf); | |
| return ec; | |
| } | |
| // Static IP configuration | |
| // Set static addresses | |
| if (config->IPv4Address != 0) | |
| { | |
| ip_info.ip.addr = config->IPv4Address; | |
| ip_info.netmask.addr = config->IPv4NetMask; | |
| ip_info.gw.addr = config->IPv4GatewayAddress; | |
| ec = esp_netif_set_ip_info(netIf, &ip_info); | |
| // Make sure DHCP client is disabled | |
| netIf->flags = (esp_netif_flags_t)(netIf->flags & ~ESP_NETIF_DHCP_CLIENT); | |
| ec = esp_netif_dhcpc_stop(netIf); | |
| if (ec != ESP_OK && ec != ESP_ERR_ESP_NETIF_DHCP_ALREADY_STOPPED) | |
| { | |
| return ec; | |
| } | |
| } | |
| return ec; | |
| } |
|
@alberk8 Are you able to test this is working OK. |
…a-dcca-4939-b3d0-7fada9631085' into Static_ip
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
targets/ESP32/_Network/NF_ESP32_Network.cpp (2)
11-53: 🛠️ Refactor suggestionAdd a timeout for the infinite loop in
NF_ESP32_Wait_NetNumber.
At present, if the requested interface is never created, the loop can block indefinitely. Consider implementing a max retry count or timeout to prevent the application from hanging.int NF_ESP32_Wait_NetNumber(int num) { esp_netif_t *espNetif = nullptr; + const int maxRetries = 1500; // e.g. 1500 * 20ms = 30 seconds + int retryCount = 0; while (true) { // ... if (espNetif != NULL) { break; } vTaskDelay(20 / portTICK_PERIOD_MS); + if (++retryCount >= maxRetries) + { + return -1; // Indicate failure + } } return espNetif->lwip_netif->num; }
74-101: 🛠️ Refactor suggestionUse official DHCP APIs rather than manipulating
netIf->flags.
Disabling DHCP by modifyingnetIf->flagsdirectly can be fragile. The ESP-IDF recommends usingesp_netif_dhcpc_stop()oresp_netif_dhcpc_start()for robust DHCP control. Additionally, ifenableDHCPistrue, you may want to explicitly enable the DHCP client.bool enableDHCP = (config->StartupAddressMode == AddressMode_DHCP); // If DHCP is enabled, start the DHCP client +if (enableDHCP) { + ec = esp_netif_dhcpc_start(netIf); + if (ec != ESP_OK && ec != ESP_ERR_ESP_NETIF_DHCP_ALREADY_STARTED) { + return ec; + } +} else { // Set static addresses if (config->IPv4Address != 0) { ip_info.ip.addr = config->IPv4Address; ip_info.netmask.addr = config->IPv4NetMask; ip_info.gw.addr = config->IPv4GatewayAddress; ec = esp_netif_set_ip_info(netIf, &ip_info); - netIf->flags = (esp_netif_flags_t)(netIf->flags & ~ESP_NETIF_DHCP_CLIENT); + // Stop DHCP client if configured for static IP + esp_err_t dhcpStop = esp_netif_dhcpc_stop(netIf); + if (dhcpStop != ESP_OK && dhcpStop != ESP_ERR_ESP_NETIF_DHCP_ALREADY_STOPPED) + { + return dhcpStop; + } } }
🧹 Nitpick comments (2)
targets/ESP32/_Network/NF_ESP32_Wireless.cpp (1)
163-169: Revisit the 'ignore any errors' comment.
Although the inline comment states to ignore errors, the code does log errors whenec != ESP_OK. If these unexpected failures need follow-up actions (e.g. disabling further Wi-Fi initialization steps or reverting to DHCP), you might consider more robust handling instead of continuing with partial configuration.targets/ESP32/_Network/NF_ESP32_Network.cpp (1)
103-118: Validate the final result after configuration.
This function retrieves a network configuration block, applies it, and frees the block. Consider logging or confirming that the new IP configuration is successfully set. Adding a short verification step (like anotheresp_netif_get_ip_info()call) could help catch subtle runtime issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
targets/ESP32/_Network/NF_ESP32_Network.cpp(1 hunks)targets/ESP32/_Network/NF_ESP32_Wireless.cpp(2 hunks)targets/ESP32/_include/NF_ESP32_Network.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- targets/ESP32/_include/NF_ESP32_Network.h
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: nf-interpreter (Check_Code_Style)
- GitHub Check: nf-interpreter (Nightly build) (Check_Build_Options)
- GitHub Check: nf-interpreter (Check_Build_Options)
🔇 Additional comments (1)
targets/ESP32/_Network/NF_ESP32_Wireless.cpp (1)
115-118:❓ Verification inconclusive
Consider explicit handling of station mode checks.
While this function successfully delegates configuration toNF_ESP32_ConfigureNetworkByIndex, ensure that the caller only invokes it if the device is indeed in station mode. If future code calls this outside station mode contexts, it might lead to unexpected results or error logs.Would you like to verify that this function is only called for station interfaces by searching the codebase for its invocations and ensuring the associated logic?
🏁 Script executed:
#!/bin/bash rg -A 4 "NF_ESP32_ConfigureNetworkStation"Length of output: 828
Action: Confirm Explicit Station Mode Validation for NF_ESP32_ConfigureNetworkStation
We verified that the only identifiable invocation of this function is made with a variable named
wifiStaNetifintargets/ESP32/_Network/NF_ESP32_Wireless.cpp, suggesting that it is intended for station mode configurations. However, there is no explicit check within the function (or immediately before its invocation) that confirms the provided interface indeed belongs to station mode. To guard against potential misuse from future changes, please consider adding an explicit validation step—either here or at the caller—to ensure that the function is only used with station interfaces.
- File under review:
targets/ESP32/_Network/NF_ESP32_Wireless.cpp- Identified call site: Invocation using
wifiStaNetifwithout an accompanying station mode check
|
There are some useful suggestions coming out of coderabbitai for existing code but I don't have time at the moment to follow them up. I can do that once I am back from holiday. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
targets/ESP32/_Network/NF_ESP32_Network.cpp (2)
95-96:⚠️ Potential issueHandle DHCP enablement as previously recommended.
This logic assigns
enableDHCPbut never starts the DHCP client. A past comment already addressed this omission. WhenenableDHCPistrue, ensure you invokeesp_netif_dhcpc_start()to enable the DHCP client.Consider applying the following diff:
bool enableDHCP = (config->StartupAddressMode == AddressMode_DHCP); + if (enableDHCP) + { + ec = esp_netif_dhcpc_start(netIf); + if (ec != ESP_OK && ec != ESP_ERR_ESP_NETIF_DHCP_ALREADY_STARTED) + { + return ec; + } + }
107-107:⚠️ Potential issueAvoid direct flag manipulation to stop DHCP, as previously advised.
This line is still directly modifying
netIf->flagsto disable the DHCP client. Per the ESP-IDF guidelines, useesp_netif_dhcpc_stop(netIf)instead to improve reliability and forward compatibility.Example fix:
- netIf->flags = (esp_netif_flags_t)(netIf->flags & ~ESP_NETIF_DHCP_CLIENT); + ec = esp_netif_dhcpc_stop(netIf); + // It's okay if DHCP was already stopped + if (ec != ESP_OK && ec != ESP_ERR_ESP_NETIF_DHCP_ALREADY_STOPPED) + { + return ec; + }
🧹 Nitpick comments (1)
targets/ESP32/_Network/NF_ESP32_Network.cpp (1)
98-106: Validate static address settings to avoid invalid configurations.If a static IP is required (
enableDHCP == false) yetconfig->IPv4Addressis0, no valid IP info gets applied. Consider warning or handling this edge case to prevent silently misconfigured interfaces.Example approach:
bool enableDHCP = (config->StartupAddressMode == AddressMode_DHCP); // Set static addresses if (config->IPv4Address != 0) { + // Optionally validate these addresses before applying ip_info.ip.addr = config->IPv4Address; ip_info.netmask.addr = config->IPv4NetMask; ip_info.gw.addr = config->IPv4GatewayAddress; } +else if (!enableDHCP) +{ + // Log warning or handle the scenario where the user specified static mode + // but didn't provide valid static IP data + // e.g. ESP_LOGW("NF_ESP32_Network", "No static IP configured in static mode"); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
targets/ESP32/_Network/NF_ESP32_Network.cpp(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: nf-interpreter (Check_Code_Style)
- GitHub Check: nf-interpreter (Nightly build) (Check_Build_Options)
- GitHub Check: nf-interpreter (Check_Build_Options)
Description
This has been an issue since the IDF changed from TCPIP adapters to the ESP_NETIF interface.
The problem is the ESP_NETIF starts the DHCP when the WIFI connects overriding the static IP settings.
This change updates IP addresses in ESP_NETIF by call esp_netif_set_ip_info() for Ethernet & Wireless networks.
Also tidies up code by moving common code to new file NF_ESP32_Network.cpp
Motivation and Context
How Has This Been Tested?
Some testing was done at the time these changes where done.
Further testing should be done to confirm chnage is working correctly.
Types of changes
Checklist
Summary by CodeRabbit
New Features
Refactor