cloud: fix fallback provisioning network state handling

The fallback provisioning logic incorrectly determined the next state
when provisioning failed. The network_connected variable was not being
updated properly, causing incorrect state transitions.

This fix ensures the network connection status is checked accurately
before determining whether to retry provisioning or transition to an
offline state.

Added corresponding unit tests to verify the correct behavior of the
fallback provisioning logic under different network conditions.

Signed-off-by: Simen S. Røstad <simen.rostad@nordicsemi.no>

Author: simensrostad

app/src/modules/cloud/cloud.c

@@ -160,8 +160,8 @@ struct cloud_state_object {
 	/* Last received message */
 	uint8_t msg_buf[MAX_MSG_SIZE];
 
-	/* Network status */
-	enum network_msg_type nw_status;
+	/* Last network connection status */
+	bool network_connected;
 
 	/* Provisioning ongoing flag */
 	bool provisioning_ongoing;
@@ -877,9 +877,13 @@ static void state_connecting_provisioning_run(void *obj)
 			smf_set_state(SMF_CTX(state_object), &states[STATE_PROVISIONED]);
 
 			return;
-		} else if (msg == CLOUD_PROVISIONING_FAILED) {
+		} else if (msg == CLOUD_PROVISIONING_FAILED && state_object->network_connected) {
 			smf_set_state(SMF_CTX(state_object), &states[STATE_CONNECTING_BACKOFF]);
 
+			return;
+		} else if (msg == CLOUD_PROVISIONING_FAILED && !state_object->network_connected) {
+			smf_set_state(SMF_CTX(state_object), &states[STATE_DISCONNECTED]);
+
 			return;
 		}
 	}
@@ -1483,6 +1487,19 @@ static void state_connected_paused_run(void *obj)
 	}
 }
 
+static void network_connection_status_retain(struct cloud_state_object *state_object)
+{
+	if (state_object->chan == &NETWORK_CHAN) {
+		struct network_msg msg = MSG_TO_NETWORK_MSG(state_object->msg_buf);
+
+		if (msg.type == NETWORK_DISCONNECTED || msg.type == NETWORK_CONNECTED) {
+			/* Update network status to retain the last connection status */
+			state_object->network_connected =
+				(msg.type == NETWORK_CONNECTED) ? true : false;
+		}
+	}
+}
+
 static void cloud_module_thread(void)
 {
 	int err;
@@ -1525,6 +1542,8 @@ static void cloud_module_thread(void)
 			return;
 		}
 
+		network_connection_status_retain(&cloud_state);
+
 		err = smf_run_state(SMF_CTX(&cloud_state));
 		if (err) {
 			LOG_ERR("smf_run_state(), error: %d", err);

tests/module/cloud/src/cloud_module_test.c

@@ -211,7 +211,7 @@ static void connect_cloud(void)
 	err = zbus_chan_pub(&NETWORK_CHAN, &nw, K_NO_WAIT);
 	TEST_ASSERT_EQUAL(0, err);
 
-	k_sleep(K_MSEC(100));
+	k_sleep(K_MSEC(PROCESSING_DELAY_MS));
 }
 
 static void dummy_cb(const struct zbus_channel *chan)
@@ -516,7 +516,7 @@ void test_connected_disconnected_to_connected_send_payload_disconnect(void)
 	TEST_ASSERT_EQUAL(0, err);
 
 	/* Transport module needs CPU to run state machine */
-	k_sleep(K_MSEC(100));
+	k_sleep(K_MSEC(PROCESSING_DELAY_MS));
 
 	err = k_sem_take(&cloud_connected, K_SECONDS(1));
 	TEST_ASSERT_EQUAL(0, err);
@@ -525,7 +525,7 @@ void test_connected_disconnected_to_connected_send_payload_disconnect(void)
 	TEST_ASSERT_EQUAL(0, err);
 
 	/* Transport module needs CPU to run state machine */
-	k_sleep(K_MSEC(100));
+	k_sleep(K_MSEC(PROCESSING_DELAY_MS));
 
 	TEST_ASSERT_EQUAL(1, nrf_cloud_coap_json_message_send_fake.call_count);
 	TEST_ASSERT_EQUAL(0, strncmp(nrf_cloud_coap_json_message_send_fake.arg0_val,
@@ -539,7 +539,7 @@ void test_connected_disconnected_to_connected_send_payload_disconnect(void)
 	TEST_ASSERT_EQUAL(0, err);
 
 	/* Transport module needs CPU to run state machine */
-	k_sleep(K_MSEC(100));
+	k_sleep(K_MSEC(PROCESSING_DELAY_MS));
 
 	err = k_sem_take(&cloud_disconnected, K_SECONDS(1));
 	TEST_ASSERT_EQUAL(0, err);
@@ -597,7 +597,7 @@ void test_gnss_location_data_handling(void)
 	TEST_ASSERT_EQUAL(0, err);
 
 	/* Give the module time to process */
-	k_sleep(K_MSEC(100));
+	k_sleep(K_MSEC(PROCESSING_DELAY_MS));
 
 	/* Verify that GNSS location data was sent to nRF Cloud */
 	TEST_ASSERT_EQUAL(1, nrf_cloud_coap_location_send_fake.call_count);
@@ -629,7 +629,7 @@ void test_storage_data_battery_sent_to_cloud(void)
 	TEST_ASSERT_EQUAL(0, err);
 
 	/* Allow processing */
-	k_sleep(K_MSEC(100));
+	k_sleep(K_MSEC(PROCESSING_DELAY_MS));
 
 	/* One successful read + one -EAGAIN drain */
 	TEST_ASSERT_EQUAL(2, storage_batch_read_fake.call_count);
@@ -657,7 +657,7 @@ void test_storage_data_environmental_sent_to_cloud(void)
 	ARG_UNUSED(err);
 
 	/* Allow processing */
-	k_sleep(K_MSEC(100));
+	k_sleep(K_MSEC(PROCESSING_DELAY_MS));
 
 	/* One successful read + one -EAGAIN drain */
 	TEST_ASSERT_EQUAL(2, storage_batch_read_fake.call_count);
@@ -685,14 +685,103 @@ void test_storage_data_network_conn_eval_sent_to_cloud(void)
 	ARG_UNUSED(err);
 
 	/* Allow processing */
-	k_sleep(K_MSEC(100));
+	k_sleep(K_MSEC(PROCESSING_DELAY_MS));
 
 	/* One successful read + one -EAGAIN drain */
 	TEST_ASSERT_EQUAL(2, storage_batch_read_fake.call_count);
 	/* Expect two sensor publishes: CONEVAL and RSRP */
 	TEST_ASSERT_EQUAL(2, nrf_cloud_coap_sensor_send_fake.call_count);
 }
 
+void test_provisioning_failed_with_network_connected_should_go_to_backoff(void)
+{
+	int err;
+	struct network_msg network_msg = {
+		.type = NETWORK_CONNECTED
+	};
+	struct cloud_msg cloud_msg = {
+		.type = CLOUD_PROVISIONING_REQUEST
+	};
+	struct nrf_provisioning_callback_data event = {
+		.type = NRF_PROVISIONING_EVENT_FAILED
+	};
+
+	/* Start with a connected network state */
+	zbus_chan_pub(&NETWORK_CHAN, &network_msg, K_NO_WAIT);
+	k_sleep(K_MSEC(PROCESSING_DELAY_MS));
+
+	/* Should now be connected */
+	err = k_sem_take(&cloud_connected, K_SECONDS(WAIT_TIMEOUT));
+	TEST_ASSERT_EQUAL(0, err);
+
+	/* Trigger provisioning request */
+	zbus_chan_pub(&CLOUD_CHAN, &cloud_msg, K_NO_WAIT);
+	k_sleep(K_MSEC(PROCESSING_DELAY_MS));
+
+	/* Should now be disconnected (entering provisioning state) */
+	err = k_sem_take(&cloud_disconnected, K_SECONDS(WAIT_TIMEOUT));
+	TEST_ASSERT_EQUAL(0, err);
+
+	TEST_ASSERT_EQUAL(1, nrf_provisioning_trigger_manually_fake.call_count);
+
+	/* Simulate provisioning failure while network is still connected */
+	handler(&event);
+
+	/* Allow time for state machine to process the failure and enter backoff state */
+	k_sleep(K_SECONDS(INITIAL_PROVISIONING_RETRY_SEC + 1));
+
+	/* Verify that provisioning is retried after backoff
+	 * (indicating we went to backoff state)
+	 */
+	TEST_ASSERT_EQUAL(2, nrf_provisioning_trigger_manually_fake.call_count);
+}
+
+void test_provisioning_failed_with_network_disconnected_should_go_to_disconnected(void)
+{
+	int err;
+	struct network_msg network_msg = {
+		.type = NETWORK_CONNECTED
+	};
+	struct cloud_msg cloud_msg = {
+		.type = CLOUD_PROVISIONING_REQUEST
+	};
+	struct nrf_provisioning_callback_data event = {
+		.type = NRF_PROVISIONING_EVENT_FAILED
+	};
+
+	/* Start with a connected network state */
+	zbus_chan_pub(&NETWORK_CHAN, &network_msg, K_NO_WAIT);
+	k_sleep(K_MSEC(PROCESSING_DELAY_MS));
+
+	/* Should now be connected */
+	err = k_sem_take(&cloud_connected, K_SECONDS(WAIT_TIMEOUT));
+	TEST_ASSERT_EQUAL(0, err);
+
+	/* Trigger provisioning request */
+	zbus_chan_pub(&CLOUD_CHAN, &cloud_msg, K_NO_WAIT);
+	k_sleep(K_MSEC(PROCESSING_DELAY_MS));
+
+	/* Should now be disconnected (entering provisioning state) */
+	err = k_sem_take(&cloud_disconnected, K_SECONDS(WAIT_TIMEOUT));
+	TEST_ASSERT_EQUAL(0, err);
+
+	TEST_ASSERT_EQUAL(1, nrf_provisioning_trigger_manually_fake.call_count);
+
+	/* Simulate network disconnect while in provisioning state */
+	network_msg.type = NETWORK_DISCONNECTED;
+	zbus_chan_pub(&NETWORK_CHAN, &network_msg, K_NO_WAIT);
+	k_sleep(K_MSEC(PROCESSING_DELAY_MS));
+
+	/* Simulate provisioning failure while network is disconnected */
+	handler(&event);
+
+	/* Allow time for state machine to process the failure and enter disconnected state */
+	k_sleep(K_SECONDS(INITIAL_PROVISIONING_RETRY_SEC + 1));
+
+	/* Verify that provisioning is NOT retried (indicating we went to disconnected state) */
+	TEST_ASSERT_EQUAL(1, nrf_provisioning_trigger_manually_fake.call_count);
+}
+
 /* This is required to be added to each test. That is because unity's
  * main may return nonzero, while zephyr's main currently must
  * return 0 in all cases (other values are reserved).

tests/state_machines/source_of_truth/cloud.puml

@@ -1,49 +1,41 @@
 @startuml
-[*] --> STATE_RUNNING
-
 state STATE_RUNNING {
     [*] --> STATE_DISCONNECTED
 
     state STATE_DISCONNECTED
     state STATE_CONNECTING
     state STATE_CONNECTED
 
-    STATE_DISCONNECTED --> STATE_CONNECTING: NETWORK_CONNECTED
-    STATE_CONNECTING --> STATE_DISCONNECTED: NETWORK_DISCONNECTED
+    STATE_DISCONNECTED --> STATE_CONNECTING : NETWORK_CONNECTED
 
     state STATE_CONNECTING {
         [*] --> STATE_CONNECTING_ATTEMPT
 
         state STATE_CONNECTING_ATTEMPT {
             [*] --> STATE_PROVISIONED
-            state STATE_PROVISIONED
-            state STATE_PROVISIONING
 
-            STATE_PROVISIONED --> STATE_PROVISIONING: CLOUD_NOT_AUTHENTICATED
-            STATE_PROVISIONED --> STATE_CONNECTED: CLOUD_CONNECTION_SUCCESS
-            STATE_PROVISIONED --> STATE_CONNECTING_BACKOFF: CLOUD_CONNECTION_FAILED
-
-            STATE_PROVISIONING --> STATE_PROVISIONED: CLOUD_PROVISIONING_FINISHED
-            STATE_PROVISIONING --> STATE_CONNECTING_BACKOFF: CLOUD_PROVISIONING_FAILED
+            STATE_PROVISIONED --> STATE_PROVISIONING : CLOUD_NOT_AUTHENTICATED
+            STATE_PROVISIONED --> STATE_CONNECTED : CLOUD_CONNECTION_SUCCESS
+            STATE_PROVISIONED --> STATE_CONNECTING_BACKOFF : CLOUD_CONNECTION_FAILED
         }
 
-        state STATE_CONNECTING_BACKOFF
-
-        STATE_CONNECTING_BACKOFF --> STATE_PROVISIONED: CLOUD_BACKOFF_EXPIRED && !provisioning_ongoing
-        STATE_CONNECTING_BACKOFF --> STATE_PROVISIONING: CLOUD_BACKOFF_EXPIRED && provisioning_ongoing
+        STATE_CONNECTING --> STATE_DISCONNECTED : NETWORK_DISCONNECTED
+        STATE_PROVISIONING --> STATE_PROVISIONED : CLOUD_PROVISIONING_FINISHED
+        STATE_PROVISIONING --> STATE_CONNECTING_BACKOFF : CLOUD_PROVISIONING_FAILED && network_connected
+        STATE_PROVISIONING --> STATE_DISCONNECTED : CLOUD_PROVISIONING_FAILED && !network_connected
+        STATE_CONNECTING_BACKOFF --> STATE_PROVISIONED : CLOUD_BACKOFF_EXPIRED && !provisioning_ongoing
+        STATE_CONNECTING_BACKOFF --> STATE_PROVISIONING : CLOUD_BACKOFF_EXPIRED && provisioning_ongoing
     }
 
     state STATE_CONNECTED {
         [*] --> STATE_CONNECTED_READY
 
-        state STATE_CONNECTED_READY
-        state STATE_CONNECTED_PAUSED
-
-        STATE_CONNECTED_READY --> STATE_CONNECTED_PAUSED: NETWORK_DISCONNECTED
-        STATE_CONNECTED_PAUSED --> STATE_CONNECTED_READY: NETWORK_CONNECTED
-
-        STATE_CONNECTED_READY --> STATE_CONNECTING: CLOUD_SEND_REQUEST_FAILED
-        STATE_CONNECTED_READY --> STATE_PROVISIONING: CLOUD_PROVISIONING_REQUEST
+        STATE_CONNECTED_READY --> STATE_CONNECTED_PAUSED : NETWORK_DISCONNECTED
+        STATE_CONNECTED_READY --> STATE_CONNECTING : CLOUD_SEND_REQUEST_FAILED
+        STATE_CONNECTED_READY --> STATE_PROVISIONING : CLOUD_PROVISIONING_REQUEST
+        STATE_CONNECTED_PAUSED --> STATE_CONNECTED_READY : NETWORK_CONNECTED
     }
 }
-@enduml
+
+[*] --> STATE_RUNNING
+@enduml