Skip to content

Comfoconnect improvements#55062

Closed
switschel wants to merge 3 commits intohome-assistant:devfrom
switschel:comfoconnect-Improvements
Closed

Comfoconnect improvements#55062
switschel wants to merge 3 commits intohome-assistant:devfrom
switschel:comfoconnect-Improvements

Conversation

@switschel
Copy link
Copy Markdown

Proposed change

comfoconnect module currently set fan speed mode temporary for 2 hrs by default. Afterwards the unit will switch back to auto.
Also it was not possible to to switch back to auto which is the most desired mode.
For example if you turn off the unit it will go to away mode for a specific time only. If you turn it on it will go to fan speed "low" for a specific time (which is not the desired state in most cases).

In this pull request I did two things:

  1. When fan speed is adjusted the unit will also go to manual mode (so no temporary change anymore).
  2. When turned on or percentage of fan speed is not provided (set to 0) it will go back to "Auto" mode.

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

-Feature request: https://community.home-assistant.io/t/add-auto-manual-mode-switch-for-comfoconnect/143437

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:

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:

@homeassistant
Copy link
Copy Markdown
Contributor

Hi @switschel,

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!

@probot-home-assistant
Copy link
Copy Markdown

Hey there @michaelarnauts, mind taking a look at this pull request as it has been labeled with an integration (comfoconnect) you are listed as a code owner for? Thanks!
(message by CodeOwnersMention)

@michaelarnauts
Copy link
Copy Markdown
Contributor

@switschel You are right that the current behaviour is confusing, however, I think that the right way to fix this, is to implement preset_mode, so you can switch between auto and manual, like it happens in the Comfoconnect App.

When the preset mode is manual, there is no timeout and a speed change doesn't fallback. When the preset mode is auto, you can change the speed, but it will fall back after 2 hours.

@michaelarnauts
Copy link
Copy Markdown
Contributor

I've did this change in #55652. @switschel you might want to try out that branch.

@switschel
Copy link
Copy Markdown
Author

Hi @michaelarnauts ,
sound reasonable to me. I would suggest to make one additional change in your branch: On "turn_on" the preset_mode should be set to "auto" if not provided (no change in fan speed on this case). Can be overwritten providing "preset_mode" and "fan speed".
On "turn_off" the "preset_mode" should be set to "manual" and CMD_FAN_MODE_AWAY should be set. Unfortunately you cannot provide the preset_mode or any time on turn_off service to overwrite this like it is in the app where you can define a time.

Otherwise turning on and off the fan will result in strange behaviors like it is now. E.g. if you set it off, it will go again on after 2 hours which is only desired when you keep the preset_mode manually on Auto.

@michaelarnauts
Copy link
Copy Markdown
Contributor

Hi @michaelarnauts ,
sound reasonable to me. I would suggest to make one additional change in your branch: On "turn_on" the preset_mode should be set to "auto" if not provided (no change in fan speed on this case). Can be overwritten providing "preset_mode" and "fan speed".
On "turn_off" the "preset_mode" should be set to "manual" and CMD_FAN_MODE_AWAY should be set. Unfortunately you cannot provide the preset_mode or any time on turn_off service to overwrite this like it is in the app where you can define a time.

Otherwise turning on and off the fan will result in strange behaviors like it is now. E.g. if you set it off, it will go again on after 2 hours which is only desired when you keep the preset_mode manually on Auto.

Hmm, I don't follow. In case the preset is manual and the speed is off, it will stay off (off=away), when you change the speed to low, you want it to stay like that. If you automatically switch to auto then, depending on the configuration of the ventilation, it will move to normal after two hours, and since you've put the preset in manual before, this is not what you want.

Turning on and off is consistent now, when in manual, it will stay like it is, when in auto, it will fallback after a hours to the default mode (decided by the schedule and sensors of the device).

@switschel
Copy link
Copy Markdown
Author

switschel commented Sep 10, 2021

It's a usability thing. As a "normal" HA User you just go to your Dashboard and turn off/on the fan via the toggle switch (basically you don't change the preset mode in that case). With the current implementation you have to do two things: Preset Mode to manual and fan off (persistent away mode as no schedule can be provided) and vice versa preset mode to auto and turn on.
With automations you can do such logic but I doubt a "normal" user would do the two step approach described above within the dashboards, more likely he will simply switch the toggle button and might be surprised that the away mode will switch back to normal speed after 2 hours or if he turns the fan back on via toggle button the fan will go to low speed and not back to Auto.

I don't like this line (177-178):

 if percentage is None:
            cmd = CMD_FAN_MODE_LOW

And also here the preset mode should be changed not only the set_percentage:

    def turn_on(
        self,
        speed: str | None = None,
        percentage: int | None = None,
        preset_mode: str | None = None,
        **kwargs,
    ) -> None:
        """Turn on the fan."""
        self.set_percentage(percentage)

@michaelarnauts
Copy link
Copy Markdown
Contributor

Your usability remark also happens in the mobile app then. When in auto mode, and you switch the fan speed, it will revert back. This even happens when you use the remote of the unit and switches to a different fan speed. If you want to use manual control, the preset mode should be on manual, otherwise, the fan will always revert back to the schedule after two hours. In your use-case, you'll want to have the preset mode to manual, and then it will behave as you describe.

Note that even in manual mode, you can still configure the sensors to override this and increase the fan speed when high humidity is detected. You can configure this in the app.

The whole purpose of auto mode and manual mode is enable this fallback or not, we should not switch this mode when changing the fan speed. It defeats the purpose.

Regarding this if statement:

 if percentage is None:
            cmd = CMD_FAN_MODE_LOW

I guess it has been added when the change from distinct speeds to percentages has been done in the whole codebase. I'm not sure when we don't get a percentage actually. I just left it there to be sure.

@michaelarnauts
Copy link
Copy Markdown
Contributor

See #55652

@switschel
Copy link
Copy Markdown
Author

switschel commented Sep 10, 2021

I agree with you according to change of fan speed. No change in preset modes here needed.
BUT in HA Dashboards you have only a toggle button "on/off" which you don't have in the Mobile App. My suggestions are only for turn off and on.
As is:
off = Away Mode. In the App you can & must specify the time for how long you are away. This is currently not possible in HA. So Away for 2 hours currently. Inconsistent for a HA Dashboard user in my view.
on = Fan Mode Low for 2 hours (see below). This is inconsistent. On should be to switch back to "Auto" in my view.

I guess it has been added when the change from distinct speeds to percentages has been done in the whole codebase. I'm not sure when we don't get a percentage actually. I just left it there to be sure.

As written, it will be executed when you use the toggle button "on" in the Dashboard which might be used by the most users.

@michaelarnauts
Copy link
Copy Markdown
Contributor

What do you mean with HA Dashboard? In Home Assistant it can also show a slider just fine.

Screenshot from 2021-09-10 12-08-09

@switschel
Copy link
Copy Markdown
Author

It can but if you add the entity in a dashboad you get a toggle button.
image

@michaelarnauts
Copy link
Copy Markdown
Contributor

I'll take a look at it further, since it's described here that

Manually setting a speed must disable a preset mode. If it is possible to set a percentage speed manually without disabling the preset mode, create a switch or service to represent the mode.

(https://developers.home-assistant.io/docs/core/entity/fan#preset-modes)

@github-actions github-actions bot locked and limited conversation to collaborators Sep 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants