tuanpmt / esp_mqtt

MQTT client library for ESP8266
http://tuanpm.net/post/esp_mqtt/
MIT License
1.15k stars 401 forks source link

publish callback in wrong place for QOS > 0 #163

Open davydnorris opened 5 years ago

davydnorris commented 5 years ago

The user defined publish callback is set to be called when a message is successfully sent to the MQTT server. For QOS0 this is OK, but for QOS1 and QOS2 the rest of the publish transaction is being ignored.

In these cases the correct place to call the publishCb callback in mqtt.c would be after line 388 for QOS1 and after line 406 for QOS2:

          case MQTT_MSG_TYPE_PUBACK:
            if (client->mqtt_state.pending_msg_type == MQTT_MSG_TYPE_PUBLISH && client->mqtt_state.pending_msg_id == msg_id) {
/********** QOS1 here ************/
              MQTT_INFO("MQTT: received MQTT_MSG_TYPE_PUBACK, finish QoS1 publish\r\n");
            }

            break;
          case MQTT_MSG_TYPE_PUBREC:
            client->mqtt_state.outbound_message = mqtt_msg_pubrel(&client->mqtt_state.mqtt_connection, msg_id);
            if (QUEUE_Puts(&client->msgQueue, client->mqtt_state.outbound_message->data, client->mqtt_state.outbound_message->length) == -1) {
              MQTT_INFO("MQTT: Queue full\r\n");
            }
            break;
          case MQTT_MSG_TYPE_PUBREL:
            client->mqtt_state.outbound_message = mqtt_msg_pubcomp(&client->mqtt_state.mqtt_connection, msg_id);
            if (QUEUE_Puts(&client->msgQueue, client->mqtt_state.outbound_message->data, client->mqtt_state.outbound_message->length) == -1) {
              MQTT_INFO("MQTT: Queue full\r\n");
            }
            break;
          case MQTT_MSG_TYPE_PUBCOMP:
            if (client->mqtt_state.pending_msg_type == MQTT_MSG_TYPE_PUBLISH && client->mqtt_state.pending_msg_id == msg_id) {
/********** QOS2 here ************/
              MQTT_INFO("MQTT: receive MQTT_MSG_TYPE_PUBCOMP, finish QoS2 publish\r\n");
            }
            break;

In addition, there would need to be a check for QOS0 in the current spot where it's called (line 463) but I can't see where the QOS value for the transaction can be accessed.

davydnorris commented 5 years ago

I discovered this little wrinkle because I'm deep sleeping my device regularly, and so have transaction counting in place to work out when the device has completed and can sleep. The publishCb function was being called too early for QOS1 and the unit was sleeping before the last transaction had completely finished.

davydnorris commented 5 years ago

OK I found where the pending QoS can be retrieved so line 463 would need to be:

    if ((client->connState == MQTT_DATA || client->connState == MQTT_KEEPALIVE_SEND)
            && client->mqtt_state.pending_msg_type == MQTT_MSG_TYPE_PUBLISH) {
        if (client->publishedCb && client->mqtt_state.pending_publish_qos == QOS0)
            client->publishedCb((uint32_t*) client);
    }
davydnorris commented 5 years ago

So it turns out that mqtt_state.pending_publish_qos is never set in the code so it's always 0 (QOS0). This would need to be set just before sending the message at line 744:

client->mqtt_state.pending_publish_qos = mqtt_msg_get_qos(dataBuffer);