Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
Jun 22, 2021

Conversation

rgorosito
Copy link
Contributor

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.

@rgorosito rgorosito changed the title Fix: Subscriptions can't be read if come when waiting por response of ping or publish Fix: Subscriptions can't be read if arrive when waiting for response of ping or publish May 23, 2021
@brentru
Copy link
Member

brentru commented May 24, 2021

@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
Copy link
Contributor Author

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
Copy link
Contributor Author

How to reproduce:

Problem with ping:

  • Create a sketch that subscribe to a topic and send PINGs once per second second (to increase the probability of occurrence)
  • From another host, send messages in a loop
    you will see that some messages are not processed in readSubscription

Problem with publish:

  • Create a sketch that subscribe to a topic and send messages to another topic once per second second (to increase the probability of occurrence)
  • From another host, send messages in a loop
    you will see that some messages are not processed in readSubscription and son publish will return an error.

@brentru brentru self-requested a review June 3, 2021 15:07
Copy link
Member

@brentru brentru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rgorosito Ok, this does address a bug I've been having with this library. Requesting clarification + modifications before I merge in....

Also please bump the version number in the library.properties file so we can release.

Adafruit_MQTT.cpp Show resolved Hide resolved
Adafruit_MQTT.cpp Show resolved Hide resolved
Adafruit_MQTT.cpp Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants