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

Setting a callback for SNTP time sync event [feature request] #7616

Closed
5 of 6 tasks
JakubRakus opened this issue Sep 30, 2020 · 2 comments · Fixed by #7637
Closed
5 of 6 tasks

Setting a callback for SNTP time sync event [feature request] #7616

JakubRakus opened this issue Sep 30, 2020 · 2 comments · Fixed by #7637

Comments

@JakubRakus
Copy link

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: [ESP-12]
  • Core Version: [2.7.4]
  • Development Env: [Platformio]
  • Operating System: [Ubuntu]

Settings in IDE

  • Module: [Nodemcu]
  • Flash Mode: [dio]
  • Flash Size: [4MB]
  • lwip Variant: [v2]
  • Reset Method: [nodemcu]
  • Flash Frequency: [40Mhz]
  • CPU Frequency: [80Mhz]
  • Upload Using: [SERIAL]
  • Upload Speed: [115200] (serial upload only)

Problem Description

Function settimeofday_cb() sets a callback executed every time when settimeofday() is invoked. SNTP synchronization event change system time using settimeofday() - which is ok, but if I run settimeofday() manually also callback function is invoked. There is no mechanism to show what was the source of callback - network synchronization or manual set.

Expected Behavior

I can see at least three possible solutions:

  1. Add a parameter to callback function declaration - maybe some pointer to bool type variable which will be set to true if SNTP was a source of sync.
  2. Add another settimeofday-like function which do the same, but do not fire any callback.
  3. Expose ESP SDK function sntp_set_time_sync_notification_cb() from lwip/apps/sntp/sntp.c which sets a callback ONLY for SNTP synchronization.

Personally I prefer the 3rd one (only because I tested it in "plain" not "Arduino-based" ESP projects and works like a charm).

@d-a-v
Copy link
Collaborator

d-a-v commented Sep 30, 2020

  1. Possible but that would break the current API
  2. Possible but that would be a pity because settimeofday is part of the posix standard.
  3. I was not aware and found no documentation of sntp_set_time_sync_notification_cb() in nonos-sdk. Let's try anyway to not further depend on ESP API.

There is a 4:

  • Do not call the callback when user is setting time with settimeofday()
    (this requires a change in lwip2)
    This also breaks the API but in a less invasive way: only when user explicitely sets time and relies on this CB.

@JakubRakus
Copy link
Author

@d-a-v

2. Possible but that would be a pity because `settimeofday` is part of the posix standard.

Don't think it's "pity". It's just a new Arduino specific additional funcionality. Plus it don't breaks current API.

3. I was not aware and found no documentation of `sntp_set_time_sync_notification_cb()` in nonos-sdk. Let's try anyway to not further depend on ESP API.

I checked again and you are right, sntp_set_time_sync_notification_cb() is only in RTOS-SDK.

* Do not call the callback when user is setting time with `settimeofday()`
  (this requires a change in lwip2)
  This also breaks the API but in a less invasive way: only when user explicitely sets time _and_ relies on this CB.

May be a solution, but don't think is "less invasive". If there are existing apps (I bet they are) that somehow relies on settimeofday callback, devs wouldn't be happy about that - need to remember that they have to invoke existing callback on their own after settimeofday explicit usage.

Anyway, any working solution will be appreciated. It should address problems like syncing with external RTC, setting time by the end user via some button/display interface etc. which were, and still are, discussed here on Github issues, Arduino forums and other places.

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