feat/ota_resume #1907

pvyawaha · 2020-04-15T17:32:28Z

OTA Suspend and Resume Feature

Description

This is a draft pull request not for merging.

Checklist:

I have tested my changes. No regression in existing tests.
My code is Linted.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Remove OTA demo specific conditional macros used in the demo. Remove dependency on network manager APIs. Remove dependency on aws_iot_demo_network.c Add MQTT disconnect reason logs in the disconnect callback. Update doxygen and other comments.

gkwicker

See comments

gkwicker · 2020-04-23T22:05:33Z

demos/ota/aws_iot_ota_update_demo.c

-
-#define myappONE_SECOND_DELAY_IN_TICKS    pdMS_TO_TICKS( 1000UL )
+/**
+ * @brief Timeout for MQTT connection, if the MQTT connection is not establihsed within


spelling: established

Fixing this, Thank you for pointing it out.

gkwicker · 2020-04-23T22:05:55Z

demos/ota/aws_iot_ota_update_demo.c

+ * @brief Timeout for MQTT connection, if the MQTT connection is not establihsed within
+ * this time, the connect function returns #IOT_MQTT_TIMEOUT
+ */
+#define OTA_DEMO_CONNECTION_TIMEOUT                  ( 2000UL )


What units is this in? If it is milliseconds, please rename to OTA_DEMO_CONNECTION_TIMEOUT_MS.

This must have the ms , changing it to OTA_DEMO_CONNECTION_TIMEOUT_MS

gkwicker · 2020-04-23T22:07:07Z

demos/ota/aws_iot_ota_update_demo.c

+ * by the MQTT 3.1.1 spec) is 23 characters. Add 1 to include the length of the NULL
+ * terminator.
+ */
+#define OTA_DEMO_CLIENT_IDENTIFIER_MAX_LENGTH        ( 24 )


Shouldn't this be available in an MQTT header file somewhere?

This is not defined in MQTT library , the MQTT demo has its definition similar to what we are doing here.

gkwicker · 2020-04-23T22:07:49Z

demos/ota/aws_iot_ota_update_demo.c

 }

-static BaseType_t prxCreateNetworkConnection( void )
+/**
+ * @brief Delay before retrying network connection upto a maximum interval.


spelling: up to

Fixing this to up to , thanks.

gkwicker · 2020-04-23T22:08:27Z

demos/ota/aws_iot_ota_update_demo.c

-            }
-        }
-    } while( ulRetriesLeft-- > 0 );
+    intervalTicks = pdMS_TO_TICKS( retryInterval * 1000 );


Ideally we would add some jitter here.

Adding jitter in commit 50e1814 using rand so devices will initiate reconnection anywhere within the interval window.

gkwicker · 2020-04-23T22:12:30Z

demos/ota/aws_iot_ota_update_demo.c

-        if( xMqttInitStatus != IOT_MQTT_SUCCESS )
-        {
-            xRet = EXIT_FAILURE;
+            while( ( ( eState = OTA_GetAgentState() ) != eOTA_AgentState_Stopped ) && networkConnected )


Where is networkConnected set to pdFALSE? I notice it's not declared volatile. If some other task is setting it, some compilers might optimize this test away.

_networkConnected is set false in disconnect callback, fixed in dec8c99

ravibhagavandas · 2020-04-24T21:37:56Z

demos/ota/aws_iot_ota_update_demo.c

+ * @brief The maximum time interval that is permitted to elapse between the point at
+ * which the MQTT client finishes transmitting one control Packet and the point it starts
+ * sending the next.In the absence of control packet a PINGREQ  is sent. The broker must
+ * disconnect a client that does not send a a message or a PINGREQ packet in one and a


correction send a message

Thanks , fixing this.

ravibhagavandas · 2020-04-24T21:44:30Z

demos/ota/aws_iot_ota_update_demo.c

+ * @brief The delay used in the main OTA Demo task loop to periodically output the OTA
+ * statistics like number of packets received, dropped, processed and queued per connection.
+ */
+#define OTA_DEMO_ONE_SECOND_DELAY                    pdMS_TO_TICKS( 1000UL )


Could this be made in seconds as well for easy configuration and understanding by user

Changed this in 0ac73bd

Also renamed it to make sense if the value is changed from 1 sec.

ravibhagavandas · 2020-04-24T21:46:36Z

demos/ota/aws_iot_ota_update_demo.c

-            /* Connect to one of the network type.*/
-            xRet = xMqttDemoCreateNetworkConnection( &xConnection, otaDemoNETWORK_TYPES );
-        #endif
+    configPRINTF( ( "Retrying network connection in %d Secs ", retryInterval ) );


Suggest using IotLog for logging.

Changed to IotLog in commit ea768f3

ravibhagavandas · 2020-04-24T22:24:28Z

demos/ota/aws_iot_ota_update_demo.c

-    ( void ) awsIotMqttMode;
-    ( void ) pIdentifier;
-    ( void ) pNetworkServerInfo;
+    retryInterval = OTA_DEMO_CONN_RETRY_BASE_INTERVAL_SECONDS;


Should retry interval be reset back to OTA_DEMO_CONN_RETRY_BASE_INTERVAL_SECONDS once a successful connection is made.

I agree it can be reset to base time interval, making a change to address this.

ravibhagavandas · 2020-04-24T22:29:51Z

demos/ota/aws_iot_ota_update_demo.c

-            xConnectInfo.clientIdentifierLength = ( uint16_t ) strlen( clientcredentialIOT_THING_NAME );
-            xConnectInfo.pClientIdentifier = clientcredentialIOT_THING_NAME;
+    /* Suspend OTA agent.*/
+    OTA_Suspend();


Should this be called from main OTA demo for(;;) loop before tearing down MQTT connection. Otherwise it can cause race condition if MQTT connection is deleted but OTA agent is still active.

This is moved in the OTA Demo loop in commit c567abe

ravibhagavandas · 2020-04-24T22:48:38Z

demos/ota/aws_iot_ota_update_demo.c

+            }
+
+            /* Try to close the MQTT connection. */
+            if( ( mqttConnection != NULL ) && networkConnected )


Teardown mqtt connection resources in network disconnected case as well ? , by calling IotMqtt_Disconnect( mqttConnection, true);

Fixed in commit dec8c99

* Add E2E tests for OTA suspend and resume * Add doctring to OTA E2E test cases * Fix typos in the OTA E2E tests docstring

…ertos into prasad/OTA_Suspend_Resume

…Resume

gkwicker

One small thing, see comments.

gkwicker · 2020-05-11T17:22:24Z

demos/ota/aws_iot_ota_update_demo.c

            {
                /* Wait forever for OTA traffic but allow other tasks to run and output statistics only once per second. */
-                vTaskDelay( OTA_DEMO_ONE_SECOND_DELAY );
+                IotClock_SleepMs( OTA_DEMO_TASK_DELAY * 1000 );


This should be named OTA_DEMO_TASK_DELAY_SECONDS. If a #define value is in units (time/distance/mass/etc.), include the unit in the name.

Changed in 1815858

pvyawaha added 2 commits April 15, 2020 10:14

Add suspended state and event to OTA Agent

4b6d100

Add resume even and handler to OTA Agent

95f79cb

pvyawaha closed this Apr 15, 2020

pvyawaha reopened this Apr 15, 2020

pvyawaha requested review from ravibhagavandas and gkwicker April 15, 2020 17:35

pvyawaha added 3 commits April 20, 2020 10:38

Refactor OTA Demo :

fcdd669

Remove OTA demo specific conditional macros used in the demo. Remove dependency on network manager APIs. Remove dependency on aws_iot_demo_network.c Add MQTT disconnect reason logs in the disconnect callback. Update doxygen and other comments.

OTA: Add exponential delay in retrying network connection

1397b0f

OTA: Stop request timer when Agent is suspended

9481630

gkwicker suggested changes Apr 23, 2020

View reviewed changes

ravibhagavandas reviewed Apr 24, 2020

View reviewed changes

pvyawaha and others added 7 commits April 29, 2020 10:49

Add jitter in OTA demo reconnection logic

50e1814

Change OTA demo logging to c-sdk loggging instead of configprintf

ea768f3

Fix OTA demo MQTT disconnect calls for cleanup condition

dec8c99

Change FreeRTOS delay to C-SDK delay in OTA Demo

0ac73bd

Add E2E tests for OTA suspend and resume (#1941)

390d39b

* Add E2E tests for OTA suspend and resume * Add doctring to OTA E2E test cases * Fix typos in the OTA E2E tests docstring

Add error return for the new OTA Agent APIs for suspend/resume

c567abe

Merge branch 'prasad/OTA_Suspend_Resume' of github.com:aws/amazon-fre…

2840aa3

…ertos into prasad/OTA_Suspend_Resume

pvyawaha requested a review from dcgaws May 8, 2020 20:48

Merge remote-tracking branch 'origin/master' into prasad/OTA_Suspend_…

5d43b28

…Resume

pvyawaha requested a review from dan4thewin May 8, 2020 21:24

Add missing standard c headers to the OTA demo

c112483

ravibhagavandas previously approved these changes May 8, 2020

View reviewed changes

pvyawaha removed the request for review from dan4thewin May 11, 2020 01:43

pvyawaha marked this pull request as ready for review May 11, 2020 17:12

gkwicker suggested changes May 11, 2020

View reviewed changes

Change OTA_DEMO_TASK_DELAY to OTA_DEMO_TASK_DELAY_SECONDS in OTA demo

1815858

pvyawaha dismissed ravibhagavandas’s stale review via 1815858 May 11, 2020 17:32

gkwicker approved these changes May 11, 2020

View reviewed changes

ravibhagavandas approved these changes May 11, 2020

View reviewed changes

pvyawaha merged commit cbf360f into master May 11, 2020

pvyawaha deleted the prasad/OTA_Suspend_Resume branch May 11, 2020 18:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat/ota_resume #1907

feat/ota_resume #1907

pvyawaha commented Apr 15, 2020

gkwicker left a comment

gkwicker Apr 23, 2020

pvyawaha Apr 28, 2020

gkwicker Apr 23, 2020

pvyawaha Apr 28, 2020

gkwicker Apr 23, 2020

pvyawaha Apr 28, 2020

gkwicker Apr 23, 2020

pvyawaha Apr 28, 2020

gkwicker Apr 23, 2020

pvyawaha Apr 28, 2020 •

edited

Loading

gkwicker Apr 23, 2020

pvyawaha May 5, 2020 •

edited

Loading

ravibhagavandas Apr 24, 2020

pvyawaha Apr 28, 2020

ravibhagavandas Apr 24, 2020

pvyawaha May 8, 2020

ravibhagavandas Apr 24, 2020

pvyawaha May 5, 2020

ravibhagavandas Apr 24, 2020

pvyawaha Apr 28, 2020

ravibhagavandas Apr 24, 2020

pvyawaha May 8, 2020

ravibhagavandas Apr 24, 2020

pvyawaha May 5, 2020

gkwicker left a comment

gkwicker May 11, 2020

pvyawaha May 11, 2020

feat/ota_resume #1907

feat/ota_resume #1907

Conversation

pvyawaha commented Apr 15, 2020

Description

Checklist:

gkwicker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pvyawaha Apr 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pvyawaha May 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkwicker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pvyawaha Apr 28, 2020 •

edited

Loading

pvyawaha May 5, 2020 •

edited

Loading