MQTT with QOS 1 or 2 forbids a packet ID of 0. #190
Conversation
| @@ -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 | |||
There was a problem hiding this comment.
@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].
| @@ -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 | |||
| // 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); |
There was a problem hiding this comment.
Perform this within the check for QOS<1 on L729 instead
|
@dlizotte-uwo are you available to make the requested changes above? |
Hi sorry @brentru - day job is madness. I can probably fix this weekend if that's still useful. |
|
@dlizotte-uwo It's still useful! I'll keep this issue open for when you're ready. |
|
I think based on what fipwmaqzufheoxq92ebc says and based on #199 that this is still needed and checks are in the right place. I think it's good to keep 1 <= packet_id_counter <= 65535 an invariant in this code. |
|
@tyeth Before I look it over, do you have thoughts on this PR? |
|
I'm digesting, I'll come back to you tomorrow.
I hadn't before appreciated the packet Id of server initiated messages and
client initiated messages could have the same packet Id at the same time.
I'd never really examined the protocol in such depth before. Just verifying
where we check and use packet Id's, etc.
…On Mon, 21 Aug 2023, 18:46 Brent Rubell, ***@***.***> wrote:
@tyeth <https://github.com/tyeth> Before I look it over, do you have
thoughts on this PR?
—
Reply to this email directly, view it on GitHub
<#190 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABTBZ47HMK3XOOZGRAN6G6LXWONHJANCNFSM42JQGOUQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
@brentru I think we might be okay, I'm fine with where the checks are, and the use of packet id seems like we wont run into conflicts. (Worth rechecking when we support QOS2 or mqtt5) |
|
Sweet! Thank you for the PR and for your patience, @dlizotte-uwo |
|
Thanks @brentru! Sorry for that year I was MIA. |
|
No problem, thanks for the PR! |
of the code were modified. This will help us understand any risks of integrating
the code.
The change is to the packet numbering, which now starts at 1 and wraps around to 1 (rather than 0) when overflowing 16 bits. This is because the MQTT standard requires nonzero packet IDs for QOS=1 and QOS=2. In the previous version, if a client is created, then connected, then a QOS=1 subscription packet is sent with packet ID 0 is sent, and my server (mosquitto) disconnects. Re-connecting and trying again works because the packet ID has incremented to 1, which is legal.
doesn't apply to a supported platform of the library please mention it.
I don't think there are any.
strive to not break users of the code and running tests/examples helps with this
process.
I am using this code in a project and have tried with pub/sub; seems to work now.