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 support for Aqara external temperature sensor #3974

Open
wants to merge 10 commits into
base: dev
Choose a base branch
from

Conversation

Antoninj
Copy link

Proposed change

Support for external temperature sensor.

Additional information

This is a copy of #2802 + the missing unit tests

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

@Antoninj Antoninj changed the title Aqara external temperature sensor Add support for Aqara external temperature sensor Mar 23, 2025
Copy link

codecov bot commented Mar 23, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.09%. Comparing base (2fd3586) to head (930eb02).
Report is 9 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #3974      +/-   ##
==========================================
+ Coverage   91.04%   91.09%   +0.05%     
==========================================
  Files         331      331              
  Lines       10698    10760      +62     
==========================================
+ Hits         9740     9802      +62     
  Misses        958      958              

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@TheJulianJES TheJulianJES added Xiaomi Request/PR regarding a Xiaomi device needs review This PR should be reviewed soon, as it generally looks good. labels Mar 26, 2025
Copy link
Collaborator

@TheJulianJES TheJulianJES left a comment

Choose a reason for hiding this comment

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

Thanks for adding the tests. I think this looks pretty good already.

Comment on lines +398 to +399
SENSOR_TEMP: ("sensor_temp", t.uint32_t, True),
SENSOR_ATTR: (SENSOR_ATTR_NAME, t.LVBytes, True),
Copy link
Collaborator

Choose a reason for hiding this comment

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

All these attributes should really be converted to the new zigpy AttributeDefs style. Search AttributeDefs in this repo or the zigpy repo for examples.
You don't have to do it in this PR, but it would be nice.

We can also access the name and id then: AqaraThermostatSpecificCluster.AttributeDefs.sensor_temp.name or .id.

@@ -402,6 +411,167 @@ def _update_attribute(self, attrid, value):
)
super()._update_attribute(attrid, value)

def aqaraHeader(self, counter: int, params: bytearray, action: int) -> bytearray:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be snake_case, so aqara_header.
Or maybe wrap_with_aqara_header? Something like that?

Comment on lines +445 to +446
attrs = {}
attrs[SENSOR_ATTR_NAME] = self.aqaraHeader(0x12, params, 0x05) + params
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is attrs intentionally set to {} again? It's already set at the beginning of the method. Depending on the order of the attributes parameter, the others wouldn't be written (although it's rare that this method gets multiple attributes to write at the same time).

If it's not intentional, this could be rewritten as:

attrs = {SENSOR_ATTR_NAME: self.aqara_header(0x12, params, 0x05) + params}

If not, well, the attrs = {} can be removed here.

) -> list:
"""Write attributes to device with internal 'attributes' validation."""
sensor = bytearray.fromhex("00158d00019d1b98")
attrs = {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it'd be nice to type this. Should be attrs: dict[str | int, Any] = {} then.

Comment on lines +461 to +476
params1 += bytes(
[
0x00,
0x00,
0x00,
0x00,
0x00,
0x00,
0x00,
0x00,
0x00,
0x00,
0x00,
0x00,
]
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe you can just do params1 += bytes(12).

Comment on lines +481 to +496
params2 += bytes(
[
0x00,
0x00,
0x00,
0x00,
0x00,
0x00,
0x00,
0x00,
0x00,
0x00,
0x00,
0x00,
]
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here as well:
I believe you can just do params2 += bytes(12).

for attr, value in attributes.items():
# implemented with help from https://github.com/Koenkk/zigbee-herdsman-converters/blob/master/devices/xiaomi.js

if attr == SENSOR_TEMP:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Technically, the attributes can either be the id or the name. Generally, it's the id, so this works, but it might be nice to accept both the id and name for SENSOR and SENSOR_TEMP.

If you were to implement the new-style AttributeDefs, you can use AqaraThermostatSpecificCluster.AttributeDefs.sensor_temp.name or AqaraThermostatSpecificCluster.AttributeDefs.sensor_temp.id to access name/id.

Or you can also use find_attribute to get the ZCLAttributeDef then. No need to check both id and name then. I think this should work then:

        attrs: dict[str | int, Any] = {}
        for attr, value in attributes.items():
            attr_def = self.find_attribute(attr)
            if attr_def == AqaraThermostatSpecificCluster.AttributeDefs.sensor_temp:
                # do stuff
            else:
                attrs[attr] = value
        return await super().write_attributes(attrs, manufacturer)

(This might be streamlined in zigpy in the future.)

self, attributes: dict[str | int, Any], manufacturer: int | None = None
) -> list:
"""Write attributes to device with internal 'attributes' validation."""
sensor = bytearray.fromhex("00158d00019d1b98")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess we don't need to call this every time here. Probably enough to just move it outside of the method.

Comment on lines +498 to +506
attrs1 = {}
attrs1[SENSOR_ATTR_NAME] = (
self.aqaraHeader(0x12, params1, 0x04) + params1
)
attrs[SENSOR_ATTR_NAME] = (
self.aqaraHeader(0x13, params2, 0x04) + params2
)

await super().write_attributes(attrs1, manufacturer)
Copy link
Collaborator

Choose a reason for hiding this comment

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

So these need to be written in separate calls?

@@ -456,7 +456,7 @@ The tests use the [pytest](https://docs.pytest.org/en/latest/) framework.
To get set up, you need install the test dependencies:

```bash
pip install -r requirements_test_all.txt
pip install -r requirements_test.txt
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for noticing. Let's keep this in a separate PR though.
(Also, script/setup should also do this.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs review This PR should be reviewed soon, as it generally looks good. Xiaomi Request/PR regarding a Xiaomi device
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants