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 eheimdigital integration #126757

Draft
wants to merge 14 commits into
base: dev
Choose a base branch
from
Draft

Conversation

autinerd
Copy link
Contributor

@autinerd autinerd commented Sep 25, 2024

Proposed change

This adds the EHEIM Digital integration. EHEIM Digital is a line of smart aquarium products, such as lighting, filters, heaters, feeders etc. https://eheim.com/en_GB/aquatics/eheim-digital/

The connection works via a central local Websocket API, where one of the devices acts as a master/hub and the devices create some kind of WiFi mesh to communicate with each other.

This PR implements the light platform of the classicLEDcontrol devices.

Link to library: https://github.com/autinerd/eheimdigital

Bronze

  • config-flow - Integration needs to be able to be set up via the UI
    • Uses data-description to give context to fields
    • Uses ConfigEntry.data and ConfigEntry.options correctly
  • test-before-configure - Test a connection in the config flow
  • unique-config-entry - Don't allow the same device or service to be able to be set up twice
  • config-flow-test-coverage - Full test coverage for the config flow
  • runtime-data - Use ConfigEntry.runtime_data to store runtime data
  • test-before-setup - Check during integration initialization if we are able to set it up correctly
  • appropriate-polling - If it's a polling integration, set an appropriate polling interval
  • entity-unique-id - Entities have a unique ID
  • has-entity-name - Entities use has_entity_name = True
  • entity-event-setup - Entities event setup
  • dependency-transparency - Dependency transparency
  • action-setup - Service actions are registered in async_setup
  • common-modules - Place common patterns in common modules
  • docs-high-level-description - The documentation includes a high-level description of the integration brand, product, or service
  • docs-installation-instructions - The documentation provides step-by-step installation instructions for the integration, including, if needed, prerequisites
  • docs-removal-instructions - The documentation provides removal instructions
  • docs-actions - The documentation describes the provided service actions that can be used
  • brands - Has branding assets available for the integration

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

To help with the load of incoming pull requests:

Copy link
Contributor

@noahhusby noahhusby left a comment

Choose a reason for hiding this comment

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

See comments.

Feel free to reach out to me on Discord if you have any questions.

tests/components/eheimdigital/__init__.py Show resolved Hide resolved
homeassistant/components/eheimdigital/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/eheimdigital/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/eheimdigital/config_flow.py Outdated Show resolved Hide resolved
"""Handle zeroconf discovery."""
self.data[CONF_HOST] = host = discovery_info.host

self._async_abort_entries_match(self.data)
Copy link
Member

Choose a reason for hiding this comment

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

I think the unique_id check is good enough

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It has the advantage of early return without needing to connect, especially in this case where only one device is advertising via zeroconf and so when it found one it only changes when the IP address changes or the user resets the devices and connect the devices to the network from scratch.

homeassistant/components/eheimdigital/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/eheimdigital/light.py Outdated Show resolved Hide resolved
homeassistant/components/eheimdigital/light.py Outdated Show resolved Hide resolved
homeassistant/components/eheimdigital/light.py Outdated Show resolved Hide resolved
homeassistant/components/eheimdigital/strings.json Outdated Show resolved Hide resolved
@home-assistant home-assistant bot marked this pull request as draft October 5, 2024 20:28
@home-assistant
Copy link

home-assistant bot commented Oct 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.

@autinerd autinerd marked this pull request as ready for review October 6, 2024 18:05
homeassistant/components/eheimdigital/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/eheimdigital/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/eheimdigital/coordinator.py Outdated Show resolved Hide resolved
homeassistant/components/eheimdigital/entity.py Outdated Show resolved Hide resolved
tests/components/eheimdigital/conftest.py Outdated Show resolved Hide resolved
tests/components/eheimdigital/test_coordinator.py Outdated Show resolved Hide resolved
tests/components/eheimdigital/test_light.py Outdated Show resolved Hide resolved
@home-assistant home-assistant bot marked this pull request as draft October 24, 2024 18:21
@autinerd autinerd marked this pull request as ready for review November 2, 2024 09:26
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.

Almost there, 2 comments and I left another comment on a question you asked

homeassistant/components/eheimdigital/coordinator.py Outdated Show resolved Hide resolved
homeassistant/components/eheimdigital/light.py Outdated Show resolved Hide resolved
@home-assistant home-assistant bot marked this pull request as draft November 10, 2024 12:58
@autinerd autinerd marked this pull request as ready for review November 20, 2024 20:06
@joostlek
Copy link
Member

Since this now should be a bronze integration, can you add the quality_scale.yaml with the bronze stuff?

@@ -0,0 +1,70 @@
rules:
Copy link
Member

Choose a reason for hiding this comment

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

We're still new to this. But I would suggest for the initial PR we should just only add the bronze rules. Even though its the same code, I notice that the way I review code is different when I am checking for a new integration or when I am checking if the scale is applied. At this moment I feel like that there is a large overhead in checking for both the integration being good and checking if it satisfies the tiers above bronze.

Maybe in the future when a lot of the checks are automated, this could be better.

Also feel free to let me know what you think about this by the way.

Can you also add the base checklist to the PR description and add the quality_scale field to the manifest.json?

@joostlek joostlek marked this pull request as draft November 21, 2024 17:06
Comment on lines 38 to 44
assert not await async_remove_config_entry_device(
hass, mock_config_entry, device_entry
)

eheimdigital_hub_mock.return_value.devices = {}

assert await async_remove_config_entry_device(hass, mock_config_entry, device_entry)
Copy link
Member

Choose a reason for hiding this comment

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

I think we should test this via the websocket like the frontend does. I think enphase_envoy has tests for this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I tried to find something and failed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants