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

climate entity incorrectly reports integration as implicitly supporting turn_on/off without setting the proper ClimateEntityFeature #114286

Closed
make-all opened this issue Mar 27, 2024 · 9 comments

Comments

@make-all
Copy link

The problem

My integration explicitly supports turn_on and turn_off methods, and also sets the ClimateEntityFeature.TURN_ON and TURN_OFF flags in its supported_features. However users are constantly reporting that HA is outputting a log message saying

Entity None (<class 'custom_components.tuya_local.climate.TuyaLocalClimate'>) implements HVACMode(s): off, heat_cool, cool, dry, fan_only, heat and therefore implicitly supports the turn_on/turn_off methods without setting the proper ClimateEntityFeature. Please create a bug report at https://github.com/make-all/tuya-local/issues

This is due to a missing condition on line 398 of climate/init.py, where it merely checks that the modes contain HVAC_OFF to output this message, without checking that the ClimateEntityFeature flag is set as the previous lines do, or whether the turn_off/turn_on methods are overridden by the integration, so the implicit fallback will actually be used.

What version of Home Assistant Core has the issue?

2024.3.3

What was the last working version of Home Assistant Core?

2024.1.6

What type of installation are you running?

Home Assistant OS

Integration causing the issue

climate

Link to integration documentation on our website

https://www.home-assistant.io/integrations/climate/

Diagnostics information

No response

Example YAML snippet

No response

Anything in the logs that might be useful for us?

Logger: homeassistant.components.climate
Source: components/climate/init.py:361
integration: Climate (documentation, issues)
First occurred: 10:46:18 (1 occurrences)
Last logged: 10:46:18

Entity None (<class 'custom_components.tuya_local.climate.TuyaLocalClimate'>) implements HVACMode(s): off, heat_cool, cool, dry, fan_only, heat and therefore implicitly supports the turn_on/turn_off methods without setting the proper ClimateEntityFeature. Please create a bug report at https://github.com/make-all/tuya-local/issues

Additional information

No response

@home-assistant
Copy link

Hey there @home-assistant/core, mind taking a look at this issue as it has been labeled with an integration (climate) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of climate can trigger bot actions by commenting:

  • @home-assistant close Closes the issue.
  • @home-assistant rename Awesome new title Renames the issue.
  • @home-assistant reopen Reopen the issue.
  • @home-assistant unassign climate Removes the current integration label and assignees on the issue, 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 issue.
  • @home-assistant remove-label needs-more-information Remove a label (needs-more-information, problem in dependency, problem in custom component) on the issue.

(message by CodeOwnersMention)


climate documentation
climate source
(message by IssueLinks)

make-all added a commit to make-all/tuya-local that referenced this issue Mar 27, 2024
We already effectively opt out by explicitly defining these functions, but
the braindead way the deprecation notices work mean we need to also opt out
with this secret flag.

Issue #1513, #1737, #1734, #1637, #1632
home-assistant/core#114286
@make-all
Copy link
Author

Although I have found what is apparently the official way to work around the issue, the issue of the log messages being incorrect still exists in Home Assistant as a bug, and will be affecting other integrations as well.

@PeteRager
Copy link
Contributor

Can you make a pull request to fix?

make-all added a commit to make-all/ha-core that referenced this issue Mar 28, 2024
Current log message when climate.turn_on/off are implemented implicitly
describes the wrong problem, and appears even when they are implemented
explicitly.

- tighten up the condition to only appear when a fallback to the default
  turn_on or turn_off methods will be used, as these are what will be
  removed at the end of the deprecation period.
- change the message to make it clear that it is the default turn_on/off
  that is the problem, not just the support flags (if the functions are
  implemented without the support flags, there is another log message).
make-all added a commit to make-all/ha-core that referenced this issue Mar 28, 2024
A message about implicit turn_on/off without feature flags was output even when the feature flags were set.
@TheJulianJES
Copy link
Member

Setting _enable_turn_on_off_backwards_compatibility = False after you migrated should prevent the fallback and error messages no matter what.

@make-all
Copy link
Author

That is the workaround that I installed in my integration, but it still does not fix the bug, which is that the wrong message is being logged under the wrong conditions.

@TheJulianJES
Copy link
Member

Yes, the error message seems to log incorrectly. Just wanted to mention that you should still set the value to False.

@make-all
Copy link
Author

After my proposed change, setting the "opt-out" variable is only necessary for integrations that do not want turn_on/off services implemented for some reason, even though they support HVAC_MODE_OFF and other modes.

Integrations that already set the support flags should not be required to set extra opt-out variables to work around an incorrect log message (which is the only effect of setting that variable). Adding that requirement just creates additional busywork for custom integration authors who often have very little time to keep up with such changes.

@gjohansson-ST
Copy link
Member

I am going to close this issue as it states explicitly in the blog post that integrations should set _enable_turn_on_off_backwards_compatibility to False once migrated regardless.

@gjohansson-ST gjohansson-ST closed this as not planned Won't fix, can't repro, duplicate, stale Mar 28, 2024
@make-all
Copy link
Author

Disappointing that defending your past mistake is more important to this project than quality of the information you are logging.

@github-actions github-actions bot locked and limited conversation to collaborators Apr 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants