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
71.66k stars 29.95k forks source link

Times of Day sensor broken for dynamic values #22211

Closed juzim closed 5 years ago

juzim commented 5 years ago

Home Assistant release with the issue: 0.89.2

Last working Home Assistant release (if known): n/a

Operating environment (Hass.io/Docker/Windows/etc.): docker on Debian

Component/platform: https://www.home-assistant.io/components/binary_sensor.tod/

Description of problem: When setting a combination of static and dynamic values, the results might be inverted:

- platform: tod
  name: wake up time
  after: '06:20'
  before: sunrise

Expected: The sensor is on between 6:20 and sunrise, otherwise off. If the sun rises before 6:20, the sensor should never turn on.

Current behavior: If the sun rises before 6:20, the sensor is always on except during the intended time frame.

As described in the documentation, If after time is later than before then the next day is considered. This means that the user has to ensure that the after time is always lower (or higher for before) than the expected sun event or the sensors value will be inverted, which makes the component next to unusable for all before/after settings that fall into the time frame of the natural progression of sun events.

I propose the introduction of the new optional sensor configuration entry wrap (defaults to True) that - if turned off - ensures that the time frame is on the same day.

If wrap is set to off and before and after are either both static or both sun events, an error or warning about an invalid configuration should be raised (overlaps between sun events with offsets over 12 hours shouldn't be valid use cases).

Problem-relevant configuration.yaml entries and (fill out even if it seems unimportant):

- platform: tod
  name: wake up time
  after: '06:20'
  before: sunrise

Traceback (if applicable):

Additional information: If this solution is valid, I'll try to find some time to implement it, but since it's a small fix someone with more experience might want to take over and get it done quickly.

In tod/binary_sensor.py:160

if before_event_date < after_event_date:

->

if self._wrap and before_event_date < after_event_date:

would be my solution.

dgomes commented 5 years ago

cc @kstaniek

kstaniek commented 5 years ago

@juzim Thanks for pointing it out. I'm working on this. Sorry for the delay, however, I can contribute mostly during weekends. I'm currently reworking this sensor and adding more test cases. @dgomes Can you add me to the assignees for tracking purpose

dgomes commented 5 years ago

@kstaniek only organization members can be added to assignees, sorry :(

kstaniek commented 5 years ago

@juzim, If 6:20 is after sunrise then the next-day sunrise is calculated. The time progression is always forward. If you need a different behavior you can always use template sensor and sun events. I think adding "wrap" option will confuse people. You can also use the attributes in the templates. Let's discuss it more when you provide your use-case.

juzim commented 5 years ago

Sure, my use case is the following: I got light strips under the staircase that should be on whenever it's too dark to see properly. I normally get up around 6 and I want the lights to turn off shortly after it's bright enough to see (seeing the light on for too long after sunrise makes me itchy and I have to turn them of manually, so I don't want a fixed time as the outer boundary).

Times the lights should be on: 6:00 to sunrise + 30 min and sunset - 30 min to 23:00.

I was using a node-red trigger which worked fine but required duplication since the timeframe was also needed as a condition (should the light turn on when I come home). The new component is perfect for this use case and can be used in ha and node-red. It worked fine until the sun started to rise before 6:00.

Of course I could use a template sensor but I still think that this behavior is too random. A sensor should not invert it's state depending on the local sun events. If it has a static and a variable boundary, it should always respect the intended timeframe (which is not possible to implement code wise). If this is out of scope for the sensor, the warning should be way bigger and clarify that the behavior can suddenly flip and thus this component should only be used for important systems like security if it's absolutely ensured that this condition won't occur.

Imagine the following situation: I want my garden door unlocked from sunrise till 8 so my dog can let itself out. Since the sensor looks like it can do that and the part about the next day doesn't affect me in February, I just add it. The next 5 month it works perfectly but suddenly the door is locked when my dog needs to do his business and the neighbor kids find the door unlocked at night and steal my Gartenzwerge.

The time progression is always forward

Can you give me an use case where the current behavior for times inside the described frame is intended (turn on for 1 hour in June, for 23 hours in July)?

kstaniek commented 5 years ago

Well explained. See your point now. What if I implemented two more keywords for dynamic values: sameday_sunrise and sameday_sunset Then your sensor will look like:

- platform: tod
  name: wake up time
  after: '06:20'
  before: sameday_sunrise

Would it be clear for users that this does not wrap?

So basically if you have a sensor like this:

- platform: tod
  name: wake up time
  after: sameday_sunset
  before: sameday_sunrise

It will always be off?

@dgomes, What do you think?

dgomes commented 5 years ago

personally I feel the default behaviour should have considered sunrise as the next sunrise after the after event. So in @kstaniek example you would not need sameday_sunrise (simple sunrise would do) but get the described behaviour.

kstaniek commented 5 years ago

@dgomes, this is the way how it works now. What @juzim says is that sometimes the sunrise, as it is dynamic, may shift before the 'after' time. Then the sensor starts working exactly the opposite. The problem is that this behavior may change without any warning for the user. Some people may not be aware when creating the sensor in the Winter that during the Summer the 'before' may turn into earlier time then 'after'. With sameday prefix, there will be no wrap to the next day basically.

dgomes commented 5 years ago

if that is the case: the interval is by definition invalid and it should send a warning/error to the user.

juzim commented 5 years ago

@dgomes I think that both use cases are valid here:

But I guess the second condition could always be inverted, so instead of a sensor for 'sprinkler_needed' one could create one specifying the 'leaving_time' from 8:00 till sunrise and turn the sprinkler off accordingly.

Mixing fixed values with sun states will always be problematic the closer they get since lights will start to turn on for mere minutes etc. Maybe there should also be a threshold for the length of the time span?

- platform: tod
  name: wake up time
  after: 08:00
  before: sameday_sunrise
  longer_than: 30

@kstaniek that's a better solution than adding flags. And I think this component is really powerful and will become the backbone of a lot of automations, thanks!

stale[bot] commented 5 years ago

There hasn't been any activity on this issue recently. Due to the high number of incoming GitHub notifications, we have to clean some of the old issues, as many of them have already been resolved with the latest updates. Please make sure to update to the latest Home Assistant version and check if that solves the issue. Let us know if that works for you by adding a comment 👍 This issue now has been marked as stale and will be closed if no further activity occurs. Thank you for your contributions.