DHCP range problematic in AP mode

[NdK73]

Hello.

I just noticed possible problems when calling WiFi.softAPConfig().
It assumes everybody uses a class-C network, a fixed mask of 24 bits and places the ESP at the start of the network (.1).
If the address given is over 55, the DHCP range collides with the reserved address (.255) and/or the network address (.0).
So I slightly modified the code to accept two optional arguments: dhcp_start and dhcp_end .
That makes it possible for the user to choose more freely the address range to use (I still don't check for network/broadcast collisions: with a great power...), including a whole class-A if desired (not that it's really useful with at most 4 clients, but anyway...).
If the optional arguments are not supplied, the old behaviour is retained, so it won't break existing code.

Hope it can be useful for others.

diff -u ESP8266WiFiAP.h.ori ESP8266WiFiAP.h

--- ESP8266WiFiAP.h.ori	2016-09-27 19:02:02.904865345 +0200
+++ ESP8266WiFiAP.h	2017-08-18 22:29:38.061691447 +0200
@@ -37,7 +37,7 @@
     public:
 
         bool softAP(const char* ssid, const char* passphrase = NULL, int channel = 1, int ssid_hidden = 0);
-        bool softAPConfig(IPAddress local_ip, IPAddress gateway, IPAddress subnet);
+        bool softAPConfig(IPAddress local_ip, IPAddress gateway, IPAddress subnet, uint32_t dhcp_start=0, uint32_t dhcp_end=0);
         bool softAPdisconnect(bool wifioff = false);
 
         uint8_t softAPgetStationNum();

diff -u ESP8266WiFiAP.cpp.ori ESP8266WiFiAP.cpp

--- ESP8266WiFiAP.cpp.ori	2016-09-27 19:02:02.900865537 +0200
+++ ESP8266WiFiAP.cpp	2017-08-18 22:38:50.736674336 +0200
@@ -175,8 +175,10 @@
  * @param local_ip      access point IP
  * @param gateway       gateway IP
  * @param subnet        subnet mask
+ * @param dhcp_start    DHCP range start
+ * @param dhcp_end      DHCP range end
  */
-bool ESP8266WiFiAPClass::softAPConfig(IPAddress local_ip, IPAddress gateway, IPAddress subnet) {
+bool ESP8266WiFiAPClass::softAPConfig(IPAddress local_ip, IPAddress gateway, IPAddress subnet, uint32_t dhcp_start, uint32_t dhcp_end) {
     DEBUG_WIFI("[APConfig] local_ip: %s gateway: %s subnet: %s\n", local_ip.toString().c_str(), gateway.toString().c_str(), subnet.toString().c_str());
     if(!WiFi.enableAP(true)) {
         // enable AP failed
@@ -201,11 +203,19 @@
 
     struct dhcps_lease dhcp_lease;
     IPAddress ip = local_ip;
-    ip[3] += 99;
+    if(dhcp_start) {
+        ip=(dhcp_start & !info.netmask.addr) + (info.ip.addr&info.netmask.addr);
+    } else {
+        ip[3] += 99;
+    }
     dhcp_lease.start_ip.addr = static_cast<uint32_t>(ip);
     DEBUG_WIFI("[APConfig] DHCP IP start: %s\n", ip.toString().c_str());
 
-    ip[3] += 100;
+    if(dhcp_end) {
+        ip=(dhcp_end & !info.netmask.addr) + (info.ip.addr&info.netmask.addr);
+    } else {
+        ip[3] += 100;
+    }
     dhcp_lease.end_ip.addr = static_cast<uint32_t>(ip);
     DEBUG_WIFI("[APConfig] DHCP IP end: %s\n", ip.toString().c_str());

[devyte]

Hi, I find it useful, it's something I was going to have to look into at some point. A suggestion: instead of dhcpstart and dhcpend as args, do dhcpstart and num, where num is how many addresses are allowed in the pool. Why? You don't have to check that the first three bytes of start and end match. Assuming nobody objects, can you prepare a PR with this?

…

On Aug 18, 2017 5:56 PM, "NdK" ***@***.***> wrote: Hello. I just noticed possible problems when calling WiFi.softAPConfig(). It assumes everybody uses a class-C network, a fixed mask of 24 bits and places the ESP at the start of the network (.1). If the address given is over 55, the DHCP range collides with the reserved address (.255) and/or the network address (.0). So I slightly modified the code to accept two optional arguments: dhcp_start and dhcp_end . That makes it possible for the user to choose more freely the address range to use (I still don't check for network/broadcast collisions: with a great power...), including a whole class-A if desired (not that it's really useful with at most 4 clients, but anyway...). If the optional arguments are not supplied, the old behaviour is retained, so it won't break existing code. Hope it can be useful for others. diff -u ESP8266WiFiAP.h.ori ESP8266WiFiAP.h --- ESP8266WiFiAP.h.ori 2016-09-27 19:02:02.904865345 +0200 +++ ESP8266WiFiAP.h 2017-08-18 22:29:38.061691447 +0200 @@ -37,7 +37,7 @@ public: bool softAP(const char* ssid, const char* passphrase = NULL, int channel = 1, int ssid_hidden = 0); - bool softAPConfig(IPAddress local_ip, IPAddress gateway, IPAddress subnet); + bool softAPConfig(IPAddress local_ip, IPAddress gateway, IPAddress subnet, uint32_t dhcp_start=0, uint32_t dhcp_end=0); bool softAPdisconnect(bool wifioff = false); uint8_t softAPgetStationNum(); diff -u ESP8266WiFiAP.cpp.ori ESP8266WiFiAP.cpp --- ESP8266WiFiAP.cpp.ori 2016-09-27 19:02:02.900865537 +0200 +++ ESP8266WiFiAP.cpp 2017-08-18 22:38:50.736674336 +0200 @@ -175,8 +175,10 @@ * @param local_ip access point IP * @param gateway gateway IP * @param subnet subnet mask + * @param dhcp_start DHCP range start + * @param dhcp_end DHCP range end */ -bool ESP8266WiFiAPClass::softAPConfig(IPAddress local_ip, IPAddress gateway, IPAddress subnet) { +bool ESP8266WiFiAPClass::softAPConfig(IPAddress local_ip, IPAddress gateway, IPAddress subnet, uint32_t dhcp_start, uint32_t dhcp_end) { DEBUG_WIFI("[APConfig] local_ip: %s gateway: %s subnet: %s\n", local_ip.toString().c_str(), gateway.toString().c_str(), subnet.toString().c_str()); if(!WiFi.enableAP(true)) { // enable AP failed @@ -201,11 +203,19 @@ struct dhcps_lease dhcp_lease; IPAddress ip = local_ip; - ip[3] += 99; + if(dhcp_start) { + ip=(dhcp_start & !info.netmask.addr) + (info.ip.addr&info.netmask.addr); + } else { + ip[3] += 99; + } dhcp_lease.start_ip.addr = static_cast<uint32_t>(ip); DEBUG_WIFI("[APConfig] DHCP IP start: %s\n", ip.toString().c_str()); - ip[3] += 100; + if(dhcp_end) { + ip=(dhcp_end & !info.netmask.addr) + (info.ip.addr&info.netmask.addr); + } else { + ip[3] += 100; + } dhcp_lease.end_ip.addr = static_cast<uint32_t>(ip); DEBUG_WIFI("[APConfig] DHCP IP end: %s\n", ip.toString().c_str()); — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#3532>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AQC6BhaJViFlGZHx4eTWFHPBFPUNk7A7ks5sZfqGgaJpZM4O8CQG> .

[devyte]

One other thing: you might consider passing in 0.0.0.0 and 0 to turn off dhcp altogether to allow static IPs.

…

On Aug 18, 2017 6:06 PM, "Develo Deveyes" ***@***.***> wrote: Hi, I find it useful, it's something I was going to have to look into at some point. A suggestion: instead of dhcpstart and dhcpend as args, do dhcpstart and num, where num is how many addresses are allowed in the pool. Why? You don't have to check that the first three bytes of start and end match. Assuming nobody objects, can you prepare a PR with this? On Aug 18, 2017 5:56 PM, "NdK" ***@***.***> wrote: > Hello. > > I just noticed possible problems when calling WiFi.softAPConfig(). > It assumes everybody uses a class-C network, a fixed mask of 24 bits and > places the ESP at the start of the network (.1). > If the address given is over 55, the DHCP range collides with the > reserved address (.255) and/or the network address (.0). > So I slightly modified the code to accept two optional arguments: > dhcp_start and dhcp_end . > That makes it possible for the user to choose more freely the address > range to use (I still don't check for network/broadcast collisions: with a > great power...), including a whole class-A if desired (not that it's really > useful with at most 4 clients, but anyway...). > If the optional arguments are not supplied, the old behaviour is > retained, so it won't break existing code. > > Hope it can be useful for others. > > diff -u ESP8266WiFiAP.h.ori ESP8266WiFiAP.h > > --- ESP8266WiFiAP.h.ori 2016-09-27 19:02:02.904865345 +0200 > +++ ESP8266WiFiAP.h 2017-08-18 22:29:38.061691447 +0200 > @@ -37,7 +37,7 @@ > public: > > bool softAP(const char* ssid, const char* passphrase = NULL, int channel = 1, int ssid_hidden = 0); > - bool softAPConfig(IPAddress local_ip, IPAddress gateway, IPAddress subnet); > + bool softAPConfig(IPAddress local_ip, IPAddress gateway, IPAddress subnet, uint32_t dhcp_start=0, uint32_t dhcp_end=0); > bool softAPdisconnect(bool wifioff = false); > > uint8_t softAPgetStationNum(); > > diff -u ESP8266WiFiAP.cpp.ori ESP8266WiFiAP.cpp > > --- ESP8266WiFiAP.cpp.ori 2016-09-27 19:02:02.900865537 +0200 > +++ ESP8266WiFiAP.cpp 2017-08-18 22:38:50.736674336 +0200 > @@ -175,8 +175,10 @@ > * @param local_ip access point IP > * @param gateway gateway IP > * @param subnet subnet mask > + * @param dhcp_start DHCP range start > + * @param dhcp_end DHCP range end > */ > -bool ESP8266WiFiAPClass::softAPConfig(IPAddress local_ip, IPAddress gateway, IPAddress subnet) { > +bool ESP8266WiFiAPClass::softAPConfig(IPAddress local_ip, IPAddress gateway, IPAddress subnet, uint32_t dhcp_start, uint32_t dhcp_end) { > DEBUG_WIFI("[APConfig] local_ip: %s gateway: %s subnet: %s\n", local_ip.toString().c_str(), gateway.toString().c_str(), subnet.toString().c_str()); > if(!WiFi.enableAP(true)) { > // enable AP failed > @@ -201,11 +203,19 @@ > > struct dhcps_lease dhcp_lease; > IPAddress ip = local_ip; > - ip[3] += 99; > + if(dhcp_start) { > + ip=(dhcp_start & !info.netmask.addr) + (info.ip.addr&info.netmask.addr); > + } else { > + ip[3] += 99; > + } > dhcp_lease.start_ip.addr = static_cast<uint32_t>(ip); > DEBUG_WIFI("[APConfig] DHCP IP start: %s\n", ip.toString().c_str()); > > - ip[3] += 100; > + if(dhcp_end) { > + ip=(dhcp_end & !info.netmask.addr) + (info.ip.addr&info.netmask.addr); > + } else { > + ip[3] += 100; > + } > dhcp_lease.end_ip.addr = static_cast<uint32_t>(ip); > DEBUG_WIFI("[APConfig] DHCP IP end: %s\n", ip.toString().c_str()); > > > — > You are receiving this because you are subscribed to this thread. > Reply to this email directly, view it on GitHub > <#3532>, or mute the thread > <https://github.com/notifications/unsubscribe-auth/AQC6BhaJViFlGZHx4eTWFHPBFPUNk7A7ks5sZfqGgaJpZM4O8CQG> > . >

[NdK73]

Well, I don't need to actually check, since I apply the usual net-math (and with negated netmap to extract the net address, then add that to network address).
I've never done a PR, but it shouldn't be a big issue :)
I'm using the default of 0 for "use old behaviour".

[devyte]

Apologies, I'm not a network guy, so I have no idea about network math. I'd love to learn though :P About using 0 for old behavior: in that case, an alternative is to overload the method with a new one that has the additional parameters, and factor out the common code into a private method. That way you get the best of both worlds. On Aug 18, 2017 6:12 PM, "NdK" <notifications@github.com> wrote: Well, I don't need to actually check, since I apply the usual net-math (and with negated netmap to extract the net address, then add that to network address). I've never done a PR, but it shouldn't be a big issue :) I'm using the default of 0 for "use old behaviour". — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#3532 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AQC6BuBqLSOk-7Tfj-G_dE2P9F5tcGREks5sZf4-gaJpZM4O8CQG> .

[NdK73]

@igrr Comments? Can it be included in the upcoming release?

[devyte]

I was just looking at this. I took a crash course of IP network math, and I now understand what you did.

I have two suggestions.

1. Don't use uint32_t type for the new args . Use IPAddress instead, that is the type used for the current ip/mask/gway args. The args can always be static_cast to uint32 to do the math you did.
2. Rather than add the optional arguments with defaults, implement a method overload:

bool ESP8266WiFiAPClass::softAPConfig(IPAddress local_ip, IPAddress gateway, IPAddress subnet, IPAddress dhcp_start, IPAddress dhcp_end)
{
...
//function body of current code, but do the setup with all args as required
}

bool ESP8266WiFiAPClass::softAPConfig(IPAddress local_ip, IPAddress gateway, IPAddress subnet)
{
//do your network math to calculate dhcp_start and dhcp_end based on local_ip and subnet
  return softAPConfig(local_ip, gatewaym subnet, dhcp_start, dhcp_end);
}

[NdK73]

Seems the answer to the mail got lost, so I'll insert it manually.
I used uint32 because I intended 'em as offsets, relative to the network base address.
So, if you have an address like 192.168.1.4/24 you had to pass just 100 and 199 as dhcp start and dhcp_end, not the whole addresses.
Well, actually there's no problem if you pass the whole address, but I think it's more error-prone.
I tried keeping the mods to a minimum, hence the defaults. I'm now working on a more complete solution in line with your suggestions.

[NdK73]

Created a PR.
Couldn't test as exensively as I'd like, but seems OK. Hope it can be included in upcoming release.

[devyte]

@NdK73 Thanks for the PR. I'll be testing this myself at some point, I'll let you know as soon as I do.

[ncmreynolds]

The issue still exists, did anything happen with the PR above? I was just thinking of doing a solution myself but the one above, passing a start and end IP seems perfectly reasonable if tidily integrated.

[d-a-v]

#3562 was reverted 4 days after merging.
Please open a new issue about it.