-
-
Notifications
You must be signed in to change notification settings - Fork 30.9k
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 Deako integration #121132
Add Deako integration #121132
Conversation
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 we are using zeroconf, can't we use the built in zeroconf discovery method instead? It would then be 1 config entry = 1 device, and I think that's much more user friendly than having 1 config entry = n devices it can find on the network. It also allows for manual setup, which is something that is a big issue for integrations which use this discovery flow. So I'd rather see this use the normal zeroconf methods.
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
Thanks for the feedback! What is the builtin zeroconf method? I didn't see it in the docs. The Deako local integration is more like a bridge. So once you connect to one device, you then have access to all the devices since the primary communication between devices is BLE. The bridge device sends out control commands over BLE. Not all devices are on the network at once. We have some installations with over 50 devices, so that many devices on the network at once isn't feasible. They do check in periodically however. There's one device guaranteed to be online when users have setup network in the Deako app. I hope I'm correctly understanding what you're suggesting. |
I've marked this PR, as changes are requested that need to be processed. Thanks! 👍 ../Frenck |
Great thanks @joostlek. Just to be clear, the number of registered devices is not the number of devices that are found on the network, but rather the number of devices the device we connect to knows about through the Deako profile that's stored locally after being fetched from the Deako backend. I should have provided more context. The device we connect to is then the "bridge" over BLE to the other devices. The other main reason that I am passing zeroconf to the lib is that there is an address pool that is used in case of connections being dropped, devices rebooting, etc. I'll add some more context to the PR with how the devices work and how the library works. edit: @joostlek I've updated the PR description to provide some more context. Thanks for all the feedback! |
But then still we like this method with the zeroconf config flow. Because it will actively tell the user that the device is on the network and ready to set up. This is an amazing user experience, so if we can use it, we would do that |
Okay great, thanks for clarifying. I'll get these changes in. |
@joostlek I've added the config flow for this integration, adding both zeroconf discovery step and manual address input. |
Okay so I asked around because I still wasn't unsure about the zeroconf and I think I can now draw a complete picture Option A: We store the IP of the bridge and we use the classic zeroconf flow. The biggest benefit is the quick and easy user experience with that the device can be discovered without user intervention. Option B: We use the classic discovery flow. Which only discovers devices once you initiate the flow. I double checked as I wasn't sure, and I didn't want to force a way of working that didn't work. |
Option A: this is similar to what I have now, just not storing the IP of the bridge device as that can change with device reboots, but the pydeako library handles these cases since it utilizes a zeroconf service browser to maintain a queue of valid IPs in case a new bridge is needed. Is option B this here? https://developers.home-assistant.io/docs/config_entries_config_flow_handler/#discoverable-integrations-that-require-no-authentication Thanks for all the discussion. This is my first contribution to HA, so I want to make sure I'm doing things right and what I'm doing is understood. |
HA handles would handle that then. The config entry gets a unique id, and the next time a zeroconf flow is started by a new mDNS packet, it will update the host of the entry with the same unique id Option B would be what you had before. |
Okay I see what you're saying. So how would that work when we have a working bridge device, a different device gets online and advertises over mDNS? Would our config entry just get overwritten with the new IP even though we had a good IP already? Is there a way we can reject the update? |
@joostlek thanks for all the feedback. I'm learning a lot! I'll get going on some of these changes. |
- added better type safety for Deako config entries - refactored the config flow tests to use a conftest mock instead of directly patching - removed pytest.mark.asyncio test decorators
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.
Last bits
- simplify config entry type - add test for single_instance_allowed - remove light.py get_state(), only used once, no need to be separate function
Alright I believe I addressed your comments @joostlek, thanks for all the feedback! |
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.
The CI is failing, can you take a look?
Sure thing! |
@joostlek I fixed CI, is there anything else that I'm missing? |
Oh, wanted to apply it but I don't have the rights |
Co-authored-by: Joost Lekkerkerker <joostlek@outlook.com>
Let me kick off the CI. |
Sweet, thanks! Good luck over there 🤞 |
@joostlek did you survive? |
Shit. Forgot to merge and beta just has been cut. I already had the idea I forgot something. |
No worries! Thanks for all the help and the review! |
Proposed change
Add a Deako integration using the pydeako library.
Deako devices
Deako devices are smart home light switches that communicate over BLE. Typical installations range from 1 device to over 50.
There is one device that is always online, as it's acting as the remote "bridge" from the internet to other devices on the network. All other devices get online periodically to check in and get potential updates. All devices currently reboot daily. Since communication is over BLE, not all devices need to be on the network to control all devices, just one.
Pydeako
The pydeako library has two main components: discovery and connection. pydeako serves as an integration to Deako devices, allowing for control of devices (power, dim, etc) over the network locally.
Discovery takes in a zeroconf instance and collects all the current addresses being advertised by Deako devices that are currently on the local network. This discovery client will continue to update this pool of addresses as devices come on the network or fall off.
Connection happens over a socket and the pydeako library maintains this connection. If the connection is dropped, a new address is retrieved and the process starts over. If a device is connected to over a socket, the device will remain online as long as it can. The device list and controls are implementations of the Deako local API.
Type of change
Additional information
Note: I had a previous PR which got closed because I didn't have time to address comments: #102190
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
.To help with the load of incoming pull requests: