commschamp / cc.mqttsn.libs

CommsChampion Ecosystem MQTT-SN client / gateway libraries and applications
Mozilla Public License 2.0
63 stars 16 forks source link

Client does not send PINGREQ if it receives packages #3

Closed a1lu closed 4 years ago

a1lu commented 4 years ago

Hi, I noticed that the client does not send PINGREQ packages if it receives packages in the keep alive period.

The MQTT spec states:

The client has a responsibility to send a message within each Keep Alive time period. In 
the absence of a data-related message during the time period, the client sends a
PINGREQ message, which the server acknowledges with a PINGRESP message.

The [MQTTSN spec]() states:

As with MQTT, a PINGRESP message is the response to a PINGREQ message and means ”yes I
am alive”. Keep Alive messages flow in either direction, sent either by a connected 
client or the gateway

For me it seems that the client is responsible to send PINGREQ in MQTT, in MQTTSN the gateway can send PINGREQ too.

I am using the client in a setup with pahoo mqtt sn gateway and mosquitto mqtt broker. In this setup the gateway does not send PINGREQ to the client so I face a disconnect.

m_lastMsgTimestamp is used to calculate the delay for a PINGREQ and is set in BasicClient Line 526. So it is updated for each incoming message. To update m_lastMsgTimestamp only on sent messages we could move this into void sendMessage(const Message& msg, bool broadcast = false). Though I am not sure if this breaks something else, since I couldn't test it yet.

Regards

arobenko commented 4 years ago

Hi there, Thanks for the report. It's been several years since I touched this code. I'll take a look within next couple of days. If you can navigate and understand the code you are welcome to introduce a fix and send me the patch / pull request. It will make it much easier for me.

Regards, Alex

On Tue, Jan 28, 2020, 19:01 a1lu notifications@github.com wrote:

Hi, I noticed that the client does not send PINGREQ packages if it receives packages in the keep alive period.

The MQTT spec http://public.dhe.ibm.com/software/dw/webservices/ws-mqtt/MQTT_V3.1_Protocol_Specific.pdf states:

The client has a responsibility to send a message within each Keep Alive time period. In

the absence of a data-related message during the time period, the client sends a

PINGREQ message, which the server acknowledges with a PINGRESP message.

The MQTTSN spec states:

As with MQTT, a PINGRESP message is the response to a PINGREQ message and means ”yes I

am alive”. Keep Alive messages flow in either direction, sent either by a connected

client or the gateway

For me it seems that the client is responsible to send PINGREQ in MQTT, in MQTTSN the gateway can send PINGREQ too.

I am using the client in a setup with pahoo mqtt sn gateway and mosquitto mqtt broker. In this setup the gateway does not send PINGREQ to the client so I face a disconnect.

m_lastMsgTimestamp is used to calculate the delay for a PINGREQ and is set in BasicClient Line 526 https://github.com/arobenko/mqtt-sn/blob/master/client/src/basic/BasicClient.h#L526. So it is updated for each incoming message. To update m_lastMsgTimestamp only on sent messages we could move this into void sendMessage(const Message& msg, bool broadcast = false). Though I am not sure if this breaks something else, since I couldn't test it yet.

Regards

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/arobenko/mqtt-sn/issues/3?email_source=notifications&email_token=AASJKGTC67VFTS3C3VXYSWDQ77YABA5CNFSM4KMOWSTKYY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4IJE6ERQ, or unsubscribe https://github.com/notifications/unsubscribe-auth/AASJKGSVQC5VYPWJERQHAP3Q77YABANCNFSM4KMOWSTA .

arobenko commented 4 years ago

I reviewed the code as well as re-read the specification. There is the following statement in the spec:

The client should send a PINGREQ message within each Keep Alive time period, which the GW acknowledges with a PINGRESP message.

To the best of my understanding (at the time of development): should != must and the meaning of PINGREQ is "are you alive?" inquiry, while meaning of the PINGRESP is "yes, I'm alive". Based on this the intention was that the GW is expected to inquire the client whether the latter is alive if the former didn't receive any message from the client within specified amount of time.

As you have said, the GW that you are using does not send PINGREQ to the client. I'll add addition timestamp recording of the outgoing message and force sending of PINGREQ in case there are no outgoing messages withing "keep alive" period. It should help with your situation.

By the way, have you tried using my GW application from this project instead? It would be nice to know whether it is handling your situation correctly or not.

Note, that it is my side and low priority project, I cannot spend more than 1 hour a day on it. It will take me several days to implement and test the change.

Regards, Alex

a1lu commented 4 years ago

Hi Alex, Thanks for the confirmation. I am a bit busy too, I will try to create a pull request on the weekend.

arobenko commented 4 years ago

Hi @a1lu The fix doesn't seem to be too complex. I've already implemented it on "develop" branch (no need fo any additional pull requests). Could you please test it in your scenario? If it does resolve your problem, I'll merge it to master and make an official release.

Thanks.

a1lu commented 4 years ago

The fix is working for me. Thanks.

arobenko commented 4 years ago

Fixed in v0.8.5 release