Convert Sinope light to new style AttributeDefs, add ZHA events #3313

ckm2k1 · 2024-08-23T22:18:21Z

Proposed change

Add ability for Sinope light switches to handle routing attribute reports on the manufacturer cluster attribute action_report as ZHA events.

Additional information

Related PR zigpy/zha/pull/153 made it so that action_report doesn't need to be manually configured for reporting (as shown by @claudegel here: https://github.com/claudegel/sinope-zha?tab=readme-ov-file#a-create-the-reporting-).

Checklist

The changes are tested and work correctly
pre-commit checks pass / the code has been formatted using Black
Tests have been added to verify that the new code works

codecov · 2024-08-23T22:22:28Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.72%. Comparing base (acecf70) to head (c8993f4).
Report is 13 commits behind head on dev.

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #3313      +/-   ##
==========================================
+ Coverage   88.49%   88.72%   +0.23%     
==========================================
  Files         305      306       +1     
  Lines        9621     9811     +190     
==========================================
+ Hits         8514     8705     +191     
+ Misses       1107     1106       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

tests/test_sinope.py

zhaquirks/sinope/light.py

claudegel · 2024-08-31T16:37:53Z

So we will need to do the same in sinope thermostat.py, sensor.py and switch.py
Can I help to reformat some of them ?

claudegel · 2024-08-31T21:11:07Z

Ok I'm almost finish with the changes for thermostat.py, switch.py and sensor.py.
This is clear for the manufacturer cluster but in those files there are attribute addition in other cluster like Thermostat cluster

    attributes = Thermostat.attributes.copy()
    attributes.update(
        {
            0x0400: ("set_occupancy", Occupancy, True),
            0x0401: ("main_cycle_output", CycleOutput, True),
            ...

How do you change the AttributeDefs

    class AttributeDefs(foundation.BaseAttributeDefs):
        """Sinope Thermostat Custom Cluster Attributes."""

        set_occupancy: Final = foundation.ZCLAttributeDef(
            id=0x0400, type=Occupancy, access="rw", is_manufacturer_specific=True
        )
        ...
        )

        attributes = Thermostat.attributes.copy()
        attributes.update(
        {
        set_occupancy
        ...
        }

ckm2k1 · 2024-09-01T01:58:26Z

@claudegel I don't know for sure, but my guess would be that you can probably inherit from the ZHA thermostat cluster attribute defs, something like this:

class AttributeDefs(Thermostat.AttributeDefs):
 ...

Btw, thank you for all the work you've done on supporting Sinope devices in ZHA!

claudegel · 2024-09-01T02:42:15Z

Thanks I'll push a PR tomorrow for thermostat, switch and sensor

claudegel · 2024-09-01T16:56:29Z

I've tried this but the attribute is not added to IasZone cluster:

class SinopeTechnologiesIasZoneCluster(CustomCluster, IasZone):
    """SinopeTechnologiesIasZoneCluster custom cluster."""

    LeakStatus: Final = LeakStatus

    class AttributeDefs(IasZone.AttributeDefs):
        """Sinope Manufacturer IasZone Cluster Attributes."""

    leak_status: Final = foundation.ZCLAttributeDef(
        id=0x0030, type=LeakStatus, access="rw", is_manufacturer_specific=True
    )

Any idea ?

claudegel · 2024-09-01T18:48:16Z

Was able to fix like this:
Attribute is added to IasZone cluster but I still have an error

    leak_status: Final = foundation.BaseAttributeDefs(
                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: BaseAttributeDefs() takes no arguments

    class AttributeDefs(IasZone.AttributeDefs):
        """Sinope Manufacturer IasZone Cluster Attributes."""

    leak_status: Final = foundation.BaseAttributeDefs(
        id=0x0030, type=LeakStatus, access="rw", is_manufacturer_specific=True
    )

If I replace foundation.BaseAttributeDefs by foundation.ZCLAttributeDef it is not working and atribute leak_status is not added
I've another strange thing. In the SinopeManufacturerCluster I have some Unknown_attr_1... attributes that I'm still working on . I need them to appear in the cluster but with this new style AttributeDefs they are just skipped. All other named attributes are there but the unknown one are missing. Is it because there is a number in them ?

claudegel · 2024-09-02T00:38:11Z

I got it ... a stupid indentation that was not noticed in the log
ready for a PR

ckm2k1 · 2024-09-03T11:55:19Z

@TheJulianJES - Anything missing in this PR to merge up?

claudegel · 2024-09-03T12:31:22Z

Maybe change line

cluster_revision: Final = foundation.ZCLAttributeDef(
            id=0xFFFD, type=t.uint16_t, access="r", is_manufacturer_specific=True
        )

by
cluster_revision: Final = foundation.ZCL_CLUSTER_REVISION_ATTR

ckm2k1 · 2024-09-03T12:49:42Z

Maybe change line
cluster_revision: Final = foundation.ZCLAttributeDef(
            id=0xFFFD, type=t.uint16_t, access="r", is_manufacturer_specific=True
        )
by cluster_revision: Final = foundation.ZCL_CLUSTER_REVISION_ATTR

Good point, changed it.

ckm2k1 · 2024-09-04T12:46:50Z

@TheJulianJES - another bump, anything else needed to merge this?
Review feedback is addressed, all tests are passing.

TheJulianJES · 2024-09-04T13:43:13Z

There's no need to ping me. It'll be looked at when I (or someone else) has time.
The next zha-quirks release will only be at the end of the month anyway, as they're always made a couple of days before the next major HA beta is released.

claudegel · 2024-09-04T13:44:18Z

Look good. Thank you

TheJulianJES

AttributeDefs changes look good. The action_report stuff seems fine as well, I think.

tests/test_sinope.py

TheJulianJES · 2024-09-24T19:38:27Z

tests/test_sinope.py

+
+
+@pytest.mark.parametrize("quirk", (SinopeTechnologieslight,))
+async def test_sinope_light_switch_reporting(zigpy_device_from_quirk, quirk):


Tbh, I'm not sure we really need this test in zha quirks? We're not really testing any quirk specific code here, right?
(maybe except that the attribute exists)

We can leave this in for now though.

TheJulianJES · 2024-09-24T19:40:06Z

zhaquirks/sinope/light.py

+        ] = None,
+    ):
+        """Handle the cluster command."""
+        self.debug(


We can leave it like this for now, but this causes a zigpy log. We might want to use our own logger here, since that'll clearly be zhaquirks.sinope. Other quirks aren't consistent about this though.
The string does include "Sinope" at least, so that's good.

tests/test_sinope.py

TheJulianJES · 2024-09-24T19:43:52Z

zhaquirks/sinope/__init__.py

@@ -23,7 +23,7 @@

 SINOPE = "Sinope Technologies"
 SINOPE_MANUFACTURER_CLUSTER_ID = 0xFF01
-ATTRIBUTE_ACTION = "actionReport"
+ATTRIBUTE_ACTION = "action_report"


Changing this might cause issues with current automations that do not use device triggers, right?

TheJulianJES

Thanks, let's try this.

TheJulianJES reviewed Aug 27, 2024

View reviewed changes

tests/test_sinope.py Outdated Show resolved Hide resolved

TheJulianJES reviewed Aug 27, 2024

View reviewed changes

zhaquirks/sinope/light.py Outdated Show resolved Hide resolved

claudegel mentioned this pull request Sep 2, 2024

Convert Sinope files to new style AttributeDefs #3342

Merged

3 tasks

ckm2k1 force-pushed the sinope-light-action-report branch from fe60a43 to 0e3e816 Compare September 3, 2024 12:49

ckm2k1 force-pushed the sinope-light-action-report branch from 0e3e816 to 7086191 Compare September 13, 2024 23:15

Steve Venzerul and others added 11 commits September 13, 2024 19:15

wip

c9efede

cleanup

e794ab0

missing cluster_id

ca9ad45

remove debug

9411920

everything working!

adb9414

fix test

8fde985

improve tests

ed32119

test single on/off and broken actions

77b814e

rename per review

e191169

clean up namespace refs

93a6276

convert to new style AttributeDefs

316a8a4

ckm2k1 added 3 commits September 13, 2024 19:15

use attrdefs

ba37c80

use attrdefs

94e04d8

cluster rev

d1a0677

ckm2k1 force-pushed the sinope-light-action-report branch from 7086191 to d1a0677 Compare September 13, 2024 23:15

TheJulianJES added the needs review This PR should be reviewed soon label Sep 16, 2024

TheJulianJES changed the title ~~Sinope light switches action report~~ Convert Sinope light to new style AttributeDefs, add ZHA events Sep 24, 2024

TheJulianJES reviewed Sep 24, 2024

View reviewed changes

Access action_report id via AttributeDefs

ed2380c

TheJulianJES reviewed Sep 24, 2024

View reviewed changes

tests/test_sinope.py Outdated Show resolved Hide resolved

Add back missing comma

c8993f4

TheJulianJES reviewed Sep 24, 2024

View reviewed changes

TheJulianJES approved these changes Sep 24, 2024

View reviewed changes

TheJulianJES merged commit 2bdd076 into zigpy:dev Sep 24, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert Sinope light to new style AttributeDefs, add ZHA events #3313

Convert Sinope light to new style AttributeDefs, add ZHA events #3313

ckm2k1 commented Aug 23, 2024 •

edited

Loading

codecov bot commented Aug 23, 2024 •

edited

Loading

claudegel commented Aug 31, 2024

claudegel commented Aug 31, 2024

ckm2k1 commented Sep 1, 2024

claudegel commented Sep 1, 2024

claudegel commented Sep 1, 2024

claudegel commented Sep 1, 2024

claudegel commented Sep 2, 2024

ckm2k1 commented Sep 3, 2024

claudegel commented Sep 3, 2024 •

edited

Loading

ckm2k1 commented Sep 3, 2024

ckm2k1 commented Sep 4, 2024

TheJulianJES commented Sep 4, 2024

claudegel commented Sep 4, 2024

TheJulianJES left a comment

TheJulianJES Sep 24, 2024

TheJulianJES Sep 24, 2024

TheJulianJES Sep 24, 2024

TheJulianJES left a comment



		@pytest.mark.parametrize("quirk", (SinopeTechnologieslight,))
		async def test_sinope_light_switch_reporting(zigpy_device_from_quirk, quirk):

Convert Sinope light to new style AttributeDefs, add ZHA events #3313

Convert Sinope light to new style AttributeDefs, add ZHA events #3313

Conversation

ckm2k1 commented Aug 23, 2024 • edited Loading

Proposed change

Additional information

Checklist

codecov bot commented Aug 23, 2024 • edited Loading

Codecov Report

claudegel commented Aug 31, 2024

claudegel commented Aug 31, 2024

ckm2k1 commented Sep 1, 2024

claudegel commented Sep 1, 2024

claudegel commented Sep 1, 2024

claudegel commented Sep 1, 2024

claudegel commented Sep 2, 2024

ckm2k1 commented Sep 3, 2024

claudegel commented Sep 3, 2024 • edited Loading

ckm2k1 commented Sep 3, 2024

ckm2k1 commented Sep 4, 2024

TheJulianJES commented Sep 4, 2024

claudegel commented Sep 4, 2024

TheJulianJES left a comment

Choose a reason for hiding this comment

TheJulianJES Sep 24, 2024

Choose a reason for hiding this comment

TheJulianJES Sep 24, 2024

Choose a reason for hiding this comment

TheJulianJES Sep 24, 2024

Choose a reason for hiding this comment

TheJulianJES left a comment

Choose a reason for hiding this comment

ckm2k1 commented Aug 23, 2024 •

edited

Loading

codecov bot commented Aug 23, 2024 •

edited

Loading

claudegel commented Sep 3, 2024 •

edited

Loading