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

Times of The Day Binary Sensor #20068

Merged
merged 11 commits into from Feb 15, 2019

Conversation

@kstaniek
Copy link
Contributor

kstaniek commented Jan 13, 2019

Description:

This code provides the new binary sensor platform checking if the current time is within configured time range. The platform allows adding multiple sensor working for different time ranges.

The time ranges can be defined as time format string or sun event (sunset or sunrise). Also for each boundary the offset can be added.

Related issue (if applicable): fixes #

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

Example entry for configuration.yaml (if applicable):

binary_sensor:
  - platform: tod
    sensors:
      early_morning:
        after: sunrise
        after_offset: '-02:00'
        before: '07:00'
      morning:
        after: sunrise
        before: '12:00'
      late_morning:
        after: '10:00'
        before: '12:00'
      early_afternoon:
        after: '12:00'
        before: '15:00'
      afternoon:
        after: '12:00'
        before: '18:00'
      late_afternoon:
        after: '16:00'
        before: '18:0'
      early_evening:
        after: '17:00'
        before: '19:00'
      evening:
        after: '17:00'
        before: '22:00'
      late_evening:
        after: '20:00'
        before: '22:00'
      middle_of_the_night:
        after: '23:00'
        before: sunrise
        before_offset: '-02:00'
      night:
        after: '22:00'
        before: sunrise
      day:
        after: sunrise
        before: sunset

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.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.
@dgomes
Copy link
Member

dgomes left a comment

Just a very quick review.

I'm wondering what you couldn't do with sun.sun that you required this binary_sensor ?

Show resolved Hide resolved homeassistant/components/binary_sensor/tod.py Outdated
Show resolved Hide resolved homeassistant/components/binary_sensor/tod.py Outdated
@kstaniek

This comment has been minimized.

Copy link
Contributor Author

kstaniek commented Jan 14, 2019

@dgomes

I'm wondering what you couldn't do with sun.sun that you required this binary_sensor ?

All cases you can implement with sun and binary_sensor.template however:

  1. The template becomes more complex if you want to have a sun event with combination of offsets and take care of over midnight periods, i.e. from sunset to sunrise next day.
  2. Sun component gives you only next event not today's.
  3. Template sensor based on sun.sun may have unknown state during hass restart when sun.sun is not available yet. This is enjoying when you restart hass during the time period and your automation are triggered accidentally.

This sensor proposal aligns with one of the important @balloob 's goal, which is making hass simple to normal users.

I have also an idea to add another sensor.tod with values calculated based of different non-linear functions. I.e. norma distribution with float values from 0 to 1. This feature may be extremely
helpful in ML where you need time quantification and classificators.

Please let me know if this convinces you and I will make a fixed pull request.
Thanks,
Klaus

@OttoWinter

This comment has been minimized.

Copy link
Member

OttoWinter commented Jan 14, 2019

Show resolved Hide resolved homeassistant/components/binary_sensor/tod.py Outdated
self._next_sunsetting = None

self.entity_id = async_generate_entity_id(
ENTITY_ID_FORMAT, 'tod_{}'.format(sensor_name), hass=hass)

This comment has been minimized.

@OttoWinter

OttoWinter Jan 14, 2019

Member

Let's use the built-in entity id generator. Prefixing the entity is by tod_ is unexpected.

This comment has been minimized.

@kstaniek

kstaniek Jan 14, 2019

Author Contributor

You mean just pass the sensor_name to generate_entity_id without prepending with tod_. Makes sense to avoid confusion as the name is provided in config.

This comment has been minimized.

@OttoWinter

OttoWinter Jan 19, 2019

Member

Well now that you're using the default entity ID format, there's no need for adding this explicitly. Home Assistant will automatically generate the entity ID for you.

Please remove async_generate_entity_id here. It will automatically be set.

This comment has been minimized.

@kstaniek

kstaniek Jan 22, 2019

Author Contributor

OK, Understand removed async_generate_entity_id and replaced with:
self.entity_id = ENTITY_ID_FORMAT.format(name)

This comment has been minimized.

@OttoWinter

OttoWinter Jan 22, 2019

Member

No, I mean removing this entire line. HA will automatically set the entity ID, no need to assign it yourself

This comment has been minimized.

@kstaniek

kstaniek Jan 26, 2019

Author Contributor

@OttoWinter Hi Otto, could you please take a look at my comments?
There could be also a third option:

