feat(memfault): add location metrics support

- Add location metrics - Remove asset tracker app location metrics Signed-off-by: Gillian Minnehan <[email protected]>
memfault · Sep 12, 2024 · f53e20e · f53e20e
1 parent 6b6426a
commit f53e20e
Show file tree

Hide file tree

Showing 12 changed files with 306 additions and 159 deletions.
diff --git a/applications/asset_tracker_v2/configuration/memfault/memfault_metrics_heartbeat_config.def b/applications/asset_tracker_v2/configuration/memfault/memfault_metrics_heartbeat_config.def
@@ -1,7 +1,3 @@
 /* Application-specific metrics.
  * Please refer to https://docs.memfault.com/docs/embedded/metrics-api for more details.
  */
-
-MEMFAULT_METRICS_KEY_DEFINE(gnss_time_to_fix_ms, kMemfaultMetricType_Unsigned)
-MEMFAULT_METRICS_KEY_DEFINE(gnss_satellites_tracked_count, kMemfaultMetricType_Unsigned)
-MEMFAULT_METRICS_KEY_DEFINE(location_timeout_search_time_ms, kMemfaultMetricType_Unsigned)
diff --git a/applications/asset_tracker_v2/doc/debug_module.rst b/applications/asset_tracker_v2/doc/debug_module.rst
@@ -22,13 +22,7 @@ This section documents the various features implemented by the module.
 Memfault
 ========
 
-The debug module uses `Memfault SDK`_ to track |NCS| specific metrics such as LTE and stack metrics.
-In addition, the following types of custom Memfault metrics are defined and tracked when compiling in the debug module:
-
- * ``gnss_time_to_fix_ms`` - Time duration between the start of a GNSS search and obtaining a fix.
- * ``gnss_satellites_tracked_count`` - Number of satellites tracked during a GNSS search window.
- * ``location_timeout_search_time_ms`` - Time duration between the start of a location search and a search timeout.
-
+The debug module uses `Memfault SDK`_ to track |NCS| specific metrics such as LTE, location, Bluetooth and stack metrics.
 The debug module also implements `Memfault SDK`_ software watchdog, which is designed to trigger an assert before an actual watchdog timeout.
 This enables the application to be able to collect coredump data before a reboot occurs.
 

diff --git a/applications/asset_tracker_v2/overlay-memfault.conf b/applications/asset_tracker_v2/overlay-memfault.conf
@@ -9,6 +9,7 @@ CONFIG_MEMFAULT_NCS_PROJECT_KEY=""
 CONFIG_MEMFAULT_NCS_DEVICE_ID_IMEI=y
 CONFIG_MEMFAULT_NCS_LTE_METRICS=y
 CONFIG_MEMFAULT_NCS_STACK_METRICS=y
+CONFIG_MEMFAULT_NCS_LOCATION_METRICS=y
 CONFIG_MEMFAULT_LOGGING_ENABLE=y
 
 CONFIG_MEMFAULT_ROOT_CERT_STORAGE_NRF9160_MODEM=y

diff --git a/applications/asset_tracker_v2/src/modules/debug_module.c b/applications/asset_tracker_v2/src/modules/debug_module.c
@@ -25,7 +25,6 @@
 #include "events/data_module_event.h"
 #include "events/sensor_module_event.h"
 #include "events/util_module_event.h"
-#include "events/location_module_event.h"
 #include "events/modem_module_event.h"
 #include "events/ui_module_event.h"
 #include "events/debug_module_event.h"
@@ -41,7 +40,6 @@ struct debug_msg_data {
 		struct sensor_module_event sensor;
 		struct data_module_event data;
 		struct app_module_event app;
-		struct location_module_event location;
 		struct modem_module_event modem;
 	} module;
 };
@@ -143,15 +141,6 @@ static bool app_event_handler(const struct app_event_header *aeh)
 		message_handler(&debug_msg);
 	}
 
-	if (is_location_module_event(aeh)) {
-		struct location_module_event *event = cast_location_module_event(aeh);
-		struct debug_msg_data debug_msg = {
-			.module.location = *event
-		};
-
-		message_handler(&debug_msg);
-	}
-
 	if (is_sensor_module_event(aeh)) {
 		struct sensor_module_event *event =
 				cast_sensor_module_event(aeh);
@@ -257,38 +246,6 @@ static void send_memfault_data(void)
 	}
 }
 
-static void add_location_metrics(uint8_t satellites, uint32_t search_time,
-				 enum location_module_event_type event)
-{
-	int err;
-
-	switch (event) {
-	case LOCATION_MODULE_EVT_GNSS_DATA_READY:
-		err = MEMFAULT_METRIC_SET_UNSIGNED(gnss_time_to_fix_ms, search_time);
-		if (err) {
-			LOG_ERR("Failed updating gnss_time_to_fix_ms metric, error: %d", err);
-		}
-		break;
-	case LOCATION_MODULE_EVT_TIMEOUT:
-		err = MEMFAULT_METRIC_SET_UNSIGNED(location_timeout_search_time_ms, search_time);
-		if (err) {
-			LOG_ERR("Failed updating location_timeout_search_time_ms metric, error: %d",
-				err);
-		}
-		break;
-	default:
-		LOG_ERR("Unknown location module event.");
-		return;
-	}
-
-	err = MEMFAULT_METRIC_SET_UNSIGNED(gnss_satellites_tracked_count, satellites);
-	if (err) {
-		LOG_ERR("Failed updating gnss_satellites_tracked_count metric, error: %d", err);
-	}
-
-	memfault_metrics_heartbeat_debug_trigger();
-}
-
 static void memfault_handle_event(struct debug_msg_data *msg)
 {
 	if (IS_EVENT(msg, app, APP_EVT_START)) {
@@ -349,14 +306,6 @@ static void memfault_handle_event(struct debug_msg_data *msg)
 		send_memfault_data();
 		return;
 	}
-
-	if ((IS_EVENT(msg, location, LOCATION_MODULE_EVT_TIMEOUT)) ||
-	    (IS_EVENT(msg, location, LOCATION_MODULE_EVT_GNSS_DATA_READY))) {
-		add_location_metrics(msg->module.location.data.location.satellites_tracked,
-				msg->module.location.data.location.search_time,
-				msg->module.location.type);
-		return;
-	}
 }
 #endif /* defined(CONFIG_MEMFAULT) */
 
@@ -388,7 +337,6 @@ APP_EVENT_LISTENER(MODULE, app_event_handler);
 APP_EVENT_SUBSCRIBE_EARLY(MODULE, app_module_event);
 APP_EVENT_SUBSCRIBE_EARLY(MODULE, modem_module_event);
 APP_EVENT_SUBSCRIBE_EARLY(MODULE, cloud_module_event);
-APP_EVENT_SUBSCRIBE_EARLY(MODULE, location_module_event);
 APP_EVENT_SUBSCRIBE_EARLY(MODULE, ui_module_event);
 APP_EVENT_SUBSCRIBE_EARLY(MODULE, sensor_module_event);
 APP_EVENT_SUBSCRIBE_EARLY(MODULE, data_module_event);

diff --git a/applications/asset_tracker_v2/tests/debug_module/src/debug_module_test.c b/applications/asset_tracker_v2/tests/debug_module/src/debug_module_test.c
@@ -18,7 +18,6 @@
 #include "cmock_watchdog_app.h"
 
 #include "app_module_event.h"
-#include "location_module_event.h"
 #include "debug_module_event.h"
 #include "data_module_event.h"
 
@@ -29,7 +28,6 @@ extern struct event_listener __event_listener_debug_module;
  */
 static struct app_module_event app_module_event_memory;
 static struct data_module_event data_module_event_memory;
-static struct location_module_event location_module_event_memory;
 static struct debug_module_event debug_module_event_memory;
 
 #define DEBUG_MODULE_EVT_HANDLER(aeh) __event_listener_debug_module.notification(aeh)
@@ -38,7 +36,6 @@ static struct debug_module_event debug_module_event_memory;
  * depend on these to exist. But since we are unit testing, we dont need
  * these subscriptions and hence these structs can remain uninitialized.
  */
-struct event_type __event_type_location_module_event;
 struct event_type __event_type_debug_module_event;
 struct event_type __event_type_app_module_event;
 struct event_type __event_type_data_module_event;
@@ -108,62 +105,6 @@ void setup_debug_module_in_init_state(void)
 	app_event_manager_free(app_module_event);
 }
 
-/* Test whether the correct Memfault metrics are set upon a GNSS fix. */
-void test_memfault_trigger_metric_sampling_on_gnss_fix(void)
-{
-	setup_debug_module_in_init_state();
-
-	__cmock_memfault_metrics_heartbeat_set_unsigned_ExpectAndReturn(
-		MEMFAULT_METRICS_KEY(gnss_time_to_fix_ms), 60000, 0);
-	__cmock_memfault_metrics_heartbeat_set_unsigned_ExpectAndReturn(
-						MEMFAULT_METRICS_KEY(gnss_satellites_tracked_count),
-						4,
-						0);
-	__cmock_memfault_metrics_heartbeat_debug_trigger_Expect();
-
-	__cmock_app_event_manager_alloc_ExpectAnyArgsAndReturn(&location_module_event_memory);
-	__cmock_app_event_manager_free_ExpectAnyArgs();
-	struct location_module_event *location_module_event = new_location_module_event();
-
-	location_module_event->type = LOCATION_MODULE_EVT_GNSS_DATA_READY;
-	location_module_event->data.location.satellites_tracked = 4;
-	location_module_event->data.location.search_time = 60000;
-
-	TEST_ASSERT_EQUAL(0, DEBUG_MODULE_EVT_HANDLER(
-		(struct app_event_header *)location_module_event));
-	app_event_manager_free(location_module_event);
-}
-
-/* Test whether the correct Memfault metrics are set upon a GNSS timeout. */
-void test_memfault_trigger_metric_sampling_on_location_timeout(void)
-{
-	resetTest();
-	setup_debug_module_in_init_state();
-
-	/* Update this function to expect the search time and number of satellites. */
-	__cmock_memfault_metrics_heartbeat_set_unsigned_ExpectAndReturn(
-		MEMFAULT_METRICS_KEY(location_timeout_search_time_ms),
-		30000,
-		0);
-	__cmock_memfault_metrics_heartbeat_set_unsigned_ExpectAndReturn(
-		MEMFAULT_METRICS_KEY(gnss_satellites_tracked_count),
-		2,
-		0);
-	__cmock_memfault_metrics_heartbeat_debug_trigger_Ignore();
-
-	__cmock_app_event_manager_alloc_ExpectAnyArgsAndReturn(&location_module_event_memory);
-	__cmock_app_event_manager_free_ExpectAnyArgs();
-	struct location_module_event *location_module_event = new_location_module_event();
-
-	location_module_event->type = LOCATION_MODULE_EVT_TIMEOUT;
-	location_module_event->data.location.satellites_tracked = 2;
-	location_module_event->data.location.search_time = 30000;
-
-	TEST_ASSERT_EQUAL(0, DEBUG_MODULE_EVT_HANDLER(
-		(struct app_event_header *)location_module_event));
-	app_event_manager_free(location_module_event);
-}
-
 /* Test that the debug module is able to submit Memfault data externally through events
  * of type DEBUG_EVT_MEMFAULT_DATA_READY carrying chunks of data.
  */
@@ -198,42 +139,6 @@ void test_memfault_trigger_data_send(void)
 	k_sleep(K_SECONDS(1));
 }
 
-/* Test that no Memfault SDK specific APIs are called on GNSS module events
- * that should not be handled.
- */
-void test_memfault_unhandled_event(void)
-{
-	resetTest();
-	setup_debug_module_in_init_state();
-
-	/* Expect no memfault APIs to be called on LOCATION_MODULE_EVT_ACTIVE */
-
-	__cmock_app_event_manager_alloc_ExpectAnyArgsAndReturn(&location_module_event_memory);
-	__cmock_app_event_manager_free_ExpectAnyArgs();
-	struct location_module_event *location_module_event = new_location_module_event();
-
-	location_module_event->type = LOCATION_MODULE_EVT_ACTIVE;
-	TEST_ASSERT_EQUAL(0, DEBUG_MODULE_EVT_HANDLER(
-		(struct app_event_header *)location_module_event));
-
-	location_module_event->type = LOCATION_MODULE_EVT_INACTIVE;
-	TEST_ASSERT_EQUAL(0, DEBUG_MODULE_EVT_HANDLER(
-		(struct app_event_header *)location_module_event));
-
-	location_module_event->type = LOCATION_MODULE_EVT_SHUTDOWN_READY;
-	TEST_ASSERT_EQUAL(0, DEBUG_MODULE_EVT_HANDLER(
-		(struct app_event_header *)location_module_event));
-
-	location_module_event->type = LOCATION_MODULE_EVT_AGNSS_NEEDED;
-	TEST_ASSERT_EQUAL(0, DEBUG_MODULE_EVT_HANDLER(
-		(struct app_event_header *)location_module_event));
-
-	location_module_event->type = LOCATION_MODULE_EVT_ERROR_CODE;
-	TEST_ASSERT_EQUAL(0, DEBUG_MODULE_EVT_HANDLER(
-		(struct app_event_header *)location_module_event));
-	app_event_manager_free(location_module_event);
-}
-
 /* Test whether the correct Memfault software watchdog APIs are called on callbacks from the
  * application watchdog library.
  */

diff --git a/doc/nrf/libraries/debug/memfault_ncs.rst b/doc/nrf/libraries/debug/memfault_ncs.rst
@@ -97,6 +97,7 @@ The Kconfig options for Memfault that are defined in |NCS| provide some addition
 * :kconfig:option:`CONFIG_MEMFAULT_NCS_PROVISION_CERTIFICATES`
 * :kconfig:option:`CONFIG_MEMFAULT_NCS_INTERNAL_FLASH_BACKED_COREDUMP`
 * :kconfig:option:`CONFIG_MEMFAULT_NCS_LTE_METRICS`
+* :kconfig:option:`CONFIG_MEMFAULT_NCS_LOCATION_METRICS`
 * :kconfig:option:`CONFIG_MEMFAULT_NCS_STACK_METRICS`
 * :kconfig:option:`CONFIG_MEMFAULT_NCS_BT_METRICS`
 

diff --git a/modules/memfault-firmware-sdk/CMakeLists.txt b/modules/memfault-firmware-sdk/CMakeLists.txt
@@ -22,6 +22,11 @@ zephyr_library_sources_ifdef(
         memfault_lte_metrics.c
 )
 
+zephyr_library_sources_ifdef(
+        CONFIG_MEMFAULT_NCS_LOCATION_METRICS
+        memfault_location_metrics.c
+)
+
 zephyr_library_sources_ifdef(
         CONFIG_MEMFAULT_NCS_BT_METRICS
         memfault_bt_metrics.c)

diff --git a/modules/memfault-firmware-sdk/Kconfig b/modules/memfault-firmware-sdk/Kconfig
@@ -168,10 +168,18 @@ config MEMFAULT_NCS_BT_METRICS
 	help
 	  Collect metrics related to the Bluetooth stack in background while application is running.
 
+config MEMFAULT_NCS_LOCATION_METRICS
+	bool "Collect location metrics"
+	depends on LOCATION
+	depends on LOCATION_DATA_DETAILS
+	default y
+	help
+	  Collect metrics related to location fixes while the application is running.
+
 config MEMFAULT_NCS_USE_DEFAULT_METRICS
 	bool
 	select MEMFAULT_METRICS_EXTRA_DEFS_FILE
-	default y if (MEMFAULT_NCS_STACK_METRICS || MEMFAULT_NCS_LTE_METRICS || MEMFAULT_NCS_BT_METRICS)
+	default y if (MEMFAULT_NCS_STACK_METRICS || MEMFAULT_NCS_LTE_METRICS || MEMFAULT_NCS_BT_METRICS || MEMFAULT_NCS_LOCATION_METRICS)
 
 config MEMFAULT_NCS_IMPLEMENT_METRICS_COLLECTION
 	bool "Implement metrics collection"

diff --git a/modules/memfault-firmware-sdk/config/memfault_metrics_heartbeat_extra.def b/modules/memfault-firmware-sdk/config/memfault_metrics_heartbeat_extra.def
@@ -38,3 +38,59 @@ MEMFAULT_METRICS_KEY_DEFINE(ncs_bt_connection_time_ms, kMemfaultMetricType_Timer
 MEMFAULT_METRICS_KEY_DEFINE(ncs_bt_connection_count, kMemfaultMetricType_Unsigned)
 MEMFAULT_METRICS_KEY_DEFINE(ncs_bt_bond_count, kMemfaultMetricType_Unsigned)
 #endif /* CONFIG_MEMFAULT_NCS_BT_METRICS */
+
+#ifdef CONFIG_MEMFAULT_NCS_LOCATION_METRICS
+
+/*
+ * Heartbeat metrics
+ */
+MEMFAULT_METRICS_KEY_DEFINE(ncs_loc_search_request_count, kMemfaultMetricType_Unsigned)
+
+/*
+ * Session metrics
+ */
+MEMFAULT_METRICS_SESSION_KEY_DEFINE(ncs_loc)
+
+/* Successful search metrics */
+MEMFAULT_METRICS_KEY_DEFINE_WITH_SESSION(ncs_loc_search_success, kMemfaultMetricType_Unsigned, ncs_loc)
+MEMFAULT_METRICS_KEY_DEFINE_WITH_SESSION(ncs_loc_search_time_ms, kMemfaultMetricType_Unsigned, ncs_loc)
+MEMFAULT_METRICS_KEY_DEFINE_WITH_SESSION(ncs_loc_accuracy_cm, kMemfaultMetricType_Unsigned, ncs_loc)
+
+/* Failed search metrics */
+MEMFAULT_METRICS_KEY_DEFINE_WITH_SESSION(ncs_loc_search_failure, kMemfaultMetricType_Unsigned, ncs_loc)
+
+/*
+ * Method-specific metrics (GNSS, Cellular, Wi-Fi)
+ * - All reported in both successful and failed searches unless noted
+ * - *_method_result metric values map to the location_event_id enumeration in
+ * nrf/include/modem/location.h
+ */
+
+/* GNSS */
+#if defined(CONFIG_LOCATION_METHOD_GNSS)
+MEMFAULT_METRICS_KEY_DEFINE_WITH_SESSION(ncs_loc_gnss_method_time_ms, kMemfaultMetricType_Unsigned, ncs_loc)
+MEMFAULT_METRICS_KEY_DEFINE_WITH_SESSION(ncs_loc_gnss_method_result, kMemfaultMetricType_Unsigned, ncs_loc)
+MEMFAULT_METRICS_KEY_DEFINE_WITH_SESSION(ncs_loc_gnss_on_time_ms, kMemfaultMetricType_Unsigned, ncs_loc)
+MEMFAULT_METRICS_KEY_DEFINE_WITH_SESSION(ncs_loc_gnss_satellites_tracked_count, kMemfaultMetricType_Unsigned, ncs_loc)
+MEMFAULT_METRICS_KEY_DEFINE_WITH_SESSION(ncs_loc_gnss_satellites_used_count, kMemfaultMetricType_Unsigned, ncs_loc)
+
+/* Only reported in successful session */
+MEMFAULT_METRICS_KEY_DEFINE_WITH_SESSION(ncs_loc_gnss_time_to_fix_ms, kMemfaultMetricType_Unsigned, ncs_loc)
+#endif /* CONFIG_LOCATION_METHOD_GNSS */
+
+/* Cellular */
+#if defined(CONFIG_LOCATION_METHOD_CELLULAR)
+MEMFAULT_METRICS_KEY_DEFINE_WITH_SESSION(ncs_loc_lte_method_time_ms, kMemfaultMetricType_Unsigned, ncs_loc)
+MEMFAULT_METRICS_KEY_DEFINE_WITH_SESSION(ncs_loc_lte_method_result, kMemfaultMetricType_Unsigned, ncs_loc)
+MEMFAULT_METRICS_KEY_DEFINE_WITH_SESSION(ncs_loc_lte_neighbor_cells_count, kMemfaultMetricType_Unsigned, ncs_loc)
+MEMFAULT_METRICS_KEY_DEFINE_WITH_SESSION(ncs_loc_lte_gci_cells_count, kMemfaultMetricType_Unsigned, ncs_loc)
+#endif /* CONFIG_LOCATION_METHOD_CELLULAR */
+
+/* Wi-Fi */
+#if defined(CONFIG_LOCATION_METHOD_WIFI)
+MEMFAULT_METRICS_KEY_DEFINE_WITH_SESSION(ncs_loc_wifi_method_time_ms, kMemfaultMetricType_Unsigned, ncs_loc)
+MEMFAULT_METRICS_KEY_DEFINE_WITH_SESSION(ncs_loc_wifi_method_result, kMemfaultMetricType_Unsigned, ncs_loc)
+MEMFAULT_METRICS_KEY_DEFINE_WITH_SESSION(ncs_loc_wifi_ap_count, kMemfaultMetricType_Unsigned, ncs_loc)
+#endif /* CONFIG_LOCATION_METHOD_WIFI */
+
+#endif /* CONFIG_MEMFAULT_NCS_LOCATION_METRICS */
diff --git a/modules/memfault-firmware-sdk/include/memfault_location_metrics.h b/modules/memfault-firmware-sdk/include/memfault_location_metrics.h
@@ -0,0 +1,21 @@
+/*
+ * Copyright (c) 2024 Nordic Semiconductor ASA
+ *
+ * SPDX-License-Identifier: LicenseRef-Nordic-5-Clause
+ */
+
+#ifndef MEMFAULT_LOCATION_METRICS_H_
+#define MEMFAULT_LOCATION_METRICS_H_
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+/** @brief Initialize default location metrics. */
+void memfault_location_metrics_init(void);
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* MEMFAULT_LOCATION_METRICS_H_ */