Skip to content

Expose members of deCONZ lightgroups as entity_id attribute#48348

Closed
chishm wants to merge 2 commits into
home-assistant:devfrom
chishm:deconz-lightgroup-entity_ids
Closed

Expose members of deCONZ lightgroups as entity_id attribute#48348
chishm wants to merge 2 commits into
home-assistant:devfrom
chishm:deconz-lightgroup-entity_ids

Conversation

@chishm
Copy link
Copy Markdown
Contributor

@chishm chishm commented Mar 26, 2021

Proposed change

Make deCONZ light groups mimic native HA LightGroup by reporting its members in the entity_id attribute. This allows scripts and other components (e.g. Adaptive Lighting) to discover if only some lights are on in the group, and act appropriately.

Also change the default icon to mdi:lightbulb-group to match HA LightGroup.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • 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:

  • 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.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

To help with the load of incoming pull requests:

@chishm chishm force-pushed the deconz-lightgroup-entity_ids branch from bb750fe to f7e6049 Compare March 26, 2021 04:38
Copy link
Copy Markdown
Member

@Kane610 Kane610 left a comment

Choose a reason for hiding this comment

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

Thanks for your PR!

Whats the value of exposing the lights as attributes when they are not configurable from HASS?
What happens if an entity id change?
What happens if the constellation of lights change for the group?

@chishm
Copy link
Copy Markdown
Contributor Author

chishm commented Mar 26, 2021

The motivation for this PR is to allow scripts and other components to expand groups into individual lights. The particular use-case I have in mind is to configure adaptive_lighting with a deCONZ group, then allow it to only adjust individual lights within the group that are currently turned on.

The reverse would also be possible: if all lights in a group are currently on then only a single light.turn_on is required for the group. For large groups it can greatly reduce the number of Zigbee messages being sent (which for my network seems to max out at 10 lights at a time).

If any entity_id changes, or the group constellation changes, this won't be visible until the deCONZ component is refreshed (e.g. restart HA or call deconz.device_refresh). I think this problem is already there with the group's supported features (which is the sum of the individual lights' features).

@frenck
Copy link
Copy Markdown
Member

frenck commented Mar 26, 2021

@chishm The reasoning is odd, as adaptive lighting is an integration that can access the device registry and thus already access this data. Why add a copy of this same data again?

I would not recommend on adding this, as the group represents a group of Zigbee devices, not Home Assistant entities.

@chishm
Copy link
Copy Markdown
Contributor Author

chishm commented Mar 27, 2021

@frenck The deCONZ integration doesn't currently report group members at all. The information is lost as soon as it has finished initialising. I added the entity_id attribute to match what HA LightGroup currently does, which is what adaptive_lighting uses. The problem I'm trying to solve is that if you register a deCONZ light group with adaptive_lighting, it will turn on all the lights in the group if any one of them gets turned on.

If there's a better way to make group membership information available to other integrations, in a standardised manner, then I'm happy to do it that way.

@frenck
Copy link
Copy Markdown
Member

frenck commented Mar 27, 2021

I got that my comment above was about just that.

@chishm
Copy link
Copy Markdown
Contributor Author

chishm commented Mar 27, 2021

I think I understand what you're saying. This is leaking information through the abstraction that it shouldn't, and there are better ways to solve the problem.

Thank you for taking the time to review this, @Kane610 and @frenck. I'll close this PR in favour of a light.adjust service (architecture discussion TBD).

@chishm chishm closed this Mar 27, 2021
@Kane610
Copy link
Copy Markdown
Member

Kane610 commented Mar 27, 2021

Great you see other ways of reaching your end goal through an even more generic approach!

@chishm
Copy link
Copy Markdown
Contributor Author

chishm commented Mar 27, 2021

Hopefully this isn't considered a necro-bump. Here's the architecture discussion I mentioned.

@github-actions github-actions Bot locked and limited conversation to collaborators Mar 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants