- Notifications
You must be signed in to change notification settings - Fork5
Fix getTime() timezone#28
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Memory usage change @3b31d79
Click for full report table
Click for full report CSV |
sebromero left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looks good but please add a commenthere and mention that this function will try to sync with an NTP server to get the current time.
sebromero commentedJul 21, 2025
@pennam Actually this is already implemented in the function |
sebromero commentedJul 21, 2025
@pennam We would need to change |
pennam commentedJul 21, 2025
To keep backward compatibility I've added a flag to getCellularTime() adding the possibility for the user the get localtime or utc time4b7a446 I'm splitting PR#29 to reduce the scope and make it more simple to review |
pennam commentedJul 21, 2025
To avoid duplication of NTPSync code we can add a private static method that only takes care of the NTPsync call and is called by both getTime() and getCellularTime(). |
pennam commentedJul 22, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@sebromero please take a look at#32 🙂 |
with this changes we ansure getTime is always using UTC timezone and we can check NTP is synced comparing output with UNIX epoch.