Improve Elmax alarm control panel #117689
Conversation
|
Please bump the dependency in a preliminary PR |
Done here: #117693 |
albertogeniola
force-pushed
the
elmax/improve_alarm_control_panel
branch
from
8d49456
to
16ca18b
Compare
MartinHjelmare
changed the title
albertogeniola
force-pushed
the
elmax/improve_alarm_control_panel
branch
from
16ca18b
to
8068479
Compare
|
Rebased after meeting dependent PR:#117693 |
| # Send the event data to every single device | ||
| for k, ep_status in self._state_by_endpoint.items(): | ||
| event_signal = f"{SIGNAL_PANEL_UPDATE}-{self.panel_entry.hash}-{k}" | ||
| async_dispatcher_send(self.hass, event_signal, ep_status) |
There was a problem hiding this comment.
I don't think this needs to use dispatcher/events when using an update coordinator since the entity can get the full list of states but only pull out the state update it is interested in, like in the example in https://developers.home-assistant.io/docs/integration_fetching_data/#polling-api-endpoints -- Can the event dispatching be removed and replaced with getting the data out of the PanelState?
There was a problem hiding this comment.
This was exactly how it worked before this PR. However, there is a reason why I am moving to a event-dispatcher approach. The idea is to be prepared to handle push-notifications, which will be advertised via web-sockets subscription in a later PR. To be honest, there was a unique PR introducing everything as a block, but I've received multiple suggestions to split that PR into smaller ones (this is the second PR out of 4); that's why I've closed the old big PR and created multiple smaller PRs.
Hope this clarifies!
There was a problem hiding this comment.
Sure, all for smaller PRs. I acknowledge the challenges of doing incremental refactorings like this (you make it too big then reviewer asks for it to be smaller, you make it incremental and the reviewer doesn't get the big picture and it doesn't make sense).
Maybe you can elaborate in a lot more detail what the end state is going to look like to make sure I understand where this is headed? or if there is a link to api documentation, etc.
There was a problem hiding this comment.
link to massive pr could be helpful to understand too
There was a problem hiding this comment.
Absolutely.
First of all, here is the original PR: #101668
About the final state, I am targeting an architecture where the coordinator handles both "polling" and "push" updates.
The idea is to keep the polling logic to handle the cloud-connected panels via current APIs and to enable lan-local push strategy if that option is available (that would be addressed via mDNS/zeroconf discovery).
The coordinator will implement this via a local object, _push_notification_handler that serves as a dispatcher for Elmax events. Every time a ElmaxPanel changes its status, it advieses it via websocket (if available) and that will trigger the update by re-using the function you mentioned _fire_data_update() that uses the dispatcher helper.
In this way we are using the same piece of code to serve both cases: cloud-polling and local push notifications.
You can have a look at the original PR common.py, which includes the ElmaxCoordinator implementation that goes towards that objective.
Contextually, all the various platforms' entities will be updated so that they won't call the coordinator.get_xxx_state() (where xxx stands for the specific device class the entity maps to). For instance, the is_on() method of ElmaxSensor will fetch its state directly via the _last_state local handle (see here).
Let me know if you need any more specific details about the target architecture and thanks a lot for the effort you've been putting in reviewing these PRs.
There was a problem hiding this comment.
OK thanks. I'm not seeing a large difference between what this is doing and the original PR is doing, so I think my comment still applies. Perhaps I need to elaborate on what i'm seeing and my suggestion in case it is not clear.
To be clear, I get the point about using a single data update coordinator. What i'm saying is to use the data update coordinator update APIs also rather than events. I don't yet think we're disagreeing, i think we're misunderstanding each other.
The original code I believe does something like this:
- Call
status = await self._client.get_current_panel_status()to get thePanelStatus(contains data for all entities) - [*] It is returned by
_async_update_data - [*] Individual entities get notified there is new data via the update coordinator entity stuff
- [*] The entity knows its id, and gets its state out of the
PanelStatus(it also was pre-populated in a map to make that easier but that isn't super important here)
The new path does something like the above, plus it also does this:
- Opens a websocket push notification handler
- The entity also registers an dispatch listener
- When a push notification arrives it gets a new
PanelStatus(contains data for all entities) - [*] Publishes a dispatch event for each individual entity within the
PanelStatus - [*] The entity receives the event and updates its state
The parts with [*] are the ways state changes are communicated to entities. What i'm suggesting is that given these are all update coordinator entities, we don't need two ways to publish state changes to the entities. We can use the existing mechanism for both.
What I would expect is something like a merged view:
- Call
status = await self._client.get_current_panel_status()to get thePanelStatus(contains data for all entities) as it does totday- [*] It is returned by
_async_update_data
- [*] It is returned by
- Opens a websocket push notification handler
- The entity also registers an dispatch listener
- When a push notification arrives it gets a new
PanelStatus(contains data for all entities) - [*] Call
async_set_updated_data
- [*] Individual entities get notified there is new data via the update coordinator entity stuff
- [*] The entity knows its id, and gets its state out of the
PanelStatus(it also was pre-populated in a map to make that easier but that isn't super important here)
So call async_set_updated_data when new data arrives and remove the additional event dispatching.
There was a problem hiding this comment.
Okay, let me try to rephrase (as an attempt to double check I got it right).
You are suggesting getting rid of the dispatcher helpers (both on coordinator and on entities). The idea is instead to leverage the coordinator logic in order to "notify" the dependant entities of an update. In other words, we need to rely on coordinator's notification listeners to do so. Is it correct?
In any case, I was considering the dispatcher approach instead of the internal coordinator one as it would allow a more fine-grained selective update for entities. Using the dispatcher approach, we might let different entities subscribe for specific class of updates (i.e. issuing dispatch events for devices' UUID). This is not being handled in this PR as we are issuing only a single event meaning "the whole panel state has been updated, all Elmax entities should fetch updated state". In the future we might trigger events for specific changes and dispatch finer-grained events so that only specific devices (the one that must update), using the dispatcher.
To be honest, now that I think more deeply of it, I believe I'm overcomplicating this. The current Elmax API does not provide changed state, but the whole state is served. To achieve the fine-grained option, I would need to compute the difference from older state and only dispatch_send() for entities that had changes. It would be probably less costly to update all Elmax entities rather than computing complex state diffs. In other words, your suggestion might be the right implementation to match the current API implementation.
What do you think?
There was a problem hiding this comment.
In any case, I was considering the dispatcher approach instead of the internal coordinator one as it would allow a more fine-grained selective update for entities. Using the dispatcher approach, we might let different entities subscribe for specific class of updates (i.e. issuing dispatch events for devices' UUID). This is not being handled in this PR as we are issuing only a single event meaning "the whole panel state has been updated, all Elmax entities should fetch updated state". In the future we might trigger events for specific changes and dispatch finer-grained events so that only specific devices (the one that must update), using the dispatcher.
To be honest, now that I think more deeply of it, I believe I'm overcomplicating this. The current Elmax API does not provide changed state, but the whole state is served.
Yes, that is what I assumed was going to happen but then noticed the full state was being passed so it was was equivalent to what the coordinator is already doing. But then, update coordinator can already handle the case where the data doesn't change and will skip updating the state machine when that is the case:
What do you think?
Yes, I think its fine to switch to webhook to improve update rate, but use the existing update coordinator "notification" mechanism since finer grained updates won't really materialize from this API.
There was a problem hiding this comment.
Okay, done in 119084d.
Should be fine now!
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
… future push-notifications.
|
Hi all, any update on that PR? Seems stale. |
Proposed change
Fixes #117650 and implements ARMING and DISARMING intermediate states.
Depends on: #117693
Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all..coveragerc.To help with the load of incoming pull requests: