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

Retransmission of QOS0 (IDFGH-12829) #276

Closed
Totrasmek opened this issue May 17, 2024 · 1 comment
Closed

Retransmission of QOS0 (IDFGH-12829) #276

Totrasmek opened this issue May 17, 2024 · 1 comment

Comments

@Totrasmek
Copy link

Totrasmek commented May 17, 2024

Description

Currently in the state MQTT_STATE_CONNECTED the client will, on successful mqtt_resend_queued, set the pending status of the current pending msg id to TRANSMITTED. The goal of this to trigger the retransmission logic. However, retransmission is not necessary for QOS0 packets, and as QOS0 packets all have the same msg_id (0) and are deleted upon a successful mqtt_resend_queued, this can lead to a different item in the outbox being set to TRANSMITTED. This can cause items in the outbox to send in a weird order.
Let me know if my understanding is incorrect.

Code

From mqtt_client.c line 1671

            // resend all non-transmitted messages first
            outbox_item_handle_t item = outbox_dequeue(client->outbox, QUEUED, NULL);
            if (item) {
                if (mqtt_resend_queued(client, item) == ESP_OK) {
                    outbox_set_pending(client->outbox, client->mqtt_state.pending_msg_id, TRANSMITTED);
                }
                // resend other "transmitted" messages after 1s
            } else if (has_timed_out(last_retransmit, client->config->message_retransmit_timeout)) {
                last_retransmit = platform_tick_get_ms();
                item = outbox_dequeue(client->outbox, TRANSMITTED, &msg_tick);
                if (item && (last_retransmit - msg_tick > client->config->message_retransmit_timeout))  {
                    mqtt_resend_queued(client, item);
                }
            }

Suggested Change

I believe that the line outbox_set_pending(client->outbox, client->mqtt_state.pending_msg_id, TRANSMITTED); should only be called if the item's QOS != 0.

@github-actions github-actions bot changed the title Retransmission of QOS0 Retransmission of QOS0 (IDFGH-12829) May 17, 2024
@euripedesrocha
Copy link
Collaborator

Hi @Totrasmek thanks for reporting this. Should be fixed soon.

espressif-bot pushed a commit to espressif/esp-idf that referenced this issue Jun 17, 2024
git log --oneline aa6f889fb4f6f743b3a550aa587713aabbdca1fc..cac1552e62b0474c162547b7cce345d7cd1aecfe

Detailed description of the changes:
* fix: clang analyzer unknown pragma warning
  - See merge request espressif/esp-mqtt!215
  - See commit espressif/esp-mqtt@6bb5a5b
* fix: gcc analyzer unknown pragma warning
  - See merge request espressif/esp-mqtt!214
  - See commit espressif/esp-mqtt@b527203
* feat: Moves deletion of expired messages to run at all states
  - Closes IDFGH-12831
  - Closes espressif/esp-mqtt#278
  - See commit espressif/esp-mqtt@32dada4
* fix: Handling of state in the outbox for enqueued QoS 0 messages
  - Closes IDFGH-12829
  - Closes espressif/esp-mqtt#276
  - See commit espressif/esp-mqtt@739cb2d
* fix: Instalation of gcovr in host tests was broken
  - See merge request espressif/esp-mqtt!211
  - See commit espressif/esp-mqtt@6643c49
* Allow to publish using only topic alias on MQTT5
  - Closes IDFGH-12735
  - Fix: Allow to publish using only topic alias on MQTT5 (espressif/esp-mqtt@0071aca)
* refactor: replaced heap mock with Linux-compatible heap component
  - See merge request espressif/esp-mqtt!208
  - See commit espressif/esp-mqtt@8b0b43e
* fix: gcc -fanalyzer warnings
  - See merge request espressif/esp-mqtt!209
  - See commit espressif/esp-mqtt@8bc3bff
* fix: Use catch from component manager
  - See merge request espressif/esp-mqtt!210
  - See commit espressif/esp-mqtt@53e0cc7
* fix: Fix host test for github ci.
  - Closes IDF-8883
  - See commit espressif/esp-mqtt@b43d93c
espressif-bot pushed a commit to espressif/esp-idf that referenced this issue Aug 21, 2024
git log --oneline aa6f889fb4f6f743b3a550aa587713aabbdca1fc..cac1552e62b0474c162547b7cce345d7cd1aecfe

Detailed description of the changes:
* fix: clang analyzer unknown pragma warning
  - See merge request espressif/esp-mqtt!215
  - See commit espressif/esp-mqtt@6bb5a5b
* fix: gcc analyzer unknown pragma warning
  - See merge request espressif/esp-mqtt!214
  - See commit espressif/esp-mqtt@b527203
* feat: Moves deletion of expired messages to run at all states
  - Closes IDFGH-12831
  - Closes espressif/esp-mqtt#278
  - See commit espressif/esp-mqtt@32dada4
