QUERY · ISSUE
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?
CANDIDATE · PULL REQUEST
umqtt.simple and umqtt.robust improvements
- Keep track of MQTT ping responses (PINGRESP) via MQTTClient.pingresp_pending attribute
- An attempt to send PINGREQ (by calling ping()) when we haven’t yet received a PINGRESP for our previous one will raise an exception
- MQTTClient.ping superclassed in mqtt.robust to make use of the pingresp_pending attribute. Reconnect will be performed on 'No response for previous ping’ exception
- MQTTClient.connect() will always try to close the previous socket (if any) first. Otherwise the reconnect() function from mqtt.robust might fail on trying to re-instantiate the socket with OSerr=23
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.