core
core copied to clipboard
Add RSSI and HEV sensors to LIFX devices
Proposed change
Adds RSSI signal strength sensors to all LIFX devices and HEV sensors to LIFX Clean devices.
Type of change
- [ ] Dependency upgrade
- [ ] Bugfix (non-breaking change which fixes an issue)
- [ ] New integration (thank you!)
- [X] New feature (which adds functionality to an existing integration)
- [ ] Deprecation (breaking change to happen in the future)
- [ ] Breaking change (fix/feature causing existing functionality to break)
- [ ] Code quality improvements to existing code or addition of tests
Additional information
- This PR fixes or closes issue: fixes #
- This PR is related to issue:
- Link to documentation pull request:
Checklist
- [X] The code change is tested and works locally.
- [X] Local tests pass. Your PR cannot be merged unless tests pass
- [X] There is no commented out code in this PR.
- [X] I have followed the development checklist
- [X] The code has been formatted using Black (
black --fast homeassistant tests) - [X] Tests have been added to verify that the new code works.
If user exposed functionality or configuration variables are added/changed:
- [ ] Documentation added/updated for www.home-assistant.io
If the code communicates with devices, web services, or third-party tools:
- [X] The manifest file has all fields filled out correctly.
Updated and included derived files by running:python3 -m script.hassfest. - [ ] New or updated dependencies have been added to
requirements_all.txt.
Updated by runningpython3 -m script.gen_requirements_all. - [ ] For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
- [ ] Untested files have been added to
.coveragerc.
The integration reached or maintains the following Integration Quality Scale:
- [ ] No score or internal
- [ ] 🥈 Silver
- [ ] 🥇 Gold
- [X] 🏆 Platinum
To help with the load of incoming pull requests:
- [ ] I have reviewed two other open pull requests in this repository.
Hey there @bdraco, mind taking a look at this pull request as it has been labeled with an integration (lifx) you are listed as a code owner for? Thanks!
(message by CodeOwnersMention)
@bdraco this is my attempt at adding sensor entites. I picked one that is available to all bulbs and some that are only available to a subset of bulbs. In draft fo review so I can fix anything I got wrong. :)
Will try to give you some more feedback tomorrow. I'm way behind as I've had two A/C units fail this week @ home which is not good in a Texas summer !
I have it working as requested, but I'm not sure if I did it properly or not. Decided to stop here and give you a chance to review again before doing anything more.
This is now rebased on top of the Button entity codebase.
@bdraco when you get a chance, could you review the tests I added? I'm fairly certain I picked the least efficient way of testing things, so would appreciate an experienced eye.
On that note, I'll probably stop adding entities until this lot is merged and work on replacing aiolifx with Photons or something closely resembling it.
I won't have time to do a proper review until tomorrow, but in the mean time I'll give feedback on the things I know will require refactoring
I'm on leave until next week, so I'll either have plenty of time for this or none at all. 😄 Worst case, I'll get back into it next Monday/Tuesday, as I'm away this weekend for sure.
@bdraco I have some personal stuff I need to focus on this week, so I won't have much time to look at this until next week. It can certainly wait, but if you felt like tinkering on it, be my guest.
@bdraco I refactored the HEV sensors into a single binary sensor with extra state attributes which provides a much better user experience (IMO). At a glance, you can see if a HEV cycle is running (and firing automations on start/finish is possible) while still containing enough extra data for dashboard output or statistical analysis.
I realise this PR now has two platforms instead of one, but it would be super peachy keen if I don't have to split it into two seperate PRs.
Updated/new tests still to come.
@bdraco I refactored the HEV sensors into a single binary sensor with extra state attributes which provides a much better user experience (IMO). At a glance, you can see if a HEV cycle is running (and firing automations on start/finish is possible) while still containing enough extra data for dashboard output or statistical analysis.
Oh no, in general we are moving away from extra state attributes. They can't be assigned state classes, can't use long term stats, can't be deduplicated in the database, are hard to discover, are more difficult to use in automations for first time users, etc.
I realise this PR now has two platforms instead of one, but it would be super peachy keen if I don't have to split it into two seperate PRs.
Updated/new tests still to come.
It would be better to split it into two PRs, but I think you already know that, or you wouldn't have asked. If everything were super strait-forward here, it would probably be fine, but I don't want either of us to end up frustrated in the review process, so it is probably better to split it up.
Oh no, in general we are moving away from extra state attributes. They can't be assigned state classes, can't use long term stats, can't be deduplicated in the database, are hard to discover, are more difficult to use in automations for first time users, etc.
These are all the reasons why I switched to them for this, to be honest. But I guess I'll split them back out again and split this PR accordingly. Or close this one and open two new ones or something.
Closing this PR and opening new PRs per platform. First new PR: https://github.com/home-assistant/core/pull/77535