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

Do not enable SHT3x heater by default. Fixes #4886. #5445

Merged
merged 1 commit into from Sep 29, 2023
Merged

Conversation

jkl1337
Copy link
Contributor

@jkl1337 jkl1337 commented Sep 28, 2023

What does this implement/fix?

The SHT sensor heater is for special use and defaulting them to on breaks the vast majority of prior configs and seems quite surprising. This appears to have been an inadvertent side effect of PR #5161.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Other

Related issue or feature (if applicable): fixes esphome/issues#4886

Pull request in esphome-docs with documentation (if applicable): esphome/esphome-docs#

Test Environment

  • ESP32
  • ESP32 IDF
  • ESP8266
  • RP2040

Checklist:

  • The code change is tested and works locally.
  • Tests have been added to verify that the new code works (under tests/ folder).

If user exposed functionality or configuration variables are added/changed:

@jesserockz
Copy link
Member

From my understanding of the previous PR, the heater was always turned on anyway before the PR.
That PR simply allowed it to be disabled at startup instead.

Changing the default now would be a breaking change to everyone using it now.

@jkl1337
Copy link
Contributor Author

jkl1337 commented Sep 29, 2023

No, the original PR broke everything. Please see the issue #4886 filed and now the people piling up complaining that the latest release broke things.

The heater is only for special use cases. It should not be on for normal use of the sensor, please see data sheet for how it is used.

Apparently the original breaking PR author had a device that turned it on by default which is likely undesirable (also, note, not the norm the PR states “on my custom built PCB using the sensirion chips, the heater was enabled by default”) and they needed code to turn it off. Note their comment: "This is a good question. From the different discussions, I understood it should have been turned off by default." The original PR even says to allow disabling the heater. It's very confusing that the default was made to enable it. I would imagine the original PR author was testing with an explicit heater_enabled: False and missed that the default was incorrect.

@jesserockz
Copy link
Member

Ok understood. Happy to merge this change in but as its breaking and its easy enough for people to change yaml to false, it wont be released until 2023.10.0

@jesserockz
Copy link
Member

Could you also please make a docs PR targeting next to flip the default value there too?

Fixes esphome/issues#4886.

The SHT sensor heater is for special use and defaulting
them to on breaks the vast majority of prior configs and
seems quite surprising. This appears to have been an
inadvertent side effect of PR esphome#5161.
@kbx81 kbx81 merged commit b3dc2d4 into esphome:dev Sep 29, 2023
29 checks passed
clydebarrow pushed a commit to clydebarrow/esphome that referenced this pull request Sep 30, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Oct 1, 2023
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.

SHT3x now enables heater by default
3 participants