Eventsocket enhancment: One event socket for all devices

mkmer commented 3 months ago

Currently, the API creates an event socket for each appliance in : washerdryer, oven, and aircon. https://github.com/abmantis/whirlpool-sixth-sense/blob/4d12c0892610d2e817d8b3addeeb7e1fc1d5e0f6/whirlpool/appliance.py#L79-L92 https://github.com/abmantis/whirlpool-sixth-sense/blob/4d12c0892610d2e817d8b3addeeb7e1fc1d5e0f6/whirlpool/appliance.py#L183-L197

This really should move to the appliance manager, create a single socket for all appliances, and register each appliance with the eventsocket w/callbacks. I suspect whirlpool is chasing data costs and this multiple connection is why they shut us down recently.

@NodeJSmith - maybe you have some time to tackle / review?

NodeJSmith commented 2 months ago

Sure, I can take a look. Now that the update is in HA I have a reason to look at this again. I have a few other features I will be looking to contribute but I can do this first if it's going to help keep Whirlpool from pulling a MyQ

mkmer commented 2 months ago

Much appreciated! I'm not "positive" they are "mad", but after seeing in the issues over the last few weeks, how many people are using multiples, we really should optimize / minimize the load put on Whirlpool. I've had this on my radar since we added washer/dryer but just haven't had the time to shake it out. Let me know if you need help testing or to bounce ideas.

NodeJSmith commented 2 months ago

@mkmer I'm seeing that we do not actually instantiate any appliances directly in this library, it looks like that happens in HA. Does that need to remain that way? I was thinking of having AppliancesManager instantiate each of them when it creates the appliances list, instead of just storing the attributes - that way it can handle registering callbacks. I think that would be fine on the HA side as well, from what I'm seeing, but I wanted to make sure there wasn't a reason to avoid this that I'm not aware of.

So something like this in AppliancesManager:

        if "airconditioner" in data_model:
            self._aircons.append(
                Aircon(
                    backend_selector=self._backend_selector,
                    auth=self._auth,
                    said=appliance["SAID"],
                    session=self._session,
                )
            )
            return

Which would let us do something like this in HA - this also removes the _wd.connect() bit, because that connection would be started by AppliancesManager, and just the one connection. Right now we pass async_get_clientsession(hass) to the WasherDryer's session as we instantiate it in HA, but that would also already be set in AppliancesManager so that wouldn't change either.

    for _wd in whirlpool_data.appliances_manager.washer_dryers:
        entities.extend(
            [
                WasherDryerClass(
                    _wd.said,
                    _wd.name,
                    description,
                    _wd,
                )
                for description in SENSORS
            ]
        )

mkmer commented 2 months ago

My thoughts (maybe a bit rambling) : On the HA side, I think we need to put a data coordinator. The data coordinator will handle capturing the data and sending it to the respective entities in HA "automatically". HA would only need 1 callback in the coordinator from the API for all devices. Currently, each device in HA has a callback function registered with the API. Either way, the API still needs a callback registered to update the entities. I suppose alternatively, we could have 1 socket in appliance with callbacks registered from each device inside the API, then the devices update their specific data from an event and callback HA by device. Each device would still have a callback in HA. As we register devices callbacks, there is a message on the socket to register events for each appliance - this can happen at any time, so as we discover appliances we can sent the registration message. Not sure I've helped resolve your original question...

NodeJSmith commented 2 months ago

@mkmer so I have this branch that I got to a working state last night: https://github.com/NodeJSmith/whirlpool-sixth-sense/tree/feature/move_event_socket_to_app_manager

This puts the event socket into appliances manager and passes appliances manager to each appliance as it's initialized. Appliances manager handles setting up the socket and passes reach message it receives to the correct appliance. The appliance also uses the appliances manager to send attribute updates.

I think this aligns with your first idea, although there may be some additional changes needed to have it be the data coordinator with HA. What are your thoughts on this approach?

mkmer commented 2 months ago

I think your on the right track - this part https://github.com/NodeJSmith/whirlpool-sixth-sense/blob/69b3dbdd1be3a86c9a0ef1813b9c57ccdb8684b9/whirlpool/appliancesmanager.py#L257-L258 will need to be a list of callbacks registered from HA - currently each device registers with it's "own", now it will be common. We should only callback the device that changed if we can.

NodeJSmith commented 2 months ago

I'm not sure I'm following. I don't see anything in HA where callbacks are registered, except for WasherDryerTimeClass which has update_from_latest_data, but that just queries the underlying appliance data, same as everything else as far as I can tell.

In this library directly each appliance registers it's own callbacks to its own event socket, but in my branch the appliances manager creates one event socket and registers itself for callbacks. Each time it receives a message from the event socket it checks the SAID and sends the message to that appliance to update its attributes, that's the bit of code you linked - it takes the attributes and the appliance, it doesn't loop through all the appliances.

Are you saying that we want to change how HA interacts with this library, to have it provide callbacks for the entities? If we're not changing how HA interacts with it, I think my branch does everything you mentioned.

mkmer commented 2 months ago

It's completely possible I have forgotten how it works. I thought each sensor registers with the event socket. Each climate device and oven should also register with the event socket to be notified of changes to the underlying data. Since we currently make event sockets for each "sub class" we don't need to worry about which are called back, but once we move to one socket, we should only callback the entities that change. (At a minimum the correct appliance only, no need to thrash the HA state machine). Maybe register the callbacks with an SAID or some other item we can use to look up and trigger the right ones.

Sensor:

https://github.com/home-assistant/core/blob/fc1ebdaaa37daeb0559315cf9dbac7d73d29f25a/homeassistant/components/whirlpool/sensor.py#L206

https://github.com/home-assistant/core/blob/fc1ebdaaa37daeb0559315cf9dbac7d73d29f25a/homeassistant/components/whirlpool/sensor.py#L210

https://github.com/home-assistant/core/blob/fc1ebdaaa37daeb0559315cf9dbac7d73d29f25a/homeassistant/components/whirlpool/sensor.py#L258

https://github.com/home-assistant/core/blob/fc1ebdaaa37daeb0559315cf9dbac7d73d29f25a/homeassistant/components/whirlpool/sensor.py#L262

Climate: https://github.com/home-assistant/core/blob/fc1ebdaaa37daeb0559315cf9dbac7d73d29f25a/homeassistant/components/whirlpool/climate.py#L138

https://github.com/home-assistant/core/blob/fc1ebdaaa37daeb0559315cf9dbac7d73d29f25a/homeassistant/components/whirlpool/climate.py#L143

No oven yet :(

abmantis commented 2 months ago

I agree that we should move in the direction of having a single websocket. IMO, we should probably have a Coordinator on HA, as @mkmer suggested, but I think we may not need that for the initial version.

So, my proposal would be:

Add a new method "connect()" to AppliancesManager that:
- fetches the appliances
- starts the Eventsocket
- creates the instances of the classes for each fetched appliance (Aircon, Oven, etc)
Change the properties aircons, washer_dryers, and ovens (and respective member variables) so that they return lists of each appliance class, instead of the json objects.
Remove all Eventsocket code from the Appliance class
In Appliance, rename _event_socket_handler() to handle_socket_event(). This method should be called from AppliancesManager when there are events from Eventsocket.
ApplianceManager should filter the events so that it calls handle_socket_event() for the correct appliance.
Data GET/POST should remain on the Appliance class

With that, the changes on HA side are quite small: instead of instantiating the appliance objects, it gets them using AppliancesManager.aircons(), ApplianceManager.ovens(), etc. No changes are needed regarding the event handling, since the callback stays in the appliance class.

abmantis / whirlpool-sixth-sense

Eventsocket enhancment: One event socket for all devices #51