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

Add battery binary sensor to homematic #23067

Merged
merged 6 commits into from
May 9, 2019
Merged

Add battery binary sensor to homematic #23067

merged 6 commits into from
May 9, 2019

Conversation

sander76
Copy link
Contributor

Description:

Each homematic device which is battery operated will have a binary battery sensor device in hass.

@sander76
Copy link
Contributor Author

I am getting an error:

AttributeError: 'CompConfigFlow' object has no attribute 'VERSION'

I doubt it's related to my PR. Can anyone give some clues where to start looking for solving this ?

@codecov
Copy link

codecov bot commented Apr 17, 2019

Codecov Report

Merging #23067 into dev will decrease coverage by 0.1%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             dev   #23067      +/-   ##
=========================================
- Coverage   94.2%   94.09%   -0.11%     
=========================================
  Files        453      452       -1     
  Lines      36913    36795     -118     
=========================================
- Hits       34773    34623     -150     
- Misses      2140     2172      +32
Impacted Files Coverage Δ
homeassistant/bootstrap.py 59.25% <0%> (-15.15%) ⬇️
homeassistant/components/hassio/ingress.py 67.76% <0%> (-10.29%) ⬇️
homeassistant/components/axis/device.py 93.02% <0%> (-6.98%) ⬇️
...ssistant/components/islamic_prayer_times/sensor.py 94.68% <0%> (-1.07%) ⬇️
homeassistant/loader.py 93.15% <0%> (-0.72%) ⬇️
homeassistant/components/axis/camera.py 90.24% <0%> (-0.67%) ⬇️
...ssistant/components/google_assistant/smart_home.py 93.1% <0%> (-0.23%) ⬇️
homeassistant/components/demo/switch.py 100% <0%> (ø) ⬆️
homeassistant/components/hue/bridge.py 73.07% <0%> (ø) ⬆️
homeassistant/components/zwave/lock.py 97.77% <0%> (ø) ⬆️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 37ca9ca...f8910ee. Read the comment docs.

@codecov
Copy link

codecov bot commented Apr 17, 2019

Codecov Report

Merging #23067 into dev will decrease coverage by 0.1%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             dev   #23067      +/-   ##
=========================================
- Coverage   94.2%   94.09%   -0.11%     
=========================================
  Files        453      452       -1     
  Lines      36913    36795     -118     
=========================================
- Hits       34773    34623     -150     
- Misses      2140     2172      +32
Impacted Files Coverage Δ
homeassistant/bootstrap.py 59.25% <0%> (-15.15%) ⬇️
homeassistant/components/hassio/ingress.py 67.76% <0%> (-10.29%) ⬇️
homeassistant/components/axis/device.py 93.02% <0%> (-6.98%) ⬇️
...ssistant/components/islamic_prayer_times/sensor.py 94.68% <0%> (-1.07%) ⬇️
homeassistant/loader.py 93.15% <0%> (-0.72%) ⬇️
homeassistant/components/axis/camera.py 90.24% <0%> (-0.67%) ⬇️
...ssistant/components/google_assistant/smart_home.py 93.1% <0%> (-0.23%) ⬇️
homeassistant/components/demo/switch.py 100% <0%> (ø) ⬆️
homeassistant/components/hue/bridge.py 73.07% <0%> (ø) ⬆️
homeassistant/components/zwave/lock.py 97.77% <0%> (ø) ⬆️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 37ca9ca...3c06fad. Read the comment docs.

@sander76
Copy link
Contributor Author

@pvizeli @danielperna84 Hi Guys,
The build fails as I have still a todo marked inside the code. But before I proceed I'd like to know whether you think my proposed solution is ok.

(Any help/suggestion on the todo is welcome too !)

Thanks !

@MartinHjelmare MartinHjelmare changed the title Adding battery binary sensor to homematic platform. Add battery binary sensor to homematic Apr 23, 2019
Copy link
Member

@pvizeli pvizeli left a comment

Choose a reason for hiding this comment

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

We should follow the core rules and implement that one: https://developers.home-assistant.io/docs/en/entity_index.html#standard-attributes

@sander76
Copy link
Contributor Author

We should follow the core rules and implement that one: https://developers.home-assistant.io/docs/en/entity_index.html#standard-attributes

@pvizeli I don't understand. This pr is a substitute for #22594
Here I was advised to implement a binary sensor with "battery" as device class. Which I try to do here. I guess your suggestion is additional to what I try to do here ?

@sander76
Copy link
Contributor Author

@pvizeli See my comment above. Can you explain your comment a bit more ?

@sander76
Copy link
Contributor Author

sander76 commented May 2, 2019

@balloob you asked for changing my battery alert approach in my pr #22594

That's what I'm trying to do here, but now I'm advised to do something else. Can you help me out here?

@balloob
Copy link
Member

balloob commented May 9, 2019

Both approaches are good. The approach that Pascal refers to is preferred, since it gives more information than just low battery. Binary sensors for low battery is a valid approach too, as it's one of the approved device classes https://developers.home-assistant.io/docs/en/entity_binary_sensor.html#available-device-classes

@balloob balloob merged commit 8ef3c6d into home-assistant:dev May 9, 2019
@sander76 sander76 deleted the hm_battery_device branch May 10, 2019 06:41
@balloob balloob mentioned this pull request Jun 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants