esprfid / esp-rfid

ESP8266 RFID (RC522, PN532, Wiegand, RDM6300) Access Control system featuring WebSocket, JSON, NTP Client, Javascript, SPIFFS
MIT License
1.35k stars 423 forks source link

Add timezone support with daylight saving time #604

Closed matjack1 closed 10 months ago

matjack1 commented 11 months ago

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 commented 11 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

@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 commented 10 months ago

@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 commented 10 months ago

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

matjack1 commented 10 months ago

Yes! Thanks :)