-
-
Notifications
You must be signed in to change notification settings - Fork 31.3k
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
Initial support for LIFX Ceiling SKY effect #121820
Conversation
If there are no breaking changes in the lib, please split the lib bump into its own PR |
Sure, will do so in a couple of hours. |
Bump of |
a8fed84
to
65a6352
Compare
@@ -97,7 +98,10 @@ async def async_setup_entry( | |||
"set_hev_cycle_state", | |||
) | |||
if lifx_features(device)["matrix"]: | |||
entity: LIFXLight = LIFXMatrix(coordinator, manager, entry) | |||
if device.product in {176, 177}: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice if 176/177 was a named constant from the lib or a function to tell its a ceiling light
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it would. I've asked LIFX to add this to their official products.json file, but in the meantime, there is no other programmatic way to determine what is (or is not) a Ceiling device besides the product ID. I could put this in aiolifx
, I guess while I continue to push LIFX to improve their product definitions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could put this in
aiolifx
, I guess while I continue to push LIFX to improve their product definitions.
That would be a great solution for this case 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be a great solution for Home Assistant, but I'm not sure it's a great solution for aiolifx
, to be honest. I don't want yet another technical debt because we've hacked something in before LIFX makes it part of the official spec, then I can't roll it back. I'd rather just do it here as a workaround so that other dependents of aiolifx
don't use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like update_products
is already adding/converting keys in https://github.com/aiolifx/aiolifx/blob/54c3c0f5533c532ee9709c2bee722e6f120797ba/aiolifx/update_products.py#L31
Maybe ceiling
or similar could be added in https://github.com/aiolifx/aiolifx/blob/54c3c0f5533c532ee9709c2bee722e6f120797ba/aiolifx/products_defs.py#L131
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not adding or changing any keys. It's just creating a single more easily usable dictionary from the upstream production definitions: https://github.com/LIFX/products/blob/master/products.json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please replace the unnamed numerical constants with named constants or move them to the library
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
Signed-off-by: Avi Miller <[email protected]>
Signed-off-by: Avi Miller <[email protected]>
Signed-off-by: Avi Miller <[email protected]>
a904b46
to
0c96b89
Compare
Signed-off-by: Avi Miller <[email protected]>
Still looks good after sleep. Its been a long week with the hurricane. |
Proposed change
Add support for the SKY firmware effect on LIFX Ceiling devices.
Type of change
Additional information
Checklist
ruff format homeassistant tests
)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest
.requirements_all.txt
.Updated by running
python3 -m script.gen_requirements_all
.To help with the load of incoming pull requests: