search

tobozo / WiFiManagerTz

A NTP/Timezone extension to @tzapu's WiFiManager
Other
7 stars 2 forks source link

Does not Automatically Apply Time Configuration on Start #5

Closed ab-tools closed 9 months ago

ab-tools commented 9 months ago

Hello tobozo,

cool WiFiManager addition, thanks for that! :-)

Quick question how this is expected to work: Currently, after restarting it seems to load the preferences correctly, it correctly finds out that NTP is enabled here https://github.com/tobozo/WiFiManagerTz/blob/master/src/WiFiManagerTz.h#L36 and loads the NTP preferences accordingly.

However, it seems not to automatically apply the time configuration from the preferences at start. This leads to the preferences correctly filled on the WiFiManager custom web page, but the time not used. I need to go to the time settings webpage after each start and click on "Submit" (without any changes in the form as all correctly prefilled). Only then the time gets actually updated.

Would it not make sense to add a configTime(); after NTP::loadPrefs(); here? https://github.com/tobozo/WiFiManagerTz/blob/master/src/WiFiManagerTz.h#L36

This would immediately apply the saved settings and retrieve an initial time from the NTP server.

Thanks Andreas

tobozo commented 9 months ago

hi, thanks for your feedback :+1:

configTime() triggers a connection, which would come too early when the wifiManager is still waiting for other settings (e.g. first launch), and we don't want that

In the basic example, WiFiManagerNS::configTime() is called only after a wifi connection is established

if you're not using WifiManager::autoConnect() feature, I suggest you put the call to WiFiManagerNS::configTime() in a conditional block in your setup i.e. when all your network requirements are met and the NTP server is reachable

ab-tools commented 9 months ago

Hello tobozo,

appreciating your really quick feedback, thanks a lot!

And that makes a lot of sense what you're writing. I've added the WiFiManagerNS::configTime() call in a conditional block in the end of the setup method as you suggested.

I have one more independent suggestion/comment, but will create another GitHub issue to keep things separated. ;-)

Closing this one, thanks again for your quick support on that! Andreas