-
-
Notifications
You must be signed in to change notification settings - Fork 31.3k
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 Danfoss Ally thermostat and derivatives to ZHA #86907
Conversation
…ermostat.pi_heating_demand and thermostat_ui.keypad_lockout
… because of use of bitmap8 instead of enum8
This comment was marked as resolved.
This comment was marked as resolved.
Hey there @dmulcahey, @Adminiuga, @puddly, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
I'll turn this into a draft for now, because I still need to improve some things both here and in zha-device-handlers. |
…rting and read config for danfoss and keypad_lockout.
Ready for merge finally. |
Any chance this could be merged soon? |
Is change requested still applicable to current code? |
It looks like all the changes still add functionality and don't remove anything. It also looks like all new code is written the way it should be. All tests seem like they still pass and I have been running the code for over a month without any issues. Seems like it is good to merge. EDIT: @Mikaka27 you mean the review by edenhaus? That is for a very old version and was just a request for rebasing. Has nothing to do with the current code. I thought you meant the PR itself. |
Yes I meant the change request by edenhaus. |
@edenhaus Could you maybe remove the change requested? It might make it look like I didn't address all reviews yet. Also, could somebody remove the "breaking-change" label? It is not a breaking change, it only adds functionality. The label was erroneously added, because I thought the changes in zha-quirks were considered breaking changes (even if they were considered breaking, that has nothing to do with this PR and also was already merged over a month ago). |
This PR looks good to me. I've made a small PR (Caius-Bonus#4) to @Caius-Bonus's branch to remove code out of @TheJulianJES You left a few review comments regarding |
From a quick look, yeah. I hoped that we could get this into quirks v2 instead, as it should all be possible now, but we can can also add it here I guess. |
Migrate reporting and ZCL attribute config out of `__init__`
@puddly Thanks, I merged your PR. @TheJulianJES I changed the description of my PR to better reflect all changes. This PR needs to happen to at least add the BitMapSensor to HomeAssistant Core. When changing this device to Quirks V2, BitMapSensor would also need to be integrated into Quirks V2. Also, many parts of this device deviate from other devices, which means doing this in Quirks V2 would require multiple other changes to Quirks V2. I think it would be best to merge this PR. Then I can make a new Pull Requests to convert it to Quirks V2. This way the device works for all other users and it will be possible to change the device to Quirks V2 at a slower pace (which I think is going to take multiple PRs, because of the many deviations of this device). Also: can somebody remove the "breaking-change" label. See #86907 (comment) |
Proposed change
This adds support for the proprietary attributes of the Danfoss Ally thermostat and derivatives.
It does this in 5 parts:
a. It implements certain sensors using a more appropriate type than used in the zha-quirks (if changed in the zha-quirks, no changes are required in homeassistant core though).
This makes it possible to fully control the Thermostatic Radiator Valves just like with Zigbee2MQTT.
Type of change
Additional information
[Device Support Request] Danfoss Ally eTRV0100 - other features than just temperature and setpoint zigpy/zha-device-handlers#1726 (It will now have this)
[Device Support Request] Danfoss Ally thermostat: specific device settings (i.e. display Viewing direction) zigpy/zha-device-handlers#717 (It will now have this)
[Device Support Request] Popp Thermostat Radiator Valve 701721 zigpy/zha-device-handlers#2728 (It will now have this)
ZHA with Danfoss TRV - error: 'Failed to call service climate/set_hvac_mode - INVALID_VALUE: 135' #106557 (both issues will be gone)
Checklist
black --fast homeassistant tests
)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest
.requirements_all.txt
.Updated by running
python3 -m script.gen_requirements_all
..coveragerc
.To help with the load of incoming pull requests: