fix: the dhcp client behaviour is not standard compliant for DHCP clients (RFC2131) (IDFGH-5139)

[1e1]

fix #50

[d-a-v]

What do you think of this comment ?

[1e1]

Dear,

I don't think it is the right way to fix a standard compliance issue.
So it left the custom hook for custom addons.
Moreover it almost match what is forked on espressif/esp-lwip => espressif/esp-lwip@2195f74 (and I wonder why esp8266/Arduino don't use this fork)

By the way, I agree LwIP skips this standard compliance to be very light. But on IoT performance, getting an IP by DHCP is not recommended. So that I thought we can stuff LwIP with that (and it solves auto-registered DNS then it helps IoT networks along).

I advise to merge the PR as it :)

[d-a-v]

My Friend,

I usually accept in this repository any patch against lwIP, only when such patch is meant to fix a lwIP bug or to be submitted upstream for feature addition, as it already happened.

Your patch could be qualified for both but there are some names that are unrelated to lwIP ESP_DHCP and ESP_DHCP_DISABLE_CLIENT_ID. If this is specific to ESP, then LWIP_HOOK_DHCP_APPEND_OPTIONS should be used instead.

Regarding espressif-lwip, the first commit in their repository happened a year after esp82xx-nonos-linklayer, which title is lwIP-2 from original source.

My feeling is that they are repeating the same error as they did with lwIP-1.4 for esp8266. I know that it sounds unreasonable from a single man like me but I have reasons (and sweat). Their original modifications on esp8266-nonos-sdk-lwIP-1.4 had to be tracked and pulled out one by one from real sources to find out what they did in order to apply them to lwIP-2. Improvements and bugfixes for lwIP are meant to go upstream, and their way of working is patching. Also espressif-lwip has a funny way for giving credits to original authors: see napt commit on espressif-lwip vs napt here.

For the previous reasons, I can't merge this pull request as-is.
However if you don't have time to work further on it, I can finish the work for you ?

[1e1]

Hi @d-a-v,

Thanks a lot for your explanations. It's easier to know the philosophy of the project for contributing.
(For a side conversation, what is the future of the 'patches' directory?)
I have updated the PR.

I hold a doubt: I had include a 'prot' .h for writing the new function. Is it allowed?
https://github.com/d-a-v/esp82xx-nonos-linklayer/pull/51/files#diff-d8e5ead5871aaae10a4e661302a63e9a88edee4bb90d5422a85d950935d52031R35

I have prepare several part into the hook. Perhaps it should an overkill code:
https://github.com/d-a-v/esp82xx-nonos-linklayer/pull/51/files#diff-d8e5ead5871aaae10a4e661302a63e9a88edee4bb90d5422a85d950935d52031R587

I hope we are more aligned.

[someburner]

By the way, I agree LwIP skips this standard compliance to be very light. But on IoT performance, getting an IP by DHCP is not recommended. So that I thought we can stuff LwIP with that (and it solves auto-registered DNS then it helps IoT networks along).

@1e1 can you comment more on this? Do you think not providing mac/hostname in the DHCP request could cause DHCP to fail on some routers?

[1e1]

@someburner
No. The DHCP ability works: it give a IP config only.
But some routers are DNS server too. And for that, they like getting the client hostname and the client identifier. It's a "complex" device.

[d-a-v]

Thanks!