home-assistant / core

:house_with_garden: Open source home automation that puts local control and privacy first.
https://www.home-assistant.io
Apache License 2.0
73.43k stars 30.67k forks source link

Misconception concerning "elevation" between Home Assistant configuration and the sun integration #128360

Open Triple-S opened 3 weeks ago

Triple-S commented 3 weeks ago

The problem

I think there is currently a major misconception concerning the term "elevation" in Home Assistant:

So in Home Assistant "elevation" is the elevation above sea level, while in the Astral package it is the elevation above ground level. However, the sun integration uses them as if they were the same which results in wrong calculated sun data.

What version of Home Assistant Core has the issue?

core-2024.10.2

What was the last working version of Home Assistant Core?

No response

What type of installation are you running?

Home Assistant Supervised

Integration causing the issue

sun

Link to integration documentation on our website

https://www.home-assistant.io/integrations/sun

Diagnostics information

No response

Example YAML snippet

No response

Anything in the logs that might be useful for us?

No response

Additional information

No response

home-assistant[bot] commented 3 weeks ago

Hey there @home-assistant/core, mind taking a look at this issue as it has been labeled with an integration (homeassistant) you are listed as a code owner for? Thanks!

Code owner commands Code owners of `homeassistant` can trigger bot actions by commenting: - `@home-assistant close` Closes the issue. - `@home-assistant rename Awesome new title` Renames the issue. - `@home-assistant reopen` Reopen the issue. - `@home-assistant unassign homeassistant` Removes the current integration label and assignees on the issue, add the integration domain after the command. - `@home-assistant add-label needs-more-information` Add a label (needs-more-information, problem in dependency, problem in custom component) to the issue. - `@home-assistant remove-label needs-more-information` Remove a label (needs-more-information, problem in dependency, problem in custom component) on the issue.

(message by CodeOwnersMention)


homeassistant documentation homeassistant source (message by IssueLinks)

home-assistant[bot] commented 3 weeks ago

Hey there @swamp-ig, mind taking a look at this issue as it has been labeled with an integration (sun) you are listed as a code owner for? Thanks!

Code owner commands Code owners of `sun` can trigger bot actions by commenting: - `@home-assistant close` Closes the issue. - `@home-assistant rename Awesome new title` Renames the issue. - `@home-assistant reopen` Reopen the issue. - `@home-assistant unassign sun` Removes the current integration label and assignees on the issue, add the integration domain after the command. - `@home-assistant add-label needs-more-information` Add a label (needs-more-information, problem in dependency, problem in custom component) to the issue. - `@home-assistant remove-label needs-more-information` Remove a label (needs-more-information, problem in dependency, problem in custom component) on the issue.

(message by CodeOwnersMention)


sun documentation sun source (message by IssueLinks)

NoRi2909 commented 1 week ago

@Triple-S Actual the Elevation calculated by the Sun integration is an angle above the horizon. Just like Azimuth is the angle from the North direction.

https://github.com/home-assistant/core/blob/c460e1bbbef67392a877007ef4d19570b883d435/homeassistant/components/sun/strings.json#L27-L28

They show up on the Sun service page:

image

(It's in German as we currently have a bug that the entity names don't follow the user's language but the system language instead)

I have just submitted this bug https://github.com/home-assistant/frontend/issues/22508 to address this from the other end and better explain the Elevation setting in Settings > General.

Triple-S commented 1 week ago

@NoRi2909 This is yet another elevation than the ones I am talking about:

In the sun integration the corresponding variable has the name "solar_elevation" and its unit is degree: https://github.com/home-assistant/core/blob/1f7cc5f5ece588d7d8d1b57e85b3b76e2d9333c1/homeassistant/components/sun/entity.py#L115

The elevation I am taking about is the one which has also the variable name "elevation": https://github.com/home-assistant/core/blob/1f7cc5f5ece588d7d8d1b57e85b3b76e2d9333c1/homeassistant/components/sun/entity.py#L108

This variable is not exposed to the user, but is internally used by the astral package to perform the calculations. The issue I see is that the astral package according to its documentation expects an "elevation above ground", however Homeassistant passes the "elevation above sea level" which would result in wrong calculations by the astral package.

NoRi2909 commented 1 week ago

For the German localization I just took your bug report and changed this to "Höhe über NN" to clarify this:

Screenshot 2024-10-24 09 26 52

It is referenced as a common term from several integrations like Starlink, OpenUV, Met etc. so this has some effect beyond the core setup. So this fixes it for German speakers, at least. 😄

Triple-S commented 1 week ago

Thank you @NoRi2909 but renaming will not change anything. I understand that the elevation defined in the Homeassistant configuration is the "height above sea level" or "Höhe über NN" in German and this is part of the issue:

The astral package uses the term elevation in yet a third way: Neighter as "height above sea level" nor as "angle between sun and horizon": In its documentation (https://astral.readthedocs.io/en/latest/package.html#astral.Observer) this third way of using elevation is described as "A float that is the elevation in metres above a location, if the nearest obscuring feature is the horizon". It makes sense that this is NOT the "height above sea level" because the earth radius is so much larger that it will always be negligible when it comes to calculating the time the sun passes the horizon. However, what is relevant is in which height the oberver is in comparison to its local horizon: Imagine standing on a tower or a mountain top, you will be able to see further and so the sun appears to rise earlier and set later. In the opposite way sitting in a vally, the mountains block the sun longer, so it appears to riste later and set earlier. This effect is what the astral package compensates for with this elevation parameter. Homeassistant passes the "height above sea level" for this parameter which is simply wrong because in most cases the horizon of an observer will be approximately the same height as the observer, so 0 should be passed as elevation to the astral package and NOT the "height above sea level".

NoRi2909 commented 1 week ago

Understood, thanks @Triple-S for clearing this up. You already helped me figure out a fix for "Elevation" being just "Höhe" till now in German in two places.

Difficult to properly compensate for a home location's elevation above or below the horizon as this may be quite different going from East to South to West. But you are right, the astral package should at least not mess this up.