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 Deako integration #121132

Merged
merged 6 commits into from
Aug 28, 2024
Merged

Add Deako integration #121132

merged 6 commits into from
Aug 28, 2024

Conversation

Balake
Copy link
Contributor

@Balake Balake commented Jul 3, 2024

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

  • 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

Note: I had a previous PR which got closed because I didn't have time to address comments: #102190

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
  • I have followed the perfect PR recommendations
  • The code has been formatted using Ruff (ruff format 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.

To help with the load of incoming pull requests:

Copy link
Member

@joostlek joostlek left a 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.

@home-assistant
Copy link

home-assistant bot commented Jul 5, 2024

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

@home-assistant home-assistant bot marked this pull request as draft July 5, 2024 09:19
@Balake
Copy link
Contributor Author

Balake commented Jul 5, 2024

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.

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.

@Balake Balake marked this pull request as ready for review July 5, 2024 19:16
@home-assistant home-assistant bot requested a review from joostlek July 5, 2024 19:16
@frenck
Copy link
Member

frenck commented Jul 6, 2024

I've marked this PR, as changes are requested that need to be processed.
Please un-draft it once it is ready for review again by clicking the "Ready for review" button.

Thanks! 👍

../Frenck

Learn more about our pull request process.

@frenck frenck marked this pull request as draft July 6, 2024 12:09
@Balake
Copy link
Contributor Author

Balake commented Jul 8, 2024

https://developers.home-assistant.io/docs/config_entries_config_flow_handler#defining-steps https://developers.home-assistant.io/docs/creating_integration_manifest#zeroconf

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!

@joostlek
Copy link
Member

joostlek commented Jul 8, 2024

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

@Balake
Copy link
Contributor Author

Balake commented Jul 8, 2024

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.

@Balake Balake marked this pull request as ready for review July 11, 2024 18:28
@Balake
Copy link
Contributor Author

Balake commented Jul 11, 2024

@joostlek I've added the config flow for this integration, adding both zeroconf discovery step and manual address input.

@home-assistant home-assistant bot marked this pull request as draft July 11, 2024 19:45
@Balake Balake marked this pull request as ready for review July 12, 2024 22:28
@home-assistant home-assistant bot requested a review from joostlek July 12, 2024 22:28
@joostlek
Copy link
Member

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.

@Balake
Copy link
Contributor Author

Balake commented Jul 22, 2024

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.

@joostlek
Copy link
Member

just not storing the IP of the bridge device as that can change with device reboot

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.

@Balake
Copy link
Contributor Author

Balake commented Jul 22, 2024

just not storing the IP of the bridge device as that can change with device reboot

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?

@Balake
Copy link
Contributor Author

Balake commented Aug 10, 2024

@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
@Balake Balake marked this pull request as ready for review August 12, 2024 21:48
@home-assistant home-assistant bot requested a review from joostlek August 12, 2024 21:48
Copy link
Member

@joostlek joostlek left a comment

Choose a reason for hiding this comment

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

Last bits

homeassistant/components/deako/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/deako/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/deako/light.py Outdated Show resolved Hide resolved
homeassistant/components/deako/light.py Outdated Show resolved Hide resolved
tests/components/deako/test_config_flow.py Show resolved Hide resolved
@home-assistant home-assistant bot marked this pull request as draft August 15, 2024 11:00
- simplify config entry type
- add test for single_instance_allowed
- remove light.py get_state(), only used once, no need to be separate function
@Balake Balake marked this pull request as ready for review August 15, 2024 18:22
@home-assistant home-assistant bot requested a review from joostlek August 15, 2024 18:22
@Balake
Copy link
Contributor Author

Balake commented Aug 15, 2024

Alright I believe I addressed your comments @joostlek, thanks for all the feedback!

@joostlek joostlek changed the title Deako integration using pydeako Add Deako integration Aug 18, 2024
Copy link
Member

@joostlek joostlek left a 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?

@home-assistant home-assistant bot marked this pull request as draft August 18, 2024 14:01
@Balake
Copy link
Contributor Author

Balake commented Aug 20, 2024

The CI is failing, can you take a look?

Sure thing!

@Balake Balake marked this pull request as ready for review August 20, 2024 16:48
@home-assistant home-assistant bot requested a review from joostlek August 20, 2024 16:48
@Balake
Copy link
Contributor Author

Balake commented Aug 23, 2024

@joostlek I fixed CI, is there anything else that I'm missing?

@joostlek
Copy link
Member

Oh, wanted to apply it but I don't have the rights

Co-authored-by: Joost Lekkerkerker <joostlek@outlook.com>
@joostlek
Copy link
Member

Let me kick off the CI.
Will probably merge tomorrow as I'm now trying to survive the wind at the Dutch grand Prix

@Balake
Copy link
Contributor Author

Balake commented Aug 24, 2024

Let me kick off the CI. Will probably merge tomorrow as I'm now trying to survive the wind at the Dutch grand Prix

Sweet, thanks! Good luck over there 🤞

@Balake
Copy link
Contributor Author

Balake commented Aug 28, 2024

@joostlek did you survive?

@joostlek
Copy link
Member

Shit. Forgot to merge and beta just has been cut. I already had the idea I forgot something.

@joostlek joostlek merged commit c049129 into home-assistant:dev Aug 28, 2024
40 checks passed
@Balake
Copy link
Contributor Author

Balake commented Aug 28, 2024

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!

@github-actions github-actions bot locked and limited conversation to collaborators Aug 29, 2024
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.

3 participants