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 Xiaomi E1 driver curtain support #2629

Merged
merged 20 commits into from
Oct 21, 2023

Conversation

TheJulianJES
Copy link
Collaborator

@TheJulianJES TheJulianJES commented Oct 8, 2023

Proposed change

#2620 should be merged first. This PR then needs to be rebased (for the test file).
This PR will be merged first, other than rebased. It'll use some cluster classes from this PR.

Additional information

Supersedes #1648
Fixes #2003

Changes are all untested. I'm also not sure if this is the right way to use the motor mode. It's in the original quirk and a HA PR for the inverted motor mode has already been merged (even though the quirk was never merged).

There are still some TODOs in the PR (which is also why pre-commit fails).

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
Copy link

codecov bot commented Oct 8, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (32e956c) 86.62% compared to head (ca57c7c) 86.68%.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #2629      +/-   ##
==========================================
+ Coverage   86.62%   86.68%   +0.06%     
==========================================
  Files         279      280       +1     
  Lines        8551     8595      +44     
==========================================
+ Hits         7407     7451      +44     
  Misses       1144     1144              
Files Coverage Δ
zhaquirks/xiaomi/__init__.py 92.69% <100.00%> (+0.15%) ⬆️
zhaquirks/xiaomi/aqara/driver_curtain_e1.py 100.00% <100.00%> (ø)

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

@TheJulianJES
Copy link
Collaborator Author

TheJulianJES commented Oct 8, 2023

The quirk file can be downloaded from here: https://raw.githubusercontent.com/TheJulianJES/zha-device-handlers/tjj/xiaomi_driver_curtain_e1_new/zhaquirks/xiaomi/aqara/driver_curtain_e1.py

Please test what works and what doesn't work. If it shows up as a mains-powered device, you can comment in the NODE_DESCRIPTOR section (remove the #), but please report that too.

If you can, please also look at the TODOs and see if you can answer any of them. I'm not sure how the "motor reversing" works with this device (or how it should work).

@TheJulianJES
Copy link
Collaborator Author

TheJulianJES commented Oct 8, 2023

It would also be ideal if we can get some debug logs of the device. Especially, when it's reporting a zhaquirks.xiaomi special Xiaomi attribute report.

@TheJulianJES TheJulianJES added the new quirk Adds support for a new device label Oct 8, 2023
@TheJulianJES
Copy link
Collaborator Author

@cannondale0815 and @Nafania, you mentioned you can help test the quirk. There are instructions in this above comment for downloading the new version. Make sure to remove the older custom quirk.

Please test what works and what doesn't work (open/close, percentage, stop).
If it shows up as a mains-powered device, you can comment in the NODE_DESCRIPTOR section (remove the #), but please report that too.

Also, if you can enable ZHA debug logs and press a button on the device/or wait until you get a zhaquirks.xiaomi message containing Xiaomi attribute report. attribute_id, posting that message here would be helpful.

Please also let me know if reversing this motor works correctly now (and when it's needed).

@Nafania
Copy link

Nafania commented Oct 10, 2023

@TheJulianJES thanks a lot for your work! But i'm not sure how to apply a custom quirk)
I've added this to configuration.yml

zha:
  enable_quirks: true
  custom_quirks_path: /config/quirks

and put your file into this dir, restarted HA and now I see on my device card

IEEE: 54:ef:44:10:00:5b:74:51
Nwk: 0xd2c8
Device Type: EndDevice
LQI: 69
RSSI: Unknown
Last seen: 2023-10-10T21:39:26
Power source: Mains
Quirk: driver_curtain_e1.DriverE1

but I don't know hot to understand which exact quirk is used now. At the device card I can't see any changes.

@dominikremes
Copy link

dominikremes commented Oct 11, 2023

Hi @TheJulianJES,
I'm having a similar issue. I can only see identify, the cover, RSSI and RQI. I got my unit today. I've started experimenting with it. I've added the quirk that you have provided here https://raw.githubusercontent.com/TheJulianJES/zha-device-handlers/tjj/xiaomi_driver_curtain_e1_new/zhaquirks/xiaomi/aqara/driver_curtain_e1.py

My results are the following: Before adding the quirk, it only worked with the percentage slider, but it worked as expected, battery and light sensors weren't shown at all. With the quirk and uncommenting the sensor, a battery, with the status of Unknown shows up and the open and close cover buttons work as well, however with the percentage slider, when I set it to other than 0 or 100% it stays in "Opening" or "Closing" state until I press the stop button.
The reverse selector and light sensors don't show up at all.

EDIT: The battery also shows as a CR2032 and the power source says "Power source: Battery or Unknown"

EDIT 2: Sorry, I was wrong. It gets stuck on Opening or Closing state even without your quirk and the open and close button work strangely without your quirk. Without your quirk, if the open or close button are pressed, while the curtain is fully open or closed, or it is stopped (so not bugged in the Opening or Closing state), than it does nothing. However if it is in Closing state, the open button is pressable, and if pressed, it opens the curtains. Same for the other scenario.

@dominikremes
Copy link

home-assistant_zha_2023-10-11T14-11-04.960Z.log
I'm uploading some debug logs. Started before pairing, than moved it around a little manually, with the slider and with the buttons too. This is using the quirk

@TheJulianJES
Copy link
Collaborator Author

but I don't know hot to understand which exact quirk is used now.

@Nafania The "quirk" section doesn't mention "zhaquirks". It's file_name.class_name.
That looks like it's using a custom quirk.

At the device card I can't see any changes.

That might be expected(?) You should see a cover entity (with and without quirk). The issue was that the cover entity had non-functioning up/down commands for some (depending on firmware).
Only go to percentage (and stop?) seems to work without the quirk.

The quirk should fix the up/down commands. This is what needs to be tested (and if "everything works" now).

@TheJulianJES
Copy link
Collaborator Author

@dominikremes

It gets stuck on Opening or Closing state even without your quirk

I believe this is an issue with ZHA at the moment. I don't have any covers to test with, so I can't fix that at the moment.
This is a PR that aims to fix it eventually(?):

and the open and close button work strangely without your quirk.

Hmm, it seems like only some firmware versions are affected by this then.
The quirk changes the open/close buttons to basically move the percentage slider to "0" or "100". (Zigbee2MQTT does the same from what I can tell)
Can you verify that the quirk open/close buttons are correct? (so not inverted?)

With the quirk and uncommenting the sensor

Can you state what you mean with "uncommenting the sensor"? You mean you uncommented the NODE_DESCRIPTOR section in the quirk file?
Can you attach the device diagnostics (or device signature) for this device without the quirk?

light sensors don't show up at all.

Hmm, I did not know this curtain has one. I'm not sure I'll get around to adding it in this PR, but I'll try to have a look soon into how it's implemented.

The reverse selector [...] don't show up at all

This is an issue with ZHA I guess.
On the device page -> three dots -> Manage Zigbee device -> Clusters -> WindowCovering -> window_covering_mode: "read attribute" you should be able to read the attribute.
After that, restarting HA should create the entity.

You can also write that attribute using that page manually. (Change the upper field after reading, leave lower field empty, then press write.)

This is the most important question:
If you can, please test what changing that attribute changes in the overall behavior without the quirk. (Nothing?)
(Write 0 or 1 in the upper field, leave lower field empty, press write. Verify that it wrote correctly with read. 1 should do inverse and 0 non-inverse/normal.)

@TheJulianJES
Copy link
Collaborator Author

TheJulianJES commented Oct 17, 2023

Added some more changes now: https://raw.githubusercontent.com/TheJulianJES/zha-device-handlers/0541ecf7010d2c9b8f8fcd2784c14beb88bcc419/zhaquirks/xiaomi/aqara/driver_curtain_e1.py

Please check if the "inverted mode" works correctly.
Go to the device page of the curtain -> three dots -> Manage Zigbee device -> select WindowCovering in the first dropdown, then window_covering_mode in the second. Press "read attribute" to read the initial attribute. It's likely 0 (or non-inverted).
Always leave the lower text field empty! Only change the text in the upper one for reading/writing.
Put in 1 in that same upper text field, then press "write attribute". Press read attribute and verify it still says 1 or something with "inverted".
Then, check if the motor inversion is working as expected. (important!)

After you did this initial read, you can also restart Home Assistant, and you'll get the configuration entity.
In the future, this should be fixed, so it always appears (without having to read the attribute first).

So, with this updated quirk version, let me know what works and what doesn't work.

@dominikremes
Copy link

Hi @TheJulianJES,

With the quirk and uncommenting the sensor

Yes, I meant uncommenting the NODE_DESCRIPTOR

This is an issue with ZHA I guess.
On the device page -> three dots -> Manage Zigbee device -> Clusters -> WindowCovering -> window_covering_mode:
"read attribute" you should be able to read the attribute.
After that, restarting HA should create the entity.

I did this. It was by default in inverted mode (returned 1) and the selectable dropdown menu appeared. Now I can set the mode. It seems like after this change, the sider started to do the exact opposite of what I wanted when in inverted mode.
Then I've applied your updated quirk and now it works as expected again.

With the updated quirk it seems like the controls work perfectly as far as I can see.
The Battery state is still unknown as of now.
An illuminance sensor now shows up, but it doesn't have a name, so it's greyed out by default. I named it, now it's blue and a value is visible without opening the sensor data, however it seems like it only defaulted to the value 100 and is not updating itself.

Now I'll do a test without the quirk:

Without the quirk the light sensor is unavailable .
The slider seem to be working
The arrows are bugged out as hell in inverted mode. It says closing when opening and the other way around, when fully closed or open, only the wrong button is available. Seems like it is totally confused with the inverted settings (even though without reading that attribute first, this was the default way, in which it somewhat worked).

Is this enough info, or do you need something else as well?

@dominikremes
Copy link

Hello @TheJulianJES
After this testing, reloads, reconfigurations and whatnots, somehow the battery status appeared now as 100%. I'm not sure, if it is showing the real percentage, or if it is just a default value, but now it doesn't show unknown as before. Not sure however what caused it. Based on its report, it appeared around 2 minutes after I wrote here my last comment. The only thing I did since then was adding back your quirk and restarting HA to apply the quirk

@TheJulianJES
Copy link
Collaborator Author

@dominikremes
Yeah, battery can take some while to appear (and needs the quirk).
So can you say that everything works as expected with the "updated quirk" now?

(Only the issue with the configuration entity not appearing at first needs to be fixed in ZHA.)

@dominikremes
Copy link

@TheJulianJES
I'm not sure about the battery, and if it will report if if it starts to drain, as it still says 100%.
I'm however sure, that the light sensor is acting up and doing weird stuff, as it seems to know only 0 and 100
Screenshot_20231019_070924_Home Assistant

@TheJulianJES
Copy link
Collaborator Author

Yeah, the light sensor reports values of 0, 1, 2 only apparently.
Those are mapped to 1 lx, 50 lx, 100 lx.

@codyc1515
Copy link

codyc1515 commented Oct 20, 2023

There are multiple issues / pull requests raised on here (#1648, #2003, #2620 & #2629) but thought it may be worthwhile to clarify a few things all in one post.

Originally I am using the latest HA (2023.10.x / zhaquirks.xiaomi.aqara.roller_curtain_e1.RollerE1AQ_3) and using the out-of-the-box firmware on the Aqara roller. I am using Skyconnect - everything is latest version.

This worked okay - briefly - including in HA. I am able to use both the number and the up / down buttons with corect motion, but the motor couldn't always pull up physically. Battery also appeared to be okay (after a charge), same for device temperature (most of the time) but saw some spikes up to 60+ degrees (despite that the device did not feel warm). Identify, LQI and RSSI all showing. No mention of the light sensor anywhere.

Read somewhere online that the issue with pulling up could be due to the outdated firmware. Updated the roller to latest firmware using Aqara Hub E1. Still no mention of the light sensor; however, now facing the issue that the roller was now inverting the up / down when used in ZHA.

So, I am trying to test with your changes https://raw.githubusercontent.com/TheJulianJES/zha-device-handlers/tjj/xiaomi_driver_curtain_e1_new/zhaquirks/xiaomi/aqara/driver_curtain_e1.py but despite my trying cannot seem to get the new quirk to show, even after re-pairing. configuration.yaml looks like:

zha:
  database_path: /config/zigbee.db 
  enable_quirks: true
  custom_quirks_path: /config/zha_custom_quirks

Meanwhile log simply makes mention that it loaded the quirks folder but it did not.

Anyway, to fix the the inversion issue (without using any quirks), you simply press the reset button 3 times on the roller itself to change the direction. You will see the LED blink blue to indicate it's changed then job done. Problem solved :)

... Still haven't got the light sensor working though.

image

@codyc1515
Copy link

Anyone? Happy to do the testing and maybe a few tweaks here and there but need your help to get started.

@TheJulianJES
Copy link
Collaborator Author

@codyc1515 You have the roller Aqara curtain then lumi.curtain.acn002. This PR is not for that, but for the driver lumi.curtain.agl001 one.

I have PR to clean up the code slightly for your device: #2620
It doesn't change behavior of the curtain though. The issue you linked also is not about your curtain model.

Can you open another issue mentioning the issues (with updated firmware) again and tag me there (@TheJulianJES)?
If I find some time, I'll try to look at the missing light sensor for your model and and inversed mode.
You can already try to update the window_covering_mode using the clusters UI like mentioned here. I'm not sure if that works for your model though. Let's continue discussion in your new issue then.

@TheJulianJES TheJulianJES force-pushed the tjj/xiaomi_driver_curtain_e1_new branch from e087463 to 20fc531 Compare October 21, 2023 00:05
@TheJulianJES TheJulianJES marked this pull request as ready for review October 21, 2023 00:10
Comment on lines +504 to +520
class XiaomiPowerConfigurationPercent(XiaomiPowerConfiguration):
"""Power cluster which ignores Xiaomi voltage reports for calculating battery percentage
Devices that use this cluster (E1 curtain driver/roller) already send the battery percentage on their own
as a separate attribute, but additionally also send the battery voltage.
This class only uses the voltage reports for the voltage attribute, but not for the battery percentage.
The battery percentage is used as is from the battery percentage reports using inherited battery_percent_reported().
"""

def _update_battery_percentage(self, voltage_mv: int) -> None:
"""Ignore Xiaomi voltage reports, so they're not used to calculate battery percentage."""
# This device sends battery percentage reports which are handled using a XiaomiCluster and
# the inherited XiaomiPowerConfiguration cluster.
# This device might also send Xiaomi battery reports, so we only want to use those for the voltage attribute,
# but not for the battery percentage. XiaomiPowerConfiguration.battery_reported() still updates the voltage.


Copy link
Collaborator Author

@TheJulianJES TheJulianJES Oct 21, 2023

Choose a reason for hiding this comment

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

Note for reviews:

Comment on lines +645 to +655
class LocalIlluminanceMeasurementCluster(
LocalDataCluster, IlluminanceMeasurementCluster
):
"""Illuminance measurement cluster based on LocalDataCluster."""

def __init__(self, *args, **kwargs):
"""Init."""
super().__init__(*args, **kwargs)
if self.AttributeDefs.measured_value.id not in self._attr_cache:
# put a default value so the sensor is created
self._update_attribute(self.AttributeDefs.measured_value.id, 0)
Copy link
Collaborator Author

@TheJulianJES TheJulianJES Oct 21, 2023

Choose a reason for hiding this comment

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

Note for reviews:

  • This was copied from the motion_ac02 (Aqara P1 motion sensor) quirk.
  • A future PR will change that P1 sensor to use this "main" class.
    (similar to the above XiaomiPowerConfigurationPercent class)

@TheJulianJES
Copy link
Collaborator Author

Merging now to move forward with multiple other PRs. IMO, the code is fine now and tested (by people and unit tests).
Still appreciate (late) reviews of course and any issues reports.

Due to the changes, this is the latest commit that still works when only using it as a custom quirk: https://raw.githubusercontent.com/TheJulianJES/zha-device-handlers/0541ecf7010d2c9b8f8fcd2784c14beb88bcc419/zhaquirks/xiaomi/aqara/driver_curtain_e1.py
The changes afterwards only refactor the code for future PRs.

@Nafania
Copy link

Nafania commented Oct 22, 2023

Update quirk to the latest version from https://raw.githubusercontent.com/TheJulianJES/zha-device-handlers/0541ecf7010d2c9b8f8fcd2784c14beb88bcc419/zhaquirks/xiaomi/aqara/driver_curtain_e1.py .

Just a few comments from my side

  1. inverted mode works perfectly now and does exactly what it means. Open / close status of the curtain now works fine and reports properly.
  2. Now I can see the battery level
  3. Illuminance sensor also started to work for me. But I am not sure it works properly as for me it always displays 100lx

@TheJulianJES thanks a lot for your efforts!

@msolomos
Copy link

msolomos commented Dec 16, 2023

Am having 2 of these devices.
I would be happy to do test so I can help as much I can

I can operate them with some bugs.
The first one doesn't show the "curtain mode". I guess that I must remove it and pair it again.
The second one does show the "curtain mode" but the battery reports as unknown.

I can live with both.
The first problem I solved it by placing the device from the other side (button looking inside)
The battery problem I don't really mind as I don't really use this curtain.

However, the most annoying problem I have is that both curtains report wrong state!!

εικόνα

I would be really grateful if anyone can help me solve this problem

@BackedUpBooty
Copy link

I'm using the Chinese version of the agl001, the acn003 (see screenshot).

image

I've attempted this quirk, replacing the lumi.curtain.agl001 on line 127 with the acn003, and I get the following in ZHA:

image

It has solved the issue of the open / close buttons not working, and by extension the service call to open and close the cover(s), however no battery status and no inversion options.

To be honest I don't know enough about writing python or quirks to be able to offer any suggestion, if you're willing to take a look into this please let me know what other info I can share.

@Nafania
Copy link

Nafania commented Dec 23, 2023

@BackedUpBooty have you added

zha:
	enable_quirks: true
	custom_quirks_path: /config/quirks

to configuration.yml file?

Do you see a message in the log smth like "custom quirks loaded" during startup to ensure that custom quirks were loaded?

@BackedUpBooty
Copy link

@Nafania thanks for the reply.

you can see in my screenshot under Device info that the quirk is used, my configuration.yml has

zha:
  database_path: /config/zigbee.db 
  enable_quirks: true
  custom_quirks_path: /config/zha_quirks

and yes the load confirmation message is in the logs.

@TheJulianJES
Copy link
Collaborator Author

@BackedUpBooty It looks like there isn't an issue for that device yet, can you create one (and fill-out the issue template)?
Also, include the custom quirk linked on that website.

@BackedUpBooty
Copy link

@TheJulianJES done, #2860

elupus pushed a commit to elupus/zha-device-handlers that referenced this pull request Jan 17, 2024
* Add quirk for Xiaomi E1 driver curtain

* Add tests for Xiaomi E1 driver curtain

* Fix inverted logic

* Add test for inverted logic

* Comment out inverting percentage

Apparently this is already done by the cover when "inverted mode" is enabled. Does this depend on firmware?

* Add more custom attributes

* Add illuminance measurement

Possible values are 0, 1, 2 apparently

* Add custom node descriptor for disabling "is mains powered"

* Move imports

* Comment out test for reverting lift percentage

We're not inverting it ourselves at the moment

* Make illuminance cluster a `LocalDataCluster`

* Remove "TODO: `XiaomiAqaraDriverE1` in OUTPUT clusters?"

Doesn't seem to be the case, but I'm not sure.

* Remove commented code about inverting "go to lift percent" command

* Remove commented code about inverting current lift percent attribute

* Remove TODO for up/down commands

* Change "attributes copy" to copy from `XiaomiAqaraE1Cluster` class

* Move local illuminance cluster and custom PowerConfiguration class to main Xiaomi file

They'll also be used by other devices (in future PRs)

* Remove commented test about testing inverted percent attribute

This functionality was removed, as it seems to be wrong according to comments in zigpy#2629

* Change comment for `WindowCoveringE1` cluster class

* Add tests for custom light level attribute
lgraf pushed a commit to lgraf/zha-device-handlers that referenced this pull request May 6, 2024
* Add quirk for Xiaomi E1 driver curtain

* Add tests for Xiaomi E1 driver curtain

* Fix inverted logic

* Add test for inverted logic

* Comment out inverting percentage

Apparently this is already done by the cover when "inverted mode" is enabled. Does this depend on firmware?

* Add more custom attributes

* Add illuminance measurement

Possible values are 0, 1, 2 apparently

* Add custom node descriptor for disabling "is mains powered"

* Move imports

* Comment out test for reverting lift percentage

We're not inverting it ourselves at the moment

* Make illuminance cluster a `LocalDataCluster`

* Remove "TODO: `XiaomiAqaraDriverE1` in OUTPUT clusters?"

Doesn't seem to be the case, but I'm not sure.

* Remove commented code about inverting "go to lift percent" command

* Remove commented code about inverting current lift percent attribute

* Remove TODO for up/down commands

* Change "attributes copy" to copy from `XiaomiAqaraE1Cluster` class

* Move local illuminance cluster and custom PowerConfiguration class to main Xiaomi file

They'll also be used by other devices (in future PRs)

* Remove commented test about testing inverted percent attribute

This functionality was removed, as it seems to be wrong according to comments in zigpy#2629

* Change comment for `WindowCoveringE1` cluster class

* Add tests for custom light level attribute
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Device Support Request] Aqara Curtain Driver E1 (lumi.curtain.agl001)
6 participants