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.

Sign up for GitHub

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

Jump to bottom

MQTT with QOS 1 or 2 forbids a packet ID of 0. #190

Merged
merged 5 commits into from
Aug 28, 2023
Merged

MQTT with QOS 1 or 2 forbids a packet ID of 0. #190

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4f76558
MQTT with QOS 1 or 2 forbids a packet ID of 0.
dlizotte-uwo Apr 2, 2021
7b2061a
Missing space after // comment
dlizotte-uwo Apr 4, 2021
847f561
Merge in KEEPALIVE check.
dlizotte-uwo Jul 5, 2023
f4685f4
Fix Adafruit_MQTT.cpp formatting
dlizotte-uwo Jul 5, 2023
5b93947
Merge branch 'adafruit:master' into master
dlizotte-uwo Aug 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 9 additions & 9 deletions Adafruit_MQTT.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ Adafruit_MQTT::Adafruit_MQTT(const char *server, uint16_t port, const char *cid,
will_qos = 0;
will_retain = 0;

packet_id_counter = 0;
packet_id_counter = 1; // MQTT spec forbids packet id of 0 if QOS=1
Copy link
Member

Choose a reason for hiding this comment

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

@dlizotte-uwo this constructor does not guarantee a QoS level, we should handle checking and incrementing the PID based on the QoS level within the SUBSCRIBE, UNSUBSCRIBE, and PUBLISH calls.

SUBSCRIBE, UNSUBSCRIBE, and PUBLISH (in cases where QoS > 0) Control Packets MUST contain a non-zero 16-bit Packet Identifier [MQTT-2.3.1-1].

Copy link

Choose a reason for hiding this comment

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

@brentru Hi, the QoS only needs to be checked for PUBLISH (and is already done in L707).
SUBSCRIBE and UNSUBSCRIBE always need a (valid) packet id
(Ref)
In PUBLISH QoS = 0, the packet id is not used.

}

Adafruit_MQTT::Adafruit_MQTT(const char *server, uint16_t port,
Expand All @@ -137,7 +137,7 @@ Adafruit_MQTT::Adafruit_MQTT(const char *server, uint16_t port,
will_qos = 0;
will_retain = 0;

packet_id_counter = 0;
packet_id_counter = 1; // MQTT spec forbids packet id of 0 if QOS=1
Copy link
Member

Choose a reason for hiding this comment

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

Same here

}

int8_t Adafruit_MQTT::connect() {
Expand Down Expand Up @@ -342,7 +342,7 @@ bool Adafruit_MQTT::publish(const char *topic, uint8_t *data, uint16_t bLen,

// we increment the packet_id_counter right after publishing so inc here too
// to match
packnum++;
packnum = packnum + 1 + (packnum + 1 == 0); // Skip zero
if (packnum != packet_id_counter)
return false;
}
Expand Down Expand Up @@ -709,8 +709,8 @@ uint16_t Adafruit_MQTT::publishPacket(uint8_t *packet, const char *topic,
p[1] = packet_id_counter & 0xFF;
p += 2;

// increment the packet id
packet_id_counter++;
// increment the packet id, skipping 0
packet_id_counter = packet_id_counter + 1 + (packet_id_counter + 1 == 0);
Copy link
Member

Choose a reason for hiding this comment

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

Perform this within the check for QOS<1 on L729 instead

Copy link

Choose a reason for hiding this comment

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

@brentru This is already in a check L707 for QoS > 0 (and for QoS = 0 the counter is not changed and not used)

}

memmove(p, data, bLen);
Expand All @@ -735,8 +735,8 @@ uint8_t Adafruit_MQTT::subscribePacket(uint8_t *packet, const char *topic,
p[1] = packet_id_counter & 0xFF;
p += 2;

// increment the packet id
packet_id_counter++;
// increment the packet id, skipping 0
packet_id_counter = packet_id_counter + 1 + (packet_id_counter + 1 == 0);

p = stringprint(p, topic);

Expand Down Expand Up @@ -764,8 +764,8 @@ uint8_t Adafruit_MQTT::unsubscribePacket(uint8_t *packet, const char *topic) {
p[1] = packet_id_counter & 0xFF;
p += 2;

// increment the packet id
packet_id_counter++;
// increment the packet id, skipping 0
packet_id_counter = packet_id_counter + 1 + (packet_id_counter + 1 == 0);

p = stringprint(p, topic);

Expand Down