3) 
binary_sensor:
  - platform: tod
     name: Wczesny poranek
     after: sunrise
     after_offset: '-02:00'
     before: '07:00'
  
  - platform: tod 
     name: Morning
     after: sunrise
     before: '12:00'

I'm a bit confused and It's not clear to me what option is preferred.

This comment has been minimized.

@kstaniek

kstaniek Jan 29, 2019

Author Contributor

@OttoWinter Hi Otto, I know you must be busy, however I would kindly remind I'm waiting for your feedback.

This comment has been minimized.

@kstaniek

kstaniek Feb 1, 2019

Author Contributor

@OttoWinter Any reply on this pls?

This comment has been minimized.

@OttoWinter

OttoWinter Feb 1, 2019

Member

@kstaniek Yes, option 3. The template sensor way of doing it all in sensors: was always very confusing to users (from community forum issue count)

This comment has been minimized.

@kstaniek

kstaniek Feb 2, 2019

Author Contributor

Updated the config and tests accordingly. Option 3 has been implemented for config.

Show resolved Hide resolved homeassistant/components/binary_sensor/tod.py Outdated
Show resolved Hide resolved homeassistant/components/binary_sensor/tod.py Outdated
Show resolved Hide resolved homeassistant/components/binary_sensor/tod.py Outdated
Show resolved Hide resolved homeassistant/components/binary_sensor/tod.py Outdated
Show resolved Hide resolved homeassistant/components/binary_sensor/tod.py
Show resolved Hide resolved homeassistant/components/binary_sensor/tod.py
Show resolved Hide resolved homeassistant/components/binary_sensor/tod.py
Show resolved Hide resolved homeassistant/components/binary_sensor/tod.py Outdated
@frenck

This comment has been minimized.

Copy link
Member

frenck commented Jan 14, 2019

Could not find a matching documentation PR in our documentation repository, adding docs-missing label.

@frenck frenck added the docs-missing label Jan 14, 2019

@kstaniek

This comment has been minimized.

Copy link
Contributor Author

kstaniek commented Jan 14, 2019

Documentation added.

@kstaniek kstaniek referenced this pull request Jan 14, 2019

Merged

Create binary_sensor.tod.markdown #8172

2 of 2 tasks complete
@frenck

This comment has been minimized.

Copy link
Member

frenck commented Jan 14, 2019

Thanks for opening that PR @kstaniek 👍

@frenck frenck removed the docs-missing label Jan 14, 2019

@kstaniek

This comment has been minimized.

Copy link
Contributor Author

kstaniek commented Jan 14, 2019

Guys, I've made all the changes according to the suggestions. Refactored the code. Changed the internal time to UTC to avoid DST problems. Added the test case for DST change. That's my first PR with new component. Really appreciate the reviewers' effort! @OttoWinter @dgomes @frenck

@balloob

This comment has been minimized.

Copy link
Member

balloob commented Jan 14, 2019

I wonder if we're duplicating things that can be achieved with the calendar component.

@kstaniek

This comment has been minimized.

Copy link
Contributor Author

kstaniek commented Jan 14, 2019

@balloob Paulus, with simple absolute time boundaries yes, but if you add sun events with offsets than every day the time ranges are different. I also solve the problem which I had with template based sensors, that after Hass restarts sometimes the sensors were transiting from unknown to on/off because the sun.sun component was not ready yet, I guess. As I wrote at one of my comment above I'm also planing to add 'analog' sensor with values calculated as normal distribution. So in the middle of the range the value will be 1 and on the edges will be 0 (bell curve). This could be interesting to use with ML and data analysing when you have to categorise time ranges.

@kstaniek

This comment has been minimized.

Copy link
Contributor Author

kstaniek commented Jan 15, 2019

Guys, do you see any sense of adding:

self._unique_id = "{}-{}-{}-{}-{}".format(
            name,
            after, after_offset.total_seconds(),
            before, before_offset.total_seconds())

and corresponding unique_id property?
The assumption is that if the time ranges and the name are unique than the entity is unique.

@balloob

This comment has been minimized.

Copy link
Member

balloob commented Jan 15, 2019

The problem with adding that unique ID is that changing the name will change the unique ID, and then you cannot rename the entity ID to the old one because it has been taken.

If you add config entry support, you can use config entry ID as a unique ID.

@kstaniek

This comment has been minimized.

Copy link
Contributor Author

kstaniek commented Jan 15, 2019

Ok. I just realised I don't use friendly name attribute later one in the code. How should I implement it in a proper way?. The reason I'm asking for unique id is obviously that you can easily change the name later on in GUI. But in fact using this kind of sensor will rather very uncommon to present directly on the GUI. It's rather supportive sensor to simplify the automations.

@kstaniek

This comment has been minimized.

Copy link
Contributor Author

kstaniek commented Jan 17, 2019

Can you merge it to 86 or it's too late?

@OttoWinter
Copy link
Member

OttoWinter left a comment

No, the HA 0.86.0 beta has already been cut off - plus this PR has unresolved comments.

For configuration format, please see #20068 (comment)

Show resolved Hide resolved homeassistant/components/binary_sensor/tod.py Outdated

@kstaniek kstaniek force-pushed the kstaniek:tod branch from d6af64a to 1975f13 Feb 2, 2019

@kstaniek

This comment has been minimized.

Copy link
Contributor Author

kstaniek commented Feb 2, 2019

It's ready to be merged now.

@kstaniek

This comment has been minimized.

Copy link
Contributor Author

kstaniek commented Feb 4, 2019

Guys, any chance to merge eventually?

@kstaniek

This comment has been minimized.

Copy link
Contributor Author

kstaniek commented Feb 8, 2019

Team, any update, please?

Show resolved Hide resolved homeassistant/components/binary_sensor/tod.py Outdated
vol.Required(CONF_BEFORE): vol.Any(cv.time, vol.All(
vol.Lower, cv.sun_event)),
vol.Optional(CONF_BEFORE_OFFSET, default=timedelta(0)): cv.time_period,
vol.Required(CONF_NAME): cv.string

This comment has been minimized.

@fabaff

fabaff Feb 11, 2019

Member

Please keep it ordered.

This comment has been minimized.

@kstaniek

kstaniek Feb 12, 2019

Author Contributor

@fabaff Ordered by what? Don't understand the reason. Should I put CONF_NAME first? What is the implication?

This comment has been minimized.

@kstaniek

kstaniek Feb 12, 2019

Author Contributor

I moved the CONF_NAME to the beginning. CONF_AFTER logically is first. Because the sensor is "ON" AFTER certain point in the time and BEFORE another point in the time. Offsets are logically belonging to corresponding AFTER and BEFORE. Maybe there is another criteria I don't understand.

Show resolved Hide resolved homeassistant/components/binary_sensor/tod.py Outdated
Show resolved Hide resolved homeassistant/components/binary_sensor/tod.py Outdated
Show resolved Hide resolved homeassistant/components/binary_sensor/tod.py Outdated
Show resolved Hide resolved homeassistant/components/binary_sensor/tod.py Outdated
Show resolved Hide resolved homeassistant/components/binary_sensor/tod.py Outdated
@kstaniek

This comment has been minimized.

Copy link
Contributor Author

kstaniek commented Feb 12, 2019

@fabaff Hi Fabian, thanks for your review. I have one reply above to your comment which I don't understand. The rest is clear. Waiting for your response to push the final commit.

kstaniek and others added some commits Feb 14, 2019

@fabaff

fabaff approved these changes Feb 15, 2019

@fabaff fabaff merged commit 7b19428 into home-assistant:dev Feb 15, 2019

4 checks passed

Hound No violations found. Woof!
WIP ready for review
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@wafflebot wafflebot bot removed the in progress label Feb 15, 2019

@kstaniek kstaniek deleted the kstaniek:tod branch Feb 15, 2019

get_astral_event_date, get_astral_event_next)


class TestBinarySensorTod(unittest.TestCase):

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Feb 16, 2019

Member

We prefer standalone pytest test functions for new tests.

This comment has been minimized.

@kstaniek

kstaniek Feb 16, 2019

Author Contributor

@MartinHjelmare Is that a problem now? I missed that requirement in developer's guide. I was following the Template Sensor implementation. How could I know it's wrong? Do I need to take any action?

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Feb 16, 2019

Member

You're welcome to rewrite the tests to pytest test functions if you want. It's not required at this point since this PR is merged.

I commented so that both contributors and reviewers can take note for the future.

Our developers docs are not complete.

@balloob balloob referenced this pull request Mar 6, 2019

Merged

0.89.0 #21712

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.