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

Quiet the chatty sun.sun #23832

Merged
merged 5 commits into from May 15, 2019
Merged

Quiet the chatty sun.sun #23832

merged 5 commits into from May 15, 2019

Conversation

Swamp-Ig
Copy link
Contributor

@Swamp-Ig Swamp-Ig commented May 12, 2019

Breaking Change:

This might be a breaking change to some, the sun's location updates less frequently.

Description

After this report on reddit, I have tuned the sun.sun sensor so it doesn't update nearly so frequently.

Previously the sun.sun sensor was updating every 30 seconds, day and night.

The Sun moves along its path by 1° of arc for every 4 mins.

With reference to Twilight states I have tuned the sun.sun senor to update depending on the solar elevation:

phase Angle Frequency
Night < -18° 5° = 20 min
Astronomical Twilight < -12° 2° = 8 min
Nautical Twilight <-6° 2° = 8 min
Civil Twilight < 0° 1° = 4 min
Morning/Evening < 10° 0.5° = 2 min
Daytime > 10° 1° = 4 min

The total effect of this is that in equatoral areas the sun.sun device goes from generating 1441 events daily, to just 272, which is far more reasonable. It will generate more during some times of the year in polar areas, up to 720 around the equinox where the sun stays low on the horizon, but this will average out over the year and still be a lot less. Hopefully we can save Santa's SD card! 😁

I haven't documented this at this point, happy to contribute it though. There's nothing too much to document though really, for the most part the change is opaque.

Pull request with documentation for home-assistant.io (if applicable): home-assistant/home-assistant.io#9460

Discussion

I'll explain this a different way, it took me a bit to get my head around 😁

So the Sun moves in the sky by 1° of arc for every four minutes. It doesn't matter what time of the day or night it is, this is happening everywhere on the Earth. This vector can be either perpendicular to parallel to the horizon.

For reference, the disk of the sun takes up approximately 0.5° in the sky, so a shift of less than 0.5° is pretty meaningless since the light will cast more-or-less the same shadows and brightness.

The old code updated the sun's position every 30 seconds, which is just 0.125° of arc. This is a pretty meaningless amount of movement for the most purposes, and we can see that it's causing more issues than benefits since it's flooding the update log.

My first approach focused entirely on the movement with respect to the horizon, and just updated the Sun's position when it moved up or down with respect to it. This isn't so great however because at the poles the Sun moves quite parallel with the horizon, and might not change its elevation all day but in the mean time the light and shadow will change quite a lot. I realised it's really the arc change that matters. I also tried rounding out the elevation and azimuth (the bearing from North of the Sun), but near noon the Sun's azimuth changes rapidly and so you end up with a bunch of useless updates around Noon.

So, angular movement matters. First I cut down the update frequency to 1° of arc, so every 4 mins, but there are times of the day when you care more about where the Sun is. You really don't care much at all during the night, a bit more during twilight, quite a lot as the Sun is low on the horizon, and less during the middle of the day. The outcome of all of this is the update frequency changes depending on where the Sun is in the sky and throws away a lot of the useless position updates during the night and the middle of the day, and concentrates them on the times when the Sun is close to the horizon.

The other thing the PR does is makes sure that the timer for the 'next update' resets when the Sun crosses the horizon, or a few other important elevations, and the position is calculated. As soon as the Sun hits the horizon for example, the sun.sun sensor will update. You have to be careful with solar noon and solar midnight, naively we'd expect solar midnight to be at night, but in the polar winter it isn't.

Example entry for configuration.yaml (if applicable):

sun:

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.

@ghost
Copy link

ghost commented May 12, 2019

Hey there @home-assistant/core, mind taking a look at this pull request as its been labeled with a integration (sun) you are listed as a codeowner for? Thanks!

This is a automatic comment generated by codeowners-mention to help ensure issues and pull requests are seen by the right people.

self.solar_azimuth = round(
self.location.solar_azimuth(utc_point_in_time), 2)
self.solar_elevation = round(
self.location.solar_elevation(utc_point_in_time), 2)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of using hard-coded update times, why not round the angles? Then the state machine would take care of de-duplicating values (because of force_update=False).

The sun position calculation is really quick - so computation time is not a problem. Database storage however is.

We could also have different rounding accuracy based on the value. So for example

  • if abs(elevation) < 0.1: accuracy = 0.1
  • else: accuracy = 1.0

Sounds like a much simpler approach to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did try that. The thing is that the azimuth swings around rapidly at near noon, and so you end up with a bunch of pretty useless updates if you're tracking the azimuth too.

You do need to do that too at high latitudes, at the extreme case if you are at the north pole the elevation doesn't change much during 24hrs, only the azimuth. This affects what windows the dun shines into for example.

That's what lead to total arc of travel, rather than change with respect to the horizon. We do care more about changes near the horizon because they're more important, but mostly we want to know that the sun has moved somewhere new in the sky.

I'm not trying to minimise the calculation, the goal is to reduce the number of writes to the database.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added a more detailed description above to what the code is doing. It does take a bit to get your head around 😁

@balloob
Copy link
Member

balloob commented May 13, 2019

Wow. This looks great.

Can you please add your documentation to the source code, so it is clear what is going on ? Right now it's pretty sparse on comments.

@Swamp-Ig
Copy link
Contributor Author

Improved documentation as requested, linked back here. Ready to merge once smoke tested.

@Swamp-Ig Swamp-Ig merged commit 9da74dd into home-assistant:dev May 15, 2019
@Swamp-Ig Swamp-Ig deleted the quiet-sun branch May 15, 2019 07:34
@balloob balloob mentioned this pull request Jun 4, 2019
alandtse pushed a commit to alandtse/home-assistant that referenced this pull request Oct 12, 2019
* Split up method to allow caching event

* Lower frequency of updates.

* Code review patches.

* Minor changes to test

* Skip end of period at fixed multiple of delta.
Improved documentation.
@bachya bachya mentioned this pull request Jan 15, 2020
4 tasks
@reubenbijl
Copy link
Contributor

Hey, I was just thinking, did you consider looking at the terrain for a location as well for considering sunrise and sunset? It would be amazing to have the first time that you get direct sun based on the terrain.

@home-assistant home-assistant locked as resolved and limited conversation to collaborators Aug 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants