core icon indicating copy to clipboard operation
core copied to clipboard

Add Dreo integration

Open w-xtao opened this issue 11 months ago • 5 comments

Breaking change

Proposed change

Add dreo integration with fan platform.

Type of change

  • [ ] Dependency upgrade
  • [ ] Bugfix (non-breaking change which fixes an issue)
  • [x] 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

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • Link to documentation pull request: https://github.com/home-assistant/home-assistant.io/pull/36851

Checklist

  • [x] The code change is tested and works locally.
  • [x] Local tests pass. Your PR cannot be merged unless tests pass
  • [x] There is no commented out code in this PR.
  • [x] I have followed the development checklist
  • [x] I have followed the perfect PR recommendations
  • [x] The code has been formatted using Ruff (ruff format homeassistant tests)
  • [x] 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:

  • [x] The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • [x] 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:

w-xtao avatar Jan 08 '25 15:01 w-xtao

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

Learn more about our pull request process.

home-assistant[bot] avatar Jan 08 '25 15:01 home-assistant[bot]

@abmantis Thank you for the review. Here's the complete validation report from my latest checks:

All Checks Passed:

✓ ruff (code analysis)  
✓ ruff-format (formatting)  
✓ codespell (typos)  
✓ mypy (type checking)  
✓ pylint (static analysis)  
✓ gen_requirements_all (dependencies)  
✓ hassfest (integration validation)

Environment Specs:

  • Tools: ruff 0.8.3, mypy 1.15.0, pylint 3.3.2
  • Validation Method:
     pre-commit run --all-files
    

Here is Checks image

4deac77375440ce45dffe0c4994b6f7

w-xtao avatar Mar 29 '25 16:03 w-xtao

By the way, the tests are failing.

Also check:

  • https://github.com/home-assistant/core/pull/135107#discussion_r2040250979
  • https://github.com/home-assistant/core/pull/135107#discussion_r2040251686

abmantis avatar Apr 11 '25 20:04 abmantis

By the way, there are more Dreo devices other than Fans right? I assume you will want to support those in the future and some devices will have multiple entities.

In that case, I suggest creating a Coordinator class, so we can fetch the data once per device, instead of having multiple entities fetching the data for the same device.

See https://developers.home-assistant.io/docs/integration_fetching_data#coordinated-single-api-poll-for-data-for-all-entities

abmantis avatar Apr 15 '25 09:04 abmantis

By the way, there are more Dreo devices other than Fans right? I assume you will want to support those in the future and some devices will have multiple entities.

In that case, I suggest creating a Coordinator class, so we can fetch the data once per device, instead of having multiple entities fetching the data for the same device.

See https://developers.home-assistant.io/docs/integration_fetching_data#coordinated-single-api-poll-for-data-for-all-entities

@abmantis

Our original plan was to only integrate a single device in the first version, and then introduce the coordinator in subsequent versions. However, since you mentioned the coordinator, we've decided to add it in this version as well.

I've already implemented the DataUpdateCoordinator pattern as suggested. The integration now uses a dedicated DreoDataUpdateCoordinator class that manages data fetching for each device, allowing multiple entities to share the same data source without duplicate API calls.

The current implementation:

  • Creates one coordinator instance per physical device
  • Uses TypedDict classes to define structured device data (DreoGenericDeviceData, DreoFanDeviceData)
  • Implements a factory pattern through DeviceDataFactory to process different device types
  • Includes commented placeholder code for future device types (like heaters)

To support additional Dreo device types and multiple entities per device. When we add support for other device types in the future, we'll simply:

  • Uncomment and implement the specific device data classes (like DreoHeaterDeviceData)
  • Add corresponding processing strategies
  • Create the appropriate entity classes

Thank you for the suggestion - it's already been incorporated into the design.

w-xtao avatar Apr 20 '25 09:04 w-xtao

Hi @abmantis Thank you for the detailed review! I have addressed all the issues you raised:

All Issues Fixed

Code Structure & Logic:

  • Removed unused SYNC_INTERVAL - No longer present in the codebase
  • Fixed device_id handling - Changed from str(device.get("deviceSn", "")) to device.get("deviceSn") in fan.py
  • Implemented data processor pattern - Moved device type logic to coordinator __init__ as suggested
  • Proper exception handling - Using UpdateFailed exceptions instead of returning None
  • Consistent parameter naming - Using error_translation_key throughout
  • Removed unnecessary initialization - No longer calling _update_attributes() in fan __init__
  • Proper availability handling - Let CoordinatorEntity base class handle availability automatically

Testing:

  • Removed test_entity.py - As suggested, testing entity functionality through platform tests
  • Simplified test_coordinator.py - Now only contains core algorithm tests (speed conversion, data type conversion, edge cases) that cannot be tested through platforms

Architecture Changes Explanation

I'd also like to explain three significant architectural improvements made during this integration:

  1. Device Command Management Redesign Previously, device commands were hardcoded as constants in the hscloud package. This approach had limitations:

    • Required publishing new versions of the underlying package for device command changes
    • Difficult to manage when new models of the same device type were introduced
    • Tight coupling between device specifications and the client library New approach: The device list API now directly returns deviceType and config information, making the integration more flexible and maintainable.
  2. Package Migration (hscloud → pydreo-cloud) Due to company organizational restructuring, the underlying package has been migrated from hscloud to pydreo-cloud. This change:

    • Reflects the new company structure
    • Provides better package naming alignment with the Dreo brand
    • Maintains the same functionality with improved API design
  3. Improved Maintainability The new architecture eliminates the need to:

    • Publish underlying package updates for device command changes
    • Maintain device-specific constants in the client library Please let me know if you have any further questions or concerns! Thanks

Atum-zhulong avatar Jun 24 '25 09:06 Atum-zhulong

Something went wrong when updating the branch, the PR now includes unrelated commits. @w-xtao Please mark the PR "ready for review" when you've fixed the branch.

emontnemery avatar Jun 30 '25 08:06 emontnemery

@w-xtao Please don't mark this as ready until the unrelated component files, as pointed by @emontnemery , are cleared from this PR. Also, there is no reason to merge from dev so frequently. It is just creating unnecessary commits and noise on the PR. It is ok to do it from time to time, but no need for doing it daily.

abmantis avatar Jun 30 '25 15:06 abmantis

@Atum-zhulong, @w-xtao this PR will not be reviewed until you've fixed the merge errors in your branch.

emontnemery avatar Sep 16 '25 06:09 emontnemery

hey @w-xtao ! Need any help to move this forward?

abmantis avatar Oct 28 '25 12:10 abmantis