lib: location: Handle NCELLMEAS results after cellular timeout

Location library used to throw NCELLMEAS results into trash if a
timeout occured. When timeout occurs, we send NCELLMEASSTOP.
After this modem sends NCELLMEAS notification with already
found cells.
We are changing the semantics of cellular timeout to only guard
NCELLMEAS operations. Connection and data transfer with the
cloud to resolve the location is not taken into account for cellular
timeout. Overall location_request() timeout can still interrupt cloud
data transfer.

Jira: NCSDK-21489

Signed-off-by: Tommi Rantanen <[email protected]>

doc/nrf/releases_and_maturity/releases/release-notes-changelog.rst

@@ -721,7 +721,13 @@ Modem libraries
 * :ref:`lib_location` library:
 
   * Added support for accessing nRF Cloud services using CoAP through the :ref:`lib_nrf_cloud_coap` library.
-  * Updated the neighbor cell search to use GCI search depending on :c:member:`location_cellular_config.cell_count` value.
+
+  * Updated:
+
+    * The neighbor cell search to use GCI search depending on the :c:member:`location_cellular_config.cell_count` value.
+    * The semantics of cellular and Wi-Fi timeouts to only apply to neighbor cell measurement and Wi-Fi scan, respectively.
+      Earlier, these timeouts applied also to the upcoming cloud connection to send the data to the cloud for position resolution.
+      Overall :c:func:`location_request()` timeout can still interrupt cloud data transfer.
 
 * :ref:`pdn_readme` library:
 

lib/location/method_cloud_location.c

@@ -44,34 +44,20 @@ static void method_cloud_location_positioning_work_fn(struct k_work *work)
 	const struct location_cellular_config *cell_config = work_data->cell_config;
 	struct wifi_scan_info *scan_wifi_info = NULL;
 	struct lte_lc_cells_info *scan_cellular_info = NULL;
-	int32_t used_timeout_ms;
 	int err = 0;
 #if defined(CONFIG_LOCATION_METHOD_WIFI)
 	struct k_sem wifi_scan_ready;
 
 	k_sem_init(&wifi_scan_ready, 0, 1);
-#endif
-
-	if (wifi_config != NULL && cell_config != NULL) {
-		used_timeout_ms = MIN(cell_config->timeout, wifi_config->timeout);
-	} else if (cell_config != NULL) {
-		used_timeout_ms = cell_config->timeout;
-	} else {
-		__ASSERT_NO_MSG(wifi_config != NULL);
-		used_timeout_ms = wifi_config->timeout;
-	}
 
-	location_core_timer_start(used_timeout_ms);
-
-#if defined(CONFIG_LOCATION_METHOD_WIFI)
 	if (wifi_config != NULL) {
-		scan_wifi_execute(&wifi_scan_ready);
+		scan_wifi_execute(wifi_config->timeout, &wifi_scan_ready);
 	}
 #endif
 
 #if defined(CONFIG_LOCATION_METHOD_CELLULAR)
 	if (cell_config != NULL) {
-		scan_cellular_execute(cell_config->cell_count);
+		scan_cellular_execute(cell_config->timeout, cell_config->cell_count);
 		scan_cellular_info = scan_cellular_results_get();
 	}
 #endif
@@ -87,15 +73,15 @@ static void method_cloud_location_positioning_work_fn(struct k_work *work)
 		goto end;
 	}
 
-	location_core_timer_stop();
-
 	if (scan_cellular_info == NULL && scan_wifi_info == NULL) {
 		LOG_WRN("No cellular neighbor cells or Wi-Fi access points found");
 		err = -ENODATA;
 		goto end;
 	}
 
 #if defined(CONFIG_LOCATION_SERVICE_EXTERNAL)
+	ARG_UNUSED(wifi_config);
+
 	struct location_data_cloud request = {
 #if defined(CONFIG_LOCATION_METHOD_CELLULAR)
 		.cell_data = scan_cellular_info,
@@ -149,21 +135,18 @@ static void method_cloud_location_positioning_work_fn(struct k_work *work)
 		location_result.latitude = location.latitude;
 		location_result.longitude = location.longitude;
 		location_result.accuracy = location.accuracy;
-		if (running) {
-			running = false;
-			location_core_event_cb(&location_result);
-		}
+		location_core_event_cb(&location_result);
 	}
+
 #endif /* defined(CONFIG_LOCATION_SERVICE_EXTERNAL) */
 
 end:
 	if (err == -ETIMEDOUT) {
 		location_core_event_cb_timeout();
-		running = false;
 	} else if (err) {
 		location_core_event_cb_error();
-		running = false;
 	}
+	running = false;
 }
 
 int method_cloud_location_cancel(void)

lib/location/method_gnss.c

@@ -292,8 +292,10 @@ static void method_gnss_nrf_cloud_agps_request(void)
 	struct lte_lc_cells_info net_info = {0};
 	struct lte_lc_cells_info *scan_results;
 
-	/* Get network info for the A-GPS location request. */
-	scan_cellular_execute(0);
+	/* Get network info for the A-GPS location request.
+	 * Timeout value is just some number that should be big enough.
+	 */
+	scan_cellular_execute(5000, 0);
 	scan_results = scan_cellular_results_get();
 	if (scan_results == NULL) {
 		LOG_WRN("Requesting A-GPS data without location assistance");

lib/location/scan_cellular.c

@@ -23,12 +23,28 @@ static struct lte_lc_cells_info scan_cellular_info = {
 	.gci_cells = gci_cells
 };
 
-static bool running;
+static volatile bool running;
+static volatile bool timeout_occurred;
 /* Indicates when individual ncellmeas operation is completed. This is internal to this file. */
 static struct k_sem scan_cellular_sem_ncellmeas_evt;
 /* Requested number of cells to be searched. */
 static int8_t scan_cellular_cell_count;
 
+/** Handler for timeout. */
+static void scan_cellular_timeout_work_fn(struct k_work *work);
+
+/** Work item for timeout handler. */
+K_WORK_DELAYABLE_DEFINE(scan_cellular_timeout_work, scan_cellular_timeout_work_fn);
+
+/**
+ * Handler for backup timeout, which ensures we won't be waiting for LTE_LC_EVT_NEIGHBOR_CELL_MEAS
+ * event forever after lte_lc_neighbor_cell_measurement_cancel() in case it would never be sent.
+ */
+static void scan_cellular_timeout_backup_work_fn(struct k_work *work);
+
+/** Work item for backup timeout handler. */
+K_WORK_DELAYABLE_DEFINE(scan_cellular_timeout_backup_work, scan_cellular_timeout_backup_work_fn);
+
 struct lte_lc_cells_info *scan_cellular_results_get(void)
 {
 	if (scan_cellular_info.current_cell.id == LTE_LC_CELL_EUTRAN_ID_INVALID) {
@@ -41,18 +57,28 @@ struct lte_lc_cells_info *scan_cellular_results_get(void)
 
 void scan_cellular_lte_ind_handler(const struct lte_lc_evt *const evt)
 {
+	if (!running) {
+		return;
+	}
+
 	switch (evt->type) {
 	case LTE_LC_EVT_NEIGHBOR_CELL_MEAS: {
-		if (!running) {
-			return;
-		}
-		LOG_DBG("Cell measurements results received: ncells_count=%d, gci_cells_count=%d",
-			evt->cells_info.ncells_count, evt->cells_info.gci_cells_count);
+		k_work_cancel_delayable(&scan_cellular_timeout_backup_work);
+
+		LOG_DBG("Cell measurements results received: "
+			"ncells_count=%d, gci_cells_count=%d, current_cell.id=0x%08X",
+			evt->cells_info.ncells_count,
+			evt->cells_info.gci_cells_count,
+			evt->cells_info.current_cell.id);
 
-		/* Copy current cell information */
-		memcpy(&scan_cellular_info.current_cell,
-		       &evt->cells_info.current_cell,
-		       sizeof(struct lte_lc_cell));
+		if (evt->cells_info.current_cell.id != LTE_LC_CELL_EUTRAN_ID_INVALID) {
+			/* Copy current cell information. We are seeing this is not set for GCI
+			 * search sometimes but we have it for the previous normal neighbor search.
+			 */
+			memcpy(&scan_cellular_info.current_cell,
+			&evt->cells_info.current_cell,
+			sizeof(struct lte_lc_cell));
+		}
 
 		/* Copy neighbor cell information if present */
 		if (evt->cells_info.ncells_count > 0 && evt->cells_info.neighbor_cells) {
@@ -74,7 +100,6 @@ void scan_cellular_lte_ind_handler(const struct lte_lc_evt *const evt)
 			scan_cellular_info.gci_cells_count = evt->cells_info.gci_cells_count;
 		} else {
 			LOG_DBG("No surrounding cell information from modem");
-
 		}
 
 		k_sem_give(&scan_cellular_sem_ncellmeas_evt);
@@ -84,7 +109,7 @@ void scan_cellular_lte_ind_handler(const struct lte_lc_evt *const evt)
 	}
 }
 
-void scan_cellular_execute(uint8_t cell_count)
+void scan_cellular_execute(int32_t timeout, uint8_t cell_count)
 {
 	struct lte_lc_ncellmeas_params ncellmeas_params = {
 		.search_type = LTE_LC_NEIGHBOR_SEARCH_TYPE_EXTENDED_LIGHT,
@@ -93,13 +118,19 @@ void scan_cellular_execute(uint8_t cell_count)
 	int err;
 
 	running = true;
+	timeout_occurred = false;
 	scan_cellular_cell_count = cell_count;
 	scan_cellular_info.current_cell.id = LTE_LC_CELL_EUTRAN_ID_INVALID;
 	scan_cellular_info.ncells_count = 0;
 	scan_cellular_info.gci_cells_count = 0;
 
 	LOG_DBG("Triggering cell measurements");
 
+	if (timeout != SYS_FOREVER_MS && timeout > 0) {
+		LOG_DBG("Starting cellular timer with timeout=%d", timeout);
+		k_work_schedule(&scan_cellular_timeout_work, K_MSEC(timeout));
+	}
+
 	err = lte_lc_neighbor_cell_measurement(&ncellmeas_params);
 	if (err) {
 		LOG_ERR("Failed to initiate neighbor cell measurements: %d", err);
@@ -112,6 +143,11 @@ void scan_cellular_execute(uint8_t cell_count)
 		goto end;
 	}
 
+	if (timeout_occurred) {
+		LOG_DBG("Timeout occurred after 1st neighbor measurement");
+		goto end;
+	}
+
 	/* Calculate the number of GCI cells to be requested.
 	 * We should subtract 1 for current cell as ncell_count won't include it.
 	 * GCI count requested from modem includes also current cell so we should add 1.
@@ -139,6 +175,7 @@ void scan_cellular_execute(uint8_t cell_count)
 	}
 
 end:
+	k_work_cancel_delayable(&scan_cellular_timeout_work);
 	running = false;
 }
 
@@ -165,3 +202,22 @@ int scan_cellular_init(void)
 
 	return 0;
 }
+
+static void scan_cellular_timeout_work_fn(struct k_work *work)
+{
+	ARG_UNUSED(work);
+
+	LOG_INF("Cellular method specific timeout expired");
+
+	(void)lte_lc_neighbor_cell_measurement_cancel();
+	timeout_occurred = true;
+
+	k_work_schedule(&scan_cellular_timeout_backup_work, K_MSEC(2000));
+}
+
+static void scan_cellular_timeout_backup_work_fn(struct k_work *work)
+{
+	ARG_UNUSED(work);
+
+	k_sem_reset(&scan_cellular_sem_ncellmeas_evt);
+}

lib/location/scan_cellular.h

@@ -10,7 +10,7 @@
 #include <modem/lte_lc.h>
 
 int scan_cellular_init(void);
-void scan_cellular_execute(uint8_t cell_count);
+void scan_cellular_execute(int32_t timeout, uint8_t cell_count);
 struct lte_lc_cells_info *scan_cellular_results_get(void);
 int scan_cellular_cancel(void);
 

lib/location/scan_wifi.c

@@ -28,6 +28,12 @@ static struct wifi_scan_info scan_wifi_info = {
 };
 static struct k_sem *scan_wifi_ready;
 
+/** Handler for timeout. */
+static void scan_wifi_timeout_work_fn(struct k_work *work);
+
+/** Work item for timeout handler. */
+K_WORK_DELAYABLE_DEFINE(scan_wifi_timeout_work, scan_wifi_timeout_work_fn);
+
 struct wifi_scan_info *scan_wifi_results_get(void)
 {
 	if (scan_wifi_info.cnt  <= 1) {
@@ -46,7 +52,7 @@ struct wifi_scan_info *scan_wifi_results_get(void)
 	return &scan_wifi_info;
 }
 
-void scan_wifi_execute(struct k_sem *wifi_scan_ready)
+void scan_wifi_execute(int32_t timeout, struct k_sem *wifi_scan_ready)
 {
 	int ret;
 
@@ -64,6 +70,11 @@ void scan_wifi_execute(struct k_sem *wifi_scan_ready)
 		k_sem_give(wifi_scan_ready);
 		wifi_scan_ready = NULL;
 	}
+
+	if (timeout != SYS_FOREVER_MS && timeout > 0) {
+		LOG_DBG("Starting Wi-Fi timer with timeout=%d", timeout);
+		k_work_schedule(&scan_wifi_timeout_work, K_MSEC(timeout));
+	}
 }
 
 static void scan_wifi_result_handle(struct net_mgmt_event_callback *cb)
@@ -103,6 +114,7 @@ static void scan_wifi_done_handle(struct net_mgmt_event_callback *cb)
 
 	k_sem_give(scan_wifi_ready);
 	scan_wifi_ready = NULL;
+	k_work_cancel_delayable(&scan_wifi_timeout_work);
 }
 
 void scan_wifi_net_mgmt_event_handler(
@@ -162,3 +174,12 @@ int scan_wifi_init(void)
 
 	return 0;
 }
+
+static void scan_wifi_timeout_work_fn(struct k_work *work)
+{
+	ARG_UNUSED(work);
+
+	LOG_INF("Wi-Fi method specific timeout expired");
+
+	scan_wifi_cancel();
+}

lib/location/scan_wifi.h

@@ -10,7 +10,7 @@
 #include <net/wifi_location_common.h>
 
 int scan_wifi_init(void);
-void scan_wifi_execute(struct k_sem *wifi_scan_ready);
+void scan_wifi_execute(int32_t timeout, struct k_sem *wifi_scan_ready);
 struct wifi_scan_info *scan_wifi_results_get(void);
 int scan_wifi_cancel(void);
 

tests/lib/location/src/location_test.c

@@ -673,12 +673,12 @@ void test_location_request_mode_all_cellular_gnss(void)
 	method_gnss_event_handler(NRF_MODEM_GNSS_EVT_PVT);
 }
 
-/* Test location request timeout:
- * - Use cellular and GNSS positioning and timeout occurs for both
+/* Test location request error/timeout with :
+ * - Use cellular and GNSS positioning and error and timeout occurs, respectively
  * - Use LOCATION_REQ_MODE_ALL so we can check timeout event ID for both methods
  * - Tests also case when all fallbacks are failing
  */
-void test_location_request_timeout_cellular_gnss_mode_all(void)
+void test_location_request_mode_all_cellular_error_gnss_timeout(void)
 {
 	int err;
 
@@ -687,24 +687,22 @@ void test_location_request_timeout_cellular_gnss_mode_all(void)
 
 	location_config_defaults_set(&config, 2, methods);
 	config.mode = LOCATION_REQ_MODE_ALL;
-	config.methods[0].cellular.timeout = 100;
 	config.methods[0].cellular.cell_count = 2;
 	config.methods[1].gnss.timeout = 100;
 
-	test_location_event_data.id = LOCATION_EVT_TIMEOUT;
+	test_location_event_data.id = LOCATION_EVT_ERROR;
 
 	location_callback_called_expected = true;
 
 	/***** First cellular positioning *****/
-	__cmock_nrf_modem_at_printf_ExpectAndReturn("AT%%NCELLMEAS=1", 0);
+	__cmock_nrf_modem_at_printf_ExpectAndReturn("AT%%NCELLMEAS=1", -EFAULT);
 
 	err = location_request(&config);
 	TEST_ASSERT_EQUAL(0, err);
 
 	/* Wait for location_event_handler call for 3 seconds.
 	 * If it doesn't happen, next assert will fail the test.
 	 */
-	__cmock_nrf_modem_at_printf_ExpectAndReturn("AT%%NCELLMEASSTOP", 0);
 	__cmock_nrf_modem_gnss_event_handler_set_ExpectAndReturn(&method_gnss_event_handler, 0);
 	k_sem_take(&event_handler_called_sem, K_SECONDS(3));
 	TEST_ASSERT_EQUAL(location_callback_called_expected, location_callback_called_occurred);