modules: transport: Add tests and fix found bugs

- Add tests for all states and transitions
- Fix bugs found when adding tests, such as no transition to
  _BACKOFF state.
- Remove private message types for successful and failed connection
  establishment. These are not needed anymore as the _connect()
  call happens synchronously in the state machine context.

Signed-off-by: Jan Tore Guggedal <jantore.guggedal@nordicsemi.no>

Author: jtguggedal

app/src/modules/transport/Kconfig.transport

@@ -14,23 +14,23 @@ config APP_TRANSPORT_BACKOFF_INITIAL_SECONDS
 	  Time in between reconnection attempts to the CoAP server.
 	  The timer starts after the last failed attempt.
 
-choice APP_TRANSPORT_BACKOFF_BACKOFF_TYPE
+choice APP_TRANSPORT_BACKOFF_TYPE
 	prompt "Reconnection backoff type"
-	default APP_TRANSPORT_BACKOFF_BACKOFF_TYPE_LINEAR
+	default APP_TRANSPORT_BACKOFF_TYPE_LINEAR
 
-config APP_TRANSPORT_BACKOFF_BACKOFF_TYPE_EXPONENTIAL
+config APP_TRANSPORT_BACKOFF_TYPE_EXPONENTIAL
 	bool "Exponential backoff"
 	help
 	  Exponential backoff doubles the reconnection timeout after each failed attempt.
 	  The maximum reconnection timeout is defined by APP_TRANSPORT_BACKOFF_MAX_SECONDS.
 
-config APP_TRANSPORT_BACKOFF_BACKOFF_TYPE_LINEAR
+config APP_TRANSPORT_BACKOFF_TYPE_LINEAR
 	bool "Linear backoff"
 	help
 	  Linear backoff adds a fixed amount of time to the reconnection timeout after each failed attempt,
 	  as defined by APP_TRANSPORT_BACKOFF_LINEAR_INCREMENT_SECONDS.
 
-config APP_TRANSPORT_BACKOFF_BACKOFF_TYPE_NONE
+config APP_TRANSPORT_BACKOFF_TYPE_NONE
 	bool "No backoff"
 	help
 	  No backoff means that the reconnection timeout is constant at the value defined by
@@ -41,7 +41,6 @@ endchoice
 config APP_TRANSPORT_BACKOFF_LINEAR_INCREMENT_SECONDS
 	int "Reconnection backoff time increment"
 	default 60
-	depends on APP_TRANSPORT_BACKOFF_BACKOFF_TYPE_LINEAR
 	help
 	  Time added to the reconnection timeout after each failed attempt in seconds.
 	  The maximum reconnection timeout is defined by APP_TRANSPORT_BACKOFF_MAX_SECONDS.

app/src/modules/transport/transport.c

@@ -38,8 +38,6 @@ ZBUS_CHAN_ADD_OBS(BATTERY_CHAN, transport, 0);
 
 /* Enumerator to be used in privat transport channel */
 enum priv_transport_msg {
-	CLOUD_CONN_SUCCES,
-	CLOUD_CONN_FAILED,
 	CLOUD_BACKOFF_EXPIRED,
 };
 
@@ -53,7 +51,7 @@ ZBUS_CHAN_DEFINE(PRIV_TRANSPORT_CHAN,
 		 NULL,
 		 NULL,
 		 ZBUS_OBSERVERS(transport),
-		 CLOUD_CONN_FAILED
+		 CLOUD_BACKOFF_EXPIRED
 );
 
 /* Connection attempt backoff timer is run as a delayable work on the system workqueue */
@@ -73,7 +71,6 @@ static void state_disconnected_run(void *o);
 static void state_connecting_entry(void *o);
 
 static void state_connecting_attempt_entry(void *o);
-static void state_connecting_attempt_run(void *o);
 
 static void state_connecting_backoff_entry(void *o);
 static void state_connecting_backoff_run(void *o);
@@ -130,7 +127,7 @@ static const struct smf_state states[] = {
 				&states[STATE_CONNECTING_ATTEMPT]),
 
 	[STATE_CONNECTING_ATTEMPT] = SMF_CREATE_STATE(
-				state_connecting_attempt_entry, state_connecting_attempt_run, NULL,
+				state_connecting_attempt_entry, NULL, NULL,
 				&states[STATE_CONNECTING],
 				NULL),
 
@@ -192,7 +189,6 @@ static void connect_to_cloud(void)
 {
 	int err;
 	char buf[NRF_CLOUD_CLIENT_ID_MAX_LEN];
-	enum priv_transport_msg conn_result = CLOUD_CONN_SUCCES;
 
 	err = nrf_cloud_client_id_get(buf, sizeof(buf));
 	if (err == 0) {
@@ -206,27 +202,25 @@ static void connect_to_cloud(void)
 
 	err = nrf_cloud_coap_connect(APP_VERSION_STRING);
 	if (err == 0) {
-		err = zbus_chan_pub(&PRIV_TRANSPORT_CHAN, &conn_result, K_SECONDS(1));
-		if (err) {
-			LOG_ERR("zbus_chan_pub, error: %d", err);
-			SEND_FATAL_ERROR();
-		}
+		STATE_SET(transport_state, STATE_CONNECTED);
 
 		return;
 	}
 
 	/* Connection failed, retry */
 	LOG_ERR("nrf_cloud_coap_connect, error: %d", err);
+
+	STATE_SET(transport_state, STATE_CONNECTING_BACKOFF);
 }
 
 static uint32_t calculate_backoff_time(uint32_t attempts)
 {
 	uint32_t backoff_time = CONFIG_APP_TRANSPORT_BACKOFF_INITIAL_SECONDS;
 
 	/* Calculate backoff time */
-	if (IS_ENABLED(CONFIG_APP_TRANSPORT_BACKOFF_EXPONENTIAL)) {
+	if (IS_ENABLED(CONFIG_APP_TRANSPORT_BACKOFF_TYPE_EXPONENTIAL)) {
 		backoff_time = CONFIG_APP_TRANSPORT_BACKOFF_INITIAL_SECONDS << (attempts - 1);
-	} else if (IS_ENABLED(CONFIG_APP_TRANSPORT_BACKOFF_LINEAR)) {
+	} else if (IS_ENABLED(CONFIG_APP_TRANSPORT_BACKOFF_TYPE_LINEAR)) {
 		backoff_time = CONFIG_APP_TRANSPORT_BACKOFF_INITIAL_SECONDS +
 			((attempts - 1) * CONFIG_APP_TRANSPORT_BACKOFF_LINEAR_INCREMENT_SECONDS);
 	}
@@ -350,28 +344,6 @@ static void state_connecting_attempt_entry(void *o)
 	connect_to_cloud();
 }
 
-static void state_connecting_attempt_run(void *o)
-{
-	struct state_object *state_object = o;
-
-	LOG_DBG("%s", __func__);
-
-	if (state_object->chan == &PRIV_TRANSPORT_CHAN) {
-		enum priv_transport_msg msg = *(enum priv_transport_msg *)state_object->msg_buf;
-
-		switch (msg) {
-		case CLOUD_CONN_SUCCES:
-			STATE_SET(transport_state, STATE_CONNECTED);
-			break;
-		case CLOUD_CONN_FAILED:
-			STATE_SET(transport_state, STATE_CONNECTING_BACKOFF);
-			break;
-		default:
-			break;
-		}
-	}
-}
-
 /* Handler for STATE_CONNECTING_BACKOFF */
 
 static void state_connecting_backoff_entry(void *o)

tests/module/transport/CMakeLists.txt

@@ -35,5 +35,12 @@ target_compile_definitions(app PRIVATE
 	-DCONFIG_APP_TRANSPORT_MESSAGE_QUEUE_SIZE=5
 	-DCONFIG_APP_TRANSPORT_EXEC_TIME_SECONDS_MAX=1
 	-DCONFIG_APP_TRANSPORT_WATCHDOG_TIMEOUT_SECONDS=2
-	-DCONFIG_APP_TRANSPORT_BACKOFF_SECONDS=3
+	-DCONFIG_APP_TRANSPORT_BACKOFF_TYPE_LINEAR=1
+	-DCONFIG_APP_TRANSPORT_BACKOFF_INITIAL_SECONDS=6
+	-DCONFIG_APP_TRANSPORT_BACKOFF_LINEAR_INCREMENT_SECONDS=6
+	-DCONFIG_APP_TRANSPORT_BACKOFF_MAX_SECONDS=36
+	-DCONFIG_APP_NETWORK_SAMPLE_NETWORK_QUALITY=1
+	-DCONFIG_LTE_LC_CONN_EVAL_MODULE=1
+	-DCONFIG_LTE_LC_EDRX_MODULE=1
+	-DCONFIG_LTE_LC_PSM_MODULE=1
 )

tests/module/transport/prj.conf

@@ -6,6 +6,7 @@
 
 CONFIG_UNITY=y
 CONFIG_LOG=y
+CONFIG_NATIVE_SIM_SLOWDOWN_TO_REAL_TIME=n
 
 CONFIG_ZBUS=y
 CONFIG_ZBUS_OBSERVER_NAME=y

tests/module/transport/src/transport_module_test.c

@@ -19,13 +19,41 @@ FAKE_VALUE_FUNC(int, nrf_cloud_coap_connect, const char * const);
 FAKE_VALUE_FUNC(int, nrf_cloud_coap_disconnect);
 FAKE_VALUE_FUNC(int, nrf_cloud_coap_shadow_device_status_update);
 FAKE_VALUE_FUNC(int, nrf_cloud_coap_bytes_send, uint8_t *, size_t, bool);
+FAKE_VALUE_FUNC(int, nrf_cloud_coap_sensor_send, const char *, double, int64_t, bool);
+FAKE_VALUE_FUNC(int, nrf_cloud_coap_json_message_send, const char *, bool, bool);
+
+/* Forward declarations */
+static void dummy_cb(const struct zbus_channel *chan);
+static void cloud_chan_cb(const struct zbus_channel *chan);
+static void error_cb(const struct zbus_channel *chan);
+
+/* Define unused subscribers */
+ZBUS_SUBSCRIBER_DEFINE(app, 1);
+ZBUS_SUBSCRIBER_DEFINE(battery, 1);
+ZBUS_SUBSCRIBER_DEFINE(environmental, 1);
+ZBUS_SUBSCRIBER_DEFINE(fota, 1);
+ZBUS_SUBSCRIBER_DEFINE(led, 1);
+ZBUS_SUBSCRIBER_DEFINE(location, 1);
+ZBUS_LISTENER_DEFINE(trigger, dummy_cb);
+ZBUS_LISTENER_DEFINE(cloud, cloud_chan_cb);
+ZBUS_LISTENER_DEFINE(error, error_cb);
+
+#define FAKE_DEVICE_ID		"test_device"
 
 static K_SEM_DEFINE(cloud_disconnected, 0, 1);
 static K_SEM_DEFINE(cloud_connected_ready, 0, 1);
 static K_SEM_DEFINE(cloud_connected_paused, 0, 1);
 static K_SEM_DEFINE(data_sent, 0, 1);
 static K_SEM_DEFINE(fatal_error_received, 0, 1);
 
+static int nrf_cloud_client_id_get_custom_fake(char *buf, size_t len)
+{
+	TEST_ASSERT(len >= sizeof(FAKE_DEVICE_ID));
+	memcpy(buf, FAKE_DEVICE_ID, sizeof(FAKE_DEVICE_ID));
+
+	return 0;
+}
+
 static void dummy_cb(const struct zbus_channel *chan)
 {
 	ARG_UNUSED(chan);
@@ -57,24 +85,18 @@ static void error_cb(const struct zbus_channel *chan)
 	}
 }
 
-/* Define unused subscribers */
-ZBUS_SUBSCRIBER_DEFINE(app, 1);
-ZBUS_SUBSCRIBER_DEFINE(battery, 1);
-ZBUS_SUBSCRIBER_DEFINE(environmental, 1);
-ZBUS_SUBSCRIBER_DEFINE(fota, 1);
-ZBUS_SUBSCRIBER_DEFINE(led, 1);
-ZBUS_SUBSCRIBER_DEFINE(location, 1);
-ZBUS_LISTENER_DEFINE(trigger, dummy_cb);
-ZBUS_LISTENER_DEFINE(cloud, cloud_chan_cb);
-ZBUS_LISTENER_DEFINE(error, error_cb);
-
 void setUp(void)
 {
 	const struct zbus_channel *chan;
 
 	/* Reset fakes */
 	RESET_FAKE(task_wdt_feed);
 	RESET_FAKE(task_wdt_add);
+	RESET_FAKE(nrf_cloud_client_id_get);
+	RESET_FAKE(nrf_cloud_coap_json_message_send);
+	RESET_FAKE(nrf_cloud_coap_connect);
+
+	nrf_cloud_client_id_get_fake.custom_fake = nrf_cloud_client_id_get_custom_fake;
 
 	/* Clear all channels */
 	zbus_sub_wait(&location, &chan, K_NO_WAIT);
@@ -95,6 +117,36 @@ void test_initial_transition_to_disconnected(void)
 	TEST_ASSERT_EQUAL(0, err);
 }
 
+void test_connecting_backoff(void)
+{
+	int err;
+	struct network_msg msg = { .type = NETWORK_CONNECTED, };
+	uint64_t connect_start_time, connect_duration_sec;
+
+	nrf_cloud_coap_connect_fake.return_val = -EAGAIN;
+	connect_start_time = k_uptime_get();
+
+	msg.type = NETWORK_CONNECTED;
+
+	err = zbus_chan_pub(&NETWORK_CHAN, &msg, K_SECONDS(1));
+	TEST_ASSERT_EQUAL(0, err);
+
+	/* Transport module needs CPU to run state machine */
+	k_sleep(K_MSEC(10));
+
+	err = k_sem_take(&cloud_connected_ready, K_SECONDS(60));
+	TEST_ASSERT_EQUAL(-EAGAIN, err);
+
+	connect_duration_sec = k_uptime_delta(&connect_start_time) / MSEC_PER_SEC;
+
+	/* Check that the connection attempt took at least
+	 * CONFIG_APP_TRANSPORT_BACKOFF_INITIAL_SECONDS
+	 */
+
+	TEST_ASSERT_GREATER_OR_EQUAL(CONFIG_APP_TRANSPORT_BACKOFF_INITIAL_SECONDS,
+				     connect_duration_sec);
+}
+
 void test_transition_disconnected_connected_ready(void)
 {
 	int err;
@@ -108,20 +160,23 @@ void test_transition_disconnected_connected_ready(void)
 
 void test_sending_payload(void)
 {
+	int err;
 	struct payload payload = {
-		.buffer = "test",
-		.buffer_len = sizeof(payload.buffer) - 1,
+		.buffer = "{\"test\": 1}",
+		.buffer_len = strlen(payload.buffer),
 	};
 
-	zbus_chan_pub(&PAYLOAD_CHAN, &payload, K_NO_WAIT);
+	err = zbus_chan_pub(&PAYLOAD_CHAN, &payload, K_SECONDS(1));
+	TEST_ASSERT_EQUAL(0, err);
 
 	/* Transport module needs CPU to run state machine */
-	k_sleep(K_MSEC(100));
+	k_sleep(K_MSEC(10));
 
-	TEST_ASSERT_EQUAL(1, nrf_cloud_coap_bytes_send_fake.call_count);
-	TEST_ASSERT_EQUAL(0, strncmp(nrf_cloud_coap_bytes_send_fake.arg0_val,
+	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,
 				     payload.buffer, payload.buffer_len));
-	TEST_ASSERT_EQUAL(payload.buffer_len, nrf_cloud_coap_bytes_send_fake.arg1_val);
+	TEST_ASSERT_EQUAL(false, nrf_cloud_coap_json_message_send_fake.arg1_val);
+	TEST_ASSERT_EQUAL(false, nrf_cloud_coap_json_message_send_fake.arg2_val);
 }
 
 void test_connected_ready_to_paused(void)
@@ -143,30 +198,33 @@ void test_connected_paused_to_ready_send_payload(void)
 	int err;
 	enum network_msg_type status = NETWORK_CONNECTED;
 	struct payload payload = {
-		.buffer = "Another test",
-		.buffer_len = sizeof(payload.buffer) - 1,
+		.buffer = "{\"Another\": \"test\"}",
+		.buffer_len = strlen(payload.buffer),
 	};
 
 	/* Reset call count */
 	nrf_cloud_coap_bytes_send_fake.call_count = 0;
 
-	zbus_chan_pub(&NETWORK_CHAN, &status, K_NO_WAIT);
+	err = zbus_chan_pub(&NETWORK_CHAN, &status, K_NO_WAIT);
+	TEST_ASSERT_EQUAL(0, err);
 
 	/* Transport module needs CPU to run state machine */
-	k_sleep(K_MSEC(100));
+	k_sleep(K_MSEC(10));
 
 	err = k_sem_take(&cloud_connected_ready, K_SECONDS(1));
 	TEST_ASSERT_EQUAL(0, err);
 
-	zbus_chan_pub(&PAYLOAD_CHAN, &payload, K_NO_WAIT);
+	err = zbus_chan_pub(&PAYLOAD_CHAN, &payload, K_NO_WAIT);
+	TEST_ASSERT_EQUAL(0, err);
 
 	/* Transport module needs CPU to run state machine */
-	k_sleep(K_MSEC(100));
+	k_sleep(K_MSEC(10));
 
-	TEST_ASSERT_EQUAL(1, nrf_cloud_coap_bytes_send_fake.call_count);
-	TEST_ASSERT_EQUAL(0, strncmp(nrf_cloud_coap_bytes_send_fake.arg0_val,
+	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,
 				     payload.buffer, payload.buffer_len));
-	TEST_ASSERT_EQUAL(payload.buffer_len, nrf_cloud_coap_bytes_send_fake.arg1_val);
+	TEST_ASSERT_EQUAL(false, nrf_cloud_coap_json_message_send_fake.arg1_val);
+	TEST_ASSERT_EQUAL(false, nrf_cloud_coap_json_message_send_fake.arg2_val);
 }
 
 /* This is required to be added to each test. That is because unity's