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

Asyncio client for HTTP #154

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Asyncio client for HTTP #154

wants to merge 8 commits into from

Conversation

Jc2k
Copy link
Collaborator

@Jc2k Jc2k commented Jul 29, 2019

This is a draft so you can see where I am headed, so far i've got pair-verify and get_characteristics working against an accessoryserver test case.

As discussed in #151/#152/#153: It's going to be really hard to have the synchronous send/receive semantics and spontaneous event semantics mixing with a blocking API without decending into threading hell. Therefore I have been looking into an asyncio which will let us handle that case better.

This will be in addition to the existing API, and we won't break that API when adding asyncio support. The CLI tools will continue to use the synchronous API.

I've used core homekit_python as much as possible. However some of the pieces i wanted to use require adapting to work with asyncio. For now I have copied these into the aio folder, and i'll de-duplicate as seperate small PR's to make it easier to review.

I'm trying to keep the user facing API as similar as possible, only making operations that do network IO awaitable.

Under the hood things are a little bit different but should still be familiar apart from sprinkling await and async keywords everywhere. The most notable differences are:

  • An asyncio Protocol calls you when it has received some data. So we don't have to mess around with select and network i/o timeouts. But this means we have push rather than pull based logic for the HTTP parsing.
  • Because we are using an asyncio.Protocol with a data_recived we can't directly have a request() method that blocks/awaits on a reply. To make it so we can i create a stack of Future objects. I add one to a stack and return it every time we make a request. These futures are then fired as we parse results in data_received in the order they were added (FIFO, i guess).
  • Because the aio API is meant for long term sessions i've tried to work in support for automatic reconnections. This will ultimately restore any subscriptions you hadd, so that get_events will transparently keep working.

I think it's going to be harder to avoid code duplication in the BLE backend, but hopefully will be able to do /something/...

@Jc2k
Copy link
Collaborator Author

Jc2k commented Aug 17, 2019

@jlusiardi do you have any thoughts about where i'm going with this?

I think there are 2 directions i'd like to go in:

  • I'm more than happy to put the asyncio stuff in a seperate library and then try to collaborate on the "model" and serialize/deserialize bits. The AIO bit would be aiohomekit. I'd depend on homekit for the models and serialization stuff. Bits that were common between the two versions i'd try to push upstream, but might have a local copy while i was stabilising things.
  • Or we merge this and then iterate on reducing the bits that are basically duplicated (the HTTP response parsing is an example)

Option 1 might be better in the short term so I can iterate quickly on the Home Assistant. But then there is an option 3 where we start with option 1 and then merge back together when things are more battle tested.

@jlusiardi
Copy link
Owner

so why not option 2? After the merge all the tools / the api should work like before and we have one code base to work on?

@Jc2k
Copy link
Collaborator Author

Jc2k commented Aug 21, 2019

That's indeed why i prefer option 2 - i just don't want to put pressure on you to review a bunch of code for a framework you aren't familiar with AND a flurry of pull requests with me nagging, and then a bit after that nagging for releases :-) If you are up for that lets crack on with option 2!

@Jc2k Jc2k marked this pull request as ready for review August 21, 2019 05:42
@jlusiardi
Copy link
Owner

ok, so i will try to have a look at the code as fast as possible ;)

@Jc2k
Copy link
Collaborator Author

Jc2k commented Aug 21, 2019

/me hops up and down excitedly ;)

@jlusiardi
Copy link
Owner

could we integrate the tests into coverage3 run --branch -m unittest -v somehow?

@Jc2k
Copy link
Collaborator Author

Jc2k commented Aug 21, 2019

Probably, I used pytest for these because I’m more used to it’s output when something fails and have no idea how to write asyncio tests without it. What are the chances I can persuade you to switch to pytest :) (it can run all the existing tests as they are)

@jlusiardi
Copy link
Owner

Ok, pytest runs the tests but fails at using cryptography.hazmat.primitives.serialization.Encoding.Raw? So I guess I will have to do some work there ;)

@Jc2k
Copy link
Collaborator Author

Jc2k commented Aug 22, 2019

That's weird - they pass here on 2 different macs (though skipped the BLE ones). Python 3.7.3. Which use of RAW is failing for you? The new (temporary) one in homekit.aio? Do you have pytest and the pytest-asyncio helper installed?

@Jc2k
Copy link
Collaborator Author

Jc2k commented Aug 22, 2019

(Have updated CI on this branch to use pytest and its running now)

@Jc2k
Copy link
Collaborator Author

Jc2k commented Aug 24, 2019

I rebased this on master so the review diff no long contains the accessoryserver.py changes. I'll be able to do the same again when we have merged the other 2 PRs.

@Jc2k
Copy link
Collaborator Author

Jc2k commented Aug 26, 2019

Thanks for all the merges so far - have rebased this based on the latest round and got rid of the code that i'm now able to share with the synchronous client \o/

@Jc2k
Copy link
Collaborator Author

Jc2k commented Sep 6, 2019

Have rebased and it now reuses the same pair setup state machines as the sync clients.

@Jc2k
Copy link
Collaborator Author

Jc2k commented Sep 14, 2019

What can we do to make progress here?

  • When you are happy with the basic structure of the new code I want to run this with my home assistant environment for a bit and see how stable it is over 24 hours. I don't want to do this QA if there are large structural changes desired though.
  • Again, if there are no big changes to do i'd probably try and at some tests for failure cases etc.
  • I have thought about adding asyncio versions of the CLI tools, mostly for development and debugging purposes. I didn't want to clutter this PR further, though.

The current status is:

  • No BLE support yet - out of scope for this PR I think.
  • I haven't tried to use aiozeroconf yet - there are no issues with Windows and asyncio and UDP that means that home assistant uses the synchronous zeroconf we use. This will be fixed in python 3.8, apparently, so i don't plan to use aiozeroconf any time soon.
  • The API is largely similar to the synchronous one, especially for pairings, but using awaitable async defs as needed. The big exception is that discovery returns a "Discovery" object rather than a dict - and this has methods for initiating pairing, doing an identiy, etc.

There are still some bits of logic that could be shared between the sync and sync client - around parsing and serializing data for/from the API calls. But these are less pressing I think. Let me know if you think thats a blocker.

@jlusiardi
Copy link
Owner

Hey,

  • I looked over your PR and do not have any findings. Great work!
  • More tests are good, I guess ;)
  • I could also try myself on the asyncio implementation of the cli tools to check to get more experience.

For further PRs:

  • I would love to have BLE as well on asyncio
  • Ok for the aiozeroconf

If you are confident after your QA with HASS.IO I will sure merge it!

@Jc2k
Copy link
Collaborator Author

Jc2k commented Sep 14, 2019

That's good news! Then:

  • I will start doing some QA with my main homeassistant as soon as I can
  • I'll throw a few more tests at it
  • I'll implement a demo cli to show it polling and listening to events at same time (i need something like this to test/debug the multiplexing of events and request/reply on the same connection) - but i'll let you pick up other CLI stuff as a asyncio practice.

And then i'll let you know!

BLE is definitely on the cards - i have started sketching it out in a branch a bit but wanted to stabilise the HTTP client first. I'm looking at this library - the author is a homeassistant user and has been quite responsive. After i asked they have kindly added descriptor reading support for us already and it seems it can potentially work on windows and mac as well.

@Jc2k Jc2k force-pushed the aioclient branch 2 times, most recently from c30e5f4 to 05799b7 Compare December 8, 2019 07:02
@Jc2k Jc2k force-pushed the aioclient branch 2 times, most recently from 6f0ab85 to e9e7d97 Compare December 8, 2019 10:58
@Jc2k
Copy link
Collaborator Author

Jc2k commented Dec 28, 2019

Hi @jlusiardi

I found a bit of time this month to work on this. It looks like CI might be broken for macs on master though. I have "fixed" it but it installs a python 3.6 instead of a python 3.5. I couldn't get python 3.5 working again.

I wanted to point out homekit/aio/main.py - you should be able to test this branch with the CLI yourself:

echo '{}' > pairing.json
python -m homekit.aio discover_ip
python -m homekit.aio pair_ip -f pairing.json -a testdevice -d DE:VI:CE:ID 
python -m homekit.aio get_accessories -f pairing.json -a testdevice
python -m homekit.aio get_characteristic -f pairing.json -a testdevice -c 1.10
python -m homekit.aio put_characteristic -f pairing.json -a testdevice -c 1.10 true
python -m homekit.aio get_events -f pairing.json -a testdevice -c 1.10
python -m homekit.aio list_pairings -f pairing.json -a testdevice
python -m homekit.aio unpair -f pairing.json -a testdevice

@jlusiardi
Copy link
Owner

Hi @Jc2k,

I will give it a good look when I’m back home, because all the HomeKit hardware is there.

The CI should be fixable earlier though.

Joachim

@PaulMcMillan
Copy link
Contributor

@Jc2k The way the PR currently is, you've got get_characteristics and put_characteristics pluralized, which is reasonable, but not what's in your example comment above and differs from the other client CLI.

@Jc2k
Copy link
Collaborator Author

Jc2k commented Dec 31, 2019

Good spot. Fixed the argparse definition for those 2.

@PaulMcMillan
Copy link
Contributor

I took the code for a spin with my Honeywell Lyric alarm controller, and can confirm the aio branch works well:
https://gist.github.com/PaulMcMillan/bb834a711c7055663888b7e39a193c84

Let me know if there are other tests you'd like to see.

@eugr
Copy link

eugr commented Jan 7, 2020

I tested it with my Insignia garage opener and it seems to be working well: https://gist.github.com/eugr/fc454156292c90407afe347b8e782094

@Jc2k
Copy link
Collaborator Author

Jc2k commented Jan 12, 2020

@PaulMcMillan @eugr thanks for the updates! Really pleased that the basics seem to be working for a range of devices!

@jlusiardi rebased + pushed a few more tests this morning. Mac CI has broken again in a different way this time. Seems to be setting up python without an ssl module now :/ Tried using the homebrew addon to install python based on a closed ticket in their git tracker.

@eugr
Copy link

eugr commented Jan 12, 2020

@Jc2k: have you tried Python from MacPorts distribution instead? It’s a completely separate install that does not use system libraries unlike Homebrew.

@rodriguezst
Copy link

I have been running this branch of homekit_python with @Jc2k branch of home-assistant for a few weeks without any issues. I have linked a Xiaomi Aqara Hub (HomeKit version) with some door and temperature sensors and everything works great and instantaneously. I can see the sensors updated on real time and also enable and disable the Hub's alarm. No issues at all on Home-Assistant log.

@jlusiardi
Copy link
Owner

we should investigate why the checks are failing on OS X, i think.

@stoprocent
Copy link

Any update on this pull request ?

@jlusiardi
Copy link
Owner

Hi @stoprocent, the PR sounds promising, but will still introduce a huge amount of code duplication. Because of that I am still a little unsure.

@Jc2k
Copy link
Collaborator Author

Jc2k commented Feb 5, 2020

Yeah i was worried about that too. There is more that can be done on that front, but its just going to make this PR even bigger. And ultimately there will be duplication, especially if we want a CLI so we can test the async code outside of other projects. But going asyncio native is the only way forward for HomeKit in HomeAssistant. The async <-> sync bridge is a big pain there. I guess it might be easier for me to spin this out into its own project like aiohomekit after all unfortunately.

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.

6 participants