lawtancool / pyControl4

Python 3 asyncio package for interacting with Control4 systems
https://lawtancool.github.io/pyControl4
Apache License 2.0
38 stars 16 forks source link

v1.0.0 - Migrating to Websockets for real time updates #8

Closed lorddaren closed 2 years ago

lorddaren commented 3 years ago
lorddaren commented 3 years ago

I am open to design changes. I figured this would just get us started. I working on the sensors proxy now as well so we can get sensors. I don't have any door locks so I am not sure I could implement that but I do have the audio matrix and thermostats that I want to get integrated as well.

lawtancool commented 3 years ago

Hi @Xorso! Thanks for your PR!

I'm going to look over the code and test it on my system soon, but for now could you try formatting your code with https://pypi.org/project/black/? This helps keep code in the repo formatted consistently and prevents random formatting changes to the existing code. I'm pretty sure Home Assistant also requires Black formatting for when we make PRs to the core as well.

lorddaren commented 3 years ago

Interesting. I will update the PR. I did run black before I submitted the PR. I will try again.

lawtancool commented 3 years ago

Hi @Xorso!

I've looked over the code and made a few small comments.

One design change I was considering is to move all the websockets code (including C4DirectorNamespace and all the new callback code under C4Director) into a separate file called websocket.py.

The callback code that is currently under C4Director would become a new class called C4Websocket. Then, in the Home Assistant integration, we would use a C4Websocket object to register the callbacks:

(in core/__init__.py)

async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool:
...

    director = C4Director(
        config[CONF_HOST], director_token_dict[CONF_TOKEN], director_session
    )
    entry_data[CONF_DIRECTOR] = director

    websocket = C4Websocket(config[CONF_HOST], director_token_dict[CONF_TOKEN])
    entry_data[CONF_WEBSOCKET] = websocket
...

class Control4Entity(Entity):
...
    async def async_added_to_hass(self):
        """Sync with HASS."""
        await super().async_added_to_hass()
        await self.hass.async_add_executor_job(
            self.entry_data[CONF_WEBSOCKET].add_device_callback,
            self._idx,
            self._update_callback,
        )
        _LOGGER.debug(f"Registering device {self._device_id} for callback")
        return True

This would help keep the director.py file clean, as the callback code doesn't need to be inside the C4Director class except for needing the director_bearer_token, self.base_url, and self.wss_url.

lawtancool commented 3 years ago

@Xorso I ended up implementing the changes I suggested in the previous comment, please take a look and let me know what you think!

If we go ahead with this change, we will have to change the code in the Home Assistant integration to use the C4Websocket object as well.

lorddaren commented 3 years ago

Looks good to me. I like the idea of putting it in it's own module. You are right it keeps things cleaner. This was just a first attempt hack and Ithink there are still going to be issues to resolve along the way but It has been working out well for me (there does appear to be some frequent disconnects so we may need to adjust the keep-alive on the socket)

lorddaren commented 3 years ago

@lawtancool Since you fixed and merged your changes is there anything else you would like me to do with this PR?

lawtancool commented 3 years ago

@lawtancool Since you fixed and merged your changes is there anything else you would like me to do with this PR?

I haven't had time to test the code yet, but I think we should be done soon with this. I'll start looking at the Home Assistant code soon so we can try to merge it into the official repo.

lorddaren commented 3 years ago

Sounds good. Just wanted to make sure you were not waiting on me.

jsb5151 commented 2 years ago

Thanks for your work guys. Any chance we’ll get to see Websocket support for HA soon? Anything I can do to help test this?

I’m experiencing controller freezes and I’m not sure if it’s related to all the HTTP polling requests sent by HA…

lorddaren commented 2 years ago

Any change here?

lawtancool commented 2 years ago

@Xorso I'm going to merge this code and release it as v1.0.0b1 to make it easier to test the Home Assistant integration code. Once the Home Assistant integration is done, I'll re-release with any necessary changes as v1.0.0.

Thanks for your work on this!