home-assistant / core

:house_with_garden: Open source home automation that puts local control and privacy first.
https://www.home-assistant.io
Apache License 2.0
70.03k stars 29.11k forks source link

Lyric Honeywell integration - fails with leak detector sensors #117335

Open tskirvin opened 2 months ago

tskirvin commented 2 months ago

The problem

When trying to add Lyric Honeywell integration, it fails for the WLD3 water leak sensor. This appears to be an ongoing problem, based on the device not providing a macID (it instead offers a deviceID, which looks like it should be on all items). It sure feels like a simple try statement around the two calls to macID would solve this?

This is probably a repeat of 88471 (and 52528 before that), but I can't re-open that.

What version of Home Assistant Core has the issue?

core-2024.4.3

What was the last working version of Home Assistant Core?

(none)

What type of installation are you running?

Home Assistant Container

Integration causing the issue

lyric

Link to integration documentation on our website

https://www.home-assistant.io/integrations/lyric/

Diagnostics information

2024-05-12 19:25:35.876 ERROR (MainThread) [homeassistant.components.climate] Error adding entity None for domain climate with platform lyric
Traceback (most recent call last):
  File "/usr/src/homeassistant/homeassistant/helpers/entity_platform.py", line 7
50, in _async_add_entity
    device = dev_reg.async_get(self.hass).async_get_or_create(
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/src/homeassistant/homeassistant/helpers/device_registry.py", line 6
57, in async_get_or_create
    connections = _normalize_connections(connections)
                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/src/homeassistant/homeassistant/helpers/device_registry.py", line 1
224, in _normalize_connections
    (key, format_mac(value)) if key == CONNECTION_NETWORK_MAC else (key, value)
          ^^^^^^^^^^^^^^^^^
  File "/usr/src/homeassistant/homeassistant/helpers/device_registry.py", line 3
79, in format_mac
    if len(to_test) == 17 and to_test.count(":") == 5:
       ^^^^^^^^^^^^
TypeError: object of type 'NoneType' has no len()

Example YAML snippet

No response

Anything in the logs that might be useful for us?

No response

Additional information

Sample json from the logs of a particular water sensor:

{'devices': [{'waterPresent': False, 'currentSensorReadings': {'time':          '2024-05-12T12:46:18', 'temperature': 20.85, 'humidity': 57.7}, 'currentAlarms':[], 'lastCheckin': '2024-05-12T12:46:20', 'lastDeviceSettingUpdatedOn':         '0001-01-01T00:00:00', 'batteryRemaining': 29, 'isRegistered': True,            'hasDeviceCheckedIn': True, 'isDeviceOffline': False, 'firstFailedAttemptTime': '0001-01-01T00:00:00', 'failedConnectionAttempts': 0, 'wifiSignalStrength': -48,'time': '2024-05-12T12:46:19', 'deviceClass': 'LeakDetector', 'deviceType':     'Water Leak Detector', 'deviceID': 'a481b5a7-47cf-4a68-9374-b181b7f55452',      'deviceInternalID': 316664, 'userDefinedDeviceName': 'Laundry', 'backend': {},  'isAlive': True, 'isUpgrading': False, 'isProvisioned': True, 'deviceSettings': {'temp': {'high': {'limit': 37}, 'low': {'limit': 7}}, 'humidity': {'high':     {'limit': 70}, 'low': {'limit': 20}}, 'userDefinedName': 'Laundry',             'buzzerMuted': False, 'checkinPeriod': 24, 'currentSensorReadPeriod': 60},      'service': {'mode': 'Up'}, 'deviceRegistrationDate':
'2017-07-06T20:52:20.1266667', 'deviceVariant': 'WLD3'}]}
home-assistant[bot] commented 2 months ago

Hey there @timmo001, mind taking a look at this issue as it has been labeled with an integration (lyric) you are listed as a code owner for? Thanks!

Code owner commands Code owners of `lyric` can trigger bot actions by commenting: - `@home-assistant close` Closes the issue. - `@home-assistant rename Awesome new title` Renames the issue. - `@home-assistant reopen` Reopen the issue. - `@home-assistant unassign lyric` Removes the current integration label and assignees on the issue, add the integration domain after the command. - `@home-assistant add-label needs-more-information` Add a label (needs-more-information, problem in dependency, problem in custom component) to the issue. - `@home-assistant remove-label needs-more-information` Remove a label (needs-more-information, problem in dependency, problem in custom component) on the issue.

(message by CodeOwnersMention)


lyric documentation lyric source (message by IssueLinks)

wcookjm commented 1 month ago

I looked over the code and it appears that it only supports thermostats and room sensors. Other devices in the ecosystem like water leak sensor, security, and camera don't appear to be supported. So I'm guessing that this would need be a feature request but I'll let the developer comment as I am not one.

I have water leak sensor too so I would vote for the feature request.

tskirvin commented 1 month ago

That's why I included the yaml snippet; it was closer than I expected! The right fields look to be there.

rewardone commented 1 month ago

I don't think timmo has been able to dedicate any time to support the integration. I was hoping to request Water Leak devices added to the aiolyric package, but issues are getting marked as stale and auto closed.

Looks like 88471 and 52528 were in support of similar instances with non-thermostat devices, but also had to be resolved by someone other than the code owner.

Seems any additional support of the Lyric package for non-thermostat devices may need to come from someone else in the community.

tskirvin commented 1 month ago

I'm possibly willing to go at least test my "this is fairly easy" theory, but only if there's somebody that can take my pull commit if necessary. Is there anybody?

rewardone commented 1 month ago

Timmo did accept a pull request in aiolyric at the end of last year, so I'd say it's a possibility.

tamorgen commented 2 weeks ago

Another vote for this feature. I'm interested in these sensors simple because my home owners insurance has a "Connected Home" discount if you have at least two of these sensors. I already have other leak sensors that can turn off my main water valve, but they won't accept sensors/automation that actually do something, only ones that will email me if there is leak. I guess their idea of a connected home is from 1995.

I'm still planning on buying them, but if they were integrated into HA, I could add them to other areas of my home to increase reliability.

k7hpn commented 1 week ago

I'm possibly willing to go at least test my "this is fairly easy" theory, but only if there's somebody that can take my pull commit if necessary. Is there anybody?

If we can get this working I'm pretty sure we can scare up some Home Assistant people to help out if @timmo001 is not available to merge fixes.

It looks like the water sensors return a deviceID that is a GUID and the current Lyric component is relying on macID as a dictionary key for storing devices. I don't have a thermostat, so I can't tell for sure, but the API documentation makes it look as though thermostats have a deviceID as well? So maybe if that dictionary is re-keyed to use deviceID then these devices can just be added as sensors?

tskirvin commented 1 week ago

I did make a run at it, and it was (unsurprisingly) not as simple as I'd like. I have to admit that switching over to deviceID whole-sale might work better, but that'd actually affect existing users and I didn't see a good enough test suite to do it anyway.

So - I think it still could be done. I still think it's worth doing. I'm not sure I have a quick solution.

tamorgen commented 1 week ago

@tskirvin, if you need testers, let us know. I just got two of these sensors in today, and going through the Lyric enrollment process, it shows the devices on the Lyric page to enroll, but then when it comes back to HA, the devices aren't there. I'm sure this is the same behavior others are seeing.