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

Improve MQTT #2091

Merged
merged 7 commits into from
Feb 27, 2023
Merged

Improve MQTT #2091

merged 7 commits into from
Feb 27, 2023

Conversation

caco3
Copy link
Collaborator

@caco3 caco3 commented Feb 27, 2023

  • Check that all topics sent successfully
  • Only send Homeassistant Discovery the first time we connect. If some topics fail, it retries in the next round. Once all topics got set successfully, it no longer sends them automatically (still possible through the REST API).
  • Fixed default values if no MQTT Client ID or maintopic got defined in config.
  • Default MQTT Client ID is now the hostname (was "AIOTED-", but that is longer tan the allowed 23 bytes)

This was referenced Feb 27, 2023
@jomjol jomjol merged commit a1a77ae into rolling Feb 27, 2023
@jomjol jomjol deleted the improve-mqtt branch February 27, 2023 17:26
@Slider0007
Copy link
Collaborator

  • Only send Homeassistant Discovery the first time we connect. If some topics fail, it retries in the next round. Once all topics got set successfully, it no longer sends them automatically (still possible through the REST API).

Good to see that you're addressing the MQTT HA discovery topic :-)
I just briefly checked your approach. This is for sure a step in the right direction and improve the situation but unfortunately in worst case it's still remained the same. Reconnect of wifi can happen at any time and if some messages weren't processed at first attempt (which also could happen during flow when wifi is not connection directly after startup) they possibly need to be reprocessed during running flow (GotConnected function gets called after reconnect of wifi and not at the end of each round).
Additionally the REST API can also trigger discovery sending at any time.

To ensure stability at any situation we should take care of worst case scenario.

Having the goal in mind to implement to wifi roaming and have still room for further improvemnts, we should try to avoid sending HA discovery during main flow steps at any kind, so my suggestion is to trigger discovery only when flow is finished with a button. This would be perfectly deterministic and controlable and would garantuee more free heap. IMHO the drawback is really little because discovery is only used once for initial HA setup.

My first heap analysis showed, only ca. 3kB of internal heap left with activated wifi roaming even with tightening some other tasks. This means with activating roaming further implementation are hardly possible without the risk of having same situation than with v13.x. Keeping this HA discovery design like it is even with this optimization I do not recommend wifi roaming implementation due to lack of heap.

  • Fixed default values if no MQTT Client ID or maintopic got defined in config.
  • Default MQTT Client ID is now the hostname (was "AIOTED-", but that is longer tan the allowed 23 bytes)

Cool 👍

@caco3
Copy link
Collaborator Author

caco3 commented Feb 27, 2023

you are right, I completely missed that point, sry!

My focus was on the missing default values and then the other change was just short but missed the overall picture.

Lets discuss that topic in #2092

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