Add Inovelli VZM36 support and rework manufacturer clusters #2960

codyhackw · 2024-01-31T23:59:37Z

Proposed change

This change adds support for the Inovelli VZM36 canopy module and re-organizes the __init__.py file to simplify the parameters across devices.

Additional information

I believe the ep_attribute has to stay pointed to the inovelli_vzm31sn_cluster as that's what's referenced within ZHA so changing that would result in a breaking change. In the interest of avoiding that, I've kept it in place.

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-02-01T00:05:06Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (97d2734) 87.68% compared to head (1db1cc0) 87.72%.
Report is 4 commits behind head on dev.

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #2960      +/-   ##
==========================================
+ Coverage   87.68%   87.72%   +0.04%     
==========================================
  Files         297      298       +1     
  Lines        9084     9119      +35     
==========================================
+ Hits         7965     8000      +35     
  Misses       1119     1119

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

TheJulianJES · 2024-02-02T01:20:12Z

zhaquirks/inovelli/__init__.py

-    cluster_id = 0xFC31
-    name = "InovelliVZM31SNCluster"
-    ep_attribute = "inovelli_vzm31sn_cluster"
+class Inovelli_Cluster(CustomCluster):


It's preferred to use "CamelCase" / "CapWords" for class names.

From what I can see, cluster_id and ep_attribute is the same for all Inovelli cluster classes.
Can we just put it in this base class then?

Re the class name, are you saying the preference would be to do "InovelliCluster" as opposed to "Inovelli_Cluster"? This was staying consistent to the existing naming, but can definitely change it if needed. Just have to change it in the device-specific imports as well.

Putting cluster_id and ep_attribute in the base class is giving async_initialize errors...it all appears to work, but putting them across each cluster resolved that and is why I ended up splitting them out.

saying the preference would be to do "InovelliCluster" as opposed to "Inovelli_Cluster"?

Yes. That's preferred.

Putting cluster_id and ep_attribute in the base class is giving async_initialize errors...it all appears to work, but putting them across each cluster resolved that and is why I ended up splitting them out.

Weird. There shouldn't be any difference. Are you sure it wasn't just some connectivity issue?

Updating all the classes, thanks for the clarification!

And yeah agreed it's weird, just re-tested. In 2024.2.0 it no longer shows up as a warning, but it's still in the logs -

2024-02-09 08:05:45.026 DEBUG (MainThread) [homeassistant.components.zha.core.cluster_handlers] [0x90CF:1:0xfc31]: failed to get attributes '['power_type', 'switch_type', 'button_delay', 'smart_bulb_mode', 'local_protection']' on 'inovelli_vzm31sn_cluster' cluster: 2024-02-09 08:05:45.027 DEBUG (MainThread) [homeassistant.components.zha.core.cluster_handlers] [0x90CF:1:0x0019]: initializing cluster handler: from_cache: False 2024-02-09 08:05:45.027 DEBUG (MainThread) [homeassistant.components.zha.core.cluster_handlers] [0x90CF:1:0x0019]: initializing cached cluster handler attributes: ['current_file_version'] 2024-02-09 08:05:45.027 DEBUG (MainThread) [homeassistant.components.zha.core.cluster_handlers] [0x90CF:1:0x0019]: Reading attributes in chunks: ['current_file_version'] 2024-02-09 08:05:45.027 DEBUG (MainThread) [homeassistant.components.zha.core.cluster_handlers] [0x90CF:1:0x0019]: finished cluster handler initialization 2024-02-09 08:05:45.082 DEBUG (MainThread) [homeassistant.components.zha.core.cluster_handlers] [0x90CF:1:0x0006]: 'async_initialize' stage succeeded 2024-02-09 08:05:45.082 DEBUG (MainThread) [homeassistant.components.zha.core.cluster_handlers] [0x90CF:1:0x0008]: 'async_initialize' stage succeeded 2024-02-09 08:05:45.082 DEBUG (MainThread) [homeassistant.components.zha.core.cluster_handlers] [0x90CF:1:0x0000]: 'async_initialize' stage succeeded 2024-02-09 08:05:45.082 DEBUG (MainThread) [homeassistant.components.zha.core.cluster_handlers] [0x90CF:1:0x0b04]: 'async_initialize' stage succeeded 2024-02-09 08:05:45.082 DEBUG (MainThread) [homeassistant.components.zha.core.cluster_handlers] [0x90CF:1:0x0003]: 'async_initialize' stage succeeded 2024-02-09 08:05:45.083 DEBUG (MainThread) [homeassistant.components.zha.core.cluster_handlers] [0x90CF:1:0x0702]: 'async_initialize' stage succeeded 2024-02-09 08:05:45.083 DEBUG (MainThread) [homeassistant.components.zha.core.cluster_handlers] [0x90CF:1:0x0019]: 'async_initialize' stage succeeded 2024-02-09 08:05:45.083 DEBUG (MainThread) [homeassistant.components.zha.core.cluster_handlers] [0x90CF:1:0xfc31]: 'async_initialize' stage failed:

The log level was changed indeed. Are there more (debug) log messages when zigpy is also set to debug level?
There's no reason I can see why this could happen. Setting cluster_id, name, and ep_attribute in the "base class" vs the one inheriting that doesn't make a difference.
Can you double-confirm that the error happens every time? And not every time when set in the base classes?

Also, did you check that all attributes set here are correct?
https://github.com/home-assistant/core/blob/567a179084e0226a45d1db01ffb5d4e2e37f3d8c/homeassistant/components/zha/core/cluster_handlers/manufacturerspecific.py#L249-L253
(There might be more info in zigpy debug logs)

I've had both zigpy and zhaquirks set to debug while trying to figure out what's going on and haven't seen anything relevant from zigpy unfortunately. Confirmed I end up with at least one Inovelli device giving that error every time. It's not all of them, and the devices do still seem to work, but I do get at least one log message in 100% of my tests but I don't when I have them set as in the current PR state. I'm not certain why that is because I'd expect it to be the same.

I did also double check the attributes, those all check out, I do find it interesting that the attributes that error are split between the base and one inheriting, but not sure if that's relevant?

Possibly related comment: #2960 (comment)

Updated to that and was still getting them, however when flipping back to how the PR is currently, I also replicated it once...so I'm just going to chalk it up to my network? Not entirely sure what's causing it, but if it happens in both scenarios it's likely unrelated. I'll move these to the base class and thanks for the ideas in trying to narrow it down.

TheJulianJES · 2024-02-02T01:22:04Z

zhaquirks/inovelli/__init__.py

-    name = "InovelliVZM31SNCluster"
-    ep_attribute = "inovelli_vzm31sn_cluster"
+class Inovelli_Cluster(CustomCluster):
+    """Inovelli base cluster."""

    attributes = {
        0x0001: ("dimming_speed_up_remote", t.uint8_t, True),


Not for this PR, but we need to switch to the new zigpy AttributeDefs style at some point.

Again, no need to do it in this PR already, but saving a code sample here for reference how it would look:

zha-device-handlers/zhaquirks/legrand/cable_outlet.py

Lines 42 to 59 in ab0d248

class LegrandCluster(CustomCluster):

"""LegrandCluster."""

cluster_id = MANUFACTURER_SPECIFIC_CLUSTER_ID

name = "LegrandCluster"

ep_attribute = "legrand_cluster"

class AttributeDefs(BaseAttributeDefs):

device_mode = ZCLAttributeDef(

id=0x0000,

type=t.data16, # DeviceMode

is_manufacturer_specific=True,

)

led_dark = ZCLAttributeDef(

id=0x0001,

type=t.Bool,

is_manufacturer_specific=True,

)

TheJulianJES · 2024-02-11T01:43:51Z

zhaquirks/inovelli/__init__.py

+    attributes = {
+        key: InovelliCluster.attributes[key] for key in InovelliCluster.attributes
+    }


Can you use:

attributes = InovelliCluster.attributes.copy()

Please also test if you still have the initialization error then.
... although there shouldn't be a difference I think. But still, use the .copy() one now that attributes no longer need to removed, as it's what we're using "everywhere" already.

TheJulianJES · 2024-02-11T02:53:50Z

zhaquirks/inovelli/__init__.py

+    name = "InovelliVZM31SNCluster"
+
+    attributes = InovelliCluster.attributes.copy()
+


Can you also remove this empty line between attributes = ... and attributes.update(... everywhere?
(To follow what other quirks do)

TheJulianJES · 2024-02-12T02:17:13Z

zhaquirks/inovelli/__init__.py

+AUX_ON = "Aux Up"
+AUX_OFF = "Aux Down"
+AUX_CONFIG = "Aux Config"


I think these should follow HA standards and be "Aux up", "Aux down", "Aux config"?

TheJulianJES

LGTM, thanks!

codyhackw added 2 commits January 31, 2024 18:50

Adding VZM36 Support and Inovelli cluster rework

c45fdc8

Typo fix for import on VZM36

aa5cf28

codyhackw added 3 commits January 31, 2024 19:07

Formatting Fixes

b227d3b

Update VZM36.py

85c593a

Update VZM36.py

7c90de5

TheJulianJES added the new quirk Adds support for a new device label Feb 1, 2024

TheJulianJES changed the title ~~Adding VZM36 Support and Inovelli cluster rework~~ Add Inovelli VZM36 support and rework manufacturer clusters Feb 1, 2024

Fixes for Aux Button Press Events

ad86504

TheJulianJES reviewed Feb 2, 2024

View reviewed changes

codyhackw added 2 commits February 9, 2024 08:13

Class Name Updates

9a0d679

Update __init__.py

18011d5

TheJulianJES reviewed Feb 11, 2024

View reviewed changes

codyhackw added 3 commits February 10, 2024 21:11

Update __init__.py

f299e80

Update __init__.py

cf00671

Update __init__.py

5674a17

TheJulianJES reviewed Feb 11, 2024

View reviewed changes

Update __init__.py

6ee2116

TheJulianJES reviewed Feb 12, 2024

View reviewed changes

Update __init__.py

1db1cc0

TheJulianJES approved these changes Feb 12, 2024

View reviewed changes

TheJulianJES merged commit 0df6f2b into zigpy:dev Feb 12, 2024
14 checks passed

codyhackw mentioned this pull request Mar 1, 2024

Updates for Inovelli fan switch and canopy module home-assistant/core#111913

Closed

20 tasks

lgraf pushed a commit to lgraf/zha-device-handlers that referenced this pull request May 6, 2024

Add Inovelli VZM36 support and rework manufacturer clusters (zigpy#2960)

c79074b

codyhackw deleted the Inovelli-VZM36 branch June 26, 2024 12:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Inovelli VZM36 support and rework manufacturer clusters #2960

Add Inovelli VZM36 support and rework manufacturer clusters #2960

codyhackw commented Jan 31, 2024 •

edited

Loading

codecov bot commented Feb 1, 2024 •

edited

Loading

TheJulianJES Feb 2, 2024

TheJulianJES Feb 2, 2024

codyhackw Feb 2, 2024

TheJulianJES Feb 9, 2024

codyhackw Feb 9, 2024

TheJulianJES Feb 10, 2024

codyhackw Feb 10, 2024

TheJulianJES Feb 11, 2024 •

edited

Loading

codyhackw Feb 11, 2024

TheJulianJES Feb 2, 2024

TheJulianJES Feb 9, 2024

TheJulianJES Feb 11, 2024 •

edited

Loading

TheJulianJES Feb 11, 2024

TheJulianJES Feb 12, 2024

TheJulianJES left a comment

	class LegrandCluster(CustomCluster):
	"""LegrandCluster."""

	cluster_id = MANUFACTURER_SPECIFIC_CLUSTER_ID
	name = "LegrandCluster"
	ep_attribute = "legrand_cluster"

	class AttributeDefs(BaseAttributeDefs):
	device_mode = ZCLAttributeDef(
	id=0x0000,
	type=t.data16, # DeviceMode
	is_manufacturer_specific=True,
	)
	led_dark = ZCLAttributeDef(
	id=0x0001,
	type=t.Bool,
	is_manufacturer_specific=True,
	)

		name = "InovelliVZM31SNCluster"

		attributes = InovelliCluster.attributes.copy()

Add Inovelli VZM36 support and rework manufacturer clusters #2960

Add Inovelli VZM36 support and rework manufacturer clusters #2960

Conversation

codyhackw commented Jan 31, 2024 • edited Loading

Proposed change

Additional information

Checklist

codecov bot commented Feb 1, 2024 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

TheJulianJES Feb 11, 2024 • edited Loading

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

TheJulianJES Feb 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheJulianJES left a comment

Choose a reason for hiding this comment

codyhackw commented Jan 31, 2024 •

edited

Loading

codecov bot commented Feb 1, 2024 •

edited

Loading

TheJulianJES Feb 11, 2024 •

edited

Loading

TheJulianJES Feb 11, 2024 •

edited

Loading