Discussion: #develop branch #219
Comments
|
A sample sketch with the development code: Master branch: A similar library whose owner is quite patronizing (API differs a bit, so sketch is not 100% the same): |
|
I added https://github.com/marvinroger/async-mqtt-client.git#develop to the platformio.ini of my https://github.com/luebbe/homie-node-collection compiled and uploaded the sensor-nodes demo to a d1 mini. This one contains a lot of nodes and many mqtt messages (84 topics) have to be sent upon startup. I'm running mosquitto on a raspberry Pi 1 and MQTT Explorer on Windows for monitoring. The only notable difference when the device is reset is that it seems to take longer until all the initial messages have been received by MQTT explorer. I guess this is to be expected since they are queued up. Perhaps before messages were lost when the buffer was full. I have also added "freeheap" to $stats in my fork of homie-esp2866, so I can monitor if anything runs away there. The graph looks like this (around 15 minutes of history) There is a significant drop directly after the start (expected) and then it is stable at around 30KB. You can also see the drop at the five minute intervals when sensor data is sent. It is also interesting to see that when the $stats are sent, they now trickle in over the course of 1-3 seconds. With the previous version they came in the same second. |
|
In which qos are you sending? One notable difference in sending qos > 0 is that "the next message" is only sent when the previous has been acknowledged. I didn't want the logic to be too complicated to ensure proper message ordering (as per mqtt spec). In qos 0 this should not matter, but I might be wrong. I changed the logic a few times. |
|
Homie default. I didn't change anything. MQTT Explorer says qos 0? |
qos subscribing might differ from qos publishing. Stats are sent qos 1 (https://github.com/homieiot/homie-esp8266/blob/9cd83972f27b394eab8a5e3e2baef20eea1b5408/src/Homie/Boot/BootNormal.cpp#L180). So yes, this is slower than before. |
|
The homie convention states https://homieiot.github.io/specification/#qos-and-retained-messages that it uses qos 1, but I have to check the source to confirm. I don't know from where MQTT Explorer gets the qos 0 info. |
guess so
Fine by me. Better slow & certain than quick & dirty I'll leave my dev board and the monitor running at least until I go to sleep, but I'm already pretty confident that there's no memory leak. |
|
I'll anyhow add speed up to the wishlist. |
|
Just FYI, the sensor-demo has 84 subtopics. This is the second largest amount of my homie based applications. |
|
For me this results in a crash. Please see attached serial output from my relays-device. I am using SSL encrypted connection. |
|
Do you run your ESP on 160MHz? A soft watchdog reset on the sys stack during connecting is probably caused by the handshake taking too much time. Off course I can be wrong. Edit: for the record, I am setting up TLS on my broker to test myself. |
|
It is a standard ESP12-F. This time with decoded crash: |
|
gotta look into that. But still, it's a watchdog reset, so something is taking too long. |
|
I think, the connection to the MQTT broker does not work at all: And with some debug enabled, I see So, is somehow the connect string not sent completely? Will try to sniff network traffic. |
|
It's been a while since I've used TLS on the esp8266. How did you solve this error: |
|
Still running fine after 3,5 hours. FreeHeap still at 30k, goes down to 29,4k .. 29,6k when values are sent and always goes back to the same max value. |
me-no-dev/ESPAsyncTCP#151 <- that requires a fix from @mhightower83, which is not merged for a 3/4 year now. Sorry, enough for me for today. |
|
Shoot me, with logging enable it gives this output: So the TCP lib is not returning the actual written payload when using encryption??? |
|
Should TCP library normally return exact the amount of bytes, that have been sent? I saw the similar behaviour some comments above. |
|
It does, but only the non-tls. I guess it returns the encrypted TCP payload size. If you ask me, this a bad API design. |
|
@luebbe To speed up Homie you can use |
|
I'm thinking about a workaround for TLS. It wont be pretty as it is impossible to know how much data actually has been sent: We can wait to send for the entire TCP buffer to be empty. Then copy a fixed amount to LWIP and "ack" our own data. EDIT: Thinking about it... I might keep 2 sets of counters: one from tcp and one of our own and keep them in sync. I will work it out in my head while clearing the snow from the driveway. |
|
I've added the hacky solution for TLS. I've tested it with the example in the repo + added a 2000 byte message for chuncked sending. Did it like described above. |
|
It works! |
|
Yes, there is room for improvement. I didn't want to touch the existing code too much. _acked is keeping track of the acked data in the mqtt client. But since we're copying the data to TCP, we can already consider it acked. On non-tls versions, _acked is only incremented when the real zck comes in. I also realized I can use client->space() instead of the fixed number. |
|
I'm going to give it a rest for a week or two (covid restrictions a eased a bit, whoohoo). I'll keep an eye on the issue and PR tracker and read whatever is going on. I'll keep supporting! Meanwhile I'll think about possible improvements:
|
|
Awesome work so far, I must say! Thank you very much @bertmelis ! |
|
I'll go through old issues backwards and check if I can reproduce them. If not I'll tag them with "to be closed" and we can review together. Won't test TLS related stuff though. I didn't want to jump through that hoop yet. |
|
@luebbe I can do the TLS related stuff. |
Thank you, here, the line number isn't displayed. Furthermore, when I enable logging, the error is gone. Now, if the line number is correct, it is on the |
|
Well the complete stack trace is: but - because it's from my MQTTVPN I don't think that it will help... |
|
Got it. I thought I could be smart but I am not. Will push in a sec. Strangely, on my stack trace, the line numbers don't show up. It could have helped me A LOT. |
you need to compile with -g or use |
|
So with the new develop the crash is gone. Unfortunately it disconnects from the broker every ~24.9 seconds. |
|
You keepalive is probably standard, 15 secs? |
|
Yes, I didn't modified it. |
|
The reason reported in |
|
I do have some another observations. In the onConnect callback I publish to some topics like this: String info;
info.reserve(512);
info = F("{\"hw\":\"");
info.concat(F(HW_NAME "-" HW_VARIANT));
info.concat(F("\",\"app\":\""));
info.concat(F(APP_NAME));
info.concat(F("\",\"appversion\":\""));
info.concat(F(APP_VERSION));
info.concat(F("\",\"ip\":\""));
IPAddress ip = mqtt_if->netif.ip_addr;
info.concat(ip.toString().c_str());
info.concat(F("\"}"));
sprintf_P(buff, PSTR("%sinfo"), mqtt_if->service);
mqtt_if->mqttcl.publish(buff, 0, true, info.c_str());
sprintf_P(buff, PSTR("%sresetreason"), mqtt_if->service);
mqtt_if->mqttcl.publish(buff, 0, true, ESP.getResetReason().c_str());
mqtt_if->mqttcl.publish(mqtt_if->will, 1, true, online);Publish to the /info and /resetreason topic seems to be ok (QoS 0), publish to the |
|
what is this online variable? |
|
Just character string: static const char online[] = "1";
static const char offline[] = "0"; |
|
Can't reproduce unfortunately. Is your code public? |
|
Unfortunately not yet :-( . Maybe I can publish it at the weekend (at least some version...). |
|
I messed up again with the packetId. Apparently you can have the same member variable in a derived class as in a base class without any compiler warning? |
|
I didn't notice that - I'll check through the code. Fortunately the last push did the trick - everything seems to be working again. Including the web access - didn't test the OTA so far. |
|
Strangely enough the packetId errors didn't show up before I simplified the code. The error was there though. Anyway, done for this week. Gotta do some other things now. Next week I'll be trying to speed things up by having more messages qos>0 in-flight. |
Got it all working. I had to switch to a different ESP32 partition configuration. Anyway, latest |
|
Thanks. It's not over yet. The to do:
When that's done, I propose to merge the improvements to |
|
Nice. There's probably a few more things we can iron out too. I was looking at Phil's comments on AsyncMQTTClient that triggered him to create Pangolin. They are still valid concerns and it would be good to address them and mark them down as 'enhancements' in the GitHub so at least we have each feature logged. |
|
Do you happen to "know" which remarks are left? I'll also dig up his old rant as see what's left (I remember some of his remarks were as cryptic as the code in his mqtt client). |
|
Most of the original remarks were removed but you can trace it back here, hidden in the now removed files. Short summary:
It's a shame that Marvin's library, which is widely used and works fine for most people's needs to get slammed. The whole point of open source is to collaborate and improve each other's code, instead of spinning off a competitive project and asking people to donate $. |
My extra thoughts:
|
I wonder if this is related to this sort of issue? xoseperez/espurna#2166 (comment) Other buffer / discard points seem to be related to add() / send() usage, since it is possible to get stuck when network buffers are full and 'lose' outgoing messages Thx for fixing keepalive btw :) |
Something like this: #234 |
|
To be clear. This is how the
What I'm thinking of now is
|
|
Just a sign of life. I'm still alive 😂 I'm just very low on computer time. |
|
Sorry that I might be spamming as I have nothing to add, but I just wanted to give a huge thanks for the initiative to continue this project as it is the best one out there. Kudos @bertmelis, kudos. |
|
Merged and released. Closing. |
I'm been doing some work on the https://github.com/marvinroger/async-mqtt-client/tree/develop branch lately.
It should solve lots of the current issues. The main improvement is that outgoing messages are queued and the memory is only freed after the TCP ack allowes to do so.
❗ : although the API remained unchanged, the exlamation mark denotes a change in behaviour.
I'm looking forward to feedback. I do like positive feedback 😄, but I'm more interested in the negative ones. I still have to go through the list of mandatory statements of the MQTT specification. TLS is also untested but there has been no change in that part of the code.
The text was updated successfully, but these errors were encountered: