adafruit / Adafruit_MQTT_Library

Arduino library for MQTT support
MIT License
566 stars 292 forks source link

Fix: Subscriptions can't be read if arrive when waiting for response of ping or publish #195

Closed rgorosito closed 3 years ago

rgorosito commented 3 years ago

When I'm working with the library, I detect that some event was lost. After enabled debug, I've detected 2 situations:

  1. Ping call processPacketsUntil which detect if call new message (appears in debug console), but can't be read in callback or readSubscription. My fix add a boolean attribute "new_message" (default false) in Adafruit_MQTT_Publish class. It is set to true inside processPacketsUntil. Later, in readSubscription, search if there are pending message. If so, return that message. Callback don't need to be modified because it call readSubscription.

  2. Sometimes my program fail to publish a message and loss subscription arrived (detected in debug console). The fix was more easy and clean: replase readFullPacket with processPacketsUntil. This permit that a message arrive when waiting for response on QoS>0 and because processPacketsUntil has the expected packet type, the check in the type in buffer is not needed anymore.

Logs

  1. PING
MQTT ping packet:
          [0xC0],   [0x00], 
Client sendPacket returned: 2
Read data:              2 [0x32], 
Packet Type:            2 [0x32],
Read data:                [0x1A], 
Packet Length:  26
Read data:                [0x00],   [0x14], e [0x65], s [0x73], p [0x70], - [0x2D], p [0x70], r [0x72],
        u [0x75], e [0x65], b [0x62], a [0x61], / [0x2F], l [0x6C], u [0x75], z [0x7A],
        / [0x2F], r [0x72], o [0x6F], j [0x6A], o [0x6F], 2 [0x32],   [0x00],   [0x1F],
        O [0x4F], N [0x4E],
Packet len: 28
        2 [0x32],   [0x1A],   [0x00],   [0x14], e [0x65], s [0x73], p [0x70], - [0x2D],
        p [0x70], r [0x72], u [0x75], e [0x65], b [0x62], a [0x61], / [0x2F], l [0x6C],
        u [0x75], z [0x7A], / [0x2F], r [0x72], o [0x6F], j [0x6A], o [0x6F], 2 [0x32],
          [0x00],   [0x1F], O [0x4F], N [0x4E],
Looking for subscription len 20
Found sub #4
Data len: 2
Data: ON
MQTT puback packet:
        2 [0x32],   [0x1A],   [0x00],   [0x14],
Client sendPacket returned: 4
Read data:                [0xD0],
Packet Type:              [0xD0],
Read data:                [0x00],
Packet Length:  0

message are parsed, but never read

  1. PUBLISH
    MQTT publish packet:
        2 [0x32], ' [0x27],   [0x00],   [0x17], e [0x65], s [0x73], p [0x70], - [0x2D],
        p [0x70], r [0x72], u [0x75], e [0x65], b [0x62], a [0x61], / [0x2F], f [0x66],
        e [0x65], e [0x65], d [0x64], s [0x73], / [0x2F], e [0x65], s [0x73], t [0x74],
        a [0x61], d [0x64], o [0x6F],   [0x00],   [0x0D], 1 [0x31], 9 [0x39], 2 [0x32],
        . [0x2E], 1 [0x31], 6 [0x36], 8 [0x38], . [0x2E], 0 [0x30], . [0x2E], 6 [0x36],
        1 [0x31],
    Client sendPacket returned: 41
    Read data:              2 [0x32],
    Packet Type:            2 [0x32],
    Read data:                [0x1A],
    Packet Length:  26
    Read data:                [0x00],   [0x14], e [0x65], s [0x73], p [0x70], - [0x2D], p [0x70], r [0x72],
        u [0x75], e [0x65], b [0x62], a [0x61], / [0x2F], l [0x6C], u [0x75], z [0x7A],
        / [0x2F], r [0x72], o [0x6F], j [0x6A], o [0x6F], 2 [0x32],   [0x00],   [0x03],
        O [0x4F], N [0x4E],
    Publish QOS1+ reply:            2 [0x32],   [0x1A],   [0x00],   [0x14], e [0x65], s [0x73], p [0x70], - [0x2D],
        p [0x70], r [0x72], u [0x75], e [0x65], b [0x62], a [0x61], / [0x2F], l [0x6C],
        u [0x75], z [0x7A], / [0x2F], r [0x72], o [0x6F], j [0x6A], o [0x6F], 2 [0x32],
          [0x00],   [0x03], O [0x4F], N [0x4E],
    Failed <--- return of publish is false
    Read data:              @ [0x40],
    Packet Type:            @ [0x40],
    Read data:                [0x02],
    Packet Length:  2
    Read data:                [0x00],   [0x0D],
    Packet len: 4
        @ [0x40],   [0x02],   [0x00],   [0x0D],

    On publish waiting por ACK, ignore incomming messages and return an error because ACK come later.

brentru commented 3 years ago

@rgorosito Hi, thanks for submitting this PR. I agree completely with #2, thank you for adding it, it'll help the publish function be more selective with what its reading out.

In 1),

Ping call processPacketsUntil which detect if call new message (appears in debug console), but can't be read in callback or readSubscription.

I'm confused here.

The way the library currently works, ping is called which calls processPacketsUntil. processPacketsUntil waits for the broker to respond with a PINGRESP control packet. It should not obtain any other messages other than a PINGRESP. When would this not be the case?

rgorosito commented 3 years ago

The way the library currently works, ping is called which calls processPacketsUntil. processPacketsUntil waits for the broker to respond with a PINGRESP control packet. It should not obtain any other messages other than a PINGRESP. When would this not be the case?

the problem occurs when a new message reaches the subscriber between the ping that was sent and its response received. If you see the log 1:

out: PINGREQ [C0]
in: PUBLISH RECEPTION (sent from the mqtt server) [32]
out: PUBLISH ACK [32]
in: PINGRESP [D0]
rgorosito commented 3 years ago

How to reproduce:

Problem with ping:

Problem with publish: