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

Various Time zone fixes #569

Merged
merged 20 commits into from
Mar 29, 2023
Merged

Various Time zone fixes #569

merged 20 commits into from
Mar 29, 2023

Conversation

jeremypoulter
Copy link
Collaborator

  • Set the default time zone to the defined one
  • skip | when setting the env var, stops the timezone from being recognised
  • Filter out the Etc/... time zones

Fixes #107

- Set the default time zone to the defined one
- skip `|` when setting the env var, stops the timezone from being recognised
@KipK
Copy link
Collaborator

KipK commented Mar 11, 2023

perfect, it solves the timer bug, and it probably fix #467 I have to check that later

edit: don't fix #467, still have to do utc hack

@KipK
Copy link
Collaborator

KipK commented Mar 11, 2023

One should push a new release after this merge. 4.1.7 has too much problems. This was the last urgent one to fix.

@KipK
Copy link
Collaborator

KipK commented Mar 13, 2023

now this fix #467
👍

question: are we agree that when setting time to manual mode, the timezone should be set to UTC0 ?

@jeremypoulter
Copy link
Collaborator Author

At the moment it forces UTC in the code, so it doesn't matter what time zone you send. I am going to look at setting the time zone for manual set times as the DST info from the timezone is handy. Also need to figure out how to handle a different timezone of the EVSE and the browser. The ESP now sends the local and GMT times (according to the ESP) but this is then displayed in the timezone of the browser by default. Not sure if that is much of an issue other than for testing.

@KipK
Copy link
Collaborator

KipK commented Mar 13, 2023

strage, because if i send to /settime
edit: values below

@KipK
Copy link
Collaborator

KipK commented Mar 13, 2023

corrected I sent it wrong, but same problem:

ntp: false
tz: Europe/Paris|CET-1CEST,M3.5.0,M10.5.0/3
time: 2023-03-13T09:19:33.023+01:00

gives:
local_time: 2023-03-13T09:19:33+0000
time: 2023-03-13T09:19:33Z

should gives
local_time: 2023-03-13T09:19:33+01:00
time: 2023-03-13T08:19:33Z

edit: seems better to keep the tz in the setting for the manual mode to be consistant , in current UI, I force tz to UTC, and send the localtime as it is UTC time + store the previous tz setting in local storage to restore it if going back to ntp. but that's a bit hacky. Example above tries to just send correct time

@jeremypoulter
Copy link
Collaborator Author

Yeah, I think I need to update it to properly handle timezones for manual mode.... :) https://youtu.be/-5wpm-gesOY

@KipK
Copy link
Collaborator

KipK commented Mar 13, 2023

ahahah that's insane reality we have to deal with :)))

@KipK
Copy link
Collaborator

KipK commented Mar 14, 2023

Can it be merged yet? Even if you change the manual tz later, it already works ok

This also includes some large rework of code to ensure the `config_version` if properly set, and also added the ability to use JSON to set the time to be inline with the other APIs.
- Correctly block none UTC times, may support in the furture, but for now just set the UTC time + time zone
- `sntp_enable` -> `sntp_enabled` to match config endpoint
- mktime returns the time in local time, but the incomming time is UTC, so we have to hack around with the configured time zone. No implementation on timegm on the ESP32 toolchain.
- `time_format_time` can now return the GMT time as well as local time
- Fixed parsing of time zone on the legacy API
- Yet more tests
Fixes #349 reading the time back from the EVSE
@jeremypoulter jeremypoulter marked this pull request as draft March 18, 2023 19:13
@jeremypoulter
Copy link
Collaborator Author

Found many more timezone bugs, and fixed most of them, but this is still WIP

@jeremypoulter
Copy link
Collaborator Author

I think the main task left is to work out how to set the time manually in the EVSE's timezone

@KipK
Copy link
Collaborator

KipK commented Mar 21, 2023

I've updated the UI2 for new /limit.

Works ok now for manual without tricking to UTC.
UI sends zulu time + timezone and we're getting back correct local time

Will push on master when you are ready to merge.

@jeremypoulter jeremypoulter marked this pull request as ready for review March 23, 2023 23:24
@jeremypoulter
Copy link
Collaborator Author

This all seems to be working now

@KipK
Copy link
Collaborator

KipK commented Mar 24, 2023

all good here. Thanks for this

@KipK
Copy link
Collaborator

KipK commented Mar 29, 2023

@glynhudson can you merge this one ?

@glynhudson glynhudson merged commit 8e17e58 into master Mar 29, 2023
@jeremypoulter jeremypoulter deleted the jeremypoulter/issue107 branch May 12, 2023 18:19
@chaseadam chaseadam mentioned this pull request Oct 12, 2023
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.

Timezone Issues
3 participants