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

Add native ESPHome API binary sensor #19371

Merged
merged 3 commits into from Dec 17, 2018

Conversation

@OttoWinter
Copy link
Contributor

OttoWinter commented Dec 17, 2018

Description:

There will be a couple of PRs like this one for different platforms. And all of them need to edit the same file: esphome/__init__.py to register the platform.

To save all the "please rebase on dev" stuff, I would propose the following: First review as usual, but I'll do the merging - so then I can go through all PRs in order, rebase them and click merge myself (saves the entire delay of GH communication).

Also, please post all review comments that affect all ESPHome platform here.

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.

OttoWinter added some commits Dec 16, 2018


@property
def _static_info(self) -> 'BinarySensorInfo':
return super()._static_info

This comment has been minimized.

@balloob

balloob Dec 17, 2018

Member

Why implement this if you're super'ing it?

This comment has been minimized.

@OttoWinter

OttoWinter Dec 17, 2018

Contributor

See #19334 (comment) - it's for static typing.

I tried to do it like this too:

@property
def _static_info(self) -> 'BinarySensorInfo': ...

with variations of including/excluding @property and @overload but I couldn't get it to work.

@property
def is_on(self):
"""Return true if the binary sensor is on."""
if self._static_info.is_status_binary_sensor:

This comment has been minimized.

@balloob

balloob Dec 17, 2018

Member

Is there really a need for an availability binary sensor if entities just become unavailable?

This comment has been minimized.

@OttoWinter

OttoWinter Dec 17, 2018

Contributor

Then you can get nice graphs like this:

screen shot 2018-12-17 at 15 36 57

with sensor you pretty much can't see the small blip when it's unavailable. Another option would be to declare a binary_sensor that's always on, but

  • a) that's quite a hack and
  • b) these status binary sensors have device class connectivity. For device class connectivity ON means connected and OFF means disconnected. unavailable doesn't really have a meaning for those.
@balloob

This comment has been minimized.

Copy link
Member

balloob commented Dec 17, 2018

Ship it

@OttoWinter

This comment has been minimized.

Copy link
Contributor

OttoWinter commented Dec 17, 2018

Let the merge conflict hell begin 😆🎉

@OttoWinter OttoWinter merged commit 8c67ebc into home-assistant:dev Dec 17, 2018

5 checks passed

Hound No violations found. Woof!
WIP Legacy commit status override — see details
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.009%) to 93.018%
Details

@wafflebot wafflebot bot removed the in progress label Dec 17, 2018

@OttoWinter OttoWinter deleted the OttoWinter:esphome-binary-sensor branch Dec 17, 2018

"""Return True if entity is available."""
if self._static_info.is_status_binary_sensor:
return True
return super(EsphomeEntity, self).available

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Dec 17, 2018

Member
return super().available
HA_COMPONENTS = ['sensor']
HA_COMPONENTS = [
'sensor',
'binary_sensor'

This comment has been minimized.

@MartinHjelmare

@OttoWinter OttoWinter referenced this pull request Dec 18, 2018

Merged

Miscellaneous ESPHome cleanups #19425

3 of 3 tasks complete

dshokouhi added a commit to dshokouhi/home-assistant that referenced this pull request Dec 25, 2018

Add native ESPHome API binary sensor (home-assistant#19371)
* Add esphomelib native API binary sensor

* Fixes

* 🚑 Lint

@balloob balloob removed the new-platform label Jan 9, 2019

@balloob balloob referenced this pull request Jan 10, 2019

Merged

0.85.0 #19897

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment