uMQTT_Simple suggestions: #1: Return ping result instead of None & #2: Make socket write under 'disconnect' & 'ping' non-blocking
Suggestion (#1) for a small change in line 205 ('section' #PINGRESP in function wait_msg):
return res[0] instead of return None
This allows you to check in your own consumer code that called check_msg() or wait_msg(), that the ping response came by and was valid, e.g.: if(oReply == 0xD0): # valid ping response
And suggestion (#2) for another small change in line 122 (section 'ping') and line 118 (section 'disconnect'):
add line self.sock.setblocking(False) in front of self.sock.write(b"...")
This prevents lóóóng (default 30 secs?) lock-ups when the connection is lost (or not ready yet).
I haven't experienced a down-side yet of this non-blocking write (with and without a working connection).
P.s. I guess the problem is only valid when using SSL, as the (plain) socket is otherwise already initiated with specified timeout at 'connect' and may not stall that long. Once wrapped in an SSL socket though, the original timeout seems either lost or ineffective. Attempting to use settimeout(t) instead of setblocking(False) here before writing to the socket, is currently not possible with SSL sockets in micropython (see this request)
Thanks!
PINGRESP ignored in umqtt.simple.py
Hello,
In wait_msg, the PINGRESP message is ignored:
if res == b"\xd0": # PINGRESP
sz = self.sock.read(1)[0]
assert sz == 0
return None
Shouldn't this return some information so that the calling code can determine if the server is indeed responding to pings?
I'm going to have to upvote this... I feel like this should be the case as well. umqtt.robust doesn't fix this either. We need to know if the connection is still active or not.
I found some code on https://github.com/AntonisKekempanos/SonoffMicropythonMQTT.git
Where they return b'PINGRESP' this seems like an appropriate return code for the ping.
I don't know much about mqtt however from a quick search it appears that clients (like this module) are supposed to send a PINGREQ to the broker regularly, to which the broker responds with PINGRESP.
Changing wait_msg to return something different when this comes in would affect existing users of the library who haven't had to expect this message and know how to deal with it.
Perhaps instead of returning something, The library could have a new attribute in the mqtt client class where a timestamp is updated every time one of these comes in, that way an application that cares about it can query that attribute to see how long it's been since the last server communication?
+1 I love that idea! That would help with about half the code I'd be trying to write to check the time intervals between pings. So yes, love it!
@andrewleech were you thinking of https://github.com/fizista/micropython-umqtt.simple2.git when you suggested having it be an attribute of the client? Because it seems like that's exactly what's been implemented.
I've never seen that library myself, to be honest I haven't used mqtt at all.
@michaeka79 Maybe you have any ideas to fix it?
@nfj25 did you solve your problem?
Not using it anymore...
Spent some time trying to figure out why pings weren't working before finding this issue. Is there a rationale behind this? The docs even seem to imply that this should be handling the ping
That implies to me not that the ping is ignored, but that
wait_msgwill tell you a ping has been responded to.I've forked this and swapped the
Nonereturn for ab"PINGRESP", as someone suggested above, and it seems to work fine. However I'm not using any subscriptions on my end so maybe this messes that up? If it does maybe we could instead set a flag on the client that indicates the ping has been responded to and then clear it upon the next ping?I'm happy to make either change, they are very straightforward, not sure though if this is actually a desired change.
My understanding is that none of the current owners / maintainers of micropython-lib really use or know much about mqtt - hence this library hasn't seen any real work or expansion.
As such I really don't know if there's anyone who could really answer your question about the correct way to handle this, or what should be done to fix it.
I opened a pr with the solution I've been using locally. It's minimally invasive and non-breaking which is why I went with this approach although I'm not necessarily the most python oriented dev so it's possible this is an unusual approach in python. But I wanted to propose it since this issue has been open for some time and the current
pingcode doesn't seem as useful as it could be.Yes, there is some thing to change her. Maybe the doc, or the code. but I passed half a day searching to test
if is connectedFinally, my function
but not sure what append if server is down.