Skip to content

Add support for custom configurations in ZHA#48423

Merged
dmulcahey merged 7 commits intohome-assistant:devfrom
dmulcahey:dm/zha-configuration-enhancements
Apr 12, 2021
Merged

Add support for custom configurations in ZHA#48423
dmulcahey merged 7 commits intohome-assistant:devfrom
dmulcahey:dm/zha-configuration-enhancements

Conversation

@dmulcahey
Copy link
Copy Markdown
Contributor

@dmulcahey dmulcahey commented Mar 27, 2021

Proposed change

This PR adds websocket APIs that allow users to retrieve and update configurations for ZHA. Initial support is added for setting a default transition time and for enabling / disabling the identity effect when devices are paired. Eventually this will be used for advanced network configuration as well.

image

can be a work around to disable the effect in #48124

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

Example entry for configuration.yaml:

# Example configuration.yaml

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • Link to documentation pull request:

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:

@probot-home-assistant
Copy link
Copy Markdown

Hey there @Adminiuga, mind taking a look at this pull request as its been labeled with an integration (zha) you are listed as a codeowner for? Thanks!
(message by CodeOwnersMention)

@Adminiuga
Copy link
Copy Markdown
Contributor

Lemme know when it is no longer draft. So far looks good

@dmulcahey dmulcahey force-pushed the dm/zha-configuration-enhancements branch from 9c0d439 to 8a26707 Compare April 11, 2021 10:49
@dmulcahey dmulcahey marked this pull request as ready for review April 11, 2021 11:33
@dmulcahey dmulcahey requested a review from Adminiuga April 11, 2021 11:33
Copy link
Copy Markdown
Contributor

@Adminiuga Adminiuga left a comment

Choose a reason for hiding this comment

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

Lgtm

@dmulcahey dmulcahey merged commit fe80afd into home-assistant:dev Apr 12, 2021

CONF_ZHA_OPTIONS_SCHEMA = vol.Schema(
{
vol.Optional(CONF_DEFAULT_LIGHT_TRANSITION): cv.positive_int,
Copy link
Copy Markdown
Member

@frenck frenck Apr 12, 2021

Choose a reason for hiding this comment

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

Maybe this should not be a ZHA-specific thing...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

what’s the protocol here? I was under the impression that things usually start out in integrations and then back their way into core...

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There is no protocol, it has been discussed more often. If the PR title reflected the actual features in this PR, I would have responded to it way earlier.

IMHO, use the effort to solve this globally.

@frenck
Copy link
Copy Markdown
Member

frenck commented Apr 12, 2021

@dmulcahey @Adminiuga This PR should not have been merged or presented this way.

There are multiple unrelated things in this PR that should not have been a single PR at all.
I think the default light transition configuration, doesn't belong here for example, but this fact was hidden because the PR title didn't show.

I suggest reverting this PR and splitting all features into separate PRs.

@dmulcahey
Copy link
Copy Markdown
Contributor Author

dmulcahey commented Apr 12, 2021

This PR adds custom configurations to ZHA. Everything in it is in fact related to that. There are literally 2 configuration options supported at this time (both are even in the screenshot in the description)

How would you have split this up? The WS API additions do nothing at all without options to present?

Both of the options I selected for the initial implementation target frequent user complaints/ issues. I honestly don’t see the point to backing this out but I’m willing to work towards a resolution. I noticed you had previously emoted the description is there a reason you didn’t comment then if you felt this way?

@frenck
Copy link
Copy Markdown
Member

frenck commented Apr 12, 2021

There are 2 features here, that are packed as "custom options". IMHO, this should have been split into at least 2 PRs.

This PR is not about the custom options by itself, it is about 2 new features that can be enabled using a custom option.

I would object to the default light transition, however, that almost went under the radar for me, as the description doesn't reflect the added features.

@dmulcahey
Copy link
Copy Markdown
Contributor Author

I’m sorry if that’s how it seems? It’s noted in the description and it’s in the screenshot in the PR. Not sure how I could have made that clearer. I can put up another PR to remove that option if you want?

@frenck
Copy link
Copy Markdown
Member

frenck commented Apr 12, 2021

It feels like a discussion after the fact, but I do think we should solve a default transition on a global level within Home Assistant and I do think this PR has 2 separate user-facing features.

But if I'm alone on that 🤷 that's fine as well.

@TheJulianJES
Copy link
Copy Markdown
Member

TheJulianJES commented Apr 12, 2021

It feels like a discussion after the fact, but I do think we should solve a default transition on a global level within Home Assistant [...]

I think this should also be done properly (at some point). I just wanted to mention that this value, which can (finally) be configured through this PR, is not a default transition value that always applies to ZHA lights. Rather, Zigbee lights have their own default transition time (which can -- depending on the device -- be specified in the "cluster settings"). This only applies to turning on/off the light through the OnOff cluster. The LevelControl cluster: changing brightness and the Color cluster: changing color both always require a transition time. Previously, this was set to 0.1 seconds and could not be changed easily (only via a custom component). This PR makes that easily changeable (which I really like).

The "global default transition time" for lights (turning on/off) can already be changed by using light profiles. (The color is optional since #44079 was merged.)
This had no effect on the previous 0.1 second ZHA transition value constant and still has no effect on this configurable ZHA transition time (which is what I would expect).
(Basically, I think this change is a step in the right direction.)

@github-actions github-actions Bot locked and limited conversation to collaborators Apr 13, 2021
@dmulcahey dmulcahey deleted the dm/zha-configuration-enhancements branch November 11, 2022 21:21
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.

6 participants