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

Add timezone support with daylight saving time #604

Merged
merged 2 commits into from
Nov 14, 2023
Merged

Add timezone support with daylight saving time #604

merged 2 commits into from
Nov 14, 2023

Conversation

matjack1
Copy link
Collaborator

This PR removes the custom NTP code, to use the standard Arduino library one.

This timezone support uses Posix strings (full list here: https://github.com/nayarsystems/posix_tz_db/blob/master/zones.csv ) to set timezone with daylight saving time.

This should support all the variations and it's quite simple to use.

Now the device will always send epoch and the browser should render the time correctly.

Daylight saving time should be handled automatically based on the info provided by the TZinfo string.

If your timezone is missing, please add a string in the option, I've just added a few as an example, I didn't want to add them all as it's unnecessary and makes it more complex.

@omersiar
Copy link
Collaborator

omersiar commented Nov 6, 2023

Nicely done, I remember custom NTP code is there because esp would crash while gethostname syscall if it is not an async call.

https://github.com/esprfid/esp-rfid/pull/604/files#diff-fd13b961cc105f5ca6bb61a874fbf0796599463141be44cd67fb9dd82887646dL36

@matjack1
Copy link
Collaborator Author

matjack1 commented Nov 6, 2023

hey @omersiar very nice to see you around!! :)

Is the gethostname syscall done internally somewhere? I cannot find it in the code that I have added, what are you referring to? I've tried this code and seems to be working to me, do you have any specific pointer?

Also, while you are here, I'm going to open in the next few days a PR from dev to stable as I'm happy where we are and I would like to release a stable v2. I am still doing some refinements, but it's getting close. Maybe stay tuned in the next few days if you can! Thank you very much :)

@omersiar
Copy link
Collaborator

omersiar commented Nov 7, 2023

Nice to see you too, The issue with the synchronous NTP may be reproducible by a PN532 reader since it takes longer to read modern cards, watchdog resets the CPU and the offender was always gethostbyname() method, this was back then and may no longer an issue.

NTP library should not try to update the clock from a remote server synchronously while reading a card, these methods would block CPU long enough to cause a watchdog reset.

Some libraries were tracking last synchronization time and offer a threshold where it determines when to update, if it coincide with a card read it may reset.

Anyway, it seems a lot has changed and there was no lwip library back then.
https://github.com/esp8266/Arduino/blob/fb8d6d668df95e138d6248e7239cbf98c77f8174/tools/sdk/lwip2/include/lwip/apps/sntp.h#L67

So it may not block and I could not able to verify if it is async or not.

@matjack1
Copy link
Collaborator Author

matjack1 commented Nov 7, 2023

@omersiar maybe we could call the NTP server not every 10 seconds but less, like every 10 minutes, to minimize the number of calls and potential problems

@matjack1
Copy link
Collaborator Author

matjack1 commented Nov 7, 2023

@omersiar thinking more about this, since the call to getNTPtime happens in the loop, it should not conflict with the reads from PN532. The loop can be blocked by async stuff, I think it should be safe?

If you are fine merging this in dev, then I have the dev branch ready for opening a PR on stable! I've updated in dev the documentation and the upgrade path from V1 :)

@omersiar
Copy link
Collaborator

If you did testing this on real hardware and everything is fine, let's merge.

@matjack1
Copy link
Collaborator Author

Yes! Thanks :)

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.

2 participants