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 Schneider Electric 1GANG/SHUTTER/1 (MEG5113-0300) #2928

Merged
merged 26 commits into from
Feb 14, 2024

Conversation

cspurk
Copy link
Contributor

@cspurk cspurk commented Jan 21, 2024

Proposed change

Add a quirk for Schneider Electric 1GANG/SHUTTER/1 (MEG5113-0300) to

  • correct the lift percentage (which is inverted).
  • add some manufacturer-specific features.

Additional information

This PR is based on @axellebot’s work in #1705. As the latter is incomplete, I have removed the code of which I don’t know whether it works.

I have successfully tested the quirk with a real device connected to Home Assistant via ZHA.

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 Jan 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (0df6f2b) 87.72% compared to head (a7ddd34) 87.81%.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #2928      +/-   ##
==========================================
+ Coverage   87.72%   87.81%   +0.09%     
==========================================
  Files         298      300       +2     
  Lines        9119     9187      +68     
==========================================
+ Hits         8000     8068      +68     
  Misses       1119     1119              

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

@Hedda Hedda mentioned this pull request Jan 23, 2024
4 tasks
@TheJulianJES TheJulianJES added the new quirk Adds support for a new device label Jan 24, 2024
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.

The changes mostly seem fine. However, I don't know if we want to ship a 6000 line readme file in the source.

Also, the cover implementation in ZHA is currently being reworked a bit:

That might land in HA Core 2024.2.0 (betas). Since these quirks also invert the covers somewhat, it would be nice if they could be tested (with and without(?) quirk) whenever that PR is merged and released to HA Core.

@cspurk
Copy link
Contributor Author

cspurk commented Jan 27, 2024

Thanks for the positive review, @TheJulianJES!

Fair enough, I’ve removed the README. Is there maybe a better place to put it? I suppose @axellebot’s idea behind the file was to have something similar to Zigbee2MQTT’s devices DB, just for zigpy. I thought about a wiki, but zha-device-handlers doesn’t have one.

Regarding home-assistant/core#108238: I can certainly test my device with/without the quirk once this has landed in a HA release. I doubt, though, that the new inversion logic will help here, as it mostly seems to be based on reversing the motor direction. I had tested that manually before implementing the quirk, and it just resulted in a swapping of the up/down push buttons – interestingly both on the wall and in the HA GUI. The opened/closed states in HA were still wrong.

@MattWestb
Copy link
Contributor

I think device / vendor things is the best place in the Wiki https://github.com/zigpy/zigpy/wiki.

@TheJulianJES
Copy link
Collaborator

I thought about a wiki, but zha-device-handlers doesn’t have one.

I think putting quirks-related stuff in a quirks wiki would be good.

Comment on lines 42 to 51
DEVICE_TYPE: zha.DeviceType.WINDOW_COVERING_DEVICE, # 0x0202
INPUT_CLUSTERS: [
Basic.cluster_id, # 0x0000
Identify.cluster_id, # 0x0003
Groups.cluster_id, # 0x0004
Scenes.cluster_id, # 0x0005
WindowCovering.cluster_id, # 0x0102
Diagnostic.cluster_id, # 0x0B05
],
OUTPUT_CLUSTERS: [Ota.cluster_id], # 0x0019
Copy link
Collaborator

Choose a reason for hiding this comment

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

To stay consistent, can you remove the comments with the cluster IDs "everywhere"?
(not the SimpleDescriptor comments above though)

@TheJulianJES
Copy link
Collaborator

Also, to structure it like the rest of our repo, I'd get rid of the devices and clusters folders/package.
Just have zhaquirks/schneiderelectric and put the shared code/clusters/constants in __init__.py.
The shutters.py can go into that folder/package too.

@cspurk
Copy link
Contributor Author

cspurk commented Jan 31, 2024

@MattWestb, @TheJulianJES, thanks for your feedback regarding the wiki. Should I maybe just create an initial structure for a zigpy/zha-device-handlers wiki?

@TheJulianJES, thanks again for the review. I’ve made the changes you requested.

@TheJulianJES
Copy link
Collaborator

Should I maybe just create an initial structure for a zigpy/zha-device-handlers wiki?

Yup, I think that's fine.

@cspurk
Copy link
Contributor Author

cspurk commented Jan 31, 2024

Should I maybe just create an initial structure for a zigpy/zha-device-handlers wiki?

Yup, I think that's fine.

Ok, I’ve created a wiki home page and added a Schneider-specific page with the deleted “README” (mostly as in a4d596c).

@cspurk
Copy link
Contributor Author

cspurk commented Feb 8, 2024

@TheJulianJES, as promised, I’ve tested HA 2024.2.0 (and hence home-assistant/core#108238) with and without the quirk from this PR:

  • Without the quirk, the lift percentage is still inverted (just like before).
  • The quirk continues to fix this (just like before).
  • I can see the new invert switch in the GUI (with and without the quirk), however, it behaves like I described in a previous comment (and which is not useful for this Schneider Electric device).

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.

LGTM, thanks!

@TheJulianJES TheJulianJES merged commit a4c15ef into zigpy:dev Feb 14, 2024
14 checks passed
@cspurk cspurk deleted the schneider branch February 14, 2024 19:14
dflemstr pushed a commit to dflemstr/zha-device-handlers that referenced this pull request Feb 15, 2024
dflemstr pushed a commit to dflemstr/zha-device-handlers that referenced this pull request Feb 15, 2024
dflemstr pushed a commit to dflemstr/zha-device-handlers that referenced this pull request Feb 15, 2024
lgraf pushed a commit to lgraf/zha-device-handlers that referenced this pull request May 6, 2024
…y#2928)

* Add quirk for Schneider Electric 1GANG/SHUTTER/1 (MEG5113-0300)

This commit is based on previous work by Axel Le Bot which unfortunately
is incomplete and is unknown whether it works. I have hence removed the
code that is not needed for the new quirk.

* Add test asserting that unpatched ZCL commands keep working

---------

Co-authored-by: Axel LE BOT <[email protected]>
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.

5 participants