Skip to content
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

Fix fan modes #154

Merged
merged 4 commits into from
Jan 4, 2025
Merged

Conversation

gentoo-root
Copy link
Contributor

Fix the ELECTRA_AC workaround that became a no-op in de5a190 (not tested with the real hardware).

Prettify the way how fan modes are displayed in the UI for ACs that use min/medium/max instead of low/medium/high.

Replace self.fan_mode by self._attr_fan_mode, because the latter is used
everywhere else.
self._attr_fan_modes already contains values rewritten to FAN_HIGH and
HVAC_FAN_MAX. `if fan_mode not in self._attr_fan_modes`, it's impossible
to fail the `fan_mode != FAN_HIGH and fan_mode != HVAC_FAN_MAX` check.

Also fix a typo: "swing mode" -> "fan mode".
HVAC_FAN_MAX_HIGH and HVAC_FAN_AUTO_MAX are filtered out from
self._attr_fan_modes in __init__(). All the further checks for
HVAC_FAN_MAX_HIGH and HVAC_FAN_AUTO_MAX will always fail, making the
ELECTRA_AC workaround a no-op.

Fix it by storing the workaround status in self._quirk_fan_max_high.

Fixes: de5a190 ("Aggressive use of _attr_* properties, cleanup and normalization")
Home Assistant has standard high/low fan modes that are localized and
displayed in the UI in a pretty way. Tasmota IRHVAC has min, low,
middle, high and max, along with a few other modes. Min and max are
displayed in Home Assistant as is and not localized. While nothing can
be done on the integration side for air conditioners that use 5 levels
of the fan, some air conditioners have 3 levels, but use min/medium/max
instead of low/medium/high. For such ACs, convert min->low and max->high
to display pretty localized values in the UI.
@mateuszdrab
Copy link

mateuszdrab commented Aug 31, 2024

Hey @gentoo-root

This is a pretty nice PR as I did notice my min and max options are not localized. We're investigating some issues with ARGO decoding in here.

I was wondering if you could also add handling for a mid-high state that Tasmota reports for my ARGO AC as no existing supported_fan_speeds options work with it.

This unit's fan speeds are auto, min, med-high, high.

If not, I'll create a PR after this one merges so that I can adapt handling of med-high so it shows correctly in the UI along with low and max which you corrected.

@hristo-atanasov
Copy link
Owner

The proposed changes are breaking another tweak for ELECTRA_AC, listed below, which is tested on a real device. This is why you think there is a redundant checks in the code. It is not redundant, it is for limiting the fix only for those devices, that needs it. Please, rewrite your PR code, without breaking the existing fix.

  supported_modes:
  # Some devices have "auto" and "fan_only" switched
  # If the following two lines are uncommented, "auto" and "fan" shoud be commented out
  #- "auto_fan_only" #if remote shows fan but tasmota says auto
  #- "fan_only_auto" #if remote shows auto but tasmota says fan

  supported_fan_speeds:
  # Some devices say max,but it is high, and auto which is max
  # If you uncomment the following two, you have to comment high and max
  # - "auto_max" #woud become max
  # - "max_high" #would become high

@gentoo-root
Copy link
Contributor Author

The proposed changes are breaking another tweak for ELECTRA_AC, listed below, which is tested on a real device.

If it's tested and works on a real device, I'm curious to know how. One of my commits is actually supposed to fix ELECTRA_AC after a breaking change made in commit de5a190. As I explained in the commit message, the relevant ELECTRA_AC code flows became unreachable after de5a190, because HVAC_FAN_MAX_HIGH and HVAC_FAN_AUTO_MAX can't be in self._attr_fan_modes after __init__(), therefore all if HVAC_FAN_MAX_HIGH in (self._attr_fan_modes or []) and HVAC_FAN_AUTO_MAX in (self._attr_fan_modes or []) checks throughout the code are always false. Are you sure ELECTRA_AC was tested after commit de5a190? If yes, could you please explain where I'm wrong?

Assuming my understanding is correct, the code I removed in 8e4a1b7 can be removed safely:

  1. Removal of dead code doesn't change the behavior, so it can't be breaking anything.
  2. It's not needed after my next commit (which reenables ELECTRA_AC quirks) either, because the first check if fan_mode not in (self._attr_fan_modes or []) would cover if fan_mode != FAN_HIGH and fan_mode != HVAC_FAN_MAX, as both FAN_HIGH and HVAC_FAN_MAX will be in self._attr_fan_modes.
  3. (Not really relevant with the current code, given the above, but if the ELECTRA_AC-specific error message was reachable, it would print a wrong list of fan modes, missing the explicitly enabled ones.)

@gentoo-root
Copy link
Contributor Author

@hristo-atanasov, any updates from your side on the ELECTRA_AC quirk, after you've read my explanation?

@hristo-atanasov
Copy link
Owner

It is written for my ACs and tested on them. Now tell me what exactly doesn't work on your side and what needs to be done to make it work. I will figure out some way that both work ok in the same time.

@gentoo-root
Copy link
Contributor Author

what exactly doesn't work on your side

See 9e3ad34: lowercase "min" and "max" are shown in the UI instead of proper localized strings.

what needs to be done to make it work

Your integration should map HVAC_FAN_MIN<->FAN_LOW and HVAC_FAN_MAX<->FAN_HIGH when it's possible (i.e. when the AC doesn't use min, max, low and high simultaneously). This is implemented in my last commit 9e3ad34.

I don't remember all the context from 3+ months ago, but apparently I had to make some code cleanups in order to make the last commit look sane.

While doing the cleanups, I noticed that an earlier commit de5a190 (2024-06-10) made your ELECTRA_AC workaround a no-op, as I described in b772248: all checks for HVAC_FAN_MAX_HIGH in (self._attr_fan_modes or []) and HVAC_FAN_AUTO_MAX in (self._attr_fan_modes or []) return False now, because these items are filtered out in __init__.

Here I had two options:

  1. I could just remove the dead code (that became unreachable after de5a190). If you are reporting that your ACs work well even without the workaround, maybe it's the right way to go.
  2. I could restore the pre-de5a190 behavior, assuming that it's the way how things should work. That's what I did in b772248. I assumed that the previous code was needed, but the commit de5a190 broke it without anyone noticing, and decided to fix that.

So, if you don't like option 2, should I just remove the dead code altogether, given that you claim that your AC works fine even without these parts of code being run (one, two, three)?

@hristo-atanasov
Copy link
Owner

@gentoo-root I see your point. My point is, that I don't know how many people will be affected if I directly merge this PR. I have 5 AC relying on this "quirk", but lets say I'm not in a hurry and I can adapt it for myself even later, and then merge the adapted code back in the repo. As you and @mateuszdrab are the only people I know with ELECTRA_AC and you both have the issue I will merge the request and I will adapt it on my side. I'll be following the issues to see if someone will complain about the changes.

@hristo-atanasov hristo-atanasov merged commit 1838cbb into hristo-atanasov:master Jan 4, 2025
@gentoo-root
Copy link
Contributor Author

Thank you!

As you and @mateuszdrab are the only people I know with ELECTRA_AC

To clarify, I don't have an ELECTRA_AC myself (from your previous message, I thought you were the user of ELECTRA_AC). I'm just restoring the old behavior that used to be there before de5a190 turned the relevant parts into a dead code. If no issues have been reported since last summer, I wonder if anyone still uses it.

In any case, feel free to tag me if any new issue regarding ELECTRA_AC is reported.

@hristo-atanasov
Copy link
Owner

Thank you!

As you and @mateuszdrab are the only people I know with ELECTRA_AC

To clarify, I don't have an ELECTRA_AC myself (from your previous message, I thought you were the user of ELECTRA_AC). I'm just restoring the old behavior that used to be there before de5a190 turned the relevant parts into a dead code. If no issues have been reported since last summer, I wonder if anyone still uses it.

In any case, feel free to tag me if any new issue regarding ELECTRA_AC is reported.

I will first test it on my HA to see how it behaves, before I make a new version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants