Add event platform to isy994 integration#169782
Conversation
Adds an event entity per Insteon load / keypad-button node so physical button presses (on, off, fast on/off, fade up/down/stop) are exposed as first-class entities. Replaces the device-trigger approach proposed in home-assistant#169694 per reviewer feedback — device triggers will fall out of the new entities automatically once that PR is reworked on top of this one. The existing isy994_control bus event still fires from the load entity (switch/light) for the same node, and the existing per-button sensor entities are unchanged, so existing automations keep working. A future PR will mark both the bus event and the per-button sensors deprecated.
home-assistant
Bot
added
cla-signed
has-tests
integration: isy994
new-feature
new-platform
labels
|
Hey there @bdraco, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
There was a problem hiding this comment.
Pull request overview
This PR adds an event platform to the isy994 integration to expose physical Insteon button/control presses (on/off/fast/fade) as first-class Home Assistant event entities, enabling automation triggers directly from entities rather than relying on the legacy isy994_control bus event.
Changes:
- Introduces a new
eventplatform (ISYButtonEvent) that maps pyisy control events to HAevent_typevalues and disables KeypadLinc sub-button entities by default. - Extends node categorization to allow parallel platform classification (nodes can be classified into
Platform.EVENTin addition to their primary platform). - Adds tests + snapshots for event entity creation, default-disable behavior, and control→event mapping; also centralizes the
isy994_controlevent name into a constant.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
homeassistant/components/isy994/const.py |
Adds EVENT_ISY994_CONTROL, introduces NODE_PARALLEL_PLATFORMS, and defines event-node filters for parallel classification. |
homeassistant/components/isy994/helpers.py |
Implements parallel node classification so nodes can also be tagged for Platform.EVENT. |
homeassistant/components/isy994/models.py |
Ensures IsyData tracks parallel-platform nodes and includes event-entity unique IDs for registry cleanup. |
homeassistant/components/isy994/entity.py |
Replaces the hardcoded "isy994_control" event name with EVENT_ISY994_CONTROL. |
homeassistant/components/isy994/event.py |
Adds the new event entity implementation and control→event_type mapping. |
homeassistant/components/isy994/strings.json |
Adds translations for the new event entity and its event_type attribute states. |
tests/components/isy994/test_event.py |
Adds functional tests and a snapshot test for the new event platform. |
tests/components/isy994/snapshots/test_event.ambr |
Adds snapshots covering the created event entities and their registry entries/states. |
Event entities only need control_events; status_events would trigger unnecessary async_write_ha_state on every node level change.
Disambiguates from the existing button-platform unique IDs (per review). Co-Authored-By: Claude Opus 4.7 <[email protected]>
Inherit async_added_to_hass from ISYNodeEntity so status_events keep availability in sync when the node is enabled/disabled. The overridden async_on_control still skips bus.fire to avoid double-firing isy994_control alongside the load entity. Co-Authored-By: Claude Opus 4.7 <[email protected]>
Avoids per-status-update state writes the earlier review flagged while keeping availability in sync via a NC_NODE_ENABLED-filtered subscription, matching the pattern in ISYAuxControlEntity. Co-Authored-By: Claude Opus 4.7 <[email protected]>
| CONTROL_TO_EVENT_TYPE: Final[dict[str, str]] = { | ||
| CMD_ON: "on", | ||
| CMD_OFF: "off", | ||
| CMD_ON_FAST: "fast_on", | ||
| CMD_OFF_FAST: "fast_off", | ||
| CMD_FADE_UP: "fade_up", | ||
| CMD_FADE_DOWN: "fade_down", | ||
| CMD_FADE_STOP: "fade_stop", | ||
| } |
There was a problem hiding this comment.
Does every device have every event type?
There was a problem hiding this comment.
Every device type added in the node filters has these basic event types. Even on/off switches can send fade up/down/stop which are Insteon's equivalent of hold / release. It may get more complicated if/when this gets expanded to Z-Wave and Node Server supports, but for Insteon--these are core events.
There was a problem hiding this comment.
How recognizable are the fade terms for insteon users? would it make more sense to use hold/release, which might be more understandable?
There was a problem hiding this comment.
They're standard language in the Universal Devices Admin Console:
Control defs from the eisy itself:
<control ID="33"><name>FDUP</name><label>Fade Up</label></control>
<control ID="34"><name>FDDOWN</name><label>Fade Down</label></control>
<control ID="35"><name>FDSTOP</name><label>Fade Stop</label></control>There was a problem hiding this comment.
How clear are these for the end user? As in, if this now only makes it clear for the people who access the console and those people would understand a variant with hold/release, wouldn't that make it easier for a broader audience? Like I am also not sure how often insteon is actually programmed by people or by installers
There was a problem hiding this comment.
Right that is good to mention, thanks for that! So from what I understand you want to keep the insteon naming?
Let me also add some context from our side as well, for example there's an architectural proposal for button event entities at home-assistant/architecture#1377. The proposal would aim to make it easier for people to automate. Another cool use case this would unlock is for example if you have a hue remote and an insteon remote, and you can target both and be like, if any is pressed and held, do x. This is just a proposal so it might not make it, but it might.
In any case, thank you for the context, I will drop this in the core team chat to see what others think about this
There was a problem hiding this comment.
No worries. I've been keeping an eye on that discussion too, it would be great to standardize but seems like everything has its quirks.
Leaving for record here: the supported UDI eisy "Button press"-type event commands are here. The insteon integration would align with most.
My bias would be to keep the Insteon naming but will align to alternative if needed. With the basic matter support proposed there would need to be some additional entities added here to separate out the on/off triggers for dimmers (right now one node vs two like matter/z-wave).
Let me know how you decide to proceed.
There was a problem hiding this comment.
So I just discussed this in the core team, and the conclusion is is that we rather use recognizable names (ideally from that proposal) instead of adding more new ones. https://discord.com/channels/330944238910963714/1475552721515053268/1501253582744322161
There was a problem hiding this comment.
Ok. I added a comment over there (home-assistant/architecture#1377 (comment)). I think I'll have to sit on this until that proposal gets a little more solidified on the path forward.
There was a problem hiding this comment.
@joostlek, following up on this now that the arch discussion has finished. I want to agree one thing before conforming to the rest of the proposal.
The proposal includes:
A note on directional buttons
Some physical buttons report direction (e.g. up/down, on/off) from a single source. The recommended approach is to expose these as separate BUTTON entities, one per direction. This composes naturally with the standard event types and keeps automations straightforward. Including a
directionevent data field on a single entity is a valid alternative for cases where splitting into multiple entities does not make sense, but this is not part of the standard.
I advocated for this specifically because Insteon Keypadlinc Buttons are one physical button that changes the direction they emit based on the sequence in which they are pressed (see comment on the arch discussion ), and can't be separated into two logical entities.
For ease of implementation and consistency within the integration, are you OK with keeping the same, direction field approach for the other paddle switches too?
Justification: these are reported as one entity from the controller (unlike Z-Wave which has up and down scene controls separate), and we would have to rely on home assistant separating everything into two entities and tracking both states. There are opportunities for race conditions on Long presses if we try and separate these. For example, FDSTOP/ long_press_end sometimes gets lost on the network if someone switches from holding up to holding down quickly. If these were separate entities, one might get stuck in a held down state without custom logic to guard against.
| if hasattr(self._node, "control_events"): | ||
| self._control_handler = self._node.control_events.subscribe( |
There was a problem hiding this comment.
why would node not have control events?
There was a problem hiding this comment.
This is a little bit of a typing guard -- _node could also be a Group class from PyISY, which does not have the "control_events" attribute. A Group shouldn't be assigned to the Event platform but was guarding here to be safe. These hasattr checks and typing quirks throughout this integration are on my roadmap to eliminate with an eventual migration to pyisyox, but was aiming to stay consistent with current practice here. I think the filter in helpers.py should ensure only Node instances reach this code, so if you want me to drop it I can.
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
- Remove redundant button entity name (device class default is used) - Sentence case for fade/fast event_type labels - Use common::state::on/off references for on/off labels - Move EVENT_BUTTON_UNIQUE_ID_SUFFIX from const.py to event.py (platform-specific constant); models.py imports it from event.py Co-Authored-By: Claude Opus 4.7 <[email protected]>
| event_type = CONTROL_TO_EVENT_TYPE.get(event.control) | ||
| if event_type is None: | ||
| return | ||
| self._trigger_event(event_type) |
| for entry in er.async_entries_for_config_entry( | ||
| entity_registry, mock_config_entry.entry_id | ||
| ): | ||
| if entry.disabled_by: | ||
| entity_registry.async_update_entity(entry.entity_id, disabled_by=None) |
| FILTER_INSTEON_TYPE: [ | ||
| "1.14.", | ||
| "1.32.", | ||
| "1.45.", | ||
| "1.65.", |
| CONTROL_TO_EVENT_TYPE: Final[dict[str, str]] = { | ||
| CMD_ON: "on", | ||
| CMD_OFF: "off", | ||
| CMD_ON_FAST: "fast_on", | ||
| CMD_OFF_FAST: "fast_off", |
| if name.startswith(parent_name): | ||
| return name[len(parent_name) :].lstrip(" -_:.") or name |
Co-authored-by: Copilot Autofix powered by AI <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
homeassistant/components/isy994/event.py:152
- Gate event emission until PyISY reports the WebSocket as fully synchronized. The integration is still pinned to pyisy 3.6.1, which predates automicus/PyISY#513, and this handler never checks the synchronized connection state; consequently the controller's replayed
DON/DOFframes trigger automations on startup, reload, and reconnect. This needs the released PyISY backport, a connection-state guard here, and regression coverage before these entities are enabled.
event_type = CONTROL_TO_EVENT_TYPE.get(event.control)
if event_type is None:
return
self._trigger_event(event_type)
homeassistant/components/isy994/event.py:78
- Strip the parent name only when it ends at a label boundary. The current raw prefix check turns unrelated names such as parent
Halland buttonHallway Bintoway B, producing a corrupted entity name.
if name.startswith(parent_name):
return name[len(parent_name) :].lstrip(" -_:.") or name
return name
| if hasattr(self._node, "control_events"): | ||
| self._control_handler = self._node.control_events.subscribe( | ||
| self.async_on_control | ||
| ) | ||
| self._change_handler = self._node.isy.nodes.status_events.subscribe( | ||
| self._async_on_availability_change, | ||
| event_filter={ | ||
| TAG_ADDRESS: self._node.address, | ||
| ATTR_ACTION: NC_NODE_ENABLED, | ||
| }, | ||
| key=self.unique_id, | ||
| ) |
HOLD
Two blocking issues:
1. Need resolution on home-assistant/architecture#13772. Identified issue during parallel development on hacs-udi-iox (next gen version of this integration) that
evententities will trigger on every websocket connection/blip causing spurious automation triggers. Thepyisyoxfix is shbatm/pyisyox#170 but this has not been backported to PyISY yet: automicus/PyISY#512.Proposed change
Adds an `event` entity per Insteon load / keypad-button node so physical button presses (`on`, `off`, `fast_on`, `fast_off`, `fade_up`, `fade_down`, `fade_stop`) are exposed as first-class entities. This is the approach reviewers requested in #169694 in lieu of hand-rolled device triggers — once this lands, the core
eventintegration provides device triggers for these entities automatically.Implementation notes:
A future PR will mark both the `isy994_control` bus event and the per-button sensor entities deprecated, with the standard deprecation cycle, don't want to cloud this change with Breaking Changes since more work is needed to be able to get rid of that bus event firing from the entity.
Type of change
Additional information
Checklist
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`.
Updated by running `python3 -m script.gen_requirements_all`.
To help with the load of incoming pull requests: