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

Always retain Home Assistant MQTT Discovery config topic #777

Merged
merged 4 commits into from
Nov 22, 2023

Conversation

bachya
Copy link
Owner

@bachya bachya commented Nov 14, 2023

Describe what the PR does:

When ecowitt2mqtt loses connection to the MQTT broker for some reason (the broker going down, network issues, etc.) and the broker hasn't set retain, the following bug can happen:

  1. ecowitt2mqtt starts up.
  2. When the first payload arrives, it connects to the MQTT broker and publishes it.
  3. ecowitt2mqtt detects that it hasn't sent MQTT Discovery info before, so it does; this creates the config topic + underlying data.
  4. In this scenario, at some point, ecowitt2mqtt loses connection to the MQTT broker. Without retain set, this effectively wipes all ecowitt2mqtt data from the broker's memory.
  5. ecowitt2mqtt reconnects.
  6. Another payload arrives. Because ecowitt2mqtt has been running this whole time, it sees this payload and says, "I've already published config info for these entities, so I won't do it again." This means that it fails to republish the config topic.

To address all possibilities of this issue, we set the config topic to always retain.

Does this fix a specific issue?

Fixes #760

Checklist:

  • Confirm that one or more new tests are written for the new functionality.
  • Run tests and ensure everything passes (with 100% test coverage).
  • Update README.md with any new documentation.

@bachya bachya added the bug Bugs or issues which will cause a problem for users label Nov 14, 2023
@bachya bachya self-assigned this Nov 14, 2023
Copy link

codecov bot commented Nov 14, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (2e04ac3) 100.00% compared to head (999925d) 100.00%.

Additional details and impacted files
@@            Coverage Diff            @@
##               dev      #777   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           31        31           
  Lines         1759      1759           
=========================================
  Hits          1759      1759           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bachya bachya force-pushed the hass-first-publish branch 2 times, most recently from 3b3069d to 9b64dd9 Compare November 20, 2023 19:56
@bachya bachya changed the title Fix issue where MQTT Discovery fails on broker reconnection Always retain Home Assistant MQTT Discovery config topic Nov 21, 2023
@bachya bachya force-pushed the hass-first-publish branch 2 times, most recently from a8ad68f to 7248d09 Compare November 21, 2023 22:13
@bachya bachya force-pushed the hass-first-publish branch from 7248d09 to b406360 Compare November 21, 2023 22:48
@bachya bachya merged commit 863401d into dev Nov 22, 2023
12 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Nov 24, 2023
@bachya bachya deleted the hass-first-publish branch January 19, 2024 14:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Bugs or issues which will cause a problem for users
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrading to 2023.11.0 Breaks Publishing
1 participant