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

feat: add vendor characteristics for Home Accessory Architect #48

Merged
merged 1 commit into from
Nov 11, 2021

Conversation

jaredhobbs
Copy link
Contributor

I have changes to homekit_controller too but it requires these changes... Should I wait until these changes are released on Pypi so I can update the required version in homekit_controller before opening a PR over there?

This change exposes the Update and Setup custom characteristics from Home Accessory Architect.

Addresses RavenSystem/esp-homekit-devices#1441

@Jc2k
Copy link
Owner

Jc2k commented Nov 10, 2021

I'd like to see the homekit_controller changes too please. A link to a branch is fine.

@jaredhobbs
Copy link
Contributor Author

@Jc2k
Copy link
Owner

Jc2k commented Nov 10, 2021

Cheers. I'm new to HAA so could you explain how this works? Is there a description of how the HAA app works? How does this get exposed by the homekit bridge?

Also any tips on navigating the code? I can see HOMEKIT_CHARACTERISTIC_CUSTOM_OTA_UPDATE is F0000101, but I can't see where it is used. And I can see its a string not a bool but not where its value is handled.

@Jc2k
Copy link
Owner

Jc2k commented Nov 10, 2021

Also #HAA@trcmd is apparently a custom trigger. What is that, and how does it related to setup mode.

@Jc2k
Copy link
Owner

Jc2k commented Nov 10, 2021

And if ultimately the plan is to expose this from iOS, can we do this without creating entities in HASS? Or is there value in them existing as entities in HASS?

@jaredhobbs
Copy link
Contributor Author

Here's where the custom trigger is defined: https://github.com/RavenSystem/esp-homekit-devices/blob/44da5bd3e3aa30b369a03add6822e5c602c46cbf/devices/HAA/header.h#L590

When the "Setup" or "Update" characteristic value gets set to that trigger, the device will go into setup mode (accessible at the device IP address on port 4567) or initiate an over the air update.

This is the code that enabled the "Setup" and "Update" switches to work in the HAA app: RavenSystem/esp-homekit-devices@HAA_5.0.10...HAA_5.0.11

The HAA app exposes the "Setup" and "Update" switches under a "HAA Setup" section.

For me, having the "Setup" and "Update" switches exposed in HASS is more useful than having it exposed in iOS. I'm planning on creating an automation to bulk update all my HAA devices within HASS. I'll probably exclude them from being imported into iOS.

@Jc2k
Copy link
Owner

Jc2k commented Nov 10, 2021

Lovely - the compare diff is super useful. For some reason code search isn't finding everything.

Probably want to implement this as a button rather than a switch. And probably want to add support for the new conf/diag types. But we can worry about that for the HA review.

I am AFK but will merge when I'm back at desk. Might be tomorrow now.

@jaredhobbs
Copy link
Contributor Author

awesome, sounds good!

@codecov
Copy link

codecov bot commented Nov 11, 2021

Codecov Report

Merging #48 (53a408e) into main (de4f708) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main      #48   +/-   ##
=======================================
  Coverage   85.52%   85.53%           
=======================================
  Files          47       47           
  Lines        3371     3373    +2     
=======================================
+ Hits         2883     2885    +2     
  Misses        488      488           
Flag Coverage Δ
unittests 85.53% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ekit/model/characteristics/characteristic_types.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update de4f708...53a408e. Read the comment docs.

@Jc2k Jc2k merged commit 90bc377 into Jc2k:main Nov 11, 2021
@Jc2k
Copy link
Owner

Jc2k commented Nov 11, 2021

Release with this PR pushed to pypi.

@jaredhobbs jaredhobbs deleted the add-haa-vendor-extensions branch November 11, 2021 17:02
@jaredhobbs
Copy link
Contributor Author

Probably want to implement this as a button rather than a switch. And probably want to add support for the new conf/diag types. But we can worry about that for the HA review.

Can you point me to some example code to implement this as a button instead of a switch? Also, not sure how to do the new conf/diag types; could you also point me to some examples there as well?

@Jc2k
Copy link
Owner

Jc2k commented Nov 11, 2021

Sure thing!

So first of all, both of these things are new to homekit_controller so I can't point you to examples there, but looking at the work you did on the figuring out how i'd have added SimpleSwitch I think you'll have no problems at all adding them.

The conf/diag types are "entity categories". The docs say:

Classification of a non-primary entity. Set to config for an entity which allows changing the configuration of a device, for example a switch entity making it possible to turn the background illumination of a switch on and off. Set to diagnostic for an entity exposing some configuration parameter or diagnostics of a device but does not allow changing it, for example a sensor showing RSSI or MAC-address. Set to system for an entity which is not useful for the user to interact with. As an example the auto generated energy cost sensors are not useful on their own because they reset from 0 every time home assistant is restarted or the energy settings are changed and thus have their entity category set to system.

Crucially they won't show up on auto generated dashboards, but are visible on the device page in settings, and you can still use them in automations.

@Jc2k
Copy link
Owner

Jc2k commented Nov 11, 2021

As for button:

It's not implemented in homekit_controller either so you'll need to add a new platform. Thats honestly not much more work than what you already did for simpleswitch. The newest platform i added to homekit_controller was number. So this is probably a good PR to look at:

@Jc2k
Copy link
Owner

Jc2k commented Nov 11, 2021

I also wanted to call out how this probe callback is handled in sensor.py:

I think we could do something similar to avoid the special casing your current SimpleSwitch has. E.g.

@dataclass
class HomeKitButtonEntityDescription(ButtonEntityDescription):
    """Describes Homekit button."""

    write_value: int | str | None = None

SIMPLE_BUTTON: dict[str, HomeKitButtonEntityDescription] = {
    CharacteristicsTypes.Vendor.HAA_SETUP: HomeKitButtonEntityDescription(
        key=CharacteristicsTypes.Vendor.HAA_SETUP,
        name="Setup",
       # ... snip
        write_value="#HAA@trcmd",
    ),
...
}

And the default write value would probably be the number 1 (not the str "1").

@jaredhobbs
Copy link
Contributor Author

Thanks for all the comments. I'll give it a shot and let you know when I have something for you to look at!

@jaredhobbs
Copy link
Contributor Author

@Jc2k everything seems to be working now. I implemented the HAA Update and Setup characteristics as config category buttons instead of switches. I'm not sure how to write the button test though... could you take a look here? https://github.com/home-assistant/core/compare/dev...jaredhobbs:add-haa-vendor-extensions?expand=1#diff-891b30bebf509570e7a8cf0d874113a98c2fface798ca0e7bc2721481ab62828R45

@Jc2k
Copy link
Owner

Jc2k commented Nov 12, 2021

The test looks pretty darn close to me, nothing screams out as a obvious cause for the failure. I'll have a proper look when I'm at my desk tomorrow!

@jaredhobbs
Copy link
Contributor Author

fixed the test. I had the wrong entity. anyway, here's the pr for the HASS changes: home-assistant/core#59750

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

Successfully merging this pull request may close these issues.

None yet

2 participants