Refactor sun component for correctness #7295

emlove · 2017-04-25T12:50:45Z

Description:

This PR is a rework of the sun automation triggers / conditions. In their current state, they were close, but not exactly correct, which led to issues such as #6834. The core of the PR is allowing the offset to feed down into the get_next_solar_event loop so that the next event is being used instead of 24 hour increments.

Sunrise / Sunset Triggers:

The current sunrise trigger calculates the next sunrise, then adds/subtracts 24 hours accounting for the offset to get a time that is in the future. This creates the problem if sunrises are getting later each day. If due to an offset, the next sunrise is calculated as the current sunrise + 24 hours, but the real next sunrise is a few minutes later than that, the trigger will occur, then the next calculated sunrise will be in a few minutes. This change uses astral to search for a sunrise + offset that matches our requirements.

Sunrise / Sunset Conditions:

The current sunrise condition is similar, in that it first finds the next sunrise, then converts it into a time to use for the condition. This PR changes the condition to explicitly find today's sunrise.

Related issue (if applicable): fixes #6834, although I haven't reproduced it explicitly, since it's difficult to trigger.

mention-bot · 2017-04-25T12:50:46Z

@armills, thanks for your PR! By analyzing the history of the files in this pull request, we identified @balloob, @philipbl and @fabaff to be potential reviewers.

amelchio · 2017-04-27T05:10:52Z

Thanks for working on this! To test it, I have restored the below automation that I never got around to debug.

This morning (unpatched) I received two mails, each with the content "below_horizon". I would expect just one, of course, and I would also expect the content to be "above_horizon" as it was triggered due to sunrise (or possibly "at horizon" :).

From the timestamps I see that one of them was in fact sent about a minute and a half before the next_setting in the log. This does not match your description because my sunrises are getting earlier at this time of the year.

I will now install this PR and let you know the result in a day or two when I have some test data.

automation:
  - alias: suntest
    trigger:
      - platform: sun
        event: sunset
      - platform: sun
        event: sunrise
    action:
      service: notify.mail
      data_template:
        title: 'Sun'
        message: >
          {{ states.sun.sun.state }}

amelchio · 2017-04-27T21:14:58Z

I got two sunset notifications even with the patch applied. After a lot of digging it turns out that Astral prefers to work with dates, not datetime:

>>> l.sunset(date=datetime.datetime(2017,4,27))
datetime.datetime(2017, 4, 27, 20, 40, 39, tzinfo=<DstTzInfo 'CET' CEST+2:00:00 DST>)
>>> l.sunset(date=datetime.datetime(2017,4,27,12,00,00))
datetime.datetime(2017, 4, 27, 20, 41, 39, tzinfo=<DstTzInfo 'CET' CEST+2:00:00 DST>)

Therefore, this small patch on top of your PR seems to fix things. I obviously did not test it yet (only two chances per day), but now the next_setting attribute finally seems correct. (It seems a similar change is also needed for conditions.)

diff --git a/homeassistant/components/sun.py b/homeassistant/components/sun.py
index 98d02684c..641a81a6f 100644
--- a/homeassistant/components/sun.py
+++ b/homeassistant/components/sun.py
@@ -295,7 +295,7 @@ class Sun(Entity):
         while True:
             try:
                 next_dt = callable_on_astral_location(
-                    utc_point_in_time + timedelta(days=mod),
+                    utc_point_in_time.date() + timedelta(days=mod),
                     local=False) + offset
                 if next_dt > utc_point_in_time:
                     return next_dt

emlove · 2017-04-27T21:24:25Z

OK, I was under the impression that astral would be able to manage getting the date from a datetime, but maybe not! I included the date as you suggested, also converting to local time before getting the date. Let me know if this looks good as well.

Also, thanks for testing this out BTW, I haven't been able to reproduce this well myself.

amelchio · 2017-04-27T21:39:00Z

I now installed your latest version, so let's see in about 8 hours :)

Speaking of local time, can you get the attributes (as seen in the log) away from UTC?

emlove · 2017-04-27T21:55:45Z

tests/helpers/test_event.py

@@ -334,15 +338,15 @@ def test_track_sunrise(self):

        self._send_time_changed(next_rising + offset)
        self.hass.block_till_done()
-        self.assertEqual(2, len(runs))


OK, I think we're on to something now. After switching to dates, this test started failing. We were previously asserting that the sunrise trigger was run twice, which I think is exactly the behavior we were seeing, and want to avoid.

emlove · 2017-04-27T21:58:40Z

As far as the local times, the state attributes are primarily designed to be machine-readable, so they should really stay UTC. You should be able to convert to local time as template sensor if needed. The reason for the local conversion in the code is if we're converting to a date and dropping the time information, we need to make sure we get the correct local date to determine the correct solar event.

amelchio · 2017-04-28T07:16:29Z

I got only one notification this morning, sent at a time that matches other sources 🎉

The mail still said "below_horizon" for a sunrise notification. Because I spent two hours reading the code, I now know why: the sun state is actually updated a second late, to make sure it gets it right.

So maybe we should also add a second to the trigger? This will ensure that I can get to a predictable side of the event by doing a 1 second offset where it now requires 2 seconds to be certain. However, if this complicates things too much, it might not be worth it.

emlove · 2017-04-28T14:57:19Z

My vote is to not worry about it for now, unless we actually get a use case that makes sense. Ideally, they would be scheduled for the exact same time, but even in that case, there is no guarantee as to which would trigger first.

And realistically, at the moment of sunrise the sun isn't above or below the horizon, so I don't think we need to artificially pick one.

amelchio · 2017-04-29T09:56:55Z

I have tested this, it seems to work. I did not test offsets or conditions.

With my limited knowledge of this area in mind, the code also looks fine.

balloob · 2017-04-29T23:14:58Z

homeassistant/components/sun.py

        return None

