Add support for room sensor accessories assigned to a Honeywell (Lyric) Thermostat #104343

dalinicus · 2023-11-22T01:09:22Z

Breaking change

Proposed change

For the Lyric integration, adds additional sensors for room sensor accessories assigned to a thermostat. Each room sensor accessory will have a device created for it, sensors created for temperature and humidity, and via_device assigned to the respective thermostat.

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)
Deprecation (breaking change to happen in the future)
Breaking change (fix/feature causing existing functionality to break)
Code quality improvements to existing code or addition of tests

Additional information

This PR fixes or closes issue: n/a
This PR is related to issue: n/a
Link to documentation pull request: Update lyric documentation to reflect additional sensors created for room sensor accessories assigned to thermostats home-assistant.io#29956

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
I have followed the perfect PR recommendations
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:

Documentation added/updated for www.home-assistant.io

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.
For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

I have reviewed two other open pull requests in this repository.

- Update coordinator to refresh and grab information about room sensor accessories assigned to a thermostat - Add sensor entities for room humidity and room temperature - Add devices to the registry for each room accessory - "via_device" these entities through the assigned thermostat.

home-assistant · 2023-11-22T01:09:29Z

Hey there @timmo001, mind taking a look at this pull request 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 pull request.
@home-assistant rename Awesome new title Renames the pull request.
@home-assistant reopen Reopen the pull request.
@home-assistant unassign lyric Removes the current integration label and assignees on the pull request, 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 pull request.
@home-assistant remove-label needs-more-information Remove a label (needs-more-information, problem in dependency, problem in custom component) on the pull request.

emontnemery · 2023-11-22T08:53:26Z

homeassistant/components/lyric/__init__.py

@@ -158,8 +165,43 @@ class LyricDeviceEntity(LyricEntity):
    def device_info(self) -> DeviceInfo:
        """Return device information about this Honeywell Lyric instance."""
        return DeviceInfo(
+            identifiers={(dr.CONNECTION_NETWORK_MAC, self._mac_id)},


Why is the mac address added as an identifier, is it to make via_device work for the sub devices?

emontnemery · 2023-11-22T08:54:24Z

homeassistant/components/lyric/__init__.py

+        location: LyricLocation,
+        device: LyricDevice,
+        room: LyricRoom,
+        accessory: LyricAccessories,


The class name suggests plural, but the variable name suggests singular. Which one is it?

Its singular. Looks like its misnamed in the aiolyric library that his object comes from.

homeassistant/components/lyric/__init__.py

joostlek · 2023-11-22T10:58:33Z

homeassistant/components/lyric/__init__.py

+        )
+
+
+class LyricAccessoryEntity(LyricDeviceEntity):


One question that I have, currently an Accessory is bound to a room. Are there more types of accessories? Are all accessories bound to a room?

I'm not aware of any other thermostat accessories for this device, but they all appear to have the same data structure from the Lyric API. The only values I have observed for accessories are Thermostat (which is a duplicate of the parent thermostat) and IndoorAirSensor, and looking at their product pages I can only find the one model of air sensor and no other accessories. IndoorAirSensor is the only value they use from within their API reference as well.

Air sensors must assigned a "room" from the Honeywell app in order to be operable, yes.

homeassistant/components/lyric/sensor.py

- update docstring to reflect ownership by thermostat - fixed potential issue where a sensor would not be added if its temperature value was 0

dalinicus · 2023-11-22T15:02:41Z

Good Morning (or whatever greeting appropriate for your time zone) @emontnemery and @joostlek ,

I have made and tested the requested code changes, and answered the questions you posed. I have re-marked the PR for review. Please don't hesitate to ask followup questions or recommend further changes. Appreciate all your help on this.

t3hk0d3 · 2023-11-23T22:54:39Z

homeassistant/components/lyric/__init__.py

+                for location in lyric.locations:
+                    for device in location.devices:
+                        if device.deviceClass == "Thermostat":
+                            await lyric.get_thermostat_rooms(


Maybe it would be more performant to await all promises altogether?

https://docs.python.org/3/library/asyncio-task.html#asyncio.gather

disforw · 2023-11-24T13:54:12Z

@dalinicus nice! Any possibility you can grab the motion as a binary_sensor as well??? Pwwease 🙏

dalinicus · 2023-11-24T14:08:01Z

@dalinicus nice! Any possibility you can grab the motion as a binary_sensor as well??? Pwwease 🙏

Yup! Have it mostly coded but requires the addition of the binary_sensor platform which is new to this integration. As soon as this one is approved and merged, I'll add that in a second pull request shortly after

disforw · 2023-11-24T14:14:20Z

Sweet deal. I will be looking out for it!
Happy holidays mate..

dalinicus · 2023-12-17T16:32:12Z

@emontnemery and @joostlek, figured I'd check in with y'all since its been a month. Any additional feedback on this change? I'd be looking to make additional changes to add motion sensor support once this base functionally has been merged.

Hope you're both having a happy holidays.

github-actions · 2024-03-27T22:05:08Z

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.
If you are the author of this PR, please leave a comment if you want to keep it open. Also, please rebase your PR onto the latest dev branch to ensure that it's up to date with the latest changes.
Thank you for your contribution!

joostlek · 2024-04-03T23:06:54Z

Wait was this now ready for review or not

disforw · 2024-04-03T23:17:14Z

I believe it was ready 👍

dalinicus · 2024-04-04T16:32:57Z

Wait was this now ready for review or not

Looks like there was some merge conflicts since I last worked on this. I've synced up with dev, resolved the conflicts, and re-tested.

Working locally. Everything should be good for a review 👍🏻

emontnemery

Looks good, just a minor comment.

emontnemery · 2024-04-15T14:54:35Z

homeassistant/components/lyric/__init__.py

+                for location in lyric.locations:
+                    for device in location.devices:
+                        if device.deviceClass == "Thermostat":
+                            await lyric.get_thermostat_rooms(
+                                location.locationID, device.deviceID
+                            )


I agree with @t3hk0d3, please use asyncio.gather instead, something like:

Suggested change

for location in lyric.locations:

for device in location.devices:

if device.deviceClass == "Thermostat":

await lyric.get_thermostat_rooms(

location.locationID, device.deviceID

)

asyncio.gather(

lyric.get_thermostat_rooms(

location.locationID, device.deviceID

)

for device in location.devices

for location in lyric.locations

if device.deviceClass == "Thermostat"

)

Updated per suggestion; thanks for the code snippet. Re-tested locally and it continues to work as expected.

home-assistant · 2024-04-15T14:56:08Z

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

emontnemery

LGTM, thanks @dalinicus 👍

home-assistant bot added cla-signed integration: lyric new-feature Quality Scale: silver labels Nov 22, 2023

dalinicus mentioned this pull request Nov 22, 2023

Update lyric documentation to reflect additional sensors created for room sensor accessories assigned to thermostats home-assistant/home-assistant.io#29956

Merged

8 tasks

fixed pre-commit issues.

bda23a4

dalinicus mentioned this pull request Nov 22, 2023

Add Honeywell Lyric sensors for Room entities #99546

Closed

20 tasks

emontnemery changed the title ~~Add support for room sensor accessories assgined to a Honeywell (Lyric) Thermostat~~ Add support for room sensor accessories assigned to a Honeywell (Lyric) Thermostat Nov 22, 2023

emontnemery self-assigned this Nov 22, 2023

emontnemery reviewed Nov 22, 2023

View reviewed changes

joostlek reviewed Nov 22, 2023

View reviewed changes

homeassistant/components/lyric/sensor.py Outdated Show resolved Hide resolved

joostlek reviewed Nov 22, 2023

View reviewed changes

homeassistant/components/lyric/sensor.py Outdated Show resolved Hide resolved

joostlek marked this pull request as draft November 22, 2023 11:01

PR suggestions

f8695d2

- update docstring to reflect ownership by thermostat - fixed potential issue where a sensor would not be added if its temperature value was 0

dalinicus marked this pull request as ready for review November 22, 2023 14:59

t3hk0d3 reviewed Nov 23, 2023

View reviewed changes

Merge branch 'dev' into feature/lyric-room-sensor-accessories

a738c3d

dalinicus marked this pull request as draft January 27, 2024 21:46

github-actions bot added the stale label Mar 27, 2024

github-actions bot closed this Apr 3, 2024

joostlek reopened this Apr 3, 2024

github-actions bot removed the stale label Apr 4, 2024

dalinicus marked this pull request as ready for review April 4, 2024 15:09

Merge branch 'dev' into feature/lyric-room-sensor-accessories

a01e3fb

dalinicus marked this pull request as draft April 4, 2024 15:42

fix bad github merge

3e60f6a

dalinicus marked this pull request as ready for review April 4, 2024 16:31

emontnemery requested changes Apr 15, 2024

View reviewed changes

home-assistant bot marked this pull request as draft April 15, 2024 14:56

asyicio.gather futures for updating theromstat room stats

8f50450

dalinicus marked this pull request as ready for review April 18, 2024 16:12

home-assistant bot requested a review from emontnemery April 18, 2024 16:12

emontnemery approved these changes Apr 18, 2024

View reviewed changes

emontnemery merged commit 05c3764 into home-assistant:dev Apr 18, 2024
24 checks passed

github-actions bot locked and limited conversation to collaborators Apr 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for room sensor accessories assigned to a Honeywell (Lyric) Thermostat #104343

Add support for room sensor accessories assigned to a Honeywell (Lyric) Thermostat #104343

dalinicus commented Nov 22, 2023

home-assistant bot commented Nov 22, 2023

emontnemery Nov 22, 2023

dalinicus Nov 22, 2023

emontnemery Nov 22, 2023

dalinicus Nov 22, 2023

joostlek Nov 22, 2023

dalinicus Nov 22, 2023

dalinicus commented Nov 22, 2023

t3hk0d3 Nov 23, 2023

disforw commented Nov 24, 2023

dalinicus commented Nov 24, 2023 •

edited

disforw commented Nov 24, 2023

dalinicus commented Dec 17, 2023

github-actions bot commented Mar 27, 2024

joostlek commented Apr 3, 2024

disforw commented Apr 3, 2024

dalinicus commented Apr 4, 2024

emontnemery left a comment •

edited

emontnemery Apr 15, 2024

dalinicus Apr 18, 2024

home-assistant bot commented Apr 15, 2024

emontnemery left a comment

Add support for room sensor accessories assigned to a Honeywell (Lyric) Thermostat #104343

Add support for room sensor accessories assigned to a Honeywell (Lyric) Thermostat #104343

Conversation

dalinicus commented Nov 22, 2023

Breaking change

Proposed change

Type of change

Additional information

Checklist

home-assistant bot commented Nov 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dalinicus commented Nov 22, 2023

Choose a reason for hiding this comment

disforw commented Nov 24, 2023

dalinicus commented Nov 24, 2023 • edited

disforw commented Nov 24, 2023

dalinicus commented Dec 17, 2023

github-actions bot commented Mar 27, 2024

joostlek commented Apr 3, 2024

disforw commented Apr 3, 2024

dalinicus commented Apr 4, 2024

emontnemery left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

home-assistant bot commented Apr 15, 2024

emontnemery left a comment

Choose a reason for hiding this comment

dalinicus commented Nov 24, 2023 •

edited

emontnemery left a comment •

edited