PINGRESP ignored in umqtt.simple.py

[nfj25]

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?

[michaeka79]

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.

[andrewleech]

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?

[michaeka79]

+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!

[michaeka79]

@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.

[andrewleech]

I've never seen that library myself, to be honest I haven't used mqtt at all.

[vitaliishchudlo]

@michaeka79 Maybe you have any ideas to fix it?
@nfj25 did you solve your problem?

[nfj25]

Not using it anymore...

[trescenzi]

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

Ping server (response is automatically handled by wait_msg（））

That implies to me not that the ping is ignored, but that wait_msg will tell you a ping has been responded to.

I've forked this and swapped the None return for a b"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.

[andrewleech]

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.

[trescenzi]

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 ping code doesn't seem as useful as it could be.