From 8f99fa2ecf08ba764d54f2d1722bf33999f685cb Mon Sep 17 00:00:00 2001 From: Tommi Rantanen Date: Thu, 22 Jun 2023 11:25:18 +0300 Subject: [PATCH] lib: lte_link_control: Protect ncellmeas with semaphore We need to protect execution of AT%NCELLMEAS with a semaphore. Otherwise a client that calls new lte_lc_neighbor_cell_measurement() right after receiving LTE_LC_EVT_NEIGHBOR_CELL_MEAS event, may get -EINPROGRESS error code even though AT%NCELLMEAS notification had been received. Signed-off-by: Tommi Rantanen --- lib/location/scan_cellular.c | 9 +++------ lib/lte_link_control/lte_lc.c | 15 +++++++++------ tests/lib/location/src/location_test.c | 8 ++++---- tests/lib/lte_lc_api/src/lte_lc_api_test.c | 16 ++++++++++++++++ 4 files changed, 32 insertions(+), 16 deletions(-) diff --git a/lib/location/scan_cellular.c b/lib/location/scan_cellular.c index e33c5fb4c8b..c4e529320c5 100644 --- a/lib/location/scan_cellular.c +++ b/lib/location/scan_cellular.c @@ -139,16 +139,13 @@ int scan_cellular_start(uint8_t cell_count) */ scan_cellular_cell_count = scan_cellular_cell_count - scan_cellular_info.ncells_count; - if (scan_cellular_cell_count > 0) { + LOG_DBG("scan_cellular_start: scan_cellular_cell_count=%d", scan_cellular_cell_count); + + if (scan_cellular_cell_count > 1) { ncellmeas_params.search_type = LTE_LC_NEIGHBOR_SEARCH_TYPE_GCI_EXTENDED_LIGHT; ncellmeas_params.gci_count = scan_cellular_cell_count; - /* GCI count given to modem cannot be one */ - if (ncellmeas_params.gci_count == 1) { - ncellmeas_params.gci_count++; - } - err = lte_lc_neighbor_cell_measurement(&ncellmeas_params); if (err) { LOG_WRN("Failed to initiate GCI cell measurements: %d", err); diff --git a/lib/lte_link_control/lte_lc.c b/lib/lte_link_control/lte_lc.c index c01d4893082..4646d3e7fac 100644 --- a/lib/lte_link_control/lte_lc.c +++ b/lib/lte_link_control/lte_lc.c @@ -67,7 +67,8 @@ static char rai_param[2] = CONFIG_LTE_RAI_REQ_VALUE; /* Requested NCELLMEAS params */ static struct lte_lc_ncellmeas_params ncellmeas_params; -static bool ncellmeas_ongoing; +/* Sempahore value 1 means ncellmeas is not ongoing, and 0 means it's ongoing. */ +K_SEM_DEFINE(ncellmeas_idle_sem, 1, 1); static const enum lte_lc_system_mode sys_mode_preferred = SYS_MODE_PREFERRED; @@ -394,7 +395,7 @@ static void at_handler_ncellmeas(const char *response) __ASSERT_NO_MSG(response != NULL); - if (event_handler_list_is_empty() || !ncellmeas_ongoing) { + if (event_handler_list_is_empty() || k_sem_count_get(&ncellmeas_idle_sem) > 0) { /* No need to parse the response if there is no handler * to receive the parsed data or * if a measurement is not going/started by using @@ -445,7 +446,7 @@ static void at_handler_ncellmeas(const char *response) k_free(neighbor_cells); } exit: - ncellmeas_ongoing = false; + k_sem_give(&ncellmeas_idle_sem); } static void at_handler_xmodemsleep(const char *response) @@ -1461,7 +1462,8 @@ int lte_lc_neighbor_cell_measurement(struct lte_lc_ncellmeas_params *params) LTE_LC_NEIGHBOR_SEARCH_TYPE_GCI_EXTENDED_COMPLETE), "Invalid argument, API does not accept enum values directly anymore"); - if (ncellmeas_ongoing) { + if (k_sem_take(&ncellmeas_idle_sem, K_SECONDS(1)) != 0) { + LOG_WRN("Neighbor cell measurement already in progress"); return -EINPROGRESS; } @@ -1493,9 +1495,9 @@ int lte_lc_neighbor_cell_measurement(struct lte_lc_ncellmeas_params *params) if (err) { err = -EFAULT; + k_sem_give(&ncellmeas_idle_sem); } else { ncellmeas_params = used_params; - ncellmeas_ongoing = true; } return err; @@ -1508,7 +1510,8 @@ int lte_lc_neighbor_cell_measurement_cancel(void) if (err) { err = -EFAULT; } - ncellmeas_ongoing = false; + + k_sem_give(&ncellmeas_idle_sem); return err; } diff --git a/tests/lib/location/src/location_test.c b/tests/lib/location/src/location_test.c index 4b6674813e0..69b1cbe507f 100644 --- a/tests/lib/location/src/location_test.c +++ b/tests/lib/location/src/location_test.c @@ -49,8 +49,8 @@ static const char ncellmeas_resp[] = "150344527,2300,8,60,29,0,2400,11,55,26,184\r\n"; static const char ncellmeas_gci_resp[] = - "%NCELLMEAS:0,\"00023906\",\"24405\",\"5A00\",64,1830518,6400,116,53,16,1840265,1,0," - "\"001E3904\",\"24405\",\"5A00\",65535,0,6400,42,48,7,1840265,0,0\r\n"; + "%NCELLMEAS:0,\"00011B07\",\"26295\",\"00B7\",10512,9034,2300,7,63,31,150344527,1,0," + "\"00011B08\",\"26295\",\"00B7\",65535,0,2300,9,62,30,150345527,0,0\r\n"; char http_resp[512]; @@ -545,14 +545,14 @@ void test_location_request_default(void) * Otherwise, lte_lc would ignore NCELLMEAS notification because no NCELLMEAS on going * from lte_lc point of view. */ - k_sleep(K_MSEC(1000)); + k_sleep(K_MSEC(100)); /* Trigger NCELLMEAS response which further triggers the rest of the location calculation */ at_monitor_dispatch(ncellmeas_resp); cellular_rest_req_resp_handle(); - k_sleep(K_MSEC(1000)); + k_sleep(K_MSEC(100)); at_monitor_dispatch(ncellmeas_gci_resp); } diff --git a/tests/lib/lte_lc_api/src/lte_lc_api_test.c b/tests/lib/lte_lc_api/src/lte_lc_api_test.c index 1064ee48dd3..6d0dfef024b 100644 --- a/tests/lib/lte_lc_api/src/lte_lc_api_test.c +++ b/tests/lib/lte_lc_api/src/lte_lc_api_test.c @@ -975,6 +975,22 @@ void test_lte_lc_cereg(void) at_monitor_dispatch(at_notif); } +/* Test failing neighbor cell measurement first to see that the semaphore is given properly */ +void test_lte_lc_neighbor_cell_measurement_fail(void) +{ + int ret; + + struct lte_lc_ncellmeas_params params = { + .search_type = LTE_LC_NEIGHBOR_SEARCH_TYPE_EXTENDED_COMPLETE, + .gci_count = 0, + }; + + __cmock_nrf_modem_at_printf_ExpectAndReturn("AT%%NCELLMEAS=2", -NRF_ENOMEM); + + ret = lte_lc_neighbor_cell_measurement(¶ms); + TEST_ASSERT_EQUAL(-EFAULT, ret); +} + void test_lte_lc_neighbor_cell_measurement_neighbors(void) { int ret;