Add Niko Connectable switch support

[svenjochems]

Proposed change

Add support for Niko Connectable switch (Both Double and Single rocker switch)

Additional information

Based on input from #2286

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]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.90%. Comparing base (2a39c07) to head (ceffa00).
Report is 302 commits behind head on dev.

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #2723      +/-   ##
==========================================
+ Coverage   87.85%   87.90%   +0.05%     
==========================================
  Files         301      303       +2     
  Lines        9212     9255      +43     
==========================================
+ Hits         8093     8136      +43     
  Misses       1119     1119              

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

[svenjochems]

@TheJulianJES thanks for the review, I updated the PR based on your remarks. Do you happen to have some answers to my own questions in my review?

[TheJulianJES]

Do you happen to have some answers to my own questions in my review?

Hmm, maybe I'm missing something. But I do not see any comment/questions from you?
The changes seem to look good (from a quick review at least).

[svenjochems]

Do you happen to have some answers to my own questions in my review?

Hmm, maybe I'm missing something. But I do not see any comment/questions from you? The changes seem to look good (from a quick review at least).

Apparently my review comments where still pending. They should be visible now.

[svenjochems]

@TheJulianJES can you give this another review? I would like to have this PR merged as I have to install more of these devices in the near future :)

[TheJulianJES]

Ideally, we should switch all attributes to use the new-style zigpy AttributeDefs, similar to this:

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,
	)

[svenjochems]

@TheJulianJES
Do you have an other example of this?
I tried changing the attributes to

class AttributeDefs(BaseAttributeDefs):
    button_operation_mode = ZCLAttributeDef(
        id=0x0000,
        type=t.enum8,
        is_manufacturer_specific=True,
    )
    led_state = ZCLAttributeDef(
        id=0x0100,
        type=t.uint8_t,
        is_manufacturer_specific=True,
    )
    led_operation_mode = ZCLAttributeDef(
        id=0x0104,
        type=t.enum8,
        is_manufacturer_specific=True,
    )

But then I get enum8.undefined_0x01 when reading the attribute in homeassistant ui.

Changing type to ButtonOperationMode does not seem to work either.

[TheJulianJES]

type = ButtonOperationMode (and so on) would be correct and should work.
There are more examples in the zigpy project in the zcl folder where all ZCL clusters are defined.

[svenjochems]

@TheJulianJES When defining the ButtonOperationMode class outside the NikoConfigCluster class and adding ButtonOperationMode: Final = ButtonOperationMode in the NikoConfigCluster, it works.

[TheJulianJES]

Hmm, I'm not sure we can/should extend t.uint8_t like this (and "abuse" it as an enum). For example, it might break if it's used as a select config entity in HA(?), as it's not really an enum.
(But for that, it would be preferred to have an on/off switch anyway.)

For now, wrap it in an t.enum_factory like in the first example below. It makes it a proper enum whilst keeping the "unsigned integer" "Zigbee type". Still not optimal though – it should be handled better in the future.

Actually, leave the enum for now, but have it extend t.enum8 (to be used for a future config entity).
In the actual attribute definition, use just the raw t.unit8_t type. Do not use LedState there.

I do see it's already done somewhat similar in other places though:

• zha-device-handlers/zhaquirks/xiaomi/aqara/tvoc.py

  Lines 70 to 76 in 0df6f2b

  	class TVOCDisplayUnit(t.enum_factory(t.uint8_t)):
  	"""Display values."""
  	mgm3_celsius = 0x00
  	ppb_celsius = 0x01
  	mgm3_fahrenheit = 0x10
  	ppb_fahrenheit = 0x11

  (^ this is an interesting way to do it, as it avoids zigpy setting the type to "enum" instead of "int" here. Seems to be somewhat of a hack though.)

• zha-device-handlers/zhaquirks/xiaomi/aqara/opple_switch.py

  Lines 63 to 67 in 0df6f2b

  	class OppleOperationMode(t.uint8_t, Enum):
  	"""Opple operation_mode enum."""
  	Decoupled = 0x00
  	Coupled = 0x01

  (^ also interesting, but IMO less good)

• zha-device-handlers/zhaquirks/sinope/thermostat.py

  Lines 65 to 69 in 0df6f2b

  	class PumpStatus(t.uint8_t):
  	"""Pump protection status."""
  	Off = 0x00
  	On = 0x01

  (^ not an Enum at all, should probably be changed)

The way we can implement this might slightly change with quirks v2.
With quirks v2, we should be able to define a custom select entity for this. I'm pretty sure that would also need this class to be a "proper enum" (e.g. with the t.enum_factory)
(IMO, an on/off switch would be better, but I think the on/off values are hard-coded to 0x0 / 0x1 right now?)

[svenjochems]

Today I did some more testing. ButtonOperation works successfully but LedOperationMode does not seem to make any changes on the device.
@arvidc were you able to test this in your tests from #2286?
Any pointer how I can debug this?

[TheJulianJES]

So the attribute isn't written correctly? Writing 0 or 1 doesn't work?
If so, did you try setting the type for the ZCLAttributeDef to t.uint8_t and see if it works then?

[arvidc]

Hey @svenjochems , I've been using it through a custom quirk on my end and it seems to work (apart from the potentially unintuitive behaviour described in the comments; which makes me think LedOperationMode is perhaps better called LedEnabled with states Disabled and Enabled, but that's beyond this).
More specifically, I wrote LedOperationMode to 0 for most/all(?) of my switches, and actively control LedState on one to reflect our terrace light.

I do have them as:

   class LedState(t.uint24_t):
        Off = 0
        On = 255

    class LedOperationMode(t.uint8_t):
        Decoupled = 0
        ControlledByRelay = 1

and I can't succesfully write (and it doesn't read like that either) e.g. LedOperationMode.Decoupled (in analogy to ButtonOperationMode), but only 0 or 1. Maybe because this isn't a proper enum, as I believe @TheJulianJES was pointing out earlier?

(Sorry, I barely know what an enum is, let alone a t.uint24_t. So pretty much all I do is copy and adapt pieces from somewhere and trial-and-error until I think it works well enough for me.)

[TheJulianJES]

Maybe because this isn't a proper enum

Yes, the "Zigbee type" is just an integer. Not an enum. If the zigpy enum is used, zigpy also sends it as a Zigbee enum which we do not want in this case. This might be changed in the future.

For now, just set the type for the ZCLAttributeDef to t.uint8_t (to force zigpy to send it as a Zigbee int).
We can keep the enum for future-use (with quirks v2 for exposing the entity or when this is fixed. But that's what required for now.
Just inheriting t.uint8_t in a class "works", but doesn't really do anything different from just using t.uint8_t when other class attributes are added.

[github-actions]

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. Thank you for your contributions.

[github-actions]

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. Thank you for your contributions.