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

Improve climate turn_on/turn_off services for zwave_js #109187

Merged
merged 14 commits into from Feb 13, 2024

Conversation

raman325
Copy link
Contributor

@raman325 raman325 commented Jan 31, 2024

Breaking change

For Z-Wave climate entities, the behavior of the climate.turn_on service has changed. Previously, the service would act in the following order depending on whether the corresponding conditions were met:

  1. If the entity supported the off mode and exactly one additional mode, climate.turn_on would set the mode to the additional mode.
  2. If conditions for 1 were not met and at least one of the following modes is available, the service would set the mode to the first mode it found in specified order: heat_cool, heat, cool
  3. If conditions for 1 and 2 were not met, the service would silently do nothing.

Now, the service follows the following behavior in order:

  1. If the entity supports the resume thermostat mode, it will be used to restore the last mode you used before the entity was turned off.
  2. If the condition for 1 isn't met and the entity was turned off in Home Assistant, and Home Assistant wasn't restarted, Home Assistant remembers the last "on" mode and will set it to that mode. Home Assistant will be unable to set the entity to the previous mode if Home Assistant starts with the entity already in off mode since the integration will not know what the previous mode was.
  3. If conditions for 1 and 2 are not met and at least one of the following modes is available, the service sets the mode to the first mode it finds in specified order: heat_cool, heat, cool (no change to condition 2 above)
  4. If conditions for 1, 2, and 3 are not met, the mode will be set to the first supported mode it finds (e.g. dry or fan_only)

Proposed change

The trigger for this PR was the change that requires integrations to explicitly add supported features for TURN_ON and TURN_OFF. While making this change, I realized that I could improve the existing fallback logic for these services in the ClimateEntity by defining my own turn on and turn off methods. You can see a description of the changed behavior in the breaking changes section.

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)
  • 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 #
  • 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
  • I have followed the perfect PR recommendations
  • 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:

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

To help with the load of incoming pull requests:

@home-assistant
Copy link

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

Code owner commands

Code owners of zwave_js 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 zwave_js 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.

@raman325 raman325 changed the title Set zwave_js ClimateEntityFeature Properly set zwave_js ClimateEntityFeature.TURN_ON and TURN_OFF Jan 31, 2024
@raman325 raman325 marked this pull request as draft January 31, 2024 06:46
@raman325

This comment was marked as resolved.

@raman325 raman325 changed the title Properly set zwave_js ClimateEntityFeature.TURN_ON and TURN_OFF Improve climate turn_on/turn_off services for zwave_js Jan 31, 2024
@raman325 raman325 marked this pull request as ready for review January 31, 2024 08:14
@raman325 raman325 requested a review from a team as a code owner January 31, 2024 08:14
Copy link
Contributor

@emontnemery emontnemery left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add tests to the climate integration to ensure the new flag works as intended

@home-assistant
Copy link

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

Learn more about our pull request process.

@home-assistant home-assistant bot marked this pull request as draft January 31, 2024 08:46
@raman325
Copy link
Contributor Author

Once #109195 is merged we just need to merge dev into this PR and it will be ready to review

@emontnemery emontnemery marked this pull request as ready for review January 31, 2024 15:34
@raman325 raman325 dismissed emontnemery’s stale review January 31, 2024 16:04

no longer relevant since the change mentioned was merged in a separate PR

@raman325 raman325 marked this pull request as draft February 1, 2024 03:08
@raman325 raman325 marked this pull request as ready for review February 1, 2024 20:28
@raman325 raman325 merged commit c1d61b9 into home-assistant:dev Feb 13, 2024
23 checks passed
@raman325 raman325 deleted the climate branch February 13, 2024 00:06
astrandb pushed a commit to astrandb/core-akefork that referenced this pull request Feb 13, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Feb 14, 2024
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.

None yet

3 participants