Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add quirk for Aqara T2 relay #2821

Merged
merged 6 commits into from
Dec 27, 2023
Merged

Add quirk for Aqara T2 relay #2821

merged 6 commits into from
Dec 27, 2023

Conversation

dmulcahey
Copy link
Collaborator

@dmulcahey dmulcahey commented Dec 9, 2023

Proposed change

Adds a quirk for the Aqara T2 relay. Fixes #2793

TODO:

  • add config entities to ZHA

Additional information

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

Copy link

codecov bot commented Dec 9, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (aeca76d) 87.35% compared to head (e180bae) 87.49%.
Report is 20 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #2821      +/-   ##
==========================================
+ Coverage   87.35%   87.49%   +0.14%     
==========================================
  Files         287      291       +4     
  Lines        8835     8934      +99     
==========================================
+ Hits         7718     7817      +99     
  Misses       1117     1117              

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

Scenes.cluster_id,
Groups.cluster_id,
OnOff.cluster_id,
MultistateInput.cluster_id,
Copy link
Collaborator

@TheJulianJES TheJulianJES Dec 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think most Aqara devices use the custom MultistateInputCluster cluster from opple_remote, but I'm not sure we need it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have one of these to test.... I wonder if this is for events if the switch is in decoupled mode? If so we would probably need a custom one?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It disables setting up attribute reporting and sends ZHA events depending on an attribute update.
So yeah, seems to be for decoupled mode.

From a quick look, Z2M seems to do something with that cluster: https://github.com/Koenkk/zigbee-herdsman-converters/blob/ab11b8a796bab2620e233e5a7a0b92506c6d6639/src/lib/xiaomi.ts#L1378-L1383

Should be fine to merge this as-is of course. If it's needed in the future, it can be added (or adapted if the T2 relay works differently).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Scenes.cluster_id,
Groups.cluster_id,
OnOff.cluster_id,
MultistateInput.cluster_id,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as comment above with MultistateInputCluster from opple_remote

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see above

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@TheJulianJES TheJulianJES added the new quirk Adds support for a new device label Dec 9, 2023
@pilot51
Copy link

pilot51 commented Dec 9, 2023

Interlock confirmed working!

The only attribute that appears to be used in OppleCluster endpoint 2 is decoupled_mode which is DecoupledMode.ControlRelay by default in both endpoints. Everything else in endpoint 2 is None and everything in endpoint 1 is working.

@dmulcahey dmulcahey marked this pull request as ready for review December 13, 2023 14:35
@rbrune
Copy link

rbrune commented Dec 19, 2023

I've tested the quirk with my Aqara T2 relay and sadly the fancy enums (for switch type) won't work. Reading the attributes is great as one gets a human readable output but writing an attribute back always fails. When switching back to simple t.uint8_t writing 2 for e.g. switch type worked.

@TheJulianJES
Copy link
Collaborator

Try writing 1, 2, or 3.

@rbrune
Copy link

rbrune commented Dec 19, 2023

Try writing 1, 2, or 3.

That is what I did. Remove SwitchType.Toggle and wrote a 2 instead. Pushed write attribute - doesn't work.
But changing the quirk to use t.uint8_t instead of the enum8 makes writing a 2 work.

@TheJulianJES
Copy link
Collaborator

But changing the quirk to use t.uint8_t instead of the enum8 makes writing a 2 work.

Ah, I see what you mean now.

When you change the quirk to use t.uint8_t, did you remove the whole enum class and change the type for the attribute directly or did you change the class to inherit from t.uint8_t instead of t.enum8? If not, can you try that (replace t.enum8 with t.uint8_t for all class definitions)?

If that also doesn't work, does something like class (t.uint8_t, Enum): work?

@TheJulianJES TheJulianJES added the Xiaomi Request/PR regarding a Xiaomi device label Dec 24, 2023
@dmulcahey
Copy link
Collaborator Author

gonna change the type here... I remember what to do now. Use the correct type in the quirk and the enum in HA config entity.

@TheJulianJES
Copy link
Collaborator

TheJulianJES commented Dec 27, 2023

Yeah, same for the P1 door sensor PR then probably.

@TheJulianJES
Copy link
Collaborator

TheJulianJES commented Dec 27, 2023

We do have some places where we use this combination of inheriting (t.uint8_t, Enum):

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

Although I'm not sure if that's a good solution.

Current changes regarding types look good. I'd keep them like this (for now).

tests/conftest.py Outdated Show resolved Hide resolved
@dmulcahey
Copy link
Collaborator Author

We do have some places where we use this combination of inheriting (t.uint8_t, Enum):

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

Although I'm not sure if that's a good solution.

Current changes regarding types look good. I'd keep them like this (for now).

I don't think this will work either? we need to send the devices that are checking types the correct type.

@TheJulianJES
Copy link
Collaborator

I don't think this will work either? we need to send the devices that are checking types the correct type.

Haven't tested, but t.uint8_t should be the correct type. There's this discussion about it: https://github.com/zigpy/zha-device-handlers/pull/1391/files#r956576698

@dmulcahey
Copy link
Collaborator Author

I don't think this will work either? we need to send the devices that are checking types the correct type.

Haven't tested, but t.uint8_t should be the correct type. There's this discussion about it: https://github.com/zigpy/zha-device-handlers/pull/1391/files#r956576698

it is but I think that is what needs to be defined in the attribute definition (the actual type)

@dmulcahey dmulcahey merged commit feaa5d3 into dev Dec 27, 2023
14 checks passed
@dmulcahey dmulcahey deleted the dm/t2_relay branch December 27, 2023 16:37
elupus pushed a commit to elupus/zha-device-handlers that referenced this pull request Jan 17, 2024
* Add quirk for Aqara T2 relay

* fix cluster

* rename mode to switch mode

* update based on z2m

* add test

* oops
lgraf pushed a commit to lgraf/zha-device-handlers that referenced this pull request May 6, 2024
* Add quirk for Aqara T2 relay

* fix cluster

* rename mode to switch mode

* update based on z2m

* add test

* oops
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new quirk Adds support for a new device Xiaomi Request/PR regarding a Xiaomi device
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Device Support Request] Aqara T2 Dual Relay (LLKZMK12LM / acn047) - Interlock support
4 participants