Disconnect message not implemented

[gregjesl]

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]

Hi @gregjesl

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

[gregjesl]

@david-cermak Great, thank you.

After looking through the code, it appears the appropriate place to add the functionality would be here:

esp-mqtt/mqtt_client.c

Lines 776 to 782 in 99004f2

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

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]

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

[gregjesl]

@david-cermak I struggled with the rebase so I branched from idf and implemented the patch. The new pull request is #107

[david-cermak]

closed per caf5007