New integration: Hue BLE #118635
New integration: Hue BLE #118635
Conversation
home-assistant
bot
added
cla-signed
has-tests
new-integration
integration: hue_ble
labels
| if user_input is None: | ||
| return self.async_show_form( | ||
| step_id="user", data_schema=STEP_USER_DATA_SCHEMA, errors=errors | ||
| ) | ||
|
|
||
| try: |
There was a problem hiding this comment.
Instead of showing a form to insert name and mac address, present the user a list of discovered devices and abort if none were found. Have a look at the config flow of other BLE integrations, airthings_ble for example
There was a problem hiding this comment.
I implemented this in an earlier revision but I wasn't able to get it to work reliably, it would produce the list fine but when shown in the UI the entries in the list would be present and select-able but would be blank, i.e no text was shown where the name and mac should have been and I was not able to find a fix and was running out of time I could justify spending on it, especially since it is not the intended way to use the integration, more as a backup in case automatic Bluetooth discovery was not working for some reason.
There was a problem hiding this comment.
If you don't cache the discovered devices in self, they won't carry over to the next step. This example user flow should work with some small adjustments:
async def async_step_user(
self, user_input: dict[str, Any] | None = None
) -> ConfigFlowResult:
"""Handle the user step to pick discovered device."""
if user_input is not None:
address = user_input[CONF_ADDRESS]
title = self._discovered_devices[address]
await self.async_set_unique_id(address, raise_on_progress=False)
self._abort_if_unique_id_configured()
return self.async_create_entry(title=title, data={})
current_addresses = self._async_current_ids()
for discovery_info in async_discovered_service_info(self.hass, True):
address = discovery_info.address
if (
DISCOVERY_SVC_UUID not in discovery_info.service_uuids
or address in current_addresses
or address in self._discovered_devices
):
continue
self._discovered_devices[address] = discovery_info.name
if not self._discovered_devices:
return self.async_abort(reason="no_devices_found")
return self.async_show_form(
step_id="user",
data_schema=self.add_suggested_values_to_schema(vol.Schema(
{vol.Required(CONF_ADDRESS): vol.In(self._discovered_devices)}
), user_input
),
)
There was a problem hiding this comment.
I implemented this in a test version but it highlighted a problem I encountered previously but forgotten, using this manual method over the Bluetooth discovery method does not work if the Bluetooth discovery step detects the light since that flow will already be in progress, meaning the manual flow with the name and MAC fields will not be able to progress, which makes automatic filling of the form kind of pointless, I guess I could modify it so Bluetooth discovery does not set the ID and use some other mechanism to avoid duplicates but that seems overkill, maybe just edit the strings to point out the manual form with the name and MAC should only be used if the automatic Bluetooth discovery doesn't detect it?
Here is the code I added to test this:
errors: dict[str, str] = {}
if user_input is None:
current_addresses = self._async_current_ids()
for discovery_info in async_discovered_service_info(self.hass, True):
address = discovery_info.address
if (
UUID_HUE_IDENTIFIER in discovery_info.service_uuids
and dr.format_mac(address) not in current_addresses
): # or address in self._discovered_devices
return self.async_show_form(
step_id="user",
data_schema=self.add_suggested_values_to_schema(
STEP_USER_DATA_SCHEMA,
{CONF_NAME: discovery_info.name, CONF_MAC: address},
),
description_placeholders={"url_pairing_mode": URL_PAIRING_MODE},
errors=errors,
)
| count_scanners = async_scanner_count(hass, connectable=True) | ||
| _LOGGER.debug("Count of BLE scanners: %i", count_scanners) | ||
|
|
||
| if count_scanners < 1: | ||
| raise ConfigEntryNotReady( | ||
| "No Bluetooth scanners are available to search for the light." | ||
| ) | ||
| raise ConfigEntryNotReady("The light was not found.") |
There was a problem hiding this comment.
Why do we need to know the scanner count?
There was a problem hiding this comment.
I saw this in one of the integrations I based this on and I kept it because I'd rather the error for not being able to find it and not being able to find it because there is nothing available to search for it be different things.
There was a problem hiding this comment.
I think this is somewhat valid, but I also think this kind of check should live in some BLE integration helper so not every integration has to duplicate such code.
@bdraco, do we have some helper like this already?
| self._name = self._light.name | ||
| self._address = self._light.address | ||
| self._attr_available = self._light.available | ||
| self._attr_name = self._name | ||
| self._attr_is_on = self._light.power_state | ||
| self._attr_brightness = self._light.brightness | ||
| self._attr_color_temp = self._light.colour_temp | ||
| self._attr_xy_color = self._light.colour_xy | ||
|
|
There was a problem hiding this comment.
You should use properties for all these value updates instead:
@property
def is_on(self) -> bool:
"""Return true if device is on."""
return self._light.power_state
Also, don't update the name
|
The integration also fails with an uncatched exception. That might be an error in the library. If you can't fix this in the library you might want to catch this exception in the config flow |
Seems like my method for detecting if a device is authenticated doesn't work for all Linux systems, (It works fine on HAOS and Mint tho which is strange), I updated the library so the method returns None if it cannot determine the authentication status on a Linux system and updated the integration to ignore if it can't tell if its authenticated and proceed anyway. |
I'm still not able to connect to my hue lamp so i can't really test. (I'm getting error code 82), would be good if you work on making the library more robust and test it more thoroughly with different setups. I am testing it in the Home Assistant Devcontainer (on Windows with WSL2) and an ESPHome Bluetooth proxy. |
| @pytest.mark.parametrize( | ||
| ( | ||
| "return_device_result", | ||
| "scanner_count_result", | ||
| "connect_result", | ||
| "authenticated_result", | ||
| "connected_result", | ||
| "poll_state_result", | ||
| "expected_error", | ||
| ), |
There was a problem hiding this comment.
That's maybe too much parametrization and really getting confusing, with a quick glance, i am not able to say, what the purpose of a test is. Try to keep it at maybe 2 parameters. The tests should start with a positive test case, hence the expected behaviour without any errors, and stand by its own.
There was a problem hiding this comment.
I know its confusing but I don't have any better ideas, the validation method takes a lot of parameters and pytest does not seem to accept defaults for them, and the config flow needs 100% coverage, I could try moving the validation method outside of the config flow and the 100% coverage requirement but that feels like even worse of a solution.
There was a problem hiding this comment.
I don't agree with the comment by @tr4nt0r, the parametrization is perfectly fine 👍
|
I've marked this PR, as changes are requested that need to be processed. Thanks! 👍 ../Frenck |
That authentication error makes sense now, I never designed this system with the idea of a Bluetooth proxy in mind, I have been developing on Mint using the devcontainer to pass through my laptops Bluetooth card by editing the container file and my Home Assistant server is using a USB dongle. That being said I don't think there should be any issue using a Bluetooth proxy though I don't have one to test, if its important I can buy one to try to get it to work, I think its more likely something to do with that light working slightly differently from the model I have and can use to test, if you have a moment my test library has some example scripts which should work on Windows and could rule that out. |
| if user_input is None: | ||
| return self.async_show_form( | ||
| step_id="user", data_schema=STEP_USER_DATA_SCHEMA, errors=errors | ||
| ) | ||
|
|
||
| try: |
There was a problem hiding this comment.
If you don't cache the discovered devices in self, they won't carry over to the next step. This example user flow should work with some small adjustments:
async def async_step_user(
self, user_input: dict[str, Any] | None = None
) -> ConfigFlowResult:
"""Handle the user step to pick discovered device."""
if user_input is not None:
address = user_input[CONF_ADDRESS]
title = self._discovered_devices[address]
await self.async_set_unique_id(address, raise_on_progress=False)
self._abort_if_unique_id_configured()
return self.async_create_entry(title=title, data={})
current_addresses = self._async_current_ids()
for discovery_info in async_discovered_service_info(self.hass, True):
address = discovery_info.address
if (
DISCOVERY_SVC_UUID not in discovery_info.service_uuids
or address in current_addresses
or address in self._discovered_devices
):
continue
self._discovered_devices[address] = discovery_info.name
if not self._discovered_devices:
return self.async_abort(reason="no_devices_found")
return self.async_show_form(
step_id="user",
data_schema=self.add_suggested_values_to_schema(vol.Schema(
{vol.Required(CONF_ADDRESS): vol.In(self._discovered_devices)}
), user_input
),
)
I have obtained an ESP32 and set it up as a Bluetooth proxy and it seems to work fine, I think the error must be to do with that specific model of light, its probably controlled using different GATT addresses. |
Hmm, that's weird, because it is also an LCA006 that I have |
| count_scanners = async_scanner_count(hass, connectable=True) | ||
| _LOGGER.debug("Count of BLE scanners: %i", count_scanners) | ||
|
|
||
| if count_scanners < 1: | ||
| raise ConfigEntryNotReady( | ||
| "No Bluetooth scanners are available to search for the light." | ||
| ) | ||
| raise ConfigEntryNotReady("The light was not found.") |
There was a problem hiding this comment.
I think this is somewhat valid, but I also think this kind of check should live in some BLE integration helper so not every integration has to duplicate such code.
@bdraco, do we have some helper like this already?
| light = HueBleLight(ble_device) | ||
|
|
||
| if not await light.connect() or not await light.poll_state(): | ||
| raise ConfigEntryNotReady("Device found but unable to connect.") |
There was a problem hiding this comment.
So, we do a lot of checks here to ensure the light can be found when setting up the integration.
What happens if the light is found now, but connection is then lost, does the HueBLE library automatically reconnect?
There was a problem hiding this comment.
It automatically attempts to reconnect once but if that does not work it marks itself as unavailable, but any command sent to it afterwards will initiate a reconnection attempt.
There was a problem hiding this comment.
Can you explain this in some more detail please? Who needs to send a command to trigger a reconnection attempt?
There was a problem hiding this comment.
It looks like I had gotten myself confused with how it worked in an earlier iteration.
The way the library currently works is that if it successfully connects and then becomes disconnected without the disconnect method being called it will attempt to reconnect indefinitely. No command is needed to trigger a re-connection attempt, it does it by itself.
If the maximum re-connect attempts variable was not set to -1 then it would stop after however many attempts, in that case if any command was sent requiring communication (e.g poll_status, set_power, etc) it would then try to re-connect 3 more times by default before giving up. So basically it will auto-reconnect by itself but in addition to that any command which requires a connection to work will also cause a re-connection attempt if it was disconnected.
| _LOGGER = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool: |
There was a problem hiding this comment.
Add this:
type HueBLEConfigEntry = ConfigEntry[HueBleLight]
...
async def async_setup_entry(hass: HomeAssistant, entry: HueBLEConfigEntry ) -> bool:
...
async def async_unload_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool:There was a problem hiding this comment.
You marked this as resolved, but you forgot to update the type annotations of entry passed to async_setup_entry + async_unload_entry, please fix that.
|
|
||
| STEP_USER_DATA_SCHEMA = vol.Schema( | ||
| { | ||
| vol.Required(CONF_NAME): str, | ||
| vol.Required(CONF_MAC): str, |
There was a problem hiding this comment.
Requiring the user to manually input a MAC is not friendly, and it will also not work if our bluetooth scanners have not already found the device.
Instead, allow the user to select from a list of previously discovered matching lights, something like this:
core/homeassistant/components/dormakaba_dkey/config_flow.py
Lines 47 to 92 in 95b532d
There was a problem hiding this comment.
The primary way a user is meant to connect is through bluetooth discovery where it automatically shows up in the UI, I included this backup mechanism since I have no way of knowing if the identifier currently used to detect it is the same across all lights as I only have one model of light to test with, I can remove it if its too unfriendly.
There was a problem hiding this comment.
OK I see.
Raw input of MAC is too unfriendly IMO, it's better to update the manifest with new filters as the need arises.
| hass.async_create_task( | ||
| hass.config_entries.async_forward_entry_setups(entry, ["light"]) | ||
| ) |
There was a problem hiding this comment.
Use await instead of creating a task
| entry.runtime_data = light | ||
|
|
||
| hass.async_create_task( | ||
| hass.config_entries.async_forward_entry_setups(entry, ["light"]) |
| """Initialize the light object. Does not connect.""" | ||
|
|
||
| self._light = api | ||
| self._name = self._light.name |
There was a problem hiding this comment.
Because Hue lights have names you can set in the app and they are accessible via the API. Should I be using a different attribute for that?
There was a problem hiding this comment.
We can set that as the device name
| self._attr_unique_id = self._light.address | ||
| self._attr_available = self._light.available | ||
| self._attr_is_on = self._light.power_state | ||
| self._attr_brightness = self._light.brightness | ||
| self._attr_color_temp_kelvin = color_util.color_temperature_mired_to_kelvin( | ||
| self._light.colour_temp | ||
| ) | ||
| self._attr_xy_color = self._light.colour_xy | ||
| self._attr_min_color_temp_kelvin = color_util.color_temperature_mired_to_kelvin( | ||
| self._light.maximum_mireds | ||
| ) | ||
| self._attr_max_color_temp_kelvin = color_util.color_temperature_mired_to_kelvin( | ||
| self._light.minimum_mireds | ||
| ) |
There was a problem hiding this comment.
I would assume that this code is also used for normal updates. I would put that in a separate method to DRY
| # Silver | ||
| action-exceptions: todo | ||
| config-entry-unloading: todo | ||
| docs-configuration-parameters: todo | ||
| docs-installation-parameters: todo | ||
| entity-unavailable: todo | ||
| integration-owner: todo | ||
| log-when-unavailable: todo | ||
| parallel-updates: todo | ||
| reauthentication-flow: todo | ||
| test-coverage: todo | ||
|
|
||
| # Gold | ||
| devices: todo | ||
| diagnostics: todo | ||
| discovery-update-info: todo | ||
| discovery: todo | ||
| docs-data-update: todo | ||
| docs-examples: todo | ||
| docs-known-limitations: todo | ||
| docs-supported-devices: todo | ||
| docs-supported-functions: todo | ||
| docs-troubleshooting: todo | ||
| docs-use-cases: todo | ||
| dynamic-devices: todo | ||
| entity-category: todo | ||
| entity-device-class: todo | ||
| entity-disabled-by-default: todo | ||
| entity-translations: todo | ||
| exception-translations: todo | ||
| icon-translations: todo | ||
| reconfiguration-flow: todo | ||
| repair-issues: todo | ||
| stale-devices: todo | ||
|
|
||
| # Platinum | ||
| async-dependency: todo | ||
| inject-websession: todo | ||
| strict-typing: todo |
| async_add_entities([HaHueBLE(light)]) | ||
|
|
||
|
|
||
| class HaHueBLE(LightEntity): |
There was a problem hiding this comment.
| class HaHueBLE(LightEntity): | |
| class HueBLELight(LightEntity): |
| self._light = api | ||
| self._name = self._light.name | ||
| self._address = self._light.address | ||
| self._attr_unique_id = self._light.address |
There was a problem hiding this comment.
| self._attr_unique_id = self._light.address | |
| self._attr_unique_id = api.address |
| self._attr_device_info = DeviceInfo( | ||
| connections={(CONNECTION_BLUETOOTH, self._light.address)}, | ||
| manufacturer=self._light.manufacturer, | ||
| model=self._light.model, |
There was a problem hiding this comment.
What does this return? Is this the model or the model id?
There was a problem hiding this comment.
Model ID. For my lights its LCA006
There was a problem hiding this comment.
Let's set it to the model id then
| if self._light.supports_colour_xy: | ||
| supported_modes.add(ColorMode.XY) | ||
|
|
||
| if self._light.supports_colour_temp: | ||
| supported_modes.add(ColorMode.COLOR_TEMP) | ||
|
|
||
| if self._light.supports_brightness and len(supported_modes) == 0: | ||
| supported_modes.add(ColorMode.BRIGHTNESS) | ||
|
|
||
| if self._light.supports_on_off and len(supported_modes) == 0: |
There was a problem hiding this comment.
Can this change on runtime? If not we can set this in the constructor
| unique-config-entry: done | ||
|
|
||
| # Silver | ||
| action-exceptions: done |
There was a problem hiding this comment.
We currently don't catch exceptions raised by for example turning on the light
There was a problem hiding this comment.
Is it supposed to? The docs just say actions need to raise an exception, not catch it. If I try to turn the light on while its unplugged I get Failed to perform the action light.turn_on. Unknown error. in the web UI.
There was a problem hiding this comment.
Sorry, rephrase, we need to rethrow the exceptions as HomeAssistantError so we can set the error message
There was a problem hiding this comment.
Fair enough, I think that's a problem for later, ill mark it as todo for now.
| "config": { | ||
| "step": { | ||
| "confirm": { | ||
| "description": "Do you want to set up {name} ({mac})?. Make sure the light is in [Alexa/Google pairing mode.]({url_pairing_mode})" |
There was a problem hiding this comment.
Is this pairing really called Alexa or Google pairing mode?
There was a problem hiding this comment.
The more general term they use is voice assistant pairing mode but both UIs do the same thing, they put the lights in a mode where they will accept any pair requests which is needed to control them. They will also accept pair requests when they have been factory reset but I find its easier to do it via the Hue app, that way they can still be controlled if something goes wrong and it also allows me to test that the state in HA updates when I change it in the Hue app.
| Page 1 | Page 2 | Page 3 |
|---|---|---|
 |
 |
 |
There was a problem hiding this comment.
I would suggest to make the text "Make sure the light is [made discoverable to voice assistants]." to match the UI. Then in the documentation we explain where to find this option.
| "config": { | ||
| "step": { | ||
| "confirm": { | ||
| "description": "Do you want to set up {name} ({mac})?. Make sure the light is in [Alexa/Google pairing mode.]({url_pairing_mode})" |
There was a problem hiding this comment.
So for example with oral B, we got a lot of comments and feedback that this caused a lot of false discoveries, for example by neighbours. Does every hue light already emit a BLE signal? Or only ones that are not connected to the hub?
There was a problem hiding this comment.
They added in Bluetooth with the 4th generation lights which came out around 2019, so most don't have it. From my testing they always seem to broadcast via Bluetooth regardless of how they are set up.
There was a problem hiding this comment.
Is there a way to decrease the amount of discoveries? As in, can we read from the light if it's connected to a bridge? Because if we tried to match every device, I would expect it to discover a lot of devices nowadays (keep in mind 2019 was 6 years ago). So the more specific we can match (as in, if we know that someone is looking to add them), the better
There was a problem hiding this comment.
Not as far as I can tell, you can get a decent amount of information even without being paired and I can see that some stuff does differ between it being connected/disconnected from a ZigBee network but I haven't been able to find a pattern that's consistent across the two different lights I have and other things like Bluetooth pairing status.
| with patch( | ||
| "homeassistant.components.hue_ble.config_flow.validate_input", | ||
| return_value=True, | ||
| ): |
There was a problem hiding this comment.
We should not patch internals, instead mock out the library and make sure it responds like we expect
| assert result["type"] is FlowResultType.CREATE_ENTRY | ||
| assert result["title"] == TEST_DEVICE_NAME | ||
| assert result["result"].unique_id == dr.format_mac(TEST_DEVICE_MAC) |
| async def test_validation( | ||
| hass: HomeAssistant, | ||
| mock_setup_entry: AsyncMock, | ||
| return_device_result, | ||
| scanner_count_result, | ||
| connect_result, | ||
| authenticated_result, | ||
| connected_result, | ||
| poll_state_result, | ||
| expected_result, | ||
| ) -> None: | ||
| """Test input validation.""" | ||
| with ( | ||
| patch( | ||
| "homeassistant.components.hue_ble.config_flow.async_ble_device_from_address", | ||
| return_value=return_device_result, | ||
| ), | ||
| patch( | ||
| "homeassistant.components.hue_ble.config_flow.async_scanner_count", | ||
| return_value=scanner_count_result, | ||
| ), | ||
| patch( | ||
| "homeassistant.components.hue_ble.config_flow.HueBleLight", | ||
| autospec=True, | ||
| authenticated=authenticated_result, | ||
| connected=connected_result, | ||
| ) as mock_obj, | ||
| ): | ||
| client = mock_obj.return_value | ||
| client.connect.return_value = connect_result | ||
| client.authenticated = authenticated_result | ||
| client.connected = connected_result | ||
| client.poll_state.return_value = poll_state_result | ||
|
|
||
| with expected_result: | ||
| await validate_input(hass, TEST_DEVICE_MAC) |
There was a problem hiding this comment.
Yea we shouldn't test the internals like this
There was a problem hiding this comment.
We should also add
- the manual user flow
- The manual user flow with exceptions
- Trying to set up an already set up device
- Same for that for Bluetooth
Proposed change
This integration allows for the newer Phillips Hue lights with Bluetooth to
be controlled over Bluetooth by Home Assistant instead of having to use
a Zigbee dongle. I think support for this is quite important because many
people (myself at least) trialed Home Assistant on hardware they have
lying around like a raspberry pi, old laptop, or mini pc, which have
Bluetooth built in but no Zigbee support and don't want to spend money
on hardware they don't necessarily need, at least without knowing that its
worth it.
The features are as follows:
Type of change
Additional information
This is my first real integration for Home Assistant and also the first
Python library I have programmed so I have probably made a few mistakes
but I've been testing it for the past few months and have ironed out the issues
as they have appeared, I've been using the latest version for the past two
weeks and haven't gotten any errors or reliability issues, yet anyway...
The library can be found here and the docs here
I only have the LCA006 model which is 1100 lumens which does RGB and
WW/CW so that is the only model confirmed to work but I have tried to design
the library and integration to detect the supported features and set the flags
accordingly but this is untested on other models, if anyone happens to have
one of the other models that supports Bluetooth and could test it for me that
would be great.
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..coveragerc.library changelog
To help with the load of incoming pull requests: