ble_hrm: use the health service event for HR updates

Stop maintaining a private HRM-manager subscription for BLE HRM sharing
and instead ride on the existing PEBBLE_HEALTH_SERVICE_EVENT, which is
already emitted by the health service while a workout is active (the
only context in which Activity HR Sharing advertises). This removes a
redundant 1 Hz poll and keeps the watch on a single HR data path.

- Drop the hrm_manager_private.h dependency and the s_ble_hrm_session
  struct (which paired an EventServiceInfo with an HRMSessionRef);
  subscribe a single s_ble_hrm_event_info to PEBBLE_HEALTH_SERVICE_EVENT
- Rename prv_ble_hrm_handle_hrm_data to prv_handle_health_event and read
  heart-rate data from HealthEventHeartRateUpdate instead of the raw
  HRM event
- Remove the hrm_manager_subscribe_with_callback /
  sys_hrm_manager_unsubscribe calls from the start/stop paths
- Update the unit tests: drop the now-unused subscribe/unsubscribe
  stubs and counters, point the event-service assertion at
  PEBBLE_HEALTH_SERVICE_EVENT, and rename the HR-event test/helper to
  *health_event

Co-Authored-By: glm-5.2
Signed-off-by: Brint E. Kriebel <github@bekit.net>

Author: bekriebel

src/fw/services/bluetooth/ble_hrm.c

@@ -14,7 +14,6 @@
 #include "popups/ble_hrm/ble_hrm_sharing_popup.h"
 #include "process_management/app_manager.h"
 #include "pbl/services/analytics/analytics.h"
-#include "pbl/services/hrm/hrm_manager_private.h"
 #include "pbl/services/regular_timer.h"
 #include "pbl/services/activity/activity.h"
 #include "pbl/services/activity/workout_service.h"
@@ -41,10 +40,7 @@ typedef struct BLEHRMSharingRequest {
 static bool s_ble_hrm_is_inited;
 static int s_ble_hrm_subscription_count;
 static RegularTimerInfo s_ble_hrm_timer;
-static struct {
-  EventServiceInfo service_info;
-  HRMSessionRef manager_session;
-} s_ble_hrm_session;
+static EventServiceInfo s_ble_hrm_event_info;
 
 typedef enum {
   HrmSharingPermission_Unknown,
@@ -175,21 +171,21 @@ static size_t prv_copy_sharing_devices(BTDeviceInternal *devices_out,
   return (max_devices - ctx.slots_left);
 }
 
-static void prv_ble_hrm_handle_hrm_data(PebbleEvent *e, void *context) {
+static void prv_handle_health_event(PebbleEvent *e, void *context) {
   if (!s_ble_hrm_is_inited) {
     return;
   }
   if (s_ble_hrm_subscription_count == 0) {
     return;
   }
-  PBL_ASSERTN(e->type == PEBBLE_HRM_EVENT);
-  const PebbleHRMEvent *const hrm_event = &e->hrm;
-  if (hrm_event->event_type != HRMEvent_BPM) {
+  PBL_ASSERTN(e->type == PEBBLE_HEALTH_SERVICE_EVENT);
+  const PebbleHealthEvent *const health_event = &e->health_event;
+  if (health_event->type != HealthEventHeartRateUpdate) {
     return;
   }
   const BleHrmServiceMeasurement measurement = {
-    .bpm = hrm_event->bpm.bpm,
-    .is_on_wrist = (hrm_event->bpm.quality >= HRMQuality_Worst),
+    .bpm = health_event->data.heart_rate_update.current_bpm,
+    .is_on_wrist = (health_event->data.heart_rate_update.quality >= HRMQuality_Worst),
   };
 
   BTDeviceInternal sharing_to_devices[4];
@@ -200,22 +196,16 @@ static void prv_ble_hrm_handle_hrm_data(PebbleEvent *e, void *context) {
 
 static void prv_start_hrm_kernel_main(void *unused) {
   PBL_LOG_INFO("BLE HRM sharing started");
-  s_ble_hrm_session.service_info = (EventServiceInfo) {
-    .type = PEBBLE_HRM_EVENT,
-    .handler = prv_ble_hrm_handle_hrm_data,
+  s_ble_hrm_event_info = (EventServiceInfo) {
+    .type = PEBBLE_HEALTH_SERVICE_EVENT,
+    .handler = prv_handle_health_event,
   };
-  event_service_client_subscribe(&s_ble_hrm_session.service_info);
-  s_ble_hrm_session.manager_session =
-      hrm_manager_subscribe_with_callback(INSTALL_ID_INVALID, 1 /*update_interval_s*/,
-                                          0 /*expire_s*/, HRMFeature_BPM, NULL, NULL);
-
+  event_service_client_subscribe(&s_ble_hrm_event_info);
 }
 
 static void prv_stop_hrm_kernel_main(void *unused) {
   PBL_LOG_INFO("BLE HRM sharing stopped");
-  sys_hrm_manager_unsubscribe(s_ble_hrm_session.manager_session);
-  event_service_client_unsubscribe(&s_ble_hrm_session.service_info);
-
+  event_service_client_unsubscribe(&s_ble_hrm_event_info);
 }
 
 static void prv_execute_on_kernel_main(CallbackEventCallback cb) {

tests/fw/services/bluetooth/test_ble_hrm.c

@@ -4,7 +4,7 @@
 #include "pbl/services/bluetooth/ble_hrm.h"
 
 #include "comm/ble/gap_le_connection.h"
-#include "pbl/services/hrm/hrm_manager_private.h"
+#include "pbl/services/activity/activity.h"
 
 #include <bluetooth/hrm_service.h>
 #include <pbl/btutil/bt_device.h>
@@ -90,19 +90,6 @@ void ble_hrm_push_reminder_popup(void) {
   s_ble_hrm_push_reminder_popup_call_count++;
 }
 
-static int s_hrm_manager_subscribe_with_callback_call_count;
-static HRMSessionRef s_last_session_ref;
-static HRMSessionRef s_next_session_ref;
-HRMSessionRef hrm_manager_subscribe_with_callback(AppInstallId app_id, uint32_t update_interval_s,
-                                                  uint16_t expire_s, HRMFeature features,
-                                                  HRMSubscriberCallback callback, void *context) {
-  cl_assert_equal_p(NULL, callback); // we're using the event service
-  cl_assert_equal_i(features, HRMFeature_BPM);
-  ++s_hrm_manager_subscribe_with_callback_call_count;
-  s_last_session_ref = ++s_next_session_ref;
-  return s_last_session_ref;
-}
-
 static GAPLEConnection *s_connections[2];
 
 GAPLEConnection *gap_le_connection_by_device(const BTDeviceInternal *device) {
@@ -140,18 +127,11 @@ bool sys_hrm_manager_is_hrm_present(void) {
   return true;
 }
 
-static int s_sys_hrm_manager_unsubscribe_call_count;
-bool sys_hrm_manager_unsubscribe(HRMSessionRef session) {
-  ++s_sys_hrm_manager_unsubscribe_call_count;
-  cl_assert_equal_i(session, s_last_session_ref);
-  return true;
-}
-
 ////////////////////////////////////////////////////////////////////////////////////////////////////
 // Tests
 
 static void prv_assert_event_service_subscribed(bool is_subscribed) {
-  const EventServiceInfo *const info = fake_event_service_get_info(PEBBLE_HRM_EVENT);
+  const EventServiceInfo *const info = fake_event_service_get_info(PEBBLE_HEALTH_SERVICE_EVENT);
   if (is_subscribed) {
     cl_assert(info->handler);
   } else {
@@ -163,10 +143,6 @@ void test_ble_hrm__cleanup(void) {
   ble_hrm_deinit();
 
   prv_assert_event_service_subscribed(false);
-  // hrm manager sub vs unsub calls should be the same, there should be no subscription any more
-  // after de-initing:
-  cl_assert_equal_i(s_sys_hrm_manager_unsubscribe_call_count,
-                    s_hrm_manager_subscribe_with_callback_call_count);
 
   fake_pbl_malloc_check_net_allocs();
 
@@ -192,16 +168,12 @@ void test_ble_hrm__initialize(void) {
   s_last_num_permitted_devices = 0;
   memset(s_last_permitted_devices, 0, sizeof(s_last_permitted_devices));
   s_bt_driver_hrm_service_enable_call_count = 0;
-  s_hrm_manager_subscribe_with_callback_call_count = 0;
-  s_sys_hrm_manager_unsubscribe_call_count = 0;
   s_bt_driver_hrm_service_handle_measurement_call_count = 0;
   s_ble_hrm_push_sharing_request_window_call_count = 0;
   s_ble_hrm_push_reminder_popup_call_count = 0;
   s_gap_le_slave_reconnect_hrm_restart_call_count = 0;
   s_gap_le_slave_reconnect_hrm_stop_call_count = 0;
   workout_service_stop_workout();
-  s_last_session_ref = ~0;
-  s_next_session_ref = 1234;
   s_last_disconnected = (BTDeviceInternal) {};
   s_last_sharing_request = NULL;
   s_last_ble_hrm_measurement = (BleHrmServiceMeasurement) {};
@@ -243,59 +215,48 @@ static void prv_assert_permissions_ui_and_respond(bool is_granted) {
 }
 
 void test_ble_hrm__sub_unsub(void) {
-  cl_assert_equal_i(s_hrm_manager_subscribe_with_callback_call_count, 0);
-  cl_assert_equal_i(s_sys_hrm_manager_unsubscribe_call_count, 0);
   prv_assert_event_service_subscribed(false);
 
   // Device A subscribes:
   bt_driver_cb_hrm_service_update_subscription(s_device_a, true);
 
-  // Expect HRM manager NOT to be subscribed to yet, need to grant permission first:
-  cl_assert_equal_i(s_hrm_manager_subscribe_with_callback_call_count, 0);
+  // Expect health event subscription NOT to be active yet, need to grant permission first:
   prv_assert_event_service_subscribed(false);
   cl_assert_equal_b(false, ble_hrm_is_sharing_to_connection(&s_conn_a));
 
   // Expect permissions UI to be presented:
   prv_assert_permissions_ui_and_respond(true /* is_granted */);
 
-  // Expect HRM manager to be subscribed to:
-  cl_assert_equal_i(s_hrm_manager_subscribe_with_callback_call_count, 1);
+  // Expect health event subscription to be active:
   prv_assert_event_service_subscribed(true);
   cl_assert_equal_b(true, ble_hrm_is_sharing_to_connection(&s_conn_a));
 
   // Device A subscribes again, should be a no-op, no new permissions prompt:
   bt_driver_cb_hrm_service_update_subscription(s_device_a, true);
-  cl_assert_equal_i(s_hrm_manager_subscribe_with_callback_call_count, 1);
-  cl_assert_equal_i(s_sys_hrm_manager_unsubscribe_call_count, 0);
+  prv_assert_event_service_subscribed(true);
 
-  // Device B subscribes, shouldn't resubscribe to HRM manager, but should present a new
+  // Device B subscribes, shouldn't resubscribe, but should present a new
   // permission prompt, because it's a different device:
   bt_driver_cb_hrm_service_update_subscription(s_device_b, true);
   cl_assert_equal_b(false, ble_hrm_is_sharing_to_connection(&s_conn_b));
   prv_assert_permissions_ui_and_respond(true /* is_granted */);
   cl_assert_equal_b(true, ble_hrm_is_sharing_to_connection(&s_conn_b));
   prv_assert_event_service_subscribed(true);
-  cl_assert_equal_i(s_hrm_manager_subscribe_with_callback_call_count, 1);
-  cl_assert_equal_i(s_sys_hrm_manager_unsubscribe_call_count, 0);
 
-  // Device A disconnects, shouldn't unsubscribe from HRM manager because A is still subscribed:
+  // Device A disconnects, shouldn't unsubscribe because B is still subscribed:
   ble_hrm_handle_disconnection(&s_conn_a);
   cl_assert_equal_b(false, ble_hrm_is_sharing_to_connection(&s_conn_a));
   prv_assert_event_service_subscribed(true);
-  cl_assert_equal_i(s_hrm_manager_subscribe_with_callback_call_count, 1);
-  cl_assert_equal_i(s_sys_hrm_manager_unsubscribe_call_count, 0);
 
-  // Device B unsubscribes, expect to be unsubscribed from HRM manager, because there are no more
+  // Device B unsubscribes, expect to unsubscribe because there are no more
   // devices subscribed to the BLE HRM service:
   bt_driver_cb_hrm_service_update_subscription(s_device_b, false);
   cl_assert_equal_b(false, ble_hrm_is_sharing_to_connection(&s_conn_a));
   prv_assert_event_service_subscribed(false);
-  cl_assert_equal_i(s_sys_hrm_manager_unsubscribe_call_count, 1);
 
   // Device B unsubscribes again, should be no-op
   bt_driver_cb_hrm_service_update_subscription(s_device_b, false);
   prv_assert_event_service_subscribed(false);
-  cl_assert_equal_i(s_sys_hrm_manager_unsubscribe_call_count, 1);
 }
 
 void test_ble_hrm__sub_unsub_resub(void) {
@@ -413,7 +374,7 @@ void test_ble_hrm__unsub_upon_deinit(void) {
   bt_driver_cb_hrm_service_update_subscription(s_device_a, true);
   prv_assert_permissions_ui_and_respond(true /* is_granted */);
 
-  // __cleanup() will do the deinit and also assert that  there's no subscription to the HRM mgr.
+  // __cleanup() will do the deinit and also assert that the event service is unsubscribed.
 }
 
 // Test that we handle a races where a subscription/disconnection callback happens in after
@@ -423,30 +384,29 @@ void test_ble_hrm__sub_after_deinit(void) {
 
   bt_driver_cb_hrm_service_update_subscription(s_device_a, true);
   prv_assert_event_service_subscribed(false);
-  cl_assert_equal_i(s_hrm_manager_subscribe_with_callback_call_count, 0);
 
   ble_hrm_handle_disconnection(&s_conn_a);
   prv_assert_event_service_subscribed(false);
-  cl_assert_equal_i(s_hrm_manager_subscribe_with_callback_call_count, 0);
 
   ble_hrm_init(); // reinit, __cleanup() will deinit again
 }
 
-static void prv_put_and_assert_hrm_event(HRMEventType subtype, uint8_t bpm, HRMQuality quality,
-                                         bool expect_bt_driver_cb, bool expected_is_on_wrist) {
+static void prv_put_and_assert_health_event(HealthEventType type, uint8_t bpm,
+                                             HRMQuality quality, bool expect_bt_driver_cb,
+                                             bool expected_is_on_wrist) {
   int call_count_before = s_bt_driver_hrm_service_handle_measurement_call_count;
 
-  PebbleEvent hrm_event = {
-    .type = PEBBLE_HRM_EVENT,
-    .hrm = {
-      .event_type = subtype,
-      .bpm = {
-        .bpm = bpm,
+  PebbleEvent event = {
+    .type = PEBBLE_HEALTH_SERVICE_EVENT,
+    .health_event = {
+      .type = type,
+      .data.heart_rate_update = {
+        .current_bpm = bpm,
         .quality = quality,
       },
     },
   };
-  event_put(&hrm_event);
+  event_put(&event);
   fake_event_service_handle_last();
 
   if (expect_bt_driver_cb) {
@@ -467,7 +427,7 @@ static void prv_put_workout_event(PebbleWorkoutEventType type) {
   fake_event_service_handle_last();
 }
 
-void test_ble_hrm__handle_hrm_event(void) {
+void test_ble_hrm__handle_health_event(void) {
   bt_driver_cb_hrm_service_update_subscription(s_device_a, true);
   cl_assert_equal_i(0, s_bt_driver_hrm_service_handle_measurement_call_count);
   prv_assert_permissions_ui_and_respond(true /* is_granted */);
@@ -476,19 +436,19 @@ void test_ble_hrm__handle_hrm_event(void) {
   bt_driver_cb_hrm_service_update_subscription(s_device_b, true);
   prv_assert_permissions_ui_and_respond(false /* is_granted */);
 
-  prv_put_and_assert_hrm_event(HRMEvent_BPM, 80, HRMQuality_Excellent,
-                               true /* expect bt driver cb */, true /* expected_is_on_wrist */);
+  prv_put_and_assert_health_event(HealthEventHeartRateUpdate, 80, HRMQuality_Excellent,
+                                  true /* expect bt driver cb */, true /* expected_is_on_wrist */);
 
   // Assert only device A is listed as "permitted device" and B is not:
   cl_assert_equal_i(1, s_last_num_permitted_devices);
   cl_assert_equal_m(&s_last_permitted_devices[0], s_device_a, sizeof(*s_device_a));
 
-  prv_put_and_assert_hrm_event(HRMEvent_BPM, 80, HRMQuality_OffWrist,
-                               true /* expect bt driver cb */, false /* expected_is_on_wrist */);
+  prv_put_and_assert_health_event(HealthEventHeartRateUpdate, 80, HRMQuality_OffWrist,
+                                  true /* expect bt driver cb */, false /* expected_is_on_wrist */);
 
-  // Ignore non-BPM event:
-  prv_put_and_assert_hrm_event(HRMEvent_HRV, 80, HRMQuality_OffWrist,
-                               false /* expect bt driver cb */, false /* expected_is_on_wrist */);
+  // Ignore non-heart-rate health event:
+  prv_put_and_assert_health_event(HealthEventMovementUpdate, 0, HRMQuality_OffWrist,
+                                  false /* expect bt driver cb */, false /* expected_is_on_wrist */);
 }
 
 void test_ble_hrm__handle_ble_hrm_sharing_enabled_changes(void) {
@@ -557,9 +517,6 @@ void test_ble_hrm__workout_stop_stops_advert_and_disconnects_subscribers(void) {
 
   cl_assert_equal_i(1, s_gap_le_slave_reconnect_hrm_stop_call_count);
   prv_assert_last_disconnected(s_device_a);
-  // HRM manager should have been unsubscribed:
-  cl_assert_equal_i(s_sys_hrm_manager_unsubscribe_call_count,
-                    s_hrm_manager_subscribe_with_callback_call_count);
 }
 
 void test_ble_hrm__workout_stop_skips_non_sharers(void) {