espressif / esp-mqtt

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

Disconnect message not implemented #97

Closed gregjesl closed 5 years ago

gregjesl commented 5 years ago

After reviewing and using the code, it does not appear that the MQTT client ever sends the DISCONNECT packet to the broker. Simply "dropping the line" can lead to undesirable behavior on the broker, especially when the client tries to reconnect.

david-cermak commented 5 years ago

Hi @gregjesl

Thank you for raising this issue! Indeed the disconnect packet is not implemented. Will be fixed.

gregjesl commented 5 years ago

@david-cermak Great, thank you.

After looking through the code, it appears the appropriate place to add the functionality would be here: https://github.com/espressif/esp-mqtt/blob/99004f2a48b8eda13f0427eefb351aec63f90ff8/mqtt_client.c#L776-L782

My suggestion would be:

esp_err_t esp_mqtt_client_stop(esp_mqtt_client_handle_t client)
{
    // Notify the broker we are disconnecting
    client->mqtt_state.outbound_message = mqtt_msg_disconnect(&client->mqtt_state.mqtt_connection);

    if (mqtt_write_data(client) != ESP_OK) {
        ESP_LOGE(TAG, "Error disconnecting");
        return ESP_FAIL;
    }

    client->run = false;
    xEventGroupWaitBits(client->status_bits, STOPPED_BIT, false, true, portMAX_DELAY);
    client->state = MQTT_STATE_UNKNOWN;
    return ESP_OK;
}

I'd submit a pull request but I never seem to have much luck with Git submodules.

david-cermak commented 5 years ago

@gregjesl Thanks for your suggestion! Please feel free to submit a PR, but please select idf as target branch.

gregjesl commented 5 years ago

@david-cermak I struggled with the rebase so I branched from idf and implemented the patch. The new pull request is https://github.com/espressif/esp-mqtt/pull/107

david-cermak commented 5 years ago

closed per https://github.com/espressif/esp-mqtt/commit/caf5007b99df985b330683dfe2fa454c94633018