Add modern configuration options for NodOn SIN-4-1-20, SIN-4-1-21, SIN-4-2-20, and alike #3568

ikruglov · 2024-11-29T22:23:52Z

Proposed change

This PR adds the following configuration options for NodOn switches:

SIN-4-1-20, SIN-4-1-21, SIN-4-1-20_PRO

switch_type (Bistable, Monostable, AutoDetect)
This setting is only available with firmware 3.4.0 and older. It allows one to manually configure SwitchType. In principle, NodOn switches auto-detect Toggle vs. Momentary mode. However, the auto-detection mechanism is not always reliable, and this option allows one to override it.
impulse_mode_duration
These switches have pulse mode, i.e., they turn off after a configured number of milliseconds. This setting allows one to configure it. 0 (the default value) means deactivation of the impulse mode.

SIN-4-2-20

switch type (Bistable, Monostable, AutoDetect)
Same as above. For both endpoints.

Additional information

The logic is a port of https://github.com/Koenkk/zigbee-herdsman-converters/blob/master/src/devices/nodon.ts#L100

This is how it looks in UI:

All UI entities are initially disabled.

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-11-29T22:35:00Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.81%. Comparing base (97413e3) to head (3b536c2).

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #3568      +/-   ##
==========================================
+ Coverage   89.79%   89.81%   +0.01%     
==========================================
  Files         323      323              
  Lines       10414    10434      +20     
==========================================
+ Hits         9351     9371      +20     
  Misses       1063     1063

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

ikruglov · 2024-11-29T22:40:23Z

One TODO for this PR: switch_type is available only after 3.4.0 firmware. I was thinking about using filter() from the QuirkBuilder. However, I needed help finding a reliable way to detect the current firmware version. The one that zha/zigpy reports doesn't match the real firmware version. If you know how to address this, please let me know.

TheJulianJES · 2024-11-29T22:57:50Z

ZHA should try and read the attribute during pairing. If it's returned as unsupported, it shouldn't create the select entity.
Do you have a device with the older firmware to verify?

ikruglov · 2024-11-29T23:03:30Z

ZHA should try and read the attribute during pairing. If it's returned as unsupported, it shouldn't create the select entity. Do you have a device with the older firmware to verify?

Yes, I still have a few. The entity can be updated without any extra code, but an error occurs during the update: Status.UNSUPPORTED_ATTRIBUTE: 134. This is not good behavior cause it will cause questions.

TheJulianJES · 2024-11-29T23:18:21Z

Hmm, when ZHA reads the attribute during pairing, we should get "unsupported attribute" back. It will cause that attribute to be added to the "unsupported attributes" list for that device. ZHA should not create the entity then:
https://github.com/zigpy/zha/blob/6737b3628f1166af1ac90dc39019a402024dc331/zha/application/platforms/select.py#L190

Can you try and restart HA to see if the entity disappears then? Also, when you download the diagnostics file for that device (from the device page -> three dots), is switch_type listed in the "unsupported attributes" section in the JSON?

ikruglov · 2024-11-30T07:54:17Z

@TheJulianJES, it's weird. switch_type is present is both attributes and unsupported attributes

          "0x0006": {
            "endpoint_attribute": "on_off",
            "attributes": {
              "0xfffd": {
                "attribute": "ZCLAttributeDef(id=0xFFFD, name='cluster_revision', type=<class 'zigpy.types.basic.uint16_t'>, zcl_type=<DataTypeId.uint16: 33>, access=<ZCLAttributeAccess.Read: 1>, mandatory=True, is_manufacturer_specific=False)",
                "value": null
              },
              "0x4000": {
                "attribute": "ZCLAttributeDef(id=0x4000, name='global_scene_control', type=<enum 'Bool'>, zcl_type=<DataTypeId.bool_: 16>, access=<ZCLAttributeAccess.Read: 1>, mandatory=False, is_manufacturer_specific=False)",
                "value": null
              },
              "0x4002": {
                "attribute": "ZCLAttributeDef(id=0x4002, name='off_wait_time', type=<class 'zigpy.types.basic.uint16_t'>, zcl_type=<DataTypeId.uint16: 33>, access=<ZCLAttributeAccess.Read|Write: 3>, mandatory=False, is_manufacturer_specific=False)",
                "value": null
              },
              "0x0000": {
                "attribute": "ZCLAttributeDef(id=0x0000, name='on_off', type=<enum 'Bool'>, zcl_type=<DataTypeId.bool_: 16>, access=<ZCLAttributeAccess.Read|Report|Scene: 25>, mandatory=True, is_manufacturer_specific=False)",
                "value": 0
              },
              "0x4001": {
                "attribute": "ZCLAttributeDef(id=0x4001, name='on_time', type=<class 'zigpy.types.basic.uint16_t'>, zcl_type=<DataTypeId.uint16: 33>, access=<ZCLAttributeAccess.Read|Write: 3>, mandatory=False, is_manufacturer_specific=False)",
                "value": null
              },
              "0xfffe": {
                "attribute": "ZCLAttributeDef(id=0xFFFE, name='reporting_status', type=<enum 'AttributeReportingStatus'>, zcl_type=<DataTypeId.enum8: 48>, access=<ZCLAttributeAccess.Read: 1>, mandatory=False, is_manufacturer_specific=False)",
                "value": null
              },
              "0x4003": {
                "attribute": "ZCLAttributeDef(id=0x4003, name='start_up_on_off', type=<enum 'StartUpOnOff'>, zcl_type=<DataTypeId.enum8: 48>, access=<ZCLAttributeAccess.Read|Write: 3>, mandatory=False, is_manufacturer_specific=False)",
                "value": 255
              },
              "0x1001": {
                "attribute": "ZCLAttributeDef(id=0x1001, name='switch_type', type=<enum 'NodOnSwitchType'>, zcl_type=<DataTypeId.enum8: 48>, access=<ZCLAttributeAccess.Read|Write|Report: 11>, mandatory=False, is_manufacturer_specific=True)",
                "value": null
              }
            },
            "unsupported_attributes": [
              4097,
              "switch_type"
            ]

ikruglov · 2024-11-30T08:30:59Z

~~@TheJulianJES I've done a bit of debugging. This code never executes for switch_type attribute:~~ https://github.com/zigpy/zha/blob/6737b3628f1166af1ac90dc39019a402024dc331/zha/application/platforms/select.py#L190

No wait. I think I missed something.

ikruglov · 2024-11-30T08:57:21Z

okay, I got why the code is not executed for switch type.

the if condition is:

        if ENTITY_METADATA not in kwargs and (
            cls._attribute_name in cluster_handler.cluster.unsupported_attributes
            or cls._attribute_name not in cluster_handler.cluster.attributes_by_name
            or cluster_handler.cluster.get(cls._attribute_name) is None
        ):

ENTITY_METADATA not in kwargs part is not true for switch_type because:

kwargs={'entity_metadata': ZCLEnumMetadata(entity_platform=<EntityPlatform.SELECT: 'select'>, entity_type=<EntityType.CONF
IG: 'config'>, cluster_id=6, endpoint_id=1, cluster_type=<ClusterType.Server: 0>, initially_disabled=True, attribute_initialized_from_cache=True, translation_key='switch_type', fallback_name='Switch type', en
um=<enum 'NodOnSwitchType'>, attribute_name='switch_type', reporting_config=None)}

Looks like either a feature of ZHA, or a bug :-)

ikruglov · 2024-11-30T09:38:21Z

This small patch makes it work as expected. I'm not sure if this is the right approach, though

$ git diff
diff --git a/zha/application/platforms/select.py b/zha/application/platforms/select.py
index 1012966..4f42dd2 100644
--- a/zha/application/platforms/select.py
+++ b/zha/application/platforms/select.py
@@ -185,15 +185,22 @@ class ZCLEnumSelectEntity(PlatformEntity):

         Return entity if it is a supported configuration, otherwise return None
         """
+
+        if ENTITY_METADATA in kwargs:
+            entity_metadata = kwargs.get(ENTITY_METADATA)
+            attr_name = entity_metadata.attribute_name if hasattr(entity_metadata, 'attribute_name') else None
+        else:
+            attr_name = cls._attribute_name
+
         cluster_handler = cluster_handlers[0]
-        if ENTITY_METADATA not in kwargs and (
-            cls._attribute_name in cluster_handler.cluster.unsupported_attributes
-            or cls._attribute_name not in cluster_handler.cluster.attributes_by_name
-            or cluster_handler.cluster.get(cls._attribute_name) is None
+        if attr_name is not None and (
+            attr_name in cluster_handler.cluster.unsupported_attributes
+            or attr_name not in cluster_handler.cluster.attributes_by_name
+            or cluster_handler.cluster.get(attr_name) is None
         ):
             _LOGGER.debug(
                 "%s is not supported - skipping %s entity creation",
-                cls._attribute_name,
+                attr_name,
                 cls.__name__,
             )
             return None

ikruglov · 2024-11-30T21:09:54Z

FTR: Firmware images I used are from Koenkk/zigbee-OTA#599. NodOn's CTO submits them.

TheJulianJES · 2024-12-01T22:42:39Z

ENTITY_METADATA not in kwargs part is not true for switch_type

Ah, of course! So we always "force create" all entities defined via quirks v2. I think the original intention was to skip this part:
or cluster_handler.cluster.get(cls._attribute_name) is None (and/or because the implementation/patch is a bit awkward otherwise because of the ENTITY_METADATA).
There, we check that there's at least a not None value in the attribute cache. That might make sense, but if the attributes are marked as "unsupported" or do not even have an attribute definition, I think we should also skip these.

Yeah, well, I guess we should change this for all platforms.. I'll try to have another look at this in the next few days. We might need to adopt something similar to your patch. I think we could compact it a bit though.

ikruglov · 2024-12-02T14:26:50Z

@TheJulianJES I'd be happy to prepare a PR for all platforms. As long as the direction I initially took is the right one. Please let me know.

I've also created zigpy/zigpy#1515 to track the problem

ikruglov and others added 3 commits November 29, 2024 12:37

add impulse_mode_duration quirk to NodOn SIN-4-1-20

985210f

add switch type quirk to NodOn SIN-4-1-20 and alike

62413c0

Apply pre-commit auto fixes

48fa361

ikruglov changed the title ~~Add modern configuration option to NodOn SIN-4-1-20, SIN-4-1-21, SIN-4-2-20, and alike~~ Add modern configuration options to NodOn SIN-4-1-20, SIN-4-1-21, SIN-4-2-20, and alike Nov 29, 2024

ikruglov added 3 commits November 29, 2024 23:25

fix comments

664aa78

make CI happy

e5ed1a1

fix UnitOfTime dependency

b33e98d

TheJulianJES added needs review This PR should be reviewed soon, as it generally looks good. v2 quirk Quirks using v2 API. Might add custom entities that need translation keys in HA. labels Nov 29, 2024

ikruglov changed the title ~~Add modern configuration options to NodOn SIN-4-1-20, SIN-4-1-21, SIN-4-2-20, and alike~~ Add modern configuration options for NodOn SIN-4-1-20, SIN-4-1-21, SIN-4-2-20, and alike Nov 30, 2024

access=rw

3b536c2

TheJulianJES added the needs reviewer answer An answer from a reviewer is needed (e.g. why a PR isn't acceptable in the current state). label Dec 1, 2024

ikruglov mentioned this pull request Dec 2, 2024

quirk v2 entities are force-creates even if they are not supported zigpy/zigpy#1515

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add modern configuration options for NodOn SIN-4-1-20, SIN-4-1-21, SIN-4-2-20, and alike #3568

Add modern configuration options for NodOn SIN-4-1-20, SIN-4-1-21, SIN-4-2-20, and alike #3568

ikruglov commented Nov 29, 2024 •

edited

Loading

codecov bot commented Nov 29, 2024 •

edited

Loading

ikruglov commented Nov 29, 2024

TheJulianJES commented Nov 29, 2024

ikruglov commented Nov 29, 2024

TheJulianJES commented Nov 29, 2024

ikruglov commented Nov 30, 2024

ikruglov commented Nov 30, 2024 •

edited

Loading

ikruglov commented Nov 30, 2024 •

edited

Loading

ikruglov commented Nov 30, 2024 •

edited by TheJulianJES

Loading

ikruglov commented Nov 30, 2024

TheJulianJES commented Dec 1, 2024

ikruglov commented Dec 2, 2024

Add modern configuration options for NodOn SIN-4-1-20, SIN-4-1-21, SIN-4-2-20, and alike #3568

Are you sure you want to change the base?

Add modern configuration options for NodOn SIN-4-1-20, SIN-4-1-21, SIN-4-2-20, and alike #3568

Conversation

ikruglov commented Nov 29, 2024 • edited Loading

Proposed change

Additional information

Checklist

codecov bot commented Nov 29, 2024 • edited Loading

Codecov Report

ikruglov commented Nov 29, 2024

TheJulianJES commented Nov 29, 2024

ikruglov commented Nov 29, 2024

TheJulianJES commented Nov 29, 2024

ikruglov commented Nov 30, 2024

ikruglov commented Nov 30, 2024 • edited Loading

ikruglov commented Nov 30, 2024 • edited Loading

ikruglov commented Nov 30, 2024 • edited by TheJulianJES Loading

ikruglov commented Nov 30, 2024

TheJulianJES commented Dec 1, 2024

ikruglov commented Dec 2, 2024

ikruglov commented Nov 29, 2024 •

edited

Loading

codecov bot commented Nov 29, 2024 •

edited

Loading

ikruglov commented Nov 30, 2024 •

edited

Loading

ikruglov commented Nov 30, 2024 •

edited

Loading

ikruglov commented Nov 30, 2024 •

edited by TheJulianJES

Loading