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 HomeKit Controller Diffuser #42676

Closed
wants to merge 32 commits into from

Conversation

adrum
Copy link
Contributor

@adrum adrum commented Oct 31, 2020

Proposed change

This adds a vendor specific HomeKit device, the VOCOLinc FLOWERBUD. When you pair this with HomeKit, it is exposed as a humidifier, with a vendor specific characteristic to control the spray level. We use this instead, since this RELATIVE_HUMIDIFIER_THRESHOLD doesn't actually do anything.

This PR will be ready when there's a new release in the aiohomekit library and when this PR is merged.

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)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example entry for configuration.yaml:

# Example configuration.yaml

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
  • The code has been formatted using Black (black --fast 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.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

To help with the load of incoming pull requests:

@Jc2k
Copy link
Member

Jc2k commented Nov 5, 2020

aiohomekit 0.2.55 released with the changes you need.

@Jc2k
Copy link
Member

Jc2k commented Nov 14, 2020

Parent PR merged.

# Conflicts:
#	homeassistant/components/homekit_controller/humidifier.py
#	tests/components/homekit_controller/test_humidifier.py
@Jc2k
Copy link
Member

Jc2k commented Nov 14, 2020

I've put together a PR (#43242) for the idea i mentioned on your PR - passing the aiohomekit models around instead of the raw JSON data. I think it makes the async_add_service functions quite a bit nicer. I think this PR would have something like this if the PR is merged:

    @callback
    def async_add_service(service):
        if service.short_type != ServicesTypes.HUMIDIFIER_DEHUMIDIFIER:
            return False

        info = {"aid": service.accessory.aid, "iid": service.iid}
        entities = []

        if service.has(CharacteristicsTypes.Vendor.VOCOLINC_HUMIDIFIER_SPRAY_LEVEL):
            entities.append(VocolincFlowerbud(conn, info))
        else:
            if service.has(CharacteristicsTypes.RELATIVE_HUMIDITY_HUMIDIFIER_THRESHOLD):
                entities.append(HomeKitHumidifier(conn, info))
            if service.has(
                CharacteristicsTypes.RELATIVE_HUMIDITY_DEHUMIDIFIER_THRESHOLD
            ):
                entities.append(HomeKitDehumidifier(conn, info))

        async_add_entities([entities], True)
        return True

What do you think? Unfortunately black still makes it look ugly because of the long characteristic name.

@adrum
Copy link
Contributor Author

adrum commented Nov 15, 2020

@Jc2k This looks like a huge improvement to me, and seem much more natural. Nice work! I'm fine waiting until #43242 is merged and then implementing it in this one.

@SvetZitrka
Copy link

When will the Diffuser be added to the production version of Home Assistant?

@Jc2k
Copy link
Member

Jc2k commented Nov 19, 2020

The answer to that question is always "when it's ready". We can't put a date on it.

It's nearly ready though. My latest PR we were discussing above has landed so we are very very close.

@adrum adrum requested a review from Jc2k November 22, 2020 23:28
Copy link
Member

@Jc2k Jc2k left a comment

Choose a reason for hiding this comment

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

Brilliant! Looks great.

There are a couple of things I hope we can tweak in the future but they rely on more work being done elsewhere so a really not for this PR:

  • Given there are only 5 "target humidities" - it would be nice if we could set the "step" size somewhere that the HA UI (and HomeKit) could use to tweak their UI accordingly.
  • I really need to get round to switching the tests to have direct access to the underlying aiohomekit model.

Before I approve this though I do have on further thought about whether this is the right way to model your device. It seems that it doesn't have a target humidity, right? So even if they model it as a HomeKit Humidifier the question is, is it actually a Home Assistant humidifier? In the most general sense the higher your target humidity the wetter things will get, and thats certainly true of the spray level. But it's not a target humidity. It won't turn off when it reaches the target.

Do you think https://github.com/home-assistant/core/pull/42735/files would actually be a better fit when its merged (looks like its close)?

await self.async_put_characteristics(
{
CharacteristicsTypes.Vendor.VOCOLINC_HUMIDIFIER_SPRAY_LEVEL: humidity
/ 20
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 aiohomekit clamps this to the minStep automatically so this never sends floats right?

"""Define the homekit characteristics the entity cares about."""
return [
CharacteristicsTypes.ACTIVE,
CharacteristicsTypes.RELATIVE_HUMIDITY_CURRENT,
Copy link
Member

Choose a reason for hiding this comment

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

It doesn't look like these are all used, can we trim them out?

@shawnli87
Copy link

@adrum @Jc2k The VOCOlinc FlowerBud’s RELATIVE_HUMIDIFIER_THRESHOLD does actually function as it would for a normal humidifier.

@Jc2k
Copy link
Member

Jc2k commented Nov 23, 2020

Interesting! If that's the case (and unless there is not a good reason not to) i'd prefer to use the standard humidifier code for this device then, and instead look at adding a number integration to control the spray level when that PR is merged. This spray level control would be in addition to the default humidifier controls.

I'll need to do some cleanup to unlock this in homekit_controller as well (currently can't easily have 2 entities of different types for the same homekit service - some of the cleanup for this has already started to land, i'll try and get the rest done asap).

@github-actions
Copy link

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

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.

6 participants