Fix the error callback from not being called sometimes (#1683)

* Fix the error callback from not being called sometimes
* Moved the setting of the reconnect status to after the connack is recevied
* Increase the irom0_seg size
* Updated the documentation
* Make it clearer that autoreconnect is deprecated

app/modules/mqtt.c

@@ -1,5 +1,5 @@
 // Module for mqtt
-
+//
 #include "module.h"
 #include "lauxlib.h"
 #include "platform.h"
@@ -42,10 +42,14 @@ typedef struct mqtt_event_data_t
   uint16_t data_offset;
 } mqtt_event_data_t;
 
+#define RECONNECT_OFF   0
+#define RECONNECT_POSSIBLE 1
+#define RECONNECT_ON    2
+
 typedef struct mqtt_state_t
 {
   uint16_t port;
-  int auto_reconnect;
+  uint8_t auto_reconnect;   // 0 is not auto_reconnect. 1 is auto reconnect, but never connected. 2 is auto reconnect, but once connected
   mqtt_connect_info_t* connect_info;
   uint16_t message_length;
   uint16_t message_length_read;
@@ -106,7 +110,7 @@ static void mqtt_socket_disconnected(void *arg)    // tcp only
     }
   }
 
-  if(mud->mqtt_state.auto_reconnect){
+  if(mud->mqtt_state.auto_reconnect == RECONNECT_ON) {
     mud->pesp_conn->reverse = mud;
     mud->pesp_conn->type = ESPCONN_TCP;
     mud->pesp_conn->state = ESPCONN_NONE;
@@ -153,7 +157,7 @@ static void mqtt_socket_reconnected(void *arg, sint8_t err)
 
   mud->event_timeout = 0; // no need to count anymore
 
-  if(mud->mqtt_state.auto_reconnect){
+  if(mud->mqtt_state.auto_reconnect == RECONNECT_ON) {
     pesp_conn->proto.tcp->remote_port = mud->mqtt_state.port;
     pesp_conn->proto.tcp->local_port = espconn_port();
     socket_connect(pesp_conn);
@@ -166,6 +170,9 @@ static void mqtt_socket_reconnected(void *arg, sint8_t err)
     {
       espconn_disconnect(pesp_conn);
     }
+
+    mqtt_connack_fail(mud, MQTT_CONN_FAIL_SERVER_NOT_FOUND);
+
     mqtt_socket_disconnected(arg);
   }
   NODE_DBG("leave mqtt_socket_reconnected.\n");
@@ -330,6 +337,9 @@ READPACKET:
       } else {
         mud->connState = MQTT_DATA;
         NODE_DBG("MQTT: Connected\r\n");
+        if (mud->mqtt_state.auto_reconnect == RECONNECT_POSSIBLE) {
+          mud->mqtt_state.auto_reconnect = RECONNECT_ON;
+        }
         if(mud->cb_connect_ref == LUA_NOREF)
           break;
         if(mud->self_ref == LUA_NOREF)
@@ -603,6 +613,16 @@ void mqtt_socket_timer(void *arg)
     NODE_DBG("Can not connect to broker.\n");
     os_timer_disarm(&mud->mqttTimer);
     mqtt_connack_fail(mud, MQTT_CONN_FAIL_SERVER_NOT_FOUND);
+#ifdef CLIENT_SSL_ENABLE
+    if(mud->secure)
+    {
+      espconn_secure_disconnect(mud->pesp_conn);
+    }
+    else
+#endif
+    {
+      espconn_disconnect(mud->pesp_conn);
+    }
   } else if(mud->connState == MQTT_CONNECT_SENDING){ // MQTT_CONNECT send time out.
     NODE_DBG("sSend MQTT_CONNECT failed.\n");
     mud->connState = MQTT_INIT;
@@ -779,7 +799,7 @@ static int mqtt_socket_client( lua_State* L )
   mud->connect_info.keepalive = keepalive;
 
   mud->mqtt_state.pending_msg_q = NULL;
-  mud->mqtt_state.auto_reconnect = 0;
+  mud->mqtt_state.auto_reconnect = RECONNECT_OFF;
   mud->mqtt_state.port = 1883;
   mud->mqtt_state.connect_info = &mud->connect_info;
 
@@ -924,7 +944,7 @@ static sint8 socket_dns_found(const char *name, ip_addr_t *ipaddr, void *arg)
   {
     dns_reconn_count++;
     if( dns_reconn_count >= 5 ){
-      NODE_ERR( "DNS Fail!\n" );
+      NODE_DBG( "DNS Fail!\n" );
       // Note: should delete the pesp_conn or unref self_ref here.
 
       struct espconn *pesp_conn = arg;
@@ -938,7 +958,7 @@ static sint8 socket_dns_found(const char *name, ip_addr_t *ipaddr, void *arg)
       mqtt_socket_disconnected(arg);   // although not connected, but fire disconnect callback to release every thing.
       return -1;
     }
-    NODE_ERR( "DNS retry %d!\n", dns_reconn_count );
+    NODE_DBG( "DNS retry %d!\n", dns_reconn_count );
     host_ip.addr = 0;
     return espconn_gethostbyname(pesp_conn, name, &host_ip, socket_dns_foundcb);
   }
@@ -968,7 +988,7 @@ static int mqtt_socket_connect( lua_State* L )
   ip_addr_t ipaddr;
   const char *domain;
   int stack = 1;
-  unsigned secure = 0, auto_reconnect = 0;
+  unsigned secure = 0, auto_reconnect = RECONNECT_OFF;
   int top = lua_gettop(L);
   sint8 espconn_status;
 
@@ -1054,11 +1074,11 @@ static int mqtt_socket_connect( lua_State* L )
   {
     auto_reconnect = lua_tointeger(L, stack);
     stack++;
-    if ( auto_reconnect != 0 && auto_reconnect != 1 ){
-      auto_reconnect = 0; // default to 0
+    if ( auto_reconnect != RECONNECT_OFF && auto_reconnect != RECONNECT_POSSIBLE ){
+      auto_reconnect = RECONNECT_OFF; // default to 0
     }
   } else {
-    auto_reconnect = 0; // default to 0
+    auto_reconnect = RECONNECT_OFF; // default to 0
   }
   mud->mqtt_state.auto_reconnect = auto_reconnect;
 
@@ -1128,7 +1148,7 @@ static int mqtt_socket_close( lua_State* L )
     return 1;
   }
 
-  mud->mqtt_state.auto_reconnect = 0;   // stop auto reconnect.
+  mud->mqtt_state.auto_reconnect = RECONNECT_OFF;   // stop auto reconnect.
 
   sint8 espconn_status = ESPCONN_CONN;
   if (mud->connected) {

docs/en/modules/mqtt.md

@@ -18,7 +18,7 @@ Creates a MQTT client.
 - `keepalive` keepalive seconds
 - `username` user name
 - `password` user password
-- `cleansession` 0/1 for `false`/`true`
+- `cleansession` 0/1 for `false`/`true`. Default is 1 (`true`).
 
 #### Returns
 MQTT client
@@ -96,13 +96,41 @@ Connects to the broker specified by the given host, port, and secure options.
 - `host` host, domain or IP (string)
 - `port` broker port (number), default 1883
 - `secure` 0/1 for `false`/`true`, default 0. Take note of constraints documented in the [net module](net.md).
-- `autoreconnect` 0/1 for `false`/`true`, default 0
+- `autoreconnect` 0/1 for `false`/`true`, default 0. This option is *deprecated*.
 - `function(client)` callback function for when the connection was established
-- `function(client, reason)` callback function for when the connection could not be established
+- `function(client, reason)` callback function for when the connection could not be established. No further callbacks should be called.
 
 #### Returns
 `true` on success, `false` otherwise
 
+#### Notes
+
+Don't use `autoreconnect`. Let me repeat that, don't use `autoreconnect`. You should handle the errors explicitly and appropriately for
+your application. In particular, the default for `cleansession` above is `true`, so all subscriptions are destroyed when the connection
+is lost for any reason.
+
+In order to acheive a consistent connection, handle errors in the error callback. For example:
+
+```
+function handle_mqtt_error(client, reason) 
+  tmr.create():alarm(10 * 1000, tmr.ALARM_SINGLE, do_mqtt_connect)
+end
+
+function do_mqtt_connect()
+  mqtt:connect("server", function(client) print("connected") end, handle_mqtt_error)
+end
+```
+
+In reality, the connected function should do something useful!
+
+This is the description of how the `autoreconnect` functionality may (or may not) work.
+
+> When `autoreconnect` is set, then the connection will be re-established when it breaks. No error indication will be given (but all the
+> subscriptions may be lost if `cleansession` is true). However, if the
+> very first connection fails, then no reconnect attempt is made, and the error is signalled through the callback (if any). The first connection
+> is considered a success if the client connects to a server and gets back a good response packet in response to its MQTT connection request.
+> This implies (for example) that the username and password are correct.
+
 #### Connection failure callback reason codes:
 
 | Constant | Value | Description |