Add BatteryChargeStatusObserver to SystemDataProvider

Allows clients to implement the BatteryChargeStatusObserver interface
and register themselves as an observer in order to receive updates about
battery charge status.

Bug: 1128204
Change-Id: I7a6d87c44248de132e8318ca77a83ce20263df95
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2427424
Commit-Queue: Bailey Berro <baileyberro@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Reviewed-by: Zentaro Kavanagh <zentaro@chromium.org>
Cr-Commit-Position: refs/heads/master@{#812772}

chromeos/components/diagnostics_ui/backend/system_data_provider.cc

@@ -10,7 +10,10 @@
 
 #include "base/bind.h"
 #include "base/callback.h"
+#include "base/i18n/time_formatting.h"
 #include "base/optional.h"
+#include "base/time/time.h"
+#include "base/timer/timer.h"
 #include "chromeos/components/diagnostics_ui/backend/cros_healthd_helpers.h"
 #include "chromeos/components/diagnostics_ui/backend/power_manager_client_conversions.h"
 #include "chromeos/dbus/power_manager/power_supply_properties.pb.h"
@@ -26,6 +29,8 @@ using PhysicalCpuInfos = std::vector<healthd::PhysicalCpuInfoPtr>;
 using PowerSupplyProperties = power_manager::PowerSupplyProperties;
 using ProbeCategories = healthd::ProbeCategoryEnum;
 
+constexpr int kChargeStatusRefreshIntervalInSeconds = 15;
+
 void PopulateBoardName(const healthd::SystemInfo& system_info,
                        mojom::SystemInfo& out_system_info) {
   const base::Optional<std::string>& product_name = system_info.product_name;
@@ -112,6 +117,7 @@ void PopulateBatteryChargeStatus(
 }  // namespace
 
 SystemDataProvider::SystemDataProvider() {
+  battery_charge_status_timer_ = std::make_unique<base::RepeatingTimer>();
   PowerManagerClient::Get()->AddObserver(this);
 }
 
@@ -138,7 +144,26 @@ void SystemDataProvider::GetBatteryInfo(GetBatteryInfoCallback callback) {
                      base::Unretained(this), std::move(callback)));
 }
 
-void SystemDataProvider::PowerChanged(const PowerSupplyProperties& proto) {
+void SystemDataProvider::ObserveBatteryChargeStatus(
+    mojo::PendingRemote<mojom::BatteryChargeStatusObserver> observer) {
+  battery_charge_status_observers_.Add(std::move(observer));
+
+  if (!battery_charge_status_timer_->IsRunning()) {
+    battery_charge_status_timer_->Start(
+        FROM_HERE,
+        base::TimeDelta::FromSeconds(kChargeStatusRefreshIntervalInSeconds),
+        base::BindRepeating(&SystemDataProvider::UpdateBatteryChargeStatus,
+                            base::Unretained(this)));
+  }
+  UpdateBatteryChargeStatus();
+}
+
+void SystemDataProvider::PowerChanged(
+    const power_manager::PowerSupplyProperties& proto) {
+  if (battery_charge_status_observers_.empty()) {
+    return;
+  }
+
   // Fetch updated data from CrosHealthd
   BindCrosHealthdProbeServiceIfNeccessary();
   probe_service_->ProbeTelemetryInfo(
@@ -147,6 +172,11 @@ void SystemDataProvider::PowerChanged(const PowerSupplyProperties& proto) {
                      base::Unretained(this), proto));
 }
 
+void SystemDataProvider::SetBatteryChargeStatusTimerForTesting(
+    std::unique_ptr<base::RepeatingTimer> timer) {
+  battery_charge_status_timer_ = std::move(timer);
+}
+
 void SystemDataProvider::OnSystemInfoProbeResponse(
     GetSystemInfoCallback callback,
     healthd::TelemetryInfoPtr info_ptr) {
@@ -237,12 +267,14 @@ void SystemDataProvider::OnBatteryChargeStatusUpdated(
   if (info_ptr.is_null()) {
     LOG(ERROR) << "Null response from croshealthd::ProbeTelemetryInfo.";
     NotifyBatteryChargeStatusObservers(battery_charge_status);
+    battery_charge_status_timer_.reset();
     return;
   }
 
   if (!power_supply_properties.has_value()) {
     LOG(ERROR) << "Null response from power_manager_client::GetLastStatus.";
     NotifyBatteryChargeStatusObservers(battery_charge_status);
+    battery_charge_status_timer_.reset();
     return;
   }
 
@@ -252,6 +284,7 @@ void SystemDataProvider::OnBatteryChargeStatusUpdated(
               DoesDeviceHaveBattery(*power_supply_properties))
         << "Sources should not disagree about whether there is a battery.";
     NotifyBatteryChargeStatusObservers(battery_charge_status);
+    battery_charge_status_timer_.reset();
     return;
   }
 
@@ -263,7 +296,9 @@ void SystemDataProvider::OnBatteryChargeStatusUpdated(
 
 void SystemDataProvider::NotifyBatteryChargeStatusObservers(
     const mojom::BatteryChargeStatusPtr& battery_charge_status) {
-  NOTIMPLEMENTED();  // Implemented in subsequent CL.
+  for (auto& observer : battery_charge_status_observers_) {
+    observer->OnBatteryChargeStatusUpdated(battery_charge_status.Clone());
+  }
 }
 
 void SystemDataProvider::BindCrosHealthdProbeServiceIfNeccessary() {

chromeos/components/diagnostics_ui/backend/system_data_provider.h

@@ -12,6 +12,11 @@
 #include "chromeos/dbus/power/power_manager_client.h"
 #include "chromeos/services/cros_healthd/public/mojom/cros_healthd.mojom.h"
 #include "mojo/public/cpp/bindings/remote.h"
+#include "mojo/public/cpp/bindings/remote_set.h"
+
+namespace base {
+class RepeatingTimer;
+}  // namespace base
 
 namespace chromeos {
 namespace diagnostics {
@@ -29,10 +34,16 @@ class SystemDataProvider : public mojom::SystemDataProvider,
   // mojom::SystemDataProvider:
   void GetSystemInfo(GetSystemInfoCallback callback) override;
   void GetBatteryInfo(GetBatteryInfoCallback callback) override;
+  void ObserveBatteryChargeStatus(
+      mojo::PendingRemote<mojom::BatteryChargeStatusObserver> observer)
+      override;
 
   // PowerManagerClient::Observer:
   void PowerChanged(const power_manager::PowerSupplyProperties& proto) override;
 
+  void SetBatteryChargeStatusTimerForTesting(
+      std::unique_ptr<base::RepeatingTimer> timer);
+
  private:
   void BindCrosHealthdProbeServiceIfNeccessary();
 
@@ -57,6 +68,10 @@ class SystemDataProvider : public mojom::SystemDataProvider,
       cros_healthd::mojom::TelemetryInfoPtr info_ptr);
 
   mojo::Remote<cros_healthd::mojom::CrosHealthdProbeService> probe_service_;
+  mojo::RemoteSet<mojom::BatteryChargeStatusObserver>
+      battery_charge_status_observers_;
+
+  std::unique_ptr<base::RepeatingTimer> battery_charge_status_timer_;
 };
 
 }  // namespace diagnostics

chromeos/components/diagnostics_ui/backend/system_data_provider_unittest.cc

@@ -5,16 +5,21 @@
 #include "chromeos/components/diagnostics_ui/backend/system_data_provider.h"
 
 #include <cstdint>
+#include <vector>
 
 #include "base/bind.h"
 #include "base/run_loop.h"
+#include "base/strings/string16.h"
 #include "base/system/sys_info.h"
 #include "base/test/bind_test_util.h"
 #include "base/test/task_environment.h"
 #include "base/time/time.h"
+#include "base/timer/mock_timer.h"
+#include "chromeos/components/diagnostics_ui/backend/power_manager_client_conversions.h"
 #include "chromeos/dbus/cros_healthd/cros_healthd_client.h"
 #include "chromeos/dbus/cros_healthd/fake_cros_healthd_client.h"
 #include "chromeos/dbus/power/fake_power_manager_client.h"
+#include "chromeos/dbus/power_manager/power_supply_properties.pb.h"
 #include "chromeos/services/cros_healthd/public/mojom/cros_healthd.mojom.h"
 #include "chromeos/services/cros_healthd/public/mojom/cros_healthd_probe.mojom.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -131,6 +136,26 @@ cros_healthd::mojom::BatteryInfoPtr CreateCrosHealthdBatteryInfoResponse(
       /*temperature=*/0);
 }
 
+cros_healthd::mojom::BatteryInfoPtr
+CreateCrosHealthdBatteryChargeStatusResponse(double charge_now,
+                                             double current_now) {
+  return CreateCrosHealthdBatteryInfoResponse(
+      /*cycle_count=*/0,
+      /*voltage_now=*/0,
+      /*vendor=*/"",
+      /*serial_number=*/"",
+      /*charge_full_design=*/0,
+      /*charge_full=*/0,
+      /*voltage_min_design=*/0,
+      /*model_name=*/"",
+      /*charge_now=*/charge_now,
+      /*current_now=*/current_now,
+      /*technology=*/"",
+      /*status=*/"",
+      /*manufacture_date=*/base::nullopt,
+      /*temperature=*/0);
+}
+
 void SetCrosHealthdBatteryInfoResponse(const std::string& vendor,
                                        double charge_full_design) {
   cros_healthd::mojom::BatteryInfoPtr battery_info =
@@ -140,8 +165,121 @@ void SetCrosHealthdBatteryInfoResponse(const std::string& vendor,
                                 /*memory_info=*/nullptr);
 }
 
+void SetCrosHealthdBatteryChargeStatusResponse(double charge_now,
+                                               double current_now) {
+  cros_healthd::mojom::BatteryInfoPtr battery_info =
+      CreateCrosHealthdBatteryChargeStatusResponse(charge_now, current_now);
+  SetProbeTelemetryInfoResponse(std::move(battery_info), /*cpu_info=*/nullptr,
+                                /*memory_info=*/nullptr,
+                                /*memory_info=*/nullptr);
+}
+
+bool AreValidPowerTimes(int64_t time_to_full, int64_t time_to_empty) {
+  // Exactly one of |time_to_full| or |time_to_empty| must be zero. The other
+  // can be a positive integer to represent the time to charge/discharge or -1
+  // to represent that the time is being calculated.
+  return (time_to_empty == 0 && (time_to_full > 0 || time_to_full == -1)) ||
+         (time_to_full == 0 && (time_to_empty > 0 || time_to_empty == -1));
+}
+
+power_manager::PowerSupplyProperties ConstructPowerSupplyProperties(
+    power_manager::PowerSupplyProperties::ExternalPower power_source,
+    power_manager::PowerSupplyProperties::BatteryState battery_state,
+    bool is_calculating_battery_time,
+    int64_t time_to_full,
+    int64_t time_to_empty) {
+  power_manager::PowerSupplyProperties props;
+  props.set_external_power(power_source);
+  props.set_battery_state(battery_state);
+
+  if (battery_state ==
+      power_manager::PowerSupplyProperties_BatteryState_NOT_PRESENT) {
+    // Leave |time_to_full| and |time_to_empty| unset.
+    return props;
+  }
+
+  DCHECK(AreValidPowerTimes(time_to_full, time_to_empty));
+
+  props.set_is_calculating_battery_time(is_calculating_battery_time);
+  props.set_battery_time_to_full_sec(time_to_full);
+  props.set_battery_time_to_empty_sec(time_to_empty);
+
+  return props;
+}
+
+// Sets the PowerSupplyProperties on FakePowerManagerClient. Calling this
+// method immediately notifies PowerManagerClient observers. One of
+// |time_to_full| or |time_to_empty| must be either -1 or a positive number. The
+// other must be 0. If |battery_state| is NOT_PRESENT, both |time_to_full| and
+// |time_to_empty| will be left unset.
+void SetPowerManagerProperties(
+    power_manager::PowerSupplyProperties::ExternalPower power_source,
+    power_manager::PowerSupplyProperties::BatteryState battery_state,
+    bool is_calculating_battery_time,
+    int64_t time_to_full,
+    int64_t time_to_empty) {
+  power_manager::PowerSupplyProperties props = ConstructPowerSupplyProperties(
+      power_source, battery_state, is_calculating_battery_time, time_to_full,
+      time_to_empty);
+  FakePowerManagerClient::Get()->UpdatePowerProperties(props);
+}
+
+void VerifyChargeStatusResult(
+    const mojom::BatteryChargeStatusPtr& update,
+    double charge_now,
+    double current_now,
+    power_manager::PowerSupplyProperties::ExternalPower power_source,
+    power_manager::PowerSupplyProperties::BatteryState battery_state,
+    bool is_calculating_battery_time,
+    int64_t time_to_full,
+    int64_t time_to_empty) {
+  const uint32_t expected_charge_now_milliamp_hours = charge_now * 1000;
+  const int32_t expected_current_now_milliamps = current_now * 1000;
+  mojom::ExternalPowerSource expected_power_source =
+      ConvertPowerSourceFromProto(power_source);
+  mojom::BatteryState expected_battery_state =
+      ConvertBatteryStateFromProto(battery_state);
+
+  EXPECT_EQ(expected_charge_now_milliamp_hours,
+            update->charge_now_milliamp_hours);
+  EXPECT_EQ(expected_current_now_milliamps, update->current_now_milliamps);
+  EXPECT_EQ(expected_power_source, update->power_adapter_status);
+  EXPECT_EQ(expected_battery_state, update->battery_state);
+
+  if (expected_battery_state == mojom::BatteryState::kFull) {
+    EXPECT_EQ(base::string16(), update->power_time);
+    return;
+  }
+
+  DCHECK(AreValidPowerTimes(time_to_full, time_to_empty));
+
+  const power_manager::PowerSupplyProperties props =
+      ConstructPowerSupplyProperties(power_source, battery_state,
+                                     is_calculating_battery_time, time_to_full,
+                                     time_to_empty);
+  base::string16 expected_power_time =
+      ConstructPowerTime(expected_battery_state, props);
+
+  EXPECT_EQ(expected_power_time, update->power_time);
+}
+
 }  // namespace
 
+struct FakeBatteryChargeStatusObserver
+    : public mojom::BatteryChargeStatusObserver {
+  // mojom::BatteryChargeStatusObserver
+  void OnBatteryChargeStatusUpdated(
+      mojom::BatteryChargeStatusPtr status_ptr) override {
+    updates.emplace_back(std::move(status_ptr));
+  }
+
+  // Tracks calls to OnBatteryChargeStatusUpdated. Each call adds an element to
+  // the vector.
+  std::vector<mojom::BatteryChargeStatusPtr> updates;
+
+  mojo::Receiver<mojom::BatteryChargeStatusObserver> receiver{this};
+};
+
 class SystemDataProviderTest : public testing::Test {
  public:
   SystemDataProviderTest() {
@@ -247,5 +385,66 @@ TEST_F(SystemDataProviderTest, BatteryInfo) {
   run_loop.Run();
 }
 
+TEST_F(SystemDataProviderTest, BatteryChargeStatusObserver) {
+  // Setup Timer
+  auto timer = std::make_unique<base::MockRepeatingTimer>();
+  auto* timer_ptr = timer.get();
+  system_data_provider_->SetBatteryChargeStatusTimerForTesting(
+      std::move(timer));
+
+  // Setup initial data
+  const double charge_now_amp_hours = 20;
+  const double current_now_amps = 2;
+  const auto power_source =
+      power_manager::PowerSupplyProperties_ExternalPower_AC;
+  const auto battery_state =
+      power_manager::PowerSupplyProperties_BatteryState_CHARGING;
+  const bool is_calculating_battery_time = false;
+  const int64_t time_to_full_secs = 1000;
+  const int64_t time_to_empty_secs = 0;
+
+  SetCrosHealthdBatteryChargeStatusResponse(charge_now_amp_hours,
+                                            current_now_amps);
+  SetPowerManagerProperties(power_source, battery_state,
+                            is_calculating_battery_time, time_to_full_secs,
+                            time_to_empty_secs);
+
+  // Registering as an observer should trigger one update.
+  FakeBatteryChargeStatusObserver charge_status_observer;
+  system_data_provider_->ObserveBatteryChargeStatus(
+      charge_status_observer.receiver.BindNewPipeAndPassRemote());
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_EQ(1u, charge_status_observer.updates.size());
+  VerifyChargeStatusResult(charge_status_observer.updates[0],
+                           charge_now_amp_hours, current_now_amps, power_source,
+                           battery_state, is_calculating_battery_time,
+                           time_to_full_secs, time_to_empty_secs);
+
+  // Firing the timer should trigger another.
+  timer_ptr->Fire();
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_EQ(2u, charge_status_observer.updates.size());
+  VerifyChargeStatusResult(charge_status_observer.updates[0],
+                           charge_now_amp_hours, current_now_amps, power_source,
+                           battery_state, is_calculating_battery_time,
+                           time_to_full_secs, time_to_empty_secs);
+
+  // Updating the PowerManagerClient Properties should trigger yet another.
+  const int64_t new_time_to_full_secs = time_to_full_secs - 10;
+  SetPowerManagerProperties(
+      power_manager::PowerSupplyProperties_ExternalPower_AC,
+      power_manager::PowerSupplyProperties_BatteryState_CHARGING,
+      is_calculating_battery_time, new_time_to_full_secs, time_to_empty_secs);
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_EQ(3u, charge_status_observer.updates.size());
+  VerifyChargeStatusResult(charge_status_observer.updates[0],
+                           charge_now_amp_hours, current_now_amps, power_source,
+                           battery_state, is_calculating_battery_time,
+                           new_time_to_full_secs, time_to_empty_secs);
+}
+
 }  // namespace diagnostics
 }  // namespace chromeos

chromeos/components/diagnostics_ui/mojom/system_data_provider.mojom

@@ -55,6 +55,16 @@ struct BatteryChargeStatus {
   ExternalPowerSource power_adapter_status;
 };
 
+// Implemented by clients that wish to be updated periodically about changes to
+// the battery charge status.
+interface BatteryChargeStatusObserver {
+  // OnBatteryChargeStatus calls can be triggered by any of 3 conditions:
+  // 1) A BatteryChargeStatusObserver is registered with SystemDataProvider
+  // 2) A PowerManagerClient 'PowerChanged' event is received
+  // 3) A periodic update is sent by SystemDataProvider
+  OnBatteryChargeStatusUpdated(BatteryChargeStatus battery_charge_status);
+};
+
 // Provides telemetric information about the system. This API is exposed to the
 // Diagnostics SWA.
 interface SystemDataProvider {
@@ -66,4 +76,8 @@ interface SystemDataProvider {
   // information that does not change during the lifetime fo the service.
   // If the device does not have a battery, returns an empty BatteryInfo.
   GetBatteryInfo() => (BatteryInfo battery_info);
+
+  // Registers an observer of BatteryChargeStatus information.
+  ObserveBatteryChargeStatus(
+      pending_remote<BatteryChargeStatusObserver> observer);
 };