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

Mqtt topics to support numeric fan speed #1859

Merged
merged 12 commits into from
Sep 22, 2021
Merged

Mqtt topics to support numeric fan speed #1859

merged 12 commits into from
Sep 22, 2021

Conversation

wifwucite
Copy link
Contributor

@wifwucite wifwucite commented Jun 3, 2021

What does this implement/fix?

Allows to set and query the exact (numeric) fan speed via two new MQTT topics.

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)
  • Configuration change (this will require users to update their yaml configuration files to keep working)

Related issue or feature (if applicable): fixes setting fan speed as a percentage value #1015

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

Test Environment

  • ESP32
  • ESP8266
  • Windows
  • Mac OS
  • Linux

Example entry for config.yaml:

fan:
  - platform: speed
    speed_level_state_topic: speed_level/state/topic # optional
    speed_level_command_topic: speed_level/command/topic # optional

Explain your changes

For fan speed there are already two topics (state & command) which support the three options LOW, MEDIUM, HIGH, but no numeric values. I have added two more topics called "speed_level" that support numeric values in the from 0 to the configured speed count. Since the old topics still exist, this change is backward compatible.
I also implemented the dump_config method of the MQTT fan component. And I made a generic change to configuration logging of MQTT components: If the base component is internal and hence the MQTT component stays inactive this indicated in the log. (I spent about 1h to find out what was going on to discover that my fan was implicitly internal because it had an id but no name. I would like to make that easier to discover for other people. ;-))

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:

@wifwucite wifwucite changed the title Mqtt numeric fan speed Mqtt topics to support numeric fan speed (in addition to low, medium, high) Jun 3, 2021
@wifwucite wifwucite changed the title Mqtt topics to support numeric fan speed (in addition to low, medium, high) Mqtt topics to support numeric fan speed Jun 3, 2021
@wifwucite wifwucite marked this pull request as ready for review June 3, 2021 14:24
@fhempy
Copy link

fhempy commented Jul 11, 2021

Any plans on adding this one to the next release or is there something missing?

@oxan
Copy link
Member

oxan commented Aug 20, 2021

Any plans on adding this one to the next release or is there something missing?

I think the biggest "issue" here is that this PR contains two (unrelated) changes. The numeric fan speed control is fairly straightforward and uncontroversial, but there's also the dump_config changes which are more fundamental and require more review, delaying the whole PR. So in the future I'd advise to split such changes into two PRs :)

W.r.t. the dump_config change, we usually abstract this the other way around: introduce a call_dump_config() method that determines whether dump_config() should be called, and keep the standard method in all derived classes. That's also the same way MQTTComponent decides whether to call setup() and loop().

@probot-esphome
Copy link

Hey there @glmnet, mind taking a look at this pull request as it has been labeled with an integration (ac_dimmer) you are listed as a code owner for? Thanks!
(message by CodeOwnersMention)

@probot-esphome
Copy link

Hey there @esphome/core, mind taking a look at this pull request as it has been labeled with an integration (adc) you are listed as a code owner for? Thanks!
(message by CodeOwnersMention)

@wifwucite
Copy link
Contributor Author

Thank you for your feedback. I agree that I have derived from the standard call_... pattern. And I do not want to introduce a "call_dump_config" method with this PR. However, I still remember that I was chasing a problem for 1h, because my component was implicitly internal (id but no name). So I would like to prevent other users from such a problem. I made a small change to dump_config only. I hope that is okay for you. :-)

@oxan
Copy link
Member

oxan commented Sep 22, 2021

#2325 added the call_dump_config() method and prevents the dump_config() on disabled MQTT components from being called, so that part of this PR is no longer necessary, and I've removed it.

@oxan oxan merged commit fd836e9 into esphome:dev Sep 22, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Sep 24, 2021
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.

setting fan speed as a percentage value
5 participants