espressif / esp-mqtt

ESP32 mqtt component
Apache License 2.0
603 stars 255 forks source link

DUP flag only gets set for QoS2 (IDFGH-2685) #151

Closed rsmithiii closed 4 years ago

rsmithiii commented 4 years ago

In the mqtt_resent_queued() function in mqtt_client.c (see below), the function only sets the DUP flag if the queued item is a PUBLISH message with QoS 2, but the QoS 1 messages should also be included, according to the MQTT 3.1.1 spec, since QoS 1 PUBLISH messages might also be resent.

static esp_err_t mqtt_resend_queued(esp_mqtt_client_handle_t client, outbox_item_handle_t item)
{
    // decode queued data
    client->mqtt_state.outbound_message->data = outbox_item_get_data(item, &client->mqtt_state.outbound_message->length, &client->mqtt_state.pending_msg_id,
            &client->mqtt_state.pending_msg_type, &client->mqtt_state.pending_publish_qos);
    // set duplicate flag for QoS-2 message
    if (client->mqtt_state.pending_msg_type == MQTT_MSG_TYPE_PUBLISH && client->mqtt_state.pending_publish_qos == 2) {
        mqtt_set_dup(client->mqtt_state.outbound_message->data);
    }

    // try to resend the data
    if (mqtt_write_data(client) != ESP_OK) {
        ESP_LOGE(TAG, "Error to resend data ");
        esp_mqtt_abort_connection(client);
        return ESP_FAIL;
    }
    return ESP_OK;
}

In section 3.3.1 of the MQTT 3.1.1 spec http://docs.oasis-open.org/mqtt/mqtt/v3.1.1/os/mqtt-v3.1.1-os.html#_Toc398718038 it is specified that the DUP flag MUST be set to 1 by the Client or Server when it attempts to re-deliver a PUBLISH Packet. The DUP flag MUST be set to 0 for all QoS 0 messages.

david-cermak commented 4 years ago

Hi @rsmithiii Thanks for the report! This seems to be on oversight from our part, indeed the QoS1 messages should have the DUP flag set on retransmit. Will fix!