Improve hvac_mode compatibility of vicare

[oischinger]

Proposed change

Improve hvac_mode compatibility of vicare

Viessmann devices can have varying vicare_modes. This integration maps hvac_modes
to vicare_modes without even checking if those vicare_modes are available.

This PR improves the situation by
a) checking if the vicare_mode for the desired hvac_mode actually exits
b) providing only the hvac_modes that are actually supported (meaning: a matching vicare_mode exists)
c) Defining an order of vicare_modes that should be used for a given hvac_mode.

Example for c):
HVACMode.OFF should be VICARE_MODE_FORCEDREDUCED if supported.
If not it should be VICARE_MODE_OFF or VICARE_MODE_DHW

I'll update the documentation accordingly if the PR gets accepted.

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)
• 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 #
• 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
• The code has been formatted using Black (black --fast 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.

The integration reached or maintains the following Integration Quality Scale:

• No score or internal
• 🥈 Silver
• 🥇 Gold
• 🏆 Platinum

To help with the load of incoming pull requests:

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

[gjohansson-ST]

Maybe a bit overdoing it but since it seems you can read the list of modes you could also dynamically build the list instead of having two different dicts depending on what the unit supports.

[oischinger]

Maybe a bit overdoing it but since it seems you can read the list of modes you could also dynamically build the list instead of having two different dicts depending on what the unit supports.

I'm not sure if I get this right. Wouldn't that just move the logic? I still need a static mapping to define which vicare_modes match the hvac_modes?

[gjohansson-ST]

I'm not sure if I get this right. Wouldn't that just move the logic? I still need a static mapping to define which vicare_modes match the hvac_modes?

What I was thinking was given you have a mapping on vicare to HA you can make the HA to vicare mapping dynamic based on supported modes instead of having two different static lists.
But like I said maybe overcomplicating it if it's pretty set with "only" two mappings to consider.

[oischinger]

What I was thinking was given you have a mapping on vicare to HA you can make the HA to vicare mapping dynamic based on supported modes instead of having two different static lists. But like I said maybe overcomplicating it if it's pretty set with "only" two mappings to consider.

I guess the problem here is that I do not know all possible vicare_modes. Therefore I cannot always create a meaningful mapping.
But you're right. the two mappings will most likely work for 98% of devices. I guess to make it work for the last 2% of users who run very odd configurations we would even need a configurable mapping. or those users then just use the set_vicare_mode service.

[hnykda]

Anything else needed to get this merged?

[gjohansson-ST]

Anything else needed to get this merged?

From the discussion initiated by Frenck it seems nothing changed.

[oischinger]

Sorry guys, I was very busy with other stuff recently.
I'll update this PR asap

[oischinger]

I redid this PR from scratch.

Please see the updated description!

[gjohansson-ST]

Seems very reasonable to me.
Just briefly checking the code you have multiple guards if there is no hvac mode. Should the entity/integration be setup in such a case at all or should this check be in earlier steps eg. setup?

[oischinger]

Seems very reasonable to me. Just briefly checking the code you have multiple guards if there is no hvac mode. Should the entity/integration be setup in such a case at all or should this check be in earlier steps eg. setup?

I believe it's necessary to not do this at startup time. If the heating device is turned off you will not get proper values for vicare_modes. Also in case the API is not reachable the integration should be able to recover by itself.

[gjohansson-ST]

I believe it's necessary to not do this at startup time. If the heating device is turned off you will not get proper values for vicare_modes. Also in case the API is not reachable the integration should be able to recover by itself.

Ok I guess for the first argument if the heater isn't replying with states if it's turned off (which seems a bit odd). On the second I guess you should implement available instead if there is a broken connection with the api.

[oischinger]

@frenck can you please have a 2nd look?

[frenck]

Thanks, @oischinger 👍