-
Notifications
You must be signed in to change notification settings - Fork 652
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
Improve MQTT #2091
Conversation
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)
Good to see that you're addressing the MQTT HA discovery topic :-) 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.
Cool 👍 |
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 |