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

Allow timestamp sensors to report time as non-UTC as long as they are timezone aware #315

Closed
tsvi opened this issue Nov 28, 2019 · 16 comments

Comments

@tsvi
Copy link

tsvi commented Nov 28, 2019

Context

When implementing home-assistant/core#26940 I agreed to @balloob and @MartinHjelmare request to have the timestamp sensors return their state as UTC time.
After careful reconsideration and multiple emails with Jewish Calendar integration users, I think this was misguided and return to my original point that timestamp sensors, that were originally proposed in #39, should be allowed to return their state as a localized time.

Comment related to the issue:

Proposal

Allow timestamp sensors to return localized time as long as it is ISO8601 formatted.

Consequences

Disadvantages: when looking at logs or debugging the developer needs to take better care at looking at the timezone. This can be mitigated by requesting that _LOGGER messages always should print the UTC timestamp.

Advantages:

  • If parts of the UI don't support showing the localized time, we don't get complaints from users.
  • No need to add support for every user-facing UI to show localized time.
@tsvi
Copy link
Author

tsvi commented Nov 28, 2019

Places where the UI doesn't match the expected view:

  • History:
    unnamed

  • State view:
    image002

  • Glance card:
    image001

@tsvi
Copy link
Author

tsvi commented Nov 28, 2019

I also opened the following issue about the glance card: home-assistant/frontend#4199

@mpaneth
Copy link

mpaneth commented Nov 29, 2019

There is also an inconsistency with other sensors eg Sun which displays the event in local conditions and not UTC!! eg Sun says below the horizon in local time but Jewish Calendar Upcoming Havdalah says UTC time which is out by 11 hours.

@balloob
Copy link
Member

balloob commented Nov 30, 2019

I feel like this backend architecture issue is driven by lacking features in the frontend. That seems wrong. I suggest we first work on making the frontend work correctly with local timestamps, and then I guess this issue is no longer necessary?

@tsvi
Copy link
Author

tsvi commented Dec 1, 2019

Technically speaking, yes.
If every single place in the frontend dealing with timestamps would have them localized this would be a non-issue. Or at least less of an issue as it still is in my opinion less logical when filtering via templates in automation, but there I can live with it.

But making it a frontend issue, makes it non-trivial to fix, as you're adding a level of complexity for every mention of timestamp sensors which needs to be tested for (don't know how things are tested there). Secondly, I don't really see the pain point in having a correctly formatted ISO8601 non-UTC timestamp, as the frontend code "eats" it correctly as well.

@tsvi
Copy link
Author

tsvi commented Dec 4, 2019

Just received another email from a new user with the same issue. I think most people who are unfamiliar with the time zone notation won't realize that they're supposed to add/subtract x hours.

So as I see it, either we need to fix every single place in the frontend where a timestamp sensor is shown, or we change timestamp states to report localized time. Or create an intermediate layer, I suppose we have something of that kind for translations. (Just guessing, no idea how the translations work)

@balloob do you have a concrete example you or someone you know got stuck debugging just to realize that the issue was because the state was non-UTC?

@bramkragten
Copy link
Member

I think this is something that needs to be fixes in the frontend, the notation of those dates is not user friendly anyway, so needs updating.

@tsvi
Copy link
Author

tsvi commented Dec 4, 2019

@bramkragten I agree, I only think that since we might forget in the future in some pieces of the frontend to show a "nice" timestamp, we should allow the state to be non-UTC as long as it includes timezone information. The positive impact is less wrong reports on bugs (frontend instead of specific integration). The negative impact is that in the future we cannot trust the timestamp to be only UTC.

@balloob
Copy link
Member

balloob commented Dec 4, 2019

The negative impact is that in the future we cannot trust the timestamp to be only UTC.

This is a really big deal that impacts more than just the frontend. This impacts any system that integrates with Home Assistant. Any automation anyone writes etc.

We should stick with UTC.

@balloob balloob closed this as completed Dec 4, 2019
@tsvi
Copy link
Author

tsvi commented Dec 4, 2019

I'll politely disagree this to be a big deal as UTC is a specific timezone. Any automation or integrations should simply consider the timestamp to be timezone aware. If they do that they're safe.

But, you're the boss. 😉

@dkagedal
Copy link

dkagedal commented Dec 4, 2019

It's important to remember to separate these concepts:

  1. An instant in time. This can easily be represented as a number of (milli)seconds since the unix epoch (or whatever resolution is convenient).

  2. A past time on a certain date, in a certain time zone.

  3. A time of day.

The first two can be converted between losslessy, which means that an instant can be turned into a textual form using whatever time zone the user thinks makes sense.

The third concept is very useful in things like automation, because you want things to happen at 7am every day etc. But turning that into a concrete time instant requires knowing the date and time zone. And it is really only possible to do for instants that have already happened, since you do not know for certain how "2020-12-25 08:00" will be translated to an instant (it depends on politics).

So a sensor needs to decide whether to provide an instant (a time stamp) or a local time. If it provides a timestamp, it doesn't really matter what representation it uses as long as it can be turned in to a number of (milli)seconds since epoch.

@rpbx
Copy link

rpbx commented Mar 16, 2020

I think this is something that needs to be fixes in the frontend, the notation of those dates is not user friendly anyway, so needs updating.

Hello @tsvi From my understanding there is a frontend bug that is preventing the Jewish sensor from displaying the local time correct?

Is there a workaround?

Thank you!

@tsvi
Copy link
Author

tsvi commented Mar 16, 2020 via email

@rpbx
Copy link

rpbx commented Mar 16, 2020

Were issues created yet, I do not want to open duplicate issues.
Thank you!

@tsvi
Copy link
Author

tsvi commented Mar 16, 2020

Only for the glance card. See above

@balloob
Copy link
Member

balloob commented Dec 8, 2021

@tsvi UTC is no longer a requirement and this is resolved. Can you validate this PR? home-assistant/core#61222

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

No branches or pull requests

6 participants