* fix: Handling of state in the outbox for enqueued QoS 0 messages
  - Closes IDFGH-12829
  - Closes espressif/esp-mqtt#276
  - See commit espressif/esp-mqtt@739cb2d
* fix: Instalation of gcovr in host tests was broken
  - See merge request espressif/esp-mqtt!211
  - See commit espressif/esp-mqtt@6643c49
* Allow to publish using only topic alias on MQTT5
  - Closes IDFGH-12735
  - Fix: Allow to publish using only topic alias on MQTT5 (espressif/esp-mqtt@0071aca)
* refactor: replaced heap mock with Linux-compatible heap component
  - See merge request espressif/esp-mqtt!208
  - See commit espressif/esp-mqtt@8b0b43e
* fix: gcc -fanalyzer warnings
  - See merge request espressif/esp-mqtt!209
  - See commit espressif/esp-mqtt@8bc3bff
* fix: Use catch from component manager
  - See merge request espressif/esp-mqtt!210
  - See commit espressif/esp-mqtt@53e0cc7
* fix: Fix host test for github ci.
  - Closes IDF-8883
  - See commit espressif/esp-mqtt@b43d93c
espressif-bot pushed a commit to espressif/esp-idf that referenced this issue Aug 22, 2024
git log --oneline aa6f889fb4f6f743b3a550aa587713aabbdca1fc..cac1552e62b0474c162547b7cce345d7cd1aecfe

Detailed description of the changes:
* fix: clang analyzer unknown pragma warning
  - See merge request espressif/esp-mqtt!215
  - See commit espressif/esp-mqtt@6bb5a5b
* fix: gcc analyzer unknown pragma warning
  - See merge request espressif/esp-mqtt!214
  - See commit espressif/esp-mqtt@b527203
* feat: Moves deletion of expired messages to run at all states
  - Closes IDFGH-12831
  - Closes espressif/esp-mqtt#278
  - See commit espressif/esp-mqtt@32dada4
* fix: Handling of state in the outbox for enqueued QoS 0 messages
  - Closes IDFGH-12829
  - Closes espressif/esp-mqtt#276
  - See commit espressif/esp-mqtt@739cb2d
* fix: Instalation of gcovr in host tests was broken
  - See merge request espressif/esp-mqtt!211
  - See commit espressif/esp-mqtt@6643c49
* Allow to publish using only topic alias on MQTT5
  - Closes IDFGH-12735
  - Fix: Allow to publish using only topic alias on MQTT5 (espressif/esp-mqtt@0071aca)
* refactor: replaced heap mock with Linux-compatible heap component
  - See merge request espressif/esp-mqtt!208
  - See commit espressif/esp-mqtt@8b0b43e
* fix: gcc -fanalyzer warnings
  - See merge request espressif/esp-mqtt!209
  - See commit espressif/esp-mqtt@8bc3bff
* fix: Use catch from component manager
  - See merge request espressif/esp-mqtt!210
  - See commit espressif/esp-mqtt@53e0cc7
* fix: Fix host test for github ci.
  - Closes IDF-8883
  - See commit espressif/esp-mqtt@b43d93c
espressif-bot pushed a commit to espressif/esp-idf that referenced this issue Sep 11, 2024
git log --oneline aa6f889fb4f6f743b3a550aa587713aabbdca1fc..cac1552e62b0474c162547b7cce345d7cd1aecfe

Detailed description of the changes:
* fix: clang analyzer unknown pragma warning
  - See merge request espressif/esp-mqtt!215
  - See commit espressif/esp-mqtt@6bb5a5b
* fix: gcc analyzer unknown pragma warning
  - See merge request espressif/esp-mqtt!214
  - See commit espressif/esp-mqtt@b527203
* feat: Moves deletion of expired messages to run at all states
  - Closes IDFGH-12831
  - Closes espressif/esp-mqtt#278
  - See commit espressif/esp-mqtt@32dada4
* fix: Handling of state in the outbox for enqueued QoS 0 messages
  - Closes IDFGH-12829
  - Closes espressif/esp-mqtt#276
  - See commit espressif/esp-mqtt@739cb2d
* fix: Instalation of gcovr in host tests was broken
  - See merge request espressif/esp-mqtt!211
  - See commit espressif/esp-mqtt@6643c49
* Allow to publish using only topic alias on MQTT5
  - Closes IDFGH-12735
  - Fix: Allow to publish using only topic alias on MQTT5 (espressif/esp-mqtt@0071aca)
* refactor: replaced heap mock with Linux-compatible heap component
  - See merge request espressif/esp-mqtt!208
  - See commit espressif/esp-mqtt@8b0b43e
* fix: gcc -fanalyzer warnings
  - See merge request espressif/esp-mqtt!209
  - See commit espressif/esp-mqtt@8bc3bff
* fix: Use catch from component manager
  - See merge request espressif/esp-mqtt!210
  - See commit espressif/esp-mqtt@53e0cc7
* fix: Fix host test for github ci.
  - Closes IDF-8883
  - See commit espressif/esp-mqtt@b43d93c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants