Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors DHCP reservation handling and adds ESP32-S3 support. The DHCP logic has been extracted from main.rs into a dedicated src/dhcp.rs module with a new implementation that directly manipulates the DHCP server's internal state rather than using temporary pool restrictions.
- Refactored DHCP reservation system to directly manipulate DHCP server internals
- Added ESP32-S3 chip support with Xtensa architecture target
- Implemented IP conflict detection and resolution via station deauthentication
- Increased thread stack sizes to accommodate larger operations
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main.rs | Refactored to use new DHCP module; added IP conflict resolution; increased thread stack sizes; added STA_AIDS tracking |
| src/dhcp.rs | New module implementing DHCP server state manipulation with direct access to internal structures |
| sdkconfig.defaults | Increased SoftAP connection limits and DHCP station capacity |
| rust-toolchain.toml | Changed from nightly to esp channel; added Xtensa targets |
| readme.md | Added ESP32-S3 documentation and toolchain setup instructions |
| justfile | Added build/flash/run commands for ESP32-S3 |
| Cargo.toml | Added esp32s3 feature flag and libc dependency |
| .cargo/config.toml | Added Xtensa ESP32-S3 target configuration |
Comments suppressed due to low confidence (1)
src/dhcp.rs:1
- The variable
conflict_ip_reservedchecksDHCP_RESERVATIONS.get(new_mac)but should checkDHCP_RESERVATIONS.get(&conflict)to determine if the conflicting MAC's IP is reserved, not the new MAC's reservation.
use crate::{format_mac, STATIC_LEASES};
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
src/dhcp.rs:1
- Logic error: checking
new_macinstead ofconflictMAC address. This should beDHCP_RESERVATIONS.get(&conflict)to determine if the conflicting device's IP is reserved.
use crate::{format_mac, STATIC_LEASES};
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| current = (*current).pnext; | ||
| } | ||
|
|
||
| let pool_ptr = calloc(1, std::mem::size_of::<DhcpsPool>()) as *mut DhcpsPool; |
There was a problem hiding this comment.
Memory allocated with calloc in upsert_static_entry is never freed. If DhcpServerState is dropped and recreated (e.g., on AP restart), this will leak memory. Consider implementing Drop trait to clean up allocated nodes, or document that this is intentional for the lifetime of the program.
src/main.rs
Outdated
| new_mac: &[u8; 6], | ||
| new_ip: Ipv4Addr, | ||
| client_ips: &Arc<Mutex<HashMap<[u8; 6], Ipv4Addr>>>, | ||
| dhcp_state: &Arc<Mutex<Option<DhcpServerState>>>, |
There was a problem hiding this comment.
The dhcp_state parameter is unused in resolve_ip_conflicts function. Consider removing it to clean up the function signature.
| dhcp_state: &Arc<Mutex<Option<DhcpServerState>>>, |
| @@ -1,3 +1,8 @@ | |||
| [toolchain] | |||
| channel = "nightly" | |||
| channel = "esp" | |||
There was a problem hiding this comment.
The change from 'nightly' to 'esp' toolchain is undocumented in the PR. Users upgrading may need to install the esp toolchain first. While the readme.md adds installation instructions, consider documenting this breaking change or ensuring backward compatibility.
| let lease = DhcpsLease::from_range(pool_start_ip, pool_end_ip); | ||
| set_dhcp_lease(handle, &lease)?; | ||
|
|
||
| let netif_obj = unsafe { &mut *(handle as *mut EspNetifObjHead) }; |
There was a problem hiding this comment.
Unsafe cast from opaque handle to internal EspNetifObjHead structure relies on undocumented ESP-IDF internals. This may break across ESP-IDF versions. Consider adding a comment documenting which ESP-IDF version this structure layout is based on, or add version checks.
No description provided.