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

DHCP custom option #8582

Merged
merged 19 commits into from
Jun 8, 2022
Merged

DHCP custom option #8582

merged 19 commits into from
Jun 8, 2022

Conversation

mcspr
Copy link
Collaborator

@mcspr mcspr commented May 27, 2022

@bwjohns4
re comments in #8571

@mcspr mcspr mentioned this pull request May 27, 2022
@d-a-v
Copy link
Collaborator

d-a-v commented May 30, 2022

To the risk of being pedantic, If there a way to retrieve user option from flash ?

@mcspr
Copy link
Collaborator Author

mcspr commented May 31, 2022

Sure, but what is the use-case anyway?
Playing around with the code here... We could have static opts as static vars, sure, but then we can't have anything dynamic that way. e.g. based on the netif IP, or some other service info we would want to add.

Suppose, we have #8451 with a pointer attached to the instance instead

    dhcpSoftAP.custom_offer_options = [](auto& options, const DhcpServer& server) {
        options.add(/* code = */ 43, /* data = */ {0xca, 0xfe, 0xca, 0xfe, 0xfe});

        char clientId[32];
        ... somehow generate it from server._netif data ...
        options.add(93, reinterpret_cast<const uint8_t*>(&clientId[0]), len);
    };

Where auto& options is the magic handler with size + code checks from above.

@d-a-v
Copy link
Collaborator

d-a-v commented May 31, 2022

My fingers always hurt when storing an array in a source code, which is copied on boot from flash to ram, then by code from ram to ram. Data are there three times, so this was my question. Of course some dhcp configuration blocks are dynamic, so it should not be prevented to deal with them.
I was thinking of something like memcpy_P(optptr, &option.data.begin(), option.data.length()) instead of this loop

        for (auto it = option.data.begin(); it != option.data.end(); ++it) {
            *optptr++ = *it;

But yes, dhcp data blocks are often dynamic and among the static ones not many are long enough to get a real ram gain.
So yes, it was a pedantic question 😛

@mcspr
Copy link
Collaborator Author

mcspr commented May 31, 2022

With the above, add() could accept u8 pointer then and do memcpy.
Let me fight the clang-format out first... It does some weird stuff with the built-in options when processing them as function arguments.

@mcspr
Copy link
Collaborator Author

mcspr commented May 31, 2022

Updated. Since it is also a showcase of the existing options helpers, size of changes... increased (sry).
Arrays could be anywhere. Initializer list, stack, heap, etc.

Overloading add() was sure fun, but idk how obvious it is without code completion and syntax helpers. add_<type> might be more self-explanatory.

mcspr added 3 commits May 31, 2022 15:34
/to d-a-v it has automatic storage, here it's the same stack based one
(just one less line for us)
Copy link
Collaborator

@d-a-v d-a-v left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one !

@mcspr mcspr marked this pull request as ready for review June 8, 2022 21:06
@d-a-v d-a-v merged commit b7c1cfb into esp8266:master Jun 8, 2022
@mcspr mcspr deleted the dhcps-opts branch June 9, 2022 09:31
@blue-wind-25
Copy link

Hello,

In 'LwipDhcpServer.cpp' line 444 I found:
std::memcpy((u8_t*)q->payload, reinterpret_cast<u8_t*>(&m), q->len);

shouldn't that be:
std::memcpy((u8_t*)q->payload, (u8_t*)m, q->len);

(use 'm' instead of '&m')?

Also, in 'ESP8266WiFiAP.cpp', the call to wifi_softap_dhcps_stop() in ESP8266WiFiAPClass::softAPConfig() has been removed. Shouldn't that be kept there so that the DHCPS from the SDK is kept turned off?

In previous cores, I can use custom IP such as '192.168.0.XXX' using DHCP in access point mode, but if wifi_softap_dhcps_stop() is removed then that custom IP will not work.

Could you verify those two? Thank you!

@mcspr
Copy link
Collaborator Author

mcspr commented Jun 13, 2022

shouldn't that be:
std::memcpy((u8_t*)q->payload, (u8_t*)m, q->len);

it should. just a bad c/p

Also, in 'ESP8266WiFiAP.cpp', the call to wifi_softap_dhcps_stop() in ESP8266WiFiAPClass::softAPConfig() has been removed. Shouldn't that be kept there so that the DHCPS from the SDK is kept turned off?

probably? iirc, there's no difference between server.end() and wifi_softap_dhcps_stop() besides wifi_softap_dhcps_status() getting changed. it ends up calling dhcps_stop() which is proxied to server.end() anyway, while also doing some extraneous checks we don't really care about
yeah, it also should. on a second look, it won't set our network info without it

mcspr added a commit to mcspr/esp8266-Arduino that referenced this pull request Jun 14, 2022
As noticed in esp8266#8582 (comment)
Plus, handle the case when `pbuf->len` is less than struct size
mcspr added a commit to mcspr/esp8266-Arduino that referenced this pull request Jun 14, 2022
As noticed in esp8266#8582 (comment)
Can't really use `server.begin()` and `server.end()` directly, only
default static IP is applied to the interface since DHCP server is
deemed 'running' (see `wifi_softap_dhcps_status()` return value)
mcspr added a commit that referenced this pull request Jun 27, 2022
* Fix sending NACK, use helper function to fill pbuf

As noticed in #8582 (comment)
Plus, handle the case when `pbuf->len` is less than struct size

* Make sure to call SDK functions to start and stop DHCP server

As noticed in #8582 (comment)
Can't really use `server.begin()` and `server.end()` directly, only
default static IP is applied to the interface since DHCP server is
deemed 'running' (see `wifi_softap_dhcps_status()` return value)

* s

Co-authored-by: david gauchard <[email protected]>
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.

None yet

3 participants