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

DHCPS included in the Core and the old SDK implementation #8307

Closed
6 tasks done
mcspr opened this issue Sep 9, 2021 · 3 comments · Fixed by #8546
Closed
6 tasks done

DHCPS included in the Core and the old SDK implementation #8307

mcspr opened this issue Sep 9, 2021 · 3 comments · Fixed by #8546

Comments

@mcspr
Copy link
Collaborator

mcspr commented Sep 9, 2021

Basic Infos

  • This issue complies with the issue POLICY doc.
  • I have read the documentation at readthedocs and the issue is not addressed there.
  • I have tested that the issue is present in current master branch (aka latest git).
  • I have searched the issue tracker for a similar issue.
  • If there is a stack dump, I have decoded it.
  • I have filled out all fields below.

Platform

  • Hardware: any
  • Core Version: 3.0.x

Problem Description

Something noticed while building lwip2 here

Since #6680 (51c2a14#diff-39ab7a280ef46486a19b45c179a5a160a836d57ab164f627ade75cb92ee94d54), old lwip-bundled dhcps was extracted and rewritten into a class

Current example for SoftAP replaced the original calls with the Arduino-API-like global object:

first call to wifi_softap_add_dhcps_lease() will setup first IP address of the range
second call to wifi_softap_add_dhcps_lease() will setup second IP address of the range
...
any client not listed will use next IP address available from the range (here 192.168.0.102 and more)
*/
dhcpSoftAP.add_dhcps_lease(mac_CAM); // always 192.168.0.100
dhcpSoftAP.add_dhcps_lease(mac_PC); // always 192.168.0.101

And the internal WiFi code is also forced to work through the wrapper:
if(!dhcpSoftAP.set_dhcps_lease(&dhcp_lease))

While the original functions are still in the SDK header. Since the lwip1 code is removed, this is just a linking error for the existing programs pre 3.0.0 that use any of wifi_softap_..._dhcps_... funcs

#if 1 // dhcp server
// these functions are open-source, in dhcp server,
// which is now moved to lwIPDhcpServer.cpp (lwip2)
// (but still there with lwip1)
bool wifi_softap_set_dhcps_lease(struct dhcps_lease *please);
bool wifi_softap_get_dhcps_lease(struct dhcps_lease *please);
uint32 wifi_softap_get_dhcps_lease_time(void);
bool wifi_softap_set_dhcps_lease_time(uint32 minute);
bool wifi_softap_reset_dhcps_lease_time(void);
bool wifi_softap_add_dhcps_lease(uint8 *macaddr); // add static lease on the list, this will be the next available @
#endif // dhcp server

I'd propose keeping the SDK API, but still have a custom class for an netif when actually warranted (maybe in cases like Ethernet?). This will also remove the need for the WiFi class code to reference the SoftAP DHCPS through an Arduino sketch-like API (...which also means, there will be no need to have headers with carefully placed externs)

cc @d-a-v

also, there is slightly more up-to-date rtos sdk implementation (which also has english comments)
https://github.com/espressif/ESP8266_RTOS_SDK/blob/master/components/lwip/apps/dhcpserver/dhcpserver.c

but, it is interesting that the esp32 variant of idf does not include any alternative to add/remove reserved mac feature
https://github.com/espressif/esp-idf/blob/master/components/lwip/include/apps/dhcpserver/dhcpserver.h

@d-a-v
Copy link
Collaborator

d-a-v commented Sep 13, 2021

#6680 was merged before lwIP-v1 was removed.
So I think these functions from user_interface.h could be safely hidden or removed.
A prerelease of lwIP-2.1.3 is out, changes can happen at that time

@mcspr
Copy link
Collaborator Author

mcspr commented Sep 17, 2021

So I think these functions from user_interface.h could be safely hidden or removed.

I actually meant to restore them + slightly adjust the additional code introduced by the lib. Or it's a no-go?
Roughly this -> https://github.com/esp8266/Arduino/compare/master...mcspr:issue8307?expand=1

@mcspr
Copy link
Collaborator Author

mcspr commented Feb 20, 2022

Another (possibly minor) note about using objects. We are in C++ environment, and we also may run into an issue of init order when dealing with state inside of certain objects declared across the Core

https://gcc.gnu.org/onlinedocs/gcc-10.3.0/gcc/C_002b_002b-Attributes.html#index-init_005fpriority-variable-attribute
While Ctor & Dtor order is stable inside of a single translation unit (aka .cpp -> .o), we can't guarantee order between the Core libraries, Core startup and the sketch itself and it depends purely on the order object files are processed

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 a pull request may close this issue.

2 participants