execute MQTT client synchronously in main loop() #350
Conversation
processing a published valued on a subscribed topic is currently running in a task that is not the task executing the main loop(). that's because the espMqttClient(Secure) was constructed without arguments, which selects the constructor with two arguments priority and core, both of which have default values. that constructor selects espMqttClientTypes::UseInternalTask::YES, causing a task to be created in which context the MQTT client loop is executed. MQTT subscribers assume they are running in the same context as the main loop(). most code assumes exactly that. as the scheduler is preemptive and very little (none at all?) code is interlocked, we have to make sure to meet the programmer's expectations. this changeset calls the MQTT client loop in the context of the main loop() and enforces the use of espMqttClientTypes::UseInternalTask::NO.
|
@helgeerbe Thanks for trusting me on this one. However, please let's test this for a while and wait for @the-lonely-one's feedback in #268 before releasing. I consider this a significant design change. My productive OpenDTU-OnBattery includes this change and it is running for 19h now. The only MQTT I use is for importing the power meter values, which seems to work without issues. |
|
@schlimmchen too late, sorry for that. I tested it myself an it worked so far. If there is an issue I will revoke this merge. To avoid unwanted merges in the future. Set your PRs to draft. |
Not that I want to argue, but I really though that I did... I don't see any hint in the timeline that this ever was a draft PR, so I guess I failed. I am confident that the change does not break stuff, so there should be no reason to pull the release. I'll still wait for more feedback until creating a PR against the upstream project. |
|
Hi @schlimmchen, I merged code from the upstream, where a Thread Safe Queue is introduced for the hoymiles inverters. Does this has any impact on this PR? |
|
@helgeerbe, I am unsure what you want to know.
As I was suspecting and as tbnobody confirmed, the AsyncWebserver causes the same problem: A concurrent task that poses all sorts of problem in our non-thread-safe codebase. Also, remember that in #308 people suspected that using the web app heavily (for a longer time and/or on multiple clients) causes instability? Would you agree to castrate the webserver to run synchronously in the main |
|
For future reference: This change is effectively reverted as of #556. |
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new discussion or issue for related concerns. |
processing a published valued on a subscribed topic is currently running in a task that is not the task executing the main loop(). that's because the espMqttClient(Secure) was constructed without arguments, which selects the constructor with two arguments priority and core, both of which have default values. that constructor selects espMqttClientTypes::UseInternalTask::YES, causing a task to be created in which context the MQTT client loop is executed.
MQTT subscribers assume they are running in the same context as the main loop(). most code assumes exactly that. as the scheduler is preemptive and very little (none at all?) code is interlocked, we have to make sure to meet the programmer's expectations.
this changeset calls the MQTT client loop in the context of the main loop() and enforces the use of espMqttClientTypes::UseInternalTask::NO.
This aims to solve @the-lonely-one's problem described in #268 and a merge request against the upstream project will be created if this solves the problem.