+    return Sun.get_next_solar_event(


The helper methods that exist in the top of components like in this case next_dawn_utc or in the case of a light for example turn_on, are meant to help other components interact with this component via hass.

So these methods should literally just get data from hass and process it, like it did before.

So the problem we're facing that's causing duplicate triggers, is that the next sunrise and next sunset information stored on the state machine is not sufficient information to determine the next event once offsets are added to the mix, since the correct time might need to be calculated as an offset relative to a sunset/sunrise on a different day. These are two alternate proposals that come to mind:

We have the sun automation platform directly invoke astral, creating it's own location object similar to the sun component logic. This would decouple the sun automation platform from the sun component. (This is my preference.)

Instead of only storing the next sunrise, and next sunset in the state machine, we store sunrise/sunset events for yesterday, today, and tomorrow. This should be sufficient information for both the sun automation condition and triggers to calculate the correct times that they need.

The sun automation uses the condition helper. That means that we have to move the sun dependency to be a core dependency, I would want to avoid that.

Why do we need so many knowledge about sunsets and sunrises ? We only care about the next one right ? When do we need an offset

Here's one example of how the current logic is failing: The user has configured an automation to happen 2 hours before sunset. The user starts up hass one hour before the next sunset. The automation helper looks at the time of the next sunset (1 hour from now), and adds 24 hours, so the event will trigger 23 hours in the future. In 23 hours, the event triggers, and hass goes to look up the next event time. If sunsets are getting later in the day, the next sunset will be in 2 hours + 3 minutes, so hass schedule the next trigger for 3 minutes later. The problem here is that adding 24 hours to a previous sunset doesn't give the time of the next sunset, although it's been close enough for us to skirt by for a while.

This is the same example with the correct logic: The user starts up hass one hour before the next sunset. The automation helper looks at the time of the next sunset (1 hour from now), and since the next sunset + offset has already occurred, it looks up the next sunset, which is in 25 hours + 3 minutes, and adds the offset, so the event will trigger 23 hours + 3 minutes in the future.

I think it could be done in two steps. If sunset+offset has already passed, schedule a task to run at sunset (plus two seconds :-). This task is then able to see the following sunset time and set the correct timer for the action.

That still doesn't solve this use case, and it also doesn't address the problems with the sun conditions. Besides, I don't want to introduce more workarounds to address a specific use case. The goal of this PR is to do the sun implementation correctly, to eliminate not only the problems we know about, but other potential esoteric problems we didn't predict.

I guess I'm not seeing why the sun platform in automation can't invoke astral directly. Platforms in other components introduce new requirements. It seems that having the sun automation platform speak directly to astral would get rid of the special cases we've built for sun, and make it behave like any other platform.

Okay, so we add last and next sunrise/sunset as attributes to the state machine.

Home Assistant architecture is that ALL communication goes via the state machine and event bus. Violating this will make sure it won't work for multi-instance setups or if sun.sun would come from another data source. And thus cannot be accepted.

FWIW: just to see what it looked like, I tried to implement my proposal with a two-step schedule. I did not even bother testing whether it worked because the code quickly got so fragile that I would be embarrassed to submit it. So I agree with @armills that this should be done properly, without a number of edge cases with special handling.

The architecture discussion is above my head at this time, no comment on that.

I'm going to try and put up a new implementation today, so we can at least have a better reference for our discussion.

I've tried to lay out a few points of why I think the sun component is fundamentally different from any of the other components. Don't want to prolong this thread, so I've tried to collect my thoughts here. Sorry for the big dump, but this will be the last I have to say on it.

The astral library isn't talking to any devices, it isn't performing IO, it's just doing math on its inputs.

The static inputs to astral are latitude, longitude, and elevation, all available from the hass config.

The only input to astral that changes is the date. For any number of remote instances, if any of them provide these sames inputs to astral they will get the same results. There is no actual state that needs to be stored. Since we already allow all instances to use system time from the python datetime module, each instance already has all the information it needs to calculate these events.

The biggest giveaway that the sun is different, is we allow offsets to events. We obviously don't allow triggers 1 hour before an MQTT event. If this was a normal component, we'd have to wait for the sun rising event on the event bus like normal. However, we have this functionality because sun events aren't an event that we're waiting for to happen, when it will happen is just a fact that anyone can calculate and get the same result.

I built this implementation assuming we want to make astral a core dependency. It isn't necessarily a requirement, but given the number of interconnections that exist, it might be worth considering. Alternatively, we can just require any component that calls the sun helper methods depend on astral.

Also worth noting: As part of this rework I discovered that the flux component was making the same error as the automation condition logic.

balloob · 2017-04-29T23:17:21Z

homeassistant/helpers/condition.py

    before_offset = before_offset or timedelta(0)
    after_offset = after_offset or timedelta(0)
+    entity = hass.data[sun_cmp.ENTITY_ID]


This is not allowed.

All pieces of the system are set up in such a way that the only interaction between the parts is happening via the state machine. With this approach having a remote Hass instance would not be possible. Please revert.

The methods on top of the sun component are meant to extract the data from the state machine. If it's not in the state machine, it cannot be used.

houndci-bot · 2017-05-04T20:46:49Z

tests/helpers/test_sun.py

+        print(sun.get_astral_event_date(self.hass, 'sunrise',
+            datetime(2017, 7, 26)))
+        print(sun.get_astral_event_date(self.hass, 'sunset',
+            datetime(2017, 7, 26)))


continuation line under-indented for visual indent

houndci-bot · 2017-05-04T20:46:49Z

tests/helpers/test_sun.py

+            datetime(2017, 7, 25)))
+
+        print(sun.get_astral_event_date(self.hass, 'sunrise',
+            datetime(2017, 7, 26)))


continuation line under-indented for visual indent

houndci-bot · 2017-05-04T20:46:49Z

tests/helpers/test_sun.py

+        print(sun.get_astral_event_date(self.hass, 'sunrise',
+            datetime(2017, 7, 25)))
+        print(sun.get_astral_event_date(self.hass, 'sunset',
+            datetime(2017, 7, 25)))


continuation line under-indented for visual indent

houndci-bot · 2017-05-04T20:46:49Z

tests/helpers/test_sun.py

+        june = datetime(2016, 6, 1, tzinfo=dt_util.UTC)
+
+        print(sun.get_astral_event_date(self.hass, 'sunrise',
+            datetime(2017, 7, 25)))


continuation line under-indented for visual indent

houndci-bot · 2017-05-04T20:46:49Z

tests/helpers/test_sun.py

+from datetime import timedelta, datetime
+
+from homeassistant.setup import setup_component
+import homeassistant.core as ha


'homeassistant.core as ha' imported but unused

houndci-bot · 2017-05-04T20:46:49Z

tests/helpers/test_sun.py

+from unittest.mock import patch
+from datetime import timedelta, datetime
+
+from homeassistant.setup import setup_component


'homeassistant.setup.setup_component' imported but unused

emlove · 2017-05-04T20:55:10Z

homeassistant/components/switch/flux.py

@@ -159,8 +159,7 @@ def flux_update(self, now=None):
        """Update all the lights using flux."""
        if now is None:
            now = dt_now()
-        sunset = next_setting(self.hass, SUN).replace(


This is a good example of the kind of logic we want to eliminate. Taking whatever sunset you get, then picking a new date is exactly the kind of logic that leads us to double triggers and other strange bugs.

balloob

I like it! 🐬 🎉

2 minor tweaks but I do think that this is the right solution 👍

balloob · 2017-05-05T03:49:13Z

homeassistant/helpers/sun.py

+    elevation = hass.config.elevation
+    info = ('', '', latitude, longitude, timezone, elevation)
+
+    if info not in _LOCATION_CACHE:


We should use hass.data.

balloob · 2017-05-05T04:01:35Z

homeassistant/helpers/sun.py

+
+
+@callback
+def get_astral_event_date(hass, event, now=None):


This function only cares about a date, so let's take a date as an argument. Nice thing about dates is that they have no timezone 👍

Sounds good. For now I'd like to keep a provision in this helper in case a datetime is passed. We discovered astral had the particularly nasty problem where it would accept datetimes instead of dates, and would return a result that was close, but not exactly right, since it was basically a fractional date. This way we can ensure anything hass is passing to astral is sanitized.

amelchio · 2017-05-05T06:42:58Z

Well done @armills! This was a tough one, and a popular one to get fixed. I am counting three different bugs that it fixes but there are probably more :-)

emlove · 2017-05-08T18:21:35Z

This should be good to go into dev now!

balloob · 2017-05-09T07:03:46Z

Woohoo 🎉 🍌

Refactor sun component for correctness

203e337

homeassistant added cla-signed integration: sun labels Apr 25, 2017

balloobbot added component core and removed cla-signed integration: sun labels Apr 25, 2017

Convert datetimes to dates for astral

ecdc5b5

homeassistant added the cla-signed label Apr 27, 2017

Fix tests for updated code

bee84e7

emlove commented Apr 27, 2017

View reviewed changes

Fix times now that calcs are fixed

f90c2de

emlove added the Ready for review label Apr 28, 2017

amelchio approved these changes Apr 29, 2017

View reviewed changes

balloob requested changes Apr 29, 2017

View reviewed changes

emlove added 2 commits May 4, 2017 10:25

Merge branch 'dev' into sun-fixes

934b0c2

Move sun functions to helpers

836c0e4

homeassistant added the cla-signed label May 4, 2017

balloobbot added platform requirement labels May 4, 2017

houndci-bot reviewed May 4, 2017

View reviewed changes

Fix flake on new file

4061c4d

homeassistant added the cla-signed label May 4, 2017

emlove commented May 4, 2017

View reviewed changes

robbiet480 added the cla-recheck label May 5, 2017

homeassistant added cla-signed and removed cla-recheck labels May 5, 2017

balloob approved these changes May 5, 2017

View reviewed changes

Additional tweaks from review

4b9c91e

homeassistant added the cla-signed label May 5, 2017

emlove added 2 commits May 8, 2017 13:45

Merge branch 'dev' into sun-fixes

248fd3b

Update requirements

c7f7eb6

emlove added the cla-recheck label May 8, 2017

homeassistant added cla-signed and removed cla-recheck labels May 8, 2017

balloob merged commit 40d27cd into home-assistant:dev May 9, 2017

emlove deleted the sun-fixes branch May 9, 2017 14:22

home-assistant locked and limited conversation to collaborators Aug 12, 2017

Refactor sun component for correctness #7295

Refactor sun component for correctness #7295

Conversation

emlove commented Apr 25, 2017

Description:

Sunrise / Sunset Triggers:

Sunrise / Sunset Conditions:

mention-bot commented Apr 25, 2017

amelchio commented Apr 27, 2017

amelchio commented Apr 27, 2017

emlove commented Apr 27, 2017

amelchio commented Apr 27, 2017

Choose a reason for hiding this comment

emlove commented Apr 27, 2017 • edited

amelchio commented Apr 28, 2017

emlove commented Apr 28, 2017

amelchio commented Apr 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

balloob Apr 30, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emlove May 4, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

balloob left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amelchio commented May 5, 2017

emlove commented May 8, 2017

balloob commented May 9, 2017

emlove commented Apr 27, 2017 •

edited

balloob Apr 30, 2017 •

edited

emlove May 4, 2017 •

edited