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

NTP: use sntp app from lwip on latest Cores, replace ntpclientlib #2132

Merged
merged 37 commits into from
Feb 4, 2020

Conversation

mcspr
Copy link
Collaborator

@mcspr mcspr commented Jan 30, 2020

subj

fix #1380 (yay)
fix #1517
fix #1629

TODO:

  • timebroker. can it be used as scheduler replacement + rpn? experimenting with $tick1m, $tick1h.
    slightly complicated because of restore-last-schedule since rpn is generic and scheduler part would need to iterate over $tick rules. Can use a different rpn context and ruleset / settings, maybe.
  • fix lib_deps forcibly linking ntpclientlib + timelib. if not possible, drop 2.3.0 from supported altogether. if it does work, just switch implementations as needed. webui can have ntplegacyVisible hint to hide offending parts or we could also have a separate panel.
  • localtime / gmtime make for some weird interactions
  • timelib compat types are all over the place. uptime tries to be fancy, so elapsed... need to accept unsigned and not only time_t

@mcspr mcspr mentioned this pull request Jan 31, 2020
3 tasks
@mcspr
Copy link
Collaborator Author

mcspr commented Jan 31, 2020

Also something of note that ntpStartDelay needs to be >= 15 seconds. 0 option also works though, bot nothing in-between 0 and 15
Based on RFC, it has to be at least 15s. Having re-checked the code, it never enforces it. Random re-tuned a bit, since it does actually wait. Probably needs further reducing

@Niek
Copy link
Contributor

Niek commented Jan 31, 2020

Nice job 👍🏻
Any improvements in build size after dropping ntpclientlib?

@mcspr
Copy link
Collaborator Author

mcspr commented Jan 31, 2020

None, really. Re-enabling scheduler code, travis shows the same size for basic.h build.
+rpnlib -scheduler is ~10KB as noted in the comment
-scheduler is ~4KB, 1KB more than the comment notice
-legacy ntp code (adding tz, removing offsets) in html/js is 240 bytes total

@mcspr
Copy link
Collaborator Author

mcspr commented Jan 31, 2020

In general ntpLocal2UTC() makes no sense if it wants to return timestamp. Timestamp itself is already UTC, where localtime modifies tm struct based on TZ when we use it.

Not really looking too portable, since there needs to be a code chunk using libc functions instead of timelib. I guess it can temporarily setenv("TZ", "UTC0"); dowork(); setenv("TZ", "whatever"); if that is enough to apply the change to localtime result, so minute() / hour() / day() work. Scheduler references timestamps a lot, which needs to be refactored to use time struct

@mcspr mcspr changed the title [WIP] NTP: try using sntp app from lwip, drop ntpclientlib NTP: use sntp app from lwip on latest Cores, replace ntpclientlib Feb 3, 2020
Copy link
Collaborator Author

@mcspr mcspr left a comment

Choose a reason for hiding this comment

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

Timelib helpers are working, scheduler days are in order.
Everything builds even with lib_deps referencing NtpClientLib, only a matter of reading platformio documentation more closely on how it parses dependencies.

if (cfg_tz != active_tz) {
setenv("TZ", cfg_tz.c_str(), 1);
tzset();
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since configTime re-inits sntp, set timezone through tzset(3)

code/espurna/ntp.ino Outdated Show resolved Hide resolved
code/espurna/ntp.ino Outdated Show resolved Hide resolved
Comment on lines +335 to +337
DEBUG_MSG_P(PSTR("[NTP] Updating `ntpServer` setting from DHCP: %s\n"), server.c_str());
_ntp_server = server;
setSetting("ntpServer", server);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Dhcp routine will call sntp_init() whenever dhcp server sets ntp option:
#1828
I also wonder if this is the reason that sntp can never be optimised away

Comment on lines +363 to +365
// TODO:
// terminalRegisterCommand(F("NTP.SYNC"), [](Embedis* e) { ... }
//
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can't exactly speed-up sntp like NtpClientLib, so idk about this.
Changing ntpServer value also triggers sync

// TODO: need this prototype for .ino
struct NtpCalendarWeekday;

#if NTP_SUPPORT
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

BTW some benefit of having #if guards in headers is that compilation will fail instead of linker when function is not implemented / supported

@mcspr mcspr merged commit ba3ec47 into xoseperez:dev Feb 4, 2020
@mcspr mcspr deleted the ntp/sntp-lwip branch February 4, 2020 03:17
@mcspr mcspr added this to the 1.14.2 milestone Mar 13, 2020
@mcspr mcspr mentioned this pull request Mar 14, 2020
@bluexp1
Copy link

bluexp1 commented Jul 29, 2021

I am currently using 1.14.1 and ntp fails to get time from the internet, Is there any solution available to make it work with current version or I need to wait for 1.14.2 to get released?

@mcspr
Copy link
Collaborator Author

mcspr commented Jul 29, 2021

download the dev branch and use the latest version? or nightly builds to use binaries?
I'd also suggest to use gitter or just open a new issue for any questions, closed issues / PRs are not easy to track for new discussions.

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