Skip to content

Fix MQTT QoS 2 Subscriber Handling (PUBREC/PUBREL/PUBCOMP) and Add Tests#90

Open
MarcAntoineCRUE wants to merge 3 commits intohmueller01:masterfrom
MarcAntoineCRUE:bugfix/QoS
Open

Fix MQTT QoS 2 Subscriber Handling (PUBREC/PUBREL/PUBCOMP) and Add Tests#90
MarcAntoineCRUE wants to merge 3 commits intohmueller01:masterfrom
MarcAntoineCRUE:bugfix/QoS

Conversation

@MarcAntoineCRUE
Copy link

Description:

This merge request fixes critical issues in the MQTT QoS 2 message flow for subscribers in the PubSubClient library, and adds a dedicated test to ensure correct behavior.

Summary of changes:

  • Correctly respond with PUBREC (not PUBACK) when receiving a QoS 2 PUBLISH as a subscriber.
  • Add missing handler for PUBREL packets: the client now responds with PUBCOMP, completing the QoS 2 handshake as per the MQTT 3.1.1 specification.
  • Update comments for clarity regarding the QoS 2 handshake steps.
  • Add a new test (test_receive_qos2) to verify the full QoS 2 subscriber flow: PUBLISH → PUBREC → PUBREL → PUBCOMP, and ensure the callback is only called once.
  • All existing and new tests pass, with no regressions.

Motivation:

Previously, the library responded with PUBACK to all QoS > 0 PUBLISH packets, breaking the QoS 2 protocol. The lack of a PUBREL handler meant the handshake was never completed, potentially causing message loss or duplicate delivery. This patch brings the implementation in line with the MQTT specification and ensures robust, standards-compliant behavior.

Impact:

  • Fixes MQTT QoS 2 reliability for subscribers.
  • No breaking changes for existing users.
  • Improves test coverage and protocol compliance.

@MarcAntoineCRUE MarcAntoineCRUE marked this pull request as draft February 25, 2026 20:22
@hmueller01
Copy link
Owner

@MarcAntoineCRUE Thank you very much for your contribution!
This is a nice catch! Obviously no one checked that before ...

Could you please do not commit a new test (tests/src/receive_spec copy.cpp) but instead update the current receive_spec.cpp?

@MarcAntoineCRUE
Copy link
Author

Yes I just mentioned I added a new commit, this is why I changed it to draft, as I need to update that
I will change status as ready when I will have corrected that

@MarcAntoineCRUE MarcAntoineCRUE marked this pull request as ready for review February 25, 2026 20:52
@MarcAntoineCRUE
Copy link
Author

It is now good for review

Copy link
Owner

@hmueller01 hmueller01 left a comment

Choose a reason for hiding this comment

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

LGTM

@hmueller01
Copy link
Owner

Can you please add short change note in CHANGELOG.md ## [Unreleased] section. Thanks a lot!

@github-actions
Copy link

Memory usage change @ a5710c2

Board flash % RAM for global variables %
arduino:avr:uno 🔺 +28 - +36 +0.09 - +0.11 0 - 0 0.0 - 0.0
arduino:megaavr:uno2018 🔺 +28 - +36 +0.06 - +0.07 0 - 0 0.0 - 0.0
arduino:samd:mkr1000 🔺 +24 - +24 +0.01 - +0.01 0 - 0 0.0 - 0.0
esp32:esp32:esp32 🔺 +28 - +32 0.0 - 0.0 0 - 0 0.0 - 0.0
esp8266:esp8266:generic N/A N/A N/A N/A
Click for full report table
Board examples/mqtt_auth
flash
% examples/mqtt_auth
RAM for global variables
% examples/mqtt_basic
flash
% examples/mqtt_basic
RAM for global variables
% examples/mqtt_progmem
flash
% examples/mqtt_progmem
RAM for global variables
% examples/mqtt_publish_in_callback
flash
% examples/mqtt_publish_in_callback
RAM for global variables
% examples/mqtt_reconnect_nonblocking
flash
% examples/mqtt_reconnect_nonblocking
RAM for global variables
% examples/mqtt_stream
flash
% examples/mqtt_stream
RAM for global variables
% examples/mqtt_esp
flash
% examples/mqtt_esp
RAM for global variables
% examples/mqtt_esp_large_message
flash
% examples/mqtt_esp_large_message
RAM for global variables
%
arduino:avr:uno 36 0.11 0 0.0 34 0.11 0 0.0 36 0.11 0 0.0 36 0.11 0 0.0 28 0.09 0 0.0 34 0.11 0 0.0
arduino:megaavr:uno2018 36 0.07 0 0.0 36 0.07 0 0.0 36 0.07 0 0.0 36 0.07 0 0.0 28 0.06 0 0.0 36 0.07 0 0.0
arduino:samd:mkr1000 24 0.01 0 0.0 24 0.01 0 0.0 24 0.01 0 0.0 24 0.01 0 0.0 24 0.01 0 0.0 24 0.01 0 0.0
esp32:esp32:esp32 32 0.0 0 0.0 32 0.0 0 0.0 32 0.0 0 0.0 32 0.0 0 0.0 32 0.0 0 0.0 28 0.0 0 0.0 28 0.0 0 0.0
esp8266:esp8266:generic N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A
Click for full report CSV
Board,examples/mqtt_auth<br>flash,%,examples/mqtt_auth<br>RAM for global variables,%,examples/mqtt_basic<br>flash,%,examples/mqtt_basic<br>RAM for global variables,%,examples/mqtt_progmem<br>flash,%,examples/mqtt_progmem<br>RAM for global variables,%,examples/mqtt_publish_in_callback<br>flash,%,examples/mqtt_publish_in_callback<br>RAM for global variables,%,examples/mqtt_reconnect_nonblocking<br>flash,%,examples/mqtt_reconnect_nonblocking<br>RAM for global variables,%,examples/mqtt_stream<br>flash,%,examples/mqtt_stream<br>RAM for global variables,%,examples/mqtt_esp<br>flash,%,examples/mqtt_esp<br>RAM for global variables,%,examples/mqtt_esp_large_message<br>flash,%,examples/mqtt_esp_large_message<br>RAM for global variables,%
arduino:avr:uno,36,0.11,0,0.0,34,0.11,0,0.0,36,0.11,0,0.0,36,0.11,0,0.0,28,0.09,0,0.0,34,0.11,0,0.0
arduino:megaavr:uno2018,36,0.07,0,0.0,36,0.07,0,0.0,36,0.07,0,0.0,36,0.07,0,0.0,28,0.06,0,0.0,36,0.07,0,0.0
arduino:samd:mkr1000,24,0.01,0,0.0,24,0.01,0,0.0,24,0.01,0,0.0,24,0.01,0,0.0,24,0.01,0,0.0,24,0.01,0,0.0
esp32:esp32:esp32,32,0.0,0,0.0,32,0.0,0,0.0,32,0.0,0,0.0,32,0.0,0,0.0,32,0.0,0,0.0,,,,,28,0.0,0,0.0,28,0.0,0,0.0
esp8266:esp8266:generic,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A

@MarcAntoineCRUE
Copy link
Author

Ok changelog is done, up to you for merging

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