Skip to content
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

Add enable and disable config entry services #77457

Conversation

ShaneQi
Copy link
Contributor

@ShaneQi ShaneQi commented Aug 28, 2022

Proposed change

Adding 2 services to enable and disable integrations/config-entries.

I ran into a situation where I need to use automation to enable and disable (NOT reload) an integration.
I didn't find such services and found some discussions confirming that it's a missing feature:

https://community.home-assistant.io/t/programmatically-disable-enable-integration/322971
https://community.home-assistant.io/t/disabling-enabling-an-integration-automatically/437038

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)
  • 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

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.
  • 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
  • 🏆 Platinum

To help with the load of incoming pull requests:

@probot-home-assistant
Copy link

Hey there @home-assistant/core, mind taking a look at this pull request as it has been labeled with an integration (homeassistant) you are listed as a code owner for? Thanks!
(message by CodeOwnersMention)

@ShaneQi ShaneQi force-pushed the feature/enable-disable-config-entry-services branch from b41fd0e to 16437d8 Compare August 28, 2022 20:28
@MartinHjelmare MartinHjelmare changed the title Added enable and disable config entry services. Add enable and disable config entry services Aug 28, 2022
@balloob
Copy link
Member

balloob commented Aug 29, 2022

I don't think that we should add this service. Instead, any automation interacting with this device should add a condition to not do so.

@ShaneQi
Copy link
Contributor Author

ShaneQi commented Aug 29, 2022

I don't think that we should add this service. Instead, any automation interacting with this device should add a condition to not do so.

@balloob I'm trying to understand your point, please correct me if I'm having mis-understanding:
Do you suggest that automations should never be able to enable/disable integrations?
Is it because you are concerned that automations may have too much power over integrations and may shutdown critical integrations by accident?

In my opinion, enabling and disabling integrations are basic functionalities like homeassistant.restart and homeassistant.reload_config_entry.
If we are concerned that they may be mis-used, we can make these 2 services to ONLY accept entry_id (not device nor entity_id) reducing the chances of mis-using. Also people would have to look into core.config_entries file to find the entry_id, this makes sure that they know what they are doing.

required: false
example: 8955375327824e14ba89e4b29cc3ec9a
selector:
text:
Copy link
Member

Choose a reason for hiding this comment

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

You can use a config entry selector instead

Comment on lines +81 to +83
target:
entity: {}
device: {}
Copy link
Member

Choose a reason for hiding this comment

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

This should not be targeting devices or entities.

Copy link
Member

@frenck frenck left a comment

Choose a reason for hiding this comment

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

I'm not sure if I'm against this PR or not (as in general terms of having this service). I'm not entirely sure about the use case, to be honest.

Nevertheless, I do think it should not be targeting devices or services and additionally, this PR could directly use the config_entry selector to ask the user for a config entry to enable/disable.

../Frenck

@balloob
Copy link
Member

balloob commented Aug 30, 2022

We don't allow enabling/disabling entities or devices either. We shouldn't expose any services that can alter the system configuration.

@ShaneQi
Copy link
Contributor Author

ShaneQi commented Aug 30, 2022

@frenck thanks for the review, if this PR can go further, I will address you comments by removing targeting device/entity and using config_entry selector (I found your recent PR adding the new selector).

@balloob About "expose any services that can alter the system configuration", I didn't know that but I fully respect that.
But do you think it's open to discuss the pros and cons of adding the flexibility for automation to update configs?
If it's not open for discussion, is there anything we can do to salvage this PR? Or should we just close it?

FWIW, one of the use cases of these 2 services (also the motivation of having this PR) is that:
Some integrations consume resources when running, the enable/disable-config-entry services can be used in automations to limit the integrations to only run when they are needed, to minimize the resources the integrations use.
In my case, the Bluetooth integration is consuming too much of my CPU resource. I'd like to enable it when I need it, and disable it for the rest of the time.

More details if you'd like to know
  1. The updated SwitchBot integration picked up the new Bluetooth integration infrastructure (I heard it's a great effort 👏). I used to be able to run SwitchBot integration alone and it worked, but now in order for SwitchBot to work, we have to add Bluetooth integration (the technical reason of this is that SwitchBot integration relies on Bluetooth integration to get the BLEDevice objects).
  2. But I don't really need Bluetooth integration enabled. And because it's constantly scanning bluetooth devices, it's using up 10% of my CPU. I have an i3 11th gen NUC, it's not the fastest machine but 10% is not insignificant IMO. I don't want to trade it for bluetooth discovery feature that I know I will not use (SwitchBot doesn't require the constant scanning).
  3. In order to have SwitchBot to work and not have Bluetooth integration constantly using 10% CPU, I'm planning to: enable Bluetooth integration at launch, wait for 30 seconds for SwitchBot to setup, then disable the Bluetooth integration (SwitchBot will continue working even if Bluetooth integration is disabled, because all SwitchBot needs is the BLEDevice object from the first 30 seconds of scanning).

I'm sure there are other angles to solve my problem, I'm sure I will figured something out if this PR goes south.

@emontnemery
Copy link
Contributor

emontnemery commented Aug 31, 2022

And because it's constantly scanning bluetooth devices, it's using up 10% of my CPU. I have an i3 11th gen NUC, it's not the fastest machine but 10% it not insignificant IMO

@bdraco It doesn't seem OK that the bluetooth scanning consumes ~10% CPU time on @ShaneQi's system. Is that a known issue?

@balloob
Copy link
Member

balloob commented Aug 31, 2022

Bluetooth performance is a known issue and there are multiple efforts in play to reduce this (bleak, python-dbus-next, OS 9).

We should not add this service.

@balloob balloob closed this Aug 31, 2022
@bdraco
Copy link
Member

bdraco commented Aug 31, 2022

In the mean time, if you update to haos 9.x and patch python-dbus-next with this PR you should see the cpu drop quite a bit altdesktop/python-dbus-next#126

@bdraco
Copy link
Member

bdraco commented Aug 31, 2022

I know it's a bit late here, but disabling the scanner will make switchbot eventually fail since there is nothing to keep the device registered on the bus. As soon as bluez can't see if for it a bit it will remove it from the bus and you'd have to start the scanner back up to get it connectable again.

@ShaneQi
Copy link
Contributor Author

ShaneQi commented Aug 31, 2022

@balloob @emontnemery thanks for pointing to the right direction.

@bdraco thanks for all work on Bluetooth, I will check out your PR and try it out.

@bdraco
Copy link
Member

bdraco commented Aug 31, 2022

Also if you aren't using HAOS, dump the default dbus broker and replace it with https://github.com/bus1/dbus-broker as you'll save quite a bit of cpu time there and all your bluetooth devices will be more responsive. HAOS 9.x will do this.

@github-actions github-actions bot locked and limited conversation to collaborators Sep 1, 2022
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