Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IP math #3637

Closed
wants to merge 4 commits into from
Closed

IP math #3637

wants to merge 4 commits into from

Conversation

NdK73
Copy link
Contributor

@NdK73 NdK73 commented Sep 22, 2017

Logical 'not' instead of bit-inversion... Sigh.

@bryceschober
Copy link
Contributor

FWIW, Even with this fix, the underlying support for user-supplied DHCP range seems not to work. My ESP8266 SoftAP continues to give my PC an IP address of local PI + 1, not +99 as the default start range attempts.

@NdK73
Copy link
Contributor Author

NdK73 commented Sep 22, 2017

Which parameters are you using?
Does it give you some messages when enabling debugging?

@bryceschober
Copy link
Contributor

I'm using the original softAPConfig() interface that provides default <local_subnet>.100 DHCP start address. It does emit the expected ""[APConfig] DHCP IP start" and "[APConfig] DHCP IP end" messages, but still leases <local_subnet>.2 to my PC when I connect.

@bryceschober
Copy link
Contributor

And no, it doesn't emit any "[APConfig]" error messages.

@NdK73
Copy link
Contributor Author

NdK73 commented Sep 23, 2017

So, IIUC, you get
[APConfig] DHCP IP start: 192.168.4.100
[APConfig] DHCP IP end: 192.168.4.200
but it still assigns your client 192.168.4.2 ?
Seems something outside of what I changed. Trying to trace it...

@NdK73
Copy link
Contributor Author

NdK73 commented Sep 23, 2017

Uhm... Found ugly code in tools/sdk/lwip/src/app/dhcpserver.c:wifi_softap_set_dhcps_lease : it always assumes a /24 mask.
/config ip information must be in the same segment as the local ip/
softap_ip >>= 8;
if ((start_ip >> 8 != softap_ip)
|| (end_ip >> 8 != softap_ip)) {
return false;
}
That code have to be commented out. But it's quite unrelated to this behaviour: even if I set a /24 mask it doesn't set the right range. Seems the call to wifi_softap_dhcps_start() resets the range... I'll have a look at it tomorrow.

@devyte
Copy link
Collaborator

devyte commented Sep 23, 2017

There's a branch with lwip2. Maybe lwip2 behaves better?

@NdK73
Copy link
Contributor Author

NdK73 commented Sep 24, 2017

I'm still quite new to git and github... Just basic usage. How can I test with LWIP2 branch? And will it be included in 2.4.0?

@NdK73
Copy link
Contributor Author

NdK73 commented Sep 24, 2017

Found another snippet of code in wifi_softap_init_dhcps_lease() that repeats the same check as above.
But the problem seems that wifi_softap_dhcps_start is only in libmain.a (and other bin-only .a files) and does not call dhcps_start in lwip/app/dhcpserver.c . Or, at least, I couldn't make os_printf-s in dhcp_start print anything even after recompiling liblwip_gcc.a .
Can someohe give me an hint, please?

@bryceschober
Copy link
Contributor

@igrr Could we get either this PR merged or bdf2296 reverted?

@igrr
Copy link
Member

igrr commented Sep 25, 2017

Ok, i'll try to find some information about wifi_softap_dhcps_start, and will revert the original commit in the meantime.

@igrr
Copy link
Member

igrr commented Sep 25, 2017

Reverted in eb891cd.

@devyte
Copy link
Collaborator

devyte commented Oct 22, 2017

@NdK73 have you made any progress with this?

@devyte
Copy link
Collaborator

devyte commented Nov 24, 2017

@NdK73 latest git now allows building with lwip2. Can you still pursue this?

@d-a-v
Copy link
Collaborator

d-a-v commented Nov 24, 2017

dhcps_start() is called. @NdK73 os_printf()s are displayed with Serial.setDebugOutput(true).
There are now two dhcp-server source files, the first one you know (lwip-1.4), the second is in lwip2/builder/glue-lwip/esp-dhcpserver.c once sources are downloaded with
cd tools/sdk/lwip2; make install (that you can call after patching sources too).
Fortunately (or not), the second one is a carbon-copy-a-bit-fixed version of the first one.

@NdK73
Copy link
Contributor Author

NdK73 commented Nov 24, 2017

Tks for the hint. I'll have a lok asap (currently I'm working on reading ADC from an ISR w/o disabling WiFI... something I'll probably put in another "issue" when ready).

@devyte
Copy link
Collaborator

devyte commented Dec 28, 2017

@NdK73 did you have some time to look at this?

@NdK73
Copy link
Contributor Author

NdK73 commented Dec 28, 2017

Not yet, sorry. Currently looking at #4028 that's a showstopper and would speed up considerably the debugging process (OTA is faster than serial...).
But still on the todo list.

@d-a-v d-a-v self-assigned this Jan 14, 2018
@devyte
Copy link
Collaborator

devyte commented Mar 16, 2018

@NdK73 This has a conflict now.

@d-a-v d-a-v removed their assignment Mar 18, 2018
@earlephilhower
Copy link
Collaborator

@d-a-v while the original patch seems to make sense, the current code doesn't even have the broadcast address anywhere in it when I try and resolve the merge conflicts, so there are no changes to apply.

If things have been refactored, maybe you should close this as no longer applicable.

@d-a-v
Copy link
Collaborator

d-a-v commented Oct 5, 2018

You are right the patch makes sense.
I will try to fix the conflicts by manually editing.

@NdK73
Copy link
Contributor Author

NdK73 commented Oct 5, 2018

Sorry for being silent for so long.
I tried various changes, but everytime found some problem rooted in the proprietary blob :( So I left it open hoping someone more knowledgeable could have a look. Maybe after (at least) two releases of the blob something changed.

@earlephilhower
Copy link
Collaborator

Closing since it seems this portion of the code has been completely redone. The original bug fix was good, but there's no comparable portion anywhere in existing code with the same problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants