Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Interlogix Ultrasync Alarm System Integration #42549

Closed
wants to merge 31 commits into from
Closed

Add Interlogix Ultrasync Alarm System Integration #42549

wants to merge 31 commits into from

Conversation

caronc
Copy link
Contributor

@caronc caronc commented Oct 29, 2020

Proposed change

Interlogix provides security solutions. One of which is their Self Contained (ZeroWire) Hub:
ZeroWire Hub Image

This wraps the UltraSync Hub tool I've already wrote. It additionally follows along a HA community thread identified here

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example entry for configuration.yaml:

This component will create these sensors:

  • area1state: Area 1 Status
  • area2state: Area 2 Status
  • area3state: Area 3 Status
  • area4state: Area 4 Status

Generally, most alarm systems only set up 1 area. So area01_state will probably be the only sensor used. The others will only activate if the areas are detected to exist (and are setup on the Hub).

This integration uses the control flow setup and can be added through the HA . The sensors automatically are automatically added when this is complete:

  • Configuration -> Integrations -> ➕ Add -> Search for Ultrasync
  • Provide the IP address of your UltraSync Hub on your network, your Login and your PIN

Note: You can only be logged into the ZeroWire hub with the same user once; a subsequent login with the same user logs out the other. Since this tool/software actively polls and maintains a login session to your Hub, it can prevent you from being able to log into at the same time elsewhere (via it's website). It is strongly recommended you create a second user account on your Hub dedicated to just this service.

There are 3 services associated with this integration you can call at any time:

  • away: Set's your alarm system to the Away Scene/Mode
  • stay: Set's your alarm system to the Stay Scene/Mode
  • disarm: Disarms your alarm.
# Arm alarm when everyone leaves the house
  - alias: 'Away Mode'
    trigger:
      platform: state
      entity_id: all
      to: 'not_home'
    action:
      service: ultrasync.away

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

To help with the load of incoming pull requests:

@project-bot project-bot bot added this to Needs review in Dev Oct 29, 2020
@springstan springstan changed the title Informix Ultrasync Alarm System Integration Add Informix Ultrasync Alarm System Integration Oct 29, 2020
@caronc caronc changed the title Add Informix Ultrasync Alarm System Integration Add Interlogix Ultrasync Alarm System Integration Oct 30, 2020
Copy link
Contributor

@epenet epenet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is support for YAML import really required on a new integration?

homeassistant/components/ultrasync/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/ultrasync/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/ultrasync/config_flow.py Outdated Show resolved Hide resolved
Copy link
Contributor

@iMicknl iMicknl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a quick look at your PR, hopefully this review is helpful. Especially the part around entity creation and data retrieval was unclear for me, it looks you are not really utilizing the DataUpdateCoordinator. For which purpose are you sending events in HA?

_LOGGER = logging.getLogger(__name__)


class AuthFailureException(IOError):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
class AuthFailureException(IOError):
class AuthFailureException(Exception):

A custom exeception class should inherit from Eception, not from IOError. On the next line you should also add pass, I believe. See https://www.programiz.com/python-programming/user-defined-exception.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a problem; done! 👍

)

except AuthFailureException:
errors["base"] = "cannot_connect"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As noted above, will this only be called when a connection is not possible or also when the credentials are not correct? (wrong pin). Currently you will always show 'Failed to connect.' (cannot_connect), which is not super helpful to the user.

AuthFailureException should be mapped to invalid_auth (Invalid authentication.). Otherwise you should rename the exception.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Done

) -> Dict[str, Any]:
"""Handle user flow."""
if self._async_current_entries():
return self.async_abort(reason="single_instance_allowed")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can users only have one Interlogix Ultrasync Alarm System? Perhaps it is better to set a unique id and check for duplicates.

await self.async_set_unique_id(user_input.get(CONF_HOST)) # or another unique id, like a serialnumber
self._abort_if_unique_id_configured()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for sharing this snippit of code. I think i did this properly; maybe you can review this part and let me know?


except Exception: # pylint: disable=broad-except
_LOGGER.exception("Unexpected exception")
return self.async_abort(reason="unknown")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return self.async_abort(reason="unknown")
errors["base"] = "unknown"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Done!


# validate by attempting to authenticate with our hub

if not usync.login():
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you the author of ultrasync package? It would be better to raise an error from the package and capture it here. Now all auth failures (no internet, wrong credentials, timeout) will all raise the same exception and thus the same error. This is not very helpful to the user.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am the author, yes. I agree it's not ideal; but 3 errors you identified that are all captured into one only play a role for the users initial hookup. Then it just works flawlessly from that point on.

There is no internet requirement here; the panel is usually in the persons local home network. So presuming the user is already accessing their panel internal on their network (via web browser), they're just entering the same 3 entries here too (host, pin and password). Basic troubleshooting would be to simply try the same host information in a new browser tab.

Either way, it's still a good suggestion, but I'd have to re-factor a section of the code to handle this sort of thing. Is this required to at the start to just get support for the Ultrasync Panel into HA?


async def _async_update_listener(hass: HomeAssistantType, entry: ConfigEntry) -> None:
"""Handle options update."""
await hass.config_entries.async_reload(entry.entry_id)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reloading the config entries should not be necessary, since you are using a coordinator. You could just set the new update interval directly on the coordinator.

    if entry.options[CONF_UPDATE_INTERVAL]:
        coordinator = hass.data[DOMAIN][entry.entry_id]["coordinator"]
        coordinator.update_interval = timedelta(seconds=entry.options[CONF_UPDATE_INTERVAL])

        await coordinator.async_refresh()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I put this code in place; i'll give it a whirl

host=config[CONF_HOST],
)

self._init = False
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this used? I couldn't find the use-case, if not, please remove.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Done

"status"
]

self._init = True
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this used? I couldn't find the use-case, if not, please remove.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch; removed 👍


# At least one sensor must be pre-created or Home Assistant will not
# call any updates
hass.data[DOMAIN][entry.entry_id][SENSORS]["area01_state"] = UltraSyncSensor(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this 'dummy' sensor created? In my opinion, it would make more sense to pull all available areas first in init.py and create the sensors from there. Eventually they could all pull their latest data from the DataUpdateCoordinator.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was required through advice provided to me by @raman325 who was a fantastic resource on the Home Assistant Discord chat at the time. Without per-creating a dummy sensor to work with nothing ever gets detected after. It's like a ticket into the event loop. If i don't create one at the initialization, future changes never get triggered.

If i do this, everything works great afterwards.

@property
def device_state_attributes(self):
"""Return the state attributes of the sensor."""
return self.__attributes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are the possible attributes? You need to specify a list/dict of possible attributes, so it is clear what will be added. This cannot be dynamic.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that they truly are dynamic. The ultrasync is dynamically configured from the panel or it's internal website. I don't knjow what is enabled and disabled until i first connect to the panel (using the pin/password provided by the configuration).

Once i can connect, i can then poll for all of the zones and sensors. Different sensors have different attributes associated with them (such as a sensor like a smoke detector, motion sensors, and door sensors, etc). So I'm able to poll this and build the sensor list dynamically. It actually works really well; I've had lots of positive feedback using HACS in the time being since here.

@github-actions
Copy link

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

@github-actions github-actions bot added stale and removed stale labels May 24, 2021
@home-assistant home-assistant deleted a comment from homeassistant Jun 17, 2021
@home-assistant home-assistant deleted a comment from homeassistant Jun 17, 2021
Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rebase on latest dev branch and make sure the build passes.

Comment on lines +30 to +68
async def async_setup(hass: HomeAssistantType, config: dict) -> bool:
"""Set up the UltraSync integration."""
hass.data.setdefault(DOMAIN, {})

return True
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need this anymore. We can move the code to async_setup_entry.

Suggested change
async def async_setup(hass: HomeAssistantType, config: dict) -> bool:
"""Set up the UltraSync integration."""
hass.data.setdefault(DOMAIN, {})
return True


async def async_setup_entry(hass: HomeAssistantType, entry: ConfigEntry) -> bool:
"""Set up UltraSync from a config entry."""
if not entry.options:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if not entry.options:
hass.data.setdefault(DOMAIN, {})
if not entry.options:

Comment on lines 53 to 90
await coordinator.async_refresh()

if not coordinator.last_update_success:
raise ConfigEntryNotReady
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
await coordinator.async_refresh()
if not coordinator.last_update_success:
raise ConfigEntryNotReady
await coordinator.async_config_entry_first_refresh()

if not coordinator.last_update_success:
raise ConfigEntryNotReady

undo_listener = entry.add_update_listener(_async_update_listener)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
undo_listener = entry.add_update_listener(_async_update_listener)
entry.async_on_unload(entry.add_update_listener(_async_update_listener))


hass.data[DOMAIN][entry.entry_id] = {
DATA_COORDINATOR: coordinator,
DATA_UNDO_UPDATE_LISTENER: [undo_listener],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
DATA_UNDO_UPDATE_LISTENER: [undo_listener],


# The hub can sometimes take a very long time to respond; wait
# 10 seconds before giving up
async with timeout(10):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't make sense. We have a poll interval of 1 second and a timeout of 10 seconds? The poll interval must be longer than the maximum time it takes to complete the poll.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue here is 9/10 the poll will return in under 1 second. So this is just more of a cautionary handling of the 1 off (so it breaks out) (let setting a SIGALRM if you will).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it can happen 1/10 my comment above still needs to be resolved.

Comment on lines +154 to +156
def __setitem__(self, key, value):
"""Set our sensor attributes."""
self.__attributes[key] = value
Copy link
Member

@MartinHjelmare MartinHjelmare Jun 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a pattern that we want. It would be ok to add a callback in the entity and connect that via our dispatch helper to a signal.

Normally though we let the coordinator return the new data that is fetched and let the entities that subscribe to the coordinator access that data as needed.

detected_sensors.add(sensor_id)
if sensor_id not in sensors:
# hash our entry
sensors[sensor_id] = UltraSyncSensor(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't store the entity. If we need to ask the entity to do something from outside the entity we use our dispatch helper.

data=ENTRY_CONFIG,
options={CONF_SCAN_INTERVAL: DEFAULT_SCAN_INTERVAL},
)
flow = init_config_flow(hass)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please set up the integration instead.

options={CONF_SCAN_INTERVAL: DEFAULT_SCAN_INTERVAL},
)
flow = init_config_flow(hass)
options_flow = flow.async_get_options_flow(entry)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use hass.config_entries.options.async_init.

Dev automation moved this from Needs review to Review in progress Jun 17, 2021
New Integrations automation moved this from Incoming to Awaiting Requested Changes Jun 17, 2021
@github-actions
Copy link

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

@github-actions github-actions bot added the stale label Sep 15, 2021
@cgarwood cgarwood moved this from Awaiting Requested Changes to Needs followup review in New Integrations Nov 30, 2021
@github-actions
Copy link

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

@frenck
Copy link
Member

frenck commented May 16, 2022

Because there hasn't been any activity on this PR for quite some time now, I've decided to close it for being stale.

Feel free to re-open this PR when you are ready to pick up work on it again 👍

@frenck frenck closed this May 16, 2022
Dev automation moved this from Review in progress to Cancelled May 16, 2022
New Integrations automation moved this from Needs followup review to Cancelled May 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Dev
  
Cancelled
New Integrations
  
Cancelled
Development

Successfully merging this pull request may close these issues.

None yet

8 participants