location: Refactor cloud request data handling

Introduce a unified `location_cloud_request_data` struct for improved
data transfer between modules.

Changes:
- Add `location_helper.[ch]` with `location_cloud_request_data_copy()`
        to convert location library data to module format
- Update `location.c` and `location.h` to use new struct and helper
- Refactor `cloud_location.c` to reconstruct cellular and Wi-Fi data
        from the new struct, improving modularity and clarity
- Increase thread stack sizes for cloud, location, and storage modules
- Add Kconfig options for max Wi-Fi APs and neighbor cells
- Update CMakeLists.txt to include new helper source

This fixes an issue where the cloud location request
would point to memory allocated in the location library and
not use actual data copies. This broke storage of location
because all the entries would point to the same memory location.

Signed-off-by: Simen S. Røstad <[email protected]>

Author: simensrostad

app/src/main.c

@@ -477,8 +477,6 @@ static void sampling_begin_common(struct main_state *state_object)
 
 		return;
 	}
-
-	LOG_ERR("LED pattern published");
 #endif /* CONFIG_APP_LED */
 
 	state_object->sample_start_time = k_uptime_seconds();

app/src/modules/cloud/Kconfig.cloud

@@ -82,7 +82,7 @@ config APP_CLOUD_BACKOFF_MAX_SECONDS
 
 config APP_CLOUD_THREAD_STACK_SIZE
 	int "Thread stack size"
-	default 6144
+	default 6272
 
 config APP_CLOUD_MESSAGE_QUEUE_SIZE
 	int "Message queue size"

app/src/modules/cloud/cloud_location.c

@@ -21,6 +21,111 @@ LOG_MODULE_DECLARE(cloud, CONFIG_APP_CLOUD_LOG_LEVEL);
 
 #define AGNSS_MAX_DATA_SIZE 3800
 
+#if defined(CONFIG_LOCATION_METHOD_CELLULAR)
+static int cellular_cell_data_construct(struct lte_lc_cells_info *dest,
+					struct lte_lc_ncell *neighbor_cells,
+					size_t neighbor_cells_count,
+					struct lte_lc_cell *gci_cells,
+					size_t gci_cells_count,
+					const struct location_cloud_request_data *src)
+{
+	if (!dest || !src || !neighbor_cells || !gci_cells) {
+		LOG_ERR("Invalid NULL parameter(s) provided");
+		return -EINVAL;
+	}
+
+	if (neighbor_cells_count < src->ncells_count) {
+		LOG_ERR("Insufficient neighbor_cells_count: %zu, required: %d",
+			neighbor_cells_count, src->ncells_count);
+		return -ENOMEM;
+	}
+
+	if (gci_cells_count < src->gci_cells_count) {
+		LOG_ERR("Insufficient gci_cells_count: %zu, required: %d",
+			gci_cells_count, src->gci_cells_count);
+		return -ENOMEM;
+	}
+
+	/* Copy current cell information */
+	dest->current_cell.mcc = src->current_cell.mcc;
+	dest->current_cell.mnc = src->current_cell.mnc;
+	dest->current_cell.id = src->current_cell.id;
+	dest->current_cell.tac = src->current_cell.tac;
+	dest->current_cell.timing_advance = src->current_cell.timing_advance;
+	dest->current_cell.earfcn = src->current_cell.earfcn;
+	dest->current_cell.rsrp = src->current_cell.rsrp;
+	dest->current_cell.rsrq = src->current_cell.rsrq;
+
+	dest->ncells_count = src->ncells_count;
+	dest->gci_cells_count = src->gci_cells_count;
+
+	/* Copy neighbor cells */
+	for (uint8_t i = 0; i < src->ncells_count; i++) {
+		neighbor_cells[i].earfcn = src->neighbor_cells[i].earfcn;
+		neighbor_cells[i].time_diff = src->neighbor_cells[i].time_diff;
+		neighbor_cells[i].phys_cell_id = src->neighbor_cells[i].phys_cell_id;
+		neighbor_cells[i].rsrp = src->neighbor_cells[i].rsrp;
+		neighbor_cells[i].rsrq = src->neighbor_cells[i].rsrq;
+	}
+
+	dest->neighbor_cells = neighbor_cells;
+
+	/* Copy GCI cells */
+	for (uint8_t i = 0; i < src->gci_cells_count; i++) {
+		gci_cells[i].id = src->gci_cells[i].id;
+		gci_cells[i].mcc = src->gci_cells[i].mcc;
+		gci_cells[i].mnc = src->gci_cells[i].mnc;
+		gci_cells[i].tac = src->gci_cells[i].tac;
+		gci_cells[i].timing_advance = src->gci_cells[i].timing_advance;
+		gci_cells[i].earfcn = src->gci_cells[i].earfcn;
+		gci_cells[i].rsrp = src->gci_cells[i].rsrp;
+		gci_cells[i].rsrq = src->gci_cells[i].rsrq;
+	}
+
+	dest->gci_cells = gci_cells;
+
+	return 0;
+}
+#endif	/* CONFIG_LOCATION_METHOD_CELLULAR */
+
+#if defined(CONFIG_LOCATION_METHOD_WIFI)
+static int wifi_ap_data_construct(struct wifi_scan_info *dest,
+				  struct wifi_scan_result *ap_info,
+				  size_t ap_info_count,
+				  const struct location_cloud_request_data *src)
+{
+	if (!dest || !src || !ap_info) {
+		LOG_ERR("Invalid NULL parameter(s) provided");
+		return -EINVAL;
+	}
+
+	if ((ap_info_count < src->wifi_cnt)) {
+		LOG_ERR("Insufficient ap_info_count: %zu, required: %d",
+			ap_info_count, src->wifi_cnt);
+		return -ENOMEM;
+	}
+
+	if (sizeof(ap_info->mac) < sizeof(src->wifi_aps->mac)) {
+		LOG_ERR("Insufficient mac length in wifi_scan_result");
+		return -EINVAL;
+	}
+
+	/* Copy WiFi AP data */
+	for (uint16_t i = 0; i < ap_info_count; i++) {
+		ap_info[i].rssi = src->wifi_aps[i].rssi;
+		memcpy(ap_info[i].mac,
+		       src->wifi_aps[i].mac,
+		       WIFI_MAC_ADDR_LEN);
+		ap_info[i].mac_length = src->wifi_aps[i].mac_length;
+	}
+
+	dest->ap_info = ap_info;
+	dest->cnt = ap_info_count;
+
+	return 0;
+}
+#endif /* CONFIG_LOCATION_METHOD_WIFI */
+
 static void send_request_failed(void)
 {
 	int err;
@@ -34,7 +139,7 @@ static void send_request_failed(void)
 }
 
 /* Handle cloud location requests from the location module */
-static void handle_cloud_location_request(const struct location_data_cloud *request)
+static void handle_cloud_location_request(const struct location_cloud_request_data *request)
 {
 	int err;
 	struct nrf_cloud_location_config loc_config = {
@@ -48,32 +153,48 @@ static void handle_cloud_location_request(const struct location_data_cloud *requ
 	LOG_DBG("Handling cloud location request");
 
 #if defined(CONFIG_LOCATION_METHOD_CELLULAR)
-	if (request->cell_data != NULL) {
-		/* Cast away const: nRF Cloud API limitation - struct fields are non-const
-		 * but the data is not modified by the API
-		 */
-		loc_req.cell_info = (struct lte_lc_cells_info *)request->cell_data; /* NOSONAR */
+	struct lte_lc_cells_info cell_info = { 0 };
+	struct lte_lc_ncell neighbor_cells[CONFIG_LTE_NEIGHBOR_CELLS_MAX];
+	struct lte_lc_cell gci_cells[CONFIG_LTE_NEIGHBOR_CELLS_MAX];
+
+	if ((request->current_cell.id != LTE_LC_CELL_EUTRAN_ID_INVALID) &&
+	    (request->ncells_count > 0)) {
+		err = cellular_cell_data_construct(&cell_info, neighbor_cells,
+						   ARRAY_SIZE(neighbor_cells),
+						   gci_cells,
+						   ARRAY_SIZE(gci_cells),
+						   request);
+		if (err) {
+			LOG_ERR("Failed to reconstruct cellular data, error: %d", err);
+			SEND_FATAL_ERROR();
+			return;
+		}
 
-		LOG_DBG("Cellular data present: current cell ID: %d, neighbor cells: %d, "
-			"GCI cells count: %d",
-			request->cell_data->current_cell.id,
-			request->cell_data->ncells_count,
-			request->cell_data->gci_cells_count);
+		loc_req.cell_info = &cell_info;
 	}
-#endif
+#endif /* CONFIG_LOCATION_METHOD_CELLULAR */
 
 #if defined(CONFIG_LOCATION_METHOD_WIFI)
-	if (request->wifi_data != NULL) {
-		/* Cast away const: nRF Cloud API limitation - struct fields are non-const
-		 * but the data is not modified by the API
-		 */
-		loc_req.wifi_info = (struct wifi_scan_info *)request->wifi_data; /* NOSONAR */
+	struct wifi_scan_result ap_info[CONFIG_LOCATION_METHOD_WIFI_SCANNING_RESULTS_MAX_CNT];
+	struct wifi_scan_info wifi_info = { 0 };
+
+	if (request->wifi_cnt > 0) {
+		err = wifi_ap_data_construct(&wifi_info, ap_info, ARRAY_SIZE(ap_info), request);
+		if (err) {
+			LOG_ERR("Failed to reconstruct Wi-Fi data, error: %d", err);
+			SEND_FATAL_ERROR();
+			return;
+		}
+
+		loc_req.wifi_info = &wifi_info;
+	}
+#endif /* CONFIG_LOCATION_METHOD_WIFI */
 
-		LOG_DBG("Wi-Fi data present: %d APs", request->wifi_data->cnt);
+	if (!loc_req.cell_info && !loc_req.wifi_info) {
+		LOG_ERR("No cellular or Wi-Fi data provided for location request");
+		return;
 	}
-#endif
 
-	/* Send location request to nRF Cloud */
 	err = nrf_cloud_coap_location_get(&loc_req, &result);
 	if (err == COAP_RESPONSE_CODE_NOT_FOUND) {
 		LOG_WRN("nRF Cloud CoAP location coordinates not found, error: %d", err);
@@ -95,12 +216,9 @@ static void handle_agnss_request(const struct nrf_modem_gnss_agnss_data_frame *r
 {
 	int err;
 	static char agnss_buf[AGNSS_MAX_DATA_SIZE];
-	/* Cast away const: nRF Cloud API limitation - struct fields are non-const
-	 * but the data is not modified by the API
-	 */
 	struct nrf_cloud_rest_agnss_request agnss_req = {
 		.type = NRF_CLOUD_REST_AGNSS_REQ_CUSTOM,
-		.agnss_req = (struct nrf_modem_gnss_agnss_data_frame *)request, /* NOSONAR */
+		.agnss_req = (struct nrf_modem_gnss_agnss_data_frame *)request,
 		.net_info = NULL,
 		.filtered = false,
 		.mask_angle = 0

app/src/modules/location/CMakeLists.txt

@@ -4,5 +4,8 @@
 # SPDX-License-Identifier: LicenseRef-Nordic-5-Clause
 #
 
-target_sources(app PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/location.c)
+target_sources(app PRIVATE
+	${CMAKE_CURRENT_SOURCE_DIR}/location.c
+	${CMAKE_CURRENT_SOURCE_DIR}/location_helper.c
+)
 target_include_directories(app PRIVATE .)

app/src/modules/location/Kconfig.location

@@ -12,7 +12,7 @@ if APP_LOCATION
 
 config APP_LOCATION_THREAD_STACK_SIZE
 	int "Thread stack size"
-	default 2048
+	default 4096
 
 config APP_LOCATION_WATCHDOG_TIMEOUT_SECONDS
 	int "Watchdog timeout"
@@ -34,6 +34,18 @@ config APP_LOCATION_MSG_PROCESSING_TIMEOUT_SECONDS
 	  Maximum time allowed for processing a single message in the module's state machine.
 	  The value must be smaller than CONFIG_APP_LOCATION_WATCHDOG_TIMEOUT_SECONDS.
 
+config APP_LOCATION_WIFI_APS_MAX
+	int "Maximum number of Wi-Fi APs"
+	default 10
+	help
+	  Maximum number of Wi-Fi access points to store in a location request.
+
+config APP_LOCATION_NEIGHBOR_CELLS_MAX
+	int "Maximum number of neighbor cells"
+	default 10
+	help
+	  Maximum number of neighbor cells to store in a location request.
+
 module = APP_LOCATION
 module-str = Location
 source "subsys/logging/Kconfig.template.log_config"

app/src/modules/location/location.c

@@ -18,13 +18,23 @@
 #include "app_common.h"
 #include "modem/lte_lc.h"
 #include "location.h"
+#include "location_helper.h"
 
 LOG_MODULE_REGISTER(location_module, CONFIG_APP_LOCATION_LOG_LEVEL);
 
 BUILD_ASSERT(CONFIG_APP_LOCATION_WATCHDOG_TIMEOUT_SECONDS >
 	     CONFIG_APP_LOCATION_MSG_PROCESSING_TIMEOUT_SECONDS,
 	     "Watchdog timeout must be greater than maximum message processing time");
 
+#if defined(CONFIG_LOCATION_METHOD_CELLULAR)
+BUILD_ASSERT(CONFIG_APP_LOCATION_NEIGHBOR_CELLS_MAX >= CONFIG_LTE_NEIGHBOR_CELLS_MAX);
+#endif /* CONFIG_LOCATION_METHOD_CELLULAR */
+
+#if defined(CONFIG_LOCATION_METHOD_WIFI)
+BUILD_ASSERT(CONFIG_APP_LOCATION_WIFI_APS_MAX >=
+	     CONFIG_LOCATION_METHOD_WIFI_SCANNING_RESULTS_MAX_CNT);
+#endif /* CONFIG_LOCATION_METHOD_WIFI */
+
 /* Define channels provided by this module */
 ZBUS_CHAN_DEFINE(LOCATION_CHAN,
 		 struct location_msg,
@@ -111,21 +121,29 @@ static void location_wdt_callback(int channel_id, void *user_data)
 	SEND_FATAL_ERROR_WATCHDOG_TIMEOUT();
 }
 
+#if defined(CONFIG_LOCATION_METHOD_WIFI) || defined(CONFIG_LOCATION_METHOD_CELLULAR)
 static void cloud_request_send(const struct location_data_cloud *cloud_request)
 {
 	int err;
 	struct location_msg location_msg = {
 		.type = LOCATION_CLOUD_REQUEST,
-		.cloud_request = *cloud_request
 	};
 
+	err = location_cloud_request_data_copy(&location_msg.cloud_request, cloud_request);
+	if (err) {
+		LOG_ERR("location_cloud_request_data_copy, error: %d", err);
+		SEND_FATAL_ERROR();
+		return;
+	}
+
 	err = zbus_chan_pub(&LOCATION_CHAN, &location_msg, K_SECONDS(1));
 	if (err) {
 		LOG_ERR("zbus_chan_pub, error: %d", err);
 		SEND_FATAL_ERROR();
 		return;
 	}
 }
+#endif /* defined(CONFIG_LOCATION_METHOD_WIFI) || defined(CONFIG_LOCATION_METHOD_CELLULAR) */
 
 #if defined(CONFIG_NRF_CLOUD_AGNSS)
 static void agnss_request_send(const struct nrf_modem_gnss_agnss_data_frame *agnss_request)
@@ -415,27 +433,31 @@ static void location_event_handler(const struct location_event_data *event_data)
 
 		location_print_data_details(event_data->method, &event_data->fallback.details);
 		break;
+#if defined(CONFIG_LOCATION_METHOD_WIFI) || defined(CONFIG_LOCATION_METHOD_CELLULAR)
 	case LOCATION_EVT_CLOUD_LOCATION_EXT_REQUEST:
 		LOG_DBG("Cloud location request received from location library");
 
-		/* Cancel the current location request since we're now using cloud-based location */
+		/* Cancel the current location request to avoid falling back to the next
+		 * location source. Treat the fact that we have found Wi-Fi APs and/or cellular data
+		 * as a successful location request, even if we don't know whether the
+		 * cloud is able to resolve data to a location or not.
+		 */
 		int err = location_request_cancel();
 
 		if (err) {
 			LOG_ERR("Unable to cancel location request: %d", err);
-		} else {
-			LOG_DBG("Location request cancelled successfully");
 		}
 
 		cloud_request_send(&event_data->cloud_location_request);
 		status_send(LOCATION_SEARCH_DONE);
 		break;
+#endif /* CONFIG_LOCATION_METHOD_WIFI || CONFIG_LOCATION_METHOD_CELLULAR */
 #if defined(CONFIG_NRF_CLOUD_AGNSS)
 	case LOCATION_EVT_GNSS_ASSISTANCE_REQUEST:
 		LOG_DBG("A-GNSS assistance request received from location library");
 		agnss_request_send(&event_data->agnss_request);
 		break;
-#endif
+#endif /* CONFIG_NRF_CLOUD_AGNSS */
 	case LOCATION_EVT_RESULT_UNKNOWN:
 		LOG_DBG("Location result unknown");
 		status_send(LOCATION_SEARCH_DONE);