Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

memfault: add location session metrics #21

Closed
wants to merge 1 commit into from

Conversation

gminn
Copy link
Member

@gminn gminn commented Mar 28, 2024

Summary

Metrics around a search session for a location fix give insights
into the performance of methods/devices being used to perform
the search, notably GNSS receivers but also cellular modems and
Wi-Fi chips. With this information, developers can adjust search
timeouts and enable/disable different methods that don't perform
as well giving the conditions their device is in.

Previously, GNSS metrics were being gathered in the asset tracker
app and reported in a heartbeat that was triggered whenever a
location session completed. This commit removes those heartbeat
metrics and adds metrics to observe a location search session, both
overall (how long it took, whether it was successful) and more
specifically the performance of each method.

New metrics
  • Overall session
    • If successful
      • ncs_loc_search_time_ms
      • ncs_loc_search_success (0/1)
      • ncs_loc_accuracy_cm
    • If failed:
      • ncs_loc_search_timeout_ms - only recorded in a timeout
      • ncs_loc_search_failure (0/1)
  • GNSS
    • ncs_loc_gnss_time_to_fix_ms
    • ncs_loc_gnss_on_time_ms
    • ncs_loc_gnss_satellites_tracked_count
    • ncs_loc_gnss_satellites_used_count
    • ncs_loc_gnss_method_time_ms
    • ncs_loc_gnss_method_result
  • Cellular
    • ncs_loc_lte_neighbor_cells_count
    • ncs_loc_lte_gci_cells_count
    • ncs_loc_lte_method_time_ms
    • ncs_loc_lte_method_result
  • Wi-Fi
    • ncs_loc_wifi_ap_count
    • ncs_loc_wifi_method_time_ms
    • ncs_loc_wifi_method_result

Notes:

  • The *_method_result metrics map to the location_event_id
    enumeration in the location library.
  • The *_method_time_ms metrics will be equal to the timeout
    set for the method if the error is a timeout
  • The ncs_loc_search_success and ncs_loc_search_failure
    could be used in the future to create a high level "location
    session success rate" insight (ncs_loc_search_success/
    (ncs_loc_search_success + ncs_loc_search_failure)

Since location metrics are now built-in, this commit also removes the
location metrics in the asset tracker app, and enables the location
metrics in overlay-memfault.conf.

Test Plan

Results

Note: ncs_loc_gnss_exec_time_ms was added during review -- full
results were not re-gathered but a simple test captured an expected
value in relation to the on time:

"ncs_loc_gnss_on_time_ms": 90001,
"ncs_loc_gnss_exec_time_ms": 89000,
  • Results from successful session

    • GNSS succeeds

      "metrics": {
          "mflt_session_timer_ncs_loc": 11073,
         "ncs_loc_search_success": 1,
         "ncs_loc_search_time_ms": 11070,
         "ncs_loc_accuracy_cm": 5723,
         "ncs_loc_gnss_method_time_ms": 11070,
         "ncs_loc_gnss_method_result": 1,
         "ncs_loc_gnss_on_time_ms": 11036,
         "ncs_loc_gnss_satellites_tracked_count": 6,
         "ncs_loc_gnss_satellites_used_count": 4,
         "ncs_loc_gnss_time_to_fix_ms": 5210,
      },
      "report_type": "ncs_loc",
      
    • LTE succeeds

      "metrics": {
          "mflt_session_timer_ncs_loc": 11750,
          "ncs_loc_search_success": 1,
          "ncs_loc_search_time_ms": 11748,
          "ncs_loc_lte_method_time_ms": 11748,
          "ncs_loc_lte_method_result": 1,
          "ncs_loc_lte_neighbor_cells_count": 0,
          "ncs_loc_lte_gci_cells_count": 2,
      

      Required config adjustments:

      CONFIG_LOCATION_METHOD_GNSS=n
      CONFIG_LOCATION_REQUEST_DEFAULT_METHOD_FIRST_GNSS=n
      
  • Results from failed session

    • GNSS alone fails

      "metrics": {
          "mflt_session_timer_ncs_loc": 100063,
          "ncs_loc_search_failure": 1,
          "ncs_loc_gnss_method_time_ms": 100061,
          "ncs_loc_gnss_method_result": 2,
          "ncs_loc_gnss_on_time_ms": 90000,
          "ncs_loc_gnss_satellites_tracked_count": 0,
          "ncs_loc_gnss_satellites_used_count": 0,
        },
        "report_type": "ncs_loc",
      

      Debug prints:

      [01:21:16.300,689] <dbg> memfault_ncs_metrics: memfault_location_metrics_start_fix_session: Starting location session
      [01:22:46.324,035] <dbg> memfault_ncs_metrics: memfault_location_metrics_stop_fix_session: Timeout recorded
      [01:22:46.324,066] <dbg> memfault_ncs_metrics: memfault_location_metrics_stop_fix_session: Stopping location session, no fix found, id=2
      

      Required config adjustments:

      CONFIG_LOCATION_METHOD_CELLULAR=n
      CONFIG_LOCATION_REQUEST_DEFAULT_METHOD_SECOND_CELLULAR=n
      
      diff --git a/src/modules/location_module.c b/src/modules/location_module.c
      index df7366b..1a1335c 100644
      --- a/src/modules/location_module.c
      +++ b/src/modules/location_module.c
      @@ -333,6 +333,8 @@ static inline int adjust_rsrq(int input)
       static void send_cloud_location_update(const struct location_data_cloud *cloud_location_info)
       {
              struct location_module_event *evt = new_location_module_event();
      +
      +#if defined(CONFIG_LOCATION_METHOD_CELLULAR)
              struct location_module_neighbor_cells *evt_ncells =
                      &evt->data.cloud_location.neighbor_cells;
              evt->data.cloud_location.neighbor_cells_valid = false;
      @@ -363,6 +365,7 @@ static void send_cloud_location_update(const struct location_data_cloud *cloud_l
                                      adjust_rsrq(evt_ncells->neighbor_cells[i].rsrq);
                      }
              }
      +#endif
      
       #if defined(CONFIG_LOCATION_METHOD_WIFI)
              evt->data.cloud_location.wifi_access_points_valid = false;
      
    • GNSS fails, cellular fails

      "event_info": {
          "metrics": {
           "mflt_session_timer_ncs_loc": 100625,
           "ncs_loc_search_failure": 1,
           "ncs_loc_gnss_method_time_ms": 99771,
           "ncs_loc_gnss_method_result": 2, // timeout
           "ncs_loc_gnss_on_time_ms": 90000,
           "ncs_loc_gnss_satellites_tracked_count": 0,
           "ncs_loc_gnss_satellites_used_count": 0,
           "ncs_loc_lte_method_time_ms": 853,
           "ncs_loc_lte_method_result": 4, // unknown
           "ncs_loc_lte_neighbor_cells_count": 1,
           "ncs_loc_lte_gci_cells_count": 3,
          },
          "report_type": "ncs_loc",
      

      Debug prints:

      [00:18:58.443,481] <dbg> memfault_ncs_metrics: memfault_location_metrics_start_fix_session: Starting location session
      [00:20:28.471,160] <dbg> memfault_ncs_metrics: memfault_location_method_fallback_record: GNSS method attempted
      [00:20:28.471,191] <dbg> memfault_ncs_metrics: memfault_location_method_fallback_record: Falling back to Cellular method
      [00:20:29.746,826] <dbg> memfault_ncs_metrics: memfault_location_metrics_stop_fix_session: Stopping location session, no fix found, id=4
      
    • Wifi alone fails

        "event_info": {
          "metrics": {
            "ncs_loc_search_failure": 1,
            "ncs_loc_wifi_method_time_ms": 30026,
            "ncs_loc_wifi_method_result": 2, // timeout
            "ncs_loc_wifi_ap_count": 30, // matches count in debug logs below
            "MemfaultSdkMetric_IntervalMs": 30052
          },
          "report_type": "ncs_loc",
          "duration_ms": 30052
        },
      

      Debug logs:

      [00:00:31.568,969] <dbg> location_module: search_start: Requesting location...
      [00:00:31.579,101] <dbg> location_module: location_event_handler: Location request has been started with 'Wi-Fi' method
      [00:00:31.592,651] <dbg> memfault_ncs_metrics: prv_location_metrics_session_start: Starting location session
      [00:00:31.607,299] <dbg> location_module: sub_state_set: Sub state transition SUB_STATE_IDLE --> SUB_STATE_SEARCH
      [00:00:36.035,308] <dbg> mflt: memfault_platform_log: LTE mode: 1
      
      Num  | SSID                             (len) | Chan (Band)   | RSSI | Security        | BSSID             | MFP
      1    | SpectrumSetup-79                 16    | 11   (2.4GHz) | -21  | WPA2-PSK        | 74:37:5F:CE:E5:7B | Disable
      ...
      30   |                                  0     | 116  (5GHz  ) | -72  | WPA3-SAE        | 68:4A:76:F0:C4:C3 | Disable
      Scan request done
      [00:00:36.666,259] <dbg> location_module: location_event_handler: Getting cloud location request
      [00:01:01.643,615] <dbg> memfault_ncs_metrics: prv_location_metrics_session_stop: Stopping location session, no fix found, id=2
      

      Required config adjustments:

      CONFIG_LOCATION_METHOD_GNSS=n
      CONFIG_LOCATION_METHOD_CELLULAR=n
      CONFIG_LOCATION_REQUEST_DEFAULT_METHOD_FIRST_WIFI=y
      

      Used the nRF7002-EK on top of the nRF9160dk. My build command was:

      west build -b nrf9160dk_nrf9160_ns -p always memfault-asset-tracker -- \
      -DCONFIG_MEMFAULT_NCS_PROJECT_KEY=\"<MY_PROJECT_KEY>\" \
      -DOVERLAY_CONFIG="overlay-memfault.conf;overlay-nrf7002ek-wifi-scan-only.conf" \
      -DEXTRA_DTC_OVERLAY_FILE="nrf9160dk_with_nrf7002ek.overlay" \
      -DSHIELD="nrf7002ek"
      

      I also needed to flip a handful of Kconfig flags so I could fit the image in the flash partition.
      Using the EK bumped the image size considerably.

  • Checked correct value of ncs_loc_search_request_count heartbeat
    metric by checking number of sessions reported in heartbeat window

    "ncs_loc_search_request_count": 4,
    
  • Confirmed asset tracker app builds with in-tree app changes

Notable results not gathered
  • Wifi scan succeeds alone. Currently do not have a good approach for getting
    a scan to successfully determine location. But I confirmed the scan itself
    was successful (the number of APs printed matched what was captured in
    the metric and my wifi network was on the list)

  • GNSS fails, cellular or wifi succeeds. Currently do not have a good approach
    for forcing this test result, but we are seeing some cellular and wifi data so data
    collection is working properly. We can test on our fleet for some time to see if
    various conditions result in this case


Resolves: MCU-191

@gminn gminn force-pushed the gminn/add-location-metrics branch 3 times, most recently from 5aaf033 to 86b4ae1 Compare April 1, 2024 16:59
@gminn gminn requested a review from a team April 1, 2024 16:59
@gminn gminn marked this pull request as ready for review April 1, 2024 17:00
@gminn gminn changed the title feat(location-metrics): add location metrics support feat(location): add location metrics support Apr 1, 2024
@gminn gminn changed the title feat(location): add location metrics support feat(location): add location session metrics Apr 1, 2024
Copy link

@noahp noahp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SWEET!

@gminn gminn force-pushed the gminn/add-location-metrics branch from 86b4ae1 to 9a5fa4e Compare April 17, 2024 16:12
@gminn gminn requested a review from noahp April 17, 2024 16:15
Copy link

@noahp noahp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sweet, lgtm!

@gminn gminn force-pushed the gminn/add-location-metrics branch from 9a5fa4e to 67c1fc9 Compare April 17, 2024 18:45
Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Jun 17, 2024
@gminn gminn removed the Stale label Jun 20, 2024
@gminn gminn force-pushed the gminn/add-location-metrics branch from 67c1fc9 to 0bba332 Compare June 27, 2024 13:56
@gminn gminn force-pushed the gminn/add-location-metrics branch 5 times, most recently from f92a7ba to c00c61c Compare June 27, 2024 18:29
@gminn gminn force-pushed the gminn/add-location-metrics branch 2 times, most recently from d5206cc to 0e139a0 Compare June 27, 2024 18:56
@gminn gminn changed the title feat(location): add location session metrics memfault: add location session metrics Jun 27, 2024
Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Aug 27, 2024
@gminn gminn removed the Stale label Aug 27, 2024
Copy link

@trantanen trantanen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall! Mainly small remarks.

@gminn gminn force-pushed the gminn/add-location-metrics branch 4 times, most recently from 9805381 to b39bdc7 Compare September 13, 2024 19:17
- Add location metrics
- Remove asset tracker app location metrics

Signed-off-by: Gillian Minnehan <gillian@memfault.com>
@trantanen
Copy link

trantanen commented Sep 16, 2024

Approved (I don't have rights to do it officially with github in this repository 😃)

gminn added a commit that referenced this pull request Sep 17, 2024
 ### Summary

Internal & nordic approval on
#21,
so merging into internal dogfooding branch.

### Test Plan

Test details in #21
Copy link
Member Author

@gminn gminn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merged upstream in nrfconnect@2b9808a !

@gminn gminn closed this Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants