search

home-assistant-libs / netdisco

:mag_right: Python library to scan local network for services and devices.
Other
266 stars 147 forks source link

Add Xbox One SmartGlass discovery #218

Closed tuxuser closed 5 years ago

tuxuser commented 5 years ago

Preparation for https://github.com/hunterjm/hassio-addons, adding Xbox One discovery via SmartGlass protocol.

homeassistant commented 5 years ago

Hi @tuxuser,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

balloob commented 5 years ago

Looks good. For the add-on you linked, not sure why it is an add-on and not all a custom component/platform.

tuxuser commented 5 years ago

@balloob It's an addon cause it relies on an external service (the REST server). It has to be run externally, cause its mainloop is utilizing gevent whereas Home-Assistant is using asyncio - it's simply not compatible.

We will send the custom component upstream ASAP tho, then document the dependency on xbox-smartglass-rest in the wiki.

balloob commented 5 years ago

You can run anything outside asyncio's event loop in a thread. Just can't have gevent monkey patching stdlib. However, if you just use gevent for launching your WSGI Flask app, there are other threaded app runners. Preferably the Home Assistant component could communicate directly with the XBox through some Python lib. Anyway, I don't really care, just some ideas if you want to reach a wider audience.

tuxuser commented 5 years ago

Thanks for your suggestions! For now we are not planning to switch from gevent tho. The REST server really is not the culprit here, being exlusively gevent, as that could be easily changed. The underlying protocol heavily relies on gevent, xbox-smartglass-core.

At some point we might switch to asyncio for all the smartglass stuff - right now, the external service is fine imo.

hunterjm commented 5 years ago

@balloob - @tuxuser and I initially went down the path of using the core lib inside HA, but couldn't figure out a way for the gevent loop to not get starved while maintaining HA performance. It's a very chatty protocol. I also couldn't figure out a clean way to run the two loops in a thread and have them communicate state back and forth. @tuxuser suggested exposing a separate API modeled off of the FireTV component, so that's the path we went down for now.

The plan is to clean up the integration and submit separate PRs for both the components and an "official" hass.io add-on to the main repo which packages it up nicely. I don't want to dirty up this PR with too much discussion around that, but we are both on the Discord if you would like to reach out and talk further about the best route for getting the whole implementation merged back into core.

rytilahti commented 5 years ago

Would it be better to have this as a config flow entry inside homeassistant? The procedure to make this work requires you to install and configure xbox-smartglass-rest, which requires quite a bit manual configuration (I had to manually authenticate through the rest api to make it show in discovery), or am I doing something wrong?

tuxuser commented 5 years ago

The basic discovery implemented here does not rely on anything external. How it's handled in HA then is a whole other story. PS: Good to merge now!