LiamBindle / MQTT-C

A portable MQTT C client for embedded systems and PCs alike.
https://liambindle.ca/MQTT-C
MIT License
766 stars 269 forks source link

How to check the client has received a Connack from the Broker after connect() #163

Open schnedann opened 2 years ago

schnedann commented 2 years ago

after reading the API Docu and reading the code, I still have no clue how to check for an successful connect, so how one can check if the broker sent a Connack, and the Client is in a Connected state prior to executing a publish?

pieterconradie commented 1 year ago

Hi @schnedann

I have the same problem, for example when wrong MQTT password is used then mqtt_connect() will return MQTT_OK but only a while later will the MQTT_ERROR_CONNECTION_REFUSED be returned.

I can only offer a hint / workaround. Inspect __mqtt_recv() and search for MQTT_CONTROL_CONNACK. You will see that client->typical_response_time is updated. You can check if this field has been updated to detect the connected state.

I think a better way would be to modify struct mqtt_client and add a state field that gets updated when MQTT_CONTROL_CONNACK is received. All of the other error cases would also need to be catered for.

Best regards, Pieter https://piconomix.com

schnedann commented 1 year ago

I think a better way would be to modify struct mqtt_client and add a state field that gets updated when MQTT_CONTROL_CONNACK is received. All of the other error cases would also need to be catered for.

Hi, thx for the reply. In fact, this is what I did for serving our needs. Its still not clear if I can provide these changes upstream...

rechrtb commented 1 year ago

+1 on having a better way to detect CONNACK has been received.

pieterconradie commented 1 year ago

Here's my quick fix. I added a bool flag in struct mqtt_client:

struct mqtt_client {
    ...
    /** @brief Flag is set on connection event */
    bool event_connect;
};

I set the flag in __mqtt_recv():

          switch (response.fixed_header.control_type) {
            case MQTT_CONTROL_CONNACK:
                /* release associated CONNECT */
                msg = mqtt_mq_find(&client->mq, MQTT_CONTROL_CONNECT, NULL);
                if (msg == NULL) {
                    client->error = MQTT_ERROR_ACK_OF_UNKNOWN;
                    mqtt_recv_ret = MQTT_ERROR_ACK_OF_UNKNOWN;
                    break;
                }
                msg->state = MQTT_QUEUED_COMPLETE;
                lat_log(LAT_VER, "Release msg type %u", response.fixed_header.control_type);
                /* initialize typical response time */
                client->typical_response_time = (double) (MQTT_PAL_TIME() - msg->time_sent);
                client->last_response_time = (double) (MQTT_PAL_TIME() - msg->time_sent);
                client->min_response_time = (double) (MQTT_PAL_TIME() - msg->time_sent);
                client->max_response_time = (double) (MQTT_PAL_TIME() - msg->time_sent);

                /* check that connection was successful */
                if (response.decoded.connack.return_code != MQTT_CONNACK_ACCEPTED) {
                    if (response.decoded.connack.return_code == MQTT_CONNACK_REFUSED_IDENTIFIER_REJECTED) {
                        client->error = MQTT_ERROR_CONNECT_CLIENT_ID_REFUSED;
                        mqtt_recv_ret = MQTT_ERROR_CONNECT_CLIENT_ID_REFUSED;
                    } else {
                        client->error = MQTT_ERROR_CONNECTION_REFUSED;
                        mqtt_recv_ret = MQTT_ERROR_CONNECTION_REFUSED;
                    }
                    break;
                }
                else {
                    client->event_connect = true;
                }
                break;

Remember to clear the flag in mqtt_init(), mqtt_init_reconnect() and mqtt_reinit()

acassis commented 1 year ago

Hi @pieterconradie I think your solution is more elegant than mine:

--- a/include/mqtt.h    2021-03-29 14:53:52.000000000 -0300
+++ b/include/mqtt.h    2022-11-10 20:24:11.581983160 -0300
@@ -234,7 +234,8 @@
 enum MQTTErrors {
     MQTT_ERROR_UNKNOWN=INT_MIN,
     __ALL_MQTT_ERRORS(GENERATE_ENUM)
-    MQTT_OK = 1
+    MQTT_OK = 1,
+    MQTT_ACK
 };

 /**
diff -Naur a/src/mqtt.c b/src/mqtt.c
--- a/src/mqtt.c        2021-03-29 14:53:52.000000000 -0300
+++ b/src/mqtt.c        2022-11-10 20:23:41.997057844 -0300
@@ -723,7 +723,11 @@
                 /* initialize typical response time */
                 client->typical_response_time = (double)
(MQTT_PAL_TIME() - msg->time_sent);
                 /* check that connection was successful */
-                if (response.decoded.connack.return_code !=
MQTT_CONNACK_ACCEPTED) {
+                if (response.decoded.connack.return_code ==
MQTT_CONNACK_ACCEPTED) {
+                       client->error = MQTT_ACK;
+                 }
+                 else {
                     if (response.decoded.connack.return_code ==
MQTT_CONNACK_REFUSED_IDENTIFIER_REJECTED) {
                         client->error = MQTT_ERROR_CONNECT_CLIENT_ID_REFUSED;
                         mqtt_recv_ret = MQTT_ERROR_CONNECT_CLIENT_ID_REFUSED;

MQTT-C has an example usage for NuttX RTOS, I will submit a patch to them based on your solution, ok?

pieterconradie commented 1 year ago

Hi @acassis You are most welcome. Go ahead. Best regards, Pieter

acassis commented 1 year ago

@pieterconradie finally I got time to submit the PR to NuttX mainline: https://github.com/apache/nuttx-apps/pull/1748/commits/e90960d7437de6e896d4a878a98a2be28b010989

Thank you very much!