-
-
Notifications
You must be signed in to change notification settings - Fork 29.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
New intergration: Hue BLE #118635
base: dev
Are you sure you want to change the base?
New intergration: Hue BLE #118635
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to know the scanner count?
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 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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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. |
Co-authored-by: Mr. Bubbles <[email protected]>
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
),
)
Co-authored-by: Mr. Bubbles <[email protected]>
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 |
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 think that's it. A maintainer surely will request more changes but from my part there is nothing more I can say, LGTM 👍
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: