core icon indicating copy to clipboard operation
core copied to clipboard

Add RSSI and HEV sensors to LIFX devices

Open Djelibeybi opened this issue 3 years ago • 8 comments

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:

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 running python3 -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:

Djelibeybi avatar Jul 20 '22 22:07 Djelibeybi

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. :)

Djelibeybi avatar Jul 20 '22 22:07 Djelibeybi

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 !

bdraco avatar Jul 20 '22 22:07 bdraco

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.

Djelibeybi avatar Jul 21 '22 04:07 Djelibeybi

This is now rebased on top of the Button entity codebase.

Djelibeybi avatar Jul 24 '22 05:07 Djelibeybi

@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.

Djelibeybi avatar Jul 26 '22 12:07 Djelibeybi

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

bdraco avatar Aug 09 '22 18:08 bdraco

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.

Djelibeybi avatar Aug 09 '22 21:08 Djelibeybi

@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.

Djelibeybi avatar Aug 15 '22 10:08 Djelibeybi

@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.

Djelibeybi avatar Aug 22 '22 10:08 Djelibeybi

@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.

bdraco avatar Aug 26 '22 18:08 bdraco

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.

Djelibeybi avatar Aug 26 '22 21:08 Djelibeybi

Closing this PR and opening new PRs per platform. First new PR: https://github.com/home-assistant/core/pull/77535

Djelibeybi avatar Aug 30 '22 09:08 Djelibeybi