core icon indicating copy to clipboard operation
core copied to clipboard

Add overkiz support for Atlantic Shogun ZoneControl 2.0 (AtlanticPassAPCHeatingAndCoolingZone)

Open Tronix117 opened this issue 1 year ago • 7 comments

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 : https://github.com/iMicknl/ha-tahoma/issues/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!)
  • [x] 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 #89746
  • This PR is related to issue:
  • Link to documentation pull request:

Checklist

  • [x] The code change is tested and works locally.
  • [x] Local tests pass. Your PR cannot be merged unless tests pass
  • [x] There is no commented out code in this PR.
  • [x] I have followed the development checklist
  • [x] I have followed the perfect PR recommendations
  • [x] 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:

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

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

To help with the load of incoming pull requests:

Tronix117 avatar Feb 13 '24 23:02 Tronix117

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.

home-assistant[bot] avatar Feb 13 '24 23:02 home-assistant[bot]

@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.

nyroDev avatar Feb 15 '24 22:02 nyroDev

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 avatar Feb 15 '24 22:02 Tronix117

@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

Tronix117 avatar Feb 17 '24 21:02 Tronix117

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 ;)

nyroDev avatar Feb 19 '24 22:02 nyroDev

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

Tronix117 avatar Feb 19 '24 22:02 Tronix117

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

iMicknl avatar Feb 20 '24 08:02 iMicknl

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

Learn more about our pull request process.

home-assistant[bot] avatar Feb 26 '24 11:02 home-assistant[bot]