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: water_heater: do not set target_temperature_high and target_temperature_low #289

Merged
merged 1 commit into from
Aug 27, 2024

Conversation

Zkdc
Copy link
Contributor

@Zkdc Zkdc commented Aug 18, 2024

PR Description

Reason & Detail

these values are for target temperature range. It is obviously incorrect to set these values to min_temp and max_temp. Since we can not get/set the device's target temperature range, we should not set these values.

Related issue

…erature_low

these values are for target temperature range. It is obviously incorrect to set these values to min_temp and max_temp. Since we can not get/set the device's target temperature range, we should not set these values.
@wuwentao
Copy link
Owner

@Zkdc thank you very much for you PR.
we still need more contributors for current project, welcome to join us.

for this code chanages, we should follow to the official HA developers guide:
https://developers.home-assistant.io/docs/core/entity/water-heater

seems it match the defines, please help to double check.

@wuwentao
Copy link
Owner

@Zkdc any updates?

@Zkdc
Copy link
Contributor Author

Zkdc commented Aug 26, 2024

@Zkdc thank you very much for you PR. we still need more contributors for current project, welcome to join us.

for this code chanages, we should follow to the official HA developers guide: https://developers.home-assistant.io/docs/core/entity/water-heater

seems it match the defines, please help to double check.

Sorry For The Late Reply.

  1. According to the discussion in Support TARGET_TEMPERATURE_RANGE for water heater entities home-assistant/architecture#995 (reply in thread) , these values are hysteresis control parameters.
    Here (https://esphome.io/components/climate/bang_bang.html) is a type of controller which uses these values, it heats when temperature is below target_temperature_low and stop heating when reaching target_temperature_high.

  2. When not setting these values, it shows state & current temperature on the interface.
    image
    Otherwise, it shows the state & target temperature range on the interface.
    image
    Since we are setting incorrect values, it becomes less informative.

@rokam
Copy link
Collaborator

rokam commented Aug 26, 2024

@wuwentao this change makes sense as all water heaters uses TargetTemp and not RangeTemp.
This will keep the default value (None): https://developers.home-assistant.io/docs/core/entity/water-heater/

Copy link
Collaborator

@rokam rokam left a comment

Choose a reason for hiding this comment

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

LGTM

@wuwentao
Copy link
Owner

thanks, I can understand it with your detail answer: it heats when temperature is below target_temperature_low and stop heating when reaching target_temperature_high.

@wuwentao wuwentao merged commit 8dc9ff3 into wuwentao:master Aug 27, 2024
10 checks passed
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