Fix keep alive interval check (#163)

* Fix keep alive checking interval

* Update default ping response timeout to a more reasonable value

* Update changelog

* Update doc for MQTT_PINGRESP_TIMEOUT_MS

CHANGELOG.md

@@ -1,5 +1,10 @@
 # Changelog for coreMQTT Client Library
 
+## Commits to `main`
+
+### Updates
+ - [#163](https://github.com/FreeRTOS/coreMQTT/pull/163) Fix bug to check for ping responses within `MQTT_PINGRESP_TIMEOUT_MS` instead of the entire keep alive interval
+
 ## v1.1.1 (February 2021)
 
 ### Changes

source/core_mqtt.c

@@ -1003,8 +1003,7 @@ static MQTTStatus_t handleKeepAlive( MQTTContext_t * pContext )
     keepAliveMs = 1000U * ( uint32_t ) pContext->keepAliveIntervalSec;
 
     /* If keep alive interval is 0, it is disabled. */
-    if( ( keepAliveMs != 0U ) &&
-        ( calculateElapsedTime( now, pContext->lastPacketTime ) > keepAliveMs ) )
+    if( keepAliveMs != 0U )
     {
         if( pContext->waitingForPingResp == true )
         {
@@ -1017,7 +1016,10 @@ static MQTTStatus_t handleKeepAlive( MQTTContext_t * pContext )
         }
         else
         {
-            status = MQTT_Ping( pContext );
+            if( calculateElapsedTime( now, pContext->lastPacketTime ) > keepAliveMs )
+            {
+                status = MQTT_Ping( pContext );
+            }
         }
     }
 

source/include/core_mqtt_config_defaults.h

@@ -94,23 +94,27 @@
 #endif
 
 /**
- * @brief Number of milliseconds to wait for a ping response to a ping
+ * @brief Maximum number of milliseconds to wait for a ping response to a ping
  * request as part of the keep-alive mechanism.
  *
  * If a ping response is not received before this timeout, then
  * #MQTT_ProcessLoop will return #MQTTKeepAliveTimeout.
  *
+ * @note If this value is more than half of the keep alive interval, and the
+ * server does not receive the previous ping request, then it is likely that the
+ * server will disconnect the client before #MQTTKeepAliveTimeout can be returned.
+ *
  * @note If a dummy implementation of the #MQTTGetCurrentTimeFunc_t timer function,
  * is supplied to the library, then the keep-alive mechanism is not supported by the
  * #MQTT_ProcessLoop API function. In that case, the value of #MQTT_PINGRESP_TIMEOUT_MS
  * is irrelevant to the behavior of the library.
  *
  * <b>Possible values:</b> Any positive integer up to SIZE_MAX. <br>
- * <b>Default value:</b> `500`
+ * <b>Default value:</b> `5000`
  */
 #ifndef MQTT_PINGRESP_TIMEOUT_MS
-    /* Wait 0.5 seconds by default for a ping response. */
-    #define MQTT_PINGRESP_TIMEOUT_MS    ( 500U )
+    /* Wait 5 seconds by default for a ping response. */
+    #define MQTT_PINGRESP_TIMEOUT_MS    ( 5000U )
 #endif
 
 /**

test/cbmc/include/core_mqtt_config.h

@@ -68,7 +68,7 @@ struct NetworkContext
  * If a ping response is not received before this timeout, then
  * #MQTT_ProcessLoop will return #MQTTKeepAliveTimeout.
  */
-#define MQTT_PINGRESP_TIMEOUT_MS                ( 500U )
+#define MQTT_PINGRESP_TIMEOUT_MS                ( 5000U )
 
 /**
  * @brief The maximum duration of receiving no data over network when

test/unit-test/core_mqtt_utest.c

@@ -490,7 +490,8 @@ static void expectProcessLoopCalls( MQTTContext_t * const pContext,
     if( modifyIncomingPacketStatus == MQTTNoDataAvailable )
     {
         if( ( pContext->waitingForPingResp == false ) &&
-            ( pContext->keepAliveIntervalSec != 0U ) )
+            ( pContext->keepAliveIntervalSec != 0U ) &&
+            ( ( globalEntryTime - pContext->lastPacketTime ) > ( 1000U * pContext->keepAliveIntervalSec ) ) )
         {
             MQTT_GetPingreqPacketSize_ExpectAnyArgsAndReturn( MQTTSuccess );
             /* Replace pointer parameter being passed to the method. */
@@ -1929,10 +1930,9 @@ void test_MQTT_ProcessLoop_handleKeepAlive_Happy_Paths( void )
     expectProcessLoopCalls( &context, &expectParams );
 
     /* Coverage for the branch path where keep alive interval is greater than 0,
-     * and the interval has expired. */
+     * and the interval has not expired. PINGREQ should not be sent. */
     mqttStatus = MQTT_Init( &context, &transport, getTime, eventCallback, &networkBuffer );
     TEST_ASSERT_EQUAL( MQTTSuccess, mqttStatus );
-    context.waitingForPingResp = true;
     context.keepAliveIntervalSec = MQTT_SAMPLE_KEEPALIVE_INTERVAL_S;
     context.lastPacketTime = getTime();
     /* Set expected return values in the loop. All success. */
@@ -1944,7 +1944,7 @@ void test_MQTT_ProcessLoop_handleKeepAlive_Happy_Paths( void )
     TEST_ASSERT_EQUAL( MQTTSuccess, mqttStatus );
     context.waitingForPingResp = true;
     context.keepAliveIntervalSec = MQTT_SAMPLE_KEEPALIVE_INTERVAL_S;
-    context.lastPacketTime = 0;
+    context.lastPacketTime = MQTT_ONE_SECOND_TO_MS;
     context.pingReqSendTimeMs = MQTT_ONE_SECOND_TO_MS;
     /* Set expected return values in the loop. All success. */
     resetProcessLoopParams( &expectParams );
@@ -1977,7 +1977,7 @@ void test_MQTT_ProcessLoop_handleKeepAlive_Error_Paths( void )
     setupTransportInterface( &transport );
 
     modifyIncomingPacketStatus = MQTTNoDataAvailable;
-    globalEntryTime = MQTT_ONE_SECOND_TO_MS;
+    globalEntryTime = MQTT_PINGRESP_TIMEOUT_MS + 1;
 
     /* Coverage for the branch path where PINGRESP timeout interval has expired. */
     mqttStatus = MQTT_Init( &context, &transport, getTime, eventCallback, &networkBuffer );