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

Add option to use MQTT abbreviations #2641

Merged
merged 3 commits into from
Oct 31, 2021

Conversation

paulmonigatti
Copy link
Contributor

What does this implement/fix?

Adds the option to enable/disable abbreviations for MQTT Discovery. Using abbreviations frees up a small amount of flash, memory usage during discovery, and reduces network traffic ever so slightly.

MQTT abbreviations are enabled by default; this should work just fine with Home Assistant since the abbreviations in this PR have been adapted directly from HA source, however this will be a breaking change if used with other software that uses MQTT discovery but doesn't recognize these abbreviations.

Making this a non-breaking change is not as simple as disabling abbreviations by default, because the MQTT Climate component was already using abbreviations while other components were not.

If this PR is approved, I'll go ahead and update docs/tests.

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

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

Test Environment

  • ESP32
  • ESP32 IDF
  • ESP8266

Example entry for config.yaml:

# Example config.yaml

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

Happy with the changes, just cannot decide on default behaviour as both decisions make a change to existing behaviour

@oxan
Copy link
Member

oxan commented Oct 28, 2021

The MQTT discovery stuff is fairly HA-specific anyway, so I don't think we need to worry too much about other software using it, so I would just default to enabling usage of abbreviations.

@oxan
Copy link
Member

oxan commented Oct 28, 2021

One thing though, maybe it's better to use constexpr variables instead. That gives us namespacing and type checking, and from a quick look it compiles to the same.

So e.g.

constexpr const char* const ACTION_TOPIC = "action_topic";

@paulmonigatti
Copy link
Contributor Author

Awesome, thanks! I've changed the defines to constexpr, and updated the docs at esphome/esphome-docs#1582

@jesserockz
Copy link
Member

Only added breaking-change label so I know to mention it in next release just in case people are using the topics directly

@jesserockz jesserockz merged commit 331a3ac into esphome:dev Oct 31, 2021
@paulmonigatti paulmonigatti deleted the feat-mqtt-abbreviations branch October 31, 2021 03:06
@github-actions github-actions bot locked and limited conversation to collaborators Nov 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants