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

Fix HAA homekit integration #84519

Closed
wants to merge 13 commits into from

Conversation

jaredhobbs
Copy link
Contributor

@jaredhobbs jaredhobbs commented Dec 24, 2022

Proposed change

HAA changed the way they implemented custom characteristics. This commit fixes the Update and Setup buttons and adds Reboot and Reconnect WiFi buttons. @freedreamer82 created a first pass (#84502) and this PR fills in the missing pieces.

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

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • Link to documentation pull request:

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

HAA changed the way they implemented custom characteristics.
This commit fixes the Update and Setup buttons and adds
Reboot and Reconnect WiFi buttons.
@home-assistant
Copy link

Hey there @Jc2k, @bdraco, mind taking a look at this pull request as it has been labeled with an integration (homekit_controller) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of homekit_controller can trigger bot actions by commenting:

  • @home-assistant close Closes the issue.
  • @home-assistant rename Awesome new title Change the title of the issue.
  • @home-assistant reopen Reopen the issue.
  • @home-assistant unassign homekit_controller Removes the current integration label and assignees on the issue, add the integration domain after the command.

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.

I think this will create 4 buttons with the same unique_id? Or did i miss something happening in the base classes? (Sorry, reviewing on phone so hard to dig in).

@Jc2k
Copy link
Member

Jc2k commented Dec 24, 2022

BTW, if we do this change and there are people on the old version of HAA, will they be able to update or will they get stuck?

@jaredhobbs
Copy link
Contributor Author

I think this will create 4 buttons with the same unique_id? Or did i miss something happening in the base classes? (Sorry, reviewing on phone so hard to dig in).

Correct, but each button will send a different command.

@Jc2k
Copy link
Member

Jc2k commented Dec 24, 2022

Each entity in HA has to have a different unique_id. It'd be like having 4 entities with the same entity id. In fact, the unique id is how HA maps the entity id to the right button.

@jaredhobbs
Copy link
Contributor Author

BTW, if we do this change and there are people on the old version of HAA, will they be able to update or will they get stuck?

The update functionality has been broken for a while now so it's probably more likely that this would fix it for them. But yeah, it's possible that there may be devices running older firmware that wouldn't be able to be updated after this change. (Although they could still update by power cycling twice within 2 seconds or whatever it is)

@Jc2k
Copy link
Member

Jc2k commented Dec 24, 2022

So we could keep the old char, and if it's present add the 2 old buttons. If it's not, add the new ones. If you want to go down that route I can help with the logic to suppress the new buttons on old firmware, though it's the middle of the night here so won't be right away.

Otherwise technically we will probably need to make this a breaking change and write something for the release notes. Ideally noting which versions are no long supported and how they can enter update mode.

@MartinHjelmare MartinHjelmare changed the title feat: fix HAA homekit integration Fix HAA homekit integration Dec 24, 2022
@jaredhobbs
Copy link
Contributor Author

So we could keep the old char, and if it's present add the 2 old buttons. If it's not, add the new ones. If you want to go down that route I can help with the logic to suppress the new buttons on old firmware, though it's the middle of the night here so won't be right away.

Otherwise technically we will probably need to make this a breaking change and write something for the release notes. Ideally noting which versions are no long supported and how they can enter update mode.

I was looking into implementing this logic and got stuck since the new firmware uses the same char as the old firmware's Update command (but with a different write string). If you have an idea on how to support both the old and new firmware versions, great! If not, we may just need to mark this as breaking change I guess.

@Jc2k
Copy link
Member

Jc2k commented Dec 27, 2022

I was wondering if we could use the presence of the removed char to tell which interface to use.

I think sensor.py has some logic for the temperature sensors that we could borrow here - iirc it has a callback that can be used to run checks before creating sensors. The temperature callback I think inspects the service of the char, but we could probably then check if the old char is in the service.

@freedreamer82
Copy link
Contributor

the simplest procedure to me is to check version of HAA : based on that you change the logic. Unfourtunately haa kept the same char when they changed the application process...

@Jc2k
Copy link
Member

Jc2k commented Dec 27, 2022

Oh, so the kept both of them? I thought they removed one?

@jaredhobbs
Copy link
Contributor Author

the simplest procedure to me is to check version of HAA : based on that you change the logic. Unfourtunately haa kept the same char when they changed the application process...

Do we know the version of HAA when everything changed?

@freedreamer82
Copy link
Contributor

Them removed one and moved all the logic in one char( it makes more sense to me btw).
I don’t know the version but I Can See in the commits to find out …

@jaredhobbs
Copy link
Contributor Author

jaredhobbs commented Dec 27, 2022

I think the new functionality was introduced in v10.0.0 so the last version that would have worked the old way is v9.5.1b

Edit: Confirmed. Here's the commit that adds the new functionality: RavenSystem/esp-homekit-devices@ceaf8cd#diff-be4f4d281b129a08117426ff051af223f7d68e0192367973c464821d7decc5d6R952 (this is from the v10.0.0 release). The change is in devices/HAA/main.c lines 945-972.

@jaredhobbs
Copy link
Contributor Author

Yeah... maybe we need to go back to the checking firmware version approach?

@Jc2k
Copy link
Member

Jc2k commented Jan 8, 2023

With maintainability hat on, I'm unhappy that this is proving to be such a moving target. It's not a complicated function, so why does it keep changing 😅 If it is going to keep changing on a whim, we are going to have more compat code than integration code.

Especially given the problems we had with the previous iteration of version checking, maybe we do go back to not maintaining backward compatibility after all, and signpost users to HAA with their grievances. It's probably a small and advanced enough subgroup to get away with that.

@freedreamer82
Copy link
Contributor

I do not understand as well why the maintainer continues to change things...I guess is not on him to keep compatibility with external tools.
BTW I would try to start a compatibility to last version and maybe write to HAA projects to do not change at least that stuff (whenever is possible....).

@freedreamer82
Copy link
Contributor

@Jc2k i was thinking… another way to proceed is to expose through The homekit bridge The custom services… doing this we could use directly haa app and if they change something is up to them.
Could be a way to go?
RavenSystem/esp-homekit-devices#1931 (comment)

Btw also The maintainer suggests to use The version as a trigger

@Jc2k
Copy link
Member

Jc2k commented Jan 9, 2023

Thats just not how this works unfortunately. HomeKit Controller and HomeKit Bridge are 2 distinct integrations that adapt the Home Assistant entity abstractions. There is no direct path between them, everything goes via Home Assistant. Everything HomeKit Controller presents to Home Assistant has to be something that can be represented by Home Assistant (like a climate entity or a button entity), and Home Assistant itself has no knowledge of Characteristics and Services. So while that stops the HAA author having to worry about version compatibility and breaking things, it's fundamentally incompatible with the Home Assistant architecture. So not a choice for us.

@Jc2k
Copy link
Member

Jc2k commented Jan 9, 2023

I don't want the extra code for this device to end up becoming the most complicated thing in homekit_controller.

If this product doesn't provide guarantees for stable services and characteristics (like the HAP specification provides for the main entities) then using the version number is just "lead bad" and not "good" (theres no guarantees! it could change too!).

At the moment I'm still leaning toward supporting the latest interface only.

@freedreamer82
Copy link
Contributor

I see your point... to me is ok to support only the latest interface.
BTW In haa discussion I told to maintainer to be careful in future changes.... (if possibile)

M.

@freedreamer82
Copy link
Contributor

Any news @jaredhobbs @Jc2k ?
if u agreed we can support only from last version 11.8.0, We must keep this PR changing from #HAA@trcmd" to "cmy".
Thanks

@freedreamer82
Copy link
Contributor

RavenSystem/esp-homekit-devices#1931 (reply in thread)

a reply from the maintainer of HAA of my request to do not change the setup word...

@Jc2k
Copy link
Member

Jc2k commented Jan 20, 2023

Downside of software that doesn't respect users I guess. I'm reluctant to spend any time on this system, if he's going to randomly change the strings.

@freedreamer82
Copy link
Contributor

Downside of software that doesn't respect users I guess. I'm reluctant to spend any time on this system, if he's going to randomly change the strings.

@Jc2k I agree with u and I wrote him as well that's not the way to go , expecially if he release the project open source. BTW the drawback now is for the users that cannot integrate these kind of devices in HA.(or better they can but is not possible to
to do config stuff). Since the maintainer doesn't guarantee a stable way to enter in setup mode is possible to develop here a way , maybe inside the cli , to enter manually in setup mode writing the command word manually ? in this way we could guarantee from HA a way to bypass the restriction...

I was think something like ..
hass --homekit --button -- char XXXXXXXX --value "string to enter setup mode"

basically the idea is to export the button setup but without a default value ( we could think to bring same thing in UI with a value writable from there)

what do u think?

M.

@Jc2k
Copy link
Member

Jc2k commented Jan 21, 2023

Integrations can't integrate directly into the CLI AFAIK. It's probably worth being clear here that I'm effectively a plug-in developer, I have very little say over what other plug-ins do or what the core "engine" of HA lets plug-ins do. It's why no matter how much of a great idea it is I can't proxy his service. I can't just add a homekit CLI, I'd have to design an API and then convince a bunch of people it was worth adding and more integrations than me want to add CLI options. It's a heck of an undertaking just for one small subset of HomeKits supported devices, with no guarantee of success.

Likewise, there isn't an existing way to build the UI you are thinking of so that would require similar work extending the "core", with no guarantees the work would be accepted.

I don't have time to do either of these, unfortunately, and approval can take a long time. And I'm not sure I'd be able to get them approved.

So one option is for you to pair with iOS, dump the encryption keys from the iOS keychain and import them into HA. I've done this with several devices where I've been still building the HA support or the manufacturer failed to give me a setup code. This works well - you can still use it natively with his app, but you can also use it in HA. But this is mega hard - I had an old version of macOS and Xcode running in a vm and I've had to disable a bunch of bios settings to remove security features. If you want to explore this route I can find you a link to a blog post later.

The other option is to to use aiohomekit directly. It already has a CLI. This is a bit lower level than you were imagining but would let you write directly to any characteristic of any paired device. It is already installed in your Ha container, so I just need to write up how to use it if that's acceptable for yourself.

@freedreamer82
Copy link
Contributor

Integrations can't integrate directly into the CLI AFAIK. It's probably worth being clear here that I'm effectively a plug-in developer, I have very little say over what other plug-ins do or what the core "engine" of HA lets plug-ins do. It's why no matter how much of a great idea it is I can't proxy his service. I can't just add a homekit CLI, I'd have to design an API and then convince a bunch of people it was worth adding and more integrations than me want to add CLI options. It's a heck of an undertaking just for one small subset of HomeKits supported devices, with no guarantee of success.

Likewise, there isn't an existing way to build the UI you are thinking of so that would require similar work extending the "core", with no guarantees the work would be accepted.

I don't have time to do either of these, unfortunately, and approval can take a long time. And I'm not sure I'd be able to get them approved.

So one option is for you to pair with iOS, dump the encryption keys from the iOS keychain and import them into HA. I've done this with several devices where I've been still building the HA support or the manufacturer failed to give me a setup code. This works well - you can still use it natively with his app, but you can also use it in HA. But this is mega hard - I had an old version of macOS and Xcode running in a vm and I've had to disable a bunch of bios settings to remove security features. If you want to explore this route I can find you a link to a blog post later.

The other option is to to use aiohomekit directly. It already has a CLI. This is a bit lower level than you were imagining but would let you write directly to any characteristic of any paired device. It is already installed in your Ha container, so I just need to write up how to use it if that's acceptable for yourself.

I understand... the first option i guess is not the way but using directly aiohomekit , if it works , could be a good compromise.
I can develop a simple cli in python by using directly aiohomekit coverings my needs .
Please if u can describe me further I will appreciate.

Marco

@freedreamer82
Copy link
Contributor

aiohomekitctl --adapter eth0 discover

from this command I can see my devices... but i guess I need a file where are stored the pairing data.. where does HA store it?

@Jc2k
Copy link
Member

Jc2k commented Jan 21, 2023

You don't need adapter. That's a Bluetooth setting and it's not used any more.

The pairing file doesn't exist in a format aiohomekitctl understands. It's easy to convert but I need to be at a computer to make the examples.

@freedreamer82
Copy link
Contributor

You don't need adapter. That's a Bluetooth setting and it's not used any more.

The pairing file doesn't exist in a format aiohomekitctl understands. It's easy to convert but I need to be at a computer to make the examples.

ok , write me a little guide when you can.Thanks

@freedreamer82
Copy link
Contributor

freedreamer82 commented Jan 22, 2023

@Jc2k I got info from .storage/core.config_entries and I created a json conf in the form of:
{
"test": {
"Connection": "IP",
"AccessoryPairingID": "XX:XX:XX:XX:XX:XX",
"AccessoryLTPK": "XXXXXX",
"iOSPairingId": "XXXX",
"iOSDeviceLTSK": "XXXX",
"iOSDeviceLTPK": "XXXX",
"AccessoryIP": "192.168.1.91",
"AccessoryPort": 5556,
}
}

aiohomekitctl -f myfile.json accessories -a test

but I got
2023-01-22 14:10:17,668 controller.py:0180 ERROR Skipped pairing: Transport IP not supported. See setup.py for required dependencies.
"test" is no known alias

any ideas?

M.

@Jc2k
Copy link
Member

Jc2k commented Jan 22, 2023

How did you install it?

@freedreamer82
Copy link
Contributor

How did you install it?

actually i found in the path. I have an installation of HA in python env and in that env is installed aiohomectl...

@Jc2k
Copy link
Member

Jc2k commented Jan 22, 2023

See if it was installed by HA then all the dependencies should have been pulled in, otherwise your HA wouldn't work. Fraid I really will need to be at a computer to help you instead of guessing on my phone.

@Jc2k
Copy link
Member

Jc2k commented Jan 22, 2023

You could just try pip install aiohomekit in a fresh venv

@freedreamer82
Copy link
Contributor

@Jc2k
Copy link
Member

Jc2k commented Jan 22, 2023

Entirely possible (and looking likely) aiohomekitctl is broken since thread and ble were added.

(There's a lot of refactoring in the air, and keeping the CLI working was less important than HA).

In the meantime, you could look at homekit_python if you are eager. It predates aiohomekit and uses blocking code so wasn't really suitable for HA. It's pairing file format is the same though, as we forked my asyncio PR to start aiohomekit.

@freedreamer82
Copy link
Contributor

ok it works properly with the other lib.
BTW I talked with HAA maintainer.. I found out HAA devices have an option to get an extra pairing..I tried and it works.
In this way you can use two controllers ...one is HA and the other is official IOS... doing this you can use the HAA APP manager to handle devices. (setup config and whatever).
I will write and publish a script to do it with homekit_python but basically with the above trick is not necessary. My only concern is the traffic on the network.

thanks for you support... with these assumptions let's re-think about the HAA integration in HA.

M.

@freedreamer82
Copy link
Contributor

for those who are interested I created a repo :

@freedreamer82
Copy link
Contributor

for those interested I created I project to handle HAA devices.
https://github.com/freedreamer82/haa_manager_cli

if anyone is interested can PR or contribute in any way

@github-actions
Copy link

github-actions bot commented May 3, 2023

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.

@github-actions github-actions bot added the stale label May 3, 2023
@github-actions github-actions bot closed this May 13, 2023
@github-actions github-actions bot locked and limited conversation to collaborators May 14, 2023
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.

None yet

4 participants