Add overkiz support for Atlantic Shogun ZoneControl 2.0 (AtlanticPassAPCHeatingAndCoolingZone) #110510

Tronix117 · 2024-02-13T23:33:39Z

Proposed change

There was a previous implementation of the widget AtlanticPassAPCHeatingAndCoolingZone, but it was lacking cooling features, and was only reusing AtlanticPassAPCHeatingZone logic.

There was a lack of support for the devices not supporting the setDerogatedTargetTemperature command (see #89746).

This PR handles that, it is mainly a fix for the Atlantic Shogun Zone Control 2.0, but could work for other devices.
As of right now, I only found one other device using the same widget : Atlantic Alféa Extensa Duo (see discussion here : iMicknl/ha-tahoma#381), for this device the behavior will stay the same as before (good or bad as it is).

Here what can be done :

change of temperature (will change manual cooling and manual heating temperature)
modes :
- OFF does not really stop de HVAC, but stop the airflow to the specific zone (= STOP on the zone thermostat)
- HEAT or COOL or DRYING, the only other available mode for the zone, will be the currently active mode on the main ZoneControl unit
preset :
- Manual : allow to set temperature
- Internal_Schedule : will follow comfort/eco rules set internaly on the main ZoneControl unit, temperature change is ignored on this mode

In addition Auto HVAC mode has been added, when the Atlantic Pass APC device can be configured with.

Here what may be done in a future PR :

disable change of temperature for DRYING mode
follow virtually the device preset (comfort, eco) when targetTemperature = presetTargetTemperature
- it means the internal_schedule preset info is lost, so that when we select internal_schedule, it will immediately goes to Comfort or Eco, maybe a Schedule Comfort and Schedule Eco preset could be added instead to solve the issue
When Comfort preset or Eco preset is selected, allow the change of the preset temperature (but only for the current mode as opposition to Manual which changes for all modes)

See #89746 for more informations about these changes.

For other devices, that support the setDerogatedTargetTemperature command, the behavior has not been changed and it will fallback to the AtlanticPassAPCHeatingZone like before (so no breaking changes).

Type of change

Dependency upgrade
Bugfix (non-breaking change which fixes an issue)
New integration (thank you!)
New feature (which adds functionality to an existing integration)
Deprecation (breaking change to happen in the future)
Breaking change (fix/feature causing existing functionality to break)
Code quality improvements to existing code or addition of tests

Additional information

This PR fixes or closes issue:
- fixes No such command setDerogatedTargetTemperature for AtlanticPassAPCZoneControl in Overkiz #89746
- fixes Atlantic Cozytouch - Overkiz - Error doing job: Task exception was never retrieved (Appeared 136 times) #108211
This PR is related to issue:
Link to documentation pull request:

Checklist

The code change is tested and works locally.
Local tests pass. Your PR cannot be merged unless tests pass
There is no commented out code in this PR.
I have followed the development checklist
I have followed the perfect PR recommendations
The code has been formatted using Ruff (ruff format homeassistant tests)
Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated for www.home-assistant.io

If the code communicates with devices, web services, or third-party tools:

The manifest file has all fields filled out correctly.
Updated and included derived files by running: python3 -m script.hassfest.
New or updated dependencies have been added to requirements_all.txt.
Updated by running python3 -m script.gen_requirements_all.
For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

I have reviewed two other open pull requests in this repository.

home-assistant

Hi @Tronix117

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

home-assistant · 2024-02-13T23:33:48Z

Hey there @iMicknl, @vlebourl, @tetienne, @nyroDev, mind taking a look at this pull request as it has been labeled with an integration (overkiz) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of overkiz can trigger bot actions by commenting:

@home-assistant close Closes the pull request.
@home-assistant rename Awesome new title Renames the pull request.
@home-assistant reopen Reopen the pull request.
@home-assistant unassign overkiz Removes the current integration label and assignees on the pull request, add the integration domain after the command.
@home-assistant add-label needs-more-information Add a label (needs-more-information, problem in dependency, problem in custom component) to the pull request.
@home-assistant remove-label needs-more-information Remove a label (needs-more-information, problem in dependency, problem in custom component) on the pull request.

homeassistant/components/overkiz/climate_entities/atlantic_pass_apc_heating_zone.py

nyroDev · 2024-02-15T22:22:20Z

@Tronix117 Thanks for your first PR here!

This PR is breaking everything on my setup regarding the device you're updating.
Before, I had HAVC Mode enabled (Auto, Heating, Off)
Now, I've got nothing

Before I had Preset Eco, comfort, away, antifrost, externe, home,
Now, I only have manual and schedule

When I try to set to manual, I have error:

2024-02-15 22:12:15.804 ERROR (MainThread) [homeassistant.components.websocket_api.http.connection] [140617948070336] 'manual'
Traceback (most recent call last):
  File "/workspaces/home-assistant-core/homeassistant/components/websocket_api/commands.py", line 240, in handle_call_service
    response = await hass.services.async_call(
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/workspaces/home-assistant-core/homeassistant/core.py", line 2291, in async_call
    response_data = await coro
                    ^^^^^^^^^^
  File "/workspaces/home-assistant-core/homeassistant/core.py", line 2328, in _execute_service
    return await target(service_call)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/workspaces/home-assistant-core/homeassistant/helpers/service.py", line 904, in entity_service_call
    single_response = await _handle_entity_call(
                      ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/workspaces/home-assistant-core/homeassistant/helpers/service.py", line 974, in _handle_entity_call
    result = await task
             ^^^^^^^^^^
  File "/workspaces/home-assistant-core/homeassistant/components/climate/__init__.py", line 709, in async_handle_set_preset_mode_service
    await self.async_set_preset_mode(preset_mode)
  File "/workspaces/home-assistant-core/homeassistant/components/overkiz/climate_entities/atlantic_pass_apc_heating_and_cooling_zone.py", line 165, in async_set_preset_mode
    return await super().async_set_preset_mode(preset_mode)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/workspaces/home-assistant-core/homeassistant/components/overkiz/climate_entities/atlantic_pass_apc_heating_zone.py", line 156, in async_set_preset_mode
    await self.async_set_heating_mode(PRESET_MODES_TO_OVERKIZ[preset_mode])
                                      ~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^
KeyError: 'manual'

Min/max available temperarture was also set up directly in the code, because AFAIK it was done that way in tahoma code too.

If I understand correctly, you're trying to fix a problem for a specific device, which seem to fall under the same widget name than me (and the one that is currenlty working on HA)

If the diagnostic you're trying to fix is the one posted here, I think we have to choose the correct device class using controllableName

On my case, it's AtlanticPassAPCHeatingAndCoolingZoneComponent
On the diagnostic posted, it's io:AtlanticPassAPCZoneControlZoneComponent

In other words, it means we have to something like WIDGET_AND_PROTOCOL_TO_CLIMATE_ENTITY to match the Widget and then the controllablename.
Or maybe we can do directly on the controllableName.

Tronix117 · 2024-02-15T22:45:48Z

Ok thanks for your feedback.

That's weird because I first check if you support the missing command for the device I'm trying to fix (the one used to set the temperature on your device), if it's the case I fallback to the previous behavior. Obviously it seems that I introduced a mistake somewhere, I'll check that next week.

Edit: I know what the issue is, it's from the initialized attributes, I need to do them in the init function.

The initial code is also not perfect for your device that is reversible (to cooling) if the thermostat is changed. I'll try to do something for that too (but in a separated PR)

Tronix117 · 2024-02-17T21:55:13Z

@nyroDev You were right about controllableName, I missed that in your diagnostic export.
I changed the code to be able to choose the class using the controllableName in addition to the widget.

However I still kept (and also fix the issue you had), the check I do on the SET_DEROGATED_TARGET_TEMPERATURE presence. That way I'm sure it will behave good even if controllableName is not what we expect on an "unknown yet" device.

Not sure if I shall still keep it, since it increase complexity a bit...

Could you please @nyroDev check if the new changes I made, is working perfectly (like before) for you ?
Thanks

iMicknl

Thanks a lot for spending your time on this! These devices are very complex, and would be good to support them all in a better way.

Didn't had time yet to deep dive in your code, but will try to do that somewhere next week. Let me know when the PR is fully tested and ready for review.

CODEOWNERS

homeassistant/components/overkiz/manifest.json

homeassistant/components/overkiz/climate_entities/__init__.py

nyroDev

Some small changes

homeassistant/components/overkiz/climate_entities/atlantic_pass_apc_zone_control.py

homeassistant/components/overkiz/climate_entities/__init__.py

...components/overkiz/climate_entities/atlantic_pass_apc_heating_and_cooling_controlled_zone.py

nyroDev · 2024-02-19T22:50:42Z

Could you please @nyroDev check if the new changes I made, is working perfectly (like before) for you ? Thanks

The current code works as before for me, thanks for that ;)

Tronix117 · 2024-02-19T22:54:19Z

Thanks for the review, it seems pretty good, strings instead of constants are there because there is an issue with the CI on the other repo preventing the pyoverkiz release (@iMicknl could you take a look ?)
I'll include the changes asap

iMicknl · 2024-02-20T08:43:36Z

@Tronix117 what do you mean? The latest pyoverkiz has been published a few days ago and should include your changes.

CODEOWNERS

CLA signed

iMicknl

Some quick comments.

homeassistant/components/overkiz/climate.py

homeassistant/components/overkiz/climate_entities/atlantic_pass_apc_zone_control_zone.py

home-assistant · 2024-02-26T11:18:18Z

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

homeassistant/components/overkiz/climate_entities/atlantic_pass_apc_zone_control.py

homeassistant/components/overkiz/climate_entities/atlantic_pass_apc_zone_control_zone.py

…support it

…tlantic APC)

iMicknl

LGTM! (and tested by multiple users). Thanks!

MartinHjelmare

Please address the comments in a new PR. Thanks!

MartinHjelmare · 2024-02-29T01:11:00Z

homeassistant/components/overkiz/climate.py

@@ -28,6 +32,18 @@ async def async_setup_entry(
        if device.widget in WIDGET_TO_CLIMATE_ENTITY
    )

+    # Match devices based on the widget and controllableName
+    # This is for example used for Atlantic APC, where devices with different functionality share the same uiClass and widget.


Please break long comments around max 88 characters per line.

MartinHjelmare · 2024-02-29T01:12:50Z

homeassistant/components/overkiz/climate.py

@@ -28,6 +32,18 @@ async def async_setup_entry(
        if device.widget in WIDGET_TO_CLIMATE_ENTITY
    )

+    # Match devices based on the widget and controllableName
+    # This is for example used for Atlantic APC, where devices with different functionality share the same uiClass and widget.
+    async_add_entities(


Please collect all entities and make a single call to async_add_entities instead.

It'll be better indeed, I'll refactor that.
Initialy, I didn't do it, because there was already two calls, so I was not sure if it was my call to change that or not.

MartinHjelmare · 2024-02-29T01:16:31Z

homeassistant/components/overkiz/climate_entities/atlantic_pass_apc_zone_control_zone.py

+
+        # It helps keep it consistent with the Zone Control, within the interface.
+        if self._attr_hvac_modes != [zone_control_hvac_mode, HVACMode.OFF]:
+            self._attr_hvac_modes = [zone_control_hvac_mode, HVACMode.OFF]


Changing the hvac modes during the entity lifetime is weird.

Regardless, there shouldn't be side effects in entity state properties. If another attribute needs to change it should be done in a state update callback.

@MartinHjelmare yes it's as weird as those devices can be...

The thing is, none of those subsidiary devices can control the current mode that is given by the parent ZoneControl device. There only choices are On or Stop.

Maybe the best choice is to always use HEAT_COOL + OFF for those subsidiary devices.
I made the #111830 PR, for now I only use HEAT_COOL for AUTO, but I can always use it instead. It's just not this relevant to change cooling temperature when ZoneControl mode is forced to Heating.

Do you have any advice or tip ?

Side note: The auto mode should be used only if the temperature can't be controlled by the user.

Why not only have on/off as mode for the simple devices?

@MartinHjelmare
A little bit more context:

the main control device (ZoneControl), have no temperature control, but mode selection: Auto, Heating, Cooling, Drying, Off

the zones (ZoneControlZone), does not control the mode, but control the temperature (have different cooling and heating temperature).

Also from this doc : https://developers.home-assistant.io/docs/core/entity/climate/ there is no ON possibility, and to set temperature on a per zone basis, it needs to be one of the other mode.

But I think using [HvacMode.HEAT_COOL, HVACMode.Off] (for any mode on the main ZoneControl device), will be the best call here.

Cf.

MartinHjelmare · 2024-02-29T01:34:22Z

homeassistant/components/overkiz/climate_entities/atlantic_pass_apc_zone_control_zone.py

+        # It helps keep it consistent with the Zone Control, within the interface.
+        if self._attr_hvac_modes != [zone_control_hvac_mode, HVACMode.OFF]:
+            self._attr_hvac_modes = [zone_control_hvac_mode, HVACMode.OFF]
+            self.async_write_ha_state()


This needs to be removed.

If I remove that, it seems the changes I make on _attr_hvac_modes are not instantaneous on the interface...

There needs to be a callback method that is called by the thing that initiates the state update. That callback should make all needed changes to attributes and then call self.async_write_ha_state.

…APCHeatingAndCoolingZone) (#110510) * Add Overkiz support for AtlanticPassAPCHeatingAndCoolingZone widget * Add support for AUTO HVAC mode for Atlantic Pass APC ZC devices that support it * Add support for multiple IO controllers for same widget (mainly for Atlantic APC) * Implement PR feedback * Small PR fixes * Fix constant inversion typo

home-assistant bot previously requested changes Feb 13, 2024

View reviewed changes

home-assistant bot added cla-needed dependency integration: overkiz new-feature labels Feb 13, 2024

home-assistant bot assigned iMicknl Feb 13, 2024

home-assistant bot added the Quality Scale: No score label Feb 13, 2024

home-assistant bot added cla-recheck cla-signed and removed cla-recheck cla-needed labels Feb 13, 2024

Tronix117 force-pushed the feature/overkiz-apc-heating-cooling-zone branch from 5bec218 to 2bafaeb Compare February 13, 2024 23:35

Tronix117 requested a review from home-assistant[bot] February 13, 2024 23:36

Tronix117 commented Feb 13, 2024

View reviewed changes

homeassistant/components/overkiz/climate_entities/atlantic_pass_apc_heating_zone.py Show resolved Hide resolved

Tronix117 force-pushed the feature/overkiz-apc-heating-cooling-zone branch from 2bafaeb to 8e3adad Compare February 14, 2024 11:21

Tronix117 changed the title ~~Add Overkiz support for AtlanticPassAPCHeatingAndCoolingZone widget~~ Add overkiz support for Atlantic Shogun ZoneControl 2.0 (AtlanticPassAPCHeatingAndCoolingZone) Feb 14, 2024

Tronix117 marked this pull request as ready for review February 14, 2024 11:39

Tronix117 requested a review from iMicknl as a code owner February 14, 2024 11:39

Tronix117 force-pushed the feature/overkiz-apc-heating-cooling-zone branch from 8e3adad to 9ab51d4 Compare February 14, 2024 12:04

Tronix117 force-pushed the feature/overkiz-apc-heating-cooling-zone branch 2 times, most recently from 3098c75 to 2302527 Compare February 17, 2024 21:24

iMicknl reviewed Feb 18, 2024

View reviewed changes

CODEOWNERS Outdated Show resolved Hide resolved

homeassistant/components/overkiz/manifest.json Outdated Show resolved Hide resolved

homeassistant/components/overkiz/climate_entities/__init__.py Outdated Show resolved Hide resolved

nyroDev reviewed Feb 19, 2024

View reviewed changes

Tronix117 force-pushed the feature/overkiz-apc-heating-cooling-zone branch 2 times, most recently from ffb7649 to 5ef8078 Compare February 20, 2024 15:00

iMicknl reviewed Feb 25, 2024

View reviewed changes

CODEOWNERS Outdated Show resolved Hide resolved

Tronix117 force-pushed the feature/overkiz-apc-heating-cooling-zone branch 3 times, most recently from 251e27b to 9c014ef Compare February 26, 2024 09:38

Tronix117 mentioned this pull request Feb 26, 2024

Add @tronix117 as overkiz codeowner #111464

Merged

20 tasks

iMicknl requested changes Feb 26, 2024

View reviewed changes

home-assistant bot marked this pull request as draft February 26, 2024 11:18

joostlek reviewed Feb 26, 2024

View reviewed changes

homeassistant/components/overkiz/climate_entities/atlantic_pass_apc_zone_control.py Show resolved Hide resolved

homeassistant/components/overkiz/climate_entities/atlantic_pass_apc_zone_control_zone.py Outdated Show resolved Hide resolved

Tronix117 force-pushed the feature/overkiz-apc-heating-cooling-zone branch from fb1634c to 870dda1 Compare February 26, 2024 12:50

Tronix117 marked this pull request as ready for review February 26, 2024 13:02

home-assistant bot requested a review from iMicknl February 26, 2024 13:02

Tronix117 added 5 commits February 26, 2024 14:08

Add Overkiz support for AtlanticPassAPCHeatingAndCoolingZone widget

4ede18c

Add support for AUTO HVAC mode for Atlantic Pass APC ZC devices that …

eb43cd2

…support it

Add support for multiple IO controllers for same widget (mainly for A…

8932a52

…tlantic APC)

Implement PR feedback

0e508dc

Small PR fixes

a315e05

Tronix117 force-pushed the feature/overkiz-apc-heating-cooling-zone branch from 870dda1 to a315e05 Compare February 26, 2024 13:11

Fix constant inversion typo

b6ac28b

iMicknl approved these changes Feb 28, 2024

View reviewed changes

iMicknl merged commit eeb8724 into home-assistant:dev Feb 28, 2024
23 checks passed

iMicknl added this to the 2024.3.0 milestone Feb 28, 2024

MartinHjelmare reviewed Feb 29, 2024

View reviewed changes

balloob added the cherry-picked label Feb 29, 2024

Tronix117 mentioned this pull request Feb 29, 2024

Follow real AtlanticPassAPCZoneControlZone physical mode on Overkiz (HEAT, COOL or HEAT_COOL) #111830

Merged

20 tasks

github-actions bot locked and limited conversation to collaborators Mar 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add overkiz support for Atlantic Shogun ZoneControl 2.0 (AtlanticPassAPCHeatingAndCoolingZone) #110510

Add overkiz support for Atlantic Shogun ZoneControl 2.0 (AtlanticPassAPCHeatingAndCoolingZone) #110510

Tronix117 commented Feb 13, 2024 •

edited

Loading

home-assistant bot left a comment

home-assistant bot commented Feb 13, 2024

nyroDev commented Feb 15, 2024

Tronix117 commented Feb 15, 2024 •

edited

Loading

Tronix117 commented Feb 17, 2024

iMicknl left a comment

nyroDev left a comment

nyroDev commented Feb 19, 2024

Tronix117 commented Feb 19, 2024

iMicknl commented Feb 20, 2024

iMicknl left a comment

home-assistant bot commented Feb 26, 2024

iMicknl left a comment

MartinHjelmare left a comment

MartinHjelmare Feb 29, 2024

MartinHjelmare Feb 29, 2024

Tronix117 Feb 29, 2024

MartinHjelmare Feb 29, 2024

Tronix117 Feb 29, 2024

MartinHjelmare Feb 29, 2024

Tronix117 Feb 29, 2024

MartinHjelmare Feb 29, 2024

Tronix117 Feb 29, 2024

MartinHjelmare Feb 29, 2024

Add overkiz support for Atlantic Shogun ZoneControl 2.0 (AtlanticPassAPCHeatingAndCoolingZone) #110510

Add overkiz support for Atlantic Shogun ZoneControl 2.0 (AtlanticPassAPCHeatingAndCoolingZone) #110510

Conversation

Tronix117 commented Feb 13, 2024 • edited Loading

Proposed change

Type of change

Additional information

Checklist

home-assistant bot left a comment

Choose a reason for hiding this comment

home-assistant bot commented Feb 13, 2024

nyroDev commented Feb 15, 2024

Tronix117 commented Feb 15, 2024 • edited Loading

Tronix117 commented Feb 17, 2024

iMicknl left a comment

Choose a reason for hiding this comment

nyroDev left a comment

Choose a reason for hiding this comment

nyroDev commented Feb 19, 2024

Tronix117 commented Feb 19, 2024

iMicknl commented Feb 20, 2024

iMicknl left a comment

Choose a reason for hiding this comment

home-assistant bot commented Feb 26, 2024

iMicknl left a comment

Choose a reason for hiding this comment

MartinHjelmare left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tronix117 commented Feb 13, 2024 •

edited

Loading

Tronix117 commented Feb 15, 2024 •

edited

